Skip to content

feat(message_validator): reject ValidatorRegistration after Gloas - #1138

Merged
mergify[bot] merged 3 commits into
sigp:epbsfrom
shane-moore:feat/reject-validator-registration-after-gloas
Jul 17, 2026
Merged

feat(message_validator): reject ValidatorRegistration after Gloas#1138
mergify[bot] merged 3 commits into
sigp:epbsfrom
shane-moore:feat/reject-validator-registration-after-gloas

Conversation

@shane-moore

Copy link
Copy Markdown
Member

Problem, Evidence, and Context (Required)

Change Overview (Required)

  • New ValidationFailure::RoleNotActiveAfterEthFork variant, filling the one empty cell in the existing 2x2 gate matrix (not-yet-active/deprecated x SSV Fork/Ethereum ForkName), and a fourth branch in validate_role_for_fork mirroring the PTCAttester gate with inverted polarity.
  • Where to focus review:
    1. lib.rs, the new branch in validate_role_for_fork: the only production logic. The gate keys on the message envelope slot's epoch (not wall clock), so the first slot of GLOAS_FORK_EPOCH rejects and every earlier slot accepts; registrations for pre-fork slots stay valid through their TTL window even when arriving post-fork.
    2. consensus_message.rs: the shared arrange/act of the Boole fork tests was factored into run_role_fork_validation so the new VR test could reuse it. Confirm the 4 existing Boole tests still assert exactly what they did.
    3. partial_signature.rs: additive tests; spec_with_gloas moved to the shared crate::tests module.
  • Intentionally unchanged: wire values (Role byte [4,0,0,0], PartialSignatureKind::ValidatorRegistration = 4) are retained for pre-Gloas decode per SIP-94 §5, and the acceptance mapping is untouched (the new variant falls through to REJECT like its three siblings).

Risks, Trade-offs, and Mitigations (Required)

  • No behavior change until a network schedules Gloas (gloas_fork_epoch = None everywhere today); pinned by a dedicated assertion.
  • REJECT rather than IGNORE: a post-Gloas-slot VR is categorically invalid per the SIP, matching the Boole deprecation of Aggregator/SyncCommittee; the peer-score penalty only hits peers still running the deprecated duty schedule.
  • Post-Gloas, a VR message on the consensus path now surfaces RoleNotActiveAfterEthFork instead of the structural UnexpectedConsensusMessage (the fork gate runs first); both map to REJECT.

Validation (Required)

  • Unit test at the exact fork boundary (last pre-fork slot accepts, first fork-epoch slot rejects, Gloas-unscheduled is a no-op) plus end-to-end REJECT tests through both entry paths (partial-sig and consensus).
  • cargo test -p message_validator: 66 passed. Workspace fmt, clippy, and cargo check clean.

Rollback (Required for behavior or runtime changes; optional otherwise)

  • Revert the single commit; no config, data, or wire impact.

Blockers / Dependencies (Optional)

N/A. Independent of #1062-#1064.

SIP-94 deprecates the ValidatorRegistration duty at the Gloas fork:
proposer preferences replace relay registrations. Reject inbound VR
partial-signature messages whose envelope slot is at or after the fork.
Wire values are retained for pre-Gloas decode per the SIP.
@shane-moore

Copy link
Copy Markdown
Member Author

@claude, please take a look

@claude-code-actions-sigp

claude-code-actions-sigp Bot commented Jul 14, 2026

Copy link
Copy Markdown

Claude finished @shane-moore's task in 3m 23s —— View job


Review: reject ValidatorRegistration after Gloas

  • Read changed files (lib.rs, consensus_message.rs, partial_signature.rs)
  • Analyze validate_role_for_fork logic
  • Verify test coverage and correctness
  • Post review feedback

Verdict: LGTM. This is a clean, well-scoped defensive change. The new branch faithfully mirrors the existing PTCAttester gate with inverted polarity, and the reasoning in the PR body matches what the code does. No blocking issues found.

Correctness — verified

  • Polarity is correct. gloas_enabled() is true at/after the Gloas fork, so if current_fork.gloas_enabled() { reject } rejects exactly at the first slot of GLOAS_FORK_EPOCH and accepts every earlier slot. The boundary test (slot 63 accepts / slot 64 rejects at gloas_fork_epoch = 2) pins this precisely.
  • Slot-keyed, not wall-clock. Both entry points feed the message's slot into validate_role_for_fork (messages.slot at partial_signature.rs:36, Slot::new(consensus_message.height) at consensus_message.rs:37‑38). Confirmed the emit side (validator_store/src/lib.rs:2848‑2874) signs VR with the duty slot, so pre-fork registrations stay valid through their TTL window even when they arrive post-fork. The two halves agree on which slot governs.
  • Acceptance mapping. RoleNotActiveAfterEthFork isn't in the Ignore arm (lib.rs:242‑262), so it falls through to _ => Reject — consistent with its three siblings and with the SIP's "categorically invalid" framing.
  • No-op until scheduled. ChainSpec::mainnet() ships gloas_fork_epoch = None, so fork_name_at_epoch never reaches Gloas and the gate never fires. Existing VR acceptance tests use the default spec and are therefore unaffected — the spec_with_gloas(None) assertion pins this explicitly.
  • Gate ordering. validate_role_for_fork runs before validate_consensus_message_semantics, so post-Gloas a VR consensus message surfaces RoleNotActiveAfterEthFork rather than the structural UnexpectedConsensusMessage. Both REJECT, so acceptance is unchanged; only the failure label differs, as the PR body states.

Design / consistency

  • New variant field types (current_fork: ForkName, deprecated_since_fork: ForkName) match the Ethereum-fork sibling RoleNotActiveBeforeEthFork rather than the SSV-Fork sibling. Correct choice given this gate reads the consensus spec.
  • The spec_with_gloas helper move to the shared crate::tests module and the run_role_fork_validation extraction are reasonable DRY refactors; the four Boole tests still route through test_role_fork_validation with unchanged arguments, so their assertions are preserved.

Test coverage

Solid — full-path REJECT through both entry points (partial-sig and consensus), a direct boundary test (last pre-fork slot / first fork-epoch slot), and the unscheduled no-op. Coverage matches the production logic added.

Notes (non-blocking)

  • I was unable to run cargo test -p message_validator / make lint in this environment (command approval required), so I'm relying on the PR's stated "66 passed / fmt+clippy clean". The change compiles cleanly on inspection and introduces no new unwrap/expect in production paths.
  • Minor: test_validator_registration_rejected_after_gloas (partial_signature.rs) matches on deprecated_since_fork only, while the consensus and boundary tests also pin role: Role::ValidatorRegistration. Harmless given the message is constructed as VR, but tightening it would keep the three tests symmetric. Not worth a revision on its own.
    · branch feat/reject-validator-registration-after-gloas

@codecov-commenter

codecov-commenter commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.29730% with 3 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (epbs@b41a4f6). Learn more about missing BASE report.

Files with missing lines Patch % Lines
anchor/message_validator/src/partial_signature.rs 96.92% 2 Missing ⚠️
anchor/message_validator/src/consensus_message.rs 96.77% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             epbs    #1138   +/-   ##
=======================================
  Coverage        ?   67.66%           
=======================================
  Files           ?      163           
  Lines           ?    30596           
  Branches        ?        0           
=======================================
  Hits            ?    20702           
  Misses          ?     9894           
  Partials        ?        0           
Flag Coverage Δ
rust 67.66% <97.29%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Matches the boundary and consensus-path tests, which already pin
Role::ValidatorRegistration in the expected failure pattern.
@shane-moore

shane-moore commented Jul 14, 2026

Copy link
Copy Markdown
Member Author

@claude Nit addressed in 16c7d44: the end-to-end test now pins role: Role::ValidatorRegistration in the expected failure pattern, matching the boundary and consensus-path tests.

@claude-code-actions-sigp

claude-code-actions-sigp Bot commented Jul 15, 2026

Copy link
Copy Markdown

Claude finished @shane-moore's task in 29s —— View job


Nit follow-up: role pinned in post-Gloas VR rejection assert

  • Read 16c7d44
  • Confirm the pinned field matches the variant definition
  • Confirm symmetry with the boundary and consensus-path tests

Confirmed — 16c7d44 resolves the non-blocking nit from my earlier review.

  • The added role: Role::ValidatorRegistration line (partial_signature.rs:1403) matches a real field on the variant: RoleNotActiveAfterEthFork { role, current_fork, deprecated_since_fork } (lib.rs:232‑236), so the matches! pattern still compiles and now asserts on role alongside deprecated_since_fork.
  • All three tests (partial-sig end-to-end, consensus-path, boundary) now pin Role::ValidatorRegistration in the expected failure pattern — symmetry restored, which is exactly what the nit asked for.
  • One-line, test-only change; no production-path impact and no change to the _ => Reject acceptance mapping.

Nothing else to flag. Verdict stands: LGTM.

Note: as before, I did not run cargo test -p message_validator / make lint in this environment (command approval required); the assessment is by inspection.
· branch feat/reject-validator-registration-after-gloas

@petarjuki7 petarjuki7 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, ready to merge after the conflicts are fixed.

@mergify

mergify Bot commented Jul 17, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@mergify
mergify Bot merged commit 68acc2e into sigp:epbs Jul 17, 2026
21 checks passed
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.

3 participants