test: proof-length byte is mis-encoded for proofs of 16+ nodes - #178
Open
boleklebovski wants to merge 1 commit into
Open
test: proof-length byte is mis-encoded for proofs of 16+ nodes#178boleklebovski wants to merge 1 commit into
boleklebovski wants to merge 1 commit into
Conversation
`0x0${n.toString(16)}` only yields a byte for n < 16; ZkTrieVerifier.spec
already uses padStart.
Signed-off-by: boleklebovski <160799963+boleklebovski@users.noreply.github.com>
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
1 participant
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.
Problem
hardhat-test/PatriciaMerkleTrieVerifier.spec.ts:130,132builds the proof-length byte by hand:The literal
0plus a variable-width hex string only adds up to one byte while the length is below 16. From 16 onwardstoString(16)returns two characters and the template becomes0x010— three hex digits, an odd-length byte string thatconcatrejects.Every current fixture has fewer than 16 proof nodes, so the suite passes and the defect is invisible. It would surface the first time someone adds a fixture with a deeper proof, and it would look like a contract bug rather than a test-harness bug.
Fix
Use the same padding the sibling spec already uses.
hardhat-test/ZkTrieVerifier.spec.tsdoes this at four sites (lines 284, 286, 296, 298):Two lines changed, bringing this file in line with its neighbour.
Verification
npx hardhat testruns in CI (.github/workflows/contracts.yml:139), so this file is live rather than dormant. The change is behaviour-preserving for every existing fixture: for any length below 16 both forms produce identical output.