zcash_client_sqlite: Allow truncate_to_chain_state to truncate to an unscanned block height.#2248
Merged
Merged
Conversation
nuttycom
force-pushed
the
bug/rewind_to_chain_state
branch
2 times, most recently
from
b44e7eb to
3842361
Compare
str4d
previously approved these changes
Mar 27, 2026
nullcopy
requested changes
Mar 27, 2026
nullcopy
previously approved these changes
Mar 27, 2026
Contributor
nullcopy
left a comment
nullcopy
left a comment
There was a problem hiding this comment.
My previous NACK was based on a misunderstanding. See https://github.com/zcash/librustzcash/pull/2248/changes#r3003267813
nuttycom
force-pushed
the
bug/rewind_to_chain_state
branch
from
5122160 to
d4e3e1d
Compare
daira
reviewed
Mar 29, 2026
| SqliteShardStore::<_, ::sapling::Node, SAPLING_SHARD_HEIGHT>::from_connection( | ||
| conn, | ||
| crate::SAPLING_TABLES_PREFIX, | ||
| // If a birthday frontiers are available and the birthday height is less than or equal to the |
Contributor
There was a problem hiding this comment.
Suggested change
| // If a birthday frontiers are available and the birthday height is less than or equal to the | |
| // If birthday frontiers are available and the birthday height is less than or equal to the |
Non-blocking.
daira
reviewed
Mar 29, 2026
daira
reviewed
Mar 29, 2026
daira
approved these changes
Mar 29, 2026
daira
requested changes
Mar 29, 2026
Contributor
daira
left a comment
daira
left a comment
There was a problem hiding this comment.
The suggestion about the target_height comparison is blocking because the code doesn't currently match the comment. (If the comment is wrong, please change that instead.)
Contributor
|
Not my wheelhouse but these changes look sound to me. |
…below wallet birthday Tests that truncate_to_chain_state succeeds when the target height is below the wallet birthday (i.e., there is no entry in the blocks table at that height). This exercises the case where a wallet needs to rewind to a height it has never scanned. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…oint intersection The previous query computed MAX(MIN(sapling), MIN(orchard)), which returns a lower bound on where both trees have started checkpointing but does not guarantee that a checkpoint at that specific height exists in both tables. This caused incorrect safe rewind height reporting and incorrect truncation targets in truncate_to_chain_state. The fix queries for the actual minimum checkpoint height that exists in the intersection of both checkpoint tables, matching the semantics used by select_truncation_height. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…canned heights. Previously, truncate_to_chain_state could fail if no entry existed in the `blocks` table at the given height. This change repeals that restriction.
Collaborator
Author
|
Moved back to draft because I discovered another failure mode in the same vein. |
nuttycom
force-pushed
the
bug/rewind_to_chain_state
branch
from
d4e3e1d to
ad7b729
Compare
…canned. This tests the scenario where the wallet has been truncated to an earlier chain state (for example, when adding an account with an earlier birthday) and then truncate_to_chain_state is called again with a *greater* height, such that a discontinuity would be introduced in the subtree roots by the chain state insertion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
nuttycom
force-pushed
the
bug/rewind_to_chain_state
branch
from
ad7b729 to
76913b5
Compare
…anned height. `Shardtree` will return an error if insertion results in a discontinuity in subtree roots. When adding an account after truncation, this can cause the insertion to fail.
nuttycom
force-pushed
the
bug/rewind_to_chain_state
branch
from
76913b5 to
eb3ea65
Compare
daira
reviewed
Mar 30, 2026
daira
reviewed
Mar 30, 2026
daira
reviewed
Mar 30, 2026
daira
previously approved these changes
Mar 30, 2026
Contributor
daira
left a comment
daira
left a comment
There was a problem hiding this comment.
Inaccurate comment which should be fixed, but I will unblock this now.
2 tasks
8 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Previously, the computation of the block height to which we would truncate to allow room for the rewind-to checkpoint would fail if the truncation height didn't have an entry in the blocks table. This fixes that problem.
Fixes #2247