sdk: migrate serviceability commands to RFC-26 instruction builders (R10)

[juan-malbeclabs]

Summary of Changes

• Delegate every serviceability commands/* execute() to the pure doublezero-serviceability-instruction builders (RFC-26), so account layout has a single source of truth in the builder crate rather than being hand-assembled per command.
• Replace the four DoubleZeroClient::execute_*(instruction, accounts) trait methods and the client-side account assembly with a single send_transaction(Instruction) that just prepends the compute-budget prelude, signs, and sends. Command execute() signatures/returns are unchanged, so CLI/sentinel/daemon consumers are unaffected. The _quiet variant is not carried forward — its only caller was the activator/ crate, deleted in activator: delete activator/ crate from workspace #3647.
• Fixes sdk: ClearTopologyCommand passes topology PDA read-only but the processor requires it writable #4078: doublezero topology clear reverted with Topology Account is not writable on a live topology — the SDK passed the topology PDA read-only while the processor asserts it writable whenever it actually clears a link reference (it decrements reference_count). The command therefore only worked on the already-closed-topology path, blocking topology delete, which requires reference_count == 0. The clear_topology builder passes the account writable; writable is a strict superset privilege, so no other path changes.
• Breaking (see CHANGELOG ### Breaking): the Rust SDK no longer attaches the payer's Permission PDA to serviceability transactions — authorize() uses the legacy GlobalState allowlist path until the builder-side Permission append is re-enabled per instruction (must be sequenced with the permission-model rollout). A payer whose only grant is an activated Permission account would lose access to gated commands.
  • No cluster is exposed — verified against live chain state. RequirePermissionAccounts is off everywhere, so the legacy fallback is live. mainnet-beta's 8 Permission accounts all grant FOUNDATION to keys already in foundation_allowlist. The one grant on testnet/devnet that no legacy authority covers (ACCESS_PASS_ADMIN to EWFXDTBWhNxsmj4mbahZL7HH1XpmC6Ao3LZUhJGCyyPh) belongs to the shred oracle, which never uses DoubleZeroClient — it assembles its serviceability instructions itself and appends its own Permission account after its own RPC existence check (doublezero-shreds, crates/shred-oracle/src/dz_ledger.rs, permission_account_meta, 9 call sites). It is unaffected by this send path. No deploy blocker.
  • The next grant is now self-documenting. permission set emits a tracing::warn! whenever the resulting bitmask carries an AUTHORIZE_GATED_FLAGS bit, and permission audit renders a matching paragraph (plus a sdk_attaches_permission_accounts field in --json), stating that the CLI/Rust SDK do not attach Permission accounts yet so the key still needs a legacy allowlist or authority entry. The chain-state check above is point-in-time by nature; this covers grants made after it. Both are removed when the append is re-enabled.
• Add a solana-program-test safety net that runs the highest-cardinality builders (create_device, create_link, create_subscribe_user, atomic delete_device, clear_topology) against the real program to catch account-order drift.
• Remove the now-unreachable permission-resolve/cache/stale-retry machinery from client.rs (the append, when re-enabled, lands in the builder — derived offline — so the RPC-based path is not needed).

This is the final PR of the RFC-26 stack; it carries the global changelog entry. See rfcs/rfc26-rust-instruction-builder-library.md.

Stack: R0 → R1 → … → R9 → RF (#4059, merged) → R10 (this PR). Now rebased onto main, so the diff below is R10 only.

Diff Breakdown

Category	Files	Lines (+/-)	Net
Core logic	13	+868 / -1441	-573
Scaffolding	78	+2396 / -3298	-902
Tests	1	+763 / -0	+763
Config	3	+6 / -0	+6
Docs	2	+10 / -3	+7
Generated	1	+2 / -0	+2

Net −697 lines: a mostly-mechanical migration (77 command delegations) on top of a genuine send-path/trait rewrite and one new program-test suite. This exceeds doublezero's ~500-line PR norm, but the migration + its program-test safety net are one logical unit and the net change is a reduction; splitting would land the safety net separately from the code it guards.

Key files (click to expand)

• smartcontract/programs/doublezero-serviceability/tests/rfc26_builders_test.rs (+763) — new program-test safety net; builds via the RFC-26 builders and runs them against the real program.
• smartcontract/sdk/rs/src/client.rs (+70/-560) — rework execute_transaction_inner into send_transaction_inner(ix); drop assemble_instructions, the permission-resolve/cache and stale-retry loop, and their tests.
• smartcontract/sdk/rs/src/commands/user/delete.rs (+153/-246) — composed unsubscribe + delete_user builder; keeps cascade orchestration.
• smartcontract/sdk/rs/src/commands/exchange/setdevice.rs (+117/-131) — two-send command migrated to set_device_exchange builder.
• smartcontract/sdk/rs/src/commands/tenant/delete.rs (+108/-116) — delete_tenant builder; keeps cascade + reference-count poll.
• smartcontract/sdk/rs/src/commands/link/update.rs (+77/-81) — update_link builder with LinkUpdateAuthority; builder recomputes use_onchain_allocation.
• smartcontract/sdk/rs/src/commands/device/delete.rs (+59/-87) — legacy/atomic paths via delete_device + DeviceDeleteResources.
• smartcontract/sdk/rs/src/commands/topology/clear.rs (+57/-85) — batched sends via clear_topology; carries the sdk: ClearTopologyCommand passes topology PDA read-only but the processor requires it writable #4078 writable-PDA fix.
• smartcontract/sdk/rs/src/doublezeroclient.rs (+7/-35) — trait: four execute_* methods → a single send_transaction.
• smartcontract/cli/src/permission/{set,audit}.rs (+92/-1) — operator-facing warning that Permission grants are inert until the builder-side append returns.

Testing Verification

• New solana-program-test suite (rfc26_builders_test.rs): 5 tests exercising the highest-cardinality builders against the real program — create_device, create_link + clear_topology, create_subscribe_user (atomic subscriber), and atomic delete_device (owners read on-chain in processor order). All pass.
• clear_topology covers both processor branches: cleared_count == 0 (untagged topology, no write) and cleared_count > 0 (the topology the link is tagged into), the latter exercising the is_writable assert and reference_count decrement that sdk: ClearTopologyCommand passes topology PDA read-only but the processor requires it writable #4078 is about. Verified it is a real guard — flipping the builder to new_readonly fails with Topology Account is not writable.
• cargo test -p doublezero_sdk: 167 passed. Rewritten command tests assert predicate::eq(builder(...)) on the exact instruction each command sends. The composed cascades in user/delete and tenant/delete pin every step — the resolved access-pass PDA, dz_prefix_count, the optional tenant account, the [current_tenant, new_tenant] reset pair, and the trailing owner (which differs from the payer on the foundation-key path).
• Assertion strength across sdk/rs/src/commands/ went up, not down: predicate::always() appears in 0 files here, down from 8 on main (where it relaxed the accounts argument of the two-arg execute_* expectation). The four multicastgroup/allowlist/* tests now pin the built instruction end to end — on main they asserted the DoubleZeroInstruction but not the accounts — with user_payer deliberately distinct from the group pubkey so an account-meta swap fails. Confirmed discriminating: perturbing client_ip in the expected instruction fails with No matching expectation found.
• permission set's new warning is gated on AUTHORIZE_GATED_FLAGS, unit-tested to fire for every gated flag and stay quiet for SENTINEL / QA / FEED_AUTHORITY (which no processor hands to authorize()), so it cannot become background noise.
• The u8 bounds on resource_count / topology_count stay errors rather than builder panics: device/create, device/interface/create and device/interface/update guard before building, with tests asserting the exact messages for both interface guards.
• Confirmed the batched topology and multi-send commands (exchange/setdevice, topology/clear, topology/assign_node_segments) preserve their per-instruction sequencing.

[ben-dz]

Two things should change before this merges.

The Permission-PDA drop needs release gating. With the builder-side append deferred and client.rs's resolve/cache/retry deleted, every migrated instruction takes the None branch of authorize(). A payer whose only grant is an activated Permission account — not on foundation_allowlist/qa_allowlist, not an authority key — starts getting NotAllowed on every gated command the moment this ships. Please confirm zero such grants exist on devnet/testnet/mainnet-beta, and note that doublezero permission audit does not answer this (it reports the strict-mode direction only). The entry also belongs in the ### Breaking section this Unreleased block already has, following the feed_pk → feed_pks precedent.

The composed cascade tests lost their exact-instruction assertions, opposite to what the Testing Verification section says: predicate::always() went 0 → 6 in user/delete.rs and 3 → 7 in tenant/delete.rs. That is the one path in this migration that is more than mechanical, and it is now unasserted.

Also worth addressing: send_transaction_quiet has no consumer left (the activator was deleted from the workspace), the clear_topology program test skips the writable path the builder's divergence exists for and its comment states the opposite, and three device commands traded an eyre error for a release-build panic on over-long input.

The mechanical migration itself holds up — tracing the risky layouts found no swapped pubkeys, dropped accounts, or writability drift beyond the documented clear_topology divergence.

Findings not anchored to the current diff:

• rfcs/rfc26-rust-instruction-builder-library.md:176 — low: "The permission cache (note_transaction_sent/invalidation) stays" describes a subsystem this PR deletes, and contradicts the Open Questions bullet updated at line 245. One-sentence fix.

[juan-malbeclabs]

Thanks — all seven findings held up under checking, including the two blocking ones. Addressed in d807db7 and a691c80, with details inline on each thread.

The Permission-PDA gate — verified against live chain state

You were right that permission audit can't answer this, so I queried the clusters directly (getProgramAccounts on AccountType::Permission = 15) and cross-referenced every user_payer against live GlobalState, replicating check_legacy_any's mapping flag by flag:

Cluster	Permission accounts	Not covered by legacy
mainnet-beta	8 — all FOUNDATION, every payer in foundation_allowlist	0
testnet	16	1
devnet	1	1

Same key on both: EWFXDTBWhNxsmj4mbahZL7HH1XpmC6Ao3LZUhJGCyyPh, ACCESS_PASS_ADMIN, Activated — not in foundation_allowlist, not the sentinel, and feed_authority_pk unset on both. It would lose SetAccessPass, CloseAccessPass, SetAccessPassFeeds, SetAccessPassFlags, and both multicast subscriber-allowlist verbs.

On your second point: RequirePermissionAccounts is off everywhere — feature_flags = 1 is the reserved deprecated bit 0, not strict mode (bit 1). The legacy fallback is live, so there's no total lockout.

What still needs an owner, not code: whether EWFXDTBW… drives those instructions through the Rust SDK, the only surface this PR changes. PERMISSION.md:121 describes precisely this grant shape as the feed oracle's, so a Go/TS oracle would be untouched. Either confirm that, or add the key to foundation_allowlist on testnet and devnet before this ships. The entry now lives in ### Breaking and states the gate explicitly.

Everything else

• Cascade assertions — predicate::always() across sdk/rs/src/commands/ is 4, down from 17; the remaining 4 are pre-existing on main. Both cascade files now assert predicate::eq(<builder>(...)) at every send, and the dead let (_pda, _) bindings are gone. The Testing Verification section is corrected — that claim was backwards.
• clear_topology — comment fixed (writable, deliberate, sdk: ClearTopologyCommand passes topology PDA read-only but the processor requires it writable #4078) and a cleared_count > 0 case added against the tagged topology. Verified it's a real guard: flipping the builder to read-only fails with Topology Account is not writable.
• send_transaction_quiet — dropped, with SimulationError / SimulationTransactionError and the quiet branch. Dead since activator: delete activator/ crate from workspace #3647 deleted activator/, so pre-existing rather than newly orphaned. rfc26:244 now records the decision instead of still proposing the variant.
• u8 guards — restored in all three commands with the original messages, builders left infallible, plus tests for both interface guards (neither had one on main).
• Batch-size cap comments — the stale doc was on both topology re-exports; replaced with pointers to the builder crate so the numbers live in one place.
• rfc26:176 — now says the permission cache is removed, matching the Open Questions bullet.

Green: cargo test -p doublezero_sdk 167 passed, full doublezero-serviceability suite passed (incl. the 5 RFC-26 builder program tests), make rust-lint and make rust-fmt clean.

Two judgement calls, flagged rather than buried:

1. Guard placement — in the commands, not the builders, since RFC-26 fixes builders as infallible and making them fallible would ripple through every call site. The .expect(...) stays as an unreachable backstop.
2. Removing the two pub error types — technically an API break. Nothing references them in or out of the workspace, and keeping them would leave orphans behind a removed method. Noted in the CHANGELOG.

[ben-dz]

lgtm — all seven earlier findings are addressed, and the one open question is answered.

On whether EWFXDTBWhNxsmj4mbahZL7HH1XpmC6Ao3LZUhJGCyyPh loses authorization through the Rust SDK: it does not, for the documented candidate. The shred oracle — which PERMISSION.md:119-122 describes as the holder of that ACCESS_PASS_ADMIN-only grant shape — never touches the SDK send path. crates/shred-oracle depends on doublezero_sdk only for data types and PDA helpers (Device, User, Exchange, get_accesspass_pda, get_permission_pda, …); there is no DoubleZeroClient, no commands/* usage, and no execute_*/send_transaction call anywhere in that repo. It assembles its serviceability instructions itself in crates/shred-oracle/src/dz_ledger.rs and appends its own Permission AccountMeta per instruction via permission_account_meta() (:264-286, applied at nine sites covering SetAccessPass, CloseAccessPass, SetAccessPassFeeds, AddMulticastGroupSubAllowlist, RemoveMulticastGroupSubAllowlist, CreateSubscribeUser, UpdateMulticastGroupRoles, and DeleteUser — the exact set the deploy note names). Its doublezero_sdk dependency is pinned by git rev, so the type surface does not move on merge either.

So the oracle keeps authorizing via its own append, independent of the SDK. Residual exposure would need an unidentified Rust-SDK consumer holding that key on testnet or devnet; the in-repo consumers are all operator-driven tools and mainnet-beta has zero exposure. No deploy blocker.

One note for whenever the deferred builder-side Permission append resumes: the oracle solved the offline-existence problem locally, so the builder will face the same constraint documented at crates/doublezero-serviceability-instruction/src/common.rs:80-86.

[juan-malbeclabs]

Thanks for tracing the oracle — that closes the one thing I couldn't settle from this repo.

I verified your findings before encoding them, since the deploy note now asserts them:

• No DoubleZeroClient anywhere in doublezero-shreds (zero occurrences); its doublezero_sdk imports are data types and status enums only — Device, User, Exchange, AccountData, AccountType, DeviceStatus, ExchangeStatus, UserStatus, UserType, GlobalState. No commands/*, no send path.
• permission_account_meta() is at crates/shred-oracle/src/dz_ledger.rs:264-287 with 9 call sites, doing exactly the RPC existence check + per-process memoization the SDK just deleted. (My local checkout of that repo was 38 commits behind and predates the helper, so I read it off remote main rather than trusting the stale copy.)

Two follow-ups pushed in 769573c:

1. The CHANGELOG note now records the answer instead of asking for it. It previously told the reader to confirm the key doesn't drive those instructions through the Rust SDK before shipping — stale, and worse than useless once resolved, since it implies a blocker that no longer exists. It now states that no cluster is exposed and why. Same correction applied to the PR description.
2. Your forward-looking note is captured where the next person will hit it — build_with_permission's activation-precondition doc now points at the oracle's caller-side solution and spells out the consequence: activation here needs either the RequirePermissionAccounts guarantee or a caller-supplied "permission exists" input threaded down from the command layer, because a pure builder can't own that check. That's the constraint your last paragraph identifies, and it's better placed next to the deferred append than only in review history.

Still green after both: cargo test -p doublezero_sdk 167 passed, make rust-lint clean.

[elitegreg]

Reviewed the full stack tip. The migration itself holds up well: I hand-compared the builder call against the old hand-assembled account list for every non-mechanical command (link/update including the side-A/side-Z preamble split and the conditional side_a/side_z + resource-extension appends, device/delete on both the legacy and atomic paths, user/create, user/delete, tenant/delete, exchange/setdevice two-send sequencing, topology/clear, topology/assign_node_segments) and found no drift. The _quiet removal checks out: nothing in the workspace calls the removed trait methods, and geolocation/'s execute_transaction is a separate trait, untouched.

Verified locally:

• cargo test -p doublezero_sdk --lib — 167 passed
• cargo test -p doublezero-serviceability --test rfc26_builders_test — 5 passed
• cargo check --workspace --all-targets — clean, no new warnings (the doublezero_geolocation::entrypoint feature-unification errors reproduce identically on main)

Four things to address before merge.

1. This PR fixes #4078 but does not say so

The builder passes the clear_topology topology PDA writable, which is exactly the fix #4078 asks for. That issue is open and describes a user-facing break: the processor asserts topology_account.is_writable whenever cleared_count > 0, so on main topology clear reverts with "Topology Account is not writable" on a live topology, which is the mandatory precursor to topology delete. After this PR it works.

Right now the only mention is a comment inside rfc26_builders_test.rs. Please add a ### Fixed CHANGELOG entry under SDK and a Fixes #4078 line in the PR body so the issue closes on merge. As written, a real bug fix ships invisibly and #4078 stays open pointing at a file that no longer has the bug.

2. Four allowlist tests lost their instruction assertion

multicastgroup/allowlist/publisher/add.rs, publisher/remove.rs, subscriber/add.rs, subscriber/remove.rs now read:

client
    .expect_send_transaction()
    .with(predicate::always())
    .returning(|_| Ok(Signature::new_unique()));

On main the same tests pinned the instruction and relaxed only the accounts:

.with(
    predicate::eq(DoubleZeroInstruction::AddMulticastGroupPubAllowlist(
        AddMulticastGroupPubAllowlistArgs { client_ip: ..., user_payer: pubkey },
    )),
    predicate::always(),
)

So client_ip and user_payer were asserted before and are not now. The PR body's "predicate::always() across sdk/rs/src/commands/ is 4, all pre-existing on main" is accurate as a file count but understates this: the assertion strength dropped in all four, and these are the only migrated tests that assert nothing about what gets sent. One-line fix each, using the predicate::eq(add_multicast_group_pub_allowlist(...)) pattern the other 73 migrations already use.

3. A Permission grant becomes a silent no-op with nothing telling the operator

The exposure analysis in the CHANGELOG is thorough and I am not disputing the conclusion. The gap is forward-looking: once this lands, doublezero permission set produces a grant that cannot work through the CLI or Rust SDK, and nothing at runtime says so. The CHANGELOG itself notes permission audit answers only the inverse (strict-mode) direction, so the natural place an operator would look does not surface it either.

Suggest a tracing::warn! on permission set / permission update, or an added line in permission audit, stating that the Rust SDK does not attach Permission accounts yet and gated commands still require a legacy allowlist or authority entry. It can be removed when the builder-side append is re-enabled. The chain-state check is point-in-time by nature; this makes the next grant self-documenting instead of a debugging session.

4. Please rebase onto origin/main

RF (#4059) merged as 9c2ceb76, but this branch still carries its three unsquashed commits, so the PR renders as 115 files / +4548 −4751 when only ~96 files are actually new. I diffed the branch's RF content against merged main: byte-identical apart from the two intended builder-crate additions (set_access_pass_flags, the build_with_permission doc note), so the rebase is content-neutral and makes the diff match what needs reviewing.

Non-blocking notes

• Several commands pass args fields the builder immediately overwrites: use_onchain_allocation: false in link/update, resource_count: 0 in device/create, dz_prefix_count / multicast_publisher_count in user/delete, user/create, user/requestban. Correct, and documented on the builder side, but at the call site it reads as if the caller controls them. A short comment at each site would help the next reader.
• Roughly 60 commands drop the GetGlobalStateCommand round-trip, which is a real latency win, but they also lose the fast-fail "Globalstate not initialized" message in favor of an onchain error on an uninitialized cluster. Probably the right trade; flagging it as deliberate rather than incidental.
• device/delete has no u8 guard on owners.len() where device/create and the two interface verbs got one, so it relies on the builder's .expect(...). Unreachable given the transaction account budget, and the builder comment says as much, so this is consistency rather than a bug.
• Worth stating in the RFC or the test module doc: the migrated command tests are structurally tautological with respect to account layout. predicate::eq(builder(...)) catches wrong arguments but cannot catch a builder layout regression, since both sides call the same builder. That is by design, with the golden fixtures and the program test as the real net, but it means around 70 builders currently have no program-level coverage. A note keeps a future reader from over-trusting the 167 passing tests.

[juan-malbeclabs]

Thanks — all four held up under checking. Addressed in cb9fa51c, on a branch now rebased onto main.

1. #4078. Added Fixes #4078 to the PR body and a CHANGELOG entry covering the revert-on-live-topology break and the writable-PDA fix.

One deviation worth flagging: you asked for a ### Fixed section, but this CHANGELOG has never had one — every past fix is a bullet under ### Changes, including the closely related ClearTopology account-layout fix (line 691). I followed the file's convention rather than introducing a new section. Happy to switch if you'd rather establish ### Fixed here.

2. Allowlist assertions. All four now pin the built instruction with predicate::eq(add_multicast_group_pub_allowlist(...)) and friends.

Two things beyond the one-line fix. user_payer is now a distinct pubkey from the group pubkey — previously both were the same value, so the builder's AccountMeta::new(user_payer, false) and the mgroup meta were indistinguishable and a swap between them would have passed. And I confirmed the assertion actually bites: perturbing client_ip in the expected instruction fails with No matching expectation found, then reverted.

You were also right that the PR body understated this, and the real numbers cut harder than the file count suggested. predicate::always() under sdk/rs/src/commands/ is 8 files on main and 0 here. The four allowlist files kept the count while inverting the meaning — on main the always() was the accounts argument of the two-arg execute_* expectation, paired with an eq() on the instruction; here it had become the only predicate. Body rewritten to say that.

3. Permission grant no-op. Did both hooks you offered rather than picking one, since the audit is where an operator looks but set is where the mistake is made:

• permission set emits a tracing::warn! when the resulting bitmask carries an AUTHORIZE_GATED_FLAGS bit. Extracted as inert_grant_warning(u128) -> Option<&'static str> so it is unit-testable; the test asserts it fires for every gated flag and stays quiet for SENTINEL / QA / FEED_AUTHORITY. Those three are absent from AUTHORIZE_GATED_FLAGS, so no processor ever hands them to authorize() and a Permission account never authorized them in the first place — warning there would be noise, not signal.
• permission audit renders a matching paragraph and exposes sdk_attaches_permission_accounts: false in --json, both keyed off a single const to flip when the append returns.

Small correction: there is no permission update verb. The subcommands are set, delete, flags, get, list, audit, resume, suspend, and set is the only grant-creating path, so that is where the warning lives.

4. Rebase. Done. A plain git rebase origin/main conflicts immediately, since the branch's three RF commits collide with the same content squash-merged as 9c2ceb76. Confirmed they were safe to drop outright before doing so — git diff ebe5e544 9c2ceb76 is empty, i.e. the branch's RF tip tree is byte-identical to merged main. Rebased with git rebase --onto origin/main ebe5e544, replaying only the four R10 commits with zero conflicts, and git diff between the old and new tips is empty, so the rebase is provably content-neutral.

The PR renders 98 files rather than your predicted ~96 — the extra two are the permission/{set,audit}.rs changes from point 3. I also refreshed the Diff Breakdown table in the body, which still described the pre-rebase 115-file diff.

Non-blocking notes. Taking the first two as follow-ups rather than growing this PR further: the call-site comments on builder-overwritten args (use_onchain_allocation, resource_count: 0, dz_prefix_count) are worth doing but touch a dozen files that are otherwise untouched by this round. On the GetGlobalStateCommand round-trip: deliberate, not incidental — the builders derive the globalstate PDA offline, so the fetch existed only to produce the fast-fail message, and paying an RPC round-trip on ~60 commands to pre-empt an error that only occurs on an uninitialized cluster is the wrong trade. On device/delete's missing u8 guard: agreed it is consistency rather than a bug, and left as-is for the reason the builder comment gives — owners.len() is bounded well below u8::MAX by the transaction account budget, whereas the interface verbs' counts are caller-supplied.

Verified: cargo test -p doublezero_sdk --lib 167 passed, cargo test -p doublezero-serviceability-cli --lib 367 passed, cargo clippy --all-targets -Dwarnings clean on both crates.