Don't send 'sendaddrv2' to pre-70016 software, and send before 'verack'

sipa commented at 11:01 pm on December 3, 2020: member

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 https://github.com/bitcoin/bips/pull/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).

fanquake added the label P2P on Dec 3, 2020

sipa commented at 11:11 pm on December 3, 2020: member

I’m deliberately not introducing a name for the 70016 constant, or reuse any of the existing constants. The number isn’t tied to any of those; it’s just… we’re observing that no clients below this number exist that support it.

jonatack commented at 11:18 pm on December 3, 2020: member

utACK 152b0be70ab5e910a6682d2178c2e81c781a2f9b

laanwj commented at 11:47 pm on December 3, 2020: member

Reluctant ACK. I think this is very messed up for reasons mentioned by Matt here. I think a P2P protocol that is not permissionlessly extensible is unworthy of bitcoin.

naumenkogs commented at 1:06 am on December 4, 2020: member

I’m not even sure what’s best here, considering an issue with another implementation. I think considering that this code is non-consensus, very simple, and won’t degrade much further — it’s fine we pay this cost. Reluctant ACK as well.

vasild commented at 4:50 am on December 4, 2020: member

While this patch is technically sound and it will most likely make it in 0.21, the problem with protocol version bumps exists in the long term.

A central body deciding what version numbers are and what goes in them is not suitable for a protocol that is developed in a decentralized manner. Eventually some disagreement may tear apart the sequentiality of the protocol version resulting in something like:

implementation1 uses 70019 for feature X and Y
implementation2 uses 70019 for feature A and B
implementation3 uses 80123 for baking potatoes

Maybe it would be more flexible to introduce new functionality via service bits. Hopefully nobody will disconnect if they see an unknown service bit set. Also, they are gossiped, so one can have an idea what to expect from a peer before connecting and choose peers based on their advertised capabilities.

sipa commented at 4:55 am on December 4, 2020: member

@vasild I completely agree with the flaws of using protocol versions for feature negotiation. That’s not what this is about.

This is a one-time courtesy for existing clients on the network to not cause disruption. It isn’t tying features to versions, and perhaps this patch can even be temporary.

in src/net_processing.cpp:2370 in 152b0be70a outdated

2364@@ -2365,7 +2365,13 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2365         m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::VERACK));
2366 
2367         // Signal ADDRv2 support (BIP155).
2368-        m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDADDRV2));
2369+        if (greatest_common_version >= 70016) {
2370+            // BIP155 defines addrv2 and sendaddrv2 for all protocol versions, but some
2371+            // implementations reject messages they don't know. As a courtesy, don't send
2372+            // it to nodes with a version before 70016, as no software is known to support
2373+            // BIP155 that doesn't announce at least that protocol version number.

MarcoFalke commented at 6:26 am on December 4, 2020:

nit: Could mention that this is a temporary workaround, safe to be reverted end of 2021 or beginning of 2022

luke-jr commented at 5:08 pm on December 6, 2020:

We’re not prophets…

MarcoFalke commented at 6:06 pm on December 6, 2020:

This change literally violates the bip, keeping the hack longer than one or two major versions would be problematic for too many reasons

sipa commented at 6:19 pm on December 6, 2020:

I don’t see how it violates the BIP. It just chooses to only use the feature in some cases.

MarcoFalke commented at 6:27 pm on December 6, 2020:

Ok, closing discussion. I still think this should be reverted eventually.

sipa commented at 6:28 pm on December 6, 2020:

Yes, agree with that, but I don’t know the time frame.

MarcoFalke commented at 6:27 am on December 4, 2020: member

review ACK 152b0be70ab5e910a6682d2178c2e81c781a2f9b seems fine as a temporary workaround to give other implementations a few months to write and deploy a fix

MarcoFalke added this to the milestone 0.21.0 on Dec 4, 2020

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

MarcoFalke commented at 6:29 am on December 4, 2020: member

(Added 0.21 milestone for now)

jonasschnelli commented at 8:11 am on December 4, 2020: contributor

utACK 152b0be70ab5e910a6682d2178c2e81c781a2f9b - agree with @laanwj about @TheBlueMatt point. - Though this “we-fix-your-software” seems acceptable. I already stated my concerns and long term risks with doing such “fixes” (it leads to lesser attention on p2p changes and probably fewer interoperability tests with alternative implementations [because things magically work, a.k.a. Core fixes it]).

laanwj commented at 9:24 am on December 4, 2020: member

Maybe it would be more flexible to introduce new functionality via service bits.

The problem with service bits is that there are (by definition) too few of them, and allocation of them is also a centralized project (lacking centralized allocation, or in presence of disagreement, the meaning of those bits starts to be ambiguous very soon—we’ve seen this with block versions). I do not think it would have been a better choice.

MarcoFalke commented at 9:38 am on December 4, 2020: member

Also, avoiding to connect to “unknown” service bits is worse than disconnection on an unknown message, because it makes sybil attacks easier. An attacker might pollute the service bits in your p2p-address manager.

jnewbery commented at 10:33 am on December 4, 2020: member

I wish we would use this as an opportunity to do sendaddrv2 negotiation in the right place (between version and verack). We’re having to change the code and BIP already, and the logic of “no software is known to support BIP155 that doesn’t announce at least that protocol version number” means that such a change would be safe.

Negotiating the addr relay version up front and treating it as const for the peer’s lifetime is a much simpler mental model. It means we can be sure that a peer’s addr relay version won’t change between receiving an addr and trying to relay it to that peer.

Currently, if we receive a tor3 address, we’ll choose which peers to relay it to in RelayAddress() without consideration of whether those peers support addrv2. When it’s time to send our addrs to those peers in SendMessages(), we won’t be able to serialize them. I think that effectively means any non-addrv2 peers become black holes for addrv2 addresses.

We solve that by only considering addrv2 peers as the relays on RelayAddress(), and that’s much easier to reason about if we know that the peer’s address relay version is constant and won’t change out under us.

laanwj commented at 10:36 am on December 4, 2020: member

@jnewbery I vaguely remember that there were some problems with sending messages between version and verack, but not sure.

Sjors commented at 4:57 pm on December 4, 2020: member

Tested 152b0be70ab5e910a6682d2178c2e81c781a2f9b against Breez and a standalone Lnd 0.11.1-beta in neutrino mode (which uses btcd as a library). They no longer hang up on my bitcoind.

lontivero commented at 5:45 pm on December 4, 2020: contributor

Concept NACK. I am currently implementing BIP 155 in NBitcoin which announces version 70012. This means that even when some clients implement addrv2 they will not receive the signal and share addrv2 addresses. @NicolasDorier what do you thing?

Fwiw I agree with @vasild that signaling new functionality via service bits would be more flexible. Signaling a feature with a new message was surprising to me.

sipa commented at 5:57 pm on December 4, 2020: member

This is not the time and place to discuss how version negotiation should be done in general. But to avoid reiterating things elsewhere: service flags are for feature discovery, not for feature negotiation. There are only a limited number of them, and for the purpose of negotiation they have no advantage over just send* messages like sendcmpct, sendheaders, … @laanwj Sending between version and verack works, we do it for wtxid relay. I don’ think there is a particularly strong reason to change that for BIP155 at this point though. @jnewbery I agree that would be slightly cleaner, but I don’t think we should make such a change at this point in an rc anymore. @lontivero That’s a good point against this, philosophically. In practice there is no problem for you though - you can announce 70016 as soon as you support BIP155. As all protocol changes since 70012 were just enabling new messages, there would be no other action required from you (70013 adds feefilter, 70014 adds compact blocks, 70016 adds wtxid relay; but if you don’t care about any of these, just ignore the messages they send to negotiate support - they only actually change behavior if you respond to those negotiation messages).

lontivero commented at 6:04 pm on December 4, 2020: contributor

Concept ACK then.

practicalswift commented at 11:42 pm on December 4, 2020: contributor

A swift Concept ACK for purely practical reasons!

laanwj commented at 8:46 am on December 7, 2020: member

@laanwj Sending between version and verack works, we do it for wtxid relay. I don’ think there is a particularly strong reason to change that for BIP155 at this point though.

Maybe, I don’t know, it makes some sense to handle different extensions in a consistent way.

benthecarman commented at 2:05 pm on December 7, 2020: contributor

tACK 152b0be70ab5e910a6682d2178c2e81c781a2f9b

Bitcoin-S needed to update to allow parsing of unknown messages to be able to sync with 0.21.0rc2. I think this change makes sense so clients don’t receive messages they aren’t expecting.

Don't send 'sendaddrv2' to pre-70016 software c5a8919660

sipa force-pushed on Dec 7, 2020

sipa renamed this:
~~Don't send 'sendaddrv2' to pre-70016 software~~
Don't send 'sendaddrv2' to pre-70016 software, and send before 'verack'
on Dec 7, 2020

sipa commented at 5:15 pm on December 7, 2020: member

Added a commit to move the sending of sendaddrv2 to before sending verack, matching https://github.com/bitcoin/bips/pull/1043.

laanwj commented at 5:21 pm on December 7, 2020: member

re-ACK b2eae522631289a9e4b7a2e80a0f761a86b65093

benthecarman commented at 5:22 pm on December 7, 2020: contributor

ACK b2eae522631289a9e4b7a2e80a0f761a86b65093

jonatack commented at 5:55 pm on December 7, 2020: member

ACK b2eae522631289a9e4b7a2e80a0f761a86b65093

sipa force-pushed on Dec 7, 2020

benthecarman commented at 6:01 pm on December 7, 2020: contributor

reACK ccd0eaab953e77d80891702243835a548708a365

in src/net_processing.cpp:2544 in ccd0eaab95 outdated

2540@@ -2535,6 +2541,11 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2541         return;
2542     }
2543 
2544+    if (msg_type == NetMsgType::SENDADDRV2) {

jnewbery commented at 6:01 pm on December 7, 2020:

I think we want to disconnect here if the sendaddrv2 message is received after VERACK (assuming that the suggested change to the BIP here: https://github.com/bitcoin/bips/pull/1043#discussion_r537711662 is taken)

MarcoFalke commented at 6:27 pm on December 7, 2020:

In theory we are free to not strictly enforce the BIP. So even with the suggested BIP change, the code here as is shouldn’t cause any issues.

vasild commented at 8:08 am on December 8, 2020:

Disconnecting if we receive sendaddrv2 after verack might make sense for the following reasons:

Allowing sendaddrv2 after verack defeats the purpose of knowing for sure that the peer does not support addrv2 (if we don’t receive sendaddrv2 before verack)
If an odd implementation starts sending sendaddrv2 after verack and we accept it, then we would be forever doomed to accept it because if we start disconnecting at some point in the future, then we would “break the existent app and network” (sounds familiar?).
It is what we do wrt wtxidrelay

jonatack commented at 5:03 pm on December 8, 2020:

Agreed. I’d be in favor of aligning BIPs 155 and 339 on this, and their implementations, unless there is a good reason not to.

MarcoFalke commented at 5:43 pm on December 8, 2020:

closing as resolved

jonatack commented at 9:06 pm on December 14, 2020:

See this comment which provides reasons not to disconnect: #20646 (comment)

MarcoFalke commented at 6:28 pm on December 7, 2020: member

re-ACK ccd0eaab95

sipa force-pushed on Dec 7, 2020

MarcoFalke commented at 6:50 pm on December 7, 2020: member

re-ACK deef9340aede6503951b133d8ef24686f833946d

jonatack commented at 6:58 pm on December 7, 2020: member

ACK deef9340aede65039

sipa commented at 7:00 pm on December 7, 2020: member

Sorry for the extra pushes. I didn’t at first change the message handling to accept incoming sendaddrv2 before verack (thanks @jnewbery), and that the tests needed adapting as well. Making the test changes without the corresponding net_processing changes fails the tests now.

benthecarman commented at 0:14 am on December 8, 2020: contributor

ACK deef9340aede6503951b133d8ef24686f833946d

DrahtBot commented at 12:32 pm on December 8, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20599 (net processing: Tolerate sendheaders and sendcmpct messages before verack by jnewbery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

vasild approved

vasild commented at 4:23 pm on December 8, 2020: member

ACK deef9340aede6503951b133d8ef24686f833946d

Another reason to send sendaddrv2 before sending verack:

When we connect to a peer and try to advertise our Tor v3 address to them we try to do that:

immediately after receiving their version message. At this time PushAddress() does nothing because we still have not received sendaddrv2 from the peer and we assume they don’t support addrv2. This is doomed.
From SendMessages(). Notice: SendMessages() does nothing if the peer’s verack has not arrived yet. So, what happens with master during my tests is that the first execution of SendMessages() happens before we have received the peer’s sendaddrv2. Then AdvertiseLocal() does nothing (because we assume addrv2 is not supported) and our next self-advertise is scheduled for after ~24 hours. It will likely succeed then, but that is a huge delay.

I confirm that with this patch, in 2., the first execution of SendMessages() proceeds with the knowledge that the peer supports addrv2 and thus we actually advertise our address to them “immediately” after establishing the connection.

sipa force-pushed on Dec 8, 2020

sipa commented at 5:38 pm on December 8, 2020: member

Updated to require SENDADDRV2 to arrive before VERACK.

sipa force-pushed on Dec 8, 2020

Send and require SENDADDRV2 before VERACK

See the corresponding BIP change: https://github.com/bitcoin/bips/pull/1043

1583498fb6

sipa force-pushed on Dec 8, 2020

MarcoFalke commented at 5:45 pm on December 8, 2020: member

ACK 1583498fb6781c01ca2f33c09319ed793964c574

jnewbery commented at 6:30 pm on December 8, 2020: member

ACK 1583498fb6781c01ca2f33c09319ed793964c574

in src/net_processing.cpp:2547 in 1583498fb6

2540@@ -2535,6 +2541,17 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2541         return;
2542     }
2543 
2544+    if (msg_type == NetMsgType::SENDADDRV2) {
2545+        if (pfrom.fSuccessfullyConnected) {
2546+            // Disconnect peers that send SENDADDRV2 message after VERACK; this
2547+            // must be negotiated between VERSION and VERACK.

jonatack commented at 8:03 pm on December 8, 2020:

maybe refer to BIP155 here if you retouch; if you do I’ll update #20592 similarly to refer to BIP339

sipa commented at 8:59 pm on December 8, 2020:

I’ll do so if the BIP change is merged before this PR is.

jonatack commented at 3:25 pm on December 14, 2020:

proposed in #20646

jonatack commented at 8:06 pm on December 8, 2020: member

ACK 1583498fb6781c01ca2f33c09319ed793964c574

vasild approved

vasild commented at 8:15 pm on December 8, 2020: member

ACK 1583498

MarcoFalke merged this on Dec 9, 2020

MarcoFalke closed this on Dec 9, 2020

sidhujag referenced this in commit b3d84bcd0b on Dec 9, 2020

MarcoFalke referenced this in commit 9e806887a8 on Dec 10, 2020

MarcoFalke referenced this in commit bead935470 on Dec 10, 2020

MarcoFalke commented at 10:50 am on December 10, 2020: member

Backported in #20612

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

random-zebra referenced this in commit b4751e10ce on Aug 11, 2021

DrahtBot locked this on Feb 15, 2022

Don’t send ‘sendaddrv2’ to pre-70016 software, and send before ‘verack’ #20564

Conflicts