Boole hardening: fork-aware partial-sig cap, checked runner assertions, role-mapper lockstep test (#2978 items 3, 4, 7) - #2989
Boole hardening: fork-aware partial-sig cap, checked runner assertions, role-mapper lockstep test (#2978 items 3, 4, 7)#2989momosh-ssv wants to merge 4 commits into
Conversation
…item 3) Pre-fork, enforce the pre-boole envelope (1512 msgs, ~229 KB) instead of the post-fork AggregatorCommittee worst case (5048 msgs, ~763 KB), keeping the pre-fork decode DoS surface at its pre-boole size. The switch is wall-clock based (slot is unknown before decode) and flips one epoch early to avoid rejecting boundary messages. Drift-guarded in const_test.go against the spec v1.2.2 worst case.
…em 4) A CreateRunnerFn returning a mismatched runner type now surfaces as a descriptive error instead of a bare interface-conversion panic.
…2978 item 7) message.RunnerRoleToString and ssvtypes.RunnerRoleToString/utils.FormatRunnerRole must produce the same strings; the contract lived only in doc comments.
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Greptile SummaryThe PR makes partial-signature validation choose a smaller pre-Boole encoded-data cap, replaces unchecked committee-runner assertions with descriptive errors, and adds a test keeping the two runner-role string mappers synchronized.
Confidence Score: 4/5The PR appears safe to merge, with only a non-blocking error-message capitalization issue in the checked runner assertions. The fork-aware cap, checked assertions, and mapper test have no established behavioral regression, while the two newly returned runner-type errors use formatting inconsistent with the repository convention. Files Needing Attention: protocol/v2/ssv/validator/committee.go
|
| Filename | Overview |
|---|---|
| message/validation/const.go | Adds a hand-derived pre-Boole partial-signature count and encoded-size cap, with no established correctness issue. |
| message/validation/const_test.go | Adds a hardcoded v1.2.2 size reference that guards the pre-fork cap from falling below the historical specification maximum. |
| message/validation/partial_validation.go | Applies the smaller cap before payload decoding and intentionally switches to the post-fork cap one epoch before activation. |
| observability/utils/format_test.go | Adds lockstep coverage for runner-role formatting across the role union supported by current forks. |
| protocol/v2/ssv/validator/committee.go | Replaces unchecked runner assertions with propagated errors; the new messages violate the repository's lowercase error convention. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Partial-signature gossip message] --> B[Decode signed SSV envelope]
B --> C{Boole active by next estimated epoch?}
C -->|No| D[Apply pre-fork encoded-data cap]
C -->|Yes| E[Apply post-fork encoded-data cap]
D --> F{Payload within cap?}
E --> F
F -->|No| G[Reject as data too big]
F -->|Yes| H[Decode PartialSignatureMessages]
H --> I[Validate message slot and fork-specific semantics]
Reviews (1): Last reviewed commit: "observability: lockstep test for the two..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| c.Runners[duty.DutySlot()] = r.(*runner.CommitteeRunner) | ||
| cr, ok := r.(*runner.CommitteeRunner) | ||
| if !ok { | ||
| return nil, fmt.Errorf("BUG: runner created for committee duty has type %T, expected *runner.CommitteeRunner", r) |
There was a problem hiding this comment.
The new type-mismatch errors begin with BUG:, contrary to the repository convention that error messages remain lowercase and concise; the aggregator mismatch at line 574 repeats the same formatting issue.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
- pin the fork gate of the partial-signature cap with a unit test (unscheduled / two-epochs-out / one-epoch-early flip / active) - extend the role-mapper lockstep test with a sweep over spec-known roles so a role added to only one mapper fails the test - guard createRunner against a nil runner returned without error - comment accuracy: the cap bounds the inner PartialSignatureMessages decode (outer decode is bounded by MaxEncodedMsgSize); note why the two drift guards compare against different spec constants; return r, nil explicitly
iurii-ssv
left a comment
iurii-ssv
left a comment
There was a problem hiding this comment.
LGTM, just minor suggestions
| ssv.Forks = networkconfig.SSVForks{Boole: booleEpoch} | ||
| return &networkconfig.Network{Beacon: networkconfig.TestNetwork.Beacon, SSV: &ssv} | ||
| } | ||
| currentEpoch := networkconfig.TestNetwork.EstimatedCurrentEpoch() |
There was a problem hiding this comment.
Minor (test robustness). currentEpoch is sampled from the wall clock here, but the helper samples EstimatedCurrentEpoch() again internally. If a real epoch boundary falls between the two reads, the "fork two epochs away" case flips to the post-fork cap and the assertion fails:
- setup reads epoch
E→boole = E+2,want = preFork - helper reads
E+1→BooleForkAtEpoch((E+1)+1)=(E+2) >= (E+2)= true → returns post-fork cap → mismatch
The window is ~microseconds within a multi-minute epoch, so this is astronomically rare rather than a real-world concern — but it is genuine nondeterminism. For full determinism, drive EstimatedCurrentEpoch() from a fixed/injected clock (or fixed genesis) instead of the live TestNetwork clock. The other three cases are immune.
| ssvMessage := signedSSVMessage.SSVMessage | ||
|
|
||
| if len(ssvMessage.Data) > maxEncodedPartialSignatureSize { | ||
| if maxSize := mv.currentMaxEncodedPartialSignatureSize(); len(ssvMessage.Data) > maxSize { |
There was a problem hiding this comment.
Test coverage. The codecov bot flags this file's uncovered lines; the untested one is this rejection branch (return nil, e for ErrSSVDataTooBig). TestCurrentMaxEncodedPartialSignatureSize exercises the cap selector, but nothing drives validatePartialSignatureMessage with a payload sized between the two caps. Consider a small end-to-end case: ssvMessage.Data in (preForkMaxEncodedPartialSignatureSize, maxEncodedPartialSignatureSize] is rejected pre-fork and accepted past the size gate post-fork. That closes the coverage gap and guards that the fork-aware cap stays wired into the validation path (not just the helper).
| // spec does not know return "UNDEFINED" and are skipped: divergence on genuinely | ||
| // unknown values is intentional (the deprecated Alan roles also stringify to | ||
| // "UNDEFINED" in the spec, but they are covered by the explicit list above). | ||
| for i := 0; i <= 15; i++ { |
There was a problem hiding this comment.
Nit. The sweep upper bound 15 is arbitrary — a spec role added at value ≥ 16 would slip past this drift guard. Enum values are sequential today (0–6), so it isn't a practical gap, but a one-line note on why 15 (headroom) — or deriving the bound — would make the intent explicit.
|
|
||
| // preForkMaxPartialSignatureMessages is the pre-boole worst case (RoleCommittee, | ||
| // min(2*V, V+SYNC_COMMITTEE_SIZE) with the spec's V=1000 bound), matching pre-boole | ||
| // ssv-spec v1.2.2 maxmsgsize.maxSizePartialSignatureMessages (1512 messages, 217748 |
There was a problem hiding this comment.
Nit (doc clarity). The parenthetical "217748 bytes" is the spec's SSZ value (20 + 1512·144), whereas preForkMaxPartialSignatureMsgsSize on line 63 evaluates to 217744 — it omits the 4-byte SSZ offset for the dynamic Messages field. Harmless (the encoding-overhead margin absorbs it, and it matches how maxPartialSignatureMsgsSize is computed), but a reader diffing 217748 vs 217744 may pause. A half-sentence noting the local figure is pre-offset would help.
| } | ||
| c.AggregatorRunners[duty.DutySlot()] = ar | ||
| default: | ||
| c.logger.Panic("BUG: attempt to create committee runner with non-committee duty type", |
There was a problem hiding this comment.
Optional / non-blocking. The two runner type-mismatch cases above now return errors, but this default (wrong duty type) still logger.Panics. The distinction is defensible — duty type is internally controlled here, whereas the runner type comes from the injected CreateRunnerFn — but the asymmetry (BUG → return vs BUG → panic) is easy to trip over. A one-line comment on why this one stays a panic would preempt the question. Fine to leave as-is.
2 participants
Knocks out the three self-contained code items from #2978 (Boole convergence hardening follow-ups).
Item 3 — fork-aware partial-signature size cap
Before the Boole convergence, message validation accepted encoded partial-signature payloads up to ~151 KB. The convergence raised that to the post-fork AggregatorCommittee worst case (~763 KB), and the new cap applied even before the fork — a ~5x bigger decode surface on networks where Boole isn't even scheduled.
Now the pre-fork cap is enforced until the fork: 1512 messages (~229 KB), switching to the post-fork cap at Boole activation. Two details worth a reviewer's attention:
maxmsgsize) puts the structural worst case forPartialSignatureMessagesat 1512 messages (min(2V, V+SYNC_COMMITTEE_SIZE)with V=1000) — the old hand-picked 1000 actually sat slightly below spec. Using 1512 means we never reject anything the pre-fork spec allows, while still cutting the surface ~3.3x. The value is drift-guarded inconst_test.goagainst the v1.2.2 constant (hardcoded there, since only one spec version can be pinned).Item 4 — checked type assertions in
createRunnerCommittee.createRunnerassertedr.(*runner.CommitteeRunner)/r.(*runner.AggregatorCommitteeRunner)unchecked. The invariant holds today, but a futureCreateRunnerFnreturning a mismatched type would crash with a bare interface-conversion panic. Both assertions are now checked and return a descriptive error instead.Item 7 — lockstep test for the runner-role string mappers
message.RunnerRoleToStringandssvtypes.RunnerRoleToString(viautils.FormatRunnerRole) are independent mappers that must produce identical strings, but the contract lived only in doc comments.TestRunnerRoleStringMappersLockstepnow asserts equality for every runner role valid in any fork, so drift fails CI instead of silently splitting duty IDs from exporter strings.What did NOT change
MaxEncodedMsgSize) is untouched — it stays at the post-fork maximum in all cases, as it must (a gossip message-size limit can't change at the fork without splitting the mesh).validatePartialSigMessagesByDutyLogicare unchanged.defaultbranch ofcreateRunnerkeeps its existinglogger.Panic.Closes nothing on its own — items tracked in #2978.
Testing
go test ./message/validation/ ./observability/utils/ ./protocol/v2/ssv/validator/...all pass, including the extended size-cap drift guard and the new lockstep test.