Skip to content

refactor(node): split AggregateCommitteeSignatures into snapshot/prove/apply (#307)#308

Merged
mananuf merged 1 commit into
perf/instrument-aggregation-phasesfrom
refactor/aggregation-snapshot-prove
May 25, 2026
Merged

refactor(node): split AggregateCommitteeSignatures into snapshot/prove/apply (#307)#308
mananuf merged 1 commit into
perf/instrument-aggregation-phasesfrom
refactor/aggregation-snapshot-prove

Conversation

@mananuf
Copy link
Copy Markdown
Collaborator

@mananuf mananuf commented May 25, 2026

Summary

Structural split of `AggregateCommitteeSignatures` into snapshot → prove → apply, mirroring ethlambda's `snapshot_aggregation_inputs → aggregate_job → publish` pattern (`ethlambda/crates/blockchain/src/aggregation.rs:120-462`). Pure refactor — no behaviour change. Closes #307.

This PR alone delivers no perf win. Its job is to unlock the prove phase from any `*ConsensusStore` reference, which is the structural prerequisite for Phase D (off-tick async dispatch — separate issue).

What changed

`node/store_aggregate.go`: +113 / -31 LOC.

Two new types + three new helpers replace the monolithic function:

```go
type AggregationSnapshot struct {
headState *types.State
attSigs map[[32]byte]*AttestationDataEntry
newEntries map[[32]byte]*PayloadEntry
knownEntries map[[32]byte]*PayloadEntry
targetStates map[[32]byte]*types.State
}

type AggregationMutations struct {
PayloadEntries []PayloadKV
KeysToDelete []AttestationDeleteKey
}

func snapshotAggregationInputs(s *ConsensusStore) *AggregationSnapshot
func aggregateFromSnapshot(snap *AggregationSnapshot, cache *xmss.PubKeyCache) ([]*SignedAggregatedAttestation, *AggregationMutations)
func applyAggregationMutations(s *ConsensusStore, m *AggregationMutations)
```

`AggregateCommitteeSignatures` becomes the thin wrapper:

```go
func AggregateCommitteeSignatures(s *ConsensusStore) []*types.SignedAggregatedAttestation {
passStart := time.Now()
defer func() { ObserveCommitteeAggregationTime(time.Since(passStart).Seconds()) }()

snap := snapshotAggregationInputs(s)
if snap == nil { return nil }
aggs, mut := aggregateFromSnapshot(snap, s.PubKeyCache)
applyAggregationMutations(s, mut)
return aggs

}
```

`selectChildProofs`' last parameter changes `s *ConsensusStore` → `cache *xmss.PubKeyCache`. That was the only store dependency in its body; threading the cache directly removes the snapshot's only escape hatch into the live store.

Behavior change

None. Same store reads, same FFI calls, same store mutations, in the same order. All four phase-attribution histograms from #305 stay where they were — they wrap the same code spans, just inside the new function boundaries now.

Specifically verified preserved:

  • `AttestationSignatures.Snapshot()` is still called once per pass
  • `NewPayloads.data` / `KnownPayloads.data` reads happen once per pass
  • `GetState(s.Head())` for headState happens once per pass
  • `GetState(attData.Target.Root)` happens once per unique target root per pass (with the new `targetStates` cache deduplicating; previously this could be called once per data root even if many shared the same target)

Net effect: very slightly fewer `GetState` calls in the rare case where multiple data roots share a target. Otherwise identical work.

Spec compliance

Zero impact. Structural refactor of gean-side code; no spec-mapped function or container modified. Same store transitions, same broadcast set.

Test plan

Lean-review

  • `dead-code`: N/A
  • `premature-abstraction`: the snapshot/mutations types each have a single use site today, but Phase D consumes them — naming them up front beats a "rewrite the file in Phase D" diff
  • `defensive-bloat`: N/A
  • `duplicates-existing-util`: N/A
  • `comment-bloat`: docstrings on the two new types are 6 and 3 lines; both explain why the type exists (the ethlambda parallel + the async prerequisite), which is non-obvious without context. Earns its keep.
  • `over-validated-boundary`: N/A

Stack

Stacks on PR #306 (instrumentation). Base: `perf/instrument-aggregation-phases`. Will be merged with Phases C + D before `perf/instrument-aggregation-phases` itself merges to devnet-4.

…e/apply (#307)

Mirrors ethlambda's snapshot_aggregation_inputs → aggregate_job worker
→ publish structural split (ethlambda/crates/blockchain/src/aggregation.rs).

Three new helpers + two new types replace the single monolithic function:

  snapshotAggregationInputs(s) → *AggregationSnapshot
      Reads head state, attestation signatures, new/known payload entries,
      and pre-resolves target states. The only function with a store
      reference; runs synchronously on the caller's thread.

  aggregateFromSnapshot(snap, cache) → ([]*SignedAggregatedAttestation, *AggregationMutations)
      Per-data-root prep/FFI/post phases. Pure function of snapshot and
      pubkey cache; holds no store reference. Returns aggregates plus a
      batched mutation set the caller is responsible for applying.

  applyAggregationMutations(s, mut)
      Applies the prove phase's KnownPayloads.PushBatch +
      AttestationSignatures.Delete in one call.

AggregateCommitteeSignatures is now a thin wrapper that calls the three
in sequence, keeping its public signature and behaviour unchanged.

selectChildProofs' last parameter changed from *ConsensusStore to
*xmss.PubKeyCache — that was the only store dependency in the body, and
threading the cache directly removes the snapshot's only escape hatch
into the live store.

Pure refactor. Same store reads, same FFI calls, same store mutations,
in the same order. All four phase-attribution histograms from #305 stay
where they were (still wrap the same code spans).

Sets up the prerequisite for Phase D async dispatch: with the snapshot
type in place, an off-tick worker goroutine has a well-defined,
self-contained input to consume.

go build ./... + go test ./node/... green.

Closes #307.
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.

1 participant