feat: merge-train/spartan#22940
Merged
Merged
Conversation
⚠️ **This PR includes #22870. Reviewers should review only the first commit.**⚠️ ## Motivation Consolidates the block-related lookup surface on `L2BlockSource` from ~17 narrow methods returning ~9 different shapes down to 4 methods returning 2 shapes (`L2Block` and `BlockData`). Replaces the per-shape getters with discriminated query objects that carry both the lookup discriminant and a single `onlyCheckpointed` filter, removing the parallel `Checkpointed*` API and the throwaway wrapper types. Additionally, this refactor centralizes block-zero handling in the archiver and threads the dynamic initial header through every component that previously hard-coded the constant, eliminating the divergence and removing the special-case branches in callers. ## Approach `L2BlockSource` exposes 4 methods that take query objects: ```ts getBlock(query: BlockQuery): Promise<L2Block | undefined> getBlocks(query: BlocksQuery): Promise<L2Block[]> getBlockData(query: BlockQuery): Promise<BlockData | undefined> getBlocksData(query: BlocksQuery): Promise<BlockData[]> type BlockQuery = ({number} | {hash} | {archive}) & { onlyCheckpointed?: boolean } type BlocksQuery = ({from, limit} | {epoch}) & { onlyCheckpointed?: boolean } ``` On-disk format is unchanged — the archiver already stored block metadata, tx bodies, and per-checkpoint L1/attestation data in separate LMDB maps; `CheckpointedL2Block` was only an in-memory join produced at read time. **Includes changes from #22870 ## API surface change ### Methods removed from `L2BlockSource` `getL2Block`, `getL2BlockByHash`, `getL2BlockByArchive`, `getCheckpointedBlock`, `getCheckpointedBlockByHash`, `getCheckpointedBlockByArchive`, `getCheckpointedBlocks`, `getCheckpointedBlocksForEpoch`, `getCheckpointedBlockHeadersForEpoch`, `getBlock(number)`, `getBlocks(from, limit)`, `getBlockData(number)`, `getBlockDataByArchive`, `getBlockDataWithCheckpointContext`, `getBlockHeader`, `getBlockHeaderByHash`, `getBlockHeaderByArchive`. ### Types deleted `CheckpointedL2Block`, `BlockDataWithCheckpointContext` — both removed entirely (file + schema + re-exports). Callers that previously read `.l1` / `.attestations` off these now do `getBlockData(...)` followed by `getCheckpointData(blockData.checkpointNumber)` and read those fields off `CheckpointData`. ### Types added `BlockQuery`, `BlocksQuery` (and matching Zod schemas) on `L2BlockSource`. No new domain types — `L2Block`, `BlockData`, `BlockHeader` are unchanged. ### AztecNode public RPC Method names preserved (`getBlock`, `getBlockHeader`, `getCheckpointedBlocks`, etc. — bodies delegate internally to the new `L2BlockSource` methods). One wire-level change: `AztecNode.getCheckpointedBlocks` element type goes `CheckpointedL2Block[]` → `BlockResponse[]`, forced by the type deletion. Older RPC clients that parse the old shape will need to update. ## Changes - **stdlib**: `BlockQuery` / `BlocksQuery` types + Zod schemas next to `L2BlockSource`. `CheckpointedL2Block` file deleted; `BlockDataWithCheckpointContext` removed from `block_data.ts`. `ArchiverApiSchema` and `MockArchiver` shrunk; new `it()` blocks cover each query discriminant. `L2BlockStream` migrated. - **archiver**: `BlockStore` consolidates to four query-object reads plus iterators. `data_source_base.ts` adds `resolveBlocksQuery` that translates `{ epoch }` → `{ from, limit }` (returns `null` for empty epochs so callers short-circuit to `[]`). Mocks honor `onlyCheckpointed`. - **aztec-node**: `server.ts` keeps the public RPC method names but delegates to the new query methods. `getCheckpointedBlocks` adds a per-call `Map<CheckpointNumber, CheckpointData>` cache to avoid an N+1. - **consumer migrations**: `world-state`, `txe`, `p2p` block-txs handler, `validator-client` (`validator.ts`, `proposal_handler.ts`), `pxe` block-stream source (honors `onlyCheckpointed` via `node.getL2Tips`), `prover-node`, `sequencer-client`, `telemetry-client`, `aztec/testing`, `L2BlockStream` in stdlib. - **tests**: per-package mocks updated for the new shapes; new test covers `getBlocks({ epoch })` empty-epoch returning `[]`.
…2935) Updates the custom no-non-primitive-in-collections rule to allow for branded primitive types like BlockNumber as keys in Maps or Sets.
End-to-end test for the "missed L1 publish" scenario under proposer pipelining. Each of 4 nodes holds exactly one validator key. We pick four consecutive slots (slotZero, slotOne, slotTwo, slotThree) such that the proposers for slotOne, slotTwo, and slotThree are three distinct validators, then warp to one L1 block before slotZero begins. The proposer for slotOne is configured to skip its L1 publish. With pipelining, the proposer for slot N+1 builds and gossips its checkpoint during slot N, then publishes that checkpoint to L1 during slot N+1. So gossip-driven `proposed` chain advances arrive one slot earlier than the L1-driven `checkpointed` advance. Expected behavior: - During slotZero, the pipelined proposer for slotOne gossips its build → every node's `proposed` tip advances to a block at slotOne. - During slotOne, the pipelined proposer for slotTwo gossips on top of the slotOne proposal → `proposed` advances to a block at slotTwo. Meanwhile the proposer for slotOne attempts L1 publish but is configured to skip it, so no checkpoint lands. - When slotOne ends with no checkpoint mined, every node's archiver prunes the uncheckpointed slotOne and slotTwo blocks; we verify rollback via the prune event. We then re-enable publishing on the formerly suppressed node so recovery can proceed. - During slotTwo, the pipelined proposer for slotThree builds on top of the (now genesis) checkpointed tip → `proposed` advances again. - During slotThree, that pipelined work is published → `checkpointed` finally advances.
Closes a slashing-soundness gap in the checkpoint attestation pool: two
different checkpoint proposals (or attestations) at the same slot with
identical archive root were considered equal in the attestation pool,
since the pool keyed on `archive`. So we'd never slash for
`DUPLICATE_PROPOSAL` / `DUPLICATE_ATTESTATION`.
There were two scenarios where two different proposals could have the
same archiveroot: a malicious or buggy node that sent two proposals with
the same root but different content (ie an archive root that doesn't
follow from the payload, ie an invalid one), or a malicious or buggy
node that sent two equal proposals but with different
`feeAssetPriceModifier` which is not covered by the archive root.
Fixes A-1013
## Pool changes
- **Dedup by payload hash, not by archive.** Each equivocation position
has two stores: a main store that keeps the *first* full entry seen at
that position, and a parallel multimap that tracks the *set of distinct
signed-payload hashes* seen there. A second distinct hash arriving at
the same position bumps the multimap count to 2, which trips `tryAdd*`'s
`count` and lets libp2p fire its duplicate callback /
`WANT_TO_SLASH_EVENT`. Bytes of the equivocating payload are not
retained.
- `attestationPerSlotAndSigner` (full) +
`attestationHashesPerSlotAndSigner` (hashes), keyed by `(slot, signer)`.
- `checkpointProposalPerSlot` (full) + `checkpointProposalHashesPerSlot`
(hashes), keyed by slot.
- `blockProposalPerSlotAndIndex` (full) +
`blockProposalHashesPerSlotAndIndex` (hashes), keyed by `(slot,
indexWithinCheckpoint)`.
- Hash is `keccak256(getPayloadToSign())`, **never over the signature**,
so non-deterministic ECDSA re-signs of the same payload do not look like
equivocation.
- `CheckpointProposal.getPayloadHash()` hashes through the
`ConsensusPayload` form so a checkpoint proposal's hash matches the
hashes of the attestations that signed it (proposers and attesters sign
different byte layouts of the same logical content).
- Secondary `blockProposalSlotAndIndexPerArchive` index keeps
`getBlockProposalByArchive(archive)` (used by the block-txs req/resp
protocol) resolving by archive root without a wire-protocol change. The
lookup now validates that the stored proposal's archive matches the
requested one and warns + returns `undefined` on mismatch.
- On-disk kv-store map names are unchanged; only the in-memory field
names and the *value format* (now `0x`-prefixed payload-hash hex) are
new.
## Branded payload-hash types
- `CheckpointProposalHash` and `BlockProposalHash` introduced as
`Branded<0x${string}>` in `foundation/branded-types`, so the two cannot
be confused at the TS type level.
`CheckpointAttestation.getPayloadHash()` returns
`CheckpointProposalHash` (the attestation and its proposal sign the same
payload).
- `generateP2PMessageIdentifier()` on `CheckpointProposal` /
`CheckpointAttestation` / `BlockProposal` now derives from the **same
bytes** as `getPayloadHash()` (returning `Buffer32` rather than the
`0x`-string), so libp2p's gossip dedup identity and the attestation
pool's dedup identity agree. A shared `getPayloadHashBuffer()` helper on
`BlockProposal` avoids double-hashing.
## Handler cache (validator-client)
- `ProposalHandler.lastCheckpointValidationResult` now keys by
`CheckpointProposalHash` instead of `(archive, slot)`. Without this fix,
two proposals at the same slot+archive with a differing
`feeAssetPriceModifier` would have shared a cached validation result and
the second proposal would have skipped re-validation.
## API renames
- `AttestationPool.getCheckpointProposal(slot)` — was
`getCheckpointProposal(archive)`.
- `AttestationPool.getBlockProposalByArchive(archive)` — was
`getBlockProposal(archive)`; now validates the resolved proposal's
archive matches.
- `AttestationPool.getCheckpointAttestationsForSlotAndProposal(slot,
proposalPayloadHash)` — was `(slot, archive)`.
- `P2PApi.getCheckpointAttestationsForSlot(slot, proposalPayloadHash?)`.
- Sentinel stores `proposalPayloadHash` alongside archive; validator
client passes `proposal.getPayloadHash()` to filter attestations.
## Tests
- New pool-level test verifies
same-archive-different-`feeAssetPriceModifier` is recognised as an
equivocation.
- New libp2p_service tests verify the equivocation surfaces all the way
to the slash callback for checkpoint proposals, attestations (incl.
negative test for two distinct signers), and block proposals.
- New proposal_handler test verifies the cache is not shared across
proposals that differ only on `feeAssetPriceModifier`.
Notify slack users directly in Grafan alerts
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BEGIN_COMMIT_OVERRIDE
refactor(archiver)!: simplify L2BlockSource block lookups (#22809)
chore(lint): allow branded primitive types as keys in collections (#22935)
test(e2e): test missed l1 publishing under pipelining (#22926)
fix: dedup attestation pool by payload hash (#22871)
chore: notify slack users directly (#22944)
END_COMMIT_OVERRIDE