Skip to content

shardtree: discard stale cap roots on truncation#147

Open
gustavovalverde wants to merge 1 commit into
zcash:mainfrom
gustavovalverde:fix/truncate-exact-boundary
Open

shardtree: discard stale cap roots on truncation#147
gustavovalverde wants to merge 1 commit into
zcash:mainfrom
gustavovalverde:fix/truncate-exact-boundary

Conversation

@gustavovalverde

@gustavovalverde gustavovalverde commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Checkpoint truncation could retain a cached cap hash covering positions after the target checkpoint. After inserting replacement commitments, root queries could return the pre-truncation root.

The cap is non-authoritative: shard data remains the source of truth. This change retains cache nodes wholly before the checkpoint and discards cached leaves and parent annotations that cross its boundary, forcing later root queries to recompute from the retained and replacement shards.

Shard and checkpoint truncation semantics are unchanged. The regression reproduces the relevant wallet sequence through public ShardTree operations: insert subtree roots, cache their combined root, scan to a valid checkpoint, truncate, and insert replacement commitments.

@nuttycom

nuttycom commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

@gustavovalverde this looks like it now conflicts with main

@gustavovalverde gustavovalverde changed the title shardtree: truncate exactly at an achievable boundary, never retaining data beyond the cut shardtree: truncate to an achievable boundary Jul 8, 2026
@gustavovalverde gustavovalverde force-pushed the fix/truncate-exact-boundary branch from 5e0a34b to ffcb132 Compare July 8, 2026 16:27
@gustavovalverde

Copy link
Copy Markdown
Contributor Author

@nuttycom rebased

@nuttycom

nuttycom commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

truncate_to_checkpoint could report success without changing the tree when the target position fell inside a pruned node. That left shards, cap roots, and checkpoints retaining hashes that committed to data beyond the requested cut, so a caller could rescan into deterministic InsertionError::Conflict failures.

This sounds to me like a problem in the pruning logic, not one that we should work around by truncating to a different point than the one that was requested. It should always be possible to truncate to exactly any checkpoint leaf; if that invariant isn't preserved by some paths, then the correct option is to find that bug rather than try to work around it.

@nuttycom nuttycom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking on figuring out how it's possible to get into the described failure state in the first place. Checkpoints should be allowed to be aggregated up the tree only in the case that a checkpoint corresponds to the right-hand leaf of some filled subtree. If there is a path to a checkpoint being constructed (or retained after pruning) for a pruned leaf, that's definitely a bug.

@gustavovalverde gustavovalverde force-pushed the fix/truncate-exact-boundary branch from ffcb132 to af93c7c Compare July 9, 2026 07:52
@gustavovalverde gustavovalverde changed the title shardtree: truncate to an achievable boundary shardtree: reject inexact checkpoint truncation Jul 9, 2026
@gustavovalverde gustavovalverde force-pushed the fix/truncate-exact-boundary branch from af93c7c to 48b5297 Compare July 9, 2026 08:17
@gustavovalverde gustavovalverde requested a review from nuttycom July 9, 2026 08:37
@gustavovalverde

Copy link
Copy Markdown
Contributor Author

@nuttycom the fix is now more targeted. The PR body was updated alongside the changelog to represent the latest code

@gustavovalverde gustavovalverde force-pushed the fix/truncate-exact-boundary branch from 48b5297 to ce08286 Compare July 12, 2026 21:23
@gustavovalverde gustavovalverde changed the title shardtree: reject inexact checkpoint truncation shardtree: discard stale cap roots on truncation Jul 12, 2026
@gustavovalverde

Copy link
Copy Markdown
Contributor Author

Thanks, you were right: the earlier tests manufactured invalid checkpoints and did not establish a real path to that state. I removed that behavior and narrowed the PR to the stale cap cache issue.

The regression now uses only a valid checkpoint and public operations: import 2 shard roots, cache their combined hash, scan the first shard, truncate at its checkpoint, then insert a replacement second shard. On main, the final root incorrectly remains abcdefghijklmnop...; with this patch it becomes abcdefghIJKLMNOP....

The fix does not change shard or checkpoint truncation semantics. It only discards cap nodes and annotations that commit beyond a successful truncation boundary, so later roots recompute from authoritative shard data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants