feat(dash-spv): accelerate compressed header sync - #950
Conversation
📝 WalkthroughWalkthroughThe change propagates precomputed header hashes through ChangesHeader synchronization and decompression
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PeerReader
participant Semaphore
participant BlockingWorker
participant MessageDispatcher
PeerReader->>Semaphore: acquire Headers2 permit
Semaphore->>BlockingWorker: run bounded decompression
PeerReader->>BlockingWorker: decompress Headers2
BlockingWorker-->>PeerReader: return headers, hashes, or error
PeerReader->>MessageDispatcher: dispatch headers in wire order
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dash-spv/src/sync/block_headers/pipeline.rs`:
- Line 145: Update receive_headers to reject unsolicited non-empty batches when
the sync is complete, requiring exactly one header before resetting the tip and
forwarding it to SegmentState::receive_headers; preserve the existing
requested-sync path for legitimate batches. Add an in-module test using two
chained headers that verifies an error is returned and the completed tip state
remains unchanged.
- Around line 128-129: Update handle_headers_pipeline to call send_pending after
take_ready_to_store so the active window is immediately refilled when draining
advances next_to_store. Add in-module and integration tests covering all active
segments completing with the final lowest-index segment, verifying the next
segment is requested in the same response cycle rather than waiting for the next
tick.
In `@dash/src/network/message_headers2.rs`:
- Around line 275-276: Keep the cached hash paired with the exact previous
header it was computed from, rather than storing only prev_header_hash. Update
is_sequential and compress to use the cached hash only when that stored header
still equals the current public prev_header; otherwise recompute or follow the
normal non-cached path. Add a unit test that changes or clears prev_header after
decompress_with_hash and verifies compression does not omit an incorrect
prev_blockhash.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 15ecb17a-5430-4778-b163-1c9a1c5b482a
📒 Files selected for processing (8)
dash-spv/src/network/manager.rsdash-spv/src/network/message_dispatcher.rsdash-spv/src/sync/block_headers/manager.rsdash-spv/src/sync/block_headers/pipeline.rsdash-spv/src/sync/block_headers/segment_state.rsdash-spv/src/sync/block_headers/sync_manager.rsdash-spv/src/types.rsdash/src/network/message_headers2.rs
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
Bot
added
the
merge-conflict
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
This is not going through CI but the idea is definitely good
| &mut self, | ||
| compressed: &CompressedHeader, | ||
| ) -> Result<Header, DecompressionError> { | ||
| ) -> Result<(Header, BlockHash), DecompressionError> { |
There was a problem hiding this comment.
Maybe return a HashedBlockHeader
There was a problem hiding this comment.
HashedBlockHeader is defined in dash-spv, while this decompressor is in the lower-level dash crate. Returning that type here would introduce an inverted/circular dependency. The tuple keeps the protocol layer independent, and dash-spv immediately combines the exact (Header, BlockHash) pair with HashedBlockHeader::with_trusted_hash at its boundary.
🤖 Posted autonomously by Codex on behalf of pasta.
| std::thread::available_parallelism() | ||
| .map(|parallelism| parallelism.get().min(MAX_CONCURRENT_HEADERS2_DECOMPRESSIONS)) | ||
| .unwrap_or(1) | ||
| } |
There was a problem hiding this comment.
I don't think this function ads real value, did you try what happens if you compute as many as you can using the Tokio runtime?? I don't see it overloading the CPU
There was a problem hiding this comment.
I tested this today against clean current Dash Core at testnet height 1,532,360. With fresh storage after warm-up, the four-worker median was 2.90 s (2.91/2.90/2.86); using all 14 available CPUs regressed to 4.31 s (4.25/4.31/4.69), about 48.6% slower. The global bound also applies consistently to both network-manager construction paths, so I retained the helper and four-worker cap. I added the A/B result to the PR description.
🤖 Posted autonomously by Codex on behalf of pasta.
d4ea644 to
9b14095
Compare
github-actions
Bot
removed
the
merge-conflict
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash-spv/src/network/manager.rs (1)
631-721: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve ordering across both header message variants.
The ordered buffer releases only
Headers2outcomes. After Line 721, the reader can receive and dispatch a later regularHeadersmessage before this earlierHeaders2batch completes. The regularHeadersbranch remains accepted and forwarded by this reader.Route both header message variants through one ordered dispatch barrier. This prevents a later header announcement from overtaking an earlier compressed announcement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv/src/network/manager.rs` around lines 631 - 721, Extend the existing ordered dispatch mechanism around ordered_headers2_results so regular NetworkMessage::Headers and compressed Headers2 share the same sequence and release barrier. Assign every header message a sequence number, enqueue the regular Headers outcome instead of dispatching it immediately, and only forward results in sequence order; preserve the existing Headers2 decompression handling and ensure later regular headers cannot overtake an earlier Headers2 batch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dash-spv/src/network/manager.rs`:
- Around line 2143-2155: Expand coverage beyond
`OrderedHeaders2Results::complete` with in-module tests for peer-session
cancellation, regular `Headers` ordering, invalid decompression handling, and
permit release after task failure. Add an integration test under the crate’s
`tests` directory that exercises ordered dispatch through the peer reader,
preserving receive-order behavior.
- Around line 655-695: Update the Headers2 decompression task spawned in the
peer reader to use a reader-scoped cancellation token or connection generation,
rather than checking only shutdown_token. Cancel or invalidate that scope before
the reader removes the peer, and check it before dispatching each completed
result in the ordered-results loop so ended sessions cannot forward headers to a
later connection using the same SocketAddr.
---
Outside diff comments:
In `@dash-spv/src/network/manager.rs`:
- Around line 631-721: Extend the existing ordered dispatch mechanism around
ordered_headers2_results so regular NetworkMessage::Headers and compressed
Headers2 share the same sequence and release barrier. Assign every header
message a sequence number, enqueue the regular Headers outcome instead of
dispatching it immediately, and only forward results in sequence order; preserve
the existing Headers2 decompression handling and ensure later regular headers
cannot overtake an earlier Headers2 batch.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c0f1cfa-bfde-4af6-a282-12df0de22472
📒 Files selected for processing (1)
dash-spv/src/network/manager.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #950 +/- ##
==========================================
+ Coverage 75.62% 75.77% +0.14%
==========================================
Files 329 329
Lines 79258 79671 +413
==========================================
+ Hits 59939 60367 +428
+ Misses 19319 19304 -15
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dash-spv/tests/header_dispatch_order.rs`:
- Around line 20-24: Parameterize the header-dispatch ordering tests and their
send_message fixture to accept both a Network value and listener address,
running the scenario for Network::Mainnet and Network::Testnet. In send_message,
derive RawNetworkMessage.magic from the selected network instead of hardcoding
Network::Regtest.magic().
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dfff518e-b782-45c9-8440-371e09f857d2
📒 Files selected for processing (2)
dash-spv/src/network/manager.rsdash-spv/tests/header_dispatch_order.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv/src/network/manager.rs
Dismissed after investigation: the sole finding requested duplicate Mainnet/Testnet executions of a network-independent Regtest localhost ordering regression. The thread documents the call-path evidence and is resolved; no code change is warranted.
🤖 Posted autonomously by Codex on behalf of pasta.
github-actions
Bot
added
ready-for-review
2 participants
Summary
headers2batch and carry it intoHashedBlockHeader, avoiding the second hash previously performed by the ordered sync pipelineheaders2messages concurrently, bounded globally to the lesser of four workers or the host's available parallelismEach checkpoint segment still permits only one in-flight request, so messages for the same segment cannot be reordered. Work from different segments may complete in any order, which the existing checkpoint pipeline already supports across peers.
Real testnet benchmark
The release binaries synced all 1,531,863 testnet headers from the same settled local Dash Core node, with full header validation, one configured peer, filters/masternodes/mempool disabled, and a fresh SPV storage directory for every run. Before and after builds were alternated. Warmups were excluded. A sequence during which the Core process restarted was discarded; the reported final sequence was collected only after Core reported
Done loadingand another warmup completed.Local runs were 11.34/11.30/11.28 s before and 2.96/2.98/2.91 s after. RTT runs were 11.22/11.15/11.61/11.25/11.49 s before and 6.38/6.13/8.15/6.01/6.04 s after.
An intermediate hash-reuse-only build measured 11.43 -> 10.84 s locally (5.2%) and 11.31 -> 10.74 s at ~100 ms RTT (5.0%). Sampling explained the smaller wall-clock change: before this PR, decompression and pipeline hashing were two similarly hot X11 stages already overlapped on separate Tokio tasks. Removing the second hash approximately halves that CPU work but makes the remaining single decompressor the throughput limiter; bounded parallel decompression realizes the larger wall-clock gain.
Peak buffered headers fell from a 1,423,863 median to 342,000, a 76.0% reduction. Profiling showed X11 rather than storage writes as the CPU bottleneck; the large buffer came from all 31 checkpoint segments downloading ahead of the earliest segment that could be stored in order.
Clean current-Core rerun
After rebasing onto current
dev, the release build was rerun against a freshly built, clean Dash Coredevelop@4defcfbe7b5aon local testnet at fixed height 1,532,360. That Core revision includes merged Dash Core PRs #7573 (configured SHA256 acceleration) and #7574 (compact-filter file-handle reuse), with no experimentalcfilters2changes. Each measurement used fresh SPV storage; one warm-up was excluded.The default run synchronized 1,532,360 headers, 1,532,361 filter headers and filters, masternode data, and mempool setup. The test mnemonic matched 733 filters; 733 historical blocks were processed, containing 6 relevant transactions. The masternode path processed 27 diffs, one QRInfo, and one validated cycle.
A reviewer-suggested host-wide decompression test was also run on the 14-CPU host. Four workers measured a 2.90 s median; 14 workers measured 4.31 s, a 48.6% regression, so the four-worker global cap remains intentional.
Validation
cargo test -p dash-spv --lib(546 passed, 2 ignored)cargo test -p dashcore --lib(578 passed, 15 ignored)cargo test -p dashcore --test headers2_compatibility_test --all-features(12 passed)cargo clippy --workspace --all-targets --all-features -- -D warningscargo build -p dash-spv --releaseThis pull request was created by Codex.
Summary by CodeRabbit
Performance
Reliability
Sync Improvements