fix(rust): do not advance session watermark on a replayed commit

[EfeDurmaz16]

fix(rust): reconcile session watermark on a replayed commit

Suggested PR title (replaces the stale "do not advance session watermark on a replayed commit"): the final semantics do advance the watermark on replay, by reconciling it to the server-settled cumulative, clamped and never regressing.

Problem

SessionConsumer::commit_directive recorded the freshly prepared voucher unconditionally after every commit, including commits the server answered with status: replayed. Per the replay rule in skills/pay-sdk-implementation/references/intents/mpp-session.md (lines 221, 294, 355-357), a commit is replayed only when the retried delivery carries the same cumulative AND the same signature as the cached committed delivery, with the signature re-verified, and the server returns the cached receipt without advancing its watermark. Both reference servers implement exactly that:

• Rust: rust/crates/mpp/src/server/session.rs:495 (replay condition), :555 (replay check inside the atomic store closure), :782 (CommitStatus::Replayed with the cached receipt)
• TypeScript: typescript/packages/mpp/src/server/Session.ts:893-901 (idempotent replay window inside updateChannel: same cumulative and same voucherSignature, signature re-verified via assertVoucherSignature, cached outcome returned and return current leaves channel state untouched)

The client, however, treated a replayed receipt like a fresh acceptance and recorded the voucher it had just prepared. Against a non-conforming or malicious server that reports replayed with an arbitrary cumulative, this over-advanced the local watermark, so the next signed voucher could over-authorize up to the on-chain deposit. Skipping the record entirely is also wrong: in the lost-response case (the original commit succeeded but the receipt never arrived) the local watermark is behind the server, and the next delivery would sign a non-monotonic cumulative the server must reject.

Fix

• ActiveSession::reconcile_settled(settled): advances the local watermark to a server-settled cumulative when it is ahead, never regresses, and bumps the request nonce on advance so the next prepared voucher does not reuse the already-settled nonce (mirrors the record_voucher nonce accounting).
• SessionConsumer::commit_directive: on CommitStatus::Replayed, parse the receipt cumulative and reconcile to min(settled, prepared) instead of recording the prepared voucher; fresh commits record the voucher as before. The clamp treats the server as untrusted: an honest lost-response replay settles at or below the voucher just signed (single-threaded session), so a server-reported cumulative above it cannot push the watermark past what the client actually authorized.
• ActiveSession::record_voucher: rejects vouchers for a different channel. This is parity catch-up with the TypeScript client, which already throws on a channel mismatch in typescript/packages/mpp/src/client/Session.ts:466.

Against a conforming server the change is unobservable: replay requires the identical cumulative and signature, Ed25519 signing is deterministic, so settled equals prepared and reconcile_settled(min(settled, prepared)) advances exactly as record_voucher did. The fix is defensive hardening on the client, not a wire change.

Note for review: TypeScript client has the pre-fix behavior

typescript/packages/mpp/src/client/SessionConsumer.ts:76 (commitDirective) still calls this.#session.recordVoucher(voucher) unconditionally, with no branch on receipt.status === 'replayed'. After this PR the Rust and TypeScript reference clients diverge on the replayed branch. The divergence is latent against conforming servers (per above), but ports mirror both references, so a matching TypeScript follow-up is needed. Happy to open it once the semantics here are agreed.

Tests

Unit tests cover: lost-response reconcile to the settled cumulative, no regression from a stale replayed receipt, clamp against an inflated server-reported cumulative, foreign-channel voucher rejection, and nonce freshness after a replay reconcile.

• cargo fmt --check: clean
• cargo clippy -p solana-mpp --all-targets: no warnings in the touched files (the remaining warnings pre-exist on main in files this PR does not modify)
• cargo test -p solana-mpp: 641 lib + 9 integration tests pass, 0 failed

Coverage is unit-level (transport fakes). The harness has no session scenario yet; when the session harness cell lands, a duplicate-commit scenario should exercise this replayed-client path cross-SDK. Tracked as a follow-up.

[greptile-apps]

Greptile Summary

This PR fixes a watermark over-advancement bug in SessionConsumer::commit_directive: a replayed receipt previously caused the client to record the freshly-prepared (higher) voucher, letting a channel close authorize more than the server settled. The fix branches on CommitStatus::Replayed and calls reconcile_settled(settled.min(prepared)) instead, while record_voucher gains a channel-id guard to prevent cross-channel voucher acceptance.

• reconcile_settled advances cumulative and bumps nonce by 1 only when the settled value exceeds the current watermark, preventing both regression and nonce reuse on the next prepared voucher.
• Clamp settled.min(prepared) in commit_directive ensures an untrusted server cannot push the watermark past what the client actually signed.
• Test coverage exercises lost-response reconciliation, no-regress invariant, server-inflation clamp, foreign-channel rejection, and nonce-reuse prevention — all green.

Confidence Score: 5/5

Safe to merge — changes are surgical and self-contained, both paths in commit_directive are fully exercised by the new tests, and the clamp invariant prevents a misbehaving server from over-authorizing.

The logic in reconcile_settled correctly mirrors record_voucher nonce+1 accounting for the single-threaded lost-response scenario. The clamp settled.min(prepared) is the critical security boundary and is verified by a dedicated test. The channel-id guard in record_voucher is a pure tightening with no callers affected. No regressions introduced.

No files require special attention.

Important Files Changed

Filename	Overview
rust/crates/mpp/src/client/session.rs	Adds reconcile_settled (advances cumulative + nonce by 1 when settling higher than current watermark) and a channel-id guard to record_voucher; both well-tested with regression, no-regress, and nonce-reuse cases.
rust/crates/mpp/src/client/session_consumer.rs	Branches commit_directive on CommitStatus::Replayed to call reconcile_settled(settled.min(prepared)) instead of record_voucher; tests cover lost-response, no-regress, and server-inflation-clamp paths.

Sequence Diagram

sequenceDiagram
    participant C as Client (SessionConsumer)
    participant S as Server (CommitTransport)
    participant W as ActiveSession (watermark)

    Note over C,W: Fresh commit path (normal)
    C->>W: prepare_increment(amount)
    C->>S: commit(directive, payload)
    S-->>C: CommitReceipt status Committed
    C->>W: "record_voucher - cumulative=X nonce=N+1"

    Note over C,W: Lost-response replay path (new fix)
    C->>W: prepare_increment(amount)
    C->>S: commit(directive, payload)
    Note over S: Server already settled at Y
    S-->>C: "CommitReceipt status Replayed cumulative=Y"
    Note over C: clamp = min(Y, X)
    C->>W: reconcile_settled(clamp) - advances if above watermark

    Note over C,W: Server-inflation attack (clamped)
    C->>W: prepare_increment(amount)
    C->>S: commit(directive, payload)
    S-->>C: "CommitReceipt status Replayed cumulative=1000000"
    Note over C: 1000000 clamped to prepared X
    C->>W: reconcile_settled(X)

Loading

_{Reviews (6): Last reviewed commit: "fix(rust): clamp replayed-receipt reconc..." | Re-trigger Greptile}

[greptile-apps]

Using a negative comparison (!= Replayed) means any future CommitStatus variant that is not Replayed will silently advance the local watermark. The comment above already states the intent is to record only on a fresh committed receipt, so a positive equality check is more defensive and self-documenting — it will safely default to not recording if a new status is ever added.

Suggested change

	if receipt.status != CommitStatus::Replayed {
	self.session.record_voucher(&payload.voucher)?;
	}
	if receipt.status == CommitStatus::Committed {
	self.session.record_voucher(&payload.voucher)?;
	}

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[greptile-apps]

reconcile_settled leaves the nonce counter behind

record_voucher (the fresh-commit path) advances self.nonce to max(current, voucher.nonce). reconcile_settled advances only self.cumulative. In the canonical lost-response scenario this creates an immediate nonce collision: the client starts at nonce=0, prepare_increment(X) emits a voucher with nonce=1, the server settles it at nonce=1, the network drops the response, the client retries, gets Replayed at the correct cumulative, calls reconcile_settled — and is now at cumulative=X, nonce=0. The very next prepare_increment produces another voucher with nonce=1, which the server has already accepted. If the channel program or the session server validates nonce monotonicity (which is the whole point of tracking it client-side), that delivery will be rejected.

reconcile_settled should also advance the nonce, either by accepting it as a second parameter or by having commit_directive call a separate reconcile_nonce(payload.voucher.data.nonce.unwrap_or(0)) helper on the replay branch. The existing regression tests do not exercise a second delivery after a replay, so the failure mode is invisible today.