fix: make wave-batch finalize retryable after payment (#140)

[Nic-dorman]

Summary

Fixes #140. A post-payment chunk-store failure in the external-signer / WalletConnect finalize path (Client::finalize_upload*) stranded the on-chain payment: the finalize consumed the PreparedUpload and dropped the paid proofs, so retrying storage was impossible without paying again — re-preparing collects fresh quotes with different quote hashes (PaymentQuote::hash() folds in the quote timestamp + node key + signature), so the already-paid {quote_hash: tx_hash} map no longer matches.

This makes a post-payment store failure recoverable without re-paying, for both the wave-batch and merkle finalize paths, behind one uniform handle.

What changed

• WaveResult retains the paid chunks on failure. store_paid_chunks_with_events previously mapped failures to (address, error) and dropped the PaidChunk (which holds proof_bytes + PUT targets). It now also returns failed_chunks: Vec<PaidChunk>.
• New Error::FinalizeStorePaidFailed carries a PaidRetryState — the paid-but-unstored material plus cumulative store progress. The finalize returns this instead of PartialUpload (wave) / instead of a silent partial Ok (merkle) on a post-payment store failure, so "the payment is retained and retryable" is a type-level distinction, not an easily-missed field.
• PaidRetryState is one uniform handle over both payment modes (PaidRetryKind::{Wave, Merkle}):
  • Wave: each unstored chunk carries its own paid PaidChunk proof.
  • Merkle: one reusable MerkleBatchPaymentResult (per-chunk proofs keyed by address) + the unstored chunk bodies. No new pool or winner hash needed to resume.
• New Client::finalize_resume / finalize_resume_with_progress dispatch on the retry kind and re-drive storage of the unstored chunks against the same payment. On success they return the full FileUploadResult; if some chunks still fail they return FinalizeStorePaidFailed again with a reduced retry state, so callers can loop. One catch, one resume, regardless of payment mode.
• Both drivers (store_paid_wave, store_paid_merkle) share a RetryShared bundle for their common fields and are used by both the initial finalize and the resume path, so first-attempt and resume behave identically.

Behaviour change worth noting for review

finalize_upload_merkle* previously returned Ok(FileUploadResult { chunks_failed: N, .. }) on a partial store — a silent partial success. It now returns Error::FinalizeStorePaidFailed when chunks remain unstored after retries. A fatal (non-quorum) merkle store error — e.g. a missing proof — is still returned as a hard error, since re-storing can't fix it.

PaidRetryState stays resident in Rust memory (the wave proofs hold non-serializable PeerId/MultiAddr); FFI consumers (ant-ffi, WithAutonomi/ant-sdk#201) retain it as an opaque handle rather than serializing across the boundary.

Design decisions worth a look

1. Dedicated error variant vs. a retry: Option<..> field on PartialUpload. Chose a new variant to keep the retry material non-optional and avoid touching the ~9 other PartialUpload construction sites. Trade-off: external callers that matched PartialUpload from finalize now see a new variant (in-repo nothing does; the only consumer is the FFI, updated in lockstep).
2. One enum-backed PaidRetryState + one finalize_resume, vs. separate merkle/wave types and methods. Chose the unified handle for consumer ergonomics (CLAUDE.md principles 2 & 7) — one error, one resume call, mode-agnostic. Internal dispatch absorbs the wave/merkle difference.
3. Hand back a post-payment PaidRetryState, not the PreparedUpload. The latter can't be reconstructed (its payment info is pre-payment); the recoverable state after payment is genuinely a different type.

Testing

• cargo clippy --all-targets --all-features -- -D warnings clean; cargo fmt --all -- --check clean.
• Unit tests lock the FFI-facing contract: PaidRetryState accessors for both wave and merkle kinds, and the FinalizeStorePaidFailed display/retry-material shape. merkle/file/batch modules: 80 passed.
• The failure→resume round-trip is inherently integration-level (needs a partial store failure after real payment); it belongs in the devnet e2e harness (e2e_file.rs already covers the external-signer happy path) and is not exercised by unit tests here.

Note: the local Windows run shows one pre-existing, unrelated failure — adaptive::tests::save_snapshot_to_unwritable_dir_does_not_panic hardcodes a Unix path (/nonexistent_root_dir_xyz_for_test/...) that Windows resolves to the current drive and successfully creates. Untouched by this PR; passes on Linux CI.

🤖 Generated with Claude Code

[dirvine]

Hermes review — PR #143

Directionally correct, but I would not merge yet.

What looks good:

• The API shape addresses the core external-signer finalize: post-payment storage failure strands the payment (PreparedUpload consumed, not retryable) #140 problem: PaidRetryState, Error::FinalizeStorePaidFailed, and Client::finalize_resume(_with_progress) give callers a way to retain paid retry material instead of dropping it after post-payment storage failure.
• Downstream FFI use looks viable: PaidRetryState can stay opaque, FinalizeStorePaidFailed is matchable, the boxed retry state can be moved into an upload/session map, and later passed back to finalize_resume.
• Wave retry narrowing looks sound: the failed chunks are retained as PaidChunks and resume retries only those chunks, with no re-quote/re-pay path.
• Merkle now has the right broad shape as well: it retains MerkleBatchPaymentResult plus unstored bodies/addresses and avoids the previous “successful FileUploadResult with chunks_failed > 0” contract problem.

Blocking gap before merge / taking out of draft:

• The tests do not yet prove the actual external-signer finalize: post-payment storage failure strands the payment (PreparedUpload consumed, not retryable) #140 acceptance path:
  • no wave behavioural test for finalize_upload → post-payment store failure → extract retry state → finalize_resume → success without a new payment;
  • no equivalent Merkle behavioural test;
  • no public upload/DataMap retry test proving the data_map_address chunk is preserved/stored and remains retrievable;
  • no assertion of progress/spend/count behaviour across failure → retry → success.

Checks observed:

• Local focused checks passed: retry-state tests, finalize_store_paid_failed, fold_single_wave, partition_addresses_by_proof, and cargo check -p ant-core -p ant-cli.
• When checked, GitHub build/unit/clippy/format/docs were passing; Security Audit was failing and E2E/Merkle E2E were still pending.

Recommendation:

• Keep the API shape.
• Add behavioural retry tests before merge. Minimum blocker tests: wave and Merkle post-payment failure → retry → success, with proof that no new payment path is entered. Public DataMap retry should be included if possible, since that is the user-visible “address is retrievable” contract.

[dirvine]

Hermes follow-up — Merkle / retry-state issues

A later review pass found two additional issues worth addressing before merge. These do not change the earlier conclusion that the API direction is right, but they tighten the blocker list.

Blockers / should-fix

1. Merkle fatal-after-payment errors can still strand the paid batch.

store_paid_merkle currently calls merkle_upload_chunks(...).await?, so any Err exits before FinalizeStorePaidFailed / PaidRetryState is constructed. merkle_upload_chunks re-raises outcome.fatal as a bare error. Some fatal classifications may come from lookup/store/network/protocol errors after payment, not only irrecoverable proof corruption.

That means an external Merkle payment can still lose the reusable batch proof/body state on a post-payment error path. If the intended contract is “after payment, storage failures retain retry material”, this path should either return retry state as well or be very narrowly classified/documented/tested as truly unrecoverable.

2. Merkle external-signer spend is reported as "0" even when payment happened.

The success path and retryable-failure spend both hard-code storage_cost_atto: "0". That conflicts with the FileUploadResult contract where zero means all chunks already existed / no storage cost. If external Merkle cannot know gas, fine, but the storage amount paid should either be carried through from prepare/finalize state or the contract should explicitly say it is unknown for external Merkle.

Warnings

• PaidRetryState / Error::FinalizeStorePaidFailed derive Debug and can include DataMap, chunk Bytes, payment proofs, and Merkle proof material. Display is safe, but {:?} logging downstream could leak private upload material. Consider a redacted manual Debug for PaidRetryState / retry kind.
• Merkle retry state keeps the whole MerkleBatchPaymentResult.proofs map even when only a small failed subset remains. Filtering proofs to unstored_addresses would reduce memory and the leak surface above.
• Final success after one or more retries reports only the latest storage attempt stats; failed-attempt stats are not accumulated in PaidRetryState. That may be acceptable, but it should be deliberate/documented if these metrics are per-final-call rather than lifetime finalize+resume totals.
• A Merkle comment is now stale: MerkleStoreOutcome::failed_addresses says the external-signer path only reads counts, but this PR now reads addresses to build retry state.

The earlier test blocker still stands: add behavioural tests for wave and Merkle post-payment failure → retry → success without new payment, plus public DataMap handling if possible.

[Nic-dorman]

Review addressed (commit 787a891)

Thanks — all blockers, warnings, and the test gap are addressed. Point-by-point:

Blockers

1. Merkle fatal-after-payment no longer strands the batch. merkle_upload_chunks stops re-raising outcome.fatal; the new pure assemble_merkle_result classifies it: proof/data-corruption (Payment/InvalidData/Serialization/Crypto/SignatureVerification) stays terminal, while transient fatals (network/timeout/io/protocol/storage) fold every unconfirmed chunk — quorum shortfalls and chunks left unattempted when the pass aborted (input-minus-stored) — into resumable retry state. The whole-file data.rs path re-raises fatal itself to keep its all-or-nothing contract.
2. Merkle spend is now the real amount. finalize_merkle_batch sets storage_cost_atto to the sum of the winner pool's per-node quoted prices, threaded into both the success result and the retry-failure spend, instead of the contract-violating "0". ⚠️ One thing worth a second look: I derive the amount as the sum of winner_pool.candidate_nodes[].price — please confirm that matches what the on-chain merkle call actually charges (the internal-wallet path gets the figure back from the wallet, which I can't replicate here). It's a single visible expression if the derivation needs adjusting.

Warnings

• Debug leak → PaidRetryState now has a manual redacted Debug (counts only — no bodies, no proofs).
• Whole proofs map retained → merkle retry state now keeps only the proofs for the unstored subset (proofs.retain(...)).
• Per-attempt stats → documented on finalize_resume that retry/timing metrics are per-call by design (counts stay cumulative).
• Stale comment → MerkleStoreOutcome::failed_addresses doc updated.

Test blocker

Split both drivers into a thin network call + a pure assemble_wave_result / assemble_merkle_result, which lets the #140 path be proven deterministically in CI without a fault-injection harness. Added behavioural round-trip tests:

• wave_finalize_then_resume_succeeds_without_new_payment — finalize → B/C fail post-payment → extract retry → resume → all 4 stored; asserts spend + cumulative counts + public data_map_address carried; resume path has no quote/pay call (structural "no second payment").
• merkle_finalize_then_resume_succeeds_and_reports_spend — same round-trip; asserts real spend ("777", not 0), proofs filtered to the unstored subset, data_map_address preserved.
• merkle_transient_fatal_is_retryable and merkle_unrecoverable_fatal_is_terminal — cover the Blocker-1 classification split.

Public-DataMap retrievability end-to-end (address actually fetchable after resume) still belongs in the devnet e2e harness — the unit tests assert the data_map_address is threaded through resume, but not a live fetch.

Local: clippy -D warnings clean, fmt clean, ant-core lib 396 passed (the one failure is a pre-existing Windows-only test with a hardcoded Unix path, unrelated). CI Security Audit failure is also unrelated — two new RUSTSEC advisories against transitive deps (crossbeam-epoch, quinn-proto) not yet in the workflow ignore list; this branch changes no Cargo files.

[Nic-dorman]

Test coverage: Tier 1 landed, Tier 2 filed

Following up on the reviewer's request for the actual #140 acceptance path:

Tier 1 — real-network failure → retry state (added, 81570b0). New e2e test_finalize_after_payment_failure_yields_retryable_state_wave (in-process MiniTestnet + real Anvil payments): prepares public, pays the quotes, collapses the network below store quorum after payment, then asserts the real store path returns Error::FinalizeStorePaidFailed with a correct PaidRetryState, that the failed finalize spends no additional tokens, and that finalize_resume re-drives the real store/assemble path and hands back an equivalent retry state — again with wallet balance unchanged (proves no second payment path). Passes locally.

It's marked #[ignore] because it drives storage against deliberately-killed peers, so its wall-clock is dominated by transport dial timeouts (~6 min, worse on virtualised runners). The deterministic coverage of the same logic runs in CI via the assemble_wave_result / assemble_merkle_result unit tests; this is an on-demand real-network proof (cargo test -p ant-core --test e2e_file -- --ignored).

Tier 2 — full live round-trip → filed as #144. The success-after-resume half (first attempt fails post-payment, network recovers, finalize_resume lands the chunks and they're retrievable incl. public data_map_address) needs a per-node restart primitive the harness lacks today (MiniTestnet has shutdown_node but no restart; LocalDevnet only exposes whole-devnet shutdown). #144 captures the recipe and the harness options (per-node stop/start, or a reversible network-block helper).

Net coverage: unit tests prove failure → resume → success deterministically in CI; Tier 1 proves the real store path produces the retry material and never re-pays; #144 tracks the live success round-trip.