Skip to content

refactor(p2p): decouple proposal validators from base class via composition#21075

Merged
spalladino merged 1 commit intomerge-train/spartanfrom
palla/refactor-proposal-validator
Mar 4, 2026
Merged

refactor(p2p): decouple proposal validators from base class via composition#21075
spalladino merged 1 commit intomerge-train/spartanfrom
palla/refactor-proposal-validator

Conversation

@spalladino
Copy link
Contributor

Summary

  • Remove pass-through getters (blockNumber, txHashes, txs) from CheckpointProposal that just delegated to lastBlock
  • Split ProposalValidator into header validation (validate) and tx validation (validateTxs)
  • BlockProposalValidator and CheckpointProposalValidator now use ProposalValidator by composition instead of inheritance
  • Consolidate test suite into a single proposal_validator.test.ts that tests ProposalValidator directly with both proposal types

Test plan

  • yarn build passes
  • proposal_validator.test.ts passes (28 tests: header validation with both block and checkpoint proposals, plus tx validation)

🤖 Generated with Claude Code

@spalladino spalladino force-pushed the palla/refactor-proposal-validator branch from 67c27dd to a4208bb Compare March 3, 2026 20:35
Comment on lines +153 to +156
// CheckpointProposalCore doesn't have lastBlock info, so use 0 as a proxy.
// blockNumber is NOT used for the primary key so it's safe to use here.
// See CheckpointHeader TODO and SigningContext types documentation.
let blockNumber: BlockNumber;
try {
blockNumber = proposal.blockNumber;
} catch {
// Checkpoint proposal may not have lastBlock, use 0 as fallback
blockNumber = BlockNumber(0);
}
const blockNumber = BlockNumber(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spypsy can you confirm this is fine?

Copy link
Member

Choose a reason for hiding this comment

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

it makes block_number in the DB pretty useless but not an issue as the field isn't used anywhere meaningfully

Copy link
Collaborator

@PhilWindle PhilWindle left a comment

Choose a reason for hiding this comment

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

LGTM. Just a question for @spypsy

Base automatically changed from palla/validator-block-limits to merge-train/spartan March 4, 2026 14:23
…sition

Remove pass-through getters (blockNumber, txHashes, txs) from CheckpointProposal
that just delegated to lastBlock. Split ProposalValidator into header validation
(validate) and tx validation (validateTxs). BlockProposalValidator and
CheckpointProposalValidator now use ProposalValidator by composition instead of
inheritance, calling validate() for header checks and validateTxs() for tx checks
(on the block proposal directly or via getBlockProposal()).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@spalladino spalladino force-pushed the palla/refactor-proposal-validator branch from a4208bb to 6155fbe Compare March 4, 2026 14:26
@spalladino spalladino merged commit 445b8a4 into merge-train/spartan Mar 4, 2026
10 checks passed
@spalladino spalladino deleted the palla/refactor-proposal-validator branch March 4, 2026 16:28
@AztecBot
Copy link
Collaborator

AztecBot commented Mar 4, 2026

❌ Failed to cherry-pick to v4 due to conflicts. Dispatching ClaudeBox to resolve. View backport run.

spalladino added a commit that referenced this pull request Mar 4, 2026
…ition) (#21120)

## Summary
Backport of #21075
to v4.

- Remove pass-through getters (`blockNumber`, `txHashes`, `txs`) from
`CheckpointProposal` that just delegated to `lastBlock`
- Split `ProposalValidator` into header validation (`validate`) and tx
validation (`validateTxs`)
- `BlockProposalValidator` and `CheckpointProposalValidator` now use
`ProposalValidator` by composition instead of inheritance
- Consolidate test suite into a single `proposal_validator.test.ts` that
tests `ProposalValidator` directly with both proposal types

## Conflict resolution
The automatic cherry-pick failed due to modify/delete conflicts on two
test files:
- `checkpoint_proposal_validator.test.ts` — modified on v4, deleted in
PR
- `proposal_validator_test_suite.ts` — modified on v4, deleted in PR

Resolution: accepted the deletions since the PR replaces both files with
a new consolidated `proposal_validator.test.ts` that covers all test
cases from both files.

## Test plan
- CI will validate the build and tests pass on v4

ClaudeBox log: http://ci.aztec-labs.com/5e44af188aafe7dc-1

Co-authored-by: Santiago Palladino <santiago@aztec-labs.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
github-merge-queue bot pushed a commit that referenced this pull request Mar 6, 2026
BEGIN_COMMIT_OVERRIDE
test: update proving-real test to mbps (#20991)
chore: epoch proving log analyzer (#21033)
chore: update pause script to allow resume (#21032)
feat: price bump for RPC transaction replacement (#20806)
refactor: remove update checker, retain version checks (#20898)
fix: (A-592) p2p client proposal tx collector test (#20998)
refactor: use publishers-per-pod in deployments (#21039)
chore: web3signer refreshes keystore (#21045)
feat(sequencer): set block building limits from checkpoint limits
(#20974)
chore(e2e): fix e2e bot L1 tx nonce reuse (#21052)
feat: Update L1 to L2 message APIs (#20913)
fix: (A-589) epochs l1 reorgs test (#20999)
feat(sequencer): add SEQ_MAX_TX_PER_CHECKPOINT config (#21016)
fix: drop --pid=host from docker_isolate (#21081)
feat: standby mode for prover broker (#21098)
fix(p2p): remove default block handler in favor of block handler
(#21105)
feat(validator): add VALIDATOR_ env vars for independent block limits
(#21060)
refactor(p2p): decouple proposal validators from base class via
composition (#21075)
feat: additional validation in public setup allowlist (onlySelf + null
msg sender) (#21122)
fix: (A-591) aztecProofSubmissionEpochs incorrectly named as
aztecProofSubmissionWindow (#21108)
refactor(sequencer): rename SEQ_GAS_PER_BLOCK_ALLOCATION_MULTIPLIER to
SEQ_PER_BLOCK_ALLOCATION_MULTIPLIER (#21125)
fix: unbound variable in check_doc_references.sh with set -u (#21126)
feat: calldata length validation of public setup function allowlist
(#21139)
fix: include mismatched values in tx metadata validation errors (#21147)
feat: single-node implementation of slash-protection signer (#20894)
feat: Remove non-protocol contracts from public setup allowlist (#21154)
chore: More updated Alpha configuration (#21155)
chore: tally slashing pruning improvements (#21161)
fix: update dependencies (#20997)
fix: omit bigint priceBumpPercentage from IPC config in testbench worker
(#21169)
refactor(p2p): (A-588) maintain sorted array in tx pool instead of
sorting on read (#21079)
fix(p2p): report most severe failure in runValidations (#21185)
fix: use dedicated L1 account for bot bridge resume tests to avoid nonce
race (#21148)
fix: parse error.message in formatViemError (#21163)
fix: bump lighthouse consensus client v7.1.0 -> v8.0.1 (#21170)
chore: code decuplication + refactor (public setup allowlist) (#21200)
END_COMMIT_OVERRIDE
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants