feat(outpost_solana): declare collateral-settlement accounts on the terminal manifest (SOL-375/379/380) - #553
Conversation
55c7903 to
3c41071
Compare
heifner
left a comment
There was a problem hiding this comment.
Reviewed the collateral-settlement terminal-manifest change against the companion program branch (wire-solana fix/SOL-375-379-380-opp-collateral: inbound.rs, mod.rs, opp_states.rs, epoch_in.rs).
The account inventory itself checks out: I traced every branch (SLASH native/SPL, WITHDRAW_REMIT native/SPL including the rent-refund close, DEPOSIT_REVERT native and SPL-refused) — seed strings, token_code endianness, writable flags, and ATA owner/mint inputs all match the on-chain handlers; the position PDA is correctly declared unconditionally, and dedup/merge via record_terminal_account is sound.
10 findings inline. The top three are one cluster: the PR's degrade-gracefully premise ("omission degrades to on-chain log-and-skip") is inverted relative to the program it targets — require_remaining_account makes a missing-account manifest abort the terminal epoch_in atomically (EffectAccountMissing), which is the epoch-stall failure class. The remainder: an unprovisioned reserve_aggregate ATA that leaves SPL slash seizures stuck, an exception-type gap in the no-throw contract, a cross-repo lock-step hazard on the SPL deposit-revert skip, a per-token repeated config fetch, a duplicate reserve_aggregate PDA derivation, a comment-only cross-extractor coupling, and magic-literal seed strings.
3c41071 to
7b83b64
Compare
heifner
left a comment
There was a problem hiding this comment.
Re-review at 7b83b644ae.
Before the findings, one structural note that changes how this PR should be read and merged.
This PR is stacked on #552, but its base says master
7b83b644ae's ancestry is 8f12916a1a → 7c4c825599 → b349fe1414 (all three of #552) → 7b83b644ae. The base branch is still master, so:
- The diff GitHub renders is 2811 / 1084 across 13 files. The change this PR actually makes is 326 / 75 across 3 files — one commit,
7b83b644ae. Everything else is #552, which I reviewed separately. - Merging this PR into
masterwould silently bring all of #552 with it, regardless of #552's own review state.
The same applies to the companion: wire-solana#409's base is next, while its head 51ac5e8d sits directly on 0dbd6493 — wire-solana#419's head. So both halves of this change are stacked on both halves of the resumable-dispatch change, which is coherent and, I assume, deliberate; only the two base settings are out of step with it.
Retargeting wire-sysio#553 → feat/resumable-opp-dispatch and wire-solana#409 → feat/resumable-opp-dispatch makes each diff show only its own work and makes the merge order explicit. Everything below is scoped to 7b83b644ae alone.
Prior findings
| # | Finding | Status |
|---|---|---|
| 1 | Custody branch read from the mutable OutpostConfig maps rather than the position's pinned custody_mint |
Not addressed — and materially worse on this stack. See inline. |
| 2 | The "degrade to on-chain log-and-skip" fallback was unachievable for an existing SPL position | Resolved by #552's architecture. Delivery and settlement are now separate instructions and the drain resumes from the on-chain cursor, so a transient config-read failure genuinely does re-drive on a later tick. The rewritten rationale on collateral_custody_for_code is now accurate. |
| 3 | Inverted "log-and-skip / never a failed envelope" rationale in the header and extractor comments | Fixed in the source. Still live in the PR description — see below. |
| 4 | Nothing creates the reserve_aggregate ATA, so SPL slash seizures stick |
Not addressed. Re-confirmed against wire-solana#409. See inline. |
| 5 | The "never throws" contract only catches fc::exception |
Not addressed, and now the odd one out in its own file. See inline. |
| 6 | SPL deposit-revert skip hard-codes today's on-chain refusal with no lock-step marker | Not addressed. The comment at :970-973 now explains why the refusal exists (the lamport-denominated penalty stub), which is a real improvement, but still does not say that a program-side policy change requires a matching relay change. When the SPL revert penalty lands, a not-yet-upgraded relay will return metas here while the upgraded handler requires the vault and ATA — EffectAccountMissing, aborting every batch that packs an SPL DEPOSIT_REVERT. One sentence naming the coupling is enough. |
| 7 | OutpostConfig re-fetched and re-decoded per token code |
Partially. See inline. |
| 8 | Duplicate reserve_aggregate PDA derivation |
Fixed — _program_client->reserve_pda is used and the local helper is gone. |
| 9 | Hidden cross-extractor coupling on the recipient sweep | Fixed. The manifest loop now declares the destination wallet itself (:964), and #552 deleted the sweep it used to depend on, so the coupling no longer exists. |
| 10 | Magic-literal seed strings | Not addressed, and now inconsistent with its own stack. See inline. |
New findings
The manifest derivation — the entire subject of this PR — is untested.
The new tests cover the extractor half well: extract_effects_slash_carries_collateral_position_key, extract_effects_keeps_distinct_collateral_token_codes, and extract_effects_skips_non_settling_operator_actions pin which effects come out and which token_code rides each one. Nothing exercises the other half — derive_collateral_position_pda, derive_collateral_vault_pda, or the settlement-owner selection at :985 (reserve_pda for SLASH vs the recipient for WITHDRAW_REMIT).
That is the half where a defect is silent locally and loud only in production: a one-character seed typo or a swapped ATA owner derives a well-formed PDA that simply is not the one the program requires, which surfaces as EffectAccountMissing at runtime, and on this stack that aborts the batch and holds the dispatch cursor. accounts_for_effect is a lambda inside drain_dispatch today, so there is no seam to test through — the same shape of problem #552 solved for the drive loop by factoring it over its RPC touchpoints. Golden-vector assertions on the three derivations against known program-side values would also work and are cheaper.
The PR description no longer describes the code.
Three symbols it documents are absent at head: extract_inbound_collateral_settlement_targets, collateral_settlement_destination, and the extract_inbound_recipient_pubkeys sweep it defers the native depositor to (deleted by #552). The description also still carries the exact inverted rationale that was correctly fixed in the source per finding 3 — "an unreadable config degrades to the handler's log-and-skip rather than aborting the whole envelope delivery", and "SLASH still flips the operator's status and then silently skips the seizure". A missing vault or destination ATA reaches require_remaining_account and returns Err(EffectAccountMissing); it does not skip. Worth refreshing, since the description outlives the branch and this is the failure class the epoch-stall runbooks cover.
Also: "Rebased onto current master (0 behind)" is no longer true given the stacking above, and "E2E flow run for the set pending" — worth landing that run before merge, paired the way #552's was (BRANCH_WIRE_SYSIO + BRANCH_WIRE_SOLANA both on their stacked branches), since the account inventory is exactly what a flow exercises and unit tests do not.
Verdict: the account inventory itself still checks out — I re-traced the branches against wire-solana#409 and the seeds, endianness, writable flags and ATA inputs match the handlers. Findings 1 and 4 are the ones I would want resolved before this merges: both produce silent or permanently-wedged outcomes rather than loud ones, and neither is reachable by retry. 5, 6, 7 and 10 are small. The stacking/base issue is worth fixing first simply so the rest of the review has a diff that matches the change.
7b83b64 to
4f748e8
Compare
cadb498 to
6db0483
Compare
4f748e8 to
af32d3d
Compare
14/14 e2e flows ✅Cross-repo E2E integration run with all three branches pinned to Directly exercising this branch's surfaces:
Run: https://github.com/Wire-Network/wire-platform-build-system/actions/runs/31853124095 |
heifner
left a comment
There was a problem hiding this comment.
Re-review at af32d3d098, traced against wire-solana#409 at 70f229ed and wire-tools-ts#57 at its current head.
The stacking is resolved — #552 is on master, this PR is one commit on 101847d4c6, and the rendered diff (652 / 84 across 3 files) is now exactly the change. The description has been rewritten and matches the code; the inverted "log-and-skip" rationale is gone from it.
The custody rework is the right fix, done well. Reading custody_mint off the CollateralPosition PDA rather than the OutpostConfig maps closes finding 1 at the source, the cache is correctly keyed on (operator, token_code) rather than the token code alone, and collateral_position_custody mirrors reserve_info_for_codes exactly — read outside the decode try, absent/empty degrades, present-but-unreadable throws loudly with the cursor untouched. build_manifests_follows_collateral_custody_per_position pins the property that matters (two operators, one token code, disagreeing custody → different manifests). Extracting build_dispatch_manifests over read_collateral_custody closes most of the "manifest derivation is untested" gap from last time.
I re-traced the account inventory branch by branch against 70f229ed. Five of the six settlement branches match the handlers exactly:
| Branch | Relay declares | Program requires | |
|---|---|---|---|
| WITHDRAW_REMIT native | operator (w), position (w) | both, via assert_writable_remaining_account |
✅ |
| WITHDRAW_REMIT SPL | operator (w), position (w), vault (w), ATA(operator, mint) (w), token program | same + operator as lenient rent recipient | ✅ |
| SLASH native | position (w) | position; vault / reserve_aggregate are named accounts |
✅ |
| SLASH SPL | position (w), vault (w), ATA(reserve_aggregate, mint) (w), token program | same | ✅ |
| DEPOSIT_REVERT native | depositor (w), position (w) | both | ✅ |
| DEPOSIT_REVERT SPL | depositor (w), position (w) | + vault, + ATA(depositor, mint), + token program | ❌ |
That last row is the one blocking finding.
1. The SPL DEPOSIT_REVERT skip is no longer true against the companion, and the outcome is a wedged epoch
outpost_solana_client.cpp:868-871:
// The SPL deposit-revert branch is refused on-chain by design (the
// lamport-denominated penalty stub has no meaning in token units),
// so it never needs vault / ATA / token-program extras.
if (effect.shape == effect_shape::deposit_revert) continue;That premise held at wire-solana 51ac5e8d — the head I traced on 08-12 — where the branch read "SPL DEPOSIT_REVERT is DELIBERATELY NOT IMPLEMENTED … the handler refuses rather than settling on an assumption" and required no accounts.
It does not hold at 70f229ed. handle_deposit_revert's non-native branch (inbound.rs:1561-1579) now settles for real:
let destination_ata =
associated_token::get_associated_token_address(&depositor, &position.custody_mint);
let Some((vault_ai, dest_ai, token_prog_ai, vault_bump_spl)) =
resolve_collateral_vault_transfer(remaining, token_code, position.custody_mint,
destination_ata, "DEPOSIT_REVERT")?resolve_collateral_vault_transfer require_remaining_accounts three accounts the relay never declares for this shape — the collateral_vault PDA (:329), the depositor's destination ATA (:362), and the SPL token program (:403) — and each returns Err(EffectAccountMissing) on absence (mod.rs:326-342, "aborting dispatch"). The ? propagates through dispatch_attestation(...)? in the settle loop, so the whole dispatch_attestations transaction aborts, dispatched_count is never written, and drive_dispatch_rounds repacks the identical window from the identical cursor on every subsequent tick. That is a permanent stall, not a retry — precisely the failure class this PR's own IMPORTANT block describes.
It is reachable on ordinary paths, not exceptional ones. deposit_non_native opens SPL-custody positions, and the depot emits DEPOSIT_REVERT as its routine deposit-rejection mechanism (sysio.opreg.cpp:1125 / :1133 / :1141 / :1153 — unknown operator, type not permitted, etc.), parameterised by chain_code / token_code with nothing restricting it to native tokens. Any rejected SPL collateral deposit on SOL trips it.
The 14/14 e2e run does not cover this: grep -rl "DEPOSIT_REVERT\|DepositRevert" packages/flow-* returns nothing, so no flow exercises the attestation type at all, in either custody mode.
The fix is small, because the existing code already computes the right owner. Deleting the early continue lets deposit_revert fall through to the same custody read and SPL block as withdraw_remit; settlement_owner (:881) is *effect.recipient for every non-slash shape, and the depositor wallet is already declared at :864. That yields exactly the five accounts the handler requires, and the rent-refund recipient the full-drain close looks for. The comment goes with it.
This is finding 6 from both prior reviews, no longer hypothetical. Worth adding the one sentence it asked for in the other direction too: this manifest and handle_deposit_revert must move together, because the relay's shape decisions are unversioned against the program.
2. Neither slash nor deposit_revert is exercised at the manifest level
effect_shape::slash and effect_shape::deposit_revert appear only in extractor tests (:1032, :1076, :1132). Every build_dispatch_manifests case goes through withdraw_remit_effect. The harness even carries a reserve_aggregate member (:1361) that no assertion ever reads.
Those two shapes are the ones with branch-specific manifest logic — the SLASH owner swap (reserve_aggregate instead of the recipient) and its deliberate omission of the operator wallet, and the deposit_revert early return. Finding 1 is exactly the defect a deposit_revert manifest test would have caught, and a SLASH SPL test would pin the one place a wrong ATA owner produces a silently-stuck seizure rather than a loud failure. Two cases in the shape of the one you already wrote.
3. Seed strings are still inline char arrays — third review
:615 {'c','o','l','l','a','t','e','r','a','l','_','p','o','s','i','t','i','o','n'} and :627 {'c','o','l','l','a','t','e','r','a','l','_','v','a','u','l','t'}, against opp_states.rs:56 COLLATERAL_POSITION_SEED and :103 COLLATERAL_VAULT_SEED.
Same file, :41-42:
constexpr std::string_view EPOCH_DELIVERIES_SEED = "epoch_deliveries";
constexpr std::string_view ENVELOPE_CHUNKS_SEED = "envelope_chunks";— with a comment stating they are byte-exact mirrors that must agree or every seeds-validated call fails. The new seeds are the same kind of constant with the same consequence, spelled the unsearchable way, 570 lines below the pattern. There are now five inline seed literals in this plugin (reserve, reserve_vault, the two new ones, and reserve_aggregate in outpost_solana_client_plugin.hpp:205). CLAUDE.md's "no magic literals" aside, a one-character typo here derives a well-formed wrong PDA that only fails at runtime as EffectAccountMissing.
4. token_custody_info::decimals is always zero, and nothing needs it
collateral_position_custody hardcodes 0 (:1404) — correctly, since CollateralPosition carries no decimals — and the header documents the field as "Chain-native decimals when a caller needs them". No caller needs them. A future one that reads .decimals gets 0 silently, which is wrong for every SPL token, and the sibling reserve_terminal_info in the same header carries a real custody_decimals alongside a custody_mint, so the two structs are one rename away from being confused. Either drop the field (leaving a struct that is just the mint) or make it std::optional<uint8_t> so an unset value cannot be read as a valid one.
5. assert_collateral_position_shape asserts four fields; the relay reads one
assert_reserve_shape asserts exactly the three fields the manifest resolves. This one requires operator, token_code and amount in addition to custody_mint, none of which the relay decodes, so a benign program-side rename of amount becomes a hard boot failure for the batch-operator role. If the extra three are a deliberate drift canary — which the header's "the four fields the on-chain settlement path binds together" implies — say so in one line; otherwise narrow it to what is read.
6. reserve_pda's doc names the wrong seed
outpost_solana_client_plugin.hpp:119 says "Pre-derived from seed outpost_reserve"; the derivation twenty lines down (:205) uses reserve_aggregate, and a comment at :199 explains why. Pre-existing, but this PR is the first consumer where reserve_pda selects an ATA owner, and a wrong owner there yields Ok(None) → "status flipped, funds stuck" rather than a loud abort. Worth correcting the comment while the path is being added.
Also cosmetic: no blank line between derive_collateral_vault_pda and count_inbound_attestations (:629).
Prior findings
| # | Finding | Status |
|---|---|---|
| 1 | Custody read from the mutable OutpostConfig rather than the position's pinned custody_mint |
Fixed, with the regression test |
| 2 | Unachievable "degrade to log-and-skip" fallback | Resolved by #552 |
| 3 | Inverted log-and-skip rationale in source and description | Fixed in both |
| 4 | Nothing creates the reserve_aggregate ATA |
Addressed — SolanaOutpostBootstrapper.ts:399-409 pre-creates it per registered SPL mint, and the program now documents it as an ops precondition. Production provisioning is a runbook item, not a relay concern. The failure mode is still a silent Ok(None) "funds stuck"; an operator alarm on it is worth a follow-up, not this PR |
| 5 | "Never throws" contract catching only fc::exception |
Moot — that function is gone. collateral_position_custody throws by design and mirrors reserve_info_for_codes's single-arm log-and-rethrow exactly; a non-fc throw loses only the diagnostic elog |
| 6 | SPL deposit-revert skip hard-codes today's on-chain refusal | Materialised — see finding 1 |
| 7 | OutpostConfig re-fetched per token code |
Gone — collateral no longer reads the config at all |
| 8 | Duplicate reserve_aggregate derivation |
Fixed |
| 9 | Cross-extractor coupling on the recipient sweep | Fixed |
| 10 | Magic-literal seed strings | Still open — see finding 3 |
| — | Base says master while stacked on #552 |
Resolved |
| — | Manifest derivation untested | Mostly closed — see finding 2 for what remains |
Two things I checked and am not raising as findings
Transaction sizing. Collateral settlements grow a manifest from one account to as many as five, but drive_dispatch_rounds packs greedily against the account union with MAX_TERMINAL_DYNAMIC_ACCOUNTS, always taking at least one attestation, so denser manifests shrink the window rather than overflowing the packet. Round-budget exhaustion resumes from the on-chain cursor on the next tick.
Read volume. One getAccountInfo per distinct (operator, token_code) is inherent to per-position custody and is the correct trade for what finding 1 bought. A 9-operator slash envelope pays nine sequential reads on the deadline-bounded path; the per-effect deadline probe plus #552's resumable drain make that a delay rather than a failure.
There is also a narrow TOCTOU: a position that is absent when the relay reads it and opened with SPL custody before the terminal transaction executes produces an abort. It self-heals, because the next tick re-reads and declares the extras — worth knowing when reading a one-off EffectAccountMissing in a log, not worth defending against.
Verdict: the custody rework, the test extraction and the description rewrite all landed well, and the inventory is right on five of six branches. Finding 1 blocks: the companion implemented SPL DEPOSIT_REVERT in the same window this branch was rebased, the relay still declares the manifest for a handler that refuses, and the outcome is a held cursor with no retry path. It is a small change here plus the manifest test in finding 2, and it wants a paired e2e — with no flow covering DEPOSIT_REVERT in either custody mode, this specific regression is invisible to the gate that just went 14/14.
d1622e0 to
61d273b
Compare
|
Re-review findings addressed at 1 — SPL 2 — SLASH / DEPOSIT_REVERT untested at the manifest level: two new
3 — seed strings: 4 — 5 — 6 — Validation: (The prior head's Apple Silicon failure was |
heifner
left a comment
There was a problem hiding this comment.
Approving at 61d273b1c9.
Finding 1 is resolved at the source rather than patched around: all three collateral shapes now run one uniform path, so no per-shape early return is left that can go stale against the program again. I re-traced every branch against wire-solana#409 at 70f229ed — all six match the handlers now, including the five-account SPL DEPOSIT_REVERT manifest (resolve_collateral_vault_transfer at inbound.rs:1565, plus the depositor wallet the full-drain close resolves leniently).
The two new manifest cases pin the right properties: the SLASH owner swap is asserted both ways, and the deposit-revert case requires five accounts where the pre-fix early return produced two — so it discriminates the regression rather than just describing it. Findings 3–6 all addressed.
One non-blocking leftover: outpost_solana_client.hpp:589-592, the effect_shape::deposit_revert enum doc, still says the SPL branch is "REFUSED on-chain by design … so this shape never carries SPL extras." Both clauses are now false and the shape carries exactly those extras. It is the last copy of the premise that produced the finding, sitting where a reader looks up what the shape means — worth deleting on the way in.
Land it once CI is green here and the paired e2e comes back. Standing caveat on that gate: no flow exercises DEPOSIT_REVERT in either custody mode, so the new unit case is what holds that line.
The SOL outpost relay builds the terminal remaining-accounts manifest for every dispatch_attestations call. The collateral-settling attestations — OPERATOR_ACTION(WITHDRAW_REMIT / SLASH / DEPOSIT_REVERT) — now settle against per-(operator, token_code) CollateralPosition PDAs (SOL-379) in the asset each position escrows (SOL-380), so the manifest must declare the accounts those on-chain handlers resolve: the CollateralPosition PDA, and for SPL custody the collateral_vault PDA, the destination ATA, and the SPL token program. Custody is resolved per position from the CollateralPosition account itself, keyed by (operator, token_code) — never from the mutable OutpostConfig token maps. The on-chain handlers branch on the custody_mint pinned into each position at first deposit; resolving it from config instead lets one token_code back positions of disagreeing custody, or an admin re-point between deposit and settlement, ship a manifest missing the SPL accounts. That aborts the consensus-pinned dispatch on-chain and wedges the epoch for every relay at once. This mirrors the reserve path, which already reads custody off the Reserve account: get_account_info sits outside the decode try so an RPC or deadline error propagates, an absent position degrades to a partial manifest, and a present-but-unreadable one fails the tick loudly with the cursor untouched. A boot-time assert_collateral_position_shape validates the CollateralPosition IDL declaration under the batch-operator role gate, so a drifted layout fails at construction rather than silently misreading a live position's custody on the first drain. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VqAoaLdCBqgV2vXgDCGhgY Change-Id: I4e6f7c2a0a079a4160155a7da8d0bd43e5d67a1a
61d273b to
7b16ace
Compare
Important
Part of a three-repo set — all must merge together:
CollateralPositionPDA, asset-identity enforcement)dispatch_attestationsmatchesremaining_accountsby pubkey. If the program landswithout this relay change, every collateral-settling handler aborts for want of an
account the relay never declared — and because the envelope bytes are consensus-pinned,
that abort recurs on every retry and wedges the epoch.
Relay side of the SOL-375/379/380 collateral work.
wire-solana #409 replaced the bounded collateral
Vecwith a per-(operator, token_code)CollateralPositionPDA and made the terminal handlers settle in the asset actuallyescrowed. Both mean the zero-data terminal
dispatch_attestationscall must declareaccounts it never carried before, for every
WITHDRAW_REMIT/SLASH/DEPOSIT_REVERT.What this does
build_dispatch_manifeststo declare each collateral-settling attestation'saccounts: the
CollateralPositionPDA, and for SPL custody thecollateral_vaultPDA,the destination ATA, and the SPL token program.
SLASHis surfaced deliberately (adropped seizure has no return attestation to re-drive it), settling into the
reserve_aggregate's ATA;WITHDRAW_REMITinto the operator's; the SPLDEPOSIT_REVERTrefunds the depositor's ATA.
collateral_positionandcollateral_vault, mirroringopp_states.rs.Custody comes from the position, never the config
The on-chain handlers branch on the
custody_mintpinned into eachCollateralPositionat first deposit. The relay resolves custody the same way — reading it off the
CollateralPositionaccount, keyed by(operator, token_code)— never from the mutableOutpostConfigtoken maps. Resolving from config would let onetoken_codeback positionsof disagreeing custody (operator A native, operator B SPL), or an admin re-point between
deposit and settlement, ship a manifest missing the SPL accounts → a permanent, correlated
epoch wedge. This mirrors the reserve path exactly:
get_account_infosits outside thedecode
tryso an RPC/deadline error propagates; an absent position degrades to a partialmanifest; a present-but-unreadable one fails the tick loudly with the cursor untouched.
A boot-time
assert_collateral_position_shape, under the batch-operator role gate, failsloudly on a drifted
CollateralPositionIDL rather than silently misreading a liveposition's custody on the first drain.
Verification
test_outpost_solana_client_plugin— green (includes a regression test proving twooperators sharing one
token_codewith disagreeing custody get different manifests).plugin_testbuilds clean.origin/master; findings addressed.🤖 Generated with Claude Code
https://claude.ai/code/session_01VqAoaLdCBqgV2vXgDCGhgY