fix(drive): set sign height when rebroadcasting #2210
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
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
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)
packages/rs-drive-abci/src/execution/platform_events/withdrawals/rebroadcast_expired_withdrawal_documents/v1/mod.rs (1)
Line range hint
30-116: Overall impact analysis and testing recommendation.The main change in this implementation is updating the withdrawal status from
BROADCASTEDtoPOOLEDfor expired withdrawal documents. While this change aligns with the PR objectives, it's crucial to ensure that all dependent systems and processes are updated accordingly.The overall structure and logic of the method remain intact, with the additional improvement of using a
Vecforexpired_withdrawal_indicesas per previous learnings.To ensure the reliability and correctness of this change:
- Conduct thorough testing of the entire withdrawal process, particularly focusing on scenarios involving expired documents.
- Update any documentation or specifications that reference the
BROADCASTEDstatus for expired withdrawal documents.- Verify that all downstream processes handling withdrawal documents are compatible with the
POOLEDstatus for previously expired documents.- Consider adding logging or monitoring to track the frequency and impact of this status change in production.
These steps will help maintain system consistency and catch any potential issues early in the development process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/rs-drive-abci/src/execution/platform_events/withdrawals/rebroadcast_expired_withdrawal_documents/v1/mod.rs (1 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: shumkov PR: dashpay/platform#2185 File: packages/rs-drive-abci/src/execution/platform_events/withdrawals/rebroadcast_expired_withdrawal_documents/v1/mod.rs:47-62 Timestamp: 2024-09-30T11:55:43.856Z Learning: In `rebroadcast_expired_withdrawal_documents_v1`, the variable `expired_withdrawal_indices` needs to be a `Vec<WithdrawalTransactionIndex>` rather than a `BTreeSet<WithdrawalTransactionIndex>`, because a vector is necessary for subsequent processing.packages/rs-drive-abci/src/execution/platform_events/withdrawals/rebroadcast_expired_withdrawal_documents/v1/mod.rs (1)
Learnt from: shumkov PR: dashpay/platform#2185 File: packages/rs-drive-abci/src/execution/platform_events/withdrawals/rebroadcast_expired_withdrawal_documents/v1/mod.rs:47-62 Timestamp: 2024-09-30T11:55:43.856Z Learning: In `rebroadcast_expired_withdrawal_documents_v1`, the variable `expired_withdrawal_indices` needs to be a `Vec<WithdrawalTransactionIndex>` rather than a `BTreeSet<WithdrawalTransactionIndex>`, because a vector is necessary for subsequent processing.
🔇 Additional comments (2)
packages/rs-drive-abci/src/execution/platform_events/withdrawals/rebroadcast_expired_withdrawal_documents/v1/mod.rs (2)
Line range hint
47-52: LGTM! Correct application of previous learning.The implementation correctly uses
Vec<WithdrawalTransactionIndex>forexpired_withdrawal_indices, as suggested in the learning from the previous PR. This change allows for the necessary subsequent processing of the withdrawal indices.
71-71: LGTM! Verify impact on dependent systems.The change from
WithdrawalStatus::BROADCASTEDtoWithdrawalStatus::POOLEDaligns with the PR objectives. This modification correctly updates the status of expired withdrawal documents when they are being reprocessed.To ensure this change doesn't introduce unintended side effects, please verify that all systems or components that depend on the withdrawal status are updated accordingly. Run the following script to identify potential areas that may need attention:
Review the results to ensure that all relevant parts of the codebase are consistent with this status change.
✅ Verification successful
The change from
WithdrawalStatus::BROADCASTEDtoWithdrawalStatus::POOLEDinrebroadcast_expired_withdrawal_documents/v1/mod.rshas been verified. All usages of both statuses across the codebase are consistent, ensuring no unintended side effects.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of WithdrawalStatus::BROADCASTED and WithdrawalStatus::POOLED echo "Searching for WithdrawalStatus::BROADCASTED usage:" rg --type rust "WithdrawalStatus::BROADCASTED" -C 3 echo "\nSearching for WithdrawalStatus::POOLED usage:" rg --type rust "WithdrawalStatus::POOLED" -C 3Length of output: 40781
QuantumExplorer
changed the title
shumkov
changed the title
|
We decided to test on Monday and make a fix if needed. |
Issue being fixed or feature implemented
We were rebroadcasting without setting the sign height which was causing the transaction to expire immediately.
What was done?
Reset the sign height.
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes