feat(mpp/session)!: final session wire contract + open context — Rust, TypeScript, Python (Go/Kotlin/Swift follow-up) - #259
Conversation
Add voucher-signer negotiation, reusable payer proofs, operator-signed use actions, and negotiated idle timeouts across the Rust, TypeScript, and Python SDKs. BREAKING CHANGE: replace settlementAuthority and its clientVoucher/delegated values with voucherSigner and client/operator.
lgalabru
changed the title
lgalabru
changed the title
Greptile SummaryFinalizes the session wire contract and lifecycle behavior across Rust, TypeScript, and Python.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported expiry and lifecycle defects are resolved in the current code. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Server
participant Store
participant Lifecycle
Client->>Server: Open with challenged context
Server->>Server: Validate expiry, transaction, and negotiated timeout
Server->>Store: Persist channel and timeout
Server->>Lifecycle: Arm channel deadline
Client->>Server: Use, voucher, top-up, or commit
Server->>Store: Atomically update watermark and activity
Server->>Lifecycle: Refresh or recheck deadline
Lifecycle->>Store: Re-read durable activity at fire time
alt Channel remains active
Lifecycle->>Lifecycle: Rearm deadline
else Channel is idle
Lifecycle->>Server: Request close and settlement
end
Reviews (6): Last reviewed commit: "style(mpp): rustfmt pass on the retry-id..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Reject expired reusable proofs in Rust and Python, honor negotiated TypeScript idle timeouts, and refresh activity only for accepted operations. Also align the new TypeScript fields with repository lint ordering.
Remove the non-spec authenticationExpires field across Rust, TypeScript, and Python. Enforce the standard challenge expiry only when opening a channel, while allowing bound channel activity until idle timeout or closure.
Require Rust and Python open handlers to receive the standard opening challenge, reject expired challenges, and match reusable proofs to the challenge ID. Chunk long TypeScript idle timers so the full negotiated range remains schedulable.
Mirror the reworked session protocol from the Rust spine to TypeScript and Python, and the wire shapes to Go/Kotlin/Swift. Add the mpp-specs e702dd8 open-transaction context: new-channel challenges carry recentBlockhash plus recentSlot (decimal-string u64, both from one getLatestBlockhash; the server fails the challenge instead of degrading, and both stay absent on resume), clients default openSlot to the challenged recentSlot (earlier override allowed, later rejected) and compile the open transaction against the challenged blockhash, and open verification enforces openSlot <= recentSlot within the 1500-slot window plus the challenged-blockhash binding before broadcast. SessionServer gains with_blockhash_cache (the per-challenge blocking get_slot fetch is gone), and the Python wire parser now rejects requests missing required fields instead of defaulting them to empty strings. BREAKING CHANGE: the open payload slot echo is renamed to openSlot and the multi-delegate open fields (initMultiDelegateTx, updateDelegationTx) are removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add a sessionWire canonical-bytes mode that round-trips challenge requests and credential actions through each SDK's production wire parser, JCS-canonicalizes, and compares byte-for-byte. TypeScript and Python implement it; Go/Kotlin/Swift declare unsupported-mode skips. Six frozen vectors: a new-channel challenge (recentBlockhash + recentSlot present), a resume challenge (both absent), the open payload (openSlot), and rejects for the pre-e702dd8 draft request shape, the draft recentSlot echo in the open payload, and an unknown action tag. The draft-request reject immediately caught the Python parser accepting the superseded shape (fixed in the previous commit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@greptile-apps review please |
…thon The open must encode the server's challenged splits, not merely splits that are self-consistent with its own transaction: the open commits the on-chain distributionHash while distribute at settle is built from the server's config, so a client-substituted list (a) steals platform-fee co-recipient shares and (b) makes the bundled settleAndSeal+distribute revert atomically, stranding vouchers until the client force-closes and reclaims the full deposit. Rust already enforced this (payload_splits != config.splits); TypeScript and Python only checked payload-transaction self-consistency. TypeScript: VerifyOpenTxExpected gains a required splits field (the challenged distribution splits) and verifyOpenTx rejects instruction recipients that differ from it; both handleOpen paths plumb the configured splits through. Python: _process_open_locked rejects payload.distribution_splits != config.splits, mirroring Rust. Regression tests in both SDKs: drop-splits, redirect-share, inflate-share, and unsolicited-splits opens are rejected; the exact challenged splits are accepted. BREAKING CHANGE: VerifyOpenTxExpected.splits is required; callers of verifyOpenTx/submitOpenTx must pass the challenged splits. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…API, mppx key length Go conformance: the seeded vector test now recognizes unsupported-mode outcomes, but only for an explicit allowlist of expected-skip vector IDs (the six session-wire-* names, pending the Go wire migration). Any unsupported-mode outcome outside the allowlist fails loudly, and a leftover-allowlist check errors once the vectors stop skipping, so the migration PR is forced to delete the allowlist rather than leave a silent escape hatch. Python harness: both session legs were written against the draft session API and died before readiness. The server adapter now composes the lower-level SessionServer with accept-all wire verifiers (preserving the draft-era rpc=None wire-level trust model; this scenario's surfnet runs no payment-channels program) and serves challenge open context through with_blockhash_cache; the client adapter moves from the removed create_server_opened_payment_channel_session_opener to the strict create_payment_channel_session_opener. The challenge now advertises a suggestedDeposit (the final client derives its deposit from the challenge and refuses one without it). Integration tests: mppx 0.8.15 enforces >=32-byte secret keys; pad the four short test secrets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Restore the behavioral coverage the session rework's test sweep deleted while its surface survived, ported to the final e702dd8 contract: - session-settlement: server money paths — topUp update and reject paths, close monotonicity, idempotent close replay, failed-settlement retry, and first-ever tests for the operator-vs-client close authorization block. - session-verify-open-tx: verifyOpenTx hardening beyond blockhash/slot — address-lookup-table rejection, payee/mint/rentPayer/fee-payer/ authorized-signer mismatches, missing payer signature, minimumDeposit, malformed encodings, waitForSignatureConfirmation semantics, and the submitOpenTx broadcast/confirm/re-verify path. - session-onchain-builders: byte-golden coverage for the settleAndSeal, topUp, distribute, and reclaim builders (accounts, roles, exact data) plus the client open instruction (openSlot encoding, PDA seed set, account ordering, Token-2022 derivation) and the Ed25519 precompile layout. - challenge-selection-session: the session leg of client challenge selection against the final wire shape (guards, network/currency filters, resume challenges, WWW-Authenticate parsing). - session-usage-meter, session-client-fetch: watermark, throttled and trailing commits, rollback, and price validation through the reworked session fetch client. Cases tied to genuinely removed surface (multiDelegate, modes, nonce and draft wire aliases) were dropped rather than ported. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The reject vectors claimed every SDK's wire parser must refuse the superseded shapes, which is untrue until Go/Kotlin/Swift migrate off the draft wire contract. Say "a migrated SDK" instead, and note on each vector that the three pending SDKs declare allowlisted expected skips enforced by their seeded conformance tests until the follow-up migration PR lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lgalabru
changed the title
CI's format:check step is prettier, separate from eslint; two of the new test files missed it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6f33059 to
2daa450
Compare
process_commit advanced the voucher watermark but never refreshed last_activity_at (Rust also: lifecycle.close_after), so a channel paying exclusively through the metered-delivery flow looked idle to the host's lifecycle worker and got idle-closed mid-use — the root cause of the failure class pay#416's lease heartbeat patches from the pay side. - rust: the committed path now re-arms last_activity_at + lifecycle exactly like the voucher/use/top-up paths; idempotent replays still leave both untouched (matching use-replay semantics). - python: the commit mutator now refreshes the durable last_activity_at the idle-close recheck and post-restart reconcile read. The route-level timer touch alone only delayed the fire; the fire-time recheck still saw the stale watermark and closed. - typescript: already correct (processCommit refreshes lastActivityAt and the lifecycle recheck is covered by session-lifecycle.test.ts); no change. - go/kotlin/swift: no session activity tracking yet — these semantics land with the wire-migration follow-up, and the spec now spells them out. Deliberately defense-in-depth with pay#416's lease heartbeat, not a replacement for it: the heartbeat covers commit-less activity (a delegated lease held open with no payments flowing), which this spine fix structurally cannot see. Both activity signals belong; do not "simplify" either away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
I'm on it ! |
EfeDurmaz16
left a comment
EfeDurmaz16
left a comment
There was a problem hiding this comment.
Solid migration overall, and both the splits challenge binding and the commit watermark fix check out line by line. I hit five money-path issues though, one in Rust, one in TypeScript, three in Python; details inline. Also flagged inline as non-blocking: the operator-mode opener cannot yet produce vouchers the server accepts in either the TS or Rust client. Verified by reading the full changed files at a569ac5 in a worktree and tracing each finding to the code; CI is green.
| } | ||
|
|
||
| verify_submit_and_fetch_open( |
There was a problem hiding this comment.
This broadcast runs before the store's atomic replay check, so a client retry after a lost response fails at preflight with AlreadyProcessed instead of returning the documented idempotent replay; the store's replay branch is unreachable against a real RPC, and the deposit sits locked on chain until a force-close. The pre-PR get_signature_status check was naturally idempotent, and TS and Python only broadcast on the fresh-create branch, so this is also a spine parity break. process_topup has the same shape. Short-circuiting to the replay path before broadcasting, or treating AlreadyProcessed plus a matching confirmed channel account as success, would restore the contract. The replay test only passes because the mock RPC accepts repeated sends.
There was a problem hiding this comment.
One sharpening note on the fix direction: reordering alone (replay check before broadcast) does not cover the hardest case, a broadcast that lands followed by a store write failure. No state exists then, so the retry still dies at AlreadyProcessed before any replay check can help. That case only recovers if AlreadyProcessed plus a matching confirmed channel account is treated as success, so I would treat that as the primary fix and the reordering as the optimization.
There was a problem hiding this comment.
Fixed in dd545d2, taking your refinement as the primary fix: the broadcast result is no longer authoritative. On a broadcast rejection, open falls through to the existing get_account + field match against the verified params, and top-up checks get_signature_status for the transaction's own signature — a landed non-error status is treated as confirmed. That covers the plain retry and the hardest case you called out (broadcast landed, store write lost, so no state exists for the replay branch); the reorder alone indeed couldn't.
The test-side gap is closed too: the mock RPC now rejects a duplicate sendTransaction at preflight with AlreadyProcessed and reports getSignatureStatuses only for landed signatures. With that mock and the fix reverted, confirmed_open_is_persisted_and_replays_without_reset, the new retried_open_survives_preflight_rejection_without_stored_state (empty store + landed broadcast), and the extended top-up test are all red — your exact scenario — and green with the fix. You were right that the old replay test only passed because the mock was friendlier than mainnet.
| throw new Error(`newDeposit ${newDeposit} must exceed current deposit ${current.deposit}`); | ||
| } | ||
| return { ...current, deposit: newDeposit }; | ||
| return { ...current, deposit: current.deposit + additionalAmount, lastActivityAt: Date.now() }; |
There was a problem hiding this comment.
handleTopUp has no idempotency: this mutator unconditionally adds additionalAmount, nothing dedupes by transaction signature, and the Rust post-confirm on-chain deposit re-check (verify_submit_and_fetch_topup) was not ported. Two concurrent submissions of the same signed top-up both pass preflight while the tx is in flight, both confirm the same signature, and both increment, so the stored deposit rises twice while escrow rose once and vouchers get accepted beyond what settle can collect. Recording processed top-up signatures on ChannelState and re-checking the on-chain deposit before incrementing would close both holes.
There was a problem hiding this comment.
Fixed in 34f293f, and your fix direction became the canonical one in all three SDKs rather than a TS patch: ChannelState records processed top-up signatures and the check-and-record lives inside the atomic mutator, so only the first submission credits. Rust got the same in-mutator dedupe in dd545d2 (its pre-CAS on-chain re-check ran on a stale read — two in-flight duplicates of the same signature could both pass it), Python in 8a94c02. The schema addition rides the round-trip/schema_version contract from 3524a11, so old writers preserve it.
Also ported the Rust post-confirm deposit re-check you noted was missing: submitTopUpTx now fetches the confirmed channel account and requires it to be open and reflect the recorded deposit plus the top-up before the cap is raised. New tests: a resubmitted top-up is answered from the recorded signature without re-broadcast, and two concurrent duplicates (Promise.all) credit exactly once — both red with the dedupe disabled.
|
|
||
| try: | ||
| await self._core.store().update_channel(channel_id, mutator) | ||
| await self._core.process_close(payload) |
There was a problem hiding this comment.
The docstring still promises a re-drivable close, but the call now delegates to core process_close, whose mutator hard-rejects any close once close_requested_at is set; the old inline mutator's settled_signature-is-None re-drive branch was dropped in this PR. The idle watchdog does not recover it either: _close_on_idle only settles closes it flipped itself. So a transient settle failure leaves the channel permanently close-pending with accepted vouchers unsettled, and the payer reclaims the full deposit after the grace period. Restoring the re-drive semantics (in core or here) fixes it.
There was a problem hiding this comment.
Small wording correction to my scenario: the payer's deposit recovery after the stranded close runs through the on-chain request_close, seal and distribute path once the grace period passes, not through reclaim; reclaim only recovers the channel rent. The merchant-side outcome is unchanged, the accepted vouchers never settle.
There was a problem hiding this comment.
Fixed in 5fb4836, both halves. The re-drive carve-out moved into core process_close where it belongs (the method docstring now just points at it): close-pending with no settlement signature re-drives a matching close idempotently — the retry must replay the recorded final voucher and the original close timestamp is preserved — and hard-rejects once a signature is recorded. The merge-base's drop-through-refactor tests are restored in that shape.
_lifecycle.remove_channel now runs after the settle attempt, not before. That alone would have been cosmetic though: _close_on_idle bailed on any close-pending channel and _reconcile_lifecycle skipped them at boot, so nothing would ever have retried. The watchdog now re-drives a stranded settle (close-pending, no signature), re-arms its timer after a failed settle, and reconciliation restores timers for stranded channels across restarts. End-to-end test drives a close whose settle raises, asserts the watchdog is not released and the channel stays re-drivable, then retries the same payload to a successful settle.
| payer_address = state.payer | ||
| if not payer_address: | ||
| raise PaymentError( | ||
| f"channel {state.channel_id} payer is unknown; cannot derive the refund account", code="invalid-config" |
There was a problem hiding this comment.
Just below this block, distribute is built with rent_payer=config.operator, but open now pins the channel rentPayer to fee_payer_key when fee_payer is set and to the payer otherwise (session.py:555). In any non-gasless config, or whenever feePayerKey differs from the operator, the on-chain rentPayer check rejects distribute and the bundled settleAndSeal reverts with it, so settle can never land. The stored state.rent_payer is the right source, matching the TS path. The harness e2e only passes because the python server runs gasless with the operator as fee payer.
There was a problem hiding this comment.
Fixed in 8a94c02 — the one-liner you suggested: distribute is built with state.rent_payer (falling back to omitting rent recovery when an old record has none), matching TS. Added the non-gasless test you asked for: a channel whose recorded rent payer is neither the operator nor the payer settles successfully and the built transaction references the recorded rent payer — red with the old config.operator code.
| except Exception as exc: | ||
| raise _wrap("open tx verification failed", exc) from exc | ||
|
|
||
| effective_idle_timeout = resolve_idle_timeout_seconds( |
There was a problem hiding this comment.
resolve_idle_timeout_seconds runs after verify_open_tx has already broadcast and confirmed the funding transaction, and it raises for a selection the server did not advertise, so an unsupported idleTimeoutSeconds locks the deposit on chain and then fails the open on every retry. Rust resolves this before the broadcast (session.rs:608) and so does TS (Session.ts:573); moving this above the verify_open_tx call restores fail-safe ordering.
There was a problem hiding this comment.
Fixed in 8a94c02: resolve_idle_timeout_seconds now runs before verify_open_tx, matching the Rust/TS ordering. Test asserts an unsupported selection rejects with zero verifier invocations, i.e. before anything could broadcast — red against the old ordering.
| const sessionSigner = parameters.sessionSigner ?? (await generateKeyPairSigner()); | ||
| const voucherSigner = challenge.request.methodDetails.voucherSigner ?? 'client'; | ||
| const sessionSigner = | ||
| voucherSigner === 'operator' |
There was a problem hiding this comment.
Non-blocking, tracking here: in operator mode the ActiveSession signer is the payer while authorizedSigner is the operator, and SessionFetch always signs commit vouchers with the session signer (SessionFetch.ts:395), so the server rejects every voucher this opener produces; the Rust client opener has the same gap (client/session.rs:611). No fund risk since everything is rejected, but the operator flow is not usable end to end from these clients yet. Fine as a follow-up alongside the Go/Kotlin/Swift migration if you prefer.
There was a problem hiding this comment.
Agreed and tracking, not fixed here: this folds into the client-side work item from the incident thread — returning the opening proof on the session handle plus the use-after-rotation conformance vector across the five SDKs — so the operator-mode opener gets fixed once with its conformance coverage rather than patched per SDK. Will reference this comment when that lands.
…rows Rolling deploys leave the previous revision (and any service pinned to an older PayKit) sweeping the shared channel store for minutes to indefinitely. An old writer decodes rows with serde defaults for fields it does not know, re-encodes, and CAS-writes — stripping newer fields such as the session proof binding off every row it reconciles (2026-08-01 modelstudio incident). - Persisted ChannelState now carries a flattened `extra` map so unknown fields round-trip verbatim through read-modify-write cycles, plus a `schema_version` stamped on every write; durable stores refuse records stamped newer than the running writer instead of decoding them lossily. - verify paths split the failure: a record whose binding fields are absent (pre-binding or wiped) now fails "session channel predates proof binding" instead of sharing "does not match the proof bound at open" with genuine mismatches, so logs name the mechanism directly. Same fail-closed outcome. - TS/Python mirror both changes; TS has no in-SDK codec, so its store contract documents the round-trip/refuse-newer duty for implementors. Go/Kotlin/Swift pick these up with the pending session wire migration.
A client retry of an open or top-up whose first submission landed dies at preflight with AlreadyProcessed, so the replay branch was unreachable against any real RPC (the test mock tolerated duplicate sends). - open/top-up broadcast rejection is no longer authoritative: the confirmed channel account matching the verified params is accepted as success, which also covers a lost store write after a landed broadcast - top-ups record processed transaction signatures on ChannelState and dedupe inside the atomic mutator, so a resubmitted or concurrently duplicated top-up credits the deposit exactly once - the mock RPC now rejects duplicate sendTransaction at preflight and reports signature statuses only for landed transactions, like mainnet
handleTopUp raised the deposit unconditionally: a client retry of a top-up whose response was lost (or two in-flight duplicates of the same signed transaction) credited the deposit twice while escrow was funded once. - ChannelState records processedTopUpSignatures; the atomic mutator checks-and-records so only the first submission credits (mirrors the Rust canonical fix, survives old writers per the schema round-trip contract) - a replayed top-up is answered from the recorded signature without re-broadcasting; a preflight-rejected duplicate whose signature landed is treated as confirmed (mirrors Rust open/top-up retry idempotency) - ported the Rust post-confirm deposit re-check: the confirmed channel account must reflect the recorded deposit plus the top-up before the cap is raised
The refactor that delegated the HTTP close to core process_close dropped the settled_signature-is-None re-drive carve-out the docstring still promised: a transient settle failure left the channel close-pending forever and the merchant's accepted vouchers never settled. - core process_close re-drives a matching close while no settlement signature is recorded (the re-drive must replay the recorded final voucher and preserves the original close timestamp); once a signature exists a second close hard-rejects as before - the idle watchdog is released only after the settle attempt, not before it: after a failed settle it is the only actor left that can retry, so _close_on_idle now re-drives stranded settles, re-arms its timer after a failure, and lifecycle reconciliation restores timers for close-pending channels with no settlement signature
…t before broadcast, credit top-ups once Three money-path fixes in the Python session server: - distribute was built with rent_payer=config.operator, but the on-chain check pins to the account recorded at open (fee payer, or the payer in non-gasless configs) — any non-gasless settle reverted forever; use state.rent_payer, as TS does - resolve_idle_timeout_seconds ran after verify_open_tx had broadcast and confirmed the funding transaction, so an unsupported selection locked the deposit in escrow with every retry failing; resolve before the verifier, mirroring the Rust and TS ordering - top-ups now record processed transaction signatures on ChannelState and dedupe inside the atomic mutator (plus an idempotent replay pre-check), mirroring the canonical Rust/TS exactly-once fix
solana-pay-kit (Rust) 0.4.0 -> 0.5.0, @solana/mpp and @solana/pay-kit (TS) 0.7.0 -> 0.8.0, solana-pay-kit (Python) 0.5.0 -> 0.6.0. New versions publish when #259 merges.
solana-pay-kit (Rust) 0.4.0 -> 0.5.0, @solana/mpp and @solana/pay-kit (TS) 0.7.0 -> 0.8.0, solana-pay-kit (Python) 0.5.0 -> 0.6.0. New versions publish when #259 merges.
|
@greptile-apps can I have a review ? |
There was a problem hiding this comment.
Five previous blockers verified fixed at 2fad1c6, and the schema-version store hardening is a good addition. Two items block this round, both Python retry recovery on the money path and both converged on by every independent review pass: the open verifier (session_onchain.py:499) and the top-up verifier (:581) still treat a broadcast rejection as authoritative, the exact failure class dd545d2 fixed in Rust; TS open shares it. Everything else inline is tracked follow-up material, including the _AlreadySettling timer drop and the voucher-wire spec decisions.
| signature = await cosign_and_broadcast_open(payload, fee_payer=fee_payer_signer, rpc=rpc_client) | ||
| else: | ||
| raw = base64.b64decode(payload.transaction, validate=True) | ||
| sent = await rpc_client.send_raw_transaction(raw) |
There was a problem hiding this comment.
Blocking: the open verifier treats a broadcast rejection as authoritative, so a landed open whose confirm timed out or whose persist was lost dies at preflight with AlreadyProcessed on every retry, deposit escrowed with no server record. Rust got the confirmed-account fallback in this range; TS submitOpenTx (on-chain.ts:916) shares the naked send. Fix: port the Rust fallback to Python and TS, same shape as dd545d2.
There was a problem hiding this comment.
Fixed in 08c77fb4 (Python) and d1f2beed (TS submitOpenTx — good catch that it shared the naked send).
Python: the broadcast error is captured on both the gasless and direct paths and _verify_channel_account still runs against the verified params — a full field match is success, anything else re-raises the original error. TS: the existing post-confirm field match now arbitrates the send/confirm failure the same way, with the transaction's own signature as the result. Both mirror dd545d2d's shape: the confirmed account matches the verified open params only if this exact open succeeded.
Red-tested per your template: the Python mock RPC now rejects duplicate sends like mainnet and the no-stored-state retry test fails with the fix reverted (test_open_verifier_rescues_landed_open_on_duplicate_preflight_rejection); TS equivalent in session-verify-open-tx.test.ts. Negative cases pin that a non-matching (or missing) confirmed account keeps the broadcast failure authoritative.
| payer = Pubkey.from_string(state.payer) | ||
| if payer_index >= len(signatures) or not signatures[payer_index].verify(payer, _signed_message_bytes(message)): | ||
| raise PaymentError("top-up payer signature is invalid", code="invalid-payload") | ||
| sent = await rpc_client.send_raw_transaction(raw) |
There was a problem hiding this comment.
Blocking, same class as the open verifier above: no landed-signature rescue, and the processed_topup_signatures dedupe only covers persisted credits, so a landed top-up whose confirm or persist was lost is uncreditable for the channel's lifetime. Fix: port the Rust/TS signature-status rescue.
There was a problem hiding this comment.
Fixed in 08c77fb4, mirroring the TS 34f293f2 rescue exactly: on send failure the transaction's own signature status arbitrates — landed clean continues to confirmation and the post-confirm deposit re-check (which, with the credit lost, still expects state.deposit + amount), while a landed-but-FAILED or unknown signature re-raises the original broadcast error.
Red-tested: test_top_up_verifier_rescues_landed_top_up_on_duplicate_preflight_rejection fails on the pre-fix code (the mock rejects duplicate sends like mainnet), and the two negative tests pin the strictness (err set / no status → original error).
| return voucher_message_bytes( | ||
| channel, | ||
| cumulative, | ||
| self.expires_at if self.expires_at is not None else DEFAULT_SESSION_EXPIRES_AT, |
There was a problem hiding this comment.
Correcting myself: not a pay-kit cross-SDK break, all three clients sign and send the same 4102444800 default. The residual is verify-side only: for a wire voucher omitting expiresAt, Rust and TS reconstruct 0 but this line substitutes 4102444800, so a spec-conforming external no-expiry voucher fails only on Python. Follow-up: encode the wire value verbatim, 0 for omitted.
There was a problem hiding this comment.
Agreed with your correction — verify-side only, since all three shipped clients emit the explicit 4102444800 default; the exposure was a spec-conforming external client omitting expiresAt. Fixed in dc30ef65: Python now encodes an omitted expiresAt verbatim as 0 into the 50-byte preimage, the highest_voucher_expires_at watermark records 0 so settle replays exactly the bytes the signature covers, and the VoucherData docstring documents the correct behavior. The remaining DEFAULT_SESSION_EXPIRES_AT uses are explicit signed wire values (operator-use path) or internal delivery-directive expiries — audited, none leak into reconstructed signed bytes.
Frozen cross-SDK: session-voucher-preimage-no-expiry pins the exact 50 bytes (expiresAt omitted → trailing zero i64) and runs in the TS/Python/Go/Kotlin/Swift runners, driving each SDK's own never-expires default.
| transaction: z.string(), | ||
| }), | ||
| z.object({ | ||
| action: z.literal('voucher'), |
There was a problem hiding this comment.
Spec says the voucher action carries a top-level channelId and the signed voucher's inner key is voucher; all three SDKs use no top-level channelId and inner key data (close nests the same object). Mutually consistent so not a blocker, but it needs a decision: align the SDKs or pin the spec to the implemented shape, and freeze a voucher-action vector either way so it cannot drift again.
There was a problem hiding this comment.
Ludovic ruled: follow the spec. Done in 7085443f across all three SDKs — inner key data → voucher (voucher action and the voucher nested in close), plus the REQUIRED top-level channelId routing key, emitted and parsed. Servers reject a voucher action whose top-level channelId diverges from the signed voucher's inner one, and a close whose nested final voucher is bound to another channel — red-tested in each SDK (the check removed → exactly the new reject tests fail).
Frozen so it cannot drift again: session-wire-action-voucher-frozen and session-wire-action-close-frozen (the close vector also freezes the omitted-expiresAt form — no key on the wire), plus session-wire-action-voucher-legacy-inner-data-reject pinning that the superseded inner-data shape now fails parse. Go's seeded skip allowlist carries the three new ids until its wire migration, which inherits this shape (noted in the PR body).
| if remaining is not None and self._lifecycle is not None: | ||
| self._lifecycle.touch(channel_id, remaining) | ||
| if due: | ||
| await self._settle_channel(channel_id) |
There was a problem hiding this comment.
Two interleavings drop the watchdog timer via the _AlreadySettling None: an idle fire during a client settle, and the losing side of two racing client closes (which also removes the timer and returns success before the winner's broadcast resolves). The channel stays close-pending but re-drivable via retry or restart, so follow-up. Fix shape: only remove the timer once a signature is recorded or the channel is sealed, and re-touch with the retry delay when a settle resolves to None while still close-pending with signer/RPC configured.
There was a problem hiding this comment.
Agreed, both interleavings are real and the channel stays re-drivable, so tracking as a follow-up (added to the PR-body list). Your fix shape matches what I'd do: timer removal only once a signature is recorded or the channel is sealed, and a retry-delay re-touch when a settle resolves None while still close-pending with signer/RPC configured.
| /// read-modify-write by an older writer can never strip a newer schema's | ||
| /// fields off a shared record (the 2026-08-01 proof-binding wipe). | ||
| #[serde(flatten)] | ||
| pub extra: serde_json::Map<String, serde_json::Value>, |
There was a problem hiding this comment.
The flattened extra map can round-trip a foreign field whose serialized bytes contain "sealed":true, false-positiving the mark_sealed_atomic/mark_finalized_atomic substring guards. Follow-up: JSON-aware check, or namespace extra under a single key.
There was a problem hiding this comment.
Agreed — the substring guard was always a pragmatic shortcut and the flattened extra round-trip gives it a real false-positive path. Tracked as a follow-up (PR-body list); leaning toward the JSON-aware check over namespacing extra, since namespacing would break the old-writer round-trip contract we just shipped.
| let channel = | ||
| payment_channels::generated::generated::accounts::Channel::from_bytes(&account.data) | ||
| .map_err(|error| Error::Other(format!("decode confirmed channel: {error}")))?; | ||
| if channel.status != 0 |
There was a problem hiding this comment.
fetch_and_match_open_channel does not require settlement == 0, so the broadcast-error adoption arm could adopt a channel with a nonzero on-chain settled watermark while fresh_state seeds zero. Hard to reach; adding the check makes adoption airtight.
There was a problem hiding this comment.
Agreed — hard to reach (it needs a settled watermark on a channel the store has never seen), but settlement == 0 makes adoption airtight and is one line. Tracked as a follow-up (PR-body list).
| return newState; | ||
| }); | ||
| args.lifecycle?.touch(channelId); | ||
| args.lifecycle?.touch(verified.channelId, persisted.idleTimeoutSeconds); | ||
|
|
||
| return Receipt.from({ |
There was a problem hiding this comment.
Spec marks intent, acceptedCumulative, spent, and idleTimeoutSeconds REQUIRED on session receipts; no SDK emits any of them, so clients cannot recover the accepted watermark from receipts. Additive follow-up across the three SDKs.
There was a problem hiding this comment.
Verified against §Receipt-Format — real, and purely additive since nothing parses receipts strictly yet. Tracked as a follow-up (PR-body list) across the three SDKs rather than folded in: it touches every receipt constructor and deserves its own vector freeze.
| `open openSlot ${openSlot.toString()} is ahead of the challenged recentSlot ${recentSlot.toString()}`, | ||
| ); | ||
| } | ||
| if (recentSlot - openSlot > OPEN_SLOT_WINDOW) { |
There was a problem hiding this comment.
Spec open step 7 also wants openSlot checked against the server's current slot, step 2 wants a non-curve authorizedSigner rejected before fees are paid, and open MUST reject a wire bump field; none are implemented in any SDK. The program fails closed, so the exposure is sponsored fees for guaranteed-to-revert opens. Follow-up.
There was a problem hiding this comment.
Agreed on all three (the step-7 current-slot window was already on our open list; the non-curve authorizedSigner reject and the wire bump MUST-reject join it). Exposure is bounded to sponsored fees on guaranteed-to-revert opens since the program fails closed, so tracked as a follow-up (PR-body list).
An omitted expiresAt is never-expires and the spec encodes it verbatim as 0 into the 50-byte voucher preimage, matching Rust (unwrap_or(0)), TS (?? 0), and the on-chain settle check. Python substituted DEFAULT_SESSION_EXPIRES_AT (4102444800), so any cross-SDK no-expiry voucher failed signature verification and Python silently converted never-expires into expires-in-2100 on chain. The highest_voucher_expires_at watermark now records 0 for the omitted form so the on-chain settle replays exactly the bytes the signature covers. The remaining DEFAULT_SESSION_EXPIRES_AT uses are explicit signed wire values (operator-use path) or internal delivery-directive expiries and never leak into reconstructed signed bytes.
…st the chain The open and top-up verifiers treated a broadcast raise as authoritative: a retry of a transaction whose first submission landed (response lost, or the store write after it failed) died at mainnet's duplicate preflight rejection, leaving the deposit escrowed with no server-side record. - open: capture the broadcast error on both the gasless and direct paths and still run the confirmed-account check; a full field match is success, anything else re-raises the original error (mirrors the Rust retry-idempotency fix) - top-up: on send failure, check the transaction's own signature status; landed clean continues to confirmation and the post-confirm deposit re-check, a landed-but-failed or unknown signature keeps the broadcast failure authoritative (mirrors the TS submitTopUpTx rescue) - the mock RPC in the new tests rejects duplicate sends like mainnet, so the regression tests exercise the real preflight path
The three SDKs uniformly drifted from the spec's voucher action shape (mpp-specs e702dd8): the signed voucher's inner data field was emitted as data instead of voucher, and the action carried no top-level channelId. The session-wire freeze covered challenge/open only, which is exactly how this went undetected. - rename the signed voucher's inner wire key data -> voucher (voucher action and the voucher nested in close) in Rust, TS, and Python - the voucher action now carries a REQUIRED top-level channelId routing key; servers reject the action when it differs from the signed voucher's inner channelId, and a close rejects a nested final voucher bound to another channel — the routing key must never diverge from the signed content - freeze voucher-action and close-action vectors in session-wire.json (the close vector also freezes the omitted-expiresAt form), plus a reject vector for the superseded inner-data shape; Go's seeded skip allowlist carries the three new ids until its wire migration - freeze the no-expiry 50-byte voucher preimage (expiresAt omitted -> encoded verbatim as 0) in session-voucher.json; the TS/Python/Kotlin/ Swift runner shims now accept the omitted form and drive their SDKs' never-expires default Folds into the same unpublished 0.5.0/0.8.0/0.6.0 breaking bump as the rest of this PR. The Go/Kotlin/Swift wire migration follow-up inherits this shape.
2fad1c6 to
7085443
Compare
|
Too many files changed for review. ( Bypass the limit by tagging |
…d channel submitOpenTx shared the Python open verifier's naked send: a retry of an open whose first submission landed (response lost, or the persist after it failed) died at mainnet's duplicate preflight rejection with the deposit escrowed and no server-side record. On send/confirm failure the existing post-confirm field match now arbitrates: the confirmed channel account matches the verified open params only if this exact open succeeded, so a full match is success (with the transaction's own signature) and anything else re-raises the original broadcast error — the same shape as the Rust retry-idempotency fix and the Python port in the previous commit.
EfeDurmaz16
left a comment
EfeDurmaz16
left a comment
There was a problem hiding this comment.
Both blockers are properly fixed at d1f2bee, traced in code: the Python open and top-up verifiers now resolve a preflight-rejected retry against the confirmed channel account and the landed signature status, same shape as the Rust fix, and TS open gained the same rescue. The voucher wire now matches the spec in all three SDKs with voucher and close vectors frozen, and omitted expiresAt encodes as 0. Remaining follow-ups are tracked in the inline threads. Verified against the full suite of green checks.
2 participants
Summary
voucherSigner: client | operatoracross Rust, TypeScript, and Pythonuseaction, and operator-signed cumulative vouchersprocess_commitnow refresheslast_activity_at/lifecycle.close_after— the idle-close root fix, folded into this PR instead of a stacked follow-up (details below)Fund-safety fix (from review)
distributionSplitsare now verified against the challenge in TypeScript and Python, mirroring the existing Rust check (payload_splits != config.splitsinserver/session.rs). Previously both SDKs only checked payload↔transaction self-consistency, so a client could open with altered or empty splits: the open commits the on-chaindistributionHashwhiledistributeat settle is built from the server's config, so the substituted list steals platform-fee co-recipient shares and makes the bundled settleAndSeal+distribute revert atomically — stranding vouchers until the client force-closes and reclaims the full deposit.VerifyOpenTxExpectedgains a requiredsplitsfield enforced insideverifyOpenTx; bothhandleOpenpaths plumb the configured splits through._process_open_lockedrejectspayload.distribution_splits != config.splits.Idle-close root fix (added post-approval,
a569ac52)process_commitadvanced the voucher watermark but never refreshed the activity state, so a channel paying exclusively through the metered-delivery flow looked idle to the host's lifecycle worker and was idle-closed mid-use — the spine-side root cause of the failure class pay#416's lease heartbeat patches from the pay side.last_activity_at+lifecycle.close_afterexactly like the voucher/use/top-up paths; idempotent commit replays leave both untouched (matching use-replay semantics). Regression test:commits_refresh_activity_and_lifecycle.last_activity_atread by the idle-close recheck and the post-restart reconcile (the route-level timer touch alone only delayed the fire). Regression test:test_commit_refreshes_activity_watermark_but_replay_does_not.processCommitrefresheslastActivityAtand the fire-time recheck is covered bysession-lifecycle.test.ts. No change.lastActivityAt.Deliberately defense-in-depth with pay#416's lease heartbeat, not a replacement: the heartbeat covers commit-less activity (a delegated lease held open with no payments flowing), which this spine fix structurally cannot see. Both activity signals belong; neither should be "simplified" away later.
Suites re-run at
a569ac52(only Rust/Python code changed): Rustcargo testworkspace — 832 passed, 0 failed (kit lib, +1 new test), all other targets green,cargo fmt --check+ clippy clean (no new warnings); Pythonjust test— 1213 passed, 1 skipped, 0 failed (+1 new test), ruff + pyright clean. TS/Go/Kotlin/Swift/conformance are untouched by this commit; their counts in the table below are from2daa4505.Breaking Changes
settlementAuthorityis removed; usevoucherSignerclientVoucherbecomesclient;delegatedbecomesoperatorcloseDelayMsis removed from the TypeScript server API; useidleTimeoutSecondsVerifyOpenTxExpectedgains a requiredsplitsfield (the challenged distribution splits)Open-transaction context (mpp-specs e702dd8)
channelId) now carryrecentBlockhash+recentSlot(decimal-string u64), both from onegetLatestBlockhash(result.value.blockhash+result.context.slot). The server fails the challenge instead of degrading when the fetch fails; both fields stay absent on resume challenges.openSlot = recentSlot(earlier override allowed, later rejected; clear error when a new-channel challenge lacks either field).openSlot <= recentSlot,recentSlot - openSlot <= OPEN_SLOT_WINDOW(1500), and that the compiled open message uses the challenged blockhash — all before broadcast. Same pattern in the x402uptoadapter viaextra.recentBlockhash/extra.recentSlot.SessionServer::with_blockhash_cache(...)(Rust; mirrored hooks in TS/Python) replaces the per-challenge blockingget_slot()fetch.Golden session wire vectors
harness/vectors/session-wire.json(newsessionWirecanonical-bytes mode): frozen new-channel challenge, resume challenge, and open payload, plus rejects for the pre-e702dd8 draft request, the draftrecentSlotopen-payload echo, and unknown action tags. Honest scope: these are enforced by the TypeScript and Python parsers; Go/Kotlin/Swift declare expected skips pinned to an explicit allowlist in the Go seeded conformance test (any new unexpected skip fails, and a leftover-allowlist check forces the allowlist's deletion once the migration lands). The vector descriptions carry the same note.Notes for the
paybumpBlockhashCachedown viaSessionServer::with_blockhash_cache(...)and drop the field overwrite incore/src/server/session.rs—SessionMppalready holds the cache under the same builder name.Follow-up PR: Go/Kotlin/Swift wire migration (required, tracked)
intents/session.go, KotlinSessionTypes.kt, SwiftSessionTypes.swiftoff the draft wire contract (top-levelcap/operator/programId, draftrecentSlotopen echo) to final e702dd8.encoding/jsonzero-values missing fields, so Go currently accepts both the draft shape and a mis-parsed final shape — an interop hazard, not cosmetic.go/cmd/conformance/main_test.go(its leftover-allowlist check fails loudly until deleted) and the Kotlin/Swift harness-runner skips.7085443f): signed voucher's inner key isvoucher(notdata, in the voucher action and nested in close), the voucher action carries a REQUIRED top-levelchannelIdthe server rejects on mismatch, and an omittedexpiresAtis never-expires encoded verbatim as 0. Also carries: commit-activity semantics,schema_version/extraround-trip,processed_topup_signaturesexactly-once.Review follow-ups (tracked, not folded in)
use/closelack outer-challenge expiry plumbing;process_useidempotency lacks a request fingerprint. (process_commitdoesn't refreshlast_activity_at/lifecycle.close_aftera569ac52, see the idle-close root fix section.)charge.py(empty-string defaults for amount/currency/recipient) and_paycore/solana.py(missing network defaults to mainnet).#assertOpenSlotBoundToChallengefails open on RPC error and Rust upto passeschallenged_slot = Noneon fetch failure — both should fail closed.conformance.test.ts— skips are uncounted, so droppingsessionfrompython.jsonwould green-skip everywhere; the driver needs skip accounting.Subscription.tsstill types/defaultsmainnet-beta; Rust doc comments still advertisetestnet.session.routes()deliveries/commit endpoints and the default-store sharing betweenroutes()andverify()have zero direct tests (new client tests mock the gateway); method-level open-replay semantics (replay preserves watermark, differentauthorizedSigner/sealed rejects, no-rebroadcast) are covered at store level but not credential level; theexpiresAtsafe-integer guard inserver/session/voucher.tsis untested.harness/src/fixtures/typescript/shared.tswill break when the harness workspace bumps its own mppx (pinned 0.5.x) past 0.8; and the python-server accept-all-verifier comment overstates what the session e2e leg proves (transaction bytes are never decoded) — wording fix.recentBlockhash/recentSlotplain-optional — it documents but does not refine the conditional rule (required whenchannelIdabsent, forbidden on resume).From Efe's round-2 pass (2026-08-01, none move money today):
_AlreadySettlingNone; channel stays close-pending but re-drivable. Fix shape: remove the timer only once a signature is recorded or the channel is sealed; re-touch with the retry delay when a settle resolvesNonewhile still close-pending with signer/RPC configured.store.rsmark_sealed_atomic/mark_finalized_atomicsubstring guards on"sealed":truecan false-positive via a foreign field round-tripped through the flattenedextra— needs a JSON-aware check.fetch_and_match_open_channeladoption should requiresettlement == 0so the broadcast-error adoption arm cannot adopt a channel with a nonzero settled watermark whilefresh_stateseeds zero.intent/acceptedCumulative/spent/idleTimeoutSeconds— clients cannot recover the accepted watermark from receipts; deserves its own vector freeze.authorizedSignerreject before fees are paid, and the open wire-bumpMUST-reject are unimplemented everywhere; program fails closed, exposure is sponsored fees on guaranteed-to-revert opens.Test results (full suites, exact counts, tree = 2daa450 (content-identical to e9e2ac0, re-signed))
cargo test -p solana-pay-kit --features server,client --lib/ defaultpnpm test+ typecheck + lintpnpm test:integrationvsharness/start-surfnet-proxy.mjspytest+ ruff + pyrightgo test ./...gradle cleanTest testswift testMPP_CONFORMANCE_LANGUAGES=typescript,python,go,kotlin,swiftsession-basic(python server + python session client)Reinstated TypeScript coverage
session-settlement(20: topUp update/rejects, close monotonicity, idempotent replay, failed-settlement retry, first-ever operator-vs-client close-auth tests),session-verify-open-tx(29: ALT reject, payee/mint/rentPayer mismatches, missing payer signature, confirmation polling, submitOpenTx re-verify),session-onchain-builders(15: byte-golden settleAndSeal/topUp/distribute/reclaim + client open incl. openSlot PDA seed),challenge-selection-session(16),session-usage-meter,session-client-fetch. Cases for genuinely removed surface (multiDelegate, modes, nonce, draft aliases) were dropped, not ported.Round-3 fix batch (Efe re-review, 2026-08-01 — tree
d1f2beed)All four round-2 blockers fixed, each red-tested (fix reverted → exactly the new test fails):
dc30ef65(Py, fund-adjacent): omitted voucherexpiresAtnow encodes verbatim as 0 in the signed 50-byte preimage (wasDEFAULT_SESSION_EXPIRES_AT), watermark records 0, docstring corrected, remaining sentinel uses audited. Cross-SDK freeze:session-voucher-preimage-no-expiryruns in the TS/Python/Go/Kotlin/Swift runners.08c77fb4(Py): open + top-up verifiers resolve preflight-rejected retries against the chain (confirmed-account field match for open; landed-clean signature status → confirm → deposit re-check for top-up; landed-but-FAILED stays authoritative). Mock RPC rejects duplicate sends like mainnet.7085443f(all 3 SDKs, breaking wire): spec voucher shape per ludovic's call — inner keydata→voucher, REQUIRED top-levelchannelIdon the voucher action (emit + parse + server reject on mismatch, close's nested voucher included). Frozen: voucher-action + close-action vectors (close also freezes omitted-expiresAt), plus a reject vector for the superseded inner-datashape. Go skip allowlist +3 until its wire migration.d1f2beed(TS):submitOpenTxshared the naked send — same confirmed-account rescue as Rust/Python.No new version bumps: folds into the unpublished 0.5.0/0.8.0/0.6.0 breaking bump. History note: the full branch was rewritten once more and every commit SSH-signed (
2fad1c61→ this range); trees verified content-identical before the force-push.d1f2beedcargo test(workspace)--features redis-store --lib360 passed vs live Redisupto.rstoo_many_argumentswarning)pnpm test+ prettier/eslint/tscpytest+ ruff + pyrightgo test ./...2026-08-01 incident follow-up (
3524a11b)Old writers (a draining revision, or any service pinned to pre-binding PayKit) sweeping the shared Redis lossily re-encoded newer rows and stripped the session proof binding. One commit, canonical here and mirrored to TS/Python:
ChannelStategains#[serde(flatten)] extra(unknown fields round-trip verbatim through read-modify-write) and aschema_versionstamped on every durable write; stores refuse records stamped newer than the running writer instead of decoding them lossily. TS ships the contract as documentation (CHANNEL_STATE_SCHEMA_VERSION) since its stores are integrator-owned."session channel predates proof binding"instead of sharing the generic"does not match the proof bound at open"— same fail-closed outcome, self-identifying in logs. No frozen wire vectors reference these strings.Verified at
3524a11b: rustcargo test12 targets / 847 passed (+ redis-store feature suite against live Redis), TS 512 passed + lint/format/typecheck, Python 1216 passed + ruff + 90% coverage gate. Cascades: pay#416 (pin + mirror), agent-gateway#14 (settlement-worker pin).