serviceability: retire a feed, with the notice its seat holders are owed - #4310
Conversation
D1 gave a feed a way to stop and start. This gives it a way to end. `RetireFeed` moves a feed to a new `Retiring` state and starts a thirty-day notice; `FinalizeFeedRetirement` moves it to `Retired` once that elapses. Retiring is terminal from the moment it starts: a retiring feed neither halts nor resumes, which is the whole difference between retirement and halt. A `Pending` feed retires with no wait. The notice exists for seat holders and a feed that was never `Active` admitted none, so the wait is zero rather than absent, which keeps one path through retirement instead of two. It has to be retirable at all because `DeleteFeed` refuses a staked feed, so refusing here as well would leave a feed that never went live with no way out. Finalizing is permissionless, unlike every other feed instruction. The clock decided when retirement started and this can only agree with it, so requiring an authority would add a way for a feed to sit in `Retiring` forever because whoever held the key stopped caring. A seat holder given a date deserves the date rather than someone's attention. `FeedRetired` is a `msg!` line. This program has no event mechanism and nothing consumes the event yet, since releasing the stake waits for the slashing work, so inventing one for an absent consumer would buy nothing. `Retiring` takes discriminant 4 rather than a value between `Halted` and `Retired`: those are written into live accounts and renumbering them would reinterpret every stored feed.
CI runs nightly rustfmt with `imports_granularity=Crate`; stable `cargo fmt` does not merge import paths, so a second `processors::` path passed locally and failed there.
fa20950 to
f5d21f3
Compare
ben-malbeclabs
left a comment
There was a problem hiding this comment.
Reviewed the program, the SDK decoders and the tests. The instruction wiring is complete in every hand-written match — unpack, instruction_name, the args formatter, entrypoint, From<DoubleZeroError>, From<u32>, and the range comment in the error round-trip test — which is the trap called out on #4307. Discriminants are appended rather than inserted, FeedStatus::Retiring = 4 does not renumber stored bytes, retires_at appends at the struct tail so pre-RFC-28 accounts still decode, and finalize_retirement.rs pins identity with both the owner check and the AccountType discriminant despite being permissionless. I found no substitution, rent or griefing vector in the permissionless path.
Two Critical and eleven Important findings inline. Two of them are product calls rather than defects: whether Retiring is meant to stop reconnections immediately, and whether a builder should be able to retire its own feed.
Evidence for the two test findings, since CI is green and would stay green: three mutations to retires_at survive the whole suite (the Pending branch writing 0 instead of now, RETIREMENT_NOTICE_SECONDS * 1000, and finalize zeroing retires_at while overwriting halted_by), and on the SDK side replacing the decoder read with a constant leaves all 126 Python and 149 TypeScript tests passing. The field this PR adds is unpinned onchain and in both decoders.
Two findings could not be anchored inline because their files are not in this PR:
Important — smartcontract/programs/doublezero-serviceability/src/processors/feed/delete.rs:59. The comment says retirement is a staked feed's path out and that it lands in D2, but this is D2 and delete.rs:60 still refuses on a non-default builder with no status exemption, so a Retired staked feed stays undeletable with its bond locked. That also makes retire.rs:62-64's justification for the Pending path self-defeating: retiring a staked Pending feed leaves it with no way out either.
Important — crates/doublezero-serviceability-instruction/src/feed.rs:160. Neither instruction has a builder, so nothing outside the program's own tests can invoke either one; a repo-wide grep for RetireFeed and FinalizeFeedRetirement hits only entrypoint.rs, instructions.rs, the CHANGELOG and feed_lifecycle_test.rs. Note finalize_retirement's layout is [feed, payer, system], one account shorter than every sibling, so it cannot reuse common::build_with_permission unchanged.
Glossary pass against edge-feed-spec/GLOSSARY.md v1.3.0 is clean: no banned word in any added line, and the one nearby source use is the qualified upstream source.
`require_feed_admits` refused every status but `Active`, so the first instant of a notice cut off the holders the notice exists to protect. It now admits `Retiring`, and `SetAccessPassFeeds` refuses a new seat on a feed that is `Retiring` or `Retired`. Service continues, sales stop. The notice assertions pinned one side of the window only, so a clock change could not fail them. They now pin both. The fixture carries `Retiring` and a negative `retires_at`, which pins the two decoders a compiler cannot reach.
Neither half had a test. Reverting `require_feed_admits` to `Active` only, and dropping the `SetAccessPassFeeds` guard, both left the suite green. One test per side now fails on that mutation: a seat holder subscribes to a retiring feed, and a newcomer is refused a seat on one while the holder keeps theirs.
There was a problem hiding this comment.
🟡 Changes recommended
Moderate issues remain around instruction builders, admission behavior, deletion bypass, and integration coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds the complete feed-retirement lifecycle, including timed notices, finalization, state serialization, SDK updates, and tests.
Changes:
- Adds
RetireFeed,FinalizeFeedRetirement,Retiring, andretires_at. - Updates feed admission, fixtures, decoders, and lifecycle coverage.
- Documents the behavior in the changelog.
File summaries
| File | Summary |
|---|---|
smartcontract/programs/doublezero-serviceability/tests/feed_lifecycle_test.rs |
Adds retirement lifecycle tests. Nits (3 and 1 votes): two tests seed lifecycle states directly instead of exercising real transitions. |
smartcontract/programs/doublezero-serviceability/src/state/feed.rs |
Adds retirement state and legacy decoding. Nits (1 vote each): clarify the retained timestamp and assert the legacy zero default. |
smartcontract/programs/doublezero-serviceability/src/processors/feed/retire.rs |
Starts retirement notices. Moderate issues (2 and 1 votes): deletion can bypass the notice, and Pending-to-Retiring admission can allow subscribers. |
smartcontract/programs/doublezero-serviceability/src/processors/feed/mod.rs |
Updates admission rules. Moderate (1 vote): all Retiring feeds, including Pending-origin feeds, can pass the gate. |
smartcontract/programs/doublezero-serviceability/src/processors/feed/finalize_retirement.rs |
Finalizes elapsed retirements permissionlessly. |
smartcontract/programs/doublezero-serviceability/src/processors/feed/create.rs |
Initializes the retirement timestamp. |
smartcontract/programs/doublezero-serviceability/src/processors/accesspass/set_feeds.rs |
Blocks new seats on retiring feeds. Moderate issues (1 and 3 votes): misleading error semantics and missing integration coverage. |
smartcontract/programs/doublezero-serviceability/src/instructions.rs |
Adds retirement instruction variants. Moderate (2 votes): supported Rust instruction builders and round-trip tests are missing. |
smartcontract/programs/doublezero-serviceability/src/error.rs |
Adds retirement-related errors. |
smartcontract/programs/doublezero-serviceability/src/entrypoint.rs |
Dispatches retirement instructions. |
sdk/serviceability/typescript/serviceability/tests/fixtures.test.ts |
Updates TypeScript fixture assertions. |
sdk/serviceability/typescript/serviceability/state.ts |
Decodes retirement fields. Nit (1 vote): clarify timestamp semantics for Retired feeds. |
sdk/serviceability/testdata/fixtures/generate-fixtures/src/main.rs |
Generates updated feed fixtures. |
sdk/serviceability/testdata/fixtures/feed.json |
Updates fixture metadata. |
sdk/serviceability/python/serviceability/tests/test_fixtures.py |
Updates Python fixture assertions. |
sdk/serviceability/python/serviceability/state.py |
Decodes retirement fields. Nit (1 vote): clarify timestamp semantics for Retired feeds. |
CHANGELOG.md |
Documents feed retirement behavior. |
Review details
Suppressed comments (8)
sdk/serviceability/python/serviceability/state.py:1285
- Issue
The public field comment saysretires_atis zero when the feed is not retiring, but finalization leaves the timestamp onRetiredfeeds. This gives Python consumers the wrong sentinel semantics.
Context
After finalization, callers see status 3 and a nonzero retires_at.
Proposed Fix
Document zero as the value before retirement starts and state that finalization preserves the timestamp.
# When the retirement notice elapses, zero when the feed is not retiring. Appended after
# halted_by, so a feed written before it reads as not retiring, which is right.
sdk/serviceability/typescript/serviceability/state.ts:1271
- Issue
The public field comment saysretiresAtis zero when the feed is not retiring, but finalization leaves the timestamp onRetiredfeeds. This gives TypeScript consumers the wrong sentinel semantics.
Context
After finalization, callers see status 3 and a nonzero retiresAt.
Proposed Fix
Document zero as the value before retirement starts and state that finalization preserves the timestamp.
// When the retirement notice elapses, zero when the feed is not retiring. Appended after
// haltedBy, so a feed written before it reads as not retiring, which is right.
smartcontract/programs/doublezero-serviceability/src/processors/accesspass/set_feeds.rs:163
- Issue
This new rejection returnsFeedNotActive, whose message says the feed is not publishing, but a Retiring feed is explicitly still publishing for existing holders.
Context
An access-pass administrator who tries to add a new seat during the notice receives an explanation that contradicts the lifecycle and does not say that provisioning is blocked.
Proposed Fix
Return a dedicated error such as FeedNotProvisionable with a message that says a retiring or retired feed takes no new seats, and map and test that error.
return Err(DoubleZeroError::FeedNotActive.into());
smartcontract/programs/doublezero-serviceability/src/processors/feed/mod.rs:127
- Issue
This condition admits everyRetiringfeed, including a feed that was stillPendingwhen retirement began.
Context
SetAccessPassFeeds currently permits a Pending feed to be placed on a pass. If the builder retires it, retire.rs records retires_at = now and changes the status to Retiring; a user holding that pre-provisioned seat can then call SubscribeFeed and pass this gate even though the feed never published and its notice is zero.
Proposed Fix
Require a Retiring feed to have an unelapsed notice, or persist whether the feed reached Active before admitting it, and cover the Pending-to-Retiring subscription path.
if !matches!(feed.status, FeedStatus::Active | FeedStatus::Retiring) {
smartcontract/programs/doublezero-serviceability/src/processors/feed/retire.rs:85
- Issue
This transition changes aPendingfeed toRetiring, and the new admission gate treats everyRetiringfeed as publishing. That lets a never-published feed accept subscribers even though this path sets its notice tonow.
Context
SetAccessPassFeeds permits pre-selling a seat on a Pending feed, and CreateSubscribeUser later calls enforce_feed_metro_gate; after this transition it can consume that seat and connect.
Proposed Fix
Preserve the pre-publication origin and reject admission for the Pending to Retiring path, then add an integration test for a pre-sold pending feed.
feed.status = FeedStatus::Retiring;
smartcontract/programs/doublezero-serviceability/src/state/feed.rs:188
- Issue
The legacy-feed test does not assert the newretires_atdefault, so a broken short-account fallback could pass while old feeds acquire a false retirement deadline.
Context
A pre-RFC-28 account reaches this fallback with no retirement bytes, but test_pre_rfc28_feed_decodes_active_with_defaults checks only the older fields.
Proposed Fix
Assert feed.retires_at == 0 in the legacy migration test or compare the complete expected feed.
retires_at: BorshDeserialize::deserialize(&mut data).unwrap_or_default(),
smartcontract/programs/doublezero-serviceability/src/state/feed.rs:122
- Issue
The field comment saysretires_atis zero when the feed is notRetiring, but finalization intentionally keeps the timestamp onRetiredfeeds, as the end-to-end test asserts. This gives consumers the wrong sentinel semantics.
Context
After FinalizeFeedRetirement, callers see status == Retired and a nonzero retires_at.
Proposed Fix
Document zero as the value before retirement starts and state that finalization preserves the timestamp.
/// When this feed's retirement notice elapses, as a unix timestamp, zero when it is not
/// retiring.
smartcontract/programs/doublezero-serviceability/tests/feed_lifecycle_test.rs:901
- Issue
This test also seeds a halted feed and stake mirror directly instead of reachingHaltedthrough the real lifecycle instructions.
Context
The new Halted-to-Retiring success path will not detect a regression in the preceding halt transition or in the account shape that real instructions produce; it only proves that the retirement handler accepts hand-written bytes.
Proposed Fix
Use a catalog feed with a real HaltFeed call before RetireFeed, or add the missing activation path first and then drive a staked feed through both transitions.
/// Halted to Retiring, the one supported transition that nothing else covers, and it must keep
/// the record of who halted.
#[tokio::test]
async fn test_a_halted_feed_retires_and_keeps_its_halter() {
let builder = test_payer();
let operator = Pubkey::new_unique();
let (mut banks_client, program_id, payer, globalstate, feed, _mirror) =
cluster_with_halted_feed("halted2retiring", builder.pubkey(), operator).await;
- Files reviewed: 19/20 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`DeleteFeed` closed a retiring catalog feed, which ended the notice the same minute it started. It now refuses one until the retirement is finalized, with `RetiringFeedCannotBeDeleted` (129). A retired feed still deletes, or a finished retirement would strand the account. The instruction crate stopped at `resume_feed`, so a caller could not build either retirement transaction without hand-encoding it. `finalize_feed_retirement` takes `[feed]` and no globalstate, because it is permissionless and there is no authority to check against.
Implements D2. Stacked on #4307, which gave a feed a way to stop and start; this gives it a way to end.
The lifecycle it completes
RetireFeedFinalizeFeedRetirementPendingRetiring, no waitActiveRetiring, thirty daysHaltedRetiring, thirty daysRetiringRetired, once the notice elapsesRetiredA retiring feed neither halts nor resumes. That is the whole difference between retirement and halt, and it has a test.
Four decisions worth arguing with
A
Pendingfeed retires with no wait. The notice exists for seat holders, and a feed that was neverActiveadmitted none:require_feed_admitsrefuses subscribers unless the feed is publishing. So the wait is zero rather than absent, which keeps one path through retirement instead of two branches. It has to be retirable at all becauseDeleteFeedrefuses a staked feed, so refusing here as well would leave a feed that never went live with no way out — the same shape of hole as the one found in halt on #4307.Finalizing is permissionless, unlike every other feed instruction. The clock decided when retirement started, and this instruction can only agree with it. Requiring an authority adds no safety and one failure mode: a feed sitting in
Retiringforever because whoever held the key stopped caring. A seat holder given a date deserves the date rather than someone's attention.FeedRetiredis amsg!line, not a new event mechanism. This program has none: nosol_log_data, no return data. Nothing consumes the event yet either, because releasing the stake on retirement waits for the slashing work. That is the same reasoning the plan already applied toStakeLockedandStakeReleasedundersolana/: inventing a mechanism for a single absent consumer buys nothing. The line's shape is fixed so a future consumer can parse it without it changing.Retiringtakes discriminant 4, not a value betweenHaltedandRetired. Those are written into live accounts, and renumbering them would reinterpret every stored feed rather than adding a state.Testing Verification
Fifteen tests in
feed_lifecycle_test.rs, six of them new:Pendingfeed's notice ends at or before now.Retiring.The elapsed and not-yet cases seed
retires_atin the past and the future rather than moving the validator's clock. The comparison againstClockis what those two tests are about; how the timestamp gets there isretire's business and has its own test.The full serviceability suite passes, plus the SDK blast radius a
Feedfield change now reaches: the fixture generator,feed.bin, and the Python and TypeScript decoders, which also gainFEED_STATUS_RETIRING. 126 Python tests and 149 TypeScript pass against the regenerated fixture, andmake check-fixturesis clean.TypeScript reads
retiresAtasBigInt.asIntN(64, r.readU64()), matching how the seat timestamps in the same file handle a signed 64-bit value; there is noreadI64.