v3: Improvements from testing sessions #7209
Conversation
📝 Walkthrough## Walkthrough
The change updates the tracing instrumentation on the `send_message` async function in `Sender<T>` to log its return value at trace level. It modifies the `Probe` struct to notify an optional `PingQueryReplier` on probe timeouts and simplifies nonce generation and pong notification. The `PingQueryReplier` struct and related ping logic were refactored to remove challenge nonce handling, simplify latency calculation, and change the notification interface to accept direct ping results. Additionally, the `NetworkTriggeredEvent` mechanism was removed across multiple crates, simplifying network event handling, peer discovery, and swarm initialization, and removing related dependencies. The default maximum first hop latency threshold was increased from 100ms to 250ms. The `process_stream_protocol` function was refactored to propagate stream opening errors explicitly. The Python SDK node class was updated to support ephemeral data directories with optional cleanup.
## Changes
| File(s) | Change Summary |
|----------------------------------------------|-------------------------------------------------------------------------------------------------------------|
| transport/probe/src/probe.rs | Added trace-level return value logging to `send_message`; notify `PingQueryReplier` on probe timeout; simplified nonce generation; changed pong notification to send latency result. |
| transport/probe/src/ping.rs | Removed challenge nonce field and verification from `PingQueryReplier`; changed `notify` to accept ping results directly; simplified latency calculation; updated tests accordingly. |
| transport/api/src/constants.rs | Removed constant `MAXIMUM_NETWORK_UPDATE_EVENT_QUEUE_SIZE`. |
| transport/api/src/lib.rs | Removed creation and passing of `NetworkTriggeredEvent` channel to `HoprSwarm::new` and `Probe::continuously_scan`. |
| transport/api/src/network_notifier.rs | Removed `emitter` field and event emission logic from `ProbeNetworkInteractions`; simplified `on_finished` method. |
| transport/network/src/network.rs | Removed `NetworkTriggeredEvent` enum and variants; simplified `update` method to return only `Result<()>` without events; updated tests. |
| transport/network/src/config.rs | Increased `DEFAULT_MAX_FIRST_HOP_LATENCY_THRESHOLD` from 100ms to 250ms; improved documentation wording. |
| transport/p2p/Cargo.toml | Removed dependencies `futures-concurrency` and `hopr-transport-network`. |
| transport/p2p/src/behavior/discovery.rs | Removed `NetworkTriggeredEvent` handling from discovery behavior; simplified constructor and connection logic; removed dial failure handling; changed peer multiaddress storage to vector. |
| transport/p2p/src/lib.rs | Removed `NetworkTriggeredEvent` parameter from `HoprNetworkBehavior::new` constructor. |
| transport/p2p/src/swarm.rs | Removed `NetworkTriggeredEvent` parameter from `build_p2p_network` and `HoprSwarm::new` constructors. |
| transport/p2p/tests/p2p_transport_test.rs | Removed `NetworkTriggeredEvent` channel and related fields from test setup; simplified swarm construction. |
| transport/protocol/src/stream.rs | Refactored `process_stream_protocol` to use explicit `Sender` and `Receiver` imports; changed cache retrieval to propagate errors explicitly with improved logging. |
| transport/protocol/Cargo.toml | Moved `anyhow` dependency from dev-dependencies to main dependencies. |
| sdk/python/localcluster/node.py | Added ephemeral temporary data directory support to `Node` class with optional automatic cleanup. |
## Possibly related PRs
- hoprnet/hoprnet#6504: Modifies the internal logic of `Network::update` to fix peer detection issues while retaining event returns; related due to changes in the same method but with different goals.
## Suggested labels
`crate:core-network`, `crate:core-transport`, `crate:core-p2p`
## Suggested reviewers
- tolbrino📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
384ee29 to
2b4077b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
transport/probe/src/probe.rs (1)
52-52: Maintain consistency in level specifiers
The maininstrumentmacro useslevel = "debug"(string literal) but theretattribute referencestracing::Level::DEBUG. For a more uniform style, consider switching theretspecifier to a string literal as well:- #[tracing::instrument(level = "debug", - skip(self, path, message), - fields(message=%message, nonce=%to_nonce(&message), pseudonym=%to_pseudonym(&path)), - ret(level = tracing::Level::DEBUG), - err(level = tracing::Level::DEBUG, Display))] + #[tracing::instrument(level = "debug", + skip(self, path, message), + fields(message=%message, nonce=%to_nonce(&message), pseudonym=%to_pseudonym(&path)), + ret(level = "debug"), + err(level = "debug", Display))]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
transport/probe/src/probe.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: hopli / docker
- GitHub Check: hoprd / docker
- GitHub Check: Docs / Rust docs
- GitHub Check: zizmor
- GitHub Check: Build Candidate Binaries (hoprd)
- GitHub Check: Build Candidate Binaries (hopli)
- GitHub Check: tests-unit
- GitHub Check: tests-unit-nightly
- GitHub Check: Linter
- GitHub Check: Cargo Audit
🔇 Additional comments (1)
transport/probe/src/probe.rs (1)
52-52: Great observability: return value logging added
Introducingret(level = tracing::Level::DEBUG)will emit the successful return ofsend_messageat debug level, giving you full visibility into both error and success paths.
3c082db to
b96d7bf
Compare
b96d7bf to
7998e64
Compare
d6f44c2 to
c78dd82
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
transport/p2p/src/behavior/discovery.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
transport/p2p/src/behavior/discovery.rs (1)
Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6553
File: transport/api/src/p2p.rs:570-574
Timestamp: 2024-10-15T20:37:20.303Z
Learning: In `transport/api/src/p2p.rs`, within the `ConnectionEstablished` event handler, we follow the libp2p convention by assigning `PeerOrigin::IncomingConnection` to all peers, regardless of whether the connection was initiated by our node or by the remote peer.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: hopli / docker
- GitHub Check: hoprd / docker
- GitHub Check: Docs / Rust docs
- GitHub Check: Build Candidate Binaries (hoprd)
- GitHub Check: Linter
- GitHub Check: Build Candidate Binaries (hopli)
- GitHub Check: tests-unit-nightly
- GitHub Check: tests-smart-contracts
- GitHub Check: tests-unit
- GitHub Check: zizmor
- GitHub Check: Cargo Audit
🔇 Additional comments (5)
transport/p2p/src/behavior/discovery.rs (5)
35-35: LGTM: Improved data structure for multiple addresses per peer.The change from
HashMap<PeerId, Multiaddr>toHashMap<PeerId, Vec<Multiaddr>>is a logical improvement that supports peers having multiple network addresses, which is common in distributed systems.
41-47: Simplified constructor aligns with NetworkTriggeredEvent removal.The constructor simplification to accept only
PeerDiscoveryevents (removing network events) is consistent with the PR objective of removing the network update feedback chain. The stream mapping is clean and efficient.
64-64: Excellent tracing instrumentation for observability.The structured logging with appropriate fields (
transport = "p2p discovery") and consistent use of debug level provides good observability for debugging connection and discovery issues. The instrumentation covers all key methods appropriately.Also applies to: 81-87, 115-115, 152-157
98-104: Handle potential empty address vector gracefully.The logic correctly handles cases where no addresses are found for a peer by returning an empty vector and logging a debug message. This approach prevents dial failures from causing unexpected errors while maintaining visibility through logging.
169-212: Robust event handling with improved multi-address support.The event handling logic properly manages multiple addresses per peer:
- Announces all addresses for allowed peers
- Stores the complete address vector
- Performs proactive dialing to establish connections before heartbeat mechanisms
The structured logging provides good visibility into the discovery process. The dial logic is well-reasoned to avoid heartbeat failures due to connection establishment delays.
cd9e894 to
a06a9ae
Compare
a06a9ae to
36b0146
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sdk/python/localcluster/node.py (1)
59-59: Resource management is appropriate despite static analysis warning.The static analysis tool suggests using 'with' for resource allocation, but this is a false positive. The
TemporaryDirectoryobject needs to persist for the lifetime of the Node instance, making manual cleanup in theclean_upmethod the correct approach.Consider adding error handling to verify the temporary directory was created successfully:
+try: self.temp_data_dir = tempfile.TemporaryDirectory(prefix=f"{NODE_NAME_PREFIX}_{self.id}_") +except OSError as e: + raise RuntimeError(f"Failed to create temporary directory for node {self.id}: {e}")🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 59-59: Consider using 'with' for resource-allocating operations
(R1732)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sdk/python/localcluster/node.py(5 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
sdk/python/localcluster/node.py
[refactor] 59-59: Consider using 'with' for resource-allocating operations
(R1732)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: hopli / docker
- GitHub Check: hoprd / docker
- GitHub Check: Docs / Rust docs
- GitHub Check: Build Candidate Binaries (hopli)
- GitHub Check: tests-unit-nightly
- GitHub Check: Build Candidate Binaries (hoprd)
- GitHub Check: tests-unit
- GitHub Check: zizmor
- GitHub Check: Add PR labels
- GitHub Check: Linter
- GitHub Check: Cargo Audit
🔇 Additional comments (4)
sdk/python/localcluster/node.py (4)
4-4: LGTM: Import addition is appropriate.The
tempfileimport is necessary for the new ephemeral data directory functionality.
48-48: LGTM: Constructor parameter follows good practices.The
remove_temp_dataparameter with a sensible default value allows users to control the cleanup behavior while maintaining backward compatibility.
57-57: LGTM: Instance variable assignment is standard.Proper assignment of the constructor parameter to an instance variable.
208-208:❓ Verification inconclusive
Verify the behavioral change from persistent to ephemeral storage.
This change represents a significant shift from using persistent data directories (
self.dir) to ephemeral temporary directories (self.temp_data_dir.name). Ensure that:
- This change is intentional and aligns with the testing session improvements mentioned in the PR
- Any critical node data that needs to persist between runs is handled elsewhere
- Tests are updated to account for this behavioral change
🏁 Script executed:
#!/bin/bash # Description: Check for any references to persistent data directory usage that might be affected # Expected: Identify any code that might depend on persistent data storage echo "Searching for references to self.dir in the codebase..." rg "self\.dir" --type py -A 3 -B 3 echo "Searching for any hardcoded data directory paths..." rg "data.*dir" --type py -A 2 -B 2Length of output: 6061
Confirm intentional switch from persistent (
self.dir) to ephemeral (self.temp_data_dir) storageThe hoprd command now receives
f"--data={self.temp_data_dir.name}"instead of
--data=self.dir, which means all node data is written to a temporary directory by default. This is a behavioral change that requires verification:• sdk/python/localcluster/node.py (around line 208):
--data={self.temp_data_dir.name}
• sdk/python/localcluster/node.py: other uses ofself.dirstill cover identity files (hoprd.id),.envloading, logs, etc.
• sdk/python/localcluster/snapshot.py: cleanup routines still referenceself.dirPlease ensure:
- The switch to ephemeral storage is intentional and matches the PR’s testing-session goals.
- Any data that must survive across runs (snapshots, identities, env files) is still written to a persistent location or handled elsewhere.
- All relevant tests and fixtures have been updated to expect temporary directories for hoprd data.
e834a15 to
01c11da
Compare
3fc69c0 to
c4bd3e5
Compare
PingerobjectNotes
This pull request simplifies the codebase by removing unused features and streamlining event handling across multiple files. It eliminates the
NetworkTriggeredEventenum and its associated logic, refactors related methods, and updates dependencies to reflect these changes. Additionally, it adjusts some configuration constants and improves documentation clarity.Event Handling Simplifications:
NetworkTriggeredEventenum and its usage across files, includingtransport/api/src/lib.rs,transport/api/src/network_notifier.rs,transport/network/src/network.rs, andtransport/p2p/src/behavior/discovery.rs. This includes eliminating associated methods likeupdate,is_ignored, and event emitters. [1] [2] [3] [4]Code Refactoring:
HoprSwarm::newand related method calls to remove unused parameters, simplifying initialization logic intransport/api/src/lib.rs. [1] [2]transport/network/src/network.rsto use theResulttype fromcrate::errorsdirectly, replacingcrate::errors::Result. [1] [2] [3]Dependency and Configuration Updates:
hopr_transport_networkdependency fromtransport/p2p/Cargo.tomlandfutures-concurrencyfrom multiple files. [1] [2]DEFAULT_MAX_FIRST_HOP_LATENCY_THRESHOLDintransport/network/src/config.rsfrom 100ms to 250ms for improved latency handling.Documentation and Style Improvements:
transport/network/src/config.rs.transport/network/src/network.rs.Testing Adjustments:
transport/network/src/network.rsto reflect the removal ofNetworkTriggeredEvent, ensuring compatibility with the refactored code. [1] [2]