fix tsnonce preventing multisignature bug #2453
Conversation
WalkthroughThe changes involve a modification to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 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: 0
🧹 Outside diff range and nitpick comments (1)
protocol/x/accountplus/ante/timestampnonce.go (1)
22-22: LGTM! Bug fix for multisignature transactions.This change successfully addresses the issue of multisignature transactions being blocked. The new implementation correctly identifies single-signature timestamp nonce transactions while allowing other types (including multisig) to proceed.
Consider renaming the function for clarity.
The current function name
IsTimestampNonceTxmight be slightly misleading, as it now returnsfalsefor multisig timestamp nonce transactions. Consider renaming it toIsSingleSignatureTimestampNonceTxor similar to more accurately reflect its behavior.Suggestion: Add debug logging for multisignature cases.
For improved observability, consider adding debug logging when encountering multisignature transactions. This could help with monitoring and debugging in production environments.
Here's a suggested implementation:
func IsTimestampNonceTx(ctx sdk.Context, tx sdk.Tx) (bool, error) { sigTx, ok := tx.(authsigning.SigVerifiableTx) if !ok { return false, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type") } signatures, err := sigTx.GetSignaturesV2() if err != nil { return false, err } - return len(signatures) == 1 && accountpluskeeper.IsTimestampNonce(signatures[0].Sequence), nil + if len(signatures) > 1 { + ctx.Logger().Debug("Multisignature transaction detected", "num_signatures", len(signatures)) + return false, nil + } + return len(signatures) == 1 && accountpluskeeper.IsTimestampNonce(signatures[0].Sequence), nil }This change adds logging for multisignature transactions without altering the function's behavior.
jerryfan01234
force-pushed
the
tsnonce_audit
branch
from
e48ba7f
to
4eee507
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
protocol/x/accountplus/ante/timestampnonce_test.go (2)
35-38: Approved: Test case updated to reflect new behavior for multisignature transactions.The changes in this test case align with the PR objectives of fixing the bug that prevented processing of multisignature transactions. The test now expects
IsTimestampNonceTxto returnfalsewithout an error for transactions with more than one signature, allowing them to be processed further in the AnteHandle function.Consider adding the following test cases to ensure comprehensive coverage:
- A test case with exactly two signatures, both being timestamp nonces.
- A test case with multiple signatures, where only some are timestamp nonces.
- A test case with the maximum allowed number of signatures in a transaction.
These additional test cases will help verify the function's behavior across various multisignature scenarios.
Line range hint
1-90: Overall impact: Test function updated correctly, minor improvement suggested.The changes to the
TestIsTimestampNonceTxfunction effectively address the multisignature transaction issue while maintaining coverage for other scenarios. This approach ensures that theIsTimestampNonceTxfunction now correctly handles all types of transactions as per the PR objectives.Consider updating the error handling in the test function to provide more specific assertions:
if tc.expectedErr { - require.NotNil(t, err) + require.Error(t, err) +} else { + require.NoError(t, err) }This change will make the test more explicit about the expected error behavior and will catch any unexpected errors in the non-error cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- protocol/x/accountplus/ante/timestampnonce.go (1 hunks)
- protocol/x/accountplus/ante/timestampnonce_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protocol/x/accountplus/ante/timestampnonce.go
🔇 Additional comments (1)
protocol/x/accountplus/ante/timestampnonce_test.go (1)
Line range hint
1-90: Summary: Changes address the multisignature issue effectively.The modifications to the
TestIsTimestampNonceTxfunction successfully update the test expectations to align with the new behavior of allowing multisignature transactions. The changes are focused, maintain existing test coverage, and correctly implement the fix described in the PR objectives.To ensure that these changes are consistent with the implementation, please run the following script to verify the
IsTimestampNonceTxfunction:This script will help confirm that the implementation matches the updated test expectations and that there are no remaining error returns related to multiple signatures.
✅ Verification successful
Verification Successful:
IsTimestampNonceTxhandles multiple signatures correctly.The
IsTimestampNonceTxfunction correctly returnsfalsewhen multiple signatures are present, aligning with the test cases. The implementation ensures that only transactions with a single timestamp nonce signature are considered valid, and additional signatures result in the expectedfalseoutcome without errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of IsTimestampNonceTx function # Test: Search for the IsTimestampNonceTx function implementation ast-grep --lang go --pattern $'func IsTimestampNonceTx($_, $_) (bool, error) { $$$ }' # Test: Check for any remaining error returns related to multiple signatures rg --type go 'return false, .*more than one signature' -A 5 -B 5Length of output: 1086
Script:
#!/bin/bash # Description: Inspect handling of multiple signatures in IsTimestampNonceTx function # Search for logic that handles cases with more than one signature ast-grep --lang go --pattern $'func IsTimestampNonceTx($_, $_) (bool, error) { $$$ if len(signatures) > 1 { $$$ } $$$ }' # Additionally, check for any conditional statements that might bypass the len(signatures) == 1 check rg --type go 'if .*len\(signatures\) > 1' -A 5 -B 5Length of output: 187
jerryfan01234
force-pushed
the
tsnonce_audit
branch
from
4eee507
to
c8bd668
Compare
|
@Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
(cherry picked from commit 77cef30)
Co-authored-by: jerryfan01234 <[email protected]>
Changelist
in AnteHandle seq number is checked if it is a ts nonce via
However, IsTimestampNonce would return an error if len(signatures) != 1, causing AnteHandle to early return. Hence, multisignature would not be possible. This PR fixes the bug.
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit