feat: nonce handling with signer #3196
Conversation
cmwaters
requested review from
evan-forbes,
rootulp,
staheri14 and
ninabarbakadze
as code owners
WalkthroughWalkthroughThe recent changes focus on refining transaction handling and error parsing, specifically targeting nonce mismatch and gas price issues. Enhancements include simplifying transaction creation and submission, improving sequence number management, and introducing new testing for concurrent operations. These updates aim to address user experience problems related to transaction ordering and nonce management, ensuring efficient processing and prioritization of transactions in environments where block production may outpace finalization. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
…app into cal/nonce-handling
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // FIXME: the signer is currently incapable of detecting an appversion | ||
| // change and could produce incorrect PFBs if it the network is at an | ||
| // appVersion that the signer does not support |
There was a problem hiding this comment.
The FIXME comment highlights a potential issue with the Signer not detecting appVersion changes. This could lead to producing incorrect PayForBlob transactions if the network's appVersion is not supported by the Signer.
Consider implementing a mechanism to check and update the appVersion dynamically or at least document a process for manually updating it to ensure the Signer remains compatible with the network.
| tx, err := s.CreateTx(msgs, opts...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| resp, err := s.BroadcastTx(ctx, txBytes) | ||
| resp, err := s.BroadcastTx(ctx, tx) |
There was a problem hiding this comment.
When submitting a transaction using SubmitTx, the error handling for the transaction broadcast is minimal. It only checks if resp.Code is not 0 but does not handle specific error codes or retry logic.
Enhance error handling by implementing specific checks for common error codes related to nonce mismatches or other transaction failures. Consider adding retry logic for recoverable errors to improve the robustness of transaction submission.
pkg/user/signer.go
Outdated
| resp, err := s.broadcastPayForBlob(ctx, blobs, opts...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| resp, err := s.BroadcastTx(ctx, txBytes) | ||
| return s.ConfirmTx(ctx, resp.TxHash) |
There was a problem hiding this comment.
Similar to the SubmitTx method, SubmitPayForBlob also has minimal error handling for the transaction broadcast. It directly returns the response from broadcastPayForBlob without additional checks or handling.
Apply the same enhancements recommended for SubmitTx to SubmitPayForBlob for consistent and robust error handling across transaction submission methods.
cmwaters
added
the
backport:v1.x
Closes: #1910 This covers most cases by serializing the actual broadcasts to the consensus node and enabling resubmissions in the case that there is a sequence mismatch. This covers most fail cases with the possible exception of proposal nodes receiving the transactions in the reverse order to the initial nodes that the user broadcasted to There are also some interesting side affects that need to be handled when an existing accepted transaction is later kicked out of the mempool via CheckTx but overall I think this is a huge improvement for the UX of users (cherry picked from commit deefb54) # Conflicts: # Makefile # app/errors/insufficient_gas_price_test.go # app/errors/nonce_mismatch_test.go # app/test/big_blob_test.go # app/test/priority_test.go # go.work.sum # pkg/user/signer.go # pkg/user/signer_test.go # test/util/blobfactory/payforblob_factory.go # test/util/blobfactory/test_util.go # test/util/direct_tx_gen.go # x/blob/types/blob_tx_test.go
Closes: celestiaorg#1910 This covers most cases by serializing the actual broadcasts to the consensus node and enabling resubmissions in the case that there is a sequence mismatch. This covers most fail cases with the possible exception of proposal nodes receiving the transactions in the reverse order to the initial nodes that the user broadcasted to There are also some interesting side affects that need to be handled when an existing accepted transaction is later kicked out of the mempool via CheckTx but overall I think this is a huge improvement for the UX of users
Closes: #1910 This covers most cases by serializing the actual broadcasts to the consensus node and enabling resubmissions in the case that there is a sequence mismatch. This covers most fail cases with the possible exception of proposal nodes receiving the transactions in the reverse order to the initial nodes that the user broadcasted to There are also some interesting side affects that need to be handled when an existing accepted transaction is later kicked out of the mempool via CheckTx but overall I think this is a huge improvement for the UX of users<hr>This is an automatic backport of pull request #3196 done by [Mergify](https://mergify.com). Co-authored-by: Callum Waters <[email protected]>
Closes: #1910
This covers most cases by serializing the actual broadcasts to the consensus node and enabling resubmissions in the case that there is a sequence mismatch.
This covers most fail cases with the possible exception of proposal nodes receiving the transactions in the reverse order to the initial nodes that the user broadcasted to
There are also some interesting side affects that need to be handled when an existing accepted transaction is later kicked out of the mempool via CheckTx but overall I think this is a huge improvement for the UX of users