[MegaRedHand]

🗒️ Description / Motivation

Every inner node of the beacon state's merkle tree is SHA-256 of exactly 64 bytes: one data block plus one constant padding block. Going through Digest per pair buffers, pads and finalizes generically for each of the millions of pairs per state root. This hashes whole layers through sha2::compress256 instead.

What Changed

• crates/common/ssz-tree/src/batch.rs (new): hashes a whole layer with sha2::compress256 directly (initial state, the pair, the fixed padding block, read the state out). sha2 still selects its hardware backend at run time, so the same SHA extensions are used.
• crates/common/ssz-tree/src/tree.rs: fold_roots folds each level in place through it; leaf roots (packed chunks and composite element roots) use a local merkleize built on the same primitive. The rayon fan-out above PARALLEL_HASH_HEIGHT is unchanged. Element roots of composite values still come from libssz.
• crates/common/ssz-tree/Cargo.toml: enables the compress feature of the sha2 already in the lockfile (compress256 is gated behind it). No new crate.

Correctness / Behavior Guarantees

• Output is byte-identical to hash_nodes(&Sha2Hasher, ..); no root changes, so no consensus or storage change.

Tests Added / Run

• New tests in batch.rs: batched pairs and odd/even layers against hash_nodes, and a proptest of merkleize against libssz's. batch_bench (ignored, run with cargo test -p ethlambda-ssz-tree --profile release-fast -- --ignored --nocapture batch_bench) times old against new on a 2^20 layer fold, a fresh List<u64> root and a fresh List<Validator> root.
• cargo fmt --all -- --check: clean
• make lint: clean
• cargo test --profile release-fast -p ethlambda-ssz-tree -p ethlambda-types -p ethlambda-state-transition --lib --bins: ssz-tree 75 passed (1 ignored: batch_bench), state-transition 630 passed (15 ignored: BLS/KZG fixture vectors), types 232 passed
• Not run locally: make test (spec suites); CI covers them.

Related Issues / PRs

Part of a beacon import perf batch, each PR independent and based on main: #752 (validation contention metrics). No file overlap with the other batch PRs.

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran make test (test-consensus plus test-node, at release-fast) — not run locally (spec suites); unit tests of the touched crates pass

[github-actions]

🤖 Claude Code Review

Review: PR 753, batched SHA-256 merkle layers

Verdict: Looks correct and well-contained. I found nothing blocking. I read the diff only and did not build it or run the tests.

Correctness (checked by hand)

• PADDING_BLOCK is right for a 64-byte message. 0x80 is the terminator, and the 512-bit length is 0x0200 big-endian in bytes 62–63. Only block[62] = 0x02 needs setting.
• The state is read out as big-endian words, and the IV constants match FIPS 180-4. compress256 with two blocks is equivalent to Digest over 64 bytes.
• The in-place fold in hash_layer is safe. Chunk output indices [done, done+n) always sit below the next read start 2*(done+n), so unread inputs are never overwritten. The odd-layer case pads with zero and then truncates to pairs. This is the same as the old fold_roots loop.
• merkleize handles the edge cases:
  • Empty input returns ZERO_HASHES[depth].
  • A single chunk gives real_depth = 0, so the chunk is lifted straight through the zero hashes.
  • limit of 0 or 1 gives depth = 0.
  • The proptest compares against libssz_merkleize for varying limit, which covers these paths.
• The tests cover odd and even layers and chunk-boundary sizes (255/256/257). That is good coverage for the scratch-chunking logic.

Minor, non-blocking

1. batch.rs, hash_pair: it copies both inputs into a temporary [Hash256; 2] for each call. The tail loop in merkleize calls it up to depth - real_depth times. For large limits like 1 << 40 that is about 40 calls, which is negligible. Mention it only if you later add a lane-based backend, since a single pair gets no batching benefit.
2. batch.rs, merkleize: chunks.to_vec() allocates one copy per leaf root. That is fine, and it is the same cost the libssz version paid internally.
3. hash_pairs_into has a #[cfg(test)] global LEGACY atomic. It only matters in the ignored benchmark, but if that benchmark is run alongside other tests in the same process, the toggle could briefly route them through the legacy path. The results would still be correct, because both paths are byte-identical, so this is harmless. The comment on LEGACY could say so.
4. Cargo.toml: the compress feature of sha2 exposes compress256. Confirm that sha2 is pinned in [workspace.dependencies] and that the make cooldown-check age rule is satisfied. The lockfile change is only the new edge, so it should be.
5. The module doc claims the output is byte-identical to hash_nodes(&Sha2Hasher, ..). That claim is verified by tests. Consider adding a test of batch::merkleize against libssz_merkleize with a large limit (e.g. 1 << 40) to cover the virtual-padding tail loop at realistic depth. The current proptest only shifts the limit by up to 2 bits (extra is 0–3).

No security or consensus concerns. Hash outputs are unchanged, so state roots remain compatible.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Codex Code Review

Looks good overall. I did not find a correctness or security flaw in the new SSZ Merkle batching path, and the added property tests are a strong fit for consensus-critical hashing code.

Notes

• The change is scoped to crates/common/ssz-tree, so it does not directly touch fork choice, attestation validation, justification/finalization, block processing, or XMSS paths.
• The batching logic in crates/common/ssz-tree/src/batch.rs:56, crates/common/ssz-tree/src/batch.rs:91, and crates/common/ssz-tree/src/batch.rs:122 is consistent with SHA-256 over fixed 64-byte inputs and appears byte-equivalent to the old hash_nodes / merkleize behavior.
• The in-place fold in crates/common/ssz-tree/src/batch.rs:104 is safe: each output chunk is computed from unread input ranges, then copied back to a lower index range only after hashing.
• The integration points in crates/common/ssz-tree/src/tree.rs:108, crates/common/ssz-tree/src/tree.rs:110, and crates/common/ssz-tree/src/tree.rs:434 preserve the previous tree semantics while removing repeated per-pair digest overhead.

Potential follow-up

• Consider adding one more regression test around non-power-of-two limit values for crates/common/ssz-tree/src/batch.rs:122. The current implementation intentionally normalizes with next_power_of_two, which matches typical SSZ usage here, but an explicit test would better lock in that assumption.

Validation

• I could not run cargo test in this environment because rustup tried to write under a read-only home path, failing before compilation. Static review of the patch did not reveal actionable issues.

If you want, I can also turn this into a ready-to-paste GitHub review comment.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[github-actions]

🤖 Kimi Code Review

I'll review this PR which introduces batched SHA-256 hashing for SSZ Merkle tree operations. This is a performance optimization that bypasses the standard Digest API to use sha2::compress256 directly.

Overall Assessment

The optimization is sound in principle—using the block-level compression function for 64-byte messages (one data block + one constant padding block) is a well-known optimization. However, there are several correctness, safety, and code quality issues to address.

---

Critical Issues

1. Endianness Bug in hash_pairs_into (Line 88-90)

for (word, bytes) in state.iter().zip(out.chunks_exact_mut(4)) {
    bytes.copy_from_slice(&word.to_be_bytes());
}

Problem: compress256 operates on u32 state in native endianness (the sha2 crate's internal representation), but the output must be big-endian bytes per FIPS 180-4. The sha2 crate's compress256 takes GenericArray<u32, U8> for state and expects/produces native-endian u32 values. However, when reading the final state as bytes, you need big-endian output.

Wait—let me re-check: Actually, word.to_be_bytes() converts to big-endian, which is correct for SHA-256 output. The state words are stored natively in u32, and to_be_bytes() properly converts. This looks correct.

Actually, re-verifying: The sha2 crate's compress256 function signature is:

pub fn compress256(state: &mut [u32; 8], blocks: &[GenericArray<u8, U64>])

The state is updated in-place as native u32 values. Converting each u32 to to_be_bytes() produces the correct big-endian byte output. This is correct.

2. Padding Block Construction (Lines 54-59)

const PADDING_BLOCK: [u8; 64] = {
    let mut block = [0u8; 64];
    block[0] = 0x80;
    block[62] = 0x02;
    block
};

Problem: The length encoding is incorrect for a 64-byte message.

For a 64-byte message (512 bits), the length in bits is 0x200 (512 in decimal = 0x0200 in hex). In the final 8 bytes of the padding block (big-endian u64 at bytes [56..64]), this should be:

• Byte 56: 0x00
• Byte 57: 0x00
• Byte 58: 0x00
• Byte 59: 0x00
• Byte 60: 0x00
• Byte 61: 0x00
• Byte 62: 0x02
• Byte 63: 0x00

The current code sets block[62] = 0x02 but leaves block[63] = 0x00. This is actually correct! 0x0200 in big-endian is [0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00], so byte 62 = 0x02 and byte 63 = 0x00.

Wait—let me re-verify: 512 decimal = 0x200 hex. As big-endian u64: 0x0000_0000_0000_0200. Bytes: [0, 0, 0, 0, 0, 0, 2, 0]. So index 62 = 2, index 63 = 0. This is correct.

3. Critical: blocks Array Reuse Bug (Lines 82-86)

let mut blocks = [padding, padding];
blocks[0][..32].copy_from_slice(&pair[0]);
blocks[0][32..].copy_from_slice(&pair[1]);
let mut state = SHA256_IV;
compress256(&mut state, &blocks);

Problem: blocks contains [padding, padding]—two identical copies of the padding block. But blocks[0] is modified to contain the data, while blocks[1] remains the padding. This is correct for a 64-byte message: one data block, one padding block.

However, there's a subtle issue: padding is GenericArray<u8, U64>, and blocks is [GenericArray<u8, U64>; 2]. The clone_from_slice creates two independent copies. Then blocks[0] is modified. This is fine.

But wait: Looking more carefully at line 81:

let padding = GenericArray::<u8, U64>::clone_from_slice(&PADDING_BLOCK);

This creates one GenericArray, then let mut blocks = [padding, padding]; copies it twice. Then blocks[0] is modified. Since GenericArray implements Copy (arrays of u8 do), this creates two independent copies. This is correct.

However, there's a performance issue: this allocates padding and blocks inside the hot loop, on every iteration. The padding block is constant and could be hoisted, and blocks could be reused or constructed more efficiently.

---

High Priority Issues

4. Unnecessary Allocation in Hot Loop (Lines 81-86)

let padding = GenericArray::<u8, U64>::clone_from_slice(&PADDING_BLOCK);
for (pair, out) in input.chunks_exact(2).zip(output.iter_mut()) {
    let mut blocks = [padding, padding];
    // ...
}

Problem: padding is cloned from slice on every loop iteration. PADDING_BLOCK is a const [u8; 64]. You can create GenericArray directly without cloning:

const PADDING_GENERIC: GenericArray<u8, U64> = GenericArray::from_array(PADDING_BLOCK);

Actually, GenericArray doesn't have from_array in const context easily. But you can at least hoist the padding creation:

let padding = GenericArray::<u8, U64>::from_slice(&PADDING_BLOCK).clone();
// or better, define PADDING_BLOCK as GenericArray directly

Recommendation: Define PADDING_BLOCK as GenericArray<u8, U64> directly, or use GenericArray::default() and fill it, or use a static/const of the right type.

5. Missing debug_assert_eq for hash_pair Optimization

pub(crate) fn hash_pair(left: &Hash256, right: &Hash256) -> Hash256 {
    let mut out = [[0u8; 32]];
    hash_pairs_into(&[*left, *right], &mut out);
    out[0]
}

This allocates a 2-element array on the stack for input and 1-element for output. For a single hash, the overhead of hash_pairs_into (including the assert_eq! and loop setup) may not be worth it. Consider a direct path, or at minimum use debug_assert_eq in hash_pairs_into to remove the check in release builds.

Actually, looking at hash_pairs_into line 68:

assert_eq!(input.len(), 2 * output.len(), "a pair per output");

This should be debug_assert_eq! since it's a pure internal function with correct callers.

6. Test-Only Code in Production Path (Lines 69-78)

#[cfg(test)]
if tests::LEGACY.load(std::sync::atomic::Ordering::Relaxed) {
    // ...
    return;
}

Problem: This is clever but fragile. The #[cfg(test)] attribute on the if statement means this code doesn't exist in non-test builds, but it creates a maintenance burden. The LEGACY static is pub(super) in tests module, which is unusual.

More importantly, this pattern means the function has two different implementations tested, but only one runs in production. The benchmark test batch_bench uses this to compare paths.

Recommendation: This is acceptable for benchmarking, but document clearly. Consider using a feature flag or separate function instead.

---

Medium Priority Issues

7. hash_layer In-Place Safety Comment (Lines 108-112)

// Output index `i` is written after input indices `2i` and `2i + 1` were
// read, and `i <= 2i`, so folding in place cannot overwrite an unread node.
// The borrow checker cannot see that through one slice, so the first
// `pairs` outputs go through a scratch copy of the hashes: a small, hot
// buffer, and cheaper than allocating a second layer.

The comment says i <= 2i which is true, but the actual condition needed is i < 2i for i > 0, or more precisely, that output index i doesn't overlap with input indices 2i and 2i+1 that haven't been read yet. For i=0, output[0] overwrites input[0] and input[1] after they've been read. This is correct.

However, the scratch buffer approach with CHUNK = 256 is conservative. For the in-place property to hold, you need done + n <= 2 * done, i.e., n <= done. The chunk size of 256 means this only works when done >= 256. But initially done = 0, so n would need to be 0.

Wait, re-reading: The scratch buffer is used for all iterations. The comment says "the first pairs outputs go through a scratch copy"—but actually the code uses scratch for all chunks, then copies back. So it's never truly in-place; it always goes through scratch. The comment is misleading.

Actually re-reading more carefully: The scratch is used for chunks of up to 256, then copied to layer[done..done+n]. Since done starts at 0 and increases, and we're reading from layer[2*done..], the read region is always ahead of the write region after the first copy. But with scratch, it's safe regardless.

The comment about "borrow checker cannot see that through one slice" is misleading because the actual reason for scratch is to enable the in-place optimization safely, but the code doesn't actually do in-place writing—it always uses scratch.

8. merkleize Empty Chunks Handling (Lines 137-140)

if chunks.is_empty() {
    return ZERO_HASHES[depth];
}

Problem: If chunks.is_empty() and limit == 0, then depth = 0.next_power_of_two().trailing_zeros() = 0, and ZERO_HASHES[0] is the hash of empty data. But limit == 0 might be invalid per SSZ spec. The assert! only checks chunks.len() <= limit, so limit = 0 with empty chunks passes.

Is limit = 0 valid? In SSZ, limit for merkleization is typically >= 1 for lists. This may be a spec edge case.

9. merkleize real_depth vs depth Logic (Lines 141-149)

let real_depth = chunks.len().next_power_of_two().trailing_zeros() as usize;
let mut layer = chunks.to_vec();
for zero in &ZERO_HASHES[..real_depth] {
    hash_layer(&mut layer, zero);
}
let mut root = layer[0];
for zero in &ZERO_HASHES[real_depth..depth] {
    root = hash_pair(&root, zero);
}

Problem: If chunks.len() is already a power of two, real_depth is correct. If chunks.len() = 1, real_depth = 1, and we do one hash_layer which pairs the single element with ZERO_HASHES[0]. Then we iterate ZERO_HASHES[1..depth] for remaining levels.

Wait: chunks.len().next_power_of_two() for len = 1 gives 1, trailing_zeros() = 0. So real_depth = 0. No hash_layer loop. Then root = layer[0], and we hash with ZERO_HASHES[0..depth].

For len = 2: next_power_of_two() = 2, trailing_zeros() = 1. real_depth = 1. One hash_layer with ZERO_HASHES[0], which pairs the two elements. Then ZERO_HASHES[1..depth].

This seems correct. But let's check len = 3: next_power_of_two() = 4, trailing_zeros() = 2. real_depth = 2. Two hash_layer calls:

• First with ZERO_HASHES[0]: pairs 3 elements → 2 nodes (3rd paired with zero)
• Second with ZERO_HASHES[1]: pairs 2 nodes → 1 node

Then ZERO_HASHES[2..depth]. Correct.

10. Test RNG Quality (Lines 177-189)

fn nodes(seed: u64, n: usize) -> Vec<Hash256> {
    let mut state = seed | 1;
    // ... xorshift ...
}

Problem: seed | 1 ensures odd, but xorshift with 64-bit state and these specific shifts (13, 7, 17) is a known good combination. However, state >> 24 as u8 only uses top 8 bits after each update. The period is 2^64-1. This is fine for testing.

---

Low Priority / Style Issues

11. Documentation Typo (Line 16)

//! The primitive takes the layer as a slice and writes digests to an output
//! slice, so a backend that hashes several independent messages per instruction
//! (AVX2 or AVX-512 lanes, or SHA-NI interleaved) replaces the loop body of
//! [`hash_pairs_into`] without changing a caller.

"without changing a caller" → "without changing callers" or "without caller changes"

12. hash_layer Could Use split_at_mut for True In-Place

The current scratch-based approach is safe but could be optimized further. For the first half of the layer where i < pairs and the write doesn't overlap with unread input, you could use split_at_mut to get two mutable slices and operate truly in-place. This is what the comment suggests but doesn't implement.

13. Benchmark Test Pollutes Test Namespace

batch_bench is #[ignore = "timing benchmark"] but still compiles in test builds. The report function and LEGACY static are test-only infrastructure that adds complexity.

---

Security Considerations

14. Constant-Time Concerns

The compress256 function likely uses hardware SHA-NI or similar when available. These are typically constant-time for the compression function itself. However, the loop structure and memory access patterns in hash_pairs_into could potentially leak information about which pairs are being hashed through cache timing. For SSZ merkleization of public data, this is not a concern.

For blockchain use, the data being hashed (validator records, balances) is public state, so timing side channels are not a security issue here.

15. SSZ Compatibility Verification

The tests verify against libssz_merkle::hash_nodes and libssz_merkle::merkleize, which is good. However, the proptest only covers len in 0usize..300 with extra in 0u32..4. This doesn't test large lists near the limit boundary, or the exact power-of-two boundaries.

Recommendation: Add edge cases for len == limit, len == limit - 1, len == limit / 2 + 1, etc.

---

Suggested Fixes

Fix 1: Use debug_assert_eq in internal function (Line 68)

// Before:
assert_eq!(input.len(), 2 * output.len(), "a pair per output");

// After:
debug_assert_eq!(input.len(), 2 * output.len(), "a pair per output");

Fix 2: Hoist padding block creation (Lines 80-86)

// Before:
let padding = GenericArray::<u8, U64>::clone_from_slice(&PADDING_BLOCK);
for (pair, out) in input.chunks_exact(2).zip(output.iter_mut()) {
    let mut blocks = [padding, padding];
    // ...
}

// After:
const PADDING_GENERIC: GenericArray<u8, U64> = {
    // Need to construct const GenericArray - may require unsafe or different approach
    // Alternative: use static
};
// Or simpler: define PADDING_BLOCK as GenericArray directly, or use from_slice outside loop
let padding_block = GenericArray::<u8, U64>::from_slice(&PADDING_BLOCK);
for (pair, out) in input.chunks_exact(2).zip(output.iter_mut()) {
    let mut blocks = [*padding_block, *padding_block];
    // ...
}

Actually GenericArray<u8, U64> implements Copy since it's an array, so *padding_block works if it's a reference. Or use clone():

let padding = GenericArray::<u8, U64>::from_slice(&PADDING_BLOCK).clone();

Wait, from_slice returns &GenericArray<u8, U64>, and clone() clones it. This is still in the loop. Hoist it:

let padding: GenericArray<u8, U64> = GenericArray::from_slice(&PADDING_BLOCK).clone();
// or if PADDING_BLOCK is already the right type, just use it directly

Best: Change PADDING_BLOCK to be GenericArray<u8, U64> directly, or use a lazy_static/once_cell. But for const, you can use:

const PADDING_BLOCK: GenericArray<u8, U64> = GenericArray::from_array([
    0x80, 0, 0, /* ... */, 0, 0x02, 0
]);

But GenericArray::from_array requires the size to match exactly. For 64 bytes, this is verbose. Alternative: use a macro or unsafe transmute in const context.

Simplest fix: just hoist the clone outside the loop:

let padding = GenericArray::<u8, U64>::clone


---
*Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt*

[MegaRedHand]

Block import benchmark (offline replay of mainnet blocks, steady state)

leg	STF per block	difference
main 012dc4f	157.7 ms
main + #753	159.8 ms	+2.1 ms (+1.4%), 95% CI [+1.5, +2.8]

No measurable change:

• Round 1: -0.2%, round 2: +2.9%. Per run, this leg is above main in 5 of 6 runs, by less than the run-to-run spread of the legs that cannot move the replay (up to ±4 ms per run). The per-block CI does not include that run-level variation.
• The state_root step is not slower where step timing exists: 24.1 ms per ordinary block for main + feat(bench): split the beacon import's stf phase into steps, classes and resources #756, 23.7 ms for nightly 0abc099 (which includes this PR).
• A microbenchmark of the layer hashing alone would resolve a 1% effect better than block replay can.

Method

• Offline ethlambda benchmark import replay on ethlambda-5 (Ryzen 7 9700X), with the node stopped.
• 3 consecutive mainnet windows fetched from that node: 173 measured blocks (slots 15567200 to 15567385), each window after one unmeasured warm-up epoch. --block-delay 1000.
• One leg per PR: the PR head merged onto main 012dc4f. The BLS PRs (perf(crypto): sum validated public keys through blst's batched affine addition #737, perf(beacon): verify a block's signatures in one random-weight batch #738, perf(beacon): verify attestations against committee key sums #742, perf(crypto): resolve registry public keys through a validator-index table #743) are stacked, so each of those legs includes its parents. --warm-committee-sums on the legs that have the flag (perf(beacon): verify attestations against committee key sums #742, perf(crypto): resolve registry public keys through a validator-index table #743, nightly).
• 2 rounds over all legs, leg order rotated per window. Every run of a window imported the same block roots.
• Numbers are per-block STF means, paired against the reference leg on the same blocks, with a bootstrap 95% CI of the mean difference.
• Steady state: 9 of the 173 blocks are dropped from every leg, because their cost exists only in a fresh process with no gossip. They are the first measured block of each window (about 31k attesters whose keys the process has never validated) and, per epoch, the first block carrying that epoch's attestations (it builds the committee shuffling inside the timed span; a live node's gossip validation builds it first). Including them makes the legs with --warm-committee-sums look about 3 points better than they are, since the flag does both before the timer starts.
• Noise floor: legs whose code the replay cannot exercise move about ±1.5% between rounds.