Check internal node keys during sliced tree recovery - #3
Open
Ergologica wants to merge 3 commits into
Open
Conversation
This was referenced Aug 13, 2026
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
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.
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.computeLabelhashes the balance and the two children labels, and nothing else:So the key of an internal node is not committed to by its label -
InternalProverNode.getNewKeysays as much. A manifest or a subtree therefore carries keys its own digest does not authenticate, and bothverify()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.checkTreealready enforces (min of right subtree doesn't match). NewNodeKeyChecks(package-private) provides two checks, applied where trust was previously extended: the complete one incombine, 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
deepInternalChainbuilt each internal node with key2i + 1, while its right subtree starts at2i + 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 use2i + 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. FullscryptoJVM/test- 19 suites, 171 tests, 0 failures.