change(db): Upgrade subtrees from the tip backwards, for compatibility with wallet syncing #7531
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
teor2345
added
C-bug
github-actions
bot
added
the
C-trivial
10 tasks
teor2345
changed the title
teor2345
force-pushed
the
upgrade-subtrees-backwards
branch
from
ada36d1
to
8ca198a
Compare
teor2345
force-pushed
the
upgrade-subtrees-backwards
branch
from
8ca198a
to
ba73b26
Compare
|
I have started a full sync with this code, so I can use it for the test in ticket #7450. |
teor2345
force-pushed
the
upgrade-subtrees-backwards
branch
from
ba73b26
to
8e8f7e6
Compare
upbqdn
approved these changes
Sep 19, 2023
|
In the merge queue:
|
This bug has nothing to do with this PR, so let's try merging it again. |
|
@Mergifyio refresh |
✅ Pull request refreshed |
arya2
pushed a commit
that referenced
this pull request
Sep 29, 2023
…y with wallet syncing (#7531) * 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 * Upgrade subtrees in reverse height order Reverts "Temporarily upgrade subtrees in forward height order" This reverts commit a9558be. * Bump the database patch version to re-run the upgrade (for testing) * Revert "Remove an early reference to reverse direction" This reverts commit c206404. --------- 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.
Motivation
We want wallets to be able to sync correctly while Zebra is upgrading the state to add subtrees.
Close #7407
Specifications
Zebra currently adds new subtrees if the tip block completes them. This is the correct behaviour, even if an upgrade is in progress.
For subtrees added during state upgrades, Zebra upgrades the indexes live in reverse height order.
API Reference
Iterator prev/current:
https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.tuple_windows
Database iterators:
https://docs.rs/rocksdb/latest/rocksdb/struct.DBCommon.html#method.iterator_cf
https://docs.rs/rocksdb/latest/rocksdb/enum.IteratorMode.html
Complex Code or Requirements
The new database code needs to get the blocks in the correct order. Please review this carefully. The order can be checked manually in the logs, or using the subtree RPC during the upgrade.
Because these code changes are 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
Do subtree upgrades in reverse height order
Bump the state version and delete previous data, so the tests run on the data from this upgrade only
Review
This is a routine change, but it is on the critical path for getting the RPCs finished and tested.
Reviewer Checklist