serviceability-instruction: scaffold pure instruction-builder crate (RFC-26 R0)#4049
serviceability-instruction: scaffold pure instruction-builder crate (RFC-26 R0)#4049juan-malbeclabs wants to merge 1 commit into
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
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.
| /// ``` | ||
| /// | ||
| /// `owners.len()` drives both the resource-PDA loop and `args.resource_count`. | ||
| /// This builder never appends a Permission account. |
There was a problem hiding this comment.
[Architecture] Doc contradiction: this says the builder never appends a Permission account, but both paths call common::build_with_permission, and process_delete_device calls authorize() (NETWORK_ADMIN, processors/device/delete.rs:110-121) — so at Permission rollout this builder WILL carry one. In a crate whose doc comments are the anti-drift spec, this sentence instructs a future maintainer to reassign the builder to common::build, silently dropping the NETWORK_ADMIN path for non-contributor payers. Replace with the accurate statement used in create_subscribe_user's doc: the processor routes through authorize(), so the builder is assigned to build_with_permission and carries a trailing Permission PDA once activated.
| | PR | Scope | | ||
| |----|-------| | ||
| | R0 | Scaffold crate + `common` + `compute_budget_prelude` + 4 exemplar builders (`create_device`, `create_link`, `suspend_device`, `create_subscribe_user`) + first fixtures | | ||
| | R0 | Scaffold crate + `common` + `compute_budget_prelude` + 4 exemplar builders (`create_device`, `create_link`, `delete_device`, `create_subscribe_user`) + first fixtures | |
There was a problem hiding this comment.
[Architecture] Plan/PR mismatch: this R0 row (edited by this PR) still promises "first fixtures," but the PR ships neither fixtures nor solana-program-test coverage — and the RFC names account-order drift "the biggest risk." The unit tests are self-referential: they recompute the expected Vec<AccountMeta> with the same pda.rs helpers and the same human-transcribed ordering as the builders, so they cannot detect drift from the processor; only tag bytes are independently asserted. Land the fixture generator alongside R0, add one solana-program-test exemplar for the four builders, or amend this row (and the PR description) so plan and PR agree.
| /// Builders are **assigned** to this method per-instruction as they are | ||
| /// implemented; the append itself is **deferred** (RFC-26 "Permission account"). | ||
| /// Today it simply delegates to [`build`], so assigning a builder here is | ||
| /// behavior-preserving. At the permission rollout, enable the append **here, in |
There was a problem hiding this comment.
[Architecture] Activation recipe overstates safety: authorize() returns InvalidAccountData when the appended Permission PDA does not exist onchain (the owner check runs before the foundation-recovery/legacy fallback), and today's SDK appends the PDA only after an RPC existence check — which a pure builder cannot do. Flipping this append on unconditionally would break every payer without an onchain Permission account. Add a sentence that the append must not be enabled until the RFC's open existence question is resolved (e.g. RequirePermissionAccounts enforced or all payers guaranteed a Permission account).
| resource_total <= u8::MAX as usize, | ||
| "device resource_count {resource_total} exceeds u8::MAX" | ||
| ); | ||
| args.resource_count = u8::try_from(resource_total).unwrap_or(u8::MAX); |
There was a problem hiding this comment.
[Architecture + Security] Silent invariant break in release: unwrap_or(u8::MAX) (here and in delete_device, lines 181-186) emits a resource_count that disagrees with the account list — the exact invariant this crate exists to make impossible — guarded only by debug_assert!. The condition is unreachable (>255 resources exceeds any transaction's account budget; security review confirmed no exploit path), and the existing SDK returns a hard error here. Replace with u8::try_from(...).expect("resource count exceeds u8::MAX") — a panic in the unreachable case is strictly better than an instruction whose declared count disagrees with its accounts.
| payer: &Pubkey, | ||
| ) -> Instruction { | ||
| accounts.push(AccountMeta::new(*payer, true)); | ||
| accounts.push(AccountMeta::new( |
There was a problem hiding this comment.
[All reviewers] System program marked writable vs RFC glossary: AccountMeta::new here matches today's assemble_instructions (sdk/rs/src/client.rs:215; harmless — the runtime demotes reserved keys), but the RFC glossary defines the trailing convention as "system_program (readonly)". Golden fixtures will freeze whichever flag ships. Decide now: switch to new_readonly, or keep writable for byte parity and correct the RFC glossary; record the choice in a comment.
| contributor: &Pubkey, | ||
| location: &Pubkey, | ||
| exchange: &Pubkey, | ||
| account_index: u128, |
There was a problem hiding this comment.
[Architecture] account_index here vs link_index in create_link both mean "globalstate.account_index + 1 for the new entity," with the +1 convention living only in prose. Unify the name (e.g. device_index/link_index) before R1/R2 replicate the split across ~90 builders.
|
|
||
| /// The unicast-default topology account is required; `CreateLink` auto-tags the | ||
| /// link into it. | ||
| const UNICAST_DEFAULT_TOPOLOGY: &str = "unicast-default"; |
There was a problem hiding this comment.
[Architecture] Fourth copy of the "unicast-default" literal (processor link/create.rs:254, SDK commands/link/create.rs, CLI). Export a pub const from doublezero-serviceability next to get_topology_pda and consume it everywhere.
|
|
||
| /// Protocol-max compute-unit limit for a serviceability transaction. Mirrors | ||
| /// `sdk/rs/src/client.rs::MAX_COMPUTE_UNIT_LIMIT`. | ||
| pub const MAX_COMPUTE_UNIT_LIMIT: u32 = 1_400_000; |
There was a problem hiding this comment.
[Architecture] MAX_COMPUTE_UNIT_LIMIT/MAX_HEAP_FRAME_BYTES duplicate private consts in sdk/rs/src/client.rs:64-65 with only reciprocal "Mirrors" comments as a tether. Fine for R0; at the SDK-migration PR, make the SDK consume these exports. Tracking only.
| /// `CreateDevice` (variant 20). | ||
| /// | ||
| /// Account layout (processor `next_account_info` order), before the trailing | ||
| /// `[payer, system]` appended by [`common::build`]: |
There was a problem hiding this comment.
[Architecture] Rustdoc warnings (verified with cargo doc): public builder docs link the private common::build (also link.rs:22, user.rs:18), emitting private_intra_doc_links warnings — and these builders actually call build_with_permission, not build. Also lib.rs:8's [Transaction](solana_program) points at the crate root, not a Transaction type. Reference public items (or build_with_permission in plain text) and fix the lib.rs link.
| feed: Option<Pubkey>, | ||
| mut args: UserCreateSubscribeArgs, | ||
| ) -> Instruction { | ||
| args.dz_prefix_count = dz_prefix_count; |
There was a problem hiding this comment.
[Architecture] The write-back silently discards any caller-set dz_prefix_count (same for resource_count in create_device). Documented, but debug_assert!(args.dz_prefix_count == 0 || args.dz_prefix_count == dz_prefix_count) would catch caller confusion cheaply.
2 participants
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.