refactor: update store to key by AttestationData by zclawz · Pull Request #656 · blockblaz/zeam

zclawz · 2026-03-10T14:29:53Z

Closes #636

Implements leanEthereum/leanSpec#436.

Summary

Old design

gossip_signatures: SignatureKey(validator_id, data_root) → StoredSignature
aggregated_payloads: SignatureKey(validator_id, data_root) → AggregatedPayloadsList
attestation_data_by_root: Root → AttestationData (reverse lookup needed to reconstruct data)

New design

gossip_signatures: AttestationData → HashMap(ValidatorIndex, StoredSignature)
aggregated_payloads: AttestationData → AggregatedPayloadsList
attestation_data_by_root removed — data is now the key, no reverse lookup needed

Changes

Remove SignatureKey type
Remove attestation_data_by_root field from ForkChoice
Update SignaturesMap to nested AttestationData → inner ValidatorIndex map
Update AggregatedPayloadsMap key from SignatureKey to AttestationData
Update computeAggregatedSignatures Phase 1 lookup (by AttestationData + ValidatorIndex)
Update computeAggregatedSignatures Phase 2 greedy set-cover to use single per-data candidate list
pruneStaleAttestationData now iterates directly by target.slot without needing a separate root set
All call sites updated: forkchoice.zig, fork_choice_runner.zig, mock.zig, block_signatures_testing.zig

Testing

zig build passes
zig build test passes (pre-existing IoUring/xev environment failures unrelated to these changes)

…on (closes #654)

…nstead of SignatureKey (closes #636) Implements leanEthereum/leanSpec#436: - Replace SignatureKey(validator_id, data_root) -> StoredSignature map with AttestationData -> HashMap(ValidatorIndex, StoredSignature) (SignaturesMap) - Replace SignatureKey -> AggregatedPayloadsList map with AttestationData -> AggregatedPayloadsList (AggregatedPayloadsMap) - Remove attestation_data_by_root reverse-lookup map (no longer needed) - Remove SignatureKey type (replaced by AttestationData as key) - Update computeAggregatedSignatures Phase 1 to look up by AttestationData - Update computeAggregatedSignatures Phase 2 greedy set-cover to use per-data candidate list instead of per-validator lookup - Update pruneStaleAttestationData to iterate gossip_signatures directly and prune by target.slot (renamed to prunePayloadMapBySlot) - Update all call sites: forkchoice.zig, fork_choice_runner.zig, mock.zig, block_signatures_testing.zig

…key after store refactor

anshalshukla

@zclawz address the comments

anshalshukla · 2026-03-18T17:18:57Z

+pub const GossipSignaturesInnerMap = std.AutoHashMap(ValidatorIndex, StoredSignature);
+
+/// Map type for gossip signatures: AttestationData -> per-validator signatures.
+/// Replaces the old SignatureKey(validator_id, data_root) -> StoredSignature design.


Remove this comment, we don't need to know the older map structure

anshalshukla · 2026-03-18T17:19:15Z

-/// Map type for aggregated payloads: SignatureKey -> list of AggregatedSignatureProof
-pub const AggregatedPayloadsMap = std.AutoHashMap(SignatureKey, AggregatedPayloadsList);
+/// Map type for aggregated payloads: AttestationData -> list of AggregatedSignatureProof.
+/// Replaces the old SignatureKey(validator_id, data_root) -> AggregatedPayloadsList design.


Remove this comment, we don't need to know the older map structure

anshalshukla · 2026-03-18T17:21:11Z


-            // Phase 2: Fallback to aggregated_payloads using greedy set-cover
+            // Phase 2: Fallback to aggregated_payloads using greedy set-cover.
+            // Candidates are now keyed by AttestationData directly (not per-validator).


Remove this comment, we don't need to know the older map structure

anshalshukla · 2026-03-18T17:28:06Z

        is_from_block: bool,
    ) !void {
-        const data_root = try attestation_data.sszRoot(self.allocator);
+        _ = validator_ids; // No longer needed: payloads are keyed by AttestationData, not per-validator


remove it from the function argument itself

anshalshukla · 2026-03-18T17:28:54Z

+        self.signatures_mutex.lock();
+        defer self.signatures_mutex.unlock();


earlier lock was scoped and the lock will be released as soon as the code section ends we should stay with that pattern and return lock as soon as the map has been written to

anshalshukla · 2026-03-18T17:32:28Z

-/// Map type for signatures_map: SignatureKey -> individual XMSS signature bytes + slot metadata
-pub const SignaturesMap = std.AutoHashMap(SignatureKey, StoredSignature);
+/// Inner map: ValidatorIndex -> StoredSignature for a given AttestationData.
+pub const GossipSignaturesInnerMap = std.AutoHashMap(ValidatorIndex, StoredSignature);


make it a struct with the put and deinit methods, there are multiple places where we do multiple checks of initialization before writing that can be abstracted out, and iterating over the map of map can be easily missed so having a deinit function is better

- Remove 'Replaces the old...' and 'not per-validator' comments from block.zig (three sites); reviewer noted these transition comments are not needed in the final code. - Make GossipSignaturesInnerMap a proper struct wrapping AutoHashMap with init/put/get/deinit/iterator/count methods, so callers don't need to manage the inner map lifecycle and the initialization pattern (getOrPut + init if not found) stays consistent across all sites. - Remove validator_ids parameter from storeAggregatedPayload — payloads are now keyed by AttestationData so the param was unused. Update all call sites: chain.zig (×2), forkchoice.zig test helper, fork_choice_runner.zig. - Scope signatures_mutex in storeAggregatedPayload using a {} block with defer unlock, releasing the lock as soon as the map write completes. Consistent with the scoped-lock pattern used everywhere else in the file.

anshalshukla · 2026-03-18T17:49:26Z

+    /// Thin wrapper around validateAttestationData for callers that have a full Attestation.
+    pub fn validateAttestation(self: *Self, attestation: types.Attestation, is_from_block: bool) !void {
+        return self.validateAttestationData(attestation.data, is_from_block);
+    }
+


we don't need a wrapper function directly call validateAttestationData with attestation.data to validate full Attestation @zclawz

…; remove validateAttestation wrapper aggregateCommitteeSignaturesUnlocked / computeAggregatedSignatures: The old flow built a flat attestation list from gossip_signatures to pass as the first arg to computeAggregatedSignatures, which then re-grouped it by AttestationData. Since gossip_signatures is already keyed by AttestationData → GossipSignaturesInnerMap this round-trip was pure overhead. New flow: - computeAggregatedSignatures(validators, signatures_map, aggregated_payloads) drops the attestations_list parameter entirely. - Iterates the *union* of signatures_map and aggregated_payloads keys so groups that only exist in aggregated_payloads (individual sigs pruned but stored proof available) are still processed. - Phase 1 iterates the inner map directly (validator_id → stored_sig) with no intermediate grouping or root-index lookup. - Before Phase 2 greedy set-cover, seeds from proof participants not already in sigmap_available so proofs covering validators absent from signatures_map are still included. aggregateCommitteeSignaturesUnlocked (forkchoice.zig): - Drops the attestations ArrayList build loop entirely. - Calls computeAggregatedSignatures directly on gossip_signatures. chain.zig: - Removes getProposalAttestations() call (was only needed as the first arg); aggregation now reads gossip_signatures directly. - Removes validateAttestation() thin wrapper (review comment #r2955207656); callers should use validateAttestationData directly. All call sites updated: chain.zig, mock.zig, block_signatures_testing.zig.

zclawz · 2026-03-18T18:22:14Z

Thanks for the review @anshalshukla! All feedback addressed:

validateAttestation() wrapper removed — callers use validateAttestationData(attestation.data, ...) directly
Mutex lock in storeAggregatedPayload is now scoped ({} block) so it releases immediately after the map write
Cleaned up stale comments about the old SignatureKey/data_root design
GossipSignaturesInnerMap now a proper struct with put/get/iterator/deinit
Removed unused validator_ids param

Changes pushed — please re-review! 🙏

…line)

Port 14 files that don't conflict with #656 store refactor: - Dual keys: validator.zig, utils.zig, state.zig, configs/lib.zig - AggregatedSignatureProof.aggregate(): aggregation.zig - SignedBlock refactor: transition.zig, lib.zig, zk.zig - Proposer signing: validator_client.zig - Testing/infra: testing.zig, network.zig, database, state_transition_runner

- ValidatorKeys struct with attestation_keypair + proposal_keypair - getAllPubkeys returns AllPubkeys{attestation_pubkeys, proposal_pubkeys} - signBlockRoot for proposer block signing - getAttestationPubkeyBytes / getProposalPubkeyBytes - Retains #656 owned_keys tracking

- Replace SignatureKey/flat SignaturesMap with #656 wrapper (AttestationData -> InnerMap(ValidatorIndex -> StoredSignature)) - AggregatedPayloadsMap keyed by AttestationData (not Root) - Retain devnet4 AggregatedAttestationsResult + extendProofsGreedily - Add AggregateInnerMap helper from #656 (adapted for devnet4 aggregate) - SignedBlock refactoring (flattened, no BlockWithAttestation) - block_signatures_testing.zig needs further adaptation (TODO)

…ckages Propagate the #656 store refactor naming changes to all consumer files: - SignedBlockWithAttestation → SignedBlock, BlockWithAttestation removed - .message.block.field → .message.field (flattened block structure) - Validator.pubkey → attestation_pubkey + proposal_pubkey (dual keys) - GenesisSpec.validator_pubkeys → validator_attestation_pubkeys - key_manager.getAllPubkeys() returns AllPubkeys{attestation, proposal} - signAttestation → signBlockRoot for proposer signatures - Removed proposer_attestation from block processing - aggregateCommitteeSignatures → aggregate(state, false)