serviceability-instruction: scaffold pure instruction-builder crate (RFC-26 R0) - #4049
Conversation
…RFC-26 R0) Add the `doublezero-serviceability-instruction` crate: pure, RPC-free builders that return a single unsigned `Instruction` per serviceability instruction (SPL-style), with the trailing-account convention centralized in `common`. - `common::build` (no-permission path) + `common::build_with_permission` (deferred, activate-in-one-place Permission append) + `compute_budget_prelude`. - Four exemplar builders: create_device, delete_device (legacy/atomic), create_link, create_subscribe_user. All route through build_with_permission. - Correct the RFC's length-detected family: only CreateUser is length-detected; DeleteUser (state-detected) and CreateSubscribeUser (split_trailing_permission) both route through authorize(). Refs #4015, RFC-26.
nikw9944
left a comment
There was a problem hiding this comment.
RFC-26 R0 scaffold is fundamentally sound: all four exemplar builders were verified line-by-line against their processors and the existing SDK commands — account ordering, writability, PDA derivations, tag bytes (20/26/28/59), and args write-back all match exactly, and tests/clippy/fmt pass. However, the crate's anti-drift value proposition has holes that should be fixed before merge: (1) delete_device's doc falsely claims it never appends a Permission account while it routes through build_with_permission (and process_delete_device does call authorize()), inviting a future rollout regression; (2) R0 ships without the RFC-promised golden fixtures / solana-program-test coverage, and the unit tests are self-referential (they recompute the expected accounts with the same helpers and ordering as the builders, so they can't catch processor drift); (3) the RFC is left internally contradictory about the Permission activation mechanism, and build_with_permission's activation recipe overstates safety — authorize() hard-fails on a non-existent Permission PDA before any legacy fallback. Plus: release-mode unwrap_or(u8::MAX) can silently break the count==accounts invariant, and the system program is marked writable while the RFC glossary says readonly (fixtures will freeze whichever ships). All fixes are small — doc corrections, an RFC section, an assert swap, and one writability decision.
- [Architecture — Medium] RFC left internally contradictory about Permission activation: rfcs/rfc26-rust-instruction-builder-library.md:111 (unchanged by this PR) still says the Permission append ships "commented out in
common.rs… enabled per-builder by the activating PR," while the implementation and the PR-updated risk section describe a separatebuild_with_permissionenabled centrally, activating every assigned builder at once. The two designs have different blast radii — update the RFC's "Permission account" section andcommon.rssketch to thebuild/build_with_permissionsplit and state which activation model applies. - [Architecture — Low] Dead code until R1/RF: nothing consumes the crate yet; only its self-referential unit tests exercise it in CI. Acceptable per the rollout plan, but worth noting in the PR description; prioritize the first consumer.
- correct delete_device doc: routes through authorize()/build_with_permission - document build_with_permission activation precondition (authorize() hard-fails on a non-existent Permission PDA before any legacy fallback) - swap unwrap_or(u8::MAX) for try_from().expect() to keep count==accounts honest - rename create_device account_index -> device_index (parity with link_index) - feed: Option<Pubkey> -> Option<&Pubkey>; add dz_prefix_count debug_assert - document system_program writable as deliberate byte-parity; align RFC glossary - share UNICAST_DEFAULT_TOPOLOGY_NAME const from doublezero-serviceability - fix rustdoc private_intra_doc_links / Transaction warnings - rewrite RFC Permission section to the build/build_with_permission split
nikw9944
left a comment
There was a problem hiding this comment.
Round-2 verification at head d55a4e5: all 13 findings from the posted review are resolved — 11 fixed and 2 resolved by documented decision (system-program and globalconfig/globalstate writability kept for byte-parity with the SDK, with the RFC glossary aligned) — and every fix was verified in code. The RFC now scopes golden fixtures and solana-program-test coverage to a follow-up PR, resolving the plan/PR mismatch; note drift protection stays deferred until that follow-up lands. Re-verified on the new head: 7 crate tests + 292 serviceability program tests pass, clippy/fmt clean on both touched crates, rustdoc warnings now zero. Remaining open items are three optional nits from the dz-ledger-program-review supplement plus one optional residual; nothing blocking.
- [Low, informational] Changelog skipped via label: the
skip-changeloglabel passes the CI gate, and the CHANGELOG delta on the branch came in via the merge of main (#4027), not this PR. Defensible for a scaffold nothing consumes yet; the RFC-26 series should land a changelog entry no later than the first consuming or publishing PR. - [Residual, non-blocking] Drift protection deferred: the RFC now honestly scopes golden fixtures and
solana-program-testcoverage to a follow-up PR, but until it lands the unit tests remain self-referential and cannot catch builder-vs-processor account-order drift. Please prioritize that follow-up. - [Optional residual on finding 15]: the
dz_prefix_countdebug_assert was added increate_subscribe_user; the analogous caller-setresource_countincreate_deviceis still silently overwritten without an assert. Optional, fine to leave.
efd6dbf to
34562c2
Compare
## Summary RFC-26 **R1** (stacked on the previous phase's branch). Complete the device domain on top of the R0 exemplars: update_device (max(old,new) dz_prefix block), set_device_health, and interface create/delete/update (Vpnv4 topology PDAs, segment-routing reconcile). All builders route through `authorize()` -> `build_with_permission` unless noted; each carries a verbatim account-layout doc-comment copied from its processor. ## Testing Verification - Unit tests assert the exact `AccountMeta` list (incl. trailing `[payer, system]`) and the borsh tag byte for every builder, covering the conditional/variable-account paths. Closes #4016. Part of RFC-26 ([rfcs/rfc26-rust-instruction-builder-library.md](rfcs/rfc26-rust-instruction-builder-library.md)). --- ### PR stack (RFC-26 builder library) Stacked PRs, merge in order (each is based on the previous one's branch): 1. #4049 — R0 scaffold + exemplars 2. #4050 — R1 device ← **this PR** 3. #4051 — R2 link 4. #4052 — R3 user 5. #4053 — R4 location/exchange/contributor 6. #4054 — R5 multicastgroup + allowlists 7. #4055 — R6 tenant + permission 8. #4056 — R7 topology + feed 9. #4057 — R8 accesspass + resource 10. #4058 — R9 globalstate/config/allowlist/index/migrate R10 (commands/* migration + program-test) and RF (fixtures) follow as separate PRs.
Post-approval follow-ups to the RFC-26 R0 scaffold (#4049), split into their own PR so #4049 stays at the state Nik approved. Stacked on #4049 — GitHub will retarget this to `main` once #4049 merges. ## Summary - Add the RFC-26 R0 entry to `CHANGELOG.md` (the scaffold PR had skipped it via label). - `delete_device`: spell out `DeviceDeleteArgs { resource_count: 0 }` instead of `::default()`, so a future field addition becomes a compile error that forces the builder to take a position. - `create_device`: guard the caller-set `resource_count` write-back with a `debug_assert` (parity with `create_subscribe_user`'s `dz_prefix_count`). - Builder tests: assert the full decoded args struct (clone input + set expected count) instead of only the count field, pinning that the `mut args` write-back touches only that field and auto-covering future fields. These address the non-blocking items and post-approval inline nits from the review on #4049. RFC: `rfcs/rfc26-rust-instruction-builder-library.md` ## Testing Verification - 7 crate unit tests pass, including the strengthened full-args-struct assertions for `create_device`, `delete_device` (atomic), and `create_subscribe_user`.
## Summary RFC-26 **R2** (stacked on the previous phase's branch). accept_link, update_link (LinkUpdateAuthority preamble + conditional tunnel_net / tunnel-resource / topology-union sections), delete_link (topology reference-count accounts), set_link_health. All builders route through `authorize()` -> `build_with_permission` unless noted; each carries a verbatim account-layout doc-comment copied from its processor. ## Testing Verification - Unit tests assert the exact `AccountMeta` list (incl. trailing `[payer, system]`) and the borsh tag byte for every builder, covering the conditional/variable-account paths. Closes #4017. Part of RFC-26 ([rfcs/rfc26-rust-instruction-builder-library.md](rfcs/rfc26-rust-instruction-builder-library.md)). --- ### PR stack (RFC-26 builder library) Stacked PRs, merge in order (each is based on the previous one's branch): 1. #4049 — R0 scaffold + exemplars 2. #4050 — R1 device 3. #4051 — R2 link ← **this PR** 4. #4052 — R3 user 5. #4053 — R4 location/exchange/contributor 6. #4054 — R5 multicastgroup + allowlists 7. #4055 — R6 tenant + permission 8. #4056 — R7 topology + feed 9. #4057 — R8 accesspass + resource 10. #4058 — R9 globalstate/config/allowlist/index/migrate R10 (commands/* migration + program-test) and RF (fixtures) follow as separate PRs.
## Summary RFC-26 **R3** (stacked on the previous phase's branch). create_user (length-detected -> build, no permission), update_user, delete_user, request_ban_user, check_user_access_pass, and set_user_bgp_status (metrics-publisher check, no authorize -> build). All builders route through `authorize()` -> `build_with_permission` unless noted; each carries a verbatim account-layout doc-comment copied from its processor. ## Testing Verification - Unit tests assert the exact `AccountMeta` list (incl. trailing `[payer, system]`) and the borsh tag byte for every builder, covering the conditional/variable-account paths. Closes #4018. Part of RFC-26 ([rfcs/rfc26-rust-instruction-builder-library.md](rfcs/rfc26-rust-instruction-builder-library.md)). --- ### PR stack (RFC-26 builder library) Stacked PRs, merge in order (each is based on the previous one's branch): 1. #4049 — R0 scaffold + exemplars 2. #4050 — R1 device 3. #4051 — R2 link 4. #4052 — R3 user ← **this PR** 5. #4053 — R4 location/exchange/contributor 6. #4054 — R5 multicastgroup + allowlists 7. #4055 — R6 tenant + permission 8. #4056 — R7 topology + feed 9. #4057 — R8 accesspass + resource 10. #4058 — R9 globalstate/config/allowlist/index/migrate R10 (commands/* migration + program-test) and RF (fixtures) follow as separate PRs.
…FC-26 R4) (#4053) ## Summary RFC-26 **R4** (stacked on the previous phase's branch). Three account_index-seeded CRUD domains (create/update/suspend/resume/delete). Exchange create/update carry globalconfig; set_device_exchange carries the device; create_contributor carries the owner. All builders route through `authorize()` -> `build_with_permission` unless noted; each carries a verbatim account-layout doc-comment copied from its processor. ## Testing Verification - Unit tests assert the exact `AccountMeta` list (incl. trailing `[payer, system]`) and the borsh tag byte for every builder, covering the conditional/variable-account paths. Closes #4019. Part of RFC-26 ([rfcs/rfc26-rust-instruction-builder-library.md](rfcs/rfc26-rust-instruction-builder-library.md)). --- ### PR stack (RFC-26 builder library) Stacked PRs, merge in order (each is based on the previous one's branch): 1. #4049 — R0 scaffold + exemplars 2. #4050 — R1 device 3. #4051 — R2 link 4. #4052 — R3 user 5. #4053 — R4 location/exchange/contributor ← **this PR** 6. #4054 — R5 multicastgroup + allowlists 7. #4055 — R6 tenant + permission 8. #4056 — R7 topology + feed 9. #4057 — R8 accesspass + resource 10. #4058 — R9 globalstate/config/allowlist/index/migrate R10 (commands/* migration + program-test) and RF (fixtures) follow as separate PRs.
…26 R5) (#4054) ## Summary RFC-26 **R5** (stacked on the previous phase's branch). create/update (conditional multicast_group_block)/suspend/reactivate/delete, update_multicast_group_roles, and the four pub/sub allowlist add/remove builders. All builders route through `authorize()` -> `build_with_permission` unless noted; each carries a verbatim account-layout doc-comment copied from its processor. ## Testing Verification - Unit tests assert the exact `AccountMeta` list (incl. trailing `[payer, system]`) and the borsh tag byte for every builder, covering the conditional/variable-account paths. Closes #4020. Part of RFC-26 ([rfcs/rfc26-rust-instruction-builder-library.md](rfcs/rfc26-rust-instruction-builder-library.md)). --- ### PR stack (RFC-26 builder library) Stacked PRs, merge in order (each is based on the previous one's branch): 1. #4049 — R0 scaffold + exemplars 2. #4050 — R1 device 3. #4051 — R2 link 4. #4052 — R3 user 5. #4053 — R4 location/exchange/contributor 6. #4054 — R5 multicastgroup + allowlists ← **this PR** 7. #4055 — R6 tenant + permission 8. #4056 — R7 topology + feed 9. #4057 — R8 accesspass + resource 10. #4058 — R9 globalstate/config/allowlist/index/migrate R10 (commands/* migration + program-test) and RF (fixtures) follow as separate PRs.
…4055) ## Summary RFC-26 **R6** (stacked on the previous phase's branch). Tenant CRUD + add/remove administrator + update_payment_status (globalstate writable on create, read-only elsewhere), and permission create/update/suspend/resume/delete (target PDA derived from args.user_payer). All builders route through `authorize()` -> `build_with_permission` unless noted; each carries a verbatim account-layout doc-comment copied from its processor. ## Testing Verification - Unit tests assert the exact `AccountMeta` list (incl. trailing `[payer, system]`) and the borsh tag byte for every builder, covering the conditional/variable-account paths. Closes #4021. Part of RFC-26 ([rfcs/rfc26-rust-instruction-builder-library.md](rfcs/rfc26-rust-instruction-builder-library.md)). --- ### PR stack (RFC-26 builder library) Stacked PRs, merge in order (each is based on the previous one's branch): 1. #4049 — R0 scaffold + exemplars 2. #4050 — R1 device 3. #4051 — R2 link 4. #4052 — R3 user 5. #4053 — R4 location/exchange/contributor 6. #4054 — R5 multicastgroup + allowlists 7. #4055 — R6 tenant + permission ← **this PR** 8. #4056 — R7 topology + feed 9. #4057 — R8 accesspass + resource 10. #4058 — R9 globalstate/config/allowlist/index/migrate R10 (commands/* migration + program-test) and RF (fixtures) follow as separate PRs.
## Summary RFC-26 **R7** (stacked on the previous phase's branch). Topology create/delete plus batched clear_topology / assign_topology_node_segments (single-chunk + *_batched; CLEAR_BATCH_SIZE=16 / BACKFILL_BATCH_SIZE=4 moved into the crate). Feed create/update/delete. All builders route through `authorize()` -> `build_with_permission` unless noted; each carries a verbatim account-layout doc-comment copied from its processor. ## Testing Verification - Unit tests assert the exact `AccountMeta` list (incl. trailing `[payer, system]`) and the borsh tag byte for every builder, covering the conditional/variable-account paths. Closes #4022. Part of RFC-26 ([rfcs/rfc26-rust-instruction-builder-library.md](rfcs/rfc26-rust-instruction-builder-library.md)). --- ### PR stack (RFC-26 builder library) Stacked PRs, merge in order (each is based on the previous one's branch): 1. #4049 — R0 scaffold + exemplars 2. #4050 — R1 device 3. #4051 — R2 link 4. #4052 — R3 user 5. #4053 — R4 location/exchange/contributor 6. #4054 — R5 multicastgroup + allowlists 7. #4055 — R6 tenant + permission 8. #4056 — R7 topology + feed ← **this PR** 9. #4057 — R8 accesspass + resource 10. #4058 — R9 globalstate/config/allowlist/index/migrate R10 (commands/* migration + program-test) and RF (fixtures) follow as separate PRs.
#4057) ## Summary RFC-26 **R8** (stacked on the previous phase's branch). Access-pass set (conditional tenant pair)/close/check-status/set-feeds, and resource-extension allocate/create/deallocate/close (resource PDA + associated account derived from the data-bearing args.resource_type). All builders route through `authorize()` -> `build_with_permission` unless noted; each carries a verbatim account-layout doc-comment copied from its processor. ## Testing Verification - Unit tests assert the exact `AccountMeta` list (incl. trailing `[payer, system]`) and the borsh tag byte for every builder, covering the conditional/variable-account paths. Closes #4023. Part of RFC-26 ([rfcs/rfc26-rust-instruction-builder-library.md](rfcs/rfc26-rust-instruction-builder-library.md)). --- ### PR stack (RFC-26 builder library) Stacked PRs, merge in order (each is based on the previous one's branch): 1. #4049 — R0 scaffold + exemplars 2. #4050 — R1 device 3. #4051 — R2 link 4. #4052 — R3 user 5. #4053 — R4 location/exchange/contributor 6. #4054 — R5 multicastgroup + allowlists 7. #4055 — R6 tenant + permission 8. #4056 — R7 topology + feed 9. #4057 — R8 accesspass + resource ← **this PR** 10. #4058 — R9 globalstate/config/allowlist/index/migrate R10 (commands/* migration + program-test) and RF (fixtures) follow as separate PRs.
…migrate builders (RFC-26 R9) (#4058) ## Summary RFC-26 **R9** (stacked on the previous phase's branch). init_global_state and migrate (no authorize -> build); setters, set_global_config (config PDA + all 8 resource pools), foundation/QA allowlist toggles, index create/delete. After this every buildable variant has a builder. All builders route through `authorize()` -> `build_with_permission` unless noted; each carries a verbatim account-layout doc-comment copied from its processor. ## Testing Verification - Unit tests assert the exact `AccountMeta` list (incl. trailing `[payer, system]`) and the borsh tag byte for every builder, covering the conditional/variable-account paths. Closes #4024. Part of RFC-26 ([rfcs/rfc26-rust-instruction-builder-library.md](rfcs/rfc26-rust-instruction-builder-library.md)). --- ### PR stack (RFC-26 builder library) Stacked PRs, merge in order (each is based on the previous one's branch): 1. #4049 — R0 scaffold + exemplars 2. #4050 — R1 device 3. #4051 — R2 link 4. #4052 — R3 user 5. #4053 — R4 location/exchange/contributor 6. #4054 — R5 multicastgroup + allowlists 7. #4055 — R6 tenant + permission 8. #4056 — R7 topology + feed 9. #4057 — R8 accesspass + resource 10. #4058 — R9 globalstate/config/allowlist/index/migrate ← **this PR** R10 (commands/* migration + program-test) and RF (fixtures) follow as separate PRs.
…4059) ## Summary RFC-26 **RF**: golden fixtures for the instruction builders + a CI drift guard. - Extend the serviceability fixture generator to depend on `doublezero-serviceability-instruction` and emit, per instruction, `ix_<name>.bin` (wire bytes = tag + borsh) and `ix_<name>.json` (`{ variant, data_hex, accounts: [{pubkey, is_signer, is_writable}] }`) from fixed, deterministic inputs. - Representative set covering the trickiest layouts: `create_device` (variable dz_prefix), `delete_device` (atomic close), `create_link`, `create_subscribe_user` (optional feed), `create_user` (length-detected), `clear_topology` / `assign_topology_node_segments` (batched), `set_global_config` (config PDA + all 8 pools). - `make generate-fixtures-check` (regenerate + fail on drift, scoped to the emitted `.bin`/`.json`) and a `fixtures-check` job in the `rust` workflow. These fixtures capture byte-for-byte the current SDK trailing convention (they will drive Go/Python/TS parity in a later phase). ## Testing Verification - `make generate-fixtures-check` passes (regenerated output is byte-identical to the committed fixtures); a hand-edited fixture makes it fail as expected. Closes #4026. Part of RFC-26 ([rfcs/rfc26-rust-instruction-builder-library.md](rfcs/rfc26-rust-instruction-builder-library.md)). --- ### PR stack (RFC-26 builder library) Stacked PRs, merge in order (each based on the previous one's branch): 1. #4049 — R0 scaffold + exemplars 2. #4050 — R1 device 3. #4051 — R2 link 4. #4052 — R3 user 5. #4053 — R4 location/exchange/contributor 6. #4054 — R5 multicastgroup + allowlists 7. #4055 — R6 tenant + permission 8. #4056 — R7 topology + feed 9. #4057 — R8 accesspass + resource 10. #4058 — R9 globalstate/config/allowlist/index/migrate 11. this PR — RF fixtures + CI guard R10 (commands/* migration + program-test) is the final PR and will carry the global changelog entry.
Summary
RFC-26 R0: scaffold the pure, RPC-free instruction-builder crate
doublezero-serviceability-instruction— SPL-style builders that return a single unsignedInstructionper serviceability instruction, no signing/sending.common::build(no-permission trailing[payer, system]) +common::build_with_permission(the deferred, activate-in-one-place Permission append) +compute_budget_prelude(1.4M CU / 256 KiB).create_device,delete_device(legacy/atomic),create_link,create_subscribe_user.doublezero-serviceability+solana-program+solana-system-interface+solana-compute-budget-interface— no RPC tree.The
suspend_deviceexemplar from the RFC was replaced withdelete_device(SuspendDeviceis a deprecated variant); the RFC and #4015 were updated, and the "length-detected family" classification was corrected (onlyCreateUseris length-detected).Why
Instruction assembly is currently coupled to signing+sending inside
commands/*::execute(); there is no reusablebuild_xxx(args) -> Instruction. See rfcs/rfc26-rust-instruction-builder-library.md.Testing Verification
AccountMetalist (incl. trailing[payer, system]) and the borsh tag byte for each exemplar, including thedelete_devicelegacy vs atomic layouts and thecreate_subscribe_useroptional-feed placement.Closes #4015. Part of RFC-26.
PR stack (RFC-26 builder library)
Stacked PRs, merge in order (each is based on the previous one's branch):
R10 (commands/* migration + program-test) and RF (fixtures) follow as separate PRs.