feat: add transport protocol preference modes#7852
Conversation
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
9c3fbd8 to
7af0215
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces TorTcp and TcpTor transport types to allow configurable protocol preferences, setting TcpTor as the default. The dialer now prioritizes addresses based on these types. Feedback identifies a breaking change in the Tor transport behavior, an unintended change to Socks5 protocol preferences, and performance inefficiencies in the new address selection logic.
| TransportType::Memory => vec![TransportProtocol::Memory], | ||
| TransportType::Tcp => vec![TransportProtocol::Ipv4, TransportProtocol::Ipv6], | ||
| TransportType::Tor => vec![ | ||
| TransportType::Tor => vec![TransportProtocol::Onion], |
There was a problem hiding this comment.
Changing the Tor transport type to only support Onion protocols is a breaking change for existing node configurations. Previously, the Tor mode allowed communication with both Onion and TCP/IP addresses (via the Tor proxy). While TorTcp has been added to provide the previous behavior, users who have explicitly set type = "tor" in their configuration files will find their nodes restricted to Onion-only connections after this update. As this represents a fundamental change in transport logic, please ensure that tests are updated to validate the new behavior and that assertions testing obsolete logic are replaced, as per the repository guidelines.
References
- When an algorithm's logic is fundamentally changed (e.g., UTXO selection method), tests should be updated to validate the new behavior. Assertions testing obsolete logic (like selection order by age) should be removed and replaced with tests for the new algorithm's properties.
| TransportType::Socks5 => vec![ | ||
| TransportProtocol::Ipv4, | ||
| TransportProtocol::Ipv6, | ||
| TransportProtocol::Onion, | ||
| ], |
There was a problem hiding this comment.
The protocol preference for Socks5 has been changed from Tor-first (Onion, Ipv4, Ipv6) to TCP-first (Ipv4, Ipv6, Onion). While this aligns with the new project-wide default preference for TCP, the PR description states that socks5 was intended to be left in place. If this change in preference order for the SOCKS5 proxy was unintended, please revert it to maintain backward compatibility for users relying on the previous behavior. If the change was intended, ensure tests are updated to reflect the new selection order and obsolete assertions are removed.
| TransportType::Socks5 => vec![ | |
| TransportProtocol::Ipv4, | |
| TransportProtocol::Ipv6, | |
| TransportProtocol::Onion, | |
| ], | |
| TransportType::Socks5 => vec![ | |
| TransportProtocol::Onion, | |
| TransportProtocol::Ipv4, | |
| TransportProtocol::Ipv6, | |
| ], |
References
- When an algorithm's logic is fundamentally changed (e.g., UTXO selection method), tests should be updated to validate the new behavior. Assertions testing obsolete logic (like selection order by age) should be removed and replaced with tests for the new algorithm's properties.
| fn select_dial_addresses( | ||
| peer_addresses: &[Multiaddr], | ||
| supported_transport_protocols: &[TransportProtocol], | ||
| excluded_dial_addresses: &[MultiaddrRange], | ||
| ) -> Vec<Multiaddr> { | ||
| supported_transport_protocols | ||
| .iter() | ||
| .flat_map(|protocol| { | ||
| peer_addresses | ||
| .iter() | ||
| .filter(move |address| { | ||
| TransportProtocol::from(*address) == *protocol && | ||
| !excluded_dial_addresses | ||
| .iter() | ||
| .any(|excluded| excluded.contains(address)) | ||
| }) | ||
| .cloned() | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
The current implementation of select_dial_addresses is inefficient as it performs the excluded_dial_addresses check repeatedly for every protocol in the preference list. Additionally, it calls TransportProtocol::from multiple times for each address. A more efficient approach would be to filter out excluded addresses once and then group or order the remaining addresses by protocol.
fn select_dial_addresses(
peer_addresses: &[Multiaddr],
supported_transport_protocols: &[TransportProtocol],
excluded_dial_addresses: &[MultiaddrRange],
) -> Vec<Multiaddr> {
let filtered_addresses: Vec<_> = peer_addresses
.iter()
.filter(|address| !excluded_dial_addresses.iter().any(|excluded| excluded.contains(address)))
.collect();
supported_transport_protocols
.iter()
.flat_map(|protocol| {
filtered_addresses
.iter()
.filter(|address| TransportProtocol::from(***address) == *protocol)
.map(|&addr| addr.clone())
})
.collect()
}7af0215 to
304b6e7
Compare
|
Closing this earlier pass in favor of #7853. The newer PR keeps the same issue target and also applies the configured transport preference to peer candidate selection, not only dial address ordering. |
Fixes #7830.
Summary
tor_tcpandtcp_tortransport modes alongsidetorandtcp.tcp_torthe default transport mode.Notes
memoryandsocks5are left in place for existing in-process test transport and proxy transport support. The four public peer selection modes requested by the bounty are covered bytor,tcp,tor_tcp, andtcp_tor.Payout details can be provided on acceptance.
Validation
cargo +1.93.0 test -p tari_p2p transport_type -- --nocapturecargo +1.93.0 test -p tari_comms select_dial_addresses -- --nocapturecargo +1.93.0 check -p tari_p2p -p tari_commscargo +1.93.0 check -p tari_integration_tests --test cucumbergit diff --check