Fix SubmitEthereumTxConfirmation bug #558
Conversation
WalkthroughThe changes focus on improving the handling of outgoing transaction signatures within the Gravity module, specifically targeting the issue where validators were unable to retroactively sign completed transactions. This enhancement allows for the inclusion of batches and contract call transactions without Ethereum signatures in the list of unconfirmed transactions, thereby enabling validators to sign these transactions post-completion. Additionally, the update ensures that the 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| k.IterateCompletedOutgoingTxsByType(ctx, types.ContractCallTxPrefixByte, func(_ []byte, cotx types.OutgoingTx) bool { | ||
| sig := k.getEthereumSignature(ctx, cotx.GetStoreIndex(), val) | ||
| if len(sig) == 0 { | ||
| call, ok := cotx.(*types.ContractCallTx) | ||
| if !ok { | ||
| panic(sdkerrors.Wrapf(types.ErrInvalid, "couldn't cast to contract call for completed tx %s", cotx)) | ||
| } | ||
| unconfirmed = append(unconfirmed, call) | ||
| } | ||
| return false | ||
| }) |
There was a problem hiding this comment.
The addition of iterating over completed outgoing transactions to include them in the list of unconfirmed contract call transactions is a crucial fix for the issue described. However, using panic for error handling (line 20) is generally discouraged in Go unless it's a truly unrecoverable error. Consider returning an error to the caller instead, which allows for more graceful error handling and logging.
Regarding performance, ensure that the iteration over completed transactions does not introduce significant overhead, especially as the number of transactions grows. If performance becomes a concern, consider optimizing the data structure used for storing and querying these transactions.
| k.IterateCompletedOutgoingTxsByType(ctx, types.BatchTxPrefixByte, func(_ []byte, cotx types.OutgoingTx) bool { | ||
| sig := k.getEthereumSignature(ctx, cotx.GetStoreIndex(), val) | ||
| if len(sig) == 0 { | ||
| batch, ok := cotx.(*types.BatchTx) | ||
| if !ok { | ||
| panic(sdkerrors.Wrapf(types.ErrInvalid, "couldn't cast to batch tx for completed tx %s", cotx)) | ||
| } | ||
| unconfirmed = append(unconfirmed, batch) | ||
| } | ||
| return false | ||
| }) |
There was a problem hiding this comment.
Similar to the changes in contract_call.go, the addition of iterating over completed outgoing transactions in GetUnsignedBatchTxs is essential for the fix. However, the use of panic for error handling (line 171) should be reconsidered for the same reasons mentioned earlier: it's generally discouraged unless for truly unrecoverable errors. Consider returning an error instead.
Also, be mindful of the performance implications of iterating over a potentially large number of transactions. If performance issues arise, exploring optimizations for the data structure or query mechanism might be necessary.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #558 +/- ##
==========================================
+ Coverage 29.54% 29.63% +0.09%
==========================================
Files 48 48
Lines 7030 7048 +18
==========================================
+ Hits 2077 2089 +12
- Misses 4771 4775 +4
- Partials 182 184 +2
|
|
Are CompletedOutGoingTxs ever pruned or does the set of transactions growth unbounded? |
They are pruned on the block after slashing applies to them gravity-bridge/module/x/gravity/abci.go Lines 109 to 121 in 4abf107 |
|
this is ready to go IMO |
Adds an additional lookup to check if the signature is targeting an already-completed outgoing tx.
I verified manually that this fixes the issue with the following steps:
Closes #555