ignore redundant tx & check pending tx even if it timed out #78
Conversation
WalkthroughThis pull request refactors the pending transaction handling in the broadcaster component. In Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant LPT as loadPendingTxs
participant Ticker as Ticker
participant TxQuery as Tx Query
participant Logger as Logger
Caller->>LPT: Invoke loadPendingTxs
loop Polling Loop
LPT->>Ticker: Wait for next tick
alt Context Cancelled
Ticker-->>LPT: Context cancellation
LPT-->>Caller: Return early
else On Tick
LPT->>TxQuery: Query pending transaction status
alt Transaction Included
TxQuery-->>LPT: Success result
LPT->>LPT: Remove transaction from pending list
else Transaction Error
TxQuery-->>LPT: Error response
alt Error matches txNotFoundRegex and timeout exceeded
LPT-->>Caller: Break loop and return
else
LPT->>Logger: Log warning
end
end
end
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
node/broadcaster/broadcaster.go (2)
153-154: Ticker-based polling is a good approach.
This is a standard pattern to repeatedly poll for tasks. Ensure your polling interval (ctx.PollingInterval()) is configured to balance performance against timeliness.
167-170: Hex decode error handling.
Currently, an invalid hex transaction hash aborts further processing. If partial success is desired for remaining items, consider skipping only the problematic transaction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
node/broadcaster/broadcaster.go(3 hunks)node/broadcaster/tx.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: opinitd
🔇 Additional comments (7)
node/broadcaster/broadcaster.go (6)
6-6: Import for regex usage looks fine.
The new "regexp" import is necessary for the pattern matching logic introduced. No issues here.
26-26: Validate error message stability.
While matching on a fixed string is acceptable, consider whether the error message might change in future CometBFT updates. If so, you may need a more robust approach or a fallback check for a specific error code.
156-159: Early return might skip final cleanup.
Exiting immediately if pendingTxs is empty prevents subsequent operations (like deleting or archiving leftover items) from running. Verify that this behavior is intended and consistent with your design.
161-165: Robust select for context cancellation.
Using a select statement with context and poll ticker is clean and allows graceful shutdown.
172-182: Handling successful transaction inclusion.
Removing the entry from both the database and the local slice ensures no retry for included transactions. This logic flow is sound.
188-193: Timeout-based break is coherent.
Once the timeout is passed, you break the loop and subsequently remove pending transactions in bulk. This matches your intended flow for "not found" transactions.node/broadcaster/tx.go (1)
25-25: Adding redundant transaction error to ignore list.
This addition helps gracefully skip errors that indicate duplicate or already-processed transactions, preventing unnecessary re-submissions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
node/broadcaster/broadcaster.go (1)
149-150: Consider implementing a backoff strategy for polling.Using a fixed polling interval might not be optimal. Consider implementing an exponential backoff strategy to reduce load on the node when transactions take longer to process.
- pollingTimer := time.NewTicker(ctx.PollingInterval()) - defer pollingTimer.Stop() + backoff := ctx.PollingInterval() + maxBackoff := 1 * time.Minute + backoffFactor := 2 + + nextPoll := time.NewTimer(backoff) + defer nextPoll.Stop()Then in the select statement:
- case <-pollingTimer.C: + case <-nextPoll.C: + // Increase backoff for next iteration + if backoff < maxBackoff { + backoff = time.Duration(float64(backoff) * float64(backoffFactor)) + if backoff > maxBackoff { + backoff = maxBackoff + } + } + nextPoll.Reset(backoff)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
node/broadcaster/broadcaster.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: opinitd
🔇 Additional comments (3)
node/broadcaster/broadcaster.go (3)
26-26: LGTM! Well-structured regex pattern.The regex pattern is correctly implemented to capture transaction hashes and is efficiently compiled at package level.
154-187: Failed transaction might loop indefinitely.When a transaction fails (code != 0), it's added to reProcessingTxs without any limit on retries. Consider implementing a maximum retry count to prevent infinite loops.
194-200: 🛠️ Refactor suggestionAdd retry limit for reprocessing transactions.
Currently, failed transactions are reprocessed without any limit. Consider adding a retry counter to each PendingTxInfo and a maximum retry limit to prevent resource exhaustion.
type PendingTxInfo struct { TxHash string Sender string Tx []byte Timestamp int64 Save bool + RetryCount int } // In loadPendingTxs - if len(reProcessingTxs) != 0 { + maxRetries := 3 // Or configure via BroadcasterConfig + retriableProcessingTxs := make([]btypes.PendingTxInfo, 0) + for _, tx := range reProcessingTxs { + if tx.RetryCount < maxRetries { + tx.RetryCount++ + retriableProcessingTxs = append(retriableProcessingTxs, tx) + } else { + ctx.Logger().Warn("transaction exceeded max retries", + zap.String("hash", tx.TxHash), + zap.Int("retries", tx.RetryCount)) + } + } + if len(retriableProcessingTxs) != 0 {Likely invalid or redundant comment.
Summary by CodeRabbit