Skip to content

fix(protocol): reject a duplicate account ID prefix before inserting - #3867

Open
erkancamli wants to merge 2 commits into
0xMiden:nextfrom
erkancamli:fix/account-tree-duplicate-prefix
Open

erkancamli wants to merge 2 commits into
0xMiden:nextfrom
erkancamli:fix/account-tree-duplicate-prefix

Conversation

@erkancamli

Copy link
Copy Markdown

What

AccountTree::insert inserts into the SMT and only then asks whether the leaf now holds two entries:

let prev_value = self.smt.insert(key, state_commitment).expect(...);

// If the leaf of the account ID now has two or more entries, we've inserted a duplicate prefix.
if self.smt.get_leaf(&key).num_entries() >= 2 {
    return Err(AccountTreeError::DuplicateIdPrefix { duplicate_prefix: account_id.prefix() });
}

The error is returned, but the insert is never undone. The caller is told the operation failed while the tree has already changed: the root advanced, and the leaf now holds two entries, which the type documents as impossible ("Each account ID prefix must be unique").

Three places rely on that invariant with unreachable!(), so the next call on the same tree panics rather than erroring:

  • account_commitments(): let SmtLeaf::Single((key, commitment)) = leaf else { unreachable!("empty and multiple variant should never be encountered") }
  • compute_mutations(): SmtLeaf::Multiple(_) => unreachable!(...)
  • the same match in AccountTree::new

For a block producer that applies account updates one at a time, a single rejected duplicate-prefix update silently moves the account tree root away from what consensus expects and turns every later account_commitments or compute_mutations call on that tree into a process-killing panic. Recovery means rebuilding the tree.

Fix

Check the leaf before mutating, which is exactly what PartialAccountTree::insert in the sibling module already does:

if let SmtLeaf::Single((existing_key, _)) = self.smt.get_leaf(&key)
    && key != existing_key
{
    return Err(AccountTreeError::DuplicateIdPrefix { duplicate_prefix: account_id.prefix() });
}

Updating an existing account keeps working, since that path hits the same key. insert_fails_on_duplicate_prefix and insert_succeeds_on_multiple_updates are unchanged and still pass.

Test

insert_with_duplicate_prefix_does_not_mutate_tree reuses the existing setup_duplicate_prefix_ids() helper, inserts the first ID, takes the root, expects the second insert to fail, and then asserts the tree is untouched. The last assertion is the important one: account_commitments() panics on a multi-entry leaf, so before the fix the test does not merely fail an assert, it aborts.

before: thread ... panicked at crates/miden-protocol/src/block/account_tree/mod.rs:545
after:  test result: ok. 23 passed (account_tree)
        test result: ok. 545 passed; 0 failed (cargo test -p miden-protocol)

Notes

  • AssetVault::add_non_composable_asset has the same insert-then-report shape and is worth a look, but it is a different crate path and a different argument, so I left it out of this PR. Happy to send it separately.
  • I could not run the pinned 1.98.1 toolchain (my environment cannot reach static.rust-lang.org), so the numbers above are from stable 1.95 with --ignore-rust-version. cargo fmt --check on stable disagrees with this repo's nightly rustfmt config across the whole crate (build.rs included), so I matched the surrounding style by hand rather than trusting it. Please lean on CI for both.

AccountTree::insert wrote the leaf and only then rejected a duplicate
prefix, leaving the rejected entry behind and breaking the one entry per
leaf invariant that the accessors rely on.

Rebased onto next; the CHANGELOG entry now appends to the existing
Unreleased Fixes section instead of opening a second one.
@erkancamli
erkancamli force-pushed the fix/account-tree-duplicate-prefix branch from b2e0feb to 950ec2e Compare September 15, 2026 18:45
@erkancamli

Copy link
Copy Markdown
Author

Rebased onto next; the conflict is gone.

The cause was mine: all three of my open PRs opened a new ### Fixes section under ## Unreleased, and next has since grown its own. The entry now appends to the existing list instead, so the CHANGELOG diff here is a single added line. #3868 and #3869 are rebased the same way.

Separately, no workflow runs have been approved on any of the three, so there are no checks for a reviewer to go on. Would someone mind enabling them?

The if-let only covered SmtLeaf::Single, so a Multiple leaf fell through
and was inserted into silently. The base code's num_entries() >= 2 check
did catch that case, so the first revision was a small regression there.

Match all three variants the way compute_mutations already does a few
lines below. It reports Multiple as unreachable!; this returns
DuplicateIdPrefix instead, so a tree that somehow already holds one is
not grown further and the caller gets an error rather than a panic.

Also documents that the tree is left unmodified on error, which is the
whole point of the change.
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