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 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.