chore: merkle tree audit 2#21475
Conversation
ledwards2225
left a comment
ledwards2225
left a comment
There was a problem hiding this comment.
LGTM just a few things to resolve before merging
|
|
||
| // TODO(#17755): This does not consider maximum leaf index and will wrap around to give incorrect values. | ||
| // e.g. if leaf_index = maximum + 1, returns the leaf at index + 1. See #17684 | ||
| // Improvement: For out-of-range reads, define the desired native/TS behavior (e.g. |
There was a problem hiding this comment.
Does the throw above effectively resolve this issue?
There was a problem hiding this comment.
It resolved only the bounds off-by-one issue earlier. The remainder is now resolved by documenting the current behaviour as the intended behaviour.
| // TODO(#17755): Throw error to world state -> TS? (native_world_state_instance.ts -> call() | ||
| // translates this to null) | ||
| if (max_size_ <= leaf_index) { | ||
| // Improvement: clarify the contract for reads beyond tree capacity and align native/TS handling |
There was a problem hiding this comment.
Can you try to get clarity on this from Miranda (or Phil?) then resolve one way or the other? Seems we shoudl be able to make a decision if this is production code? Same with the other TODOs
There was a problem hiding this comment.
I discussed with Phil and we decided to document the current behaviour in code as the intended behaviour and close the TODO. I’ve updated the comments in the code accordingly and closed the TODOs.
| return; | ||
| } | ||
| if (blockData.size < leaf_index) { | ||
| if (blockData.size <= leaf_index) { |
|
|
||
| Repository: https://github.com/AztecProtocol/aztec-packages | ||
| Commit hash: TBD (link) | ||
| Commit hash: 158dd845c99f8f702979c20f1625730d126c4b20 |
There was a problem hiding this comment.
Can you update this to:
| Commit hash: 158dd845c99f8f702979c20f1625730d126c4b20 | |
| Commit hash: Most recent commit on branch 'next' |
There was a problem hiding this comment.
Updated with the latest commit from next
nishatkoti
force-pushed
the
nk/merkle-tree-two
branch
from
b834830 to
46a9859
Compare
…ntAddressedAppendOnlyTree.hpp
nishatkoti
force-pushed
the
nk/merkle-tree-two
branch
from
46a9859 to
40e2604
Compare
BEGIN_COMMIT_OVERRIDE fix: reject VK with log_circuit_size=0 in UltraKeccak verifier (#22319) chore: merkle tree audit 2 (#21475) fix: graceful failures in verifier code paths + other fixes (#22311) fix: Fr::from_u64 big-endian encoding to match C++ msgpack format (#22233) fix: corrupt low-order bytes in batch verifier test to avoid non-canonical field encoding (#22333) fix: skip MsgpackRejectsNonCanonical test in WASM builds (#22335) END_COMMIT_OVERRIDE
2 participants
content_addressed_append_only_tree.hpp:leaf_index >= max_size_→ failure (out of tree range)leaf_index < max_size_but leaf not written → return 0 withsuccess = trueleaf_index >= blockData.size→ failure (out of block range)leaf_index < blockData.sizebut leaf not written → return 0 withsuccess = true