fix(state): Use correct end heights for end of block subtrees during the full sync #7566
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rent subtree indexes instead
Co-authored-by: Marek <[email protected]>
Merge branch 'main' into upgrade-subtrees-backwards
6 tasks
Questions for reviewersDoes this PR have any data bugs? Since this PR is on the critical path, I'd like other suggestions to go in other PRs or tickets. |
upbqdn
previously approved these changes
Sep 18, 2023
zebra-state/src/service/finalized_state/disk_format/upgrade/add_subtrees.rs
Outdated
Show resolved
Hide resolved
zebra-state/src/service/finalized_state/disk_format/upgrade/add_subtrees.rs
Outdated
Show resolved
Hide resolved
zebra-state/src/service/finalized_state/disk_format/upgrade/add_subtrees.rs
Outdated
Show resolved
Hide resolved
zebra-state/src/service/finalized_state/disk_format/upgrade/add_subtrees.rs
Outdated
Show resolved
Hide resolved
zebra-state/src/service/finalized_state/disk_format/upgrade/add_subtrees.rs
Outdated
Show resolved
Hide resolved
zebra-state/src/service/finalized_state/disk_format/upgrade/add_subtrees.rs
Show resolved
Hide resolved
|
I just reviewed this PR. Feel free to close all comments labeled as optional right away without addressing them. |
4 tasks
oxarbitrage
approved these changes
Sep 19, 2023
There was a problem hiding this comment.
approving without reviewing so it can be merged fast, @upbqdn approved previously.
upbqdn
reviewed
Sep 19, 2023
teor2345
commented
Sep 19, 2023
4 tasks
arya2
pushed a commit
that referenced
this pull request
Sep 29, 2023
…the full sync (#7566) * Avoid manual handling of previous sapling trees by using iterator windows instead * Avoid manual sapling subtree index handling by comparing prev and current subtree indexes instead * Simplify adding notes by using the exact number of remaining notes * Simplify by skipping the first block, because it can't complete a subtree * Re-use existing tree update code * Apply the sapling changes to orchard subtree updates * add a reverse database column family iterator function * Make skipping the lowest tree independent of iteration order * Move new subtree checks into the iterator, rename to end_height * Split subtree calculation into a new method * Split the calculate and write methods * Quickly check the first subtree before running the full upgrade * Do the quick checks every time Zebra runs, and refactor slow check error handling * Do quick checks for orchard as well * Make orchard tree upgrade match sapling upgrade code * Upgrade subtrees in reverse height order * Bump the database patch version so the upgrade runs again * Reset previous subtree upgrade data before doing this one * Add extra checks to subtree calculation to diagnose errors * Use correct heights for subtrees completed at the end of a block * Add even more checks to diagnose issues * Instrument upgrade methods to improve diagnostics * Prevent modification of re-used trees * Debug with subtree positions as well * Fix an off-by-one error with completed subtrees * Fix typos and confusing comments Co-authored-by: Marek <[email protected]> * Fix mistaken previous tree handling and end tree comments * Remove unnecessary subtraction in remaining leaves calc * Log heights when assertions fail * Fix new subtree detection filter * Move new subtree check into a method, cleanup unused code * Remove redundant assertions * Wait for subtree upgrade before testing RPCs * Fix subtree search in quick check * Temporarily upgrade subtrees in forward height order * Clarify some comments * Fix missing test imports * Fix subtree logging * Add a comment about a potential hang with future upgrades * Fix zebrad var ownership * Log more info when add_subtrees.rs fails * cargo fmt --all * Fix unrelated clippy::unnecessary_unwrap * cargo clippy --fix --all-features --all-targets; cargo fmt --all * Stop the quick check depending on tree de-duplication * Refactor waiting for the upgrade into functions * Wait for state upgrades whenever the cached state is updated * Wait for the testnet upgrade in the right place * Fix unused variable * Fix a subtree detection bug and comments * Remove an early reference to reverse direction * Stop skipping subtrees completed at the end of blocks * Actually fix new subtree code --------- Co-authored-by: Marek <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reviewer Questions
Does this PR have any data bugs?
Are there any other blocking issues? (For example, security bugs.)
Since this PR is on the critical path, I'd like other suggestions to go in other PRs or tickets.
Motivation
When a subtree is created during the full sync, and the final note commitment is at the end of a block, the end height is one higher than it should be.
Close #7532
Specifications
zcashduses the height of the block the subtree ends in:zcash/zcash#6694
Complex Code or Requirements
Because this bug fix is complex, I added some extra tests, and bumped the state version so the whole database will be upgraded again. I also cleared any data from the previous upgrade. This will make sure all our tests run on the new data.
Solution
Refactor the subtree upgrade so it uses block validation code that has already been tested
Bump the state version and delete previous data, so the tests run on the data from this upgrade only
Use correct heights for subtrees completed at the end of a block. Previously, these subtrees had a height that was 1 higher than the block their final note was in. This bug only happened for subtrees created by new blocks. (And not for upgraded blocks.)
This bug would have been caught by ticket #7446, but only if we found a test vector for "Completed Subtree at the end of the block", and "subtrees added with new tip blocks", and ran it on a full sync Zebra state.
I refactored the upgrade and tip block subtrees to use the same code, which is how I found this bug. So a similar bug shouldn't happen again.
Related Changes
Wait for the state upgrade in tests:
Refactor the subtree upgrade so the upgrade can be done in reverse order, but don't reverse the order yet.
Fix some clippy lints.
Testing
Add extra state validity tests, including a quick check before the upgrade runs for 20 minutes. The quick check runs on every Zebra launch, even in production.
Review
This is a routine change, but it is on the critical path for getting the RPCs finished and tested. It is also blocking all the rest of the test tickets.
I'd like non-blocking suggestions to go in other PRs or tickets.
Reviewer Checklist