perf(handler): add retry logic to broadcasting to cometbft mempool #1242
Conversation
WalkthroughThe changes introduce a retry mechanism for failed transactions and enhance nonce handling in JSON RPC tests. The retry mechanism improves transaction broadcasting reliability, while the nonce handling changes ensure accurate transaction processing. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (2)
- cosmos/txpool/handler.go (9 hunks)
- e2e/localnet/network/fixture_test.go (1 hunks)
Additional comments (Suppressed): 12
e2e/localnet/network/fixture_test.go (3)
192-194: The result of
contract.ConsumeGasis now assigned totxwhich is used later to wait for the transaction receipt. This is a good practice as it allows for better tracking and handling of transactions.208-209: The
ExpectSuccessReceiptfunction is used to wait for the transaction receipt and verify success. This is a good addition as it ensures that the transaction has been processed successfully before proceeding with the test.211-216: The test now checks that
NonceAtandPendingNonceare equal after the transaction has been included in a block. This is a good practice as it verifies that the nonce values are updated correctly after a transaction.cosmos/txpool/handler.go (9)
38-44: The introduction of
maxRetriesandretryDelayconstants is a good practice for managing the retry mechanism. However, ensure that these values are optimal for the system's performance and do not lead to unnecessary retries or delays.72-76: The
failedTxstruct is a good addition to handle failed transactions. It encapsulates the transaction and the number of retries, which is useful for the retry mechanism.94-94: The
failedTxschannel is used to queue failed transactions for retry. This is a good approach to handle failed transactions and improve the reliability of transaction broadcasting.108-108: The
failedTxschannel is initialized with the same size as thetxsChchannel. Ensure that this size is optimal for the system's performance and does not lead to unnecessary memory usage.118-119: The
mainLoopandfailedLoopgoroutines are started concurrently. This is a good approach to handle new transactions and retry failed transactions simultaneously.130-131: Two stop signals are sent to the
stopChchannel to stop both themainLoopandfailedLoopgoroutines. This is a good approach to ensure that both goroutines are stopped when the handler is stopped.156-173: The
failedLoopfunction is a good addition to handle the retry of failed transactions. It listens for failed transactions from thefailedTxschannel and retries them until the maximum number of retries is reached. The use of a delay between retries is a good practice to avoid overwhelming the system with retries.203-203: The
broadcastTransactionfunction is now called with an initial retry count of 0 for each transaction. This is a good approach to track the number of retries for each transaction.208-227: The
broadcastTransactionfunction now includes aretriesparameter and adds failed transactions to thefailedTxschannel for retry. This is a good approach to handle failed transactions and improve the reliability of transaction broadcasting. However, ensure that the retry mechanism does not lead to unnecessary retries or delays.
|
CodeRabbit review skipped By default, CodeRabbit only reviews PRs on the default branch. If you wish to have PRs reviewed on additional branches, you can configure that under the repository settings in the UI. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1242 +/- ##
==========================================
+ Coverage 49.23% 49.24% +0.01%
==========================================
Files 77 77
Lines 4611 4630 +19
==========================================
+ Hits 2270 2280 +10
- Misses 2178 2187 +9
Partials 163 163
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- cosmos/txpool/handler.go (9 hunks)
Additional comments (Suppressed): 10
cosmos/txpool/handler.go (10)
28-29: The import of the
timepackage is new and is used for the retry delay in the retry mechanism. Ensure that this package is compatible with the rest of the codebase.40-44: The constants
maxRetriesandretryDelayhave been added for the retry mechanism. Ensure that these values are appropriate for the application's requirements.72-76: The
failedTxstruct has been introduced to represent transactions that failed to broadcast. This is a good approach to encapsulate the transaction and its retry count.93-95: The
failedTxschannel has been added to thehandlerstruct to store transactions that failed to broadcast. This is a good approach to queue failed transactions for retry.108-108: The
failedTxschannel is initialized with the same size as thetxsChchannel. Ensure that this size is appropriate for the expected number of failed transactions.118-119: Two goroutines,
mainLoopandfailedLoop, are started in theStartmethod. This is a good approach to handle new transactions and retry failed transactions concurrently.129-131: Two stop signals are sent to the
stopChchannel to stop both themainLoopandfailedLoopgoroutines. This is a good approach to ensure that both goroutines stop when the handler is stopped.156-173: The
failedLoopfunction has been introduced to periodically retry failed transactions. This is a good approach to enhance the reliability of transaction broadcasting.203-203: The
broadcastTransactionfunction is now called with aretriesparameter. This is a good approach to track the number of retry attempts for each transaction.208-227: The
broadcastTransactionfunction has been modified to handle failed transactions. If a transaction fails to broadcast, it is added to thefailedTxschannel for retry. This is a good approach to handle failed transactions.
|
might upgrade this to use a heap + exponential backoff. |
|
makes sense so far. can also get more sophisticated with retry priority |
|
yup, was a 2am first pass for sure. |
|
in practice, the retries will almost never be useful. If the retries are even required something has likely gone really wrong with comet. |
|
I am still pro gossip via geth devp2p |
|
yeah broadly agree, seems like overall more performant/battle-tested |
Summary by CodeRabbit