feat(client): resumable external-signer finalize for both paths - #172
Conversation
Nic-dorman
changed the title
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
Requesting changes for one liveness blocker in the Merkle resume path. Partial payment is accepted, but unpaid chunks are placed into a resume handle that has no mechanism to acquire their missing proofs, so repeated finalize_resume calls cannot drain it. I also found a smaller documentation mismatch around fatal store errors.
Local verification: cargo fmt --check, cargo clippy -p ant-core --all-targets -- -D warnings, cargo check -p ant-core -p ant-cli, all 441 ant-core library tests, and doc tests passed at the exact head. A clean merge with current main also passed cargo check and the 30 focused file tests. The full workspace run reached the long E2E suite with no failures observed, but exceeded the 600s local limit; GitHub's unit/E2E/Merkle E2E/build/audit checks are all green.
| }) => { | ||
| // Recoverable: retain the spill and the already-signed proofs so the | ||
| // caller can drain the remainder against the same payment. | ||
| let unstored_addresses: Vec<[u8; 32]> = failed.iter().map(|(addr, _)| *addr).collect(); |
There was a problem hiding this comment.
[High] This resume handle can be permanently non-drainable after a partial Merkle payment. fold_external_merkle_payments accepts a mix of Some and None; upload_merkle_from_spill reports every unpaid chunk as failed because it has no proof. This collects all such addresses into unstored_addresses, but finalize_resume reuses the same batch_result.proofs and accepts no new payment/proof input. Therefore every unpaid address is classified as missing-proof again on every call, returning Partial forever rather than progressing to Complete against the same payment as the public contract promises. Please make partial payment terminal/non-resumable here, or add a way to supply the newly paid proof material on resume, and cover the Some + None case with a repeated-resume regression test.
There was a problem hiding this comment.
Fixed by making partial payment terminal for the resumable path (a4b669c): finalize_upload_merkle_multi_resumable now rejects a Some/None mix up front (require_fully_paid_for_resumable) with a typed Error::Payment that points at the non-resumable finalize_upload_merkle_multi, which keeps accepting partial payment and surfacing unpaid chunks via PartialUpload (ADR-0003). A resume handle therefore only ever exists fully paid, so every chunk it carries has its proof and the handle is always drainable in principle.
I went terminal rather than supply-proofs-on-resume: the handle doesn't retain the unpaid PreparedMerkleBatches, so accepting new winner hashes on resume would mean carrying those through the handle and growing the API surface for a path nobody needs yet. If mixed payment + resume is ever wanted, it can be added compatibly (MerkleFinalizeResume is #[non_exhaustive]).
Tests added: resumable_guard_rejects_partial_payment (the Some+None regression — asserts the unpaid count and the pointer at the non-resumable path), resumable_guard_accepts_fully_paid, and merkle_resume_handle_drains_to_complete (two-pass repeated-resume regression: shortfall pass → handle carries exactly the unstored set + original payment → drain pass reaches Complete with whole-file counts).
| /// | ||
| /// Returns an error if no sub-batch was paid, the winner-hash count does | ||
| /// not match the prepared batches, the payment info is wave-batch rather | ||
| /// than merkle, or a non-recoverable store failure occurs. A plain quorum |
There was a problem hiding this comment.
[Medium] The documented fatal-store Err path is not reachable through the current drive path. upload_merkle_from_spill converts fatal aborts into Error::PartialUpload via partial_upload_after_fatal, and assemble_merkle_finalize_outcome converts every PartialUpload to Ok(FinalizeOutcome::Partial). finalize_resume therefore also returns Partial for persistent spill-I/O/network failures rather than the non-recoverable Err promised here. Please either preserve a fatal classification in the implementation, or document that callers must bound the resume loop because store failures—including persistent ones—surface as Partial.
There was a problem hiding this comment.
Kept the behaviour and fixed the documentation (a4b669c). Folding fatal store aborts into Partial is deliberate — the spill and proofs are retained precisely so an abort that looks fatal in the moment (e.g. a transient network collapse) can be retried against the same payment rather than stranding it. What was wrong was the docs promising an Err that the drive path can't produce.
Now documented on finalize_resume: store failures — including persistent ones — surface as Partial on every call, never Err, so callers must bound their resume loop (cap attempts / back off / treat a handle that stops shrinking as stuck). Err is reserved for failures outside the chunk store itself. The # Errors sections on the resumable finalize methods, the drive_merkle_finalize internal doc ("genuinely fatal errors still propagate via Err" — removed), the FinalizeOutcome enum doc, and the CHANGELOG entry are all aligned to match.
…2-571) PR #172 review: a Some/None winner-hash mix produced a resume handle whose unpaid chunks could never acquire proofs — finalize_resume reuses the folded batch_result and accepts no new payment material, so every call reported the unpaid chunks as missing-proof again and the handle never drained to Complete, violating the loop-until-Complete contract. - finalize_upload_merkle_multi_resumable now requires every sub-batch paid (require_fully_paid_for_resumable); the typed error points at the non-resumable finalize_upload_merkle_multi, which still accepts partial payment per ADR-0003 - docs: store failures — including persistent ones and fatal aborts — surface as FinalizeOutcome::Partial, never Err, so callers must bound their resume loop (finalize_resume, the resumable finalize methods, drive_merkle_finalize, FinalizeOutcome, CHANGELOG) - tests: Some+None rejection regression, fully-paid guard pass, and a two-pass repeated-resume drain to Complete Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Re-review verdict: APPROVE (code) at The earlier high-severity resumability issue is fixed: mixed paid/unpaid Merkle batches are now rejected before producing a permanently non-drainable resume handle. The previous drain-loop concern is also covered by the current logic/tests. I found no remaining diff-specific blocker. Local verification: Current required CI still has the repository-wide |
External-signer finalize could strand an on-chain payment: a post-payment storage shortfall surfaced as `Error::PartialUpload`, and because both `finalize_upload` (wave-batch) and `finalize_upload_merkle_multi` (merkle) consume `PreparedUpload` by value, the paid material was dropped — the caller could not re-store the unstored chunks without re-preparing and re-signing (the core of #140, on both external paths). Add a resumable path that retains the recovery material across a partial, unified across both payment paths: - `FinalizeOutcome { Complete(FileUploadResult), Partial { result, resume } }` and an opaque `FinalizeResume { Wave(..), Merkle(..) }` handle. The wave variant owns the already-paid `PaidChunk`s still needing storage; the merkle variant owns the on-disk spill + already-signed proofs plus the cumulative-stored / still-unstored sets. Both `Debug`s are redacted to counts only (no bodies/proofs/data map). - `Client::finalize_upload_resumable` and `Client::finalize_upload_merkle_multi_resumable` (each + `_with_progress`) to start a resumable finalize, and one `Client::finalize_resume` (+ `_with_progress`) that re-drives storage for only the unstored chunks against the same payment — no re-quote, no second signature, no double pay. Loop until `Complete`. - Pure `assemble_wave_finalize_outcome` / `assemble_merkle_finalize_outcome` so the resume-handoff contract is unit-tested deterministically (5 tests: complete + partial-retains-resume per path, plus fatal-propagates) without a network. The existing consuming `finalize_upload` / `finalize_upload_merkle_multi` are unchanged (still return `Error::PartialUpload`). Live round-trip coverage tracked in #144. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…2-571) PR #172 review: a Some/None winner-hash mix produced a resume handle whose unpaid chunks could never acquire proofs — finalize_resume reuses the folded batch_result and accepts no new payment material, so every call reported the unpaid chunks as missing-proof again and the handle never drained to Complete, violating the loop-until-Complete contract. - finalize_upload_merkle_multi_resumable now requires every sub-batch paid (require_fully_paid_for_resumable); the typed error points at the non-resumable finalize_upload_merkle_multi, which still accepts partial payment per ADR-0003 - docs: store failures — including persistent ones and fatal aborts — surface as FinalizeOutcome::Partial, never Err, so callers must bound their resume loop (finalize_resume, the resumable finalize methods, drive_merkle_finalize, FinalizeOutcome, CHANGELOG) - tests: Some+None rejection regression, fully-paid guard pass, and a two-pass repeated-resume drain to Complete Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a4b669c to
879d377
Compare
jacderida
left a comment
jacderida
left a comment
There was a problem hiding this comment.
Approving on the strength of dirvine's re-review verdict (APPROVE (code) at a4b669c), which confirms the [High] non-drainable-handle finding is fixed by require_fully_paid_for_resumable and the [Medium] doc mismatch is resolved. Note the formal review state on this PR still reads CHANGES_REQUESTED because that verdict was posted as an issue comment rather than a review submission — the code concern itself is cleared.
Rebased onto current main (a4b669c -> 879d377, clean, no conflicts) to pick up #177 (h2 -> 0.4.17, clears the RUSTSEC-2026-0258 Security Audit failure dirvine flagged as the one remaining non-green item) and #178 (Merkle E2E timeout 60m -> 90m). Verified locally on the rebased head: cargo fmt clean, cargo test -p ant-core --lib 465 passed / 0 failed.
Scope confirmed additive: +916/-4, and the only deletions are one import line and one doc line — finalize_upload and finalize_upload_merkle_multi are untouched, so no existing caller changes behavior.
3 participants
Summary
External-signer finalize could strand an on-chain payment: a post-payment storage shortfall surfaced as
Error::PartialUpload, and because bothfinalize_upload(wave-batch) andfinalize_upload_merkle_multi(merkle) consumePreparedUploadby value, the paid material was dropped — the caller couldn't re-store the unstored chunks without re-preparing and re-signing. That is the core of #140, on both external paths.This adds a resumable finalize for both paths, unified so a consumer (the mobile FFI) holds one handle type and calls one resume method:
FinalizeOutcome { Complete(FileUploadResult), Partial { result, resume } }+ opaqueFinalizeResume { Wave(..), Merkle(..) }. The wave variant owns the already-paidPaidChunks still needing storage; the merkle variant owns the on-disk spill + already-signed proofs plus the cumulative-stored / still-unstored sets. BothDebugs are redacted to counts only (no chunk bodies, proofs, or data map).Client::finalize_upload_resumable(wave-batch) andClient::finalize_upload_merkle_multi_resumable(merkle), each+ _with_progress, start a resumable finalize.Client::finalize_resume(+_with_progress) re-drives storage for only the still-unstored chunks against the same on-chain payment — no re-quote, no second signature, no double payment. Loopable untilComplete, with a bound: store failures — including persistent ones — surface asPartial, neverErr, so callers cap their attempts (documented per review).Some/Nonewinner-hash mix is rejected up front with a pointer at the non-resumablefinalize_upload_merkle_multi(which still accepts partial payment per ADR-0003).assemble_wave_finalize_outcome/assemble_merkle_finalize_outcomeso the resume-handoff contract is unit-tested deterministically without a network.The existing consuming
finalize_upload/finalize_upload_merkle_multiare unchanged (they still surface a shortfall asError::PartialUpload).Closes #140 (both external paths). The live devnet round-trip (fail → resume → success, bytes retrievable) is tracked separately in #144 — it needs a per-node-restart / reversible-network-block harness primitive that does not exist yet — and stays open. FFI/SDK adoption is ant-sdk #201.
Linear issue
Fixes V2-571 — https://linear.app/autonominetwork/issue/V2-571/external-signer-finalize-make-post-payment-storage-failure-retryable
Risk tier
Compatibility
FinalizeOutcome,FinalizeResume,WaveFinalizeResume,MerkleFinalizeResume, andClient::{finalize_upload_resumable, finalize_upload_merkle_multi_resumable, finalize_resume}(each+ _with_progress); nothing is removed or changed. The existingfinalize_upload/finalize_upload_merkle_multikeep their signatures andError::PartialUploadbehavior.Semver impact
Test evidence
cargo test -p ant-core --lib: 441 passed, including 5 new deterministic unit tests over the pure outcome assemblers (assemble_{wave,merkle}_finalize_outcome): full-store →Complete, quorum-shortfall →Partialretaining exactly the unstored chunks / already-signed material (per path), and fatal-error → propagates.a4b669c): +3 unit tests —resumable_guard_rejects_partial_payment(Some+None→ typed error naming the unpaid count and the non-resumable path),resumable_guard_accepts_fully_paid, andmerkle_resume_handle_drains_to_complete(two-pass repeated-resume regression: shortfall handle → drain →Completewith whole-file counts). fmt/clippy (-D warnings) clean; ant-core lib suite green on Windows apart from the pre-existing platform-specificadaptive::save_snapshot_to_unwritable_dir_does_not_panic(assumes Unix root permissions; unrelated to this PR).cargo clippy -p ant-core --all-targets -- -D warnings: clean.cargo fmt --check: clean.cargo check --workspace: clean (ant-cli builds against the additive API).New dependency
none.
ADR
n/a — Tier 1. Additive to the existing ADR-0003 external-signer finalize API; no protocol, storage-format, or payment-economics change (the resume reuses the original on-chain payment).
Mitigation / rollback
Revert the PR. The change is purely additive client-side recovery API and the existing consuming finalize methods are untouched, so backing it out removes the new methods with no effect on stored data, wire protocol, or payments.