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

[sipa]

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

[sipa]

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]

utACK 152b0be

[laanwj]

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]

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]

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]

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

[maflcko]

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

[maflcko]

(Added 0.21 milestone for now)

[jonasschnelli]

utACK 152b0be - 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]

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.

[maflcko]

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]

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]

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

[Sjors]

Tested 152b0be 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]

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]

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]

Concept ACK then.

[practicalswift]

A swift Concept ACK for purely practical reasons!

[laanwj]

@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]

tACK 152b0be

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.

[sipa]

Added a commit to move the sending of sendaddrv2 to before sending verack, matching bitcoin/bips#1043.

[laanwj]

re-ACK b2eae52

[benthecarman]

ACK b2eae52

[maflcko]

re-ACK deef934

[jonatack]

ACK deef934

[sipa]

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]

ACK deef934

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• net processing: Tolerate sendheaders and sendcmpct messages before verack #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]

ACK deef934

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:

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

2. 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]

Updated to require SENDADDRV2 to arrive before VERACK.

[maflcko]

ACK 1583498

[jnewbery]

ACK 1583498

[jonatack]

ACK 1583498

[jonatack]

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

[sipa]

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

[jonatack]

proposed in #20646

[vasild]

ACK 1583498

[maflcko]

Backported in #20612