serviceability-instruction: golden fixtures + CI guard (RFC-26 RF) - #4059
Conversation
e0235a0 to
9240816
Compare
70541a5 to
57fe622
Compare
9240816 to
9e23736
Compare
57fe622 to
7ab5da2
Compare
155cbc7 to
d934652
Compare
7ab5da2 to
92e417d
Compare
d934652 to
bde6207
Compare
92e417d to
b0a7627
Compare
ben-dz
left a comment
There was a problem hiding this comment.
Two things worth changing before merge; nothing blocking. (1) The new fixtures-check job in rust.yml is a strict duplicate of the pre-existing sdk-fixture-drift job in sdk.yml — same make check-fixtures, both triggered on every PR/push with no path filters — so the drift check runs twice per PR for zero added coverage, and the new copy runs on an oversized ubuntu-24.04-16c-64gb runner with no timeout-minutes. Delete one, or move the check and delete the other. (2) The set_global_config fixture is built from SetGlobalConfigArgs::default(), emitting an all-zero body that pins only the length — it can't catch field-order/endianness/signedness/Option-encoding regressions, which is the whole point of a wire-format golden; use distinct nonzero values. Low: the fixture JSON records neither program_id nor the input args, so a future parity harness must read the Rust source to reproduce inputs. Fixtures are deterministic (regenerated locally, byte-identical) and otherwise correctly capture the builder output for the representative set.
…+ CI guard (RFC-26 RF)
Extend the serviceability fixture generator to depend on the builder crate and
emit, per instruction, ix_<name>.bin (tag + borsh wire bytes) and ix_<name>.json
({ variant, data_hex, accounts:[{pubkey,is_signer,is_writable}] }) with fixed,
deterministic inputs. Covers a representative set exercising 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.
Add `make generate-fixtures-check` (regenerate + fail on drift) and a
`fixtures-check` job in the rust CI workflow.
Refs #4026, RFC-26.
…nerator feed borrow The clear_topology builder now marks the topology account writable (the processor decrements its reference_count), so regenerate ix_clear_topology.json. Also fix the generator to pass feed as Option<&Pubkey> (Some(&d)), matching the current create_subscribe_user signature.
…iscriminating global_config fixture, self-describing goldens - Delete duplicate fixtures-check job from rust.yml; sdk-fixture-drift in sdk.yml already runs make check-fixtures (on ubuntu-latest with a timeout). - Build set_global_config args from distinct nonzero values instead of ::default(), so the golden pins field order, endianness, both u32 ASNs, and the Some-encoding of next_bgp_community rather than only the body length. - Record instruction.program_id in the ix_*.json goldens so a future parity harness need not read the generator to reproduce inputs.
b0a7627 to
ebe5e54
Compare
ben-dz
left a comment
There was a problem hiding this comment.
All three findings from the prior review are addressed in ebe5e54 and verified in the current code: the duplicate fixtures-check CI job is deleted from rust.yml (sdk.yml's better-provisioned sdk-fixture-drift retained); set_global_config now builds SetGlobalConfigArgs with distinct nonzero values so the golden pins field order, both u32 ASNs, and the optional community's Some-encoding instead of an all-zero body; and IxFixtureMeta records program_id in every ix_*.json. The passing sdk-fixture-drift job confirms the goldens regenerate byte-identically. lgtm.
…R10) (#4060) ## 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 #3647. - **Fixes #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`](https://github.com/malbeclabs/doublezero/blob/main/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. <details> <summary>Key files (click to expand)</summary> - `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 #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. </details> ## 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 #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.
Summary
RFC-26 RF: golden fixtures for the instruction builders + a CI drift guard.
doublezero-serviceability-instructionand emit, per instruction,ix_<name>.bin(wire bytes = tag + borsh) andix_<name>.json({ variant, data_hex, accounts: [{pubkey, is_signer, is_writable}] }) from fixed, deterministic inputs.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 afixtures-checkjob in therustworkflow.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-checkpasses (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).
PR stack (RFC-26 builder library)
Stacked PRs, merge in order (each based on the previous one's branch):
R10 (commands/* migration + program-test) is the final PR and will carry the global changelog entry.