Skip to content

ProgressiveMerkleHasher and progressive container support - #43

Open
michaelsproul wants to merge 34 commits into
mainfrom
progressive
Open

ProgressiveMerkleHasher and progressive container support#43
michaelsproul wants to merge 34 commits into
mainfrom
progressive

Conversation

@michaelsproul

@michaelsproul michaelsproul commented Dec 10, 2025

Copy link
Copy Markdown
Member

This PR contains all of the tree_hash changes required to support EIP-7916 and EIP-8016:

  • ProgressiveMerkleHasher: an analog of MerkleHasher, it has a lazy API where it can ingest bytes and hash them incrementally. It uses a MerkleHasher under the hood to do the hashing of the binary subtrees.
  • TreeHash implementation for Bitfield<Progressive> from ethereum_ssz.
  • Derive macro support for struct_behaviour = "progressive_container". This generates code that uses a ProgressiveMerkleHasher for the fields. It comes with a new attribute active_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.
  • Derive macro support for 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.
  • As part of compatible union support, we add a new variant attribute tree_hash(selector = "1") which can set the selector for compatible unions only. To avoid duplication with ethereum_ssz this selector is read from the ssz attribute, or the tree_hash attribute (they must be consistent if both are set). See discussion: Progressive data structures and tests EIP-7916, EIP-8016 lighthouse#8505 (comment).

Depends on:

Copilot AI and others added 12 commits December 8, 2025 02:50
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 macladson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to work on flipping the tree structure to the new spec

Comment thread tree_hash/src/lib.rs Outdated
@codecov

codecov Bot commented Feb 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.54545% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.49%. Comparing base (adef128) to head (03d9fa4).

Files with missing lines Patch % Lines
tree_hash/src/progressive_merkle_hasher.rs 86.79% 7 Missing ⚠️
tree_hash/src/merkle_hasher.rs 0.00% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

/// 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>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

macladson and others added 8 commits June 30, 2026 02:56
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants