Skip to content

Check internal node keys during sliced tree recovery - #3

Open
Ergologica wants to merge 3 commits into
ergoplatform:masterfrom
Ergologica:patch-1
Open

Check internal node keys during sliced tree recovery#3
Ergologica wants to merge 3 commits into
ergoplatform:masterfrom
Ergologica:patch-1

Conversation

@Ergologica

Copy link
Copy Markdown

Moving this here from input-output-hk#118, as discussed - that master is protected and the repo has not moved since March 2025.

The problem

InternalNode.computeLabel hashes the balance and the two children labels, and nothing else:

hf.hash(Array(InternalNodePrefix, b), left.label, right.label)

So the key of an internal node is not committed to by its label - InternalProverNode.getNewKey says as much. A manifest or a subtree therefore carries keys its own digest does not authenticate, and both verify() methods compare labels only.

This is worth stating explicitly against the current combine: matching subtrees by digest label, as the recent hardening does, is a strong guarantee about the shape and contents of a subtree, but it is blind to internal keys - two trees differing only in an internal key have identical labels. An adversary can hand over a subtree with an altered internal key and every existing check passes, after which the prover routes lookups down the wrong branch.

The approach

The issue offers two routes, dropping the keys or checking them. This checks them, since dropping them would change the serialization format and downstream consumers (ergo vendors its own ManifestSerializer / SubtreeSerializer) would have to move in lockstep. No wire format change here.

The keys do not need to be trusted because they are recomputable: the key of an internal node is the smallest key of its right subtree - the invariant BatchAVLProver.checkTree already enforces (min of right subtree doesn't match). New NodeKeyChecks (package-private) provides two checks, applied where trust was previously extended: the complete one in combine, once subtrees are attached and the tree is materialized, and an in-order ordering check for a manifest, whose proxy nodes cannot be descended into.

One change to an existing test

deepInternalChain built each internal node with key 2i + 1, while its right subtree starts at 2i + 2 - a tree that violates the invariant above. It went unnoticed because nothing checked internal keys; with this PR the height tests built on it fail. I changed the fixture to use 2i + 2, so it is a well-formed tree, and the height assertions keep testing what they were written to test.

Tests

Four new properties: a label does not commit to the key of an internal node; a subtree with a forged internal key does not verify; a manifest with an internal key out of order does not verify; combining a tree with a forged internal key fails.

scryptoJVM/testOnly ...AVLBatchSerializationSpecification - 37/37. Full scryptoJVM/test - 19 suites, 171 tests, 0 failures.

@Ergologica Ergologica changed the title Patch 1 Check internal node keys during sliced tree recovery Aug 13, 2026
@kushti kushti added this to the 3.1.2 milestone Aug 17, 2026
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