fixes 1162; race condition between delegation record queries and withdrawal acks #1183
Conversation
joe-bowman
requested review from
faddat,
minhngoc274 and
tuantran1702
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe modifications primarily focus on enhancing the handling of delegation records and withdrawal acknowledgements to address potential race conditions. Changes include updates to Dockerfile base images, script adjustments for unbonding features, introduction and refinement of callback functions, error handling improvements, and adjustments in handling withdrawal waitgroups and redemption rate calculations. These changes aim to synchronize delegation record updates with unbonding transaction acknowledgements, ensuring accurate reflection of the host chain state for redemption rate calculations. Changes
Assessment against linked issues
Poem
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}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
scripts/registerzone.jsonis excluded by:!**/*.json
Files selected for processing (11)
- Dockerfile (2 hunks)
- scripts/setup.sh (1 hunks)
- x/interchainstaking/keeper/callbacks.go (7 hunks)
- x/interchainstaking/keeper/callbacks_test.go (1 hunks)
- x/interchainstaking/keeper/delegation.go (2 hunks)
- x/interchainstaking/keeper/delegation_test.go (1 hunks)
- x/interchainstaking/keeper/hooks.go (3 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers.go (15 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers_test.go (2 hunks)
- x/interchainstaking/keeper/receipt.go (3 hunks)
- x/interchainstaking/keeper/redemptions.go (1 hunks)
Additional comments: 22
Dockerfile (2)
- 1-1: The downgrade from
alpine3.19toalpine3.18in the Dockerfile's builder stage is noted. Ensure all dependencies installed are compatible with Alpine 3.18.- 25-25: The final container also uses
alpine:3.18instead ofalpine:3.19. This change is consistent with the builder stage and aims to address compatibility issues with wasm. Ensure to test the final container for any potential issues arising from the downgrade.x/interchainstaking/keeper/hooks.go (2)
- 104-106: The adjustment to reset the
withdrawal waitgroupto 0 if it's greater than 0 at this point in the epoch end logic is noted. Ensure to test this logic thoroughly to avoid potential issues with waitgroup management.- 161-161: The introduction of a new call to
IncrementWithdrawalWaitgroupfor delegations trigger is a critical change. It's important to ensure that this increment is correctly balanced with a corresponding decrement elsewhere in the code to avoid waitgroup leaks.x/interchainstaking/keeper/delegation.go (2)
- 332-333: The logic to trigger redemption rate calculation when there are non-positive coins to flush and the withdrawal waitgroup is 0 is noted. This seems like a sensible fallback to ensure the redemption rate is updated correctly. Ensure this logic is thoroughly tested, especially in edge cases where the withdrawal waitgroup might be manipulated concurrently.
- 358-362: Adjusting the logic to trigger redemption rate calculation when no messages are sent and the withdrawal waitgroup is 0 is a critical change. This ensures that the redemption rate is updated even when no delegation flush messages are sent. It's important to validate this behavior through testing, particularly in scenarios where the withdrawal waitgroup's state might change unexpectedly.
x/interchainstaking/keeper/receipt.go (3)
- 215-215: Logging the successful transfer of qAssets via the "return to sender" (RTS) mechanism is a good practice for traceability. However, it's important to ensure that sensitive information is not logged, especially in production environments.
Ensure that no sensitive data is included in the log statements to maintain security and privacy.
- 225-225: Logging the transfer of qAssets to a mapped account is beneficial for traceability. Similar to the previous comment, ensure that sensitive information, such as account details, is handled appropriately in the logs.
Continue to monitor and review logging practices to ensure they align with security and privacy standards.
- 228-228: The logging statement after transferring qAssets to the sender provides useful information for auditing and debugging. It's crucial to maintain this level of transparency while ensuring that sensitive data is protected.
Keep up the good practice of detailed logging, ensuring that it does not compromise user privacy or security.
x/interchainstaking/keeper/redemptions.go (1)
- 253-255: The addition of
IncrementWithdrawalWaitgroupbefore setting the zone in the context is a strategic move to manage withdrawal waitgroups effectively. This change is crucial for synchronizing the handling of unbonding messages with the updates to delegation records, aiming to eliminate the race condition issue identified. It's important to ensure that theIncrementWithdrawalWaitgroupmethod is implemented with thread safety in mind, considering the concurrent nature of blockchain operations. Given the learnings from previous PRs, it's reassuring to know that the counterpart methodDecrementWithdrawalWaitgroupincludes underflow protection, which is a good practice for maintaining system stability. However, it would be beneficial to verify that similar protective measures are in place for increment operations to prevent potential overflow issues, especially under high load conditions.Ensure that the
IncrementWithdrawalWaitgroupmethod includes necessary protections against overflow and is thread-safe to handle concurrent operations effectively.x/interchainstaking/keeper/callbacks.go (6)
- 76-78: The addition of
delegations_epochanddelegation_epochcallbacks enhances the system's ability to handle delegation updates more effectively, especially in terms of epoch-based operations. This is a positive change that aligns with the PR objectives to address race conditions in delegation record updates.- 129-131: The enhanced error handling and logging within the
RewardsCallbackfunction, specifically the non-abortive error handling strategy, is a good practice. It ensures that even if errors occur, the system can continue processing other messages, which is crucial for maintaining system resilience and reliability.- 139-141: The introduction of the
DelegationsEpochCallbackfunction, which utilizes thedelegationsCallbackwith anisEpochflag, is a smart way to reuse code for both epoch and non-epoch delegation updates. This approach reduces code duplication and maintains consistency in handling delegation callbacks.- 162-178: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [147-166]
The
delegationsCallbackfunction's implementation, which handles both epoch and non-epoch delegation updates, is well-designed. However, it's important to ensure that theisEpochflag's impact on the system's behavior is thoroughly tested, especially regarding how delegation records are updated and how it affects race condition resolution.
- 162-178: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [168-224]
The
DelegationEpochCallbackanddelegationCallbackfunctions, similar to the delegations callbacks, use a shared implementation with anisEpochflag to differentiate between epoch and non-epoch operations. This consistency in design is commendable. However, the handling of delegations with zero shares, specifically the removal of such delegations, should be carefully reviewed to ensure it does not introduce unintended side effects, especially in the context of race condition resolution.
- 662-673: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [642-670]
The logic within
DelegationAccountBalanceCallbackto decrement the withdrawal waitgroup and potentially trigger the redemption rate calculation is crucial for addressing the race condition issue outlined in the PR objectives. However, it's essential to ensure that this logic is robust and does not inadvertently introduce new race conditions or synchronization issues, especially when handling delegation account balances.scripts/setup.sh (1)
- 480-480: The change to enable the unbonding feature in the interchainstaking parameters is correctly implemented with a
jqcommand to modify the genesis configuration. This aligns with the PR objectives to manage delegation and unbonding processes more effectively.x/interchainstaking/keeper/ibc_packet_handlers.go (2)
- 372-375: The addition of a new case to handle refunds for deposits with a specific memo ("refund") is a straightforward and effective way to address the need for refunding unprocessable deposits. This change is clear and follows the existing pattern for handling different message types. The logging provides useful context for this operation.
- 764-765: Incrementing the withdrawal waitgroup after emitting a delegation_epoch query is a necessary step to ensure that the system correctly tracks the number of outstanding operations that need to be completed before certain actions, like redemption rate calculations, can be performed. This change is consistent with the approach to manage synchronization and prevent race conditions. The logging provides useful context for this operation.
x/interchainstaking/keeper/ibc_packet_handlers_test.go (1)
- 827-827: The error field in the test case for
TestHandleWithdrawRewardshas been changed fromtruetofalse. This change indicates that the test case now expects the operation to succeed where it previously expected a failure. Ensure that this change aligns with the intended behavior and logic of theHandleWithdrawRewardsfunction. If the function's behavior or the conditions under which it succeeds have not changed, this modification might indicate an oversight or misunderstanding in the test's expectations.x/interchainstaking/keeper/callbacks_test.go (1)
- 2652-2652: The modification to assert that no error occurs aligns with the updated behavior of the
DelegationAccountBalancesCallbackfunction. It's important to ensure that this change accurately reflects the intended behavior and does not overlook potential errors that should be caught by the test.x/interchainstaking/keeper/delegation_test.go (1)
- 220-220: The addition of a new boolean argument
falseto theUpdateDelegationRecordForAddressmethod call within the test function indicates a change in the method's signature or functionality. Ensure that this change is accurately documented and that the test case reflects the intended behavior controlled by this new argument. It's also important to verify that other usages of this method throughout the codebase have been updated accordingly to accommodate this change.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/interchainstaking/keeper/callbacks.go (7 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers.go (15 hunks)
- x/interchainstaking/keeper/redemptions.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/interchainstaking/keeper/callbacks.go
- x/interchainstaking/keeper/ibc_packet_handlers.go
- x/interchainstaking/keeper/redemptions.go
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1183 +/- ##
==========================================
- Coverage 63.40% 63.13% -0.28%
==========================================
Files 171 171
Lines 11145 11205 +60
==========================================
+ Hits 7067 7074 +7
- Misses 3370 3412 +42
- Partials 708 719 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
1. Summary
Fixes #1162
Historically the the acknowledgement from a withdrawal triggered the reduction in the DelegationRecord for the validator to which the record pertained. However, simultaneously, an ICQ request to update delegation records is triggered at the epoch, and depending on the order of the two independent requests, the record ends up correct, or the undelegate amount less than it ought to be. This discrepancy affects redemption rate calculations, and when there is a large amount unbonding this can be material.
2.Type of change
3. Implementation details
To solve this issue, we add the undelegation messages to the WithdrawalWaitgroup. This means the undelegation messages and their subsequent queries for updated delegation records must complete before the RR is updated.
Note: also revert alpine to 3.18 as 3.19 is not compatible with current version of wasm.
Note (2): We intentionally choose not to throw errors when WithdrawalWaitgroup underflows, but instead log errors and consciously do not trigger a RR update. This is because failing will cause ICA messages to not be delivered. The waitgroup can end up in this state if there are undelivered acks from the previous epoch.
Summary by CodeRabbit
delegations_epochanddelegation_epochcallbacks for improved interchain staking functionality.