Skip to content

refactor(message_validator): delete the unused received_from plumbing and peer-keyed preference map - #1284

Merged
shane-moore merged 1 commit into
sigp:epbsfrom
shane-moore:refactor/delete-received-from-plumbing
Aug 27, 2026
Merged

refactor(message_validator): delete the unused received_from plumbing and peer-keyed preference map#1284
shane-moore merged 1 commit into
sigp:epbsfrom
shane-moore:refactor/delete-received-from-plumbing

Conversation

@shane-moore

Copy link
Copy Markdown
Member

Problem, Evidence, and Context (Required)

Change Overview (Required)

  • Drop received_from: Option<PeerId> from the five-signature validation chain (validate -> validate_decoded_message -> validate_ssv_message -> validate_partial_signature_message -> update_for_partial_signature) and its one production call site in message_receiver::manager.
  • Narrow both SigningRootBudgets maps from HashMap<Hash256, Option<PeerId>> to HashSet<Hash256>; rewrite the two doc comments that described the stored deliverer.
  • Tests: delete the peer fixtures, remove the trailing peer argument everywhere, collapse tests whose only remaining distinction was the deliverer identity, and delete three older tests that became strict subsets of the dedup criteria tests once the peer axis vanished.
  • Reading order: duty_state.rs (the only semantic hunks: set narrowing + enforcement site), then lib.rs/partial_signature.rs/manager.rs (mechanical arity changes), then the test module.
  • Intentionally unchanged: classification behavior and error variants, membership-before-capacity ordering, the exhaustive root_budget match, the lazy boxed budget allocation, and manager.rs's use of propagation_source for the gossipsub Outcome report. libp2p stays a dependency (MessageAcceptance re-export and conversions).

Risks, Trade-offs, and Mitigations (Required)

  • Main risk is an accidental semantic change smuggled in via the test collapse. Mitigation: the acceptance criterion was that every surviving test passes with argument removals, renames, and comment fixes only; zero expectation changes were needed. The RelayedDuplicateMessage -> Ignore mapping pin, the cap/poisoning/budget-independence tests, and the ring-collision test all survive.
  • Validator::validate is a public signature, so this is a source-level API change; the single in-repo caller is updated in the same diff.
  • Trade-off declared: net test count drops 132 -> 126. Each deleted test was verified as a strict behavioral subset of a surviving criteria test before deletion.

Validation (Required)

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

  • Pure revert; no config, data, or wire impact. Runtime verdicts are unchanged, so mixed deployments are unaffected.

Blockers / Dependencies (Optional)

PR sigp#1272 made role-8 duplicate-root classification peer-agnostic per
SIP-94 s7, removing the only read of the stored propagation peer. Drop
the received_from parameter from the five-signature validation chain and
its one production call site, and narrow the per-kind signing-root
budgets from HashMap<Hash256, Option<PeerId>> to HashSet<Hash256>.
Collapse the tests whose only remaining distinction was the deliverer
identity. Closes sigp#1273.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.76923% with 5 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (epbs@7c428cf). Learn more about missing BASE report.

Files with missing lines Patch % Lines
anchor/message_validator/src/lib.rs 0.00% 3 Missing ⚠️
anchor/message_receiver/src/manager.rs 0.00% 1 Missing ⚠️
anchor/message_validator/src/partial_signature.rs 94.73% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             epbs    #1284   +/-   ##
=======================================
  Coverage        ?   78.30%           
=======================================
  Files           ?      175           
  Lines           ?    38638           
  Branches        ?        0           
=======================================
  Hits            ?    30257           
  Misses          ?     8381           
  Partials        ?        0           
Flag Coverage Δ
rust 78.30% <80.76%> (?)

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.

@shane-moore

Copy link
Copy Markdown
Member Author

@claude, please take a look

@claude-code-actions-sigp

claude-code-actions-sigp Bot commented Aug 27, 2026

Copy link
Copy Markdown

Claude finished @shane-moore's task in 2m 40s —— View job


Review: refactor(message_validator): delete unused received_from plumbing

  • Gather context (read diff and changed files)
  • Analyze semantic changes in duty_state.rs
  • Review mechanical arity changes (lib.rs, partial_signature.rs, manager.rs)
  • Verify test collapse preserves behavior
  • Post review feedback

Verdict

Looks good — the change is exactly what the description claims: a pure dead-code deletion with no behavioral change. No blocking issues found.

What I verified

Semantic core (duty_state.rs) is sound. The only real logic change is narrowing SigningRootBudgets from HashMap<Hash256, Option<PeerId>> to HashSet<Hash256>:

  • seen_roots.contains_key(&root)contains(&root) — equivalent membership test.
  • seen_roots.insert(root, received_from)insert(root) — the previous return (Option<Option<PeerId>>) was already discarded, and HashSet::insert's bool return is likewise unused. No control flow depended on it.
  • Membership-before-capacity ordering (duty_state.rs:128-141) and the len()-based cap check are preserved unchanged; len() behaves identically on both types.

Arity changes are mechanical and complete. The received_from parameter is dropped consistently through validatevalidate_decoded_messagevalidate_ssv_messagevalidate_partial_signature_messageupdate_for_partial_signature. grep -rn received_from anchor/ returns zero hits, confirming no orphaned references. The sole production caller (manager.rs:83) is updated in the same diff.

propagation_source is correctly retained. manager.rs still uses it for the gossipsub Outcome report (manager.rs:95) — it was only removed from the validate() call, not from the receive path. libp2p remains a dependency via the MessageAcceptance re-export (lib.rs:17). Both are as the description states.

Test collapse preserves coverage. I checked each deletion against its surviving counterpart:

  • ..._distinct_roots_same_proposal_slot_accepted — strict subset of criterion 3 (..._distinct_root_cap_then_over_cap_ignored), which accepts CAP distinct roots at one slot.
  • ..._duplicate_root_same_proposal_slot_ignored — subset of criterion 2 (..._repeat_root_ignored_as_relayed_duplicate).
  • ..._root_cap_enforced — subsumed by criteria 3 + 4.
  • Old criteria 7 (None own-emission) and 8 (double-relay by distinct peers) tested only the now-deleted peer axis; the peer-agnostic RelayedDuplicateMessage → Ignore pin they asserted survives in criterion 2. The RelayedDuplicateMessage → Ignore mapping assertion was correctly folded into the surviving repeat test.

No expectation values changed — every surviving test differs only by argument removal, rename, or comment edit, matching the stated acceptance criterion.

Notes (non-blocking)

  • The Codecov "missing lines" (patch 80.7%) are just the reworked arity lines in validate/validate_partial_signature_message/manager.rs; they carry no new branches, so this is a coverage-attribution artifact, not a real gap.
  • I was not able to run cargo test/cargo check in this environment (command approval required), so the "126 passed" and clean-lint results are as reported by the author, not independently reproduced here. The diff is self-consistent and I found nothing that would break compilation.
    · branch refactor/delete-received-from-plumbing

@shane-moore shane-moore left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewed at dc80d806.

No findings survived self-audit. The received_from value was write-only, and narrowing the root budgets from HashMap<Hash256, Option<PeerId>> to HashSet<Hash256> preserves membership, capacity, insertion, and per-kind budget semantics. The parameter removal is complete, while propagation_source remains available for the gossipsub outcome.

The surviving tests retain coverage of first-root acceptance, duplicate Ignore mapping, capacity and non-poisoning behavior, independent per-kind budgets, and ring retention. All 126 message_validator tests passed locally, message_receiver compiled, and current CI is green.

Ship-shaped.

Reviewed by gpt-5.6-sol max.

@shane-moore
shane-moore merged commit 52ba589 into sigp:epbs Aug 27, 2026
21 checks passed
@shane-moore
shane-moore deleted the refactor/delete-received-from-plumbing branch August 27, 2026 21:44
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.

2 participants