ProgressiveMerkleHasher and progressive container support - #43
ProgressiveMerkleHasher and progressive container support#43michaelsproul wants to merge 34 commits into
Conversation
Co-authored-by: michaelsproul <4452260+michaelsproul@users.noreply.github.com>
Co-authored-by: michaelsproul <4452260+michaelsproul@users.noreply.github.com>
Co-authored-by: michaelsproul <4452260+michaelsproul@users.noreply.github.com>
Co-authored-by: michaelsproul <4452260+michaelsproul@users.noreply.github.com>
… stream in Co-authored-by: michaelsproul <4452260+michaelsproul@users.noreply.github.com>
…er method Co-authored-by: michaelsproul <4452260+michaelsproul@users.noreply.github.com>
…inary tree hashing Co-authored-by: michaelsproul <4452260+michaelsproul@users.noreply.github.com>
macladson
left a comment
There was a problem hiding this comment.
I'm going to work on flipping the tree structure to the new spec
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
==========================================
+ Coverage 91.36% 92.49% +1.13%
==========================================
Files 7 8 +1
Lines 463 626 +163
==========================================
+ Hits 423 579 +156
- Misses 40 47 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /// Number of chunks written to the current hasher. | ||
| current_level_chunks: usize, | ||
| /// Buffer for bytes that haven't been completed into a chunk yet. | ||
| buffer: Vec<u8>, |
There was a problem hiding this comment.
I think this being a Vec here is probably pretty inefficient so when we are looking at doing optimizations later we should consider changing how we handle the buffer
| // Check if current level is complete | ||
| if self.current_level_chunks == self.current_level_size { | ||
| // Move to next level (4x larger) | ||
| let next_level_size = self.current_level_size * 4; |
There was a problem hiding this comment.
This technically could overflow, we could use checked_mul instead and add a new error variant for the overflow, but practically the number of levels would you need to trigger this is enormous
| // Process any remaining bytes in the buffer as a final chunk | ||
| if !self.buffer.is_empty() { | ||
| let mut chunk = [0u8; BYTES_PER_CHUNK]; | ||
| chunk[..self.buffer.len()].copy_from_slice(&self.buffer); |
There was a problem hiding this comment.
This isn't really an issue but just noting that this will panic in cases where self.buffer.len() > BYTES_PER_CHUNK. This is guaranteed to be impossible since write will always return leaving buffer < BYTES_PER_CHUNK.
Previously the consistency check between variant-level `tree_hash` and `ssz` attributes compared the whole parsed `VariantOpts` structs. Since both parsers tolerate unknown keys, an attribute that is present but does not set `selector` (e.g. one carrying only ssz-specific keys) would parse as `selector: None` and spuriously fail the consistency assertion against the other attribute's explicit selector. Merge the two attributes field-by-field instead, asserting consistency only when both actually set a selector. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The variant checks in the transparent and union derive paths only
verified the field *count*, so a single named-field variant like
`A { x: u8 }` passed the check and then failed with a confusing
"expected tuple struct" error in the generated match pattern.
Check explicitly for a single unnamed field and panic with a proper
message naming the offending variant.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Regular unions already panic with "0-variant union is not permitted"
(via `compute_union_selectors`), but a 0-variant compatible union slipped
through selector validation and instead failed on the generated
`match self {}`, which is not exhaustive for `&Self` even when `Self` is
uninhabited. Panic with the same message as the regular union path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A `selector` attribute on a variant of a transparent enum was silently ignored, because `parse_variant_opts` was only called from the union derive path. Transparent enums never mix in a selector, so a manual one is a configuration error: reject it in the same way that the regular "union" behaviour already does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This PR contains all of the
tree_hashchanges required to support EIP-7916 and EIP-8016:ProgressiveMerkleHasher: an analog ofMerkleHasher, it has a lazy API where it can ingest bytes and hash them incrementally. It uses aMerkleHasherunder the hood to do the hashing of the binary subtrees.TreeHashimplementation forBitfield<Progressive>fromethereum_ssz.struct_behaviour = "progressive_container". This generates code that uses aProgressiveMerkleHasherfor the fields. It comes with a new attributeactive_fields({0,1},+)which is used to insert zero hashes for inactivated fields. The derive macro also generates a 32-byte array representing the active fields as a bitfield (at compile time), and generates code to mix this bitfield into the root, following the spec.enum_behaviour = "compatible_union". Compatible unions behave quite similarly to the existing (and now deprecated) union type, with the difference being that their g-indices and merkle tree structure is stable across all enum variants. For now the macro does not verify that enum variants are compatible, because this verification is not mandatory (it is safe if the only compatible unions happen to be compatible, which the spec should ensure). Verification is left for a follow-up PR: Compatible union validation ethereum_ssz#72.tree_hash(selector = "1")which can set the selector for compatible unions only. To avoid duplication withethereum_sszthis selector is read from thesszattribute, or thetree_hashattribute (they must be consistent if both are set). See discussion: Progressive data structures and tests EIP-7916, EIP-8016 lighthouse#8505 (comment).Depends on: