serviceability: split access pass removal into one instruction per pass type - #4267
serviceability: split access pass removal into one instruction per pass type#4267bgm-malbeclabs wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
It changes onchain instruction semantics and introduces a deploy-time breaking change that needs coordinated updates across external callers.
Pull request overview
This pull request hardens the DoubleZero serviceability onchain program by preventing a caller from closing or deleting state for the wrong access pass type. It introduces per-pass-type removal instructions, deprecates the old generic variants while keeping their wire discriminants decodable, and adds program tests that validate the new mismatch guard.
Changes:
- Add
AccessPassKindandDoubleZeroError::AccessPassTypeMismatchto enforce that close and delete instructions match the storedAccessPassType. - Introduce 10 new instruction variants (5 close, 5 delete) and dispatch them in
entrypoint.rs, while making variants 42 and 69 payload-free and deprecated. - Update processors and tests to use the new variants, plus add new integration suites and a design spec doc.
File summaries
| File | Description |
|---|---|
| smartcontract/programs/doublezero-serviceability/src/state/accesspass.rs | Add AccessPassKind and conversions for variant-only matching. |
| smartcontract/programs/doublezero-serviceability/src/error.rs | Add AccessPassTypeMismatch error and wire-code mapping. |
| smartcontract/programs/doublezero-serviceability/src/instructions.rs | Add new close/delete variants; make 42 and 69 payload-free deprecated; update decode/name/args tests. |
| smartcontract/programs/doublezero-serviceability/src/entrypoint.rs | Route new variants to shared handlers with an expected kind; return Deprecated for 42 and 69. |
| smartcontract/programs/doublezero-serviceability/src/processors/accesspass/close.rs | Enforce kind match and fix the prior try_borrow_data fall-through close behavior. |
| smartcontract/programs/doublezero-serviceability/src/processors/user/delete.rs | Enforce kind match before deeper access pass validation and state mutation. |
| smartcontract/programs/doublezero-serviceability/src/processors/multicastgroup/subscribe.rs | Update comment text to reflect new Delete<Kind>User instruction naming. |
| smartcontract/programs/doublezero-serviceability/tests/accesspass_test.rs | Migrate close tests to the correct per-kind close variants. |
| smartcontract/programs/doublezero-serviceability/tests/accesspass_allow_multiple_ip.rs | Migrate user delete call to DeletePrepaidUser. |
| smartcontract/programs/doublezero-serviceability/tests/user_tests.rs | Migrate delete calls to DeletePrepaidUser. |
| smartcontract/programs/doublezero-serviceability/tests/user_old_test.rs | Migrate delete call to DeletePrepaidUser. |
| smartcontract/programs/doublezero-serviceability/tests/user_onchain_allocation_test.rs | Migrate delete calls to DeletePrepaidUser. |
| smartcontract/programs/doublezero-serviceability/tests/delete_user_dynamic_accesspass.rs | Migrate delete calls to DeletePrepaidUser. |
| smartcontract/programs/doublezero-serviceability/tests/create_subscribe_user_test.rs | Migrate delete call to DeletePrepaidUser. |
| smartcontract/programs/doublezero-serviceability/tests/test_helpers.rs | Add a shared helper to assert ProgramError::Custom matches a DoubleZeroError. |
| smartcontract/programs/doublezero-serviceability/tests/deprecated_removal_instructions_test.rs | Add coverage that variants 42 and 69 return Deprecated. |
| smartcontract/programs/doublezero-serviceability/tests/close_access_pass_kind_test.rs | Add integration coverage for close mismatch guard across all pass kinds. |
| smartcontract/programs/doublezero-serviceability/tests/delete_user_kind_test.rs | Add integration coverage for delete mismatch guard across all pass kinds, including an EdgeSeat seat-release case. |
| crates/doublezero-serviceability-instruction/src/accesspass.rs | Keep the deprecated close_access_pass builder compiling by emitting payload-free variant 69. |
| crates/doublezero-serviceability-instruction/src/user.rs | Keep the deprecated delete_user builder compiling by emitting payload-free variant 42 and updating the builder test. |
| docs/superpowers/specs/2026-09-01-per-pass-type-removal-guard-design.md | Add a design spec documenting the per-kind instruction strategy and rollout constraints. |
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if !accesspass_account.data_is_empty() { | ||
| // Read Access Pass |
There was a problem hiding this comment.
Fixed in cbd95b6. The guard is dead as you say, and it matters more than tidiness here: the new access pass kind check sits inside that block, so a reader could not see it was unconditional without proving the earlier AccessPassNotFound return 45 lines above, and a future edit to that return could silently make the kind check skippable. Removed the wrapper and unindented the body; every statement and its order are unchanged. The guard predates this branch (it is on main too), so this is a cleanup of code the change already touches.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b164d63. Configure here.
| executable: false, | ||
| rent_epoch: 0, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Close tests skip real pass creation
Low Severity
Issue
The close-kind suite writes a raw access pass into the test bank and funds the payer by hand. It never runs the set-pass instruction, so the kind check never runs against a pass the program created.
Context
An operator closes a pass that SetAccessPass wrote. This suite instead serializes AccessPass bytes, inserts that account before start(), and funds test_payer() so it can sign. The delete-kind suite in this same change already creates the pass through SetAccessPass after InitGlobalState.
Proposed Fix
Create each pass with SetAccessPass after InitGlobalState, and drop the hand-written account and the hand-funded payer.
Triggered by project rule: Comment shape
Reviewed by Cursor Bugbot for commit b164d63. Configure here.
There was a problem hiding this comment.
Fixed in cbd95b6. The suite now runs InitGlobalState and creates each pass with SetAccessPass, matching the delete-kind suite, and the hand-written account and hand-funded payer are gone. SetAccessPass covers all five kinds, with AccessPassType::EdgeSeat(vec![]) for the EdgeSeat case, which is all the close path needs since closing does not read feed seats. The assertions are unchanged: per kind, a close for a different kind is refused with AccessPassTypeMismatch and the pass survives, then the matching close removes it.
| .set_account(&user_pubkey, &AccountSharedData::from(user_account)); | ||
|
|
||
| feed_key | ||
| } |
There was a problem hiding this comment.
Delete test rewrites pass by hand
Low Severity
Issue
The EdgeSeat delete case writes pass and user bytes after create. It never runs subscribe, so the seat-release path never sees a seat the program granted.
Context
A matching DeleteEdgeSeatUser must release a feed seat that subscribe recorded. seed_feed_seat overwrites the pass type and the user's feed list in the bank after SetAccessPass and CreateUser. A feedless EdgeSeat pass makes seat release a no-op, which is why this case exists.
Proposed Fix
Provision the seat with SetAccessPassFeeds and SubscribeFeed after the user exists.
Additional Locations (1)
Triggered by project rule: Comment shape
Reviewed by Cursor Bugbot for commit b164d63. Configure here.
There was a problem hiding this comment.
Half right, and the half you found is the more useful one: the comment was false. It claimed seeding by hand was "the only way" to put a real seat in front of release_feed_seats, and it is not — SetAccessPassFeeds is the real path and three suites already use it. That is corrected in cbd95b6.
I kept the direct seeding, for a reason worth stating. SetAccessPassFeeds puts a seat on the pass but never ticks current_users and never touches a user. Recording the feed on a user, ticked, is done by CreateSubscribeUser or SubscribeFeed, and both need a real MulticastGroup with its own create instruction and ResourceExtension accounts, which this suite does not otherwise stand up. Standing that up to seed one seat would roughly double the file without changing what the delete path is tested against: the assertion is still full-struct equality showing the seat's current_users going 1 to 0 after DeleteEdgeSeatUser, so the release path is exercised either way.
The comment now names SetAccessPassFeeds, points at set_access_pass_feeds_test.rs, and says why this suite seeds directly, so the next reader can weigh it rather than take my word for it.
1d689b9 to
7cd6c0c
Compare
Remove a stale #[repr(u8)] from the AccessPassKind design-doc snippet (the enum never has one, deliberately). Delete a dead data_is_empty() wrapper in process_delete_user that made the new AccessPassTypeMismatch check look conditional on an early return 45 lines above. Rewrite close_access_pass_kind_test.rs to build its passes with SetAccessPass instead of hand-inserting AccessPass accounts, matching its delete_user_kind_test.rs sibling. Correct delete_user_kind_test.rs's seed_feed_seat comment, which claimed direct seeding was the only way to put a real seat on a user: SetAccessPassFeeds is the real provisioning path, but ticking a user's seat for real needs CreateSubscribeUser/SubscribeFeed and a live MulticastGroup, which this suite does not otherwise stand up.
cbd95b6 to
af05027
Compare


Part of malbeclabs/infra#2470. Child of malbeclabs/infra#2385.
Serviceability has one
CloseAccessPass(variant 69) and oneDeleteUser(variant 42). Neither looks atAccessPassType. Both destroy state, so a caller meaning to remove a prepaid pass can remove an EdgeSeat pass and the program accepts it.This adds one instruction per pass type. Each loads the pass and refuses unless the stored pass matches the instruction.
This PR is purely additive.
CloseAccessPassandDeleteUserkeep working exactly as they do today, so this is safe to merge and safe to deploy on its own. The follow-up, #4268, deprecates them and moves every caller in one commit range.Summary of Changes
ClosePrepaidAccessPass,CloseSolanaValidatorAccessPass,CloseSolanaRPCAccessPass,CloseOthersAccessPass,CloseEdgeSeatAccessPass. 124-128 delete a user:DeletePrepaidUser,DeleteSolanaValidatorUser,DeleteSolanaRPCUser,DeleteOthersUser,DeleteEdgeSeatUser.AccessPassKind, a payload-free tag forAccessPassType. It has no Borsh derive on purpose: issue telemetry: add pcap input/stdout support to flow-enricher #2470 requires that the pass type is never an instruction argument, so the kind is a Rust parameter the dispatch arm supplies and it never reaches the wire.AccessPassTypeMismatch(119), returned when the instruction and the stored pass disagree.process_close_access_passandprocess_delete_usereach gained oneexpected: AccessPassKindparameter. Ten copies of the 250-line delete body would be ten places to fix its next bug.process_close_access_passandprocess_delete_usertake anOption<AccessPassKind>.Noneis the old general-purpose path and performs no kind check, which is how 69 and 42 keep their current behaviour. The follow-up removes theOption.close.rswrapped its account-type check and itsconnection_countcheck inif let Ok(data) = accesspass_account.try_borrow_data(), with anelsethat logged a warning and then closed the pass anyway. A failed borrow skipped both checks. The read is now hoisted and a failed borrow returns an error.Not breaking, on purpose
Nothing changes for existing callers in this PR. That is deliberate: an earlier version of this split deprecated 69 and 42 here, and CI showed the CLI on this very branch then failed with
Deprecatedondoublezero user delete. Deploying that alone would have broken user deletion until the follow-up landed.The deprecation, and the warning that the oracle in
doublezero-shredsmust ship with it, live in #4268.Diff Breakdown
Mostly tests and instruction registration. The behaviour change is about 100 lines across three files.
Key files (click to expand)
smartcontract/programs/doublezero-serviceability/src/state/accesspass.rs— theAccessPassKindtag, itsFrom<&AccessPassType>andDisplay. One arm per pass type in one place, so a future pass type costs one line.smartcontract/programs/doublezero-serviceability/src/instructions.rs— the ten variants, each registered in the enum, the decoder,get_name, the args formatter and the round-trip test.smartcontract/programs/doublezero-serviceability/src/entrypoint.rs— ten dispatch arms, each naming its kind. 69 and 42 join the existing deprecated arm.smartcontract/programs/doublezero-serviceability/src/processors/accesspass/close.rs— the kind check, plus thetry_borrow_datafall-through fix.smartcontract/programs/doublezero-serviceability/src/processors/user/delete.rs— the kind check, placed before the ownership checks so a wrong-kind call fails for the reason it actually failed.crates/doublezero-serviceability-instruction/src/{accesspass,user}.rs— the two builders keep compiling against the now payload-free 69 and 42. The follow-up PR replaces both functions.Testing Verification
close_access_pass_kind_test.rsanddelete_user_kind_test.rs, each covering all five kinds: the matching instruction removes the account, and an instruction for any other kind is refused withAccessPassTypeMismatchwhile the account survives. Assertions are on the specific error code, notis_err().FeedSeatand a matchingfeed_pksentry on the user, then asserts the seat was released after the delete. A feedless EdgeSeat pass makesrelease_feed_seatsa silent no-op, so without the seat the test would prove nothing.deprecated_removal_instructions_test.rsasserts variants 69 and 42 both returnDeprecated, so the wire discriminants stay decodable rather than failing to parse.DeleteUserandCloseAccessPasscall site in the program's tests was traced back to the pass type its own setup creates before being moved to the matching variant.cd smartcontract && make test-programs: 78 binaries, zero failures.make rust-lintclean.