Skip to content

fix(trie): pull lone sibling leaf up on delete so the state-trie root is history-independent#823

Merged
github-actions[bot] merged 1 commit into
mainfrom
fix/canonical-smt-delete
Jun 21, 2026
Merged

fix(trie): pull lone sibling leaf up on delete so the state-trie root is history-independent#823
github-actions[bot] merged 1 commit into
mainfrom
fix/canonical-smt-delete

Conversation

@satyakwok

Copy link
Copy Markdown
Member

Problem

SentrixTrie::delete() only collapsed (empty, empty) internal nodes. It never pulled a lone sibling leaf back up to its canonical short-circuit depth. So after deleting one of two keys that shared a prefix (and were therefore pushed deep on insert), the surviving leaf was left at its deep position instead of returning to the shallow position a direct insert produces.

Consequence: the SMT root became a function of insert/delete history, not of the current key→value set. insert() is already canonical (push-down is deterministic), so the asymmetry was delete-only.

This is the root cause of the long-running cross-validator state_root divergence:

  • The apply path deletes trie keys — blockchain_trie_ops.rs deletes an account when balance == 0 and a validator's pending_rewards entry when it reaches 0 on a claim (frequent on a reward-claiming network).
  • Validators that applied different block subsets (the #1e-mismatch skip-local-write path) accumulated different delete/insert histories → structurally different tries for byte-identical account state → different computed state_root.
  • That is why cp / treasury-rebase convergence never held: those only equalize values, not the history-dependent structure. Two nodes with the same accounts still computed different roots.

Repro

crates/sentrix-trie/tests/delete_canonical_repro.rs — with A and B sharing a 255-bit prefix:

  • {A} via insert A vs {A} via insert A, insert B, delete B produce different roots on the current code (legacy), and identical roots once delete is canonical.

Fix

delete() Phase 2 is rewritten as an Up { Empty, Leaf, Internal } walk-up. When canonical-delete is on, an empty side with a lone sibling leaf pulls that leaf up to the shallower depth (and keeps pulling while the sibling stays empty), restoring the exact shape a direct insert yields. The leaf node itself is unchanged (its hash is depth-independent), so no node is rewritten.

Fork gate

CANONICAL_DELETE_HEIGHT (default u64::MAX on both nets). The legacy branch is byte-identical to the original, so pre-fork blocks reproduce historical roots exactly. update_trie_for_block sets the per-block flag from the >= predicate. Inert until an activation height is pinned — this PR changes nothing at runtime when merged.

Activation is a separate, coordinated step (testnet first): the fleet must be value-converged at the fork height (canonical-delete makes the root history-independent going forward; it does not retro-heal already-diverged state), then halt-all + state-root-alignment pre-flight + CANONICAL_DELETE_HEIGHT=<tip+lead> + simul-start + bake, then mainnet — same discipline as the speculative-apply fork (#814).

Tests

  • cargo test -p sentrix-trie: all 84 existing lib tests pass (legacy path unchanged) + 2 new repro tests.
  • cargo check -p sentrix-core -p sentrix-fork-heights clean with -D warnings.

Relation to #814

Once this is active and the fleet is converged, nodes compute identical roots, which unblocks the SIP-5 speculative-apply verify-before-vote (#814) — it can only pass when validators agree on the computed state_root.

… is history-independent

SentrixTrie::delete only collapsed (empty,empty) nodes; it never pulled a
surviving sibling leaf back up to its canonical short-circuit depth. So after
deleting one of two keys that shared a prefix (and were pushed deep on insert),
the survivor was left at its deep position — making the SMT root a function of
insert/delete HISTORY, not the current key->value set. insert was already
canonical; the asymmetry was delete-only.

The apply path deletes keys (balance==0 accounts, pending_rewards==0 on a
claim), so validators that applied different block subsets (skip-local-write on
a #1e mismatch) ended up with structurally different tries for byte-identical
account state and computed different state_roots — the cross-validator drift
that cp / treasury-rebase convergence never held.

Post-fork, delete restores the canonical shape (pulls the lone sibling leaf up),
so the root is a pure function of state. Fork-gated by CANONICAL_DELETE_HEIGHT
(default u64::MAX on both nets) — pre-fork behaviour is byte-identical, so
historical roots stay reproducible. Inert until an activation height is set
after a halt-all + state-root-alignment pre-flight + simul-start, testnet first.

Regression test (crates/sentrix-trie/tests/delete_canonical_repro.rs): the root
of {A} is identical whether reached directly or via insert-B-then-delete-B in
canonical mode, and still differs in legacy mode. All 84 existing trie tests
pass; cargo check -p sentrix-core -p sentrix-fork-heights clean with -D warnings.
@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@satyakwok, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 19 minutes and 22 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 76f89e80-75e0-49bf-b782-42ff59edd644

📥 Commits

Reviewing files that changed from the base of the PR and between 457eee1 and 52e06be.

📒 Files selected for processing (5)
  • crates/sentrix-core/src/blockchain.rs
  • crates/sentrix-core/src/blockchain_trie_ops.rs
  • crates/sentrix-fork-heights/src/lib.rs
  • crates/sentrix-trie/src/tree.rs
  • crates/sentrix-trie/tests/delete_canonical_repro.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/canonical-smt-delete

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot enabled auto-merge (squash) June 21, 2026 03:12
@codecov

codecov Bot commented Jun 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.45455% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/sentrix-trie/src/tree.rs 70.45% 13 Missing ⚠️

📢 Thoughts on this report? Let us know!

@github-actions github-actions Bot merged commit e0f3573 into main Jun 21, 2026
18 of 19 checks passed
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.

1 participant