feat: admit one envelope dissemination per signer and sign by binding - #1288
Draft
shane-moore wants to merge 4 commits into
Draft
feat: admit one envelope dissemination per signer and sign by binding#1288shane-moore wants to merge 4 commits into
shane-moore wants to merge 4 commits into
Conversation
SIP-94 §7 admitted one EnvelopeDissemination per (MessageId, slot), signer-independent, and §6 signed whichever one arrived first. The first structurally valid carrier was therefore the only one an operator would forward or consider, so a committee member that won the race with a well-formed but decision-unbound envelope made every non-builder abstain and cost the cluster its self-build reveal. Admit one per (MessageId, signer, slot) instead, and have the non-builder task sign the first candidate whose envelope passes the decided context's four bindings rather than the first that arrives. Forwarding stays bounded because only committee members pass validation. The validator's dedup record moves from a signer-independent ring on DutyState to a flag on the signer's own SignerState, which is already the per-signer per-slot record and already carries the max_slot and per-epoch duty-count effects the old record reproduced by hand. Nothing can wipe the flag: the only path that replaces a SignerState is a consensus message, and those are rejected for non-QBFT roles before any state update. The store keeps every accepted candidate per (validator, slot) with a watch counter, and wait_matching walks them once each in arrival order, running the caller's predicate outside the lock. Waiters subscribe to the counter in the same critical section that reads the candidates: a watch receiver treats the value present at subscription as seen, so subscribing afterwards could sleep to the deadline with a match already stored. DisseminationUndecodable goes with the terminal decode arm that built it. An undecodable candidate is now skipped like any other unusable one, and is unreachable through gossip regardless, which decodes the inner envelope before accepting. This does not close the residual: payload_root is unchecked by design, so a forgery that passes all four bindings and arrives first is still signed. A test pins that, so adopting sign-all flips a test rather than a comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SJP7vu2EGTFknFswZpjNjJ
Four review angles over the previous commit's diff. What survived: The signer now rides ValidatedSSVMessage::EnvelopeDissemination instead of being re-derived in the receiver. Validation already proves there is exactly one signer and binds it, so the receiver was recomputing known information into a tuple match whose failure arm conflated two conditions validation guarantees separately. The receiver arm returns to the single-pattern match it had before the signer was needed. The store keeps candidates behind an Arc. A candidate carries up to SSVMessageDataLen bytes and was deep-copied under the same mutex the gossip receiver takes, which let a remote committee member's byte count set this node's lock-hold time. A visit is now a refcount bump. The watch payload carries nothing: only changed() was ever read, so the usize and its hand-written Default impl are gone in favour of a derive. The per-call sweep moves above the loop, since the slot is fixed for the call and a key that survives one sweep cannot go stale during it. The rejection counter is a plain usize. FnMut grants the mutation, let-else drops the initializer's temporaries before the divergent block, and &mut usize keeps the spawned future Send, so the atomic was advertising cross-thread sharing that does not exist. The predicate flattens to two inspect_err chains, dropping a duplicated increment-and-warn tail. Tests: one insert helper instead of three, and the store's deadline-only tests run on a paused clock rather than real time. Tried and rejected: a shared get_or_create_signer_state on OperatorState. Three angles suggested it, but it cannot borrow through &mut self in both match arms (E0499), and every form that compiles introduces an expect. The existing partial-signature copy only compiles because it reborrows through a local binding. The total, panic-free version stays. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SJP7vu2EGTFknFswZpjNjJ
Four context-free lanes over the finished diff: defect lenses, architecture, concurrency, and spec conformance. Verdict was that the shape is right and not over-engineered, with these corrections. The wakeup signal moves from the map entry to the store. Tying it to the entry is what forced waiters to subscribe under the same lock they read candidates under, and it made an evicted entry look like "nothing more can arrive" when a later insert for the same slot can re-create the key. A store-level sender outlives every entry, so a waiter subscribes once before its first read and no sweep can strand it. The Entry struct, its Default, the six-line hazard comment, and the sender-dropped arm all go, and a waiter no longer creates a map entry just to wait. Two guards were documented in prose but pinned by nothing: mutation testing showed that making record_dissemination always replace the SignerState, and dropping the ring's stored-slot filter from is_dissemination_recorded, both passed the entire suite. The first is a live budget-reset bug, since gossip does not order a signer's dissemination against its own Envelope share and the two classes share one per-(signer, slot) entry. The second would silently Ignore an honest dissemination exactly stored_slot_count slots after a recorded one. Both now have tests, and both tests were confirmed to fail against the mutation before being kept. Two comments were wrong or incomplete. The dissemination flag's survival rests on role 9 having no consensus path, since that is the only writer that replaces a SignerState outright; that now says so at the field. And when a binding-passing forgery races the honest carrier, operators split across two roots and the symptom is a signature collection timeout, not the dissemination timeout the surrounding comment implied. Declined, recorded here so the reasoning is not relitigated: a candidate cap in the store. Three lanes asked for it. It cannot fire on the validated path, where at most one candidate per committee member is admitted, so it only fires once some other invariant is already broken, and then it drops an arbitrary candidate that may be the honest one. That trades a bounded transient memory cost for a missed reveal. Also declined: a yield point in the drain and a cursor-generation guard, both unreachable under the current slot invariant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SJP7vu2EGTFknFswZpjNjJ
Two wording fixes from an adversarial review of the finished branch. The sweep comment asserted that a surviving key cannot go stale during the wait because the slot is fixed, which restates the conclusion instead of giving the reason. The reason is the deadline: eviction needs an insert carrying a slot at least MAX_DISSEMINATION_AGE_SLOTS + 1 ahead, roughly a minute out, while envelope_deadline falls half a slot into the slot itself. The margin is an order of magnitude, but nothing in the comment pointed at the deadline, so a later change moving it past a slot boundary would have had no warning that it makes the swept-then-recreated key reachable and leaves the cursor past candidates it never visited. Candidates are also in insertion order, not arrival order. The receiver dispatches validation onto a worker pool, so the order the store sees is the order that pool finished in, which need not match the network. Selection is by binding and any binding-passing candidate is acceptable, so nothing changes behaviourally, but "arrival order" promises a determinism the store cannot give. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SJP7vu2EGTFknFswZpjNjJ
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## epbs #1288 +/- ##
=======================================
Coverage ? 78.80%
=======================================
Files ? 178
Lines ? 39653
Branches ? 0
=======================================
Hits ? 31250
Misses ? 8403
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
shane-moore
commented
Sep 3, 2026
Member
Author
There was a problem hiding this comment.
Reviewed 571590c1 against epbs at e874c1ea.
No actionable findings. Per-signer admission is recorded only after all validation, signer state preserves prior partial-signature accounting, the candidate handoff is committee-bounded and lost-wakeup safe, and the non-builder path skips unbound candidates while preserving the payload-due deadline. The residual unchecked payload_root behavior is explicit and tested.
Focused local tests (319), formatting, Clippy with warnings denied, and diff checks passed. All exact-head GitHub checks, including the local testnet, are green.
Reviewed by gpt-5.6-sol xhigh.
shane-moore
added a commit
to shane-moore/SIPs
that referenced
this pull request
Sep 3, 2026
Anchor implemented this in sigp/anchor#1288 and validated it on a Gloas devnet, so the rule is known implementable before it is written down here. Section 7 keyed the dissemination dedup on (MessageID, slot), so the first structurally valid carrier was the only one an operator would forward or consider. One committee member broadcasting a well-formed but decision-unbound envelope ahead of the builder therefore made every operator abstain, costing the cluster its reveal. Dedup is now one per (MessageID, signer, slot), and section 6 signs the first candidate whose envelope satisfies the four bindings rather than the first that arrives, continuing to the payload-due cutoff. The previous commit reconciled section 6 to section 7 after a review found them disagreeing about what first-valid meant. That resolved the conflict in the wrong direction: section 6's original content-based reading was correct, and section 7's per-slot dedup was what made it unreachable. Two rules that were implicit are now stated, because an implementation that reads them differently is exploitable or unbounded. Recording must follow the signature check, or a forged carrier claiming another operator's identity consumes that operator's budget and suppresses its honest message. And forwarding is bounded by committee size, which holds only because the signature check is scoped to committee members. Security Considerations narrows accordingly: the binding-failing carrier is no longer a liveness risk, and what remains is a binding-passing forgery that differs only in the unchecked PayloadRoot, splitting operators across two roots by node-local arrival order. That case tolerates no Byzantine member when ExecutionRequests is empty, and the named hardening is now sign-all relative to the new rule rather than to first-valid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SJP7vu2EGTFknFswZpjNjJ
shane-moore
marked this pull request as draft
September 7, 2026 20:13
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
2 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.
Problem, Evidence, and Context (Required)
A single committee member could cost the cluster its self-build reveal by winning a race.
SIP-94 §7 admitted one
EnvelopeDisseminationper(MessageId, slot), signer-independent, and §6 signed whichever one arrived first. The first structurally valid carrier was therefore the only one an operator would forward or consider. A member that broadcast a well-formed but decision-unbound envelope ahead of the builder made every non-builder abstain, and the payload never revealed. Nothing about that requires a sophisticated attacker: one misbehaving member, once, is enough.Both halves of the rule come from review of the SIP by Iurii (ssvlabs), whose asks were classified MUST where they are observable on the SSV wire. These two are: a go-ssv implementation that dedups per slot instead of per signer would drop disseminations Anchor admits, and the two would disagree about which envelope to sign. The SIP text edit follows this PR rather than preceding it, so the rule is proven implementable before it is written down.
Builds on #1286.
Change Overview (Required)
Admit one dissemination per
(MessageId, signer, slot), and have the non-builder task sign the first candidate whose envelope satisfies the decided block's four bindings rather than the first that arrives. Forwarding stays bounded because only committee members pass validation, so the candidate list is capped by committee size.Read it in this order:
message_validator/src/duty_state.rs- the dedup record moves from a signer-independent ring onDutyStateto a flag on the signer's ownSignerState, which is already the per-signer per-slot record and already carries the max-slot and per-epoch duty-count effects the old record reproduced by hand.message_validator/src/dissemination.rs- the validation rule itself.common/dissemination_store/src/lib.rs- the store keeps every accepted candidate per(validator, slot)behind anArc, andwait_matchingwalks them once each, running the caller's predicate outside the lock.validator_store/src/lib.rs-sign_disseminated_envelopesupplies that predicate.message_receiver/src/manager.rsandoperator_doppelganger/src/service.rs- mechanical pattern updates for the reshaped variant.What did not change: the wire format, the four bindings themselves, the builder's own publish path, and the deliberate decision to leave
payload_rootunchecked. No Lighthouse changes.Risks, Trade-offs, and Mitigations (Required)
The accepted residual is unchanged and still real.
payload_rootis not among the checked bindings, so a forgery that passes all four bindings while carrying a different payload root splits operators across two roots. Arrival order is node-local, so each root collects some shares and neither reaches threshold. The symptom is a signature collection timeout, never a wrong payload. This is documented at the selection site.A store entry now holds a list rather than one value. Bounded on the validated path by committee size, at most one candidate per member per slot. A cap was proposed in review and declined: it cannot fire unless some other invariant is already broken, and it would then drop a candidate that may be the honest one, trading bounded transient memory for a missed reveal.
The dedup flag is never cleared. That rests on role 9 having no consensus path, since
OperatorState::updateis the only writer that replaces aSignerStateoutright and it is reachable only for QBFT roles. A future role that both disseminates and runs QBFT would need the bit carried across the replacement. Stated at the field.Validation (Required)
Unit: 12 in
dissemination_store, 139 inmessage_validator, 168 inanchor_validator_store, workspace otherwise green,make cargo-fmt-checkandmake lintclean.Two guards were documented in prose but pinned by no test, found by mutation testing. Making
record_disseminationalways replace theSignerStatepassed the entire suite, and is a live budget-reset bug: gossip does not order a signer's dissemination against its own partial signature and the two share one per-signer-per-slot entry. Dropping the ring's stored-slot filter also passed, and would silently ignore an honest dissemination exactly one ring length later. Both now have tests, each confirmed to fail against its mutation.Devnet: 4-operator all-Anchor Gloas run on ssv-mini, operators split across two beacon nodes so each proposal has both builder and non-builder operators. 11 managed proposals, 11 of 11 envelopes revealed on chain and served by the BN. The accounting invariant held exactly on every operator (
signed_other + published == received, anddelegated == lh_sentinel == signed_other). Zero dissemination timeouts, zero binding mismatches, zero skipped candidates, zero validation failures, zero restarts. Reveal timing 0.15s to 0.39s into the slot.That topology also exercises the §7 change directly, which is worth stating because it is not obvious: two operators share the builder's beacon node and build the identical block, so both broadcast, putting two disseminations from two distinct signers on one key every proposal. Under the old per-slot rule the second would have been rejected at validation on every receiving node.
Not exercised, and not claimed: a candidate that fails the bindings. Both candidates in a healthy cluster pass, so the skip-and-continue path never runs. Demonstrating it needs a patched operator broadcasting a binding-failing envelope.
Rollback (Required for behavior or runtime changes; optional otherwise)
Revert the four commits. No config, no schema, no wire change, so a reverted node interoperates with a non-reverted one except that it reverts to admitting one dissemination per slot.
Blockers / Dependencies (Optional)
The SIP-94 §6/§7 text edit follows this PR.
Additional Info / Next Steps (Optional)
The review found eight points a second implementer could resolve differently, which will go into the SIP text rather than staying here. Two are sharp: recording a claimed signer before verifying its signature would be a censorship primitive, and the committee-size bound on the candidate list only holds if the text requires the signer to be a committee member rather than merely a known operator.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SJP7vu2EGTFknFswZpjNjJ