feat(disputes): verdict judgment length prefix + extrinsic caps + drop culprits rule (GP v0.8.0) (#1017)#1032
Conversation
…culprits rule (GP v0.8.0 #1017) Three v0.8.0 changes: - Wire (encodedisputes): each verdict's judgment sequence is now length-prefixed (var{...}); v0.7.x emitted a fixed ValidatorsSuperMajority entries. Encode emits the prefix, Decode reads and validates it still equals ValidatorsSuperMajority (floor(2V/3)+1 = 5 tiny / 683 full). Reflection registries stay on v0.7.x per #1015 precedent. - Extrinsic caps (eq:disputesextrinsics): |verdicts| <= 16 (Cmaxextrinsicverdicts), |culprits|, |faults| <= 16 (Cmaxextrinsicoffenses). New constants + rejection in Disputes() with provisional error codes too_many_verdicts/too_many_offenses (ordinals pend the official v0.8.0 error enum). - Rule removal: v0.8.0 keeps only 'good verdict requires >= 1 fault'; the v0.7.x 'bad verdict requires >= 2 culprits' rule is deleted, so ValidateCulprits and its call site are removed. NotEnoughCulprits keeps its ordinal (conformance mapping) but is no longer produced. Everything else verified already-aligned against the v0.7.2 -> v0.8.0 judgments.tex diff: good/bad/wonky set updates, offender tracking, rho clearing, age in {e, e-1}, active-vs-previous key-set selection, judge-index bounds, sort/uniqueness rules. Also: TestDisputes pointed at a non-existent vector dir and silently ran zero vectors -- path fixed to stf/disputes with a fail-on-empty guard, then skipped until official v0.8.0 vectors land. TestEncodeVerdict_V080LengthPrefix asserts the new byte layout.
| // GP v0.8.0 eq:disputesextrinsics caps the extrinsic sequences: | ||
| // |verdicts| <= Cmaxextrinsicverdicts, |culprits|,|faults| <= | ||
| // Cmaxextrinsicoffenses (both 16). | ||
| if len(disputeExtrinsic.Verdicts) > types.MaxExtrinsicVerdicts { |
There was a problem hiding this comment.
[SUGGESTION] Enforce extrinsic caps in DisputesExtrinsic.Validate()
GP v0.8.0 caps (|verdicts| ≤ 16, |culprits|, |faults| ≤ 16) are checked inline here. Consider moving them into the existing DisputesExtrinsic.Validate() in internal/types/types.go instead:
func (d *DisputesExtrinsic) Validate() error {
if len(d.Verdicts) > MaxExtrinsicVerdicts {
return errors.New("too_many_verdicts")
}
if len(d.Culprits) > MaxExtrinsicOffenses || len(d.Faults) > MaxExtrinsicOffenses {
return errors.New("too_many_offenses")
}
for _, verdict := range d.Verdicts {
if err := verdict.Validate(); err != nil {
return err
}
}
return nil
}ScaleDecode already calls Validate() after decode, so oversized extrinsics would fail at the type layer. Disputes() could then call disputeExtrinsic.Validate() (mapping the error string) rather than duplicating length checks.
| // ">= 2 culprits per bad verdict" rule (not_enough_culprits) was dropped, | ||
| // so both the .bin layout and several expected outputs no longer hold. | ||
| // Re-enable on official v0.8.0 vectors (#1012). | ||
| t.Skip("v0.7.x disputes vectors predate GP v0.8.0 (#1017); re-enable on official v0.8.0 vectors") |
There was a problem hiding this comment.
[SUGGESTION] Add unit tests for cap rejection
No test currently covers the too_many_verdicts / too_many_offenses paths. A small table-driven test (e.g. 17 verdicts / 17 culprits / 17 faults) would lock behaviour once caps live in Validate() or Disputes().
…-1017-v080 # Conflicts: # internal/types/encode_test.go
|
Thanks @yu2C — both applied:
|
…ate (review follow-up, #1017) Address yu2C's review on #1032: - The eq:disputesextrinsics sequence caps (verdicts <= 16, culprits/ faults <= 16) move from inline checks in Disputes() into DisputesExtrinsic.Validate(), so ScaleDecode rejects oversized extrinsics at the type layer too. Disputes() calls Validate() and maps the known error strings to conformance codes (unknown validate errors propagate as-is). - New table-driven TestDisputesExtrinsicCaps covers rejection at 17 verdicts / 17 culprits / 17 faults plus acceptance exactly at the caps.
|
@YCC3741 On it — to clarify, #1032 itself merged fine; the conflict GitHub was flagging lived on #1034 (its base picked up #1033's review follow-up, which touched the same test file as #1034's). Resolved and pushed: #1034 and #1035 both show MERGEABLE / CLEAN now, tests green at the top of the stack. Reminder from the #1012 thread for whoever merges next: after merging a stacked PR into its base branch, that base branch still needs to reach |
…ate (review follow-up, #1017) Address yu2C's review on #1032: - The eq:disputesextrinsics sequence caps (verdicts <= 16, culprits/ faults <= 16) move from inline checks in Disputes() into DisputesExtrinsic.Validate(), so ScaleDecode rejects oversized extrinsics at the type layer too. Disputes() calls Validate() and maps the known error strings to conformance codes (unknown validate errors propagate as-is). - New table-driven TestDisputesExtrinsicCaps covers rejection at 17 verdicts / 17 culprits / 17 faults plus acceptance exactly at the caps.
…ate (review follow-up, #1017) Address yu2C's review on #1032: - The eq:disputesextrinsics sequence caps (verdicts <= 16, culprits/ faults <= 16) move from inline checks in Disputes() into DisputesExtrinsic.Validate(), so ScaleDecode rejects oversized extrinsics at the type layer too. Disputes() calls Validate() and maps the known error strings to conformance codes (unknown validate errors propagate as-is). - New table-driven TestDisputesExtrinsicCaps covers rejection at 17 verdicts / 17 culprits / 17 faults plus acceptance exactly at the caps.
Part of #1012 · Closes #1017
What — three v0.8.0 changes
var{...}); was a fixed⌊2V/3⌋+1entries. Decode still validates the count (5 tiny / 683 full)encodedisputes(serialization.tex)ValidateCulpritsand its call site removed (the issue text didn't mention this deletion)Already aligned (verified against the tex diff, no change): good/bad/wonky set updates, offender tracking (
eq:offendersdefis a LaTeX re-flow only), rho clearing, age ∈ {e, e−1}, active-vs-previous key-set selection, judge-index bounds, sort/uniqueness rules. The κ-vs-V threshold re-expression is a no-op here (offender keys zeroed in place ⇒ |κ| ≡ V).Notes
NotEnoughCulpritskeeps its ordinal (conformance error mapping) but is no longer produced;too_many_verdicts/too_many_offensesordinals are provisional until the official v0.8.0 error enum ships.TestDisputespointed at a non-existent vector dir and silently ran zero vectors. Path fixed tostf/disputeswith a fail-on-empty guard, then skipped (v0.7.x vectors predate both the wire change and the culprits-rule removal).Tested
TestEncodeVerdict_V080LengthPrefix(new) — round-trip + prefix at byte offset 36, total length assertgo build/go vet/gofmtclean;internal/extrinsic,internal/typesgreen