improve merkle upload throughput: ramp store concurrency + de-wave the store phase - #137
Conversation
On fast links the store adaptive-concurrency limiter sat pinned at its cold-start floor of ~8 for a whole upload, capping throughput. Three compounding causes, all fixed here: 1. Transport quorum-shortfall pollution. A close-group PUT that misses quorum only because dead/stale relayed peer addresses could not be dialled surfaced as `InsufficientPeers` -> `NetworkError`, counting against `success_rate` and cutting the cap. This is remote peer churn (same root cause as V2-551), not local backpressure. Split per-peer failures into timeout (genuine backpressure) vs dial/relay churn, and route the aggregate: any timeout keeps `InsufficientPeers` (still cuts the cap), a pure dial-churn shortfall surfaces the new neutral `Error::CloseGroupShortfall` (classified `ApplicationError`), and an app-only shortfall keeps its representative error (V2-468). The new variant stays a recoverable/deferred shortfall in the merkle retry. 2. Asymmetric increase/decrease gate. A Decrease fires every `min_window_ops` (8) samples but an Increase only every `window_ops` (32), and a Decrease zeroed the increase counter, so a few-percent shortfall permanently starved growth. Retain the increase credit across a Decrease AND relax the store `success_target` 0.95 -> 0.88 so normal per-chunk noise reads as a healthy window; genuine congestion (larger shortfall / timeout ceiling) still cuts the cap. Store-scoped; quote keeps the classic gate. 3. Per-wave/per-file cap snapshot. Store fan-out used `buffer_unordered(cap)` read once per wave/file, so mid-flight limiter growth never reached in-flight work. `merkle_store_with_retry` and the wave `store_paid_chunks_with_events` now roll a `FuturesUnordered` that re-reads the cap as each slot frees (the byte budget still bounds the wave path). Covers the merkle wave, whole-file, and deferred-retry paths. All logic is in ant-core with unit tests for each fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-out The whole-file merkle upload stored chunks in sequential fixed 64-chunk waves with a hard barrier between them. On a churny network the wave barrier is the throughput bottleneck: a handful of chunks whose close-group peers are dead/stale relayed addresses take minutes to dial/revalidate, and the whole wave cannot advance until its slowest chunk finishes — so the next wave's chunks don't even begin peer resolution. A production 2.36 GB / 609-chunk upload spent ~70% of its wall-clock in these inter-wave gaps (measured on STG-UL-01), while the V2-554 store limiter had ramped to 64 and the actual store bursts hit ~50 concurrent. Store concurrency was no longer the long pole; the wave serialization was. Remove the waves. The payment/proofs are already computed up front, so the store phase only needs a memory bound, not batching. Store the whole file as a single cap-bounded fan-out: - `merkle_store_with_retry` now takes addresses and a `store_one(addr)` that acquires its own body (reads it from the spill on demand), so only the <=cap in-flight stores hold a body. The store cap maxes at 64, so peak resident memory stays ~64 x MAX_CHUNK_SIZE — the same bound the 64-chunk waves gave — with no barrier. A slow straggler now overlaps with the rest of the file instead of stalling it. - `merkle_deferred_retry` drops its per-batch `read_bodies`/`batch_size` memory-bounding (the cap now bounds memory) and stores each round's whole pending set in one pass. - `upload_merkle_from_spill` (renamed from `upload_waves_merkle`) makes a single store call over all chunks instead of looping waves; deferred retry, stats, progress, and fatal/PartialUpload handling are preserved. New tests: the store pass has no barrier (a blocked straggler must not deadlock the others) and holds at most `cap` bodies in flight (memory bound). This overlaps but does not fix the underlying dead-relay resolution latency (V2-551); it stops that latency from serializing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Hermes review updateI reviewed PR #137 locally at head Code-level verdict: the upload-throughput direction looks sound, and the focused local checks passed, but I would not call this release-ready until the CI gates below are resolved. Blocking / release-gating
Warning worth considering before merge
I would either clamp the merkle spill store cap to the same 64/byte-budget invariant, or update the comment/config contract so the memory bound is explicit and intentional. Local checks runPassed locally:
No GitHub approval/request-changes submitted; this is a review comment only. |
|
Follow-up from the review team: one additional API/progress-contract warning.
Evidence:
This probably does not break the CLI progress bar because |
…event Address PR WithAutonomi#137 review (Hermes / @dirvine): - The de-waved `upload_merkle_from_spill` fan-out sized concurrency straight from `store_limiter.current()`. The default store cap is 64 (~256 MB of spilled bodies, matching the old wave bound), but `AdaptiveConfig::sanitize` permits `adaptive.max.store` above 64, so a custom high max could hold many more full chunk bodies resident. Clamp the fan-out (and the deferred-retry rounds) via `merkle_store_cap`, capped at `MERKLE_STORE_MAX_IN_FLIGHT` (64), so peak resident store memory stays at the wave-era ~256 MB regardless of the configured max. Throughput is unaffected at the default cap. - The de-wave removed the per-wave `UploadEvent::WaveComplete` emission, but the event is a public phase-completion signal consumed by the CLI progress adapter and external API clients. Emit a single terminal `WaveComplete` ("wave 1 of 1") after the store pass and deferred retries so consumers keying on it still see the store phase complete. Per-chunk `ChunkStored` events drive the bar as before. New test `merkle_store_cap_clamps_to_memory_bound`. cargo fmt + clippy clean; 374 ant-core lib tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review. Addressed the two code-level items in Memory bound on the merkle spill fan-out. Good catch — the fan-out sized concurrency straight from
Security Audit ( |
Follow-up to the PR WithAutonomi#137 review discussion. `WaveComplete` had no real consumer: the CLI handler only mirrored `ChunkStored` (`set_position(stored)`), and the ant-ui Tauri backend maps it to `None` (never forwarded to the frontend). Emitting a synthetic "wave 1 of 1" from the de-waved merkle path to preserve it was therefore pointless. Remove the event entirely instead: drop the `UploadEvent::WaveComplete` variant, both emissions (the single/wave-batch path and the merkle terminal one added in the previous commit), and the redundant CLI match arm. Per-chunk `ChunkStored` drives all upload progress bars, so behaviour is unchanged. `UploadEvent` is an internal in-process type (derives only Debug/Clone, not serialised), so this is not an API-schema change; ant-ui pins ant-core by tag and will drop its own dead match arm when it next bumps. cargo fmt + clippy clean; 374 ant-core lib tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Follow-up on the Checked the actual consumers first:
With no real consumer, emitting a "wave 1 of 1" was pointless, so I dropped the |
Follow-up to the PR WithAutonomi#137 review discussion. `WaveComplete` had no real consumer: the CLI handler only mirrored `ChunkStored` (`set_position(stored)`), and the ant-ui Tauri backend maps it to `None` (never forwarded to the frontend). Emitting a synthetic "wave 1 of 1" from the de-waved merkle path to preserve it was therefore pointless. Remove the event entirely instead: drop the `UploadEvent::WaveComplete` variant, both emissions (the single/wave-batch path and the merkle terminal one added in the previous commit), and the redundant CLI match arm. Per-chunk `ChunkStored` drives all upload progress bars, so behaviour is unchanged. `UploadEvent` is an internal in-process type (derives only Debug/Clone, not serialised), so this is not an API-schema change; ant-ui pins ant-core by tag and will drop its own dead match arm when it next bumps. cargo fmt + clippy clean; 374 ant-core lib tests pass. BREAKING CHANGE: The `UploadEvent::WaveComplete` variant is removed from ant-core's public `UploadEvent` enum. Any consumer that matches on it must drop that arm; upload progress is fully covered by `UploadEvent::ChunkStored`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bea9ea8 to
feec056
Compare
|
Follow-up review on latest head The two code-level comments I left are addressed:
Local checks passed:
Current PR checks at this head: format, clippy, unit tests, documentation, and both build jobs are green. Security Audit is still failing on the unrelated With the security-audit exception handled separately, I do not have remaining code-level objections to this PR. I would still wait for the pending E2E/Merkle E2E jobs before cutting the client/desktop release artifact. |
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
Approved on code review for head feec056de9b54cbb10ea524cdf1c74ece1859cec.
I re-checked the addressed review items:
- merkle spill store concurrency is now clamped via
MERKLE_STORE_MAX_IN_FLIGHT = 64/merkle_store_cap(...), including deferred retries; - the previous
UploadEvent::WaveCompletemismatch is resolved by removing the event and the CLI consumer; - local focused checks passed:
cargo check -p ant-core -p ant-cli, fmt, clippy forant-coreandant-cli, and focused adaptive/merkle/file unit tests.
Caveat: this approval is for the PR code changes. The Security Audit failure (RUSTSEC-2026-0185 / quinn-proto) remains outside this PR per Chris’s note, and the pending E2E/Merkle E2E jobs should still be treated as release/merge gates by the maintainer.
2 participants
Two related changes that together cut a production merkle upload from ~73 min to ~34 min for a comparable file (see Validation).
Summary
1. Let the store AIMD limiter ramp off its cold-start floor (V2-554). On fast links the store concurrency limiter sat pinned at ~8 for a whole upload. Three compounding causes fixed:
InsufficientPeers→NetworkError, dragging the limiter down. This is remote peer churn, not local backpressure. Per-peer failures are now split into timeout (genuine backpressure, keepsInsufficientPeers) vs dial/relay churn (new neutralError::CloseGroupShortfall, classifiedApplicationError, excluded from the limiter's health signal). App-only shortfalls keep their representative error (V2-468).min_window_ops(8) samples but an Increase only everywindow_ops(32), and a Decrease zeroed the increase counter, so a few-percent shortfall permanently starved growth. The store channel now retains the increase credit across a Decrease and relaxessuccess_target0.95→0.88 so normal per-chunk noise reads as a healthy window. Genuine congestion (larger shortfall / timeout ceiling) still cuts the cap. Quote channel keeps the classic gate.buffer_unordered; it now rolls aFuturesUnorderedre-reading the cap per freed slot so mid-flight growth reaches in-flight work.2. De-wave the merkle store phase into one cap-bounded fan-out. The whole-file upload stored chunks in sequential fixed 64-chunk waves with a hard barrier between them. On a churny network a handful of chunks whose close-group peers are dead/stale relayed addresses take minutes to dial, and the whole wave cannot advance until its slowest chunk finishes — so the next wave's chunks don't even begin peer resolution. A 609-chunk production upload spent ~70% of its wall-clock in these inter-wave gaps.
merkle_store_with_retrynow takes addresses and astore_one(addr)that reads its own body from the spill on demand, so only the ≤capin-flight stores hold a body. The store cap maxes at 64, so peak memory stays ~64 × MAX_CHUNK_SIZE— the same bound the 64-chunk waves gave — with no barrier.merkle_deferred_retrydrops its now-redundant per-batch body reads (the cap bounds memory) and stores each round's whole pending set in one pass.upload_merkle_from_spill(renamed fromupload_waves_merkle) makes a single store call over all chunks; deferred retry, stats, progress, and fatal/PartialUploadhandling are preserved.This overlaps but does not fix the underlying dead-relay peer-resolution latency (V2-551); it stops that latency from serializing the upload.
Test plan
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo test -p ant-core --lib— 373 passed. New unit tests: transport-vs-dial shortfall routing; the rebalanced gate ramps where the classic gate pins; the store scheduler re-reads the cap per slot; the store pass has no barrier (a blocked straggler must not deadlock the others); the store pass holds ≤capbodies in flight (memory bound).Validation (production merkle uploads, lon1)
Wave N/Mmarkers), the store cap ramping 8→16→32→64 with 0 decreases, and sustained 50–90 completions/min with no inter-wave gaps through the main pass, followed by a short deferred-retry tail.Effective throughput ~3× for a comparable-or-larger file. Both changes verified against real uploads; further gains on churny links are gated by V2-551 (relay affinity).