refactor(handler): Improve transaction broadcast retry logic

[itsdevbear]

Summary by CodeRabbit

• New Feature: Implemented a lock-free queue system for handling failed transactions, improving the efficiency and reliability of transaction processing.
• Refactor: Streamlined transaction broadcasting process by introducing a queue system, enhancing the system's ability to handle high volumes of transactions.
• New Feature: Added a function to check if the transaction handler is running, providing better control and monitoring capabilities.
• Refactor: Renamed and refactored several methods for clarity and improved functionality, making the system easier to understand and maintain.
• New Feature: Introduced a delay in the block building process to ensure optimal payload time, enhancing the overall performance of the system.

[coderabbitai]

Walkthrough

The changes introduce a lock-free queue to handle failed transactions, improving the efficiency and concurrency of the transaction handling process. The miner's block building function now includes a sleep period to ensure a minimum payload time. The transaction broadcasting process has been refactored to enqueue transactions for later broadcasting, enhancing the system's overall performance.

Changes

File	Summary
cosmos/miner/miner.go	Introduced a sleep period in the buildBlock function to ensure a minimum payload time.
cosmos/txpool/handler.go	Replaced the failedTx struct with a lock-free queue for handling failed transactions. Refactored transaction broadcasting to enqueue transactions for later broadcasting.
lib/queue/queue.go	Implemented a new lock-free FIFO queue data structure for efficient handling of failed transactions.

🐇💻

In the land of code, where logic is king,

A rabbit hopped forth, changes to bring.

With a queue that's lock-free, and a miner's rest,

The system now runs at its very best.

Transactions are queued, not rushed in a spree,

Celebrate the changes, as swift as can be! 🎉🥕

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• If you reply to a review comment from CodeRabbit, the bot will automatically respond.
• To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
• Note: Review comments are made on code diffs or files, not on the PR overview.
• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

[coderabbitai]

Review Status

Actionable comments generated: 6

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 9c4e32c and aa2f696.

Files selected for processing (3)

• cosmos/miner/miner.go (3 hunks)
• cosmos/txpool/handler.go (6 hunks)
• lib/queue/queue.go (1 hunks)

Additional comments: 15

cosmos/miner/miner.go (3)

• 26-26: The time package is imported to use the time.Sleep function and time.Millisecond constant in the buildBlock function.

• 43-45: A constant minPayloadtime is defined to set a minimum time for the payload to be built. This is used in the buildBlock function to pause execution and allow the payload to be built.

• 91-94: A sleep of minPayloadtime (200 milliseconds) is added before resolving the envelope in the buildBlock function. This is to ensure that the payload has enough time to be built before it is resolved.

cosmos/txpool/handler.go (10)

• 37-45: The import of the new queuelib package and the definition of emptyQueueBackoff constant are introduced. The maxRetries constant is removed, indicating a change in the retry logic for failed transactions. Ensure that the removal of maxRetries does not affect other parts of the codebase that might be using it.

• 71-76: The failedTx struct is removed, indicating a change in the way failed transactions are handled. Ensure that this change does not affect other parts of the codebase that might be using the failedTx struct.

• 87-90: The txQueue field is added to the handler struct, replacing the failedTxs channel. This change is part of the refactor to use a lock-free queue for storing failed transactions, improving thread safety.

• 103-105: The txQueue field is initialized in the newHandler function using the NewLockFreeQueue function from the queuelib package. This change is part of the refactor to use a lock-free queue for storing failed transactions.

• 114-121: The mainLoop and failedLoop methods are replaced with queueLoop and broadcastLoop methods respectively. This change reflects the new roles of these methods in the refactored logic.

• 132-144: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [135-155]

The queueLoop method is introduced to handle the subscription to the txpool and enqueue transactions. This method replaces the mainLoop method in the old code.

• 157-184: The broadcastLoop and broadcastTx methods are introduced. The broadcastLoop method continuously broadcasts transactions from the queue, replacing the failedLoop method in the old code. The broadcastTx method dequeues the next transaction off the queue and attempts to broadcast it, handling specific errors related to the mempool being full or the transaction already existing in the mempool.

• 186-196: The enqueueTxBytes and dequeueTxBytes methods are introduced to add and remove transactions from the queue respectively. These methods replace the use of the failedTxs channel in the old code for storing failed transactions.

• 198-215: The stop method is updated to close the stopCh channel twice, ensuring that both the queueLoop and broadcastLoop methods stop. Ensure that this change does not cause any issues with the stopping of the handler.

• 217-244: The queueTransactions and broadcastTransaction methods are updated to enqueue transactions and broadcast a transaction from the queue respectively. These methods replace the broadcastTransactions and broadcastTransaction methods in the old code, reflecting the new logic for handling and broadcasting transactions.

lib/queue/queue.go (2)

• 108-111: The IsEmpty method is correct and provides a way to check if the queue is empty. However, it should be noted that this method may not always provide accurate results in a multi-threaded environment due to the nature of lock-free data structures. If accuracy is required, consider using a different data structure or synchronization method.

• 85-106: ```diff

• var t T
• for {

•   head := q.head.Load()
  

•   tail := q.tail.Load()
  

•   next := head.next.Load()
  

•   if head == q.head.Load() {
  

•   	if head == tail {
  

•   		if next == nil {
  

•   			return t
  

•   		}
  

•   		q.tail.CompareAndSwap(tail, next)
  

•   	} else {
  

•   		v := next.value
  

•   		if q.head.CompareAndSwap(head, next) {
  

•   			return v
  

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between aa2f696 and 23a5a72.

Files selected for processing (1)

• cosmos/miner/miner.go (1 hunks)

Files skipped from review due to trivial changes (1)

• cosmos/miner/miner.go

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 23a5a72 and f259b6f.

Files selected for processing (1)

• cosmos/txpool/handler.go (6 hunks)

Additional comments: 12

cosmos/txpool/handler.go (12)

• 37-45: The import of the new queuelib package and the introduction of the emptyQueueBackoff constant indicate the use of a new queue-based approach for handling transactions. Ensure that the emptyQueueBackoff delay is appropriate for the system's performance requirements.

• 71-76: The failedTx struct has been removed, indicating a change in how failed transactions are handled. This change is likely related to the introduction of the txQueue.

• 87-90: The failedTxs channel has been replaced with a txQueue of type LockFreeQueue[[]byte]. This change suggests that failed transactions will now be handled using a lock-free queue, which should improve concurrency and performance.

• 114-120: The mainLoop and failedLoop methods have been replaced with queueLoop and broadcastLoop methods respectively. This change suggests a shift from the previous approach of handling failed transactions to a new approach based on a queue.

• 132-144: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [135-155]

The queueLoop method subscribes to the transaction pool and queues transactions for broadcasting. This method replaces the mainLoop method, which directly broadcasted transactions. This change is part of the shift to a queue-based approach for handling transactions.

• 157-171: The broadcastLoop method continuously broadcasts transactions from the queue. This method replaces the failedLoop method, which attempted to re-broadcast failed transactions. This change is part of the shift to a queue-based approach for handling transactions.

• 173-184: The broadcastTx method dequeues the next transaction from the queue and attempts to broadcast it. If the broadcast fails due to a full mempool, the transaction is re-enqueued. This method replaces the broadcastTransaction method, which propagated an individual transaction to the CometBFT mempool and handled failed broadcasts by adding the transaction to the failedTxs channel.

• 186-190: The enqueueTxBytes method adds a transaction to the queue. This method is part of the new queue-based approach for handling transactions.

• 192-196: The dequeueTxBytes method removes and returns the next transaction from the queue. This method is part of the new queue-based approach for handling transactions.

• 198-215: The stop method has been updated to close the stopCh channel twice. This change is likely related to the introduction of the broadcastLoop method, which also listens for stop signals. Ensure that this change does not introduce any issues with channel closing and that all goroutines that could potentially write to this channel have exited before it is closed.

• 217-228: The queueTransactions method queues a batch of transactions for broadcasting. This method replaces the broadcastTransactions method, which propagated a batch of transactions to the CometBFT mempool. This change is part of the shift to a queue-based approach for handling transactions.

• 230-244: The broadcastTransaction method has been updated to accept a byte slice representing the transaction instead of a coretypes.Transaction object. This change is likely related to the shift to a queue-based approach for handling transactions, as byte slices can be more easily stored in a queue.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between f259b6f and b5a4141.

Files selected for processing (2)

• cosmos/txpool/handler.go (6 hunks)
• cosmos/txpool/handler_test.go (2 hunks)

Files skipped from review due to trivial changes (1)

• cosmos/txpool/handler_test.go

Additional comments: 11

cosmos/txpool/handler.go (11)

• 37-45: The maxRetries constant has been removed and two new constants emptyQueueBackoff and retryDelay have been introduced. Ensure that the new constants are used correctly throughout the codebase.

• 71-76: The failedTx struct has been removed. This indicates a shift in the handling of failed transactions, which are now likely to be managed by the new txQueue.

• 87-90: The failedTxs channel has been replaced with a lock-free queue txQueue for storing failed transactions. This change should improve performance by avoiding potential blocking issues associated with channels.

• 114-121: The mainLoop and failedLoop methods have been renamed to queueLoop and broadcastLoop respectively. This change should not affect functionality but it's important to ensure that these methods are called correctly throughout the codebase.

• 132-144: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [135-155]

The mainLoop method has been renamed to queueLoop and the logic remains the same. It handles the subscription to the transaction pool and broadcasts transactions.

• 157-171: The failedLoop method has been renamed to broadcastLoop. The logic has been significantly changed. Instead of reading from the failedTxs channel and retrying failed transactions, it now dequeues transactions from txQueue and broadcasts them. This change should improve performance by avoiding potential blocking issues associated with channels.

• 174-184: The broadcastTx method now accepts transaction bytes instead of a coretypes.Transaction object. This change simplifies the method by removing the need to serialize the transaction within the method. However, it's important to ensure that transactions are correctly serialized before they are passed to this method.

• 186-196: The enqueueTxBytes and dequeueTxBytes methods have been added to handle adding and removing transactions from txQueue. These methods should improve code readability and maintainability by encapsulating the queue operations.

• 198-214: The stop method remains the same except for the removal of the line close(h.failedTxs). This is expected since failedTxs has been replaced with txQueue.

• 216-227: The broadcastTransactions method has been renamed to queueTransactions and the logic has been changed to enqueue transactions to txQueue instead of broadcasting them directly. This change should improve performance by allowing transactions to be broadcasted in a separate goroutine.

• 229-243: The broadcastTransaction method now accepts transaction bytes instead of a coretypes.Transaction object. This change simplifies the method by removing the need to serialize the transaction within the method. However, it's important to ensure that transactions are correctly serialized before they are passed to this method.

[codecov]

Codecov Report

Merging #1249 (e1f0383) into main (febc5a6) will decrease coverage by 0.14%.
Report is 1 commits behind head on main.
The diff coverage is 38.09%.

❗ Current head e1f0383 differs from pull request most recent head 6f24208. Consider uploading reports for the commit 6f24208 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1249      +/-   ##
==========================================
- Coverage   49.22%   49.08%   -0.14%     
==========================================
  Files          77       77              
  Lines        4642     4645       +3     
==========================================
- Hits         2285     2280       -5     
- Misses       2195     2204       +9     
+ Partials      162      161       -1     

Files	Coverage Δ
cosmos/txpool/handler.go	57.42% <38.09%> (-6.86%)	⬇️

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between b5a4141 and ef472d5.

Files selected for processing (1)

• cosmos/txpool/handler.go (6 hunks)

Additional comments: 12

cosmos/txpool/handler.go (12)

• 37-45: The maxRetries constant has been removed and two new constants emptyQueueBackoff and retryDelay have been introduced. Ensure that the removal of maxRetries does not affect the retry logic and that the new constants are used appropriately.

• 71-76: The failedTx struct has been removed. Ensure that the functionality provided by this struct is adequately replaced in the new code.

• 87-90: A new field txQueue of type *queuelib.LockFreeQueue[[]byte] has been added to the handler struct. This queue is used to handle transactions that need to be broadcasted. Ensure that this queue is used correctly throughout the code.

• 114-121: The Start method now starts queueLoop and broadcastLoop instead of mainLoop and failedLoop. Ensure that these new methods provide the same functionality as the old ones.

• 132-144: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [135-155]

The queueLoop method replaces the mainLoop method. It handles the subscription to the txpool and queues transactions for broadcasting. Ensure that this method correctly handles the subscription and queuing of transactions.

• 157-171: The broadcastLoop method replaces the failedLoop method. It continuously broadcasts transactions from the queue. Ensure that this method correctly handles the broadcasting of transactions and that the retry logic is implemented correctly.

• 173-184: The broadcastTx method dequeues the next transaction from the queue and attempts to broadcast it. If the mempool is full, the transaction is re-enqueued. If the transaction already exists in the CometBFT layer, nothing is done. If an error occurs, it is logged. Ensure that this method correctly handles these cases.

• 186-190: The enqueueTxBytes method adds a transaction to the queue. Ensure that this method correctly enqueues transactions.

• 192-196: The dequeueTxBytes method removes and returns the next transaction from the queue. Ensure that this method correctly dequeues transactions.

• 198-214: The stop method stops the handler. It marks the handler as not running, logs any errors, unsubscribes from the txsSub, and closes the channels. Ensure that this method correctly stops the handler.

• 216-227: The queueTransactions method replaces the broadcastTransactions method. It queues a batch of transactions for broadcasting instead of broadcasting them directly. Ensure that this method correctly queues transactions.

• 229-243: The broadcastTransaction method broadcasts a transaction to the CometBFT mempool. If the response is nil or the code is 0, it returns nil. If an error occurs, it returns the error. Ensure that this method correctly broadcasts transactions and handles errors.