refactor(dash-spv): network manager refactor and sync pipelines optimized - #902
refactor(dash-spv): network manager refactor and sync pipelines optimized #902ZocoLini wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughThe PR replaces request-sender networking with broker-based routing, rewrites peer and synchronization pipelines, updates client lifecycle coordination, adds Tokio message framing, and adapts FFI, tests, examples, and the seed fetcher. ChangesSPV network and synchronization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #902 +/- ##
==========================================
+ Coverage 75.18% 75.36% +0.17%
==========================================
Files 328 319 -9
Lines 78194 76195 -1999
==========================================
- Hits 58792 57426 -1366
+ Misses 19402 18769 -633
|
39b7db2 to
fd614fc
Compare
|
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
726da21 to
0ed48ff
Compare
github-actions
Bot
removed
the
merge-conflict
9615c7a to
5693f59
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
dash-spv/src/sync/blocks/manager.rs (1)
66-75: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInflated
requestedprogress counter.BlocksPipeline::send_pendingnow returns the whole wanted set rather than newly-issued requests, and it is invoked on every tick, so the runningadd_requestedsum grows without bound.
dash-spv/src/sync/blocks/manager.rs#L66-L75: stop summing the returned count — either have the pipeline report only newly declared hashes, or expose the wanted-set size and set (not add) the requested metric.dash-spv/src/sync/blocks/sync_manager.rs#L200-L203: keep the tick re-declaration, but ensure it no longer feeds the cumulative counter once the manager side is fixed.🤖 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/sync/blocks/manager.rs` around lines 66 - 75, The requested progress counter is inflated because BlocksPipeline::send_pending returns the full wanted set on every tick; update dash-spv/src/sync/blocks/manager.rs lines 66-75 to stop adding that returned count, instead reporting newly declared hashes or setting the requested metric from the wanted-set size. Preserve the tick re-declaration in dash-spv/src/sync/blocks/sync_manager.rs lines 200-203, ensuring it no longer contributes repeatedly to the cumulative counter.dash-spv/src/sync/block_headers/pipeline.rs (1)
143-158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRe-add the pending-locator guard for empty
headersresponses.
SegmentState::receive_headers(&[])marks the tip segment complete, andhandle_headers_pipelinewill still send more requests because the tip is already complete. If an empty response arrives for an older/unsolicited tip locator, the pipeline can stop mid-catch-up after routing only the latest segment request to a lagging peer. Only route[]when the caller can prove it answers the currently active tip locator, and then release that locator/key.🤖 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/sync/block_headers/pipeline.rs` around lines 143 - 158, The empty-headers branch in handle_headers_pipeline must verify that the response corresponds to the currently active tip locator before routing it. Reintroduce the pending-locator guard, route and complete only the matching tip request, then release its locator/key; ignore unsolicited or stale empty responses without calling SegmentState::receive_headers.dash-spv/src/sync/chainlock/sync_manager.rs (1)
59-73: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnchecked
send_toresult + permanent dedup can silently drop a ChainLock request forever.
chainlocks_to_requestis unconditionally added toself.requested_chainlocksafter callingnetwork.send_to(...), without checking the returned success bool.requested_chainlocksis only cleared inon_disconnect(), which per theSyncManagertrait doc fires only when all peers are lost — not on an individual peer send failure. UnlikeMnListDiff, thisGetDatarequest isn't registered with the broker via aRequestKey, so there's also no timeout/retry safety net (tick()here does "no periodic work"). If the send fails, or the peer never responds, this specific ChainLock hash can never be re-requested even when other peers re-announce it viaInv, until a full disconnect/reconnect cycle.🛡️ Only mark as requested on a successful send
if !chainlocks_to_request.is_empty() { tracing::info!( "Received {} ChainLock announcements, requesting via getdata", chainlocks_to_request.len() ); - network + let sent = network .send_to(peer, NetworkMessage::GetData(chainlocks_to_request.clone())) .await; - - for item in &chainlocks_to_request { - if let Inventory::ChainLock(hash) = item { - self.requested_chainlocks.insert(*hash); - } - } + if sent { + for item in &chainlocks_to_request { + if let Inventory::ChainLock(hash) = item { + self.requested_chainlocks.insert(*hash); + } + } + } }Consider also whether ChainLock
GetDatashould go through the broker's tracked-request path (with aRequestKeyvariant) so a non-responding peer triggers the existing timeout/retry monitor, rather than relying solely on send success.🤖 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/sync/chainlock/sync_manager.rs` around lines 59 - 73, Update the ChainLock request flow around send_to and requested_chainlocks so hashes are inserted only when the GetData send succeeds; handle the returned success value and leave failed sends eligible for later requests. Consider registering ChainLock GetData requests through the broker’s tracked RequestKey path so non-responsive peers receive existing timeout/retry handling, while preserving the current deduplication behavior for successful requests.dash-spv/src/sync/sync_coordinator.rs (1)
191-198: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc: managers no longer receive a request sender.
Line 196 still advertises "A request sender for outgoing network messages"; the context now carries
Arc<dyn NetworkManager>.📝 Proposed fix
/// - A message stream filtered by its subscribed types /// - An event bus subscription for inter-manager events - /// - A request sender for outgoing network messages + /// - A handle to the network manager for declaring outgoing requests /// - A shutdown token for graceful termination🤖 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/sync/sync_coordinator.rs` around lines 191 - 198, Update the documentation for SyncCoordinator::start to remove the outdated request-sender item and describe that each manager receives the network manager context, matching the Arc<dyn NetworkManager> argument now used.dash-spv/src/network/discovery.rs (1)
52-73: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDNS seed resolution has no timeout and runs sequentially, delaying the first fill round.
tokio::net::lookup_hostdoes not time out on its own, so one unresponsive seed blocksget()— and therefore the supervisor's firstfill_round— for as long as the resolver takes, even though the compiled-in seeds are already in hand. Resolving concurrently under a timeout keeps discovery bounded.🛡️ Proposed fix
- let port = network.default_p2p_port(); - for seed in network.dns_seeds() { - match tokio::net::lookup_host((*seed, port)).await { - Ok(iter) => { - let resolved: Vec<SocketAddr> = iter.collect(); - tracing::info!("DNS seed {} returned {} addresses", seed, resolved.len()); - addresses.extend(resolved); - } - Err(e) => { - tracing::warn!("Failed to resolve DNS seed {} (backup source): {}", seed, e); - } - } - } + const DNS_TIMEOUT: Duration = Duration::from_secs(5); + let port = network.default_p2p_port(); + let lookups = network.dns_seeds().iter().map(|seed| async move { + match tokio::time::timeout(DNS_TIMEOUT, tokio::net::lookup_host((*seed, port))).await { + Ok(Ok(iter)) => iter.collect::<Vec<SocketAddr>>(), + Ok(Err(e)) => { + tracing::warn!("Failed to resolve DNS seed {} (backup source): {}", seed, e); + Vec::new() + } + Err(_) => { + tracing::warn!("DNS seed {} timed out (backup source)", seed); + Vec::new() + } + } + }); + for resolved in futures::future::join_all(lookups).await { + addresses.extend(resolved); + }🤖 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/discovery.rs` around lines 52 - 73, Update discover to resolve all network.dns_seeds() concurrently, applying a bounded timeout to each tokio::net::lookup_host operation so one unresponsive seed cannot block discovery indefinitely. Preserve the existing logging for successful resolutions and failures, then merge all resolved addresses with the compiled-in seeds before sorting and deduplicating.
🧹 Nitpick comments (13)
dash-spv/src/sync/blocks/pipeline.rs (1)
104-117: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-tick re-declaration is O(wanted set) messages.
Every
tick(and everyBlocksNeeded) re-sends oneGetDataper wanted hash, so with a large match set this pushes thousands of broker messages per cycle purely to be de-duplicated. Consider tracking declared hashes and only re-declaring on a slower cadence (or in bounded batches).🤖 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/sync/blocks/pipeline.rs` around lines 104 - 117, The send_pending method re-sends every wanted block hash on each tick, causing excessive broker traffic. Track which hashes have already been declared and only send newly wanted hashes, or re-declare existing hashes on a slower cadence or through bounded batches; keep hash_to_height synchronized so removed or fulfilled hashes are no longer tracked, and return the number actually sent.dash-spv/src/client/transactions.rs (1)
33-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
dispatch_localruns on the untracked path too.When
enable_mempool_trackingis false the tx is broadcast at Line 39 and also injected locally at Line 45, even though no mempool manager exists to consume it. Making the two paths mutually exclusive matches the doc comment and avoids a pointless dispatch (or a duplicate send if anything else ever subscribes toTx).♻️ Make the paths exclusive
if !self.config.read().await.enable_mempool_tracking { // Legacy untracked path: fan out to every peer. self.network.broadcast(NetworkMessage::Tx(tx.clone())); + return Ok(()); } // Inject locally so the mempool manager picks it up through handle_tx.🤖 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/client/transactions.rs` around lines 33 - 45, Make the transaction handling branches in the surrounding submission method mutually exclusive: retain the direct peer broadcast for disabled enable_mempool_tracking, and call dispatch_local only when tracking is enabled so the mempool manager processes it. Preserve the existing not-connected error behavior and transaction cloning.dash-spv/src/sync/mempool/manager.rs (1)
804-807: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the shared
test_socket_addresshelper.
crate::test_utils::test_socket_addressalready exists and is imported bydash-spv/src/sync/mempool/sync_manager.rs; the local copy duplicates it (and could drift, e.g. differing port/IP encoding).♻️ Import instead of redefining
- use crate::test_utils::MockNetworkManager; - - /// Deterministic loopback socket address for peer-keyed test state. - fn test_socket_address(id: u8) -> SocketAddr { - SocketAddr::from(([127, 0, 0, id], id as u16)) - } + use crate::test_utils::{test_socket_address, MockNetworkManager};🤖 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/sync/mempool/manager.rs` around lines 804 - 807, Remove the local test_socket_address helper and import and reuse crate::test_utils::test_socket_address wherever it is needed in the mempool manager tests, matching the existing usage in sync_manager.rs. Preserve the current call behavior while eliminating the duplicate implementation.dash-spv/src/sync/mempool/sync_manager.rs (1)
3-11: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning
NetworkMessage::Txwhen handling the owned message.
NetworkMessage::Txholdstransaction::Transactiondirectly, so matching onmsglets the handler receive the transaction by move and removes(*tx).clone().♻️ Match the owned message
- match &msg { - NetworkMessage::Inv(inv) => self.handle_inv(inv, peer, network).await, - NetworkMessage::Tx(tx) => self.handle_tx((*tx).clone(), peer, network).await, - _ => Ok(vec![]), - } + match msg { + NetworkMessage::Inv(inv) => self.handle_inv(&inv, peer, network).await, + NetworkMessage::Tx(tx) => self.handle_tx(*tx, peer, network).await, + _ => Ok(vec![]), + }🤖 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/sync/mempool/sync_manager.rs` around lines 3 - 11, Update the owned-message handling in the sync manager’s message-processing method to match `NetworkMessage::Tx` by value rather than by reference, moving the contained transaction into the handler and removing the `(*tx).clone()` call. Preserve the existing handling for all other `NetworkMessage` variants.dash-spv/src/sync/filters/manager.rs (1)
1005-1007: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDangling doc sentence on
test_network.The first doc line is a truncated leftover from the previous helper.
🧹 Proposed cleanup
- /// A `NetworkManager` that makes no outbound connections and does no - /// An in-memory mock network manager: it swallows any messages the manager + /// An in-memory mock network manager: it swallows any messages the manager /// tries to send, so these tests observe manager state, not sent messages.🤖 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/sync/filters/manager.rs` around lines 1005 - 1007, Remove the truncated first documentation line above the test_network mock helper, leaving only the complete description that identifies it as an in-memory mock network manager.dash-spv/src/sync/filter_headers/sync_manager.rs (1)
125-126: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRe-declaring the full wanted set on every response is avoidable here.
dash-spv/src/sync/filters/sync_manager.rsdeliberately dropssend_pendingfrom the message path (its comment: re-declaring per response re-scans every wanted batch on the hot path) and relies ontickinstead. This path still re-sends oneGetCFHeadersper wanted batch for each arrivingcfheaders, which is O(batches) sends per response. Sincetick(Line 159) already re-declares, consider dropping this call for consistency.♻️ Proposed change
- // Declare any remaining wanted batches to the broker - self.pipeline.send_pending(network).await?; - + // No `send_pending` here: the wanted set is already declared to the broker, + // which paces it out. `tick` re-declares to pick up newly-wanted batches.🤖 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/sync/filter_headers/sync_manager.rs` around lines 125 - 126, Remove the self.pipeline.send_pending(network).await? call from the response-handling path in sync_manager, matching the filters sync manager; rely on the existing tick-based declaration to resend pending wanted batches and avoid scanning and sending the full wanted set for every cfheaders response.dash-spv/src/sync/masternodes/manager.rs (1)
461-469: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale error branch:
send_qrinfo_for_tipcan no longer fail on dispatch.
network.sendis infallible fire-and-forget (see the comment you added at Line 545), so the remaining failure modes ofsend_qrinfo_for_tipare allOkearly returns. The warn text describing a failed dispatch that leavescurrent_cycle_attemptsat 0 is now misleading. Either reword it to cover the real remaining error sources or collapse thematchonce the signature stops being fallible.🤖 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/sync/masternodes/manager.rs` around lines 461 - 469, Update the send_qrinfo_for_tip handling in the catch-up path to reflect that network.send is infallible and dispatch no longer produces errors. Remove the stale failed-dispatch warning and collapse the match if the method signature is made non-fallible, while preserving events extension and the existing early-return behavior.dash-spv/src/sync/sync_manager.rs (1)
106-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the event's
best_heightrather than re-readingnetwork.tip().
PeersUpdatedalready carriesbest_heightfrom the same atomic, so destructuring it keeps the handler consistent with the event it is reacting to and avoids a second source of truth.♻️ Suggested change
- if let NetworkEvent::PeersUpdated { - .. - } = event - { + if let NetworkEvent::PeersUpdated { + best_height, + .. + } = event + { // Seed every manager's target from the peers' advertised tip so the // height shows up right away (matches the pre-network `best_height`). - manager.update_target_height(network.tip()); + manager.update_target_height(*best_height);🤖 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/sync/sync_manager.rs` around lines 106 - 117, Update the PeersUpdated pattern in the event handler to destructure its best_height field and pass that value to manager.update_target_height instead of re-reading network.tip(). Keep the existing WaitingForConnections check and sync startup flow unchanged.dash-spv/src/network/discovery.rs (1)
33-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
expect()here; the cache read can be expressed without it.Coding guidelines say to avoid
unwrap()/expect()in library code. This one is provably safe, butget_or_insert_with-style construction removes it entirely.As per coding guidelines: "Avoid
unwrap()andexpect()in library code; use proper error types (e.g., viathiserror)".♻️ Suggested rewrite
- if self.discovered.is_none() { - let found = Self::discover(self.network).await; - self.discovered = Some(found); - } - self.discovered.as_ref().expect("just set") + match &self.discovered { + Some(found) => found, + None => { + let found = Self::discover(self.network).await; + self.discovered.insert(found) + } + }🤖 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/discovery.rs` around lines 33 - 49, Update the discovered-peer cache handling in get to initialize and borrow self.discovered through a get_or_insert_with-style construction after discovery, eliminating the expect("just set") call while preserving the existing fixed-pool and restrict_to_configured_peers behavior.Source: Coding guidelines
dash-spv/src/network/manager.rs (2)
421-437: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePartial key overlap drops the whole message.
If a
getdatanames several blocks and only one key is already in play, the entire message — including the blocks not yet requested — is discarded, and re-declaring next tick hits the same overlap until the in-play key resolves. Today the blocks pipeline sends one block pergetdata(see the invariant documented indash-spv/src/network/peer.rslines 261-265), so this is latent rather than live, but it is worth either splitting the message per-unseen-key or asserting the one-key invariant here.🤖 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 421 - 437, Update NetworkManager::send to handle partial key overlap without dropping unseen requests: either split the message and enqueue only unseen keys, or enforce the documented one-key-per-getdata invariant with an assertion. Preserve duplicate suppression for keys already in flight and ensure every unseen key remains queued.
1100-1113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRe-locking between the capacity check and the push.
Lines 1105 and 1107 take
connectedtwice for one decision. It is safe today only because the supervisor is the sole writer that pushes and the other tasks only remove, so the count can only shrink. That invariant is easy to break later; holding one guard across check-and-push documents and enforces it.♻️ Suggested tightening
- if acceptable && self.connected.lock().await.len() < self.max_peers { - let addr = peer.addr(); - self.connected.lock().await.push((peer, State {})); - let _ = self.events.send(NetworkEvent::PeerConnected(addr)); - accepted += 1; - } else { - leftover.push(peer); - } + let addr = peer.addr(); + let mut guard = self.connected.lock().await; + if acceptable && guard.len() < self.max_peers { + guard.push((peer, State {})); + drop(guard); + let _ = self.events.send(NetworkEvent::PeerConnected(addr)); + accepted += 1; + } else { + drop(guard); + leftover.push(peer); + }🤖 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 1100 - 1113, Update the peer acceptance loop around connected capacity handling to acquire one `self.connected` lock guard, perform the max-peer check, and push the accepted peer while that guard remains held. Reuse the guard for both the capacity decision and insertion, preserving the existing event emission and leftover behavior.dash-spv/src/network/mod.rs (1)
78-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSelf-named delegation is a silent stack-overflow trap; the TODO is worth acting on.
Every body calls a method with the same name as the trait method it implements. This compiles to the inherent method today, but if any inherent method is later renamed or removed, the call resolves to the trait method instead — infinite recursion at runtime rather than a compile error. Either inline the bodies as the TODO says, or make the delegation explicit with
PeerNetworkManager::send(self, msg).awaitso the inherent path is enforced by the compiler.Want me to open an issue to track inlining these?
🤖 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/mod.rs` around lines 78 - 121, Replace the self-named delegations in the NetworkManager implementation for PeerNetworkManager with explicit PeerNetworkManager::method(self, ...) calls, preserving each method’s arguments, await behavior, and return values so dispatch remains bound to the inherent methods and cannot recurse.dash-spv/src/network/peer.rs (1)
385-407: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMessages received during the lag probe are silently discarded.
The
_ => {}arm drops anything that is notpong/pingin this window, and the framed reader is only handed tospawn_readerafterwards. Nothing is requested yet, so only unsolicited announcements (inv,headers,addr) can be lost, but that includes the tip announcement a peer sends right aftersendheaders. Buffering these and re-injecting them viainboundafter the reader starts would avoid depending on the next announcement.Also,
#[allow(clippy::too_many_arguments)]on line 308 now guards a four-argument function and can go.🤖 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/peer.rs` around lines 385 - 407, Preserve non-probe messages received in the post-handshake lag loop instead of discarding them: buffer unmatched valid payloads, then re-inject them through the existing inbound channel after spawn_reader starts, while continuing to respond to NetworkMessage::Ping and match the expected Pong. Also remove the obsolete #[allow(clippy::too_many_arguments)] attribute from the now four-argument function.
🤖 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 551-554: Update the routing loop around route_tick to snapshot or
clone the connected peer handles while holding connected, then release the mutex
before awaiting any socket writes. Pass the snapshot to route_tick so peer send,
in_flight, and capacity operations remain available without retaining the global
connected_peers guard.
- Around line 886-896: Clamp cap_ema during the additive-increase branch of the
cap_ema update so CAP_GROW cannot raise it above PEER_CEIL. Keep the existing
FLOOR_PER_PEER minimum and multiplicative backoff behavior intact, and ensure
the stored EMA—not only the later cap_peer value—is bounded.
In `@dash-spv/src/network/peer.rs`:
- Around line 513-516: Update the reader termination flow around
PeerEvent::Disconnected and stash_backups so closures of probe-only peers are
identified and suppressed. Ensure disconnected events are sent only for peers
that entered connected_peers, while preserving disconnection events for
established connections.
- Around line 276-288: Update is_single_response to classify
NetworkMessage::NotFound(_) as a single-message response so the reader releases
the peer’s in-flight unit. Also ensure the reader forwards or surfaces notfound
through the owning request pipeline so RequestKey::Block reaches
request_answered and its OnWire entry is cleared.
- Around line 181-199: Update Peer::send so pipeline requests are accounted for
before awaiting write_all, including the in_flight increment and latency.on_send
call. If serialization or writing fails, roll back the in-flight accounting and
corresponding latency state before returning the existing connection error;
leave non-pipeline requests unchanged.
In `@dash-spv/src/sync/block_headers/sync_manager.rs`:
- Around line 192-201: Update the send flow in the sync manager around
NetworkMessage::GetHeaders to capture the boolean result returned by send_to and
insert the peer address into announced_peers only when that result indicates
success. Leave failed sends unrecorded so subsequent retries remain possible.
In `@dash-spv/src/sync/filters/manager.rs`:
- Around line 686-701: Restore use of monitored_filter_elements_for in the
compact-filter matching paths, including rescan_batch and scan_batch, instead of
always passing an empty extra_elements slice to
check_compact_filters_for_elements. Retrieve each wallet’s monitored filter
elements alongside monitored_script_pubkeys_for and pass them as the third
argument so matches based solely on non-script elements are queued.
In `@dash-spv/src/sync/sync_manager.rs`:
- Around line 54-60: Remove the local Inbound type alias and import or re-export
the existing crate::network::Inbound alias instead. Update
SyncManagerTaskContext to use that shared alias for message_receiver, preserving
compatibility with the receiver returned by network.subscribe().
In `@dash-spv/tests/dashd_sync/helpers.rs`:
- Around line 223-251: Update wait_for_mempool_txid to accept a timeout
parameter instead of hard-coding 30 seconds, and use that parameter when
creating the sleep future. Update every transaction test call to pass the
configured MEMPOOL_TIMEOUT value, preserving existing event matching and timeout
behavior.
In `@dash-spv/tests/dashd_sync/tests_mempool.rs`:
- Around line 413-418: Update the remaining NetworkEvent::PeerConnected pattern
in the affected mempool synchronization test, including the occurrence around
the later connection wait, to use the tuple-variant form consistent with the
migrated NetworkEvent definition. Preserve the existing matching behavior and
predicates.
In `@masternode-seeds-fetcher/src/peer.rs`:
- Around line 37-74: Add Tokio loopback tests for the Peer transport methods
connect, send_message, and receive_message. Use a local listener to verify
successful message framing, rejection of foreign-network magic, returning None
after connection closure, and timeout behavior for stalled writes. Exercise the
real TCP path and assert the expected Result or error for each case.
- Around line 67-72: Update receive_message to validate raw.magic against
self.magic before constructing Message from raw.payload; reject
mismatched-network frames with an error instead of exposing their payload, while
preserving the existing decode-error and end-of-stream behavior.
- Around line 55-63: Update send_message to wrap writer.write_all with the
configured I/O timeout, preserving the existing “send message” context and
propagating timeout or write errors through the Result.
---
Outside diff comments:
In `@dash-spv/src/network/discovery.rs`:
- Around line 52-73: Update discover to resolve all network.dns_seeds()
concurrently, applying a bounded timeout to each tokio::net::lookup_host
operation so one unresponsive seed cannot block discovery indefinitely. Preserve
the existing logging for successful resolutions and failures, then merge all
resolved addresses with the compiled-in seeds before sorting and deduplicating.
In `@dash-spv/src/sync/block_headers/pipeline.rs`:
- Around line 143-158: The empty-headers branch in handle_headers_pipeline must
verify that the response corresponds to the currently active tip locator before
routing it. Reintroduce the pending-locator guard, route and complete only the
matching tip request, then release its locator/key; ignore unsolicited or stale
empty responses without calling SegmentState::receive_headers.
In `@dash-spv/src/sync/blocks/manager.rs`:
- Around line 66-75: The requested progress counter is inflated because
BlocksPipeline::send_pending returns the full wanted set on every tick; update
dash-spv/src/sync/blocks/manager.rs lines 66-75 to stop adding that returned
count, instead reporting newly declared hashes or setting the requested metric
from the wanted-set size. Preserve the tick re-declaration in
dash-spv/src/sync/blocks/sync_manager.rs lines 200-203, ensuring it no longer
contributes repeatedly to the cumulative counter.
In `@dash-spv/src/sync/chainlock/sync_manager.rs`:
- Around line 59-73: Update the ChainLock request flow around send_to and
requested_chainlocks so hashes are inserted only when the GetData send succeeds;
handle the returned success value and leave failed sends eligible for later
requests. Consider registering ChainLock GetData requests through the broker’s
tracked RequestKey path so non-responsive peers receive existing timeout/retry
handling, while preserving the current deduplication behavior for successful
requests.
In `@dash-spv/src/sync/sync_coordinator.rs`:
- Around line 191-198: Update the documentation for SyncCoordinator::start to
remove the outdated request-sender item and describe that each manager receives
the network manager context, matching the Arc<dyn NetworkManager> argument now
used.
---
Nitpick comments:
In `@dash-spv/src/client/transactions.rs`:
- Around line 33-45: Make the transaction handling branches in the surrounding
submission method mutually exclusive: retain the direct peer broadcast for
disabled enable_mempool_tracking, and call dispatch_local only when tracking is
enabled so the mempool manager processes it. Preserve the existing not-connected
error behavior and transaction cloning.
In `@dash-spv/src/network/discovery.rs`:
- Around line 33-49: Update the discovered-peer cache handling in get to
initialize and borrow self.discovered through a get_or_insert_with-style
construction after discovery, eliminating the expect("just set") call while
preserving the existing fixed-pool and restrict_to_configured_peers behavior.
In `@dash-spv/src/network/manager.rs`:
- Around line 421-437: Update NetworkManager::send to handle partial key overlap
without dropping unseen requests: either split the message and enqueue only
unseen keys, or enforce the documented one-key-per-getdata invariant with an
assertion. Preserve duplicate suppression for keys already in flight and ensure
every unseen key remains queued.
- Around line 1100-1113: Update the peer acceptance loop around connected
capacity handling to acquire one `self.connected` lock guard, perform the
max-peer check, and push the accepted peer while that guard remains held. Reuse
the guard for both the capacity decision and insertion, preserving the existing
event emission and leftover behavior.
In `@dash-spv/src/network/mod.rs`:
- Around line 78-121: Replace the self-named delegations in the NetworkManager
implementation for PeerNetworkManager with explicit
PeerNetworkManager::method(self, ...) calls, preserving each method’s arguments,
await behavior, and return values so dispatch remains bound to the inherent
methods and cannot recurse.
In `@dash-spv/src/network/peer.rs`:
- Around line 385-407: Preserve non-probe messages received in the
post-handshake lag loop instead of discarding them: buffer unmatched valid
payloads, then re-inject them through the existing inbound channel after
spawn_reader starts, while continuing to respond to NetworkMessage::Ping and
match the expected Pong. Also remove the obsolete
#[allow(clippy::too_many_arguments)] attribute from the now four-argument
function.
In `@dash-spv/src/sync/blocks/pipeline.rs`:
- Around line 104-117: The send_pending method re-sends every wanted block hash
on each tick, causing excessive broker traffic. Track which hashes have already
been declared and only send newly wanted hashes, or re-declare existing hashes
on a slower cadence or through bounded batches; keep hash_to_height synchronized
so removed or fulfilled hashes are no longer tracked, and return the number
actually sent.
In `@dash-spv/src/sync/filter_headers/sync_manager.rs`:
- Around line 125-126: Remove the self.pipeline.send_pending(network).await?
call from the response-handling path in sync_manager, matching the filters sync
manager; rely on the existing tick-based declaration to resend pending wanted
batches and avoid scanning and sending the full wanted set for every cfheaders
response.
In `@dash-spv/src/sync/filters/manager.rs`:
- Around line 1005-1007: Remove the truncated first documentation line above the
test_network mock helper, leaving only the complete description that identifies
it as an in-memory mock network manager.
In `@dash-spv/src/sync/masternodes/manager.rs`:
- Around line 461-469: Update the send_qrinfo_for_tip handling in the catch-up
path to reflect that network.send is infallible and dispatch no longer produces
errors. Remove the stale failed-dispatch warning and collapse the match if the
method signature is made non-fallible, while preserving events extension and the
existing early-return behavior.
In `@dash-spv/src/sync/mempool/manager.rs`:
- Around line 804-807: Remove the local test_socket_address helper and import
and reuse crate::test_utils::test_socket_address wherever it is needed in the
mempool manager tests, matching the existing usage in sync_manager.rs. Preserve
the current call behavior while eliminating the duplicate implementation.
In `@dash-spv/src/sync/mempool/sync_manager.rs`:
- Around line 3-11: Update the owned-message handling in the sync manager’s
message-processing method to match `NetworkMessage::Tx` by value rather than by
reference, moving the contained transaction into the handler and removing the
`(*tx).clone()` call. Preserve the existing handling for all other
`NetworkMessage` variants.
In `@dash-spv/src/sync/sync_manager.rs`:
- Around line 106-117: Update the PeersUpdated pattern in the event handler to
destructure its best_height field and pass that value to
manager.update_target_height instead of re-reading network.tip(). Keep the
existing WaitingForConnections check and sync startup flow unchanged.
🪄 Autofix (Beta)
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: 0b41ee2a-b2f3-4d01-98d2-79dfc157ed95
📒 Files selected for processing (73)
dash-spv-bench/src/main.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/client.rsdash-spv/Cargo.tomldash-spv/examples/filter_sync.rsdash-spv/examples/simple_sync.rsdash-spv/examples/spv_with_wallet.rsdash-spv/src/client/core.rsdash-spv/src/client/event_handler.rsdash-spv/src/client/events.rsdash-spv/src/client/lifecycle.rsdash-spv/src/client/queries.rsdash-spv/src/client/transactions.rsdash-spv/src/lib.rsdash-spv/src/main.rsdash-spv/src/network/addrv2.rsdash-spv/src/network/constants.rsdash-spv/src/network/discovery.rsdash-spv/src/network/event.rsdash-spv/src/network/handshake.rsdash-spv/src/network/manager.rsdash-spv/src/network/message_dispatcher.rsdash-spv/src/network/message_type.rsdash-spv/src/network/mod.rsdash-spv/src/network/peer.rsdash-spv/src/network/pool.rsdash-spv/src/network/reputation.rsdash-spv/src/network/reputation_tests.rsdash-spv/src/network/tests.rsdash-spv/src/storage/mod.rsdash-spv/src/storage/peers.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/sync/blocks/manager.rsdash-spv/src/sync/blocks/pipeline.rsdash-spv/src/sync/blocks/sync_manager.rsdash-spv/src/sync/chainlock/manager.rsdash-spv/src/sync/chainlock/sync_manager.rsdash-spv/src/sync/download_coordinator.rsdash-spv/src/sync/filter_headers/manager.rsdash-spv/src/sync/filter_headers/pipeline.rsdash-spv/src/sync/filter_headers/sync_manager.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/pipeline.rsdash-spv/src/sync/filters/sync_manager.rsdash-spv/src/sync/instantsend/manager.rsdash-spv/src/sync/instantsend/sync_manager.rsdash-spv/src/sync/masternodes/manager.rsdash-spv/src/sync/masternodes/pipeline.rsdash-spv/src/sync/masternodes/sync_manager.rsdash-spv/src/sync/mempool/manager.rsdash-spv/src/sync/mempool/sync_manager.rsdash-spv/src/sync/mod.rsdash-spv/src/sync/sync_coordinator.rsdash-spv/src/sync/sync_manager.rsdash-spv/src/test_utils/network.rsdash-spv/tests/dashd_masternode/setup.rsdash-spv/tests/dashd_sync/helpers.rsdash-spv/tests/dashd_sync/setup.rsdash-spv/tests/dashd_sync/tests_mempool.rsdash-spv/tests/dashd_sync/tests_restart.rsdash-spv/tests/dashd_sync/tests_transaction.rsdash-spv/tests/peer_test.rsdash-spv/tests/test_handshake_logic.rsdash-spv/tests/wallet_integration_test.rsdash/Cargo.tomldash/src/network/message.rsmasternode-seeds-fetcher/Cargo.tomlmasternode-seeds-fetcher/src/main.rsmasternode-seeds-fetcher/src/peer.rsmasternode-seeds-fetcher/src/probe.rs
💤 Files with no reviewable changes (16)
- dash-spv/src/network/pool.rs
- dash-spv/src/network/tests.rs
- dash-spv/src/sync/mod.rs
- dash-spv/src/network/reputation_tests.rs
- dash-spv/tests/test_handshake_logic.rs
- dash-spv/src/network/message_type.rs
- dash-spv/src/network/event.rs
- dash-spv/src/network/constants.rs
- dash-spv/src/storage/peers.rs
- dash-spv/tests/peer_test.rs
- dash-spv/src/storage/mod.rs
- dash-spv/src/network/reputation.rs
- dash-spv/src/network/addrv2.rs
- dash-spv/src/network/message_dispatcher.rs
- dash-spv/src/network/handshake.rs
- dash-spv/src/sync/download_coordinator.rs
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 (3)
dash-spv/src/network/discovery.rs (1)
34-51: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winDon’t cache DNS discovery until
start()succeeds
PeerNetworkManagerowns onePeerDiscovererthrough the client/network lifecycle, and the supervisor only refreshes candidates viadiscoverer.get(). If the first DNS discovery call fails, laterget()calls reuse that failed cache, leavingstart()with only the fixed seed list for the whole manager. Clear the DNS cache onstart()failure or retry fresh discovery when the cached result is not usable.🤖 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/discovery.rs` around lines 34 - 51, Update the PeerDiscoverer lifecycle around get() and the PeerNetworkManager start path so failed DNS discovery is not retained across start attempts. Clear self.discovered when start() fails, or make get() retry discovery when the cached result is unusable, while preserving fixed-peer and restrict_to_configured_peers behavior.dash-spv/src/sync/sync_manager.rs (1)
268-277: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd closed-channel handling when receiving manager messages.
If
context.message_receivercloses,recv()resolves toNoneand this branch does not complete, sotokio::select!continues the same iteration and can re-enterselect!without waiting unless another branch becomes ready. Break from the task when the message receiver closes, matching the existing closedsync_event_receiverandnetwork_event_receiverpaths.🐛 Proposed fix
- Some((peer, message)) = context.message_receiver.recv() => { + msg = context.message_receiver.recv() => { + let Some((peer, message)) = msg else { + tracing::warn!("{} message channel closed, exiting", identifier); + break; + }; tracing::trace!("{} received message: {}", identifier, message.cmd());🤖 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/sync/sync_manager.rs` around lines 268 - 277, Update the `context.message_receiver` branch in the manager task’s `tokio::select!` to handle `recv()` returning `None` by breaking the task loop immediately. Preserve the existing message-processing flow for `Some((peer, message))`, matching the closed-channel handling used by `sync_event_receiver` and `network_event_receiver`.dash-spv/src/sync/blocks/sync_manager.rs (1)
81-109: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake block acceptance recoverable on storage errors.
BlocksPipeline::receive_blockremoves the wanted hash before the manager callsrequest_answered,get_header_height_by_hash, orstore_block. A failed header lookup or persistence path then leaves the block only in memory, with no broker retry, because the request is already marked answered and the wanted entry is gone. Resolve the height and persist the block before removing the wanted entry/correlating the request, or add rollback and requeue handling for these failures.🤖 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/sync/blocks/sync_manager.rs` around lines 81 - 109, Make block handling recoverable by completing header lookup and block persistence before `BlocksPipeline::receive_block` removes the wanted hash and before `network.request_answered` marks the request complete. Alternatively, add rollback and requeue handling so failures from `get_header_height_by_hash` or `store_block` restore the wanted entry and leave the request eligible for retry; preserve the unrequested-block early return.
🧹 Nitpick comments (3)
dash-spv/src/network/discovery.rs (1)
53-84: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winResolve DNS seeds concurrently instead of sequentially.
Each seed in
network.dns_seeds()is resolved one after another, each with its ownDNS_LOOKUP_TIMEOUTof 5 seconds. If several seeds are slow or unreachable, worst-case discovery latency grows linearly with the seed count (up toN × 5s). Run the lookups concurrently, for example withfutures::future::join_all, so the total wait is bounded by the single slowest lookup.♻️ Proposed refactor to resolve DNS seeds concurrently
- let port = network.default_p2p_port(); - for seed in network.dns_seeds() { - match tokio::time::timeout(DNS_LOOKUP_TIMEOUT, tokio::net::lookup_host((*seed, port))) - .await - { - Ok(Ok(iter)) => { - let resolved: Vec<SocketAddr> = iter.collect(); - tracing::info!("DNS seed {} returned {} addresses", seed, resolved.len()); - addresses.extend(resolved); - } - Ok(Err(e)) => { - tracing::warn!("Failed to resolve DNS seed {} (backup source): {}", seed, e); - } - Err(_) => { - tracing::warn!( - "DNS seed {} did not resolve within {:?} (backup source)", - seed, - DNS_LOOKUP_TIMEOUT - ); - } - } - } + let port = network.default_p2p_port(); + let lookups = network.dns_seeds().iter().map(|seed| async move { + let result = + tokio::time::timeout(DNS_LOOKUP_TIMEOUT, tokio::net::lookup_host((*seed, port))) + .await; + (seed, result) + }); + for (seed, result) in futures::future::join_all(lookups).await { + match result { + Ok(Ok(iter)) => { + let resolved: Vec<SocketAddr> = iter.collect(); + tracing::info!("DNS seed {} returned {} addresses", seed, resolved.len()); + addresses.extend(resolved); + } + Ok(Err(e)) => { + tracing::warn!("Failed to resolve DNS seed {} (backup source): {}", seed, e); + } + Err(_) => { + tracing::warn!( + "DNS seed {} did not resolve within {:?} (backup source)", + seed, + DNS_LOOKUP_TIMEOUT + ); + } + } + }🤖 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/discovery.rs` around lines 53 - 84, Update discover to resolve all entries from network.dns_seeds() concurrently, using join_all or an equivalent futures combinator, while retaining the per-seed DNS_LOOKUP_TIMEOUT and existing success, warning, address aggregation, sorting, and deduplication behavior. Ensure discovery waits only for the slowest lookup rather than processing seeds sequentially.dash-spv/src/sync/blocks/sync_manager.rs (1)
112-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign retry comments with the actual tick behavior.
Line 201 passes a network handle, but Lines 202-203 only process buffered blocks. Lines 112-115 claim that pending work is “topped up on tick”.
dash-spv/src/sync/blocks/pipeline.rsLines 104-108 make the same claim.The network manager already requeues work on timeout and peer disconnect. Remove the obsolete tick claims, or restore the tick declaration. Keep retry ownership explicit.
This finding is based on the supplied
NetworkManagertimeout and disconnect handling.Also applies to: 201-203
🤖 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/sync/blocks/sync_manager.rs` around lines 112 - 115, Update the comments around sync_manager’s process_buffered_blocks call and the corresponding pipeline.rs comments to match the actual tick behavior: remove claims that pending work is declared or topped up on tick unless the implementation is changed to do so. Keep retry ownership explicit by documenting that NetworkManager handles timeout and peer-disconnect requeueing, and ensure the comments accurately describe the network handle passed at the tick path.dash-spv/src/sync/blocks/pipeline.rs (1)
102-123: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd broker request-correlation tests.
Lines 111-123 replace coordinator-managed request tracking with repeated one-block
GetDatadeclarations. The changed test indash-spv/src/sync/blocks/manager.rscheckssent_messages(), but the new contract also requires the matchingRequestKey::Blockto be answered and duplicate declarations to remain de-duplicated.Add an in-module asynchronous unit test and an integration test for response handling, duplicate responses, timeout retries, and peer-disconnect retries.
As per path instructions, new functionality in
dash-spvrequires comprehensive in-module unit tests and integration tests.🤖 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/sync/blocks/pipeline.rs` around lines 102 - 123, Add comprehensive asynchronous unit and integration coverage for the broker contract exercised by send_pending: verify each block declaration is correlated with RequestKey::Block, duplicate declarations remain de-duplicated, and valid responses complete requests without duplicate-response effects. Also cover timeout retries and peer-disconnect retries, asserting re-declarations and resulting sent messages through the existing test helpers.Source: Path instructions
🤖 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/chainlock/sync_manager.rs`:
- Around line 64-71: Update the ChainLock request tracking around the successful
GetData send in the sync manager so entries in requested_chainlocks cannot
remain indefinitely when no CLSig arrives. Add expiry and retry handling for
tracked hashes, or route these requests through the existing request broker
using RequestKey::ChainLock and clear the entry from process_chainlock when the
response is processed.
In `@dash-spv/src/sync/filters/sync_manager.rs`:
- Around line 225-246: Add focused in-module tests for the rescan decision
around the logic in the sync manager’s restart path, covering restart_at equal
to committed, greater than committed, and scan_floor overriding stale_min_synced
+ 1; assert that only eligible cases reset active filter batches. Add an
integration test that drives the complete rescan flow through start_download and
verifies the resulting reset and scan behavior.
---
Outside diff comments:
In `@dash-spv/src/network/discovery.rs`:
- Around line 34-51: Update the PeerDiscoverer lifecycle around get() and the
PeerNetworkManager start path so failed DNS discovery is not retained across
start attempts. Clear self.discovered when start() fails, or make get() retry
discovery when the cached result is unusable, while preserving fixed-peer and
restrict_to_configured_peers behavior.
In `@dash-spv/src/sync/blocks/sync_manager.rs`:
- Around line 81-109: Make block handling recoverable by completing header
lookup and block persistence before `BlocksPipeline::receive_block` removes the
wanted hash and before `network.request_answered` marks the request complete.
Alternatively, add rollback and requeue handling so failures from
`get_header_height_by_hash` or `store_block` restore the wanted entry and leave
the request eligible for retry; preserve the unrequested-block early return.
In `@dash-spv/src/sync/sync_manager.rs`:
- Around line 268-277: Update the `context.message_receiver` branch in the
manager task’s `tokio::select!` to handle `recv()` returning `None` by breaking
the task loop immediately. Preserve the existing message-processing flow for
`Some((peer, message))`, matching the closed-channel handling used by
`sync_event_receiver` and `network_event_receiver`.
---
Nitpick comments:
In `@dash-spv/src/network/discovery.rs`:
- Around line 53-84: Update discover to resolve all entries from
network.dns_seeds() concurrently, using join_all or an equivalent futures
combinator, while retaining the per-seed DNS_LOOKUP_TIMEOUT and existing
success, warning, address aggregation, sorting, and deduplication behavior.
Ensure discovery waits only for the slowest lookup rather than processing seeds
sequentially.
In `@dash-spv/src/sync/blocks/pipeline.rs`:
- Around line 102-123: Add comprehensive asynchronous unit and integration
coverage for the broker contract exercised by send_pending: verify each block
declaration is correlated with RequestKey::Block, duplicate declarations remain
de-duplicated, and valid responses complete requests without duplicate-response
effects. Also cover timeout retries and peer-disconnect retries, asserting
re-declarations and resulting sent messages through the existing test helpers.
In `@dash-spv/src/sync/blocks/sync_manager.rs`:
- Around line 112-115: Update the comments around sync_manager’s
process_buffered_blocks call and the corresponding pipeline.rs comments to match
the actual tick behavior: remove claims that pending work is declared or topped
up on tick unless the implementation is changed to do so. Keep retry ownership
explicit by documenting that NetworkManager handles timeout and peer-disconnect
requeueing, and ensure the comments accurately describe the network handle
passed at the tick path.
🪄 Autofix (Beta)
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: d80f18c0-f92b-4cb7-ad93-f8a2fe9394dd
📒 Files selected for processing (16)
dash-spv/src/network/discovery.rsdash-spv/src/network/manager.rsdash-spv/src/network/peer.rsdash-spv/src/sync/block_headers/sync_manager.rsdash-spv/src/sync/blocks/manager.rsdash-spv/src/sync/blocks/pipeline.rsdash-spv/src/sync/blocks/sync_manager.rsdash-spv/src/sync/chainlock/sync_manager.rsdash-spv/src/sync/filter_headers/sync_manager.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/sync_manager.rsdash-spv/src/sync/masternodes/sync_manager.rsdash-spv/src/sync/mempool/manager.rsdash-spv/src/sync/mempool/sync_manager.rsdash-spv/src/sync/sync_coordinator.rsdash-spv/src/sync/sync_manager.rs
💤 Files with no reviewable changes (4)
- dash-spv/src/sync/block_headers/sync_manager.rs
- dash-spv/src/sync/filter_headers/sync_manager.rs
- dash-spv/src/sync/masternodes/sync_manager.rs
- dash-spv/src/sync/filters/manager.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- dash-spv/src/sync/sync_coordinator.rs
- dash-spv/src/network/manager.rs
- dash-spv/src/sync/mempool/sync_manager.rs
- dash-spv/src/sync/mempool/manager.rs
github-actions
Bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Few questions and bunch of AI bullet points that should be addressed/looked at.
- It seems like this continuously does ~2800 connection attempts per hour to only a handful of nodes without any limits to probe connections?
- Why did you stop requesting compressed headers? (its now 2000/message instead of 8000/message)
- Why was the reputation handling removed?
- I still often get into the states where the sync doesn't kick in after start it takes a minute or so until something happens.
- Seems like you stopped persisting the peers? Not sure how often that would be relevant but would it make sense to always first retry the last good peers (maybe unless they are older than a week or a few) first since they most likely might be around still?
- Why did you remove the getaddr address collection? This means we will only ever know about the masternode list and not about any other peers in the network.. which might be not a big deal for now but i wonder why you removed it instead of making use of it.
- Also i noticed that it quite fequently
Please have a look at the bullet points below i think most of them should be addressed.
Blockers
1. One bad headers batch permanently wedges header sync
sync/block_headers/manager.rs:152 calls network.request_answered(...) before validation. take_ready_to_store() then removes the headers from the segment via take_buffered() (advancing the segment cursor), and only afterwards does store_headers (manager.rs:108) run the PoW and continuity check. If validation fails, the manager run loop logs and swallows the error, the headers are gone from the pipeline, and storage never advanced. Every subsequent batch now trips the Segment chain break guard at manager.rs:180, forever. Any peer can trigger this once with a batch that links correctly but fails PoW, and (see blocker 2) there is no way to get rid of that peer.
Suggested fix: validate before calling request_answered, and on a store failure either re-buffer the headers or reset the segment instead of silently advancing.
2. All peer misbehavior handling was removed with no replacement
reputation.rs (scoring, 24h bans, escalation), storage/peers.rs, and the disconnect_peer API are deleted, and the new NetworkManager trait has no eviction method at all. The only remaining eviction is the silence-based timeout monitor. A peer that answers promptly with garbage (invalid PoW headers, cfheaders failing the chain check) keeps sending bytes, is judged live, and is never dropped. Because the supervisor ranks purely on handshake ping, a fast malicious peer is actively preferred over honest slower ones. Sync managers that detect invalid data can only log it (sync/filters/sync_manager.rs:121 even has a TODO: should we penalize the peer a bit?).
Suggested fix: at minimum a disconnect_peer equivalent on the trait plus a session-scoped deny list, so validation failures have consequences. Persisted bans are optional for an SPV client, in-session eviction is not.
3. Responses that never correlate become a permanent kick-a-healthy-peer-every-10s loop
request_answered is the only code that ever removes a key from the registry: there is no cancel, no attempt cap, no terminal state (the Registry doc at network/manager.rs:91 refers to a cancel method that does not exist). Several managers deliberately withhold the answer so the broker will retry:
sync/masternodes/sync_manager.rs:345: mnlistdiff whose header is not yet stored, comment says "do not answer the broker so the broker's timeout/retry re-sends it".sync/block_headers/manager.rs:152-161: headers answered only whenmatched.is_some(), which fails afterreset_tip_segment().sync/blocks/sync_manager.rs:82-89: a block rejected as unrequested returns before answering.
But the broker's only retry path re-queues the message verbatim and closes the peer that responded correctly. If the retried request keeps producing the same non-correlating response (stale locator, header still absent), the loop never ends: one healthy peer is evicted every 10 seconds, indefinitely, and the registry entry is never freed.
Suggested fix: decouple retry from eviction, add a bounded attempt count and a terminal state, and give managers an explicit way to cancel or re-key a request.
4. One non-reading peer can freeze the whole network layer, unrecoverably
The router locks the global connected_peers mutex and holds it across every socket write in the round (network/manager.rs:554 into route_tick, then peer.rs:204 write_all with no write timeout). A peer that stays TCP-alive but stops reading eventually makes write_all pend forever. The router then holds the global lock forever, and everything that needs it blocks behind it: the timeout monitor (the very component that would kick that peer), the peer supervisor, the bandwidth controller, and the pump's disconnect handling. broadcast() (manager.rs:492-500) has the same shape.
Suggested fix: move socket writes out of the connected lock (snapshot the peers, or per-peer send channels) and put a timeout on writes.
5. stop() then run() silently bricks the client
SyncCoordinator::start moves every manager out with .take() and never restores them, and both the coordinator's and the network manager's CancellationTokens are created once and never recreated. After any stop():
- a second
start()passes thetasks.is_empty()guard, spawns zero managers, and returnsOk(()), PeerNetworkManager::start()spawns a supervisor on an already-cancelled token, which still connects one batch of peers before exiting, while the router, pump, and timeout monitor (spawned innew()) are already dead,runningflips to true and the client reports a peer count while never syncing.
Reachable through the documented FFI lifecycle (background stop, foreground run) and through dash_spv_ffi_client_clear_storage, which calls stop() unconditionally. Related: a stop() completing before start() begins is erased by stop_requested.store(false) at client/lifecycle.rs:190, and the FFI's 5s abort path can leave monitor tasks invoking FFIEventCallbacks after dash_spv_ffi_client_stop returned, violating the documented user_data lifetime (use-after-free risk).
Suggested fix: make teardown reversible (re-create tokens, restore managers) or make start() after stop() return a hard error, and cover stop→run→sync in a dashd integration test.
6. Peer identity is address-only, so a stale disconnect event evicts a live connection
The pump handles disconnects as guard.retain(|(peer, _)| peer.addr() != addr) (network/manager.rs:1509) with no connection identity. Two producers of delayed close events exist: stash_backups closes probed peers whose addresses return to the candidate pool seconds later, and retire_drained keeps a displaced peer alive up to 90s. PeerEvent::Disconnected shares the unbounded channel with every cfilter, so under filter-sync load the stale event can drain long after the supervisor reconnected to the same address, removing the new live connection: leaked reader task, spurious PeerDisconnected to the sync managers, healthy requests re-queued. The retire path also never actually drains, because request_completed resolves the peer via connected_peers, which a retired peer has left, so its in-flight never reaches zero and it always burns the full 90s.
Suggested fix: a monotonically increasing connection id carried in PeerEvent and compared before eviction, and make request_completed reach retired peers.
Major
7. The request timeout mis-attributes stalls in both directions
The registry-age rule (network/manager.rs:1305) refreshes last_progress only for cfilters, so a getheaders or block getdata queued behind megabytes of filter stream on a fast high-cap peer routinely exceeds the 10s REQUEST_TIMEOUT and gets that healthy peer killed mid-stream (the peers most likely to be kicked are the fastest ones). Conversely, the refresh at manager.rs:1428-1442 touches every on-wire cfilters request of that peer on any inbound cfilter, so a peer streaming batches B and C while silently never sending batch A keeps A's deadline perpetually refreshed. A's key stays in the registry, so the pipeline's re-declaration is deduped away too, and the store frontier stalls for as long as the peer keeps talking.
8. Unbounded out-of-order buffers replaced the deleted concurrency caps
MAX_CONCURRENT_FILTER_BATCHES and MAX_CONCURRENT_BLOCK_DOWNLOADS (20 each) died with the DownloadCoordinator and nothing bounds completed-but-unconsumed work. filters/manager.rs:347 buffers every completed batch until next_batch_to_store arrives, and blocks/pipeline.rs:161 drains downloaded only in height order. With one frontier batch stuck (finding 7 is a ready trigger), the buffers grow toward the whole remaining sync range, full 2MB block bodies included. In an embedded/mobile context that is an OOM kill rather than a slowdown.
9. In-flight accounting and the cap controller's inputs are corruptible
The reader decrements in_flight and pops a latency sample for any inbound headers/headers2/cfheaders/block with no check that a request was outstanding (peer.rs:497-502). Since the handshake requests sendheaders, every unsolicited tip announcement steals a decrement from a real outstanding request and consumes the oldest latency timestamp, inflating the completion rate and producing near-zero service-time samples that pin min_w and decay the peer's cap to the floor (manager.rs:877-895). Separately: when the same getcfilters ends up at two peers (routine after retries), only the peer delivering the final filter is credited, the other's slot leaks until the timeout monitor kills it, and filters/sync_manager.rs:114-125 returns early on a cfilter for an unknown block hash without ever freeing the slot.
10. Total peer loss is invisible to the sync layer
PeersUpdated is emitted only when peers are accepted or swapped (manager.rs:1140, :1196), never on disconnect, so connected_count: 0 is unreachable. The stop_sync() on zero peers at sync/block_headers/sync_manager.rs:210-212 can never fire and WaitingForConnections is never re-entered, so recovery after losing every peer rests entirely on the broker's re-queue path, untested against real dashd. FFI consumers bound to on_peers_updated see a count that never decreases.
11. broadcast_transaction can return Ok(()) having sent nothing
broadcast became a detached fire-and-forget spawn discarding all write errors (manager.rs:492-500). On the untracked path every peer write can fail with success still returned, and broadcast_transaction followed promptly by stop() can cancel the peers before the spawned send runs, silently dropping the transaction. The old implementation returned an error when all peer writes failed.
12. Mainnet bootstrap became fragile: seeds-only, one-shot DNS, stricter service gate
Three compounding changes plus one bug:
- Peers must now advertise
COMPACT_FILTERSwhen filters are enabled (manager.rs:297-299). In Dash Core that flag is off by default (DEFAULT_PEERBLOCKFILTERS = false) and effectively only masternodes set it, so most non-seed addresses are discarded after a full handshake. addr/addrv2learning is gone entirely: nogetaddris ever sent, inbound address messages are decoded and dropped (noMessageTypevariant), yet the handshake still advertisessendaddrv2.- Peer persistence (
storage/peers.rs) is gone and DNS seeds are resolved once per process (discovery.rs:41-45). - Bug:
discovery.rs:36checksfixedbeforerestrict_to_configured_peers, so configuring a single peer silently disables all discovery even with the restrict flag off.
Net effect: a stale seed list plus one bad DNS round is a hard bootstrap failure with no recovery path. The service gate is defensible, but the combination deserves an explicit decision, plus at least periodic DNS re-resolution or restored address learning, and a fix for the discovery gate ordering.
13. DIP-0025 compressed headers silently dropped
build_version advertises ServiceFlags::NONE where the old handshake advertised NODE_HEADERS_COMPRESSED and negotiated sendheaders2. dashd gates every headers2 decision on that flag, so full header sync now runs uncompressed. Self-consistent (nothing handles headers2 anymore, and the GetHeaders2 branches in classify/request_keys are dead code), but it is a real bandwidth regression on mainnet header sync that the PR never mentions.
14. The new timeout/retry machinery has zero tests, and the mock can't catch its bugs
Test count goes 572 to 508. All 12 old pipeline timeout/retry/requeue tests died with the code they tested, and the broker machinery that replaced them (timeout monitor, culprit selection, disconnect re-queue, the last_progress refresh that the branch's own 5854b15ed fixed a live bug in) has no tests at all. MockNetworkManager records sends without dedup, timeouts, or request lifecycle, so a manager that mis-uses the broker contract looks correct in every sync test and fails only in production. At minimum the monitor and re-queue paths need unit tests, and the mock should honor the RequestKey dedup contract.
15. Downstream API breakage needs a coordinated update
Loud compile errors rather than silent, but worth planning: network::manager is now private while downstream code imports PeerNetworkManager from it, NetworkEvent variants changed shape (PeersUpdated lost addresses, connect/disconnect became tuple variants), and DashSpvClient::disconnect_peer is gone with no replacement. The FFI C ABI itself is unchanged (all 44 exported symbols identical).
704f1fb to
f71cac0
Compare
github-actions
Bot
removed
merge-conflict
There was a problem hiding this comment.
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/sync/filters/manager.rs (1)
2823-2823: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove literal peer addresses from broker-path tests.
All three tests use the same literal peer endpoint. Inject the peer value through shared test setup.
dash-spv/src/sync/filters/manager.rs#L2823-L2823: replace the literal peer address with the injected test peer.dash-spv/src/sync/filters/manager.rs#L2891-L2891: replace the literal peer address with the injected test peer.dash-spv/src/sync/filters/manager.rs#L2931-L2931: replace the literal peer address with the injected test peer.As per coding guidelines, “Never hardcode network parameters, addresses, or keys.”
🤖 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/sync/filters/manager.rs` at line 2823, Replace the hardcoded peer endpoint in the broker-path tests at dash-spv/src/sync/filters/manager.rs lines 2823, 2891, and 2931 with the peer value provided by the shared test setup. Ensure all three tests use the injected test peer consistently.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@dash-spv/src/sync/filters/manager.rs`:
- Line 2823: Replace the hardcoded peer endpoint in the broker-path tests at
dash-spv/src/sync/filters/manager.rs lines 2823, 2891, and 2931 with the peer
value provided by the shared test setup. Ensure all three tests use the injected
test peer consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 859e6436-ebf3-4acc-aced-57f690e7dc75
📒 Files selected for processing (4)
dash-spv/src/sync/filters/manager.rsdash-spv/src/sync/instantsend/manager.rsdash-spv/src/sync/instantsend/sync_manager.rsdash-spv/tests/dashd_sync/tests_transaction.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- dash-spv/src/sync/instantsend/sync_manager.rs
- dash-spv/tests/dashd_sync/tests_transaction.rs
2 participants
The manager owns de-duplication, pacing, timeouts, retries and peer hot-swap, so the
per-pipeline
DownloadCoordinatoris gone.getheaders,getcfheaders,getcfilters,getmnlistdiff, blockgetdata) carries aRequestKey. Re-declaring onealready queued or on the wire is a no-op, which is what makes it safe for a pipeline to
re-declare its whole wanted set freely.
blocks, filters, filter headers, block headers — so a backlog of one type never blocks
another behind it. Messages popped but not sent go back to the front of their class.
measured downlink throughput, and each peer's cap is sized by Little's Law from its own
completion rate and service time. Fast peers earn a high cap, slow peers a low one.
that ignored it is dropped; the requester reports success via
request_answered, so onlythe caller can retire a request (an unsolicited response can't clear someone else's).
handshake latency and swaps out a slow peer for a clearly faster one, keeping displaced
peers alive until their in-flight work drains.
what each asked for, handing over the payload without a copy when it has a single consumer.
NetworkManagertrait plus an in-memoryMockNetworkManager; the client is generic over the network (DashSpvClient<W, N, S>).dispatcher / handshake / pool modules.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes