WIRE-301: Retire legacy staking attestations - #544
Conversation
Change-Id: I6295e0e573de1a523870927964b07419518febd3
Change-Id: I5089a8c7ef276f6977909595d7616e448e7e751d
Change-Id: I854a2f145483551ecde57249a92b19265cc2e9ce
heifner
left a comment
There was a problem hiding this comment.
Review — high effort
Read the full diff plus the surrounding buildenv / dispatch_attestation / estimate_svm_dynamic_accounts code, the KV secondary-index erase contract in wire-cdt, the ABI enum fallback in abi_serializer, the FC_REFLECT_ENUM reflector, and the Solidity generator.
4 findings inline — 1 medium (the proto reserved list is incomplete), 3 low.
Verified clean — stating these so they don't get re-litigated
- The
it = idx.erase(std::move(it))pattern matches the two pre-existing loops later in the same function. CDT'seraseadvances before erasing, the move is required (move-only iterator), every loop path increments, and theit != end()short-circuit holds — no infinite loop, no end-iterator deref. - Dropping the two values from
sysio.msgch.abi/sysio.uwrit.abidoes not break table reads:abi_serializer::_binary_to_variantfalls back to the raw integer for unknown enum values. - Dropping them from
FC_REFLECT_ENUMis safe — the only host-side consumer (underwriter_plugin) resolves viaAttestationType_Parse/_Name, which returnsfalse/""for unknown values andcontinues. I diffed theFC_REFLECT_ENUMmember list against the proto enum: exact match. dispatch_attestationhas adefault: break;, so retired wire values genuinely land on the documented no-op path.- The
reservedstatements sitting aboveATTESTATION_TYPE_UNSPECIFIED = 0do not violate proto3's first-value-must-be-zero rule, and the max enum value is unchanged (60962) — no Solidity UDVT width change from this PR. encode_envelope_padded_to's switch from STAKE (2-byte varint) to CHALLENGE_RESPONSE (3-byte varint) is self-correcting via the probe pass, and CHALLENGE_RESPONSE has no dispute side effect inevalcons— an equivalent inert padding type.
The two I'd act on before merge are the proto reserved gap (mechanical, closes the whole class of retired-slot reverts) and the unbounded prune (a bounded per-call cap, matching the pattern 3673936b9f already established).
| // --------------------------------------------------------------------------- | ||
|
|
||
| enum AttestationType { | ||
| reserved 3001, 3002; |
There was a problem hiding this comment.
Only 2 of the ~12 retired slots are reserved.
The new generator passthrough keys off reserved_range, so only 3001/3002 get the fromRaw blackhole. The same enum documents ~10 other retired numeric slots as bare comments — 60929 (NATIVE_YIELD_REWARD), 60933 (SLASH_OPERATOR), 60935–60938 (UNDERWRITE_*), 60946 (EPOCH_SYNC), 60948 (REMIT_CONFIRM), 60954, 60957 — and those still hit revert InvalidEnumValue in the generated AttestationTypeLib.fromRaw.
Concrete failure: a historical depot→ETH envelope carrying attestation type 60933 reaches OPPInbound. The whole envelope decode reverts, the outpost never reaches consensus for that epoch, and OPP circulation halts — the exact failure mode this PR's blackhole was built to prevent, just for a different retired number.
Adding those numbers to the reserved list costs nothing and closes the class.
| // Erase them before destination-specific estimation so neither the SVM | ||
| // terminal-account gate nor an outpost decoder can be blocked by a | ||
| // protocol value that no longer has a generated enum/message type. | ||
| if (is_retired_staking_attestation(it->type)) { |
There was a problem hiding this comment.
Unbounded prune inside an inline action.
This erases every retired READY row across all chains in a single transaction, and buildenv runs as an inline action inside sysio.epoch::advance.
If the residual legacy row count is large enough that N × (primary erase + 3 secondary-index erases) exceeds the transaction CPU budget, advance reverts. The erases roll back with it, so every subsequent advance retries the identical work and reverts again — a deterministic, non-self-healing epoch stall.
The immediately preceding master commit (3673936b9f, uwrit UWREQ expiry) used bounded epoch-driven pruning for this same class of migration. A per-call cap plus a "skip, don't candidate" branch for the over-budget remainder would make this drain across epochs instead of all-or-nothing.
Rating this low because the realistic residual count is small — an EVM-bound STAKE row would already have been packed and drained by the pre-upgrade contract, so only SVM-bound rows survive — but the failure mode has no recovery path if it does land.
There was a problem hiding this comment.
Addressed on current head 2c29ce1. buildenv caps retired-row erasure at 32 per call and continues skipping every over-budget retired row as an envelope candidate, so cleanup drains deterministically across calls without forwarding tombstones or turning one advance into an unbounded erase sweep. Coverage includes 33 retired rows followed by an active row; the active row emits and exactly one tombstone remains READY.
| values: EnumValueInfo[], | ||
| reservedRanges: EnumReservedRangeInfo[] = [] | ||
| ): string { | ||
| const maxReserved = reservedRanges.map(range => range.end) |
There was a problem hiding this comment.
reserved N to max; would silently widen the UDVT.
reserved N to max; is a common protobuf idiom and yields end = 2147483647. Folding that into the width computation promotes the enum from uint8 to uint32, changing the ABI encoding of every generated Solidity struct that contains it — a wire-incompatible break triggered by a declaration that adds no valid values.
A reserved slot is by definition never a valid value, so it arguably shouldn't participate in sizing at all: either clamp/ignore open-ended ranges, or size on declared values only and range-check in fromRaw before the cast.
There was a problem hiding this comment.
Addressed on current head 2c29ce1. Solidity enum width is derived only from declared active values. fromRaw checks type(underlying).max before any reserved-value cast, so an open-ended reservation cannot widen the UDVT or be truncated during narrowing. Generator regressions cover both active-value width selection and overflow-before-cast; all 84 generator tests pass.
| /// value are tombstoned before destination-specific envelope construction. | ||
| /// Active rows behind the tombstone still emit normally, so one legacy row | ||
| /// cannot strand the queue or reach an outpost decoder. | ||
| BOOST_FIXTURE_TEST_CASE(buildenv_tombstones_retired_staking_rows, |
There was a problem hiding this comment.
Coverage gap: this only exercises the EVM path.
The scenario the tombstone primarily protects against is the SVM one — where an untombstoned retired row hits check(estimate.has_value(), "no Solana terminal account estimate for READY attestation") and hard-reverts buildenv. That path is untested.
Also untested: the retired-row-is-the-last-READY-row case, where erase must return the end iterator and the loop condition has to short-circuit before dereferencing.
Both are cheap additions to this same fixture.
Change-Id: I54ece9a5e0960db22552d75ab8a85dc6e7a9390f
…-stake-unstake Change-Id: I35809cf5227b54138a5588c96c5d45c56a04a6c4
Change-Id: I07bc8da01d67db07ef97f2349fe10d6d9eaffd53
Change-Id: I5c06591c04db2c66d2fa9d792f37579c4c258c5b
huangminghuang
left a comment
There was a problem hiding this comment.
Addressed all four review findings on current head 2c29ce1; implementation and validation details are replied inline.
| // Erase them before destination-specific estimation so neither the SVM | ||
| // terminal-account gate nor an outpost decoder can be blocked by a | ||
| // protocol value that no longer has a generated enum/message type. | ||
| if (is_retired_staking_attestation(it->type)) { |
There was a problem hiding this comment.
Addressed on current head 2c29ce1. buildenv caps retired-row erasure at 32 per call and continues skipping every over-budget retired row as an envelope candidate, so cleanup drains deterministically across calls without forwarding tombstones or turning one advance into an unbounded erase sweep. Coverage includes 33 retired rows followed by an active row; the active row emits and exactly one tombstone remains READY.
heifner
left a comment
There was a problem hiding this comment.
Reviewed 2c29ce1b03 against the previously reviewed 1672016757. All four findings are addressed. One new LOW — found by walking the proto's git history rather than its comments, which is where my original list came from.
Verification of the four findings
1. MEDIUM — incomplete reserved list → addressed for everything I listed
reserved 3001, 3002, 60929, 60933, 60935 to 60938, 60946, 60948, 60954, 60957; — an exact match to the ten I named. See the new finding below; my list was itself incomplete.
2. LOW — unbounded prune → FIXED, exactly the suggested shape
MAX_RETIRED_STAKING_PRUNE_PER_BUILD = 32, with over-budget retired rows taking the ++it skip branch so they are never envelope candidates (sysio.msgch.cpp:1733-1741). I re-walked the loop: every path still advances it, and the it != end() && status == READY condition still short-circuits. Drain is 32 per buildenv call per chain, and the scan continues past the cap so active rows behind tombstones still emit — which the new test asserts directly.
3. LOW — reserved N to max; widening the UDVT → FIXED, and the guard is real
computeUnderlyingType now sizes on declared values only, and fromRaw gets if (_raw > type(<underlying>).max) revert emitted before the reserved-range casts.
I checked the thing that would have made this cosmetic: fromRaw's parameter is always uint64 regardless of the underlying type (enum.ts:105), so the guard genuinely catches an unrepresentable reserved value rather than being a tautology. Ordering is right — active values match by equality first, reserved values below the cap wrap, reserved values above it revert. The two new generator tests assert that ordering via indexOf rather than mere presence.
4. LOW — coverage gap → FIXED, and the fixture hack is sound
All three cases landed: SVM path, last-row erase, and the 33-row cap with an active row behind.
I checked the part that could have made the SVM test hollow. retarget_attestation_for_upgrade_test mutates chainbase KV directly, so I verified the claim in its comment: attestations_t declares exactly three secondary indices — bystatus, bytype, byepoch — and none of them indexes chain_code (sysio.msgch.hpp:192-199), so the retarget leaves no stale index entry behind. The hack is also genuinely necessary: queueout:1657-1660 hard-checks estimate_svm_dynamic_accounts(...).has_value() for SVM destinations, so a retired type cannot be queued there through the action.
Also verified
- The committed
sysio.msgch.wasmhashes toa01ce94d99b98566845092bb33f4699028b3d151973934deb4360d73bb6a2df1, matching the PR body. - The diff against the merge base is still 23 WIRE-301-scoped files, so the master merge did not drag unrelated content into the PR's own diff.
- Active max is still 60962 →
uint16, so no UDVT width change.
New finding
LOW — five more historically-declared slots are still unreserved
libraries/opp/proto/sysio/opp/types/types.proto:232
My original list came from the retirement comments in the enum, and the reserved list now matches it exactly. But walking all 33 revisions of types.proto turns up 17 values that were once declared and are gone today — and five of them are in neither the active set nor reserved:
| Slot | Former name | Where it went |
|---|---|---|
60931 |
ATTESTATION_TYPE_OPERATOR_REG_DEREG |
became OPERATOR_ACTION = 2001 |
60939 |
ATTESTATION_TYPE_CHALLENGE_REQUEST |
renumbered to 60945 |
60940 |
ATTESTATION_TYPE_EPOCH_SYNC |
renumbered to 60946, then retired |
60941 |
ATTESTATION_TYPE_ROSTER_UPDATE |
no current equivalent |
60942 |
ATTESTATION_TYPE_REMIT_CONFIRM |
renumbered to 60948, then retired |
These hit revert InvalidEnumValue in the generated AttestationTypeLib.fromRaw exactly like the ten now covered, and the PR's own stated criterion is "reserve every historical raw protobuf slot that no longer has an active enum member".
I would calibrate the severity down from my original framing, and it is worth saying plainly: per no-back-compat-before-release.md nothing is deployed, so there is no field envelope carrying 60931 — the decode-revert scenario is bounded to long-lived test clusters whose state predates the change.
What does hold regardless of release status is reserved's primary purpose: preventing accidental reuse of a vacated number. That hazard is not hypothetical here — the history shows 60957 already carried two different names (SWAP_REJECTED, then EMISSIONS_BLOCKED) before being retired. Adding five numbers to the existing line costs nothing.
Note
ATTESTATION_TYPE_EMISSIONS_BLOCKED = 60962 arrived from the master merge documented as "DEPRECATED — no longer emitted", but stays declared rather than deleted-and-reserved. Keeping it declared is the safer state (it decodes rather than reverts), so this is not a defect — but the release now carries two different retirement conventions for the same enum. Worth deciding which one is the standard before a third retirement picks arbitrarily.
I did not rebuild the WASMs from source. I verified the committed sysio.msgch.wasm matches the hash you published, but source-to-binary reproduction across the six changed contracts is the Release build's job, not something confirmable from the diff.
…-stake-unstake Change-Id: I85dc5a951f109d851e895643b9e10adf6fca0e19
Change-Id: I07c0b476f48297807dc3a1c45fcff926e3a397bf
heifner
left a comment
There was a problem hiding this comment.
Re-reviewed 90e92f54d2 against the previously reviewed 2c29ce1b03. The PR-owned delta is one file — types.proto — plus a master merge that brought only PR #547's Python test.
The open finding is fixed, and I verified it mechanically rather than against my list
My original list came from reading the retirement comments, which is why it was incomplete twice. This time I scripted it: walked all 35 revisions of types.proto, extracted every ATTESTATION_TYPE_* = N ever declared, and diffed against head.
- 42 distinct values ever declared. Head has 25 active + 17 reserved = 42. Zero gaps.
- Also zero reserved numbers that were never declared — the list is not over-reserved either.
protocparses the whole head proto tree clean, which independently proves no reserved value collides with a declared one.- The descriptor dump confirms 10
reserved_rangeentries, with60935 to 60942as one inclusive range.
I checked the thing that could have made that last point cosmetic: the Solidity plugin hand-rolls EnumDescriptorProto rather than using the official descriptor, and its reserved_range field number (4) and start/end (1/2) match descriptor.proto. Enum reserved ranges are inclusive on both ends there — unlike message reserved ranges — so the generator genuinely sees all 17 and the emitted bounds are right.
The final commit cannot have invalidated the committed binaries
protoc-gen-zppcontains no reference toreservedanywhere in its source, so a reserved-only proto edit cannot change the generated CDT headers. The WASMs predating this commit are still correct for it.- The final master merge brought only
tests/nodeop_chainbase_allocation_test.py— no contract sources. sysio.msgch.wasmstill hashesa01ce94d99b98566845092bb33f4699028b3d151973934deb4360d73bb6a2df1.
I did not rebuild from source; that remains the Release build's job.
New finding — LOW: four other OPP enums are still in the pre-PR state
Running the same history walk across every enum in every OPP proto turns up four with vacated slots documented by comment only, or not at all:
| Enum | Vacated slot(s) | Former name | Documented? |
|---|---|---|---|
ChainKind |
4 | CHAIN_KIND_SUI |
comment only |
TokenKind |
256, 257, 258, 259, 496, 512, 752 | ETH, ERC20, ERC721, ERC1155, LIQETH, SOL, LIQSOL |
comment only |
ActionType |
5 | ACTION_TYPE_WITHDRAW_CONFIRMED |
no |
UnderwriteStatus |
4 | UNDERWRITE_STATUS_SLASHED (now 10) |
no |
The reuse hazard is not hypothetical here. TokenKind's own comment records that 257/258/259 were re-issued at slots 2/3/4. AttestationType did it too: slot 60953 carried EMISSIONS_BLOCKED on 5/04 and was reused for UNDERWRITE_INTENT_COMMIT on 5/08.
One precision worth stating: for TokenKind the vacated values sit above the narrowed uint8, so the overflow guard added in this PR fires first and fromRaw still reverts. Reserving those buys reuse-prevention only, not decode tolerance. ChainKind 4, ActionType 5, and UnderwriteStatus 4 would get both.
Fine to take here or as a follow-up — the point is that the exhaustive-walk methodology should apply enum-wide rather than to one enum.
New finding — LOW: the runtime tombstone covers 2 of the 17 retired slots
is_retired_staking_attestation hardcodes 3001/3002. estimate_svm_dynamic_accounts ends in default: return std::nullopt, and buildenv hard-checks estimate.has_value(), so an SVM-bound legacy READY row carrying any of the other 15 retired values reproduces exactly the failure this PR's tombstone was built for. The generated Solidity blackhole now covers all 17; the contract-side gate covers 2.
Calibrating this honestly, and downward: the other 15 were vacated between 4/13 and 5/20, and the SEC-94 SVM gate landed 7/01, so a residual row would have to have survived unclaimed since well before either — much thinner than the 3001/3002 window, which stayed open until this PR. Redeploy is the recovery pre-launch, so this is not a stuck-state argument.
What I would act on is the coupling. A future retirement now has to remember two places with nothing linking them. Either widen the predicate to a named constexpr array of retired slots, or at minimum put a pointer in the proto's reserved line to the contract predicate so the next retirement finds it.
Nothing else changed since the last round, and the four earlier findings remain fixed.
…-stake-unstake Change-Id: I5a3b357118626f452abe6449cd77aa10af03a73e
Change-Id: I819c3799420430cdb24cc17dcb988f35ed0e70f6
…-stake-unstake Change-Id: I5a092f3111c3f2044ec179ce67b92000801b41bf # Conflicts: # contracts/sysio.reserv/sysio.reserv.wasm # contracts/sysio.uwrit/sysio.uwrit.wasm
Change-Id: If79c1c0ca74d9ae52145860e224ef8d5882c3059
Summary
Companion PRs
Validation
c2c10fa70afd57519ebe6cbe68fce7aa6e646600, containing currentmaster03b610de39715d5e887582b57dcbf00ed06f9cac.363545fb6c8c118079bd35582822c8853b640529, wire-libraries-ts7cce53843e81cc92099bb09ec927458813b4f586, wire-tools-tsa5a1d77a828e5da42f6399bf948094c6c48a31a9, and wire-platform-manifest02eced64a2f539f8f51ef6a773fa57fe369c6f4e.d138b01251d4a2d51494ced31518f09edaac7039; the local-only integration merge was not pushed.Residual risk
The pre-existing runtime-safety/scalability concern around bound tombstone traversal is tracked separately in WIRE-329 and is outside WIRE-301.
Rollout
Draft. This protocol PR must merge before generated model packages are published. No package publication may be dispatched from this feature branch. After merge, publish from the resulting
master, update downstream versions and lockfiles, run clean-install validation, and complete the post-publication shared Release E2E before companion merge or deployment.