Add transport peer selection modes#7855
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the p2p transport configuration to support combined transport selection modes (Tor, Tcp, TorTcp, TcpTor) and updates peer selection and dialing logic to prioritize addresses based on these configured transport preferences. The review feedback identifies a critical issue where applying transport protocol preference ordering to random peer selection (get_n_random_peers) biases gossip and discovery, potentially isolating Tor-only nodes. It is recommended to keep random peer selection truly random and avoid sorting or truncating them by transport preference.
| .spawn_with_transport(transport) | ||
| .await? | ||
| }, | ||
| let comms = if transport_config.is_memory_transport() { |
There was a problem hiding this comment.
dont use a nested if, its hard to follow, the setup here is also not correct, look at the change diff
There was a problem hiding this comment.
Noted. I applied that and other bug fixes in the latest commits.
TransportConfig::get_supported_protocols()only includes onion for tor_tcp/tcp_tor whentcp.tor_socks_addressis configured. I interpreted “both enabled” in the bounty scope as “both actually dialable” not just both named in the enumeration.
| enum TransportOverride { | ||
| #[default] | ||
| None, | ||
| Memory, | ||
| Socks5, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy, Eq, PartialEq)] | ||
| pub(crate) enum CommsTransport { | ||
| Memory, | ||
| Tcp, | ||
| TorHiddenService, | ||
| Socks5, |
There was a problem hiding this comment.
why add these?
Why not keep the original transaport type and just use them?
There was a problem hiding this comment.
I agree it was an unnecessary over-abstraction. TransportType is again the main control flow for public modes, and the only remaining extra state is the private TransportOverride, which is there solely to preserve the existing internal new_memory / new_socks5 paths.
2 participants
Fixes #7830
Summary
tor,tcp,tor_tcp, andtcp_tor.tcp_torthe default peer transport mode.TransportConfigoverrides for existing test/internal transport support.Validation
git diff --checkrustfmt --check comms/core/src/peer_manager/storage/database.rscargo test -p tari_comms --lib --no-default-features --features tari_common_sqlite/bundledcargo test -p tari_p2p --lib --no-default-features --features tari_common_sqlite/bundledcargo test -p tari_integration_tests --test cucumber --no-run --no-default-features --features tari_common_sqlite/bundled