shardtree: discard stale state when truncating#196
Open
nuttycom wants to merge 4 commits into
Open
Conversation
`LocatedPrunableTree::truncate_to_position` and its callers mishandle two situations in which the tree's pruned representation interacts badly with the truncation position: 1. The parent nodes reconstructed along the truncation path retain their cached root annotations. Such an annotation is a hash over the parent's complete subtree — including the data being truncated away — so a stale annotation later causes a spurious `Insert(Conflict(..))` (or a silently incorrect root) when the truncated region is refilled with different data, as happens when a wallet re-scans a divergent chain after a reorg. Annotations of this shape are routinely written by `insert_frontier`, which is invoked with the chain state at the start of every scanned batch. 2. When the truncation position falls in the interior of a pruned subtree (a merged hash node), `truncate_to_position` returns `None` — correctly, since truncating there would have to discard retained data along with the requested suffix — but `truncate_to_checkpoint` reacts to the refusal by silently skipping the truncation while still reporting success, leaving all post-checkpoint state in place. (Before the preceding cap-truncation fix, the cap had already been modified at that point, additionally leaving the store internally inconsistent.) The annotation tests and the `truncate_to_checkpoint` coherence test are RED at this commit; `truncate_to_position_fails_inside_pruned_subtree` passes, pinning the refusal contract that the fix must preserve. The fix follows in the next commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`LocatedPrunableTree::truncate_to_position` now guarantees that nothing
derived from the discarded suffix of the tree survives a truncation
that it performs, and refuses truncation at any position the tree does
not individually represent:
- A subtree lying entirely at or below the truncation position is
retained unmodified (including any cached root annotation). This
check is now made once at the top of the recursion, generalizing the
check previously made only for pruned leaf nodes.
- Parent nodes whose subtrees lose data in the truncation are
reconstructed without their cached root annotations: such an
annotation is a hash over the parent's complete subtree, which by
construction of the truncation path includes discarded data.
- Truncation at a position in the interior of a pruned subtree (or at
which the tree holds no data) is refused, as before; it is the
caller's responsibility to select a position at which truncation is
possible.
`ShardTree::truncate_to_checkpoint{_depth}` now handle the refusal by
failing with `QueryError::CheckpointPruned`, leaving the store
unmodified, instead of silently skipping the truncation while still
reporting success.
The cap remains truncated with `truncate_cache_to_position`,
introduced by the preceding cap-truncation fix: the cap is a cache —
every node in it is a hash recomputable from the shards below — so a
cached entry spanning the truncation boundary is discarded rather
than making the position unusable. With refusal now handled up front,
the cap, shard, and checkpoint updates are performed unconditionally.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
2 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.
Builds upon #147: this branch includes @gustavovalverde's commits fixing stale cached cap roots on truncation, and extends them to fix two further defects in checkpoint truncation discovered while testing wallet reorg recovery in librustzcash. Merging this PR lands the entire change as a single merge, and GitHub will automatically mark #147 as merged once its commits are reachable from
main.The defects fixed on top of #147:
Stale cached annotations survived truncation inside shards.
LocatedPrunableTree::truncate_to_positionreconstructed the parent nodes along the truncation path with their cached root annotations intact. Such an annotation is a hash over the parent's complete subtree — including the data being truncated away — so a retained annotation caused a spuriousInsert(Conflict(..))error (or a silently incorrect root) once the truncated region was refilled with different data. Annotations of this shape are routinely written byinsert_frontier, which wallets invoke with the chain state at the start of every scanned batch, so any wallet that rewinds past a scan-batch boundary and then re-scans a divergent (post-reorg) chain can hit this.Truncation at a position the tree does not individually represent was silently skipped. When the truncation position fell in the interior of a pruned subtree (a merged hash node),
truncate_to_positionreturnedNone, andtruncate_to_checkpointreacted by skipping the truncation while still reporting success. (On shardtree 0.7.0, before shardtree: discard stale cap roots on truncation #147's fix, the cap had already been truncated at that point, additionally leaving the store internally inconsistent.)The fix:
truncate_to_positioncontinues to refuse truncation at a position the tree does not individually represent: succeeding would require discarding retained data along with the requested suffix.ShardTree::truncate_to_checkpointandtruncate_to_checkpoint_depthnow surface that refusal as aQueryError::CheckpointPrunederror, leaving the store unmodified, instead of reporting success.truncate_cache_to_position: the cap is a cache of hashes recomputable from the shards below, so a cap entry spanning the truncation boundary is discarded rather than making the position unusable. With refusal handled up front, the cap, shard, and checkpoint truncation now proceed unconditionally.The first commit on top of #147 adds regression tests (red at that commit); the second contains the fix. One of the tests reproduces the wallet-shaped failure entirely through the public API: append leaves, insert a batch-boundary frontier, truncate to an earlier checkpoint, then insert a frontier carrying divergent replacement data — which previously failed with
Insert(Conflict(..)).An earlier revision of this change was also validated against librustzcash by path-patching it into its workspace: a wallet-level test that re-mines a transaction on a divergent chain after
rewind_to_chain_statefails against shardtree 0.7.0 withPutBlocksCommitmentTree { error: Insert(Conflict(..)) }whenever earlier scan batches had written frontier ommers above the rewind target, and passes with the fix. (That revision differed from this one only in the handling of non-representable truncation positions; the wallet scenario truncates to a representable position, for which the behavior is identical.)Supersedes and closes #194.
🤖 Generated with Claude Code