feat: add display info to yamux #6904
Conversation
Added display info to yamux worker
WalkthroughThis pull request introduces a new structure, Changes
Sequence Diagram(s)sequenceDiagram
participant Peer as Dialer/Listener
participant Yamux as Yamux::upgrade_connection
participant Worker as YamuxWorker
Peer->>Yamux: Create PeerConnectionInfo with connection details
Yamux-->>Peer: Initiate connection upgrade with info
Yamux->>Worker: Spawn incoming stream worker (passing PeerConnectionInfo)
Worker->>Worker: Log and manage connection using peer info
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
comms/core/src/connection_manager/listener.rs (1)
420-426: Good implementation of PeerConnectionInfoThe creation and usage of
PeerConnectionInfolook good. The struct is populated with appropriate information from the authenticated peer and correctly passed toYamux::upgrade_connection.Consider optimizing the repeated cloning of
valid_peer_identity:- let peer_connection_info = PeerConnectionInfo { - public_key: Some(authenticated_public_key.clone()), - features: Some(valid_peer_identity.clone().claim.features), - user_agent: Some(valid_peer_identity.clone().metadata.user_agent.clone()), - }; + let peer_connection_info = PeerConnectionInfo { + public_key: Some(authenticated_public_key.clone()), + features: Some(valid_peer_identity.claim.features), + user_agent: Some(valid_peer_identity.metadata.user_agent.clone()), + };comms/core/src/connection_manager/common.rs (1)
61-72: Display implementation can be optimizedThe
Displayimplementation works correctly but contains unnecessary cloning operations that could impact performance.impl Display for PeerConnectionInfo { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( f, "PeerConnectionInfo(public_key: {}, node_id: {}, features: {}, user_agent: {})", - self.public_key.clone().unwrap_or_default().to_hex(), - NodeId::from_public_key(&self.public_key.clone().unwrap_or_default()), - self.features.unwrap_or_default(), - self.user_agent.clone().unwrap_or_default() + self.public_key.as_ref().unwrap_or(&CommsPublicKey::default()).to_hex(), + NodeId::from_public_key(self.public_key.as_ref().unwrap_or(&CommsPublicKey::default())), + self.features.unwrap_or_default(), + self.user_agent.as_deref().unwrap_or_default() ) } }This optimization:
- Uses
as_ref()instead ofclone()forpublic_keyto avoid unnecessary cloning- Uses
as_deref()foruser_agentto convertOption<String>toOption<&str>without cloning
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
comms/core/src/connection_manager/common.rs(2 hunks)comms/core/src/connection_manager/dialer.rs(2 hunks)comms/core/src/connection_manager/listener.rs(2 hunks)comms/core/src/connection_manager/mod.rs(1 hunks)comms/core/src/multiplexing/yamux.rs(18 hunks)comms/core/src/test_utils/transport.rs(2 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
comms/core/src/connection_manager/listener.rs (1)
comms/core/src/multiplexing/yamux.rs (1)
upgrade_connection(53-78)
comms/core/src/connection_manager/common.rs (3)
comms/core/src/connection_manager/manager.rs (1)
fmt(84-104)comms/core/src/peer_manager/peer.rs (1)
fmt(290-333)comms/core/src/peer_manager/node_id.rs (1)
from_public_key(86-88)
comms/core/src/connection_manager/dialer.rs (1)
comms/core/src/multiplexing/yamux.rs (1)
upgrade_connection(53-78)
comms/core/src/test_utils/transport.rs (1)
comms/core/src/multiplexing/yamux.rs (1)
upgrade_connection(53-78)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: ci
🔇 Additional comments (28)
comms/core/src/connection_manager/mod.rs (1)
37-37: Good addition: Exposing PeerConnectionInfo entityThis addition correctly exports the new
PeerConnectionInfostruct from the common module, making it available to other modules that import fromconnection_manager. This is consistent with the PR objective to enhance stream error display information.comms/core/src/test_utils/transport.rs (3)
26-26: Import updated correctlyThe import statement has been properly updated to include the new
PeerConnectionInfostruct.
42-46: Test initialization looks goodThe test code correctly initializes the
PeerConnectionInfowith all fields set toNone, which is appropriate for test cases.
47-49: Function call updated appropriatelyThe calls to
Yamux::upgrade_connectionhave been correctly updated to include the newpeer_connection_infoparameter, with the first call using a cloned instance and the second using the original.comms/core/src/connection_manager/listener.rs (2)
52-52: Import addition looks goodThe import for
PeerConnectionInfohas been correctly added to the file.
415-415: Proper key clone addedThe
authenticated_public_keyis now correctly cloned when passed tocreate_or_update_peer_from_validated_peer_identity, ensuring the original value remains available for subsequent use.comms/core/src/connection_manager/common.rs (2)
25-26: Import additions look goodThe imports for
Display,Formatter, andHexhave been correctly added to support the new functionality.
54-59: Well-structured PeerConnectionInfo definitionThe
PeerConnectionInfostruct is well designed with appropriate optional fields for peer connection details. The use ofOptiontypes is a good choice for cases where some information may not be available.comms/core/src/connection_manager/dialer.rs (2)
48-48: Appropriate import added for PeerConnectionInfoThe import of
PeerConnectionInfois correctly added to the existing imports.
470-476: Well-structured use of PeerConnectionInfoThe PeerConnectionInfo structure is properly instantiated with the available peer data and passed to the Yamux::upgrade_connection method. This enhances the contextual information available during connection establishment and subsequent error handling.
comms/core/src/multiplexing/yamux.rs (18)
36-36: Import correctly updated for PeerConnectionInfoThe import statement has been appropriately updated to include PeerConnectionInfo.
53-57: Method signature properly updatedThe upgrade_connection method signature has been correctly updated to accept the new PeerConnectionInfo parameter.
70-71: Parameter correctly passed to workerThe peer_connection_info parameter is properly forwarded to the spawn_incoming_stream_worker method.
82-86: Worker method signature properly updatedThe spawn_incoming_stream_worker method signature has been correctly updated to accept the peer_connection_info parameter.
92-92: New parameter properly passed to YamuxWorker constructorThe peer_connection_info parameter is correctly passed to the YamuxWorker::new method.
245-245: New field added to YamuxWorker structThe YamuxWorker struct has been correctly updated with the new peer_connection_info field.
255-255: Worker constructor parameter addedThe YamuxWorker::new method signature has been properly updated to accept the peer_connection_info parameter.
262-262: Field assignment correctly implementedThe peer_connection_info parameter is properly assigned to the struct field.
274-278: Improved logging with connection contextThe log message now includes peer connection information, which will help with debugging connection issues by providing more context in the logs.
286-286: Enhanced error loggingThe error log now includes the peer connection information, providing better context for troubleshooting connection failures.
297-301: Consistent log enhancement for channel closureThe log message for channel closure now consistently includes peer connection information.
308-311: Improved substream end loggingThe substream end log message now includes peer connection information, maintaining consistency with other log messages.
317-320: Enhanced error reporting for substream errorsThe error log for substream errors now includes detailed peer connection information, which will be valuable for diagnosing stream issues.
395-399: Test correctly updated with PeerConnectionInfoThe test for open_substream has been properly updated to create and pass a PeerConnectionInfo instance.
429-436: Test properly modified for substream_countThe substream_count test has been correctly updated to include PeerConnectionInfo.
473-478: Close test properly updatedThe test for connection closure has been correctly modified to include PeerConnectionInfo.
518-524: Rude close test correctly updatedThe test for handling abrupt connection closures has been properly updated to create and pass PeerConnectionInfo instances.
554-559: Big message test properly updatedThe test for sending large messages has been correctly updated to include PeerConnectionInfo.
Test Results (CI) 3 files 129 suites 43m 26s ⏱️ Results for commit 355a3c4. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
comms/core/src/connection_manager/common.rs (2)
54-59: LGTM: New PeerConnectionInfo struct with appropriate fieldsThis new struct properly encapsulates the optional peer connection details (public key, features, and user agent) as mentioned in the PR objectives. The use of
Optionfor all fields allows for flexibility when complete information isn't available.Consider adding documentation comments to describe the purpose of this struct and its fields, which would help other developers understand its usage throughout the codebase.
#[derive(Clone, Debug, Eq, PartialEq)] +/// Contains information about a peer connection that can be displayed for debugging purposes. pub struct PeerConnectionInfo { + /// The public key of the connected peer, if available pub public_key: Option<CommsPublicKey>, + /// The features supported by the peer, if available pub features: Option<PeerFeatures>, + /// The user agent string of the peer, if available pub user_agent: Option<String>, }
61-72: Implementation of Display trait provides good error informationThe Display implementation effectively formats the peer connection information for debugging, which aligns with the PR objective of enhancing stream error information. The implementation correctly handles optional fields with appropriate defaults.
Two potential improvements to consider:
- You're generating a NodeId from the public key in every Display call. For frequently displayed objects, you might consider caching this or adding a helper method.
- You could implement a Default for PeerConnectionInfo to centralize default value logic.
+impl Default for PeerConnectionInfo { + fn default() -> Self { + Self { + public_key: None, + features: None, + user_agent: None, + } + } +} impl Display for PeerConnectionInfo { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( f, "PeerConnectionInfo(public_key: {}, node_id: {}, features: {}, user_agent: {})", self.public_key.as_ref().unwrap_or(&CommsPublicKey::default()).to_hex(), NodeId::from_public_key(self.public_key.as_ref().unwrap_or(&CommsPublicKey::default())), self.features.unwrap_or_default(), self.user_agent.as_deref().unwrap_or_default() ) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
comms/core/src/connection_manager/common.rs(2 hunks)comms/core/src/connection_manager/listener.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- comms/core/src/connection_manager/listener.rs
🧰 Additional context used
🧬 Code Definitions (1)
comms/core/src/connection_manager/common.rs (4)
comms/core/src/connection_manager/manager.rs (1)
fmt(84-104)comms/core/src/peer_manager/peer.rs (1)
fmt(290-333)base_layer/core/src/transactions/transaction_components/transaction_output.rs (1)
fmt(523-541)comms/core/src/peer_manager/node_id.rs (2)
as_ref(228-230)from_public_key(86-88)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
🔇 Additional comments (1)
comms/core/src/connection_manager/common.rs (1)
25-25: LGTM: Imports for the new Display trait implementationThe addition of the necessary imports for
Display,Formatter, andHexis appropriate for the new functionality being added.Also applies to: 30-30
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
comms/core/src/connection_manager/common.rs (4)
60-60: Fix typo in field nameThere appears to be a typo in the field name
node_if, which should likely benode_idbased on its purpose and usage.- node_if: Option<NodeId>, + node_id: Option<NodeId>,
67-76: Avoid unnecessary clone in constructorIn the constructor,
public_keyis unnecessarily cloned. Since it's already anOptionand you're taking ownership, you can avoid the clone.impl PeerConnectionInfo { pub fn new(public_key: Option<CommsPublicKey>, features: Option<PeerFeatures>, user_agent: Option<String>) -> Self { Self { - public_key: public_key.clone(), + public_key, node_if: public_key.as_ref().map(NodeId::from_public_key), features, user_agent, } } }
78-89: Update Display implementation to match field name changeIf you rename the field as suggested above, don't forget to update the Display implementation as well. Also, consider handling edge cases where Debug representation might be more informative for complex types.
impl Display for PeerConnectionInfo { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( f, "PeerConnectionInfo(public_key: {}, node_id: {}, features: {}, user_agent: {})", self.public_key.as_ref().unwrap_or(&CommsPublicKey::default()).to_hex(), - self.node_if.as_ref().unwrap_or(&NodeId::default()), + self.node_id.as_ref().unwrap_or(&NodeId::default()), self.features.unwrap_or_default(), self.user_agent.as_deref().unwrap_or_default() ) } }
71-71: Update all references to field name if changedIf you update the field name from
node_iftonode_id, also update this reference in the constructor.- node_if: public_key.as_ref().map(NodeId::from_public_key), + node_id: public_key.as_ref().map(NodeId::from_public_key),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
comms/core/src/connection_manager/common.rs(2 hunks)comms/core/src/connection_manager/dialer.rs(2 hunks)comms/core/src/connection_manager/listener.rs(2 hunks)comms/core/src/multiplexing/yamux.rs(18 hunks)comms/core/src/test_utils/transport.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- comms/core/src/test_utils/transport.rs
- comms/core/src/connection_manager/listener.rs
- comms/core/src/connection_manager/dialer.rs
- comms/core/src/multiplexing/yamux.rs
🧰 Additional context used
🧬 Code Definitions (1)
comms/core/src/connection_manager/common.rs (3)
comms/core/src/connection_manager/manager.rs (2)
fmt(84-104)new(208-281)comms/core/src/peer_manager/peer.rs (1)
fmt(290-333)comms/core/src/peer_manager/node_id.rs (2)
from_public_key(86-88)as_ref(228-230)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: ci
🔇 Additional comments (1)
comms/core/src/connection_manager/common.rs (1)
54-65: Good addition of structured connection informationThe new
PeerConnectionInfostruct is a good addition for encapsulating peer connection details. This will help with the PR's goal of enhancing yamux by adding display information for stream errors.
hansieodendaal
force-pushed
the
ho_yamux_display
branch
from
3550a25 to
355a3c4
Compare
Description --- - Yamux should not poll after an error. - Adds further specialization on error handling when the remote peer disconnects forcefully. Motivation and Context --- Tha yamux close was erroneously introduced in a previous PR #6904. How Has This Been Tested? --- Not tested - just reverting What process can a PR reviewer use to test or verify this change? --- Code review <!-- Checklist --> <!-- 1. Is the title of your PR in the form that would make nice release notes? The title, excluding the conventional commit tag, will be included exactly as is in the CHANGELOG, so please think about it carefully. --> Breaking Changes --- - [x] None - [ ] Requires data directory on base node to be deleted - [ ] Requires hard fork - [ ] Other - Please specify <!-- Does this include a breaking change? If so, include this line as a footer --> <!-- BREAKING CHANGE: Description what the user should do, e.g. delete a database, resync the chain --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Enhanced handling of connection issues to deliver more stable and reliable network interactions. - Updated the connection closure process to better manage unexpected interruptions and reduce disruptions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
* development: chore: new release v1.13.3-pre.0 (tari-project#6919) fix: fix migration 5 (tari-project#6915) chore: update change logs (tari-project#6913) chore: new release v1.13.2-pre.0 (tari-project#6912) fix: randomX seed management (tari-project#6910) fix(comms/yamux): dont poll the substream after closing/error (tari-project#6911) fix: dont poll yamux substream after an error (tari-project#6909) feat: remove memory allocation for max_size_vec (tari-project#6903) feat: add display info to yamux (tari-project#6904)
Description
Added display info to yamux worker.
Motivation and Context
We need more info on yamux stream errors.
How Has This Been Tested?
System-level testing
This
now changed to
What process can a PR reviewer use to test or verify this change?
Code review.
Breaking Changes
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
shutdown_signalfrom theInboundstruct initialization, refining its shutdown management behavior.