BIP155: change when sendaddrv2 is to be sent by vasild · Pull Request #1043 · bitcoin/bips

vasild · 2020-12-07T15:11:39Z

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.

vasild · 2020-12-07T15:16:46Z

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

change net_processing.cpp

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

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

maflcko · 2020-12-07T15:26:04Z

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

vasild · 2020-12-07T15:33:41Z

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).

jonatack · 2020-12-07T15:44:28Z

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

vasild · 2020-12-07T17:08:10Z

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

laanwj · 2020-12-07T17:14:05Z

ACK 50613bb

benthecarman · 2020-12-07T17:22:58Z

ACK 50613bb

bip-0155.mediawiki

jonatack

Concept ACK

bip-0155.mediawiki

maflcko · 2020-12-08T06:41:55Z

ACK c2c55cf

bip-0155.mediawiki

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`.

vasild · 2020-12-08T09:37:00Z

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

maflcko · 2020-12-08T09:40:35Z

ACK e549ed3

harding · 2020-12-08T10:13:48Z

ACK e549ed3

Thanks for updating!

jnewbery · 2020-12-08T10:16:51Z

ACK e549ed3

jonatack

ACK e549ed3

hebasto

ACK e549ed3, 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.

maflcko · 2020-12-08T11:51:38Z

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

laanwj · 2020-12-08T14:04:19Z

re-ACK e549ed3

See the corresponding BIP change: bitcoin/bips#1043

jaysonmald35 · 2021-08-14T23:14:12Z

Thank you

See the corresponding BIP change: bitcoin/bips#1043 Github-Pull: #20564 Rebased-From: 1583498 (cherry picked from commit bead935)

Summary --- This is a backport of bitcoin/bitcoin@1583498 > See the corresponding BIP change: bitcoin/bips#1043 Test plan --- * `ninja all check-all`

…nd send before 'verack' 1583498 Send and require SENDADDRV2 before VERACK (Pieter Wuille) c5a8919 Don't send 'sendaddrv2' to pre-70016 software (Pieter Wuille) Pull request description: BIP155 defines addrv2 and sendaddrv2 for all protocol versions, but some implementations reject messages they don't know. As a courtesy, don't send it to nodes with a version before 70016, as no software is known to support BIP155 that doesn't announce at least that protocol version number. Also move the sending of sendaddrv2 earlier (before sending verack), as proposed in bitcoin/bips#1043. This has the side effect that local address broadcast of torv3 will work (as it'll only trigger after we know whether or not the peer supports addrv2). ACKs for top commit: MarcoFalke: ACK 1583498 jnewbery: ACK 1583498 jonatack: ACK 1583498 vasild: ACK 1583498 Tree-SHA512: 3bd5833fa8c8567b6dedd99e4a9b6bb71c127aa66d5284b217503c86d597dc59aa7382c41f3a4bf561bb658b89db81d1a7703a700eef4ffc17cb916660e23a82

vasild force-pushed the bip155_fix_send_recv_typo branch from 33db426 to 50613bb Compare December 7, 2020 17:06

vasild changed the title ~~BIP155: fix sending vs receiving verack typo~~ BIP155: change when sendaddrv2 is to be sent Dec 7, 2020

sipa mentioned this pull request Dec 7, 2020

Don't send 'sendaddrv2' to pre-70016 software, and send before 'verack' bitcoin/bitcoin#20564

Merged

jnewbery reviewed Dec 7, 2020

View reviewed changes

bip-0155.mediawiki Outdated Show resolved Hide resolved

maflcko reviewed Dec 7, 2020

View reviewed changes

bip-0155.mediawiki Outdated Show resolved Hide resolved

jonatack reviewed Dec 7, 2020

View reviewed changes

bip-0155.mediawiki Outdated Show resolved Hide resolved

sipa mentioned this pull request Dec 8, 2020

Btcd disconnects upon receiving sendaddrv2 btcsuite/btcd#1661

Closed

vasild force-pushed the bip155_fix_send_recv_typo branch from 50613bb to c2c55cf Compare December 8, 2020 06:03

harding reviewed Dec 8, 2020

View reviewed changes

bip-0155.mediawiki Outdated Show resolved Hide resolved

BIP155: change when sendaddrv2 is to be sent

e549ed3

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`.

vasild force-pushed the bip155_fix_send_recv_typo branch from c2c55cf to e549ed3 Compare December 8, 2020 09:35

jonatack reviewed Dec 8, 2020

View reviewed changes

hebasto approved these changes Dec 8, 2020

View reviewed changes

furszy pushed a commit to furszy/bitcoin-core that referenced this pull request Aug 10, 2021

Send and require SENDADDRV2 before VERACK

ba954ca

See the corresponding BIP change: bitcoin/bips#1043

apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 1, 2021

Send and require SENDADDRV2 before VERACK

b207c11

See the corresponding BIP change: bitcoin/bips#1043 Github-Pull: #20564 Rebased-From: 1583498 (cherry picked from commit bead935)

apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 18, 2021

Send and require SENDADDRV2 before VERACK

9ae0204

See the corresponding BIP change: bitcoin/bips#1043 Github-Pull: #20564 Rebased-From: 1583498 (cherry picked from commit bead935)

apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 22, 2021

Send and require SENDADDRV2 before VERACK

f15c72c

See the corresponding BIP change: bitcoin/bips#1043 Github-Pull: #20564 Rebased-From: 1583498 (cherry picked from commit bead935)

Conversation

vasild commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Dec 7, 2020

Uh oh!

maflcko commented Dec 7, 2020

Uh oh!

vasild commented Dec 7, 2020

Uh oh!

jonatack commented Dec 7, 2020

Uh oh!

vasild commented Dec 7, 2020

Uh oh!

laanwj commented Dec 7, 2020

Uh oh!

benthecarman commented Dec 7, 2020

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Dec 8, 2020

Uh oh!

Uh oh!

vasild commented Dec 8, 2020

Uh oh!

maflcko commented Dec 8, 2020

Uh oh!

harding commented Dec 8, 2020

Uh oh!

jnewbery commented Dec 8, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 8, 2020

Uh oh!

laanwj commented Dec 8, 2020

Uh oh!

jaysonmald35 commented Aug 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

vasild commented Dec 7, 2020 •

edited

Loading