refactor: default to relay=false in P2P handshake#536
Open
xdustinface wants to merge 1 commit intov0.42-devfrom
Open
refactor: default to relay=false in P2P handshake#536xdustinface wants to merge 1 commit intov0.42-devfrom
relay=false in P2P handshake#536xdustinface wants to merge 1 commit intov0.42-devfrom
Conversation
Contributor
📝 WalkthroughWalkthroughRemoved the MempoolStrategy dependency from handshake and peer network management. HandshakeManager::new now takes (Network, Option) and build_version_message() sets relay = false. PeerNetworkManager dropped its mempool_strategy field and related uses. Tests updated to the new HandshakeManager signature. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #536 +/- ##
=============================================
- Coverage 66.89% 66.82% -0.07%
=============================================
Files 313 313
Lines 64753 64746 -7
=============================================
- Hits 43317 43269 -48
- Misses 21436 21477 +41
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (2)
dash-spv/src/network/handshake.rs (1)
47-58:⚠️ Potential issue | 🟠 MajorHardcoded
relay=falseremoves runtime policy and risks mempool regression.
HandshakeManagerno longer accepts any relay policy andbuild_version_message()always sendsrelay: false. That can break expected mempool behavior for configs that still enable tracking.Proposed fix (make relay explicit and configurable)
pub struct HandshakeManager { _network: Network, @@ user_agent: Option<String>, + relay: bool, } @@ - pub fn new(network: Network, user_agent: Option<String>) -> Self { + pub fn new(network: Network, user_agent: Option<String>, relay: bool) -> Self { Self { _network: network, @@ version_sent: false, user_agent, + relay, } } @@ - relay: false, // SPV client enables this on demand + relay: self.relay,Then pass a policy from
PeerNetworkManager(for example derived fromClientConfig::enable_mempool_tracking/mempool_strategy) instead of dropping it.Also applies to: 288-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/network/handshake.rs` around lines 47 - 58, The handshake currently hardcodes relay=false in build_version_message; update HandshakeManager to store an explicit relay policy and thread it through construction and message building: add a field (e.g., relay: bool or relay_policy enum) to the HandshakeManager struct, change HandshakeManager::new to accept a relay parameter (derived upstream from PeerNetworkManager/ClientConfig::enable_mempool_tracking or mempool_strategy) and set that field, and update build_version_message() to use self.relay instead of the hardcoded false; also ensure the same fix is applied to the other constructor/usage mentioned around the second occurrence (line ~288) so all instantiations pass the configured policy.dash-spv/tests/handshake_test.rs (1)
10-43:⚠️ Potential issue | 🟠 MajorNetwork integration test currently allows false positives.
If the connection fails, this test logs and exits without failing, so handshake regressions can pass CI unnoticed. Since this requires a live node, mark it
#[ignore]and fail when explicitly run.As per coding guidelines, "Mark network-dependent or long-running tests with `#[ignore]` attribute; run with `-- --ignored`."Proposed fix
#[tokio::test] +#[ignore = "requires a Dash Core node at 127.0.0.1:9999"] async fn test_handshake_with_mainnet_peer() { @@ - Err(e) => { - println!("✗ Handshake failed with peer {}: {}", peer_addr, e); - // For CI/testing environments where the peer might not be available, - // we'll make this a warning rather than a failure - println!("Note: This test requires a Dash Core node running at 127.0.0.1:9999"); - println!("Error details: {}", e); - } + Err(e) => { + panic!("Handshake failed with peer {}: {}", peer_addr, e); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/tests/handshake_test.rs` around lines 10 - 43, Mark the network-dependent async test function test_handshake_with_mainnet_peer with both #[tokio::test] and #[ignore] so it only runs when explicitly requested, and change the Err(e) branch (where Peer::connect fails) from printing/warning to failing the test (panic! or assert!) so the test will surface regressions when run; keep the rest of the logic using Peer::connect and HandshakeManager::perform_handshake unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@dash-spv/src/network/handshake.rs`:
- Around line 47-58: The handshake currently hardcodes relay=false in
build_version_message; update HandshakeManager to store an explicit relay policy
and thread it through construction and message building: add a field (e.g.,
relay: bool or relay_policy enum) to the HandshakeManager struct, change
HandshakeManager::new to accept a relay parameter (derived upstream from
PeerNetworkManager/ClientConfig::enable_mempool_tracking or mempool_strategy)
and set that field, and update build_version_message() to use self.relay instead
of the hardcoded false; also ensure the same fix is applied to the other
constructor/usage mentioned around the second occurrence (line ~288) so all
instantiations pass the configured policy.
In `@dash-spv/tests/handshake_test.rs`:
- Around line 10-43: Mark the network-dependent async test function
test_handshake_with_mainnet_peer with both #[tokio::test] and #[ignore] so it
only runs when explicitly requested, and change the Err(e) branch (where
Peer::connect fails) from printing/warning to failing the test (panic! or
assert!) so the test will surface regressions when run; keep the rest of the
logic using Peer::connect and HandshakeManager::perform_handshake unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a33f8cfc-774f-4918-9cf5-941b11b3773c
📒 Files selected for processing (4)
dash-spv/src/network/handshake.rsdash-spv/src/network/manager.rsdash-spv/tests/handshake_test.rsdash-spv/tests/test_handshake_logic.rs
xdustinface
force-pushed
the
refactor/relay-false
branch
from
38df8b7 to
72c757c
Compare
Contributor
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 (2)
dash-spv/tests/test_handshake_logic.rs (1)
8-15:⚠️ Potential issue | 🟡 MinorAdd a regression test for the new relay default.
This update only follows the constructor signature change; it still does not assert the behavior changed by the PR. Please add coverage that the emitted VERSION message defaults
relaytofalseso this cannot regress silently. Best placed as a#[cfg(test)]unit test next tobuild_version_message().As per coding guidelines, "Write unit tests for new functionality in Rust code" and "Place unit tests alongside code with
#[cfg(test)]attribute".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/tests/test_handshake_logic.rs` around lines 8 - 15, Add a unit test (#[cfg(test)]) next to build_version_message() that constructs a Version message via build_version_message() (or the public API that emits the VERSION message from HandshakeManager) and asserts that the emitted message's relay field defaults to false; reference build_version_message() and HandshakeManager::build_version_message (or the function used to produce the VERSION message) and check the Version object's relay boolean (e.g., version_msg.relay == false) to prevent regressions.dash-spv/src/network/handshake.rs (1)
47-58:⚠️ Potential issue | 🟠 MajorDon't turn a default flip into a hardcoded behavior.
Removing the strategy from
HandshakeManager::newand hardcodingrelay: falsemakes the handshake path unable to express any opt-in at all. In the manager flow shown atdash-spv/src/network/manager.rs:256-266, every outbound handshake now sends the same relay setting, so the distinction fromMempoolStrategy::{FetchAll, BloomFilter}indash-spv/src/client/config.rs:14-19is gone at handshake time. Please keepnew()defaulting tofalse, but preserve an explicit override onHandshakeManagerso full-tx relay can still be enabled when needed.🔧 Suggested shape
pub struct HandshakeManager { _network: Network, state: HandshakeState, our_version: u32, peer_version: Option<u32>, peer_services: Option<ServiceFlags>, version_received: bool, verack_received: bool, version_sent: bool, + relay: bool, user_agent: Option<String>, } impl HandshakeManager { pub fn new(network: Network, user_agent: Option<String>) -> Self { - Self { + Self::with_relay(network, false, user_agent) + } + + pub fn with_relay(network: Network, relay: bool, user_agent: Option<String>) -> Self { + Self { _network: network, state: HandshakeState::Init, our_version: constants::PROTOCOL_VERSION, peer_version: None, peer_services: None, version_received: false, verack_received: false, version_sent: false, + relay, user_agent, } } } ... - relay: false, + relay: self.relay,Also applies to: 287-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/network/handshake.rs` around lines 47 - 58, Handshake::new currently drops the relay option and hardcodes relay behavior; restore a default relay: false in the Handshake struct initialization inside Handshake::new, and preserve a way for HandshakeManager to explicitly override it when constructing/starting a handshake. Concretely: add relay: false back into the returned Self in Handshake::new (ensuring the Handshake struct still has a relay field), and update HandshakeManager (the code that creates the Handshake) to set handshake.relay (or pass the relay value into the Handshake constructor) based on the client config/mempool strategy (MempoolStrategy::{FetchAll, BloomFilter}) so full-tx relay can still be enabled when required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@dash-spv/src/network/handshake.rs`:
- Around line 47-58: Handshake::new currently drops the relay option and
hardcodes relay behavior; restore a default relay: false in the Handshake struct
initialization inside Handshake::new, and preserve a way for HandshakeManager to
explicitly override it when constructing/starting a handshake. Concretely: add
relay: false back into the returned Self in Handshake::new (ensuring the
Handshake struct still has a relay field), and update HandshakeManager (the code
that creates the Handshake) to set handshake.relay (or pass the relay value into
the Handshake constructor) based on the client config/mempool strategy
(MempoolStrategy::{FetchAll, BloomFilter}) so full-tx relay can still be enabled
when required.
In `@dash-spv/tests/test_handshake_logic.rs`:
- Around line 8-15: Add a unit test (#[cfg(test)]) next to
build_version_message() that constructs a Version message via
build_version_message() (or the public API that emits the VERSION message from
HandshakeManager) and asserts that the emitted message's relay field defaults to
false; reference build_version_message() and
HandshakeManager::build_version_message (or the function used to produce the
VERSION message) and check the Version object's relay boolean (e.g.,
version_msg.relay == false) to prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afe7ba6a-bb24-4b57-a5b0-09c1e11d8849
📒 Files selected for processing (4)
dash-spv/src/network/handshake.rsdash-spv/src/network/manager.rsdash-spv/tests/handshake_test.rsdash-spv/tests/test_handshake_logic.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv/src/network/manager.rs
Just always default to `false`. The new coming mempool implementation will enable it on demand.
xdustinface
force-pushed
the
refactor/relay-false
branch
from
72c757c to
f819a4d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dash-spv/tests/handshake_test.rs (1)
19-20: Consider simpler string conversion.Using
.parse().unwrap()to convert&strtoStringis unusual. A more idiomatic approach would be:✨ Suggested simplification
- let mut handshake_manager = - HandshakeManager::new(Network::Mainnet, Some("handshake_test".parse().unwrap())); + let mut handshake_manager = + HandshakeManager::new(Network::Mainnet, Some("handshake_test".to_string()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/tests/handshake_test.rs` around lines 19 - 20, The test uses an odd `.parse().unwrap()` to convert a string slice when constructing the HandshakeManager: replace the `Some("handshake_test".parse().unwrap())` argument with a straightforward String conversion (e.g., `Some("handshake_test".to_string())` or `Some(String::from("handshake_test"))`) when calling `HandshakeManager::new(Network::Mainnet, ...)` to make the code idiomatic and clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dash-spv/tests/handshake_test.rs`:
- Around line 19-20: The test uses an odd `.parse().unwrap()` to convert a
string slice when constructing the HandshakeManager: replace the
`Some("handshake_test".parse().unwrap())` argument with a straightforward String
conversion (e.g., `Some("handshake_test".to_string())` or
`Some(String::from("handshake_test"))`) when calling
`HandshakeManager::new(Network::Mainnet, ...)` to make the code idiomatic and
clearer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 804d2b34-ba94-413f-ad35-b03e2b5bd299
📒 Files selected for processing (4)
dash-spv/src/network/handshake.rsdash-spv/src/network/manager.rsdash-spv/tests/handshake_test.rsdash-spv/tests/test_handshake_logic.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv/src/network/manager.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Just always default to
false. The new coming mempool implementation will enable it on demand.Summary by CodeRabbit
Refactor
Tests