Only select from addrv2-capable peers for torv3 address relay #20661

sipa commented at 5:08 PM on December 15, 2020: member

When selecting peers to relay an address to, only pick addrv2-capable ones if the address cannot be represented in addr(v1).

Without this I expect that propagation of torv3 addresses over the cleartext network will be very hard for a while.

MarcoFalke added this to the milestone 0.21.1 on Dec 15, 2020

MarcoFalke added the label Needs backport (0.21) on Dec 15, 2020

MarcoFalke added the label P2P on Dec 15, 2020

MarcoFalke commented at 5:30 PM on December 15, 2020: member

Assuming backport to 0.21.1

laanwj commented at 5:32 PM on December 15, 2020: member

Concept ACK

in src/net_processing.cpp:1447 in 0048b841a2 outdated

1442 | @@ -1443,8 +1443,8 @@ static void RelayAddress(const CNode& originator,
1443 |      std::array<std::pair<uint64_t, CNode*>,2> best{{{0, nullptr}, {0, nullptr}}};
1444 |      assert(nRelayNodes <= best.size());
1445 |  
1446 | -    auto sortfunc = [&best, &hasher, nRelayNodes, &originator](CNode* pnode) {
1447 | -        if (pnode->RelayAddrsWithConn() && pnode != &originator) {
1448 | +    auto sortfunc = [&best, &hasher, nRelayNodes, &originator, &addr](CNode* pnode) {
1449 | +        if (pnode->RelayAddrsWithConn() && pnode != &originator && (addr.IsAddrV1Compatible() || pnode->m_wants_addrv2)) {

vasild commented at 5:39 PM on December 15, 2020:

Does it make sense to add a new method CNode::SupportsAddress(const CAddress&) and do here: pnode->SupportsAddress(addr) and in CNode::PushAddress() instead of the variable addr_format_supported?

sipa commented at 5:52 PM on December 15, 2020:

~~That makes sense, but if m_wants_addrv2 is going to move to net_processing's data structures (where it IMO belongs) it's perhaps wasteful to push more into net now?~~

EDIT: oh, i see, that logic already exists in net. Yeah, fixing.

sipa commented at 5:58 PM on December 15, 2020:

Done.

vasild approved

vasild commented at 5:40 PM on December 15, 2020: member

ACK 0048b841a2c4b854607a6de8c02a808152510457

Excellent!

I got a bit confused by the description, getting the impression that we would try to relay torv3 addresses to non-addrv2 peers and I said to myself "huh, but we already avoid that in PushAddress() with m_wants_addrv2 || _addr.IsAddrV1Compatible()!?".

sipa force-pushed on Dec 15, 2020

in src/net.h:1191 in a298924293 outdated

1185 |      {
1186 |          // Whether the peer supports the address in `_addr`. For example,
1187 |          // nodes that do not implement BIP155 cannot receive Tor v3 addresses
1188 |          // because they require ADDRv2 (BIP155) encoding.
1189 | -        const bool addr_format_supported = m_wants_addrv2 || _addr.IsAddrV1Compatible();
1190 | +        const bool addr_format_supported = IsAddrCompatible(_addr);

jonatack commented at 6:27 PM on December 15, 2020:

perhaps hoist the documentation too (rewritten slightly here)

+    /**
+     * Whether the peer supports the address. For example, a peer that does not
+     * implement BIP155 cannot receive Tor v3 addresses because it requires
+     * ADDRv2 (BIP155) encoding.
+     */
     bool IsAddrCompatible(const CAddress& addr) const
     {
         return m_wants_addrv2 || addr.IsAddrV1Compatible();
@@ -1183,9 +1188,6 @@ public:
 
     void PushAddress(const CAddress& _addr, FastRandomContext &insecure_rand)
     {
-        // Whether the peer supports the address in `_addr`. For example,
-        // nodes that do not implement BIP155 cannot receive Tor v3 addresses
-        // because they require ADDRv2 (BIP155) encoding.
         const bool addr_format_supported = IsAddrCompatible(_addr);

sipa commented at 6:49 PM on December 15, 2020:

Good idea, done.

jonatack commented at 6:30 PM on December 15, 2020: member

This seems like a good idea.

Initial code review ACK a29892429388a10e1cab9a69c3013a537410a20d

Building and testing.

sipa force-pushed on Dec 15, 2020

jonatack commented at 6:53 PM on December 15, 2020: member

ACK 794ae71701f3766a86fd61044368d36004d5687f

sipa renamed this:
~~Only relay torv3 addresses to addrv2-capable peers~~
Only select from addrv2-capable peers for torv3 address relay
on Dec 15, 2020

in src/net.h:1191 in 794ae71701 outdated

1190 |      {
1191 | -        // Whether the peer supports the address in `_addr`. For example,
1192 | -        // nodes that do not implement BIP155 cannot receive Tor v3 addresses
1193 | -        // because they require ADDRv2 (BIP155) encoding.
1194 | -        const bool addr_format_supported = m_wants_addrv2 || _addr.IsAddrV1Compatible();
1195 | +        const bool addr_format_supported = IsAddrCompatible(_addr);

vasild commented at 8:39 PM on December 15, 2020:

nit: I think the variable name is as readable as the function call and it is used in just one place, so it may as well be ditched:

-if (_addr.IsValid() && !m_addr_known->contains(_addr.GetKey()) && addr_format_supported) {
+if (_addr.IsValid() && !m_addr_known->contains(_addr.GetKey()) && IsAddrCompatible(_addr)) {

sipa commented at 8:46 PM on December 15, 2020:

Done.

vasild approved

vasild commented at 8:42 PM on December 15, 2020: member

ACK 794ae71701f3766a86fd61044368d36004d5687f

refactor: add IsAddrCompatible() to CNode 83f8821a6f

Only consider addrv2 peers for relay of non-addrv1 addresses 37fe80e626

sipa force-pushed on Dec 15, 2020

jonatack commented at 10:04 PM on December 15, 2020: member

ACK 37fe80e6267094f6051ccf9bec0c7f1a6b9e15da

vasild approved

vasild commented at 9:13 AM on December 16, 2020: member

ACK 37fe80e6267094f6051ccf9bec0c7f1a6b9e15da

laanwj merged this on Dec 16, 2020

laanwj closed this on Dec 16, 2020

sidhujag referenced this in commit 2859e59435 on Dec 17, 2020

MarcoFalke removed the label Needs backport (0.21) on Dec 17, 2020

MarcoFalke removed this from the milestone 0.21.1 on Dec 17, 2020

Fabcien referenced this in commit 3d475507d4 on Jan 27, 2022

UdjinM6 referenced this in commit b3991f0880 on Jun 7, 2022

UdjinM6 referenced this in commit fa7aa843a2 on Jun 7, 2022

UdjinM6 referenced this in commit b0d655216b on Jun 7, 2022

DrahtBot locked this on Aug 18, 2022