fix(shielded-pool): bound the zero-hash ladder and the tree depth shift - #124
Merged
Merged
Conversation
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.
Verified by instantiating at 32 and confirming the error, then removing the probe. The assertion has to be referenced from
capacity()— an associated const is not evaluated where nothing uses it.On reachability
This is hardening of latent code, not a live hole. Every
levelat every call site comes from a0..depthloop bound byDEFAULT_TREE_DEPTH, so no external input selects one. What changes is where the bound lives: attached to the danger, rather than in a runtime config that a downstream caller of apubgeneric type never sees.Verification
-D warningsThe equivalence test is the one that matters. It compares the loop against a local recursive reference across levels 0–24, including past the cache boundary where the fall-through lives. Mutation-checked: breaking the loop by a single iteration (
0..level.saturating_sub(1)) makes it fail.The ladder E2E reads the digests back out of real Merkle paths served by the Wasm runtime — not computed natively — and matches all 19 levels against pinned values. It requires an empty tree and aborts with instructions otherwise, since on a populated tree those siblings are real subtrees and the comparison reports a divergence that is not one.
The adversarial E2E tries to break the node instead of confirming it:
u32::MAXleaf indices, the sign boundary, 64 KiB and non-hex commitments, a 50-request burst, and storage queries at levels 21 through 255 — checking block height after every batch. Everything is refused without the node hanging or stalling.That last one proves less than it looks. Tracing the callers showed the vector is not reachable from outside at all, so it confirms the surfaces reject garbage rather than demonstrating the fix works. The 1 MiB stack measurement above is what actually demonstrates it.
Corrections made while verifying
An earlier version of the test comment claimed 5,000 frames already overflowed 1 MB. That was never measured and is wrong — the threshold sits between 10,000 and 20,000. Corrected in both the test and the changelog with the measured figures.
Mid-measurement, a single
zero_hash_at_level(1)overflowed a 1 MB stack, which looked like something worse than this PR: one Poseidon call exhausting the runtime stack. It is a debug artifact — unoptimised arkworks leaves large structures on the stack, and release builds pass comfortably. Production runs release. Checked before reporting it as a finding.Scope
Six files: the two primitives, their tests, and the two dev-node runs. The
spec_versionbump ships separately.