BIP155: change when sendaddrv2 is to be sent #1043

Mandate to send sendaddrv2 to the peer before sending our verack to them.

This way we know that the peer does not support addrv2 if we did not receive sendaddrv2 from them before receiving their verack.

If the implementation is to be changed instead, then this patch is to be applied to it:

<details> <summary>change net_processing.cpp</summary>

diff --git i/src/net_processing.cpp w/src/net_processing.cpp
index 1b4a05f0b..e46cbe487 100644
--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -2361,15 +2361,12 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
         if (greatest_common_version >= WTXID_RELAY_VERSION) {
             m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::WTXIDRELAY));
         }
 
         m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::VERACK));
 
-        // Signal ADDRv2 support (BIP155).
-        m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDADDRV2));
-
         pfrom.nServices = nServices;
         pfrom.SetAddrLocal(addrMe);
         {
             LOCK(pfrom.cs_SubVer);
             pfrom.cleanSubVer = cleanSubVer;
         }
@@ -2509,12 +2506,14 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
             uint64_t nCMPCTBLOCKVersion = 2;
             if (pfrom.GetLocalServices() & NODE_WITNESS)
                 m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
             nCMPCTBLOCKVersion = 1;
             m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
         }
+        // Signal ADDRv2 support (BIP155).
+        m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDADDRV2));
         pfrom.fSuccessfullyConnected = true;
         return;
     }
 
     // Feature negotiation of wtxidrelay should happen between VERSION and
     // VERACK, to avoid relay problems from switching after a connection is up

</details>

that would send sendaddrv2 at the same time as sending sendheaders and sendcmpct.

A third option would be to use the same feature negotiation order that was used for wtxidrelay

A third option would be to use the same feature negotiation order that was used for wtxidrelay

That would be "sendaddrv2 SHOULD be sent before sending the verack message to the peer" and would require changing both the BIP and the code. Any of the 3 would work wrt the sendaddrv2 / addrv2 logic (= it does not matter when sendaddrv2 is sent).

In the same way as wtxidrelay SGTM for being simpler rather than a different mechanism for each.

Changed to send it before sending verack (like wtxidrelay).

ACK 50613bb82ce89d31027ad27daecf46f05aff198e

ACK 50613bb82ce89d31027ad27daecf46f05aff198e

Concept ACK

ACK c2c55cfbf0284d20cc3080871cbceb9a55d1894e

There are many ways to say it, I hope the current phrasing is clear enough and good for everybody.

ACK e549ed36e8bbb0d15b1bd245cc5bb2c5664d5aa2

ACK e549ed36e8bbb0d15b1bd245cc5bb2c5664d5aa2

Thanks for updating!

ACK e549ed36e8bbb0d15b1bd245cc5bb2c5664d5aa2

ACK e549ed3

ACK e549ed36e8bbb0d15b1bd245cc5bb2c5664d5aa2, I believe that the establishing of connection invariants in a such manner--in response to the version and prior to sending the verack--is the right way both for new addrv2 message and for other future features.

According to the BIP process, this needs an ACK by the BIP champion @laanwj :)

re-ACK e549ed36e8bbb0d15b1bd245cc5bb2c5664d5aa2

@luke-jr

Thank you