fix: update pending nonces when aborting a cctx through MsgAbortStuckCCTX #3230
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request encompasses a series of changes aimed at enhancing the functionality and stability of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3230 +/- ##
===========================================
+ Coverage 61.75% 61.78% +0.02%
===========================================
Files 431 431
Lines 30772 30786 +14
===========================================
+ Hits 19004 19021 +17
+ Misses 10911 10909 -2
+ Partials 857 856 -1
|
kingpinXD
changed the title
kingpinXD
requested review from
brewmaster012,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (14)
x/crosschain/keeper/msg_server_abort_stuck_cctx.go (2)
43-44: Consider adding logging for this important state transition.While the status update implementation is correct, adding logging would help with debugging and monitoring of aborted transactions in production.
// update the status +k.Logger(ctx).Info("Aborting stuck CCTX", "index", cctx.Index) cctx.CctxStatus.UpdateStatusAndErrorMessages(types.CctxStatus_Aborted, AbortMessage, "") +k.Logger(ctx).Info("CCTX aborted successfully", "index", cctx.Index, "status", types.CctxStatus_Aborted)
Line range hint
27-29: Enhance function documentation.Consider expanding the function documentation to describe:
- The side effects (status changes, nonce updates)
- The conditions under which a CCTX is considered "stuck"
- The implications of aborting a CCTX
// AbortStuckCCTX aborts a stuck CCTX // Authorized: admin policy group 2 +// Side effects: +// - Updates CCTX status to Aborted +// - Removes the transaction from pending nonces +// - Updates outbound transaction state +// A CCTX is considered stuck when it remains in a pending status without progress +// Aborting a CCTX is irreversible and should be used with caution func (k msgServer) AbortStuckCCTX(x/crosschain/keeper/outbound_tracker_test.go (1)
61-66: Consider enhancing the negative test case.While testing for non-panicking behavior is good, consider adding assertions to verify the system state remains unchanged after attempting to remove a non-existent tracker.
Here's a suggested enhancement:
t.Run("Do nothing if tracker doesn't exist", func(t *testing.T) { keeper, ctx, _, _ := keepertest.CrosschainKeeper(t) + // Create some initial state + items := createNOutboundTracker(keeper, ctx, 1) + initialState := keeper.GetAllOutboundTracker(ctx) require.NotPanics(t, func() { keeper.RemoveOutboundTrackerFromStore(ctx, 1, 1) }) + + // Verify system state remains unchanged + finalState := keeper.GetAllOutboundTracker(ctx) + require.Equal(t, initialState, finalState) })x/crosschain/types/status.go (2)
90-92: LGTM! Consider adding documentation.The implementation is correct and aligns with the state transition map. Consider adding documentation to explain what constitutes a terminal status and its significance in the CCTX lifecycle.
Add documentation above the method:
+// IsTerminalStatus returns true if the status represents a final state +// from which no further transitions are possible (Aborted, Reverted, or OutboundMined). func (c CctxStatus) IsTerminalStatus() bool {
94-96: LGTM! Consider adding documentation and using constants.The implementation correctly identifies pending states. Consider documenting the method and using constants for better maintainability.
Consider refactoring to use constant sets for better maintainability:
var ( terminalStatuses = map[CctxStatus]bool{ CctxStatus_Aborted: true, CctxStatus_Reverted: true, CctxStatus_OutboundMined: true, } pendingStatuses = map[CctxStatus]bool{ CctxStatus_PendingInbound: true, CctxStatus_PendingOutbound: true, CctxStatus_PendingRevert: true, } ) func (c CctxStatus) IsTerminalStatus() bool { return terminalStatuses[c] } func (c CctxStatus) IsPendingStatus() bool { return pendingStatuses[c] }This approach:
- Makes it easier to maintain status categories
- Provides better performance for larger sets
- Makes the code more maintainable
x/crosschain/types/status_test.go (2)
178-197: LGTM! Consider adding documentation for terminal status definition.The test implementation is clean and comprehensive, following Go testing best practices with table-driven tests. Consider adding a comment block explaining what constitutes a terminal status (OutboundMined, Reverted, Aborted) for better maintainability.
+// TestCctxStatus_IsTerminalStatus verifies the IsTerminalStatus method +// Terminal statuses represent final states (OutboundMined, Reverted, Aborted) +// from which no further transitions are possible func TestCctxStatus_IsTerminalStatus(t *testing.T) {
199-218: LGTM! Consider adding documentation for pending status definition.The test implementation is clean and comprehensive, following Go testing best practices with table-driven tests. Consider adding a comment block explaining what constitutes a pending status (PendingInbound, PendingOutbound, PendingRevert) for better maintainability.
+// TestCctxStatus_IsPendingStatus verifies the IsPendingStatus method +// Pending statuses represent intermediate states (PendingInbound, PendingOutbound, PendingRevert) +// from which transitions to other states are expected func TestCctxStatus_IsPendingStatus(t *testing.T) {x/crosschain/keeper/msg_server_abort_stuck_cctx_test.go (3)
33-41: Consider reducing test setup duplicationThe setup code for TssNonce and PendingNonces is duplicated across all test cases. Consider extracting this into a helper function to improve maintainability.
func setupCCTXWithPendingNonces(t *testing.T, k *keeper.Keeper, ctx sdk.Context, status crosschaintypes.CctxStatus, statusMessage string) *crosschaintypes.CrossChainTx { cctx := sample.CrossChainTx(t, "cctx_index") cctx.CctxStatus = &crosschaintypes.Status{ Status: status, StatusMessage: statusMessage, } cctx.GetCurrentOutboundParam().TssNonce = 1 k.SetCrossChainTx(ctx, *cctx) k.GetObserverKeeper().SetPendingNonces(ctx, observertypes.PendingNonces{ NonceLow: 0, NonceHigh: 10, ChainId: cctx.GetCurrentOutboundParam().ReceiverChainId, Tss: cctx.GetCurrentOutboundParam().TssPubkey, }) return cctx }Also applies to: 79-87, 129-137
57-61: Enhance pending nonces verificationThe assertions for pending nonces state could be more comprehensive. Consider adding checks for:
- NonceHigh value remains unchanged
- ChainId and Tss values match the CCTX parameters
// Add these assertions after the existing ones require.Equal(t, int64(10), pendingNonces.NonceHigh) require.Equal(t, cctx.GetCurrentOutboundParam().ReceiverChainId, pendingNonces.ChainId) require.Equal(t, cctx.GetCurrentOutboundParam().TssPubkey, pendingNonces.Tss)Also applies to: 103-109, 153-157
Line range hint
18-224: Consider adding edge case testsThe test suite has good coverage of main scenarios, but consider adding these edge cases:
- Test aborting when nonce equals NonceHigh
- Test handling of nonce overflow scenarios
Example test case:
t.Run("can handle nonce at upper bound", func(t *testing.T) { // Arrange k, ctx, _, _ := keepertest.CrosschainKeeperWithMocks(t, keepertest.CrosschainMockOptions{ UseAuthorityMock: true, }) // Setup CCTX with nonce = NonceHigh cctx := sample.CrossChainTx(t, "cctx_index") cctx.GetCurrentOutboundParam().TssNonce = 10 // ... rest of the setup // Assert appropriate handling })e2e/utils/zetacore.go (2)
Line range hint
69-72: Consider documenting the implications of usingInTxHashToCctxData.The TODO comment mentions replacing
InTxHashToCctxDatawithInboundHashToCctxDatabut doesn't explain the implications or potential risks of using the current approach. This information would be valuable for maintainers.
Line range hint
1-324: Consider abstracting common waiting patterns.The file contains several functions with similar waiting patterns (
WaitCctxMinedByInboundHash,WaitCCTXMinedByIndex,WaitCctxByInboundHash). Consider creating a generic waiting mechanism to reduce code duplication and improve maintainability.Example abstraction:
type WaitConfig struct { Timeout time.Duration RetryDelay time.Duration LogInterval int Query func(ctx context.Context) (interface{}, error) IsComplete func(result interface{}) bool } func WaitForCondition(ctx context.Context, config WaitConfig, logger infoLogger) (interface{}, error) { startTime := time.Now() for i := 0; ; i++ { if time.Since(startTime) > config.Timeout { return nil, errors.New("timeout waiting for condition") } result, err := config.Query(ctx) if err != nil { if i%config.LogInterval == 0 { logger.Info("Error querying condition: %s", err.Error()) } time.Sleep(config.RetryDelay) continue } if config.IsComplete(result) { return result, nil } time.Sleep(config.RetryDelay) } }changelog.md (2)
Line range hint
1-24: Consider adding a table of contents for better navigation.The changelog is well-organized chronologically, but as it grows, navigation becomes challenging. Consider adding a table of contents at the top with links to each version section.
# CHANGELOG + +## Table of Contents + +- [Unreleased](#unreleased) +- [v23.0.0](#v2300) +- [v22.1.2](#v2212) +...
17-17: Add more context to the fix description.The fix description "update pending nonces when aborting a cctx through MsgAbortStuckCCTX" could benefit from additional context about the problem it solves and its impact.
-* [3230](https://github.com/zeta-chain/node/pull/3230) - update pending nonces when aborting a cctx through MsgAbortStuckCCTX +* [3230](https://github.com/zeta-chain/node/pull/3230) - fix: update pending nonces when aborting a cctx through MsgAbortStuckCCTX to prevent nonce tracking inconsistencies
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
changelog.md(1 hunks)e2e/utils/zetacore.go(2 hunks)x/crosschain/keeper/cctx.go(2 hunks)x/crosschain/keeper/msg_server_abort_stuck_cctx.go(1 hunks)x/crosschain/keeper/msg_server_abort_stuck_cctx_test.go(5 hunks)x/crosschain/keeper/outbound_tracker_test.go(1 hunks)x/crosschain/types/status.go(1 hunks)x/crosschain/types/status_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
e2e/utils/zetacore.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/cctx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/msg_server_abort_stuck_cctx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/msg_server_abort_stuck_cctx_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/outbound_tracker_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/status.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/status_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (8)
x/crosschain/keeper/msg_server_abort_stuck_cctx.go (1)
39-39: Clean refactoring of status check logic!
The use of IsPendingStatus() method encapsulates the status check logic, making the code more maintainable and less prone to errors.
x/crosschain/keeper/outbound_tracker_test.go (1)
45-59: LGTM! Well-structured test for existing tracker removal.
The test case effectively verifies the removal of existing outbound trackers and follows Go testing best practices by using sub-tests.
x/crosschain/keeper/cctx.go (1)
Line range hint 37-50: Implementation aligns with PR objectives
The code correctly handles nonce mapping for pending states and updates zeta accounting for aborted transactions. This implementation addresses the issue mentioned in #3157 regarding pending nonces during CCTX abortion.
Let's verify the status handling implementation:
✅ Verification successful
Nonce mapping implementation is consistent with status transitions
The verification confirms that:
- The status transitions between
PendingOutboundandPendingRevertare well-defined inx/crosschain/types/status.go - The nonce mapping is consistently set only for these two states across the codebase
- The implementation aligns with the state transition rules and test cases validate the behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of status handling across the codebase
# Check for other places where CctxStatus_PendingOutbound and CctxStatus_PendingRevert are used
rg "CctxStatus_PendingOutbound|CctxStatus_PendingRevert" --type go
# Check for potential inconsistencies in nonce handling
ast-grep --pattern 'SetNonceToCctx($$$)'
Length of output: 18409
x/crosschain/keeper/msg_server_abort_stuck_cctx_test.go (1)
7-7: LGTM: Clean import addition
The addition of the observertypes import is well-placed and necessary for the new pending nonces functionality.
e2e/utils/zetacore.go (3)
Line range hint 116-132: LGTM! Well-structured status check refactoring.
The change from using a function to a method call (IsTerminalStatus()) on the status type improves encapsulation and maintainability. The rate-limited logging is also well-implemented to prevent log spam.
Line range hint 173-185: LGTM! Consistent status check implementation.
The status check implementation maintains consistency with the previous refactoring, using the same method call pattern and logging approach.
Line range hint 13-19: Investigate increasing timeout requirements.
The timeout has been increased from 4 to 8 minutes, which might indicate underlying performance issues or race conditions. This warrants investigation as noted in issue #2690.
Run the following script to analyze the historical context of timeout-related changes:
✅ Verification successful
Let me gather more context about the timeout changes and their rationale.
Let me check the referenced issue and the commit history around this timeout change.
Timeout increase appears to be a deliberate performance optimization
The timeout increase from 4 to 8 minutes in DefaultCctxTimeout is a justified change based on the following findings:
- The timeout is specifically used in end-to-end (e2e) testing scenarios, not production code
- The change is documented with a reference to issue
e2e: cctx timeout need to be increased to avoid test failures #2690 - The timeout is consistently applied across multiple CCTX-related functions in the test utilities
- No recurring timeout-related test failures were found in the e2e test logs
The increased duration accommodates longer processing times during test scenarios without indicating underlying performance issues or race conditions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze timeout-related changes and their context
# Search for timeout-related changes in the codebase
rg -A 3 "DefaultCctxTimeout.*=.*time\.Minute"
# Search for timeout-related issues in error logs
rg -A 3 "timeout.*cctx not mined"
Length of output: 789
Script:
#!/bin/bash
# Search for timeout-related changes in test files and configurations
rg -B 2 -A 2 "time\.Minute.*timeout"
# Search for any timeout-related configurations in the codebase
rg -B 2 -A 2 "DefaultCctxTimeout|CCTX_TIMEOUT"
# Look for test failures or issues related to timeouts
rg -B 2 -A 2 "test.*failed.*timeout|error.*timeout"
Length of output: 2915
Script:
#!/bin/bash
# Check the git history for the timeout change
git log -p --grep="timeout" e2e/utils/zetacore.go
# Look for the referenced issue number in commit messages
git log --grep="#2690"
# Search for timeout-related test failures in the e2e directory
rg -B 3 -A 3 "failed.*timeout|error.*timeout" e2e/
Length of output: 399
changelog.md (1)
Line range hint 1-1500: LGTM! The changelog follows good practices.
The changelog effectively documents:
- Breaking changes with clear explanations
- Features, fixes, and refactoring changes
- Links to relevant PRs and issues
- Version numbers following semantic versioning
…ns and add unit tests
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
Description
Closes #3157
#3243
The pr does the following
Calls
SaveOutboundat the end of MsgAbortCCTX , which processes the outbound in the same way as a regular cctxAdd simple functions for the status check , in the types package
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests