Came up when working on #21528.
Currently, sending a SENDADDRV2 message to an outbound block-relay-only peer is harmless but misleading. However, if we move forward with the approached proposed in that PR, this would mistakenly indicate that the node is interested in participating in addr relay.
Would be great to include this in v22.0 so only v0.21 peers will have this misleading behavior.
Sending this message will mistakenly indicate that we are interested in
participating in addr relay.
ACK ee5fb1f8dcdca66af992f05081c71703aa43fdff
Reviewed code and checked that the test fails on master but not on this PR.
79+ conn.sync_with_ping()
80+ assert_equal(conn.message_count['sendaddrv2'], 1)
81+
82+ self.log.info("Check that the node does not send a SENDADDRV2 message to outbound block-relay-only peers")
83+ conn = self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=1, connection_type="block-relay-only")
84+ conn.sync_with_ping()
question in ee5fb1f8dcdca66af992f05081c71703aa43fdff:
Is there a reason for the sync_with_ping’s? Ideally the add_outbound_p2p_connection helper should fully setup the connection (including handshake). And it seems it does do that, as there is a p2p_conn.wait_for_verack() and p2p_conn.sync_with_ping() inside.
sync_with_ping() inside add_outbound_p2p_connection, so I was adding buffer.
review ACK ee5fb1f8dcdca66af992f05081c71703aa43fdff 🌸
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK ee5fb1f8dcdca66af992f05081c71703aa43fdff 🌸
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUhwDQv/aA/IrlAXamITFkzobmrxfxxEGwySzzaoCGBQLhmIhFPWYNJ7a/VS1NOY
84w9blPA6MWMPBMbO/FxhIBnMIDN6gUZk56uHqqUBUT/QzBm0EdXJ4sFyeKIBwEfn
9t1bu+QSmHpXL/6/1oJMcXYUn9NRB9UrTFv1JAYfZmaTqCdeh4Z8dCsEOeXwWKH5z
102AS/GyadZ+WRtUNv5NALMLi4L+ieORbHwHJuQmXrxd7gtq2FQMvBy/ttMgSKM8Ow
11RvEpvnTFP1pHFM1FH92+2hZ+2DoIrHwU+Ga6BwPjybR+86FfKrw89SJCPuB79+Y0
12pZ/e+6PnZ+R9e7tf206ccrOjzu1I8BBtc8nd9sAJtOWi85ubAH7c8AicVPmYZH8+
13tq6qcJpEb/c91oehGdLBB5P7IhmdB60mPSbqXFkS0+YuVYIsazle704IZT3iDKnd
14LJbehq3611lbFvV4Pbiwkh/higfwsyXnGIawjb7MMF6RvBr3iVWfTue3Ju0VgtiN
15+AeAcO6k
16=AgQN
17-----END PGP SIGNATURE-----
Timestamp of file with hash 544b364255daf646e0efe38975b58e28d3de4b2d668e04228a24fe7fb181e59e -
easy re-ACK 89524b25efc397b9fb89536d827df3a600b6fbdf 🏳
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3easy re-ACK 89524b25efc397b9fb89536d827df3a600b6fbdf 🏳
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUi0MAv/YQTTCEElYXs2V3M0yc4sL22r/0V9G0RBxJOy95a6lFev8gkwnAhlUaTY
8cE5bogD3Iy5EzNXYmKQQsI/6LyaBEkgBInlraDA0YtZdJ6NDAeiEBBVT9npeaLVh
9LM5NG6guJaFdDPVCJHc2zdrNyJBt65B3jUCCPRb6Z10LOCRsiz43W5OsRk9vEQDx
10pbQVNAkyfDBfrlQ8EogDTxNyeDw3yluQBghbWm4di88KNtMK0xP4NMN4FKMmnUtm
115VjME2xMlBvOscp9ja4U1o4eWnS3WCT53tkh5lcoSCjARfIi4mqC+HiFZr00TRd9
120tcxv+T6YW5B2292KzW+UCrmzBC2X7lj7eCfpqJ0AdKRJ2jAviHL76Tv1nUKCuLS
13alyKmXCgtk2bTk+0ts0lYsv59ecbvP0KW1wbkI3BlauM+vEhDxhlQ3nTQ8VCXVQa
14yL0LuWgY92pxx0Od/y5HJXZw0MkeZkTYsFxxy8N/7uZhL2JmPWvZ3iPP7rsG8VZI
15c1gEV58T
16=R9+p
17-----END PGP SIGNATURE-----
Timestamp of file with hash 2a48f9b25bb976253d76e769baa2281f8a5327db2b65c3cb3f3e909d023e9827 -
We might want to update BIP155 for this (@vasild what do you think?). How I’ve always interpreted is that in principle, the sendaddrv2 message means that the sending node supports v2 addr messages. This is orthogonal to whether it wants to receive address messages in the first place.
I’m not opposed to combining these, though it’s not clear to me how a node would signal it doesn’t want to receive address messages at all.
@MarcoFalke, @jnewbery - thanks for the quick re-reviews!
sorry to have force-pushed again, but looking at #22243 made me realize I was introducing potential for a race here, so I added acquiring p2p_lock to the new tests.
@laanwj
We might want to update BIP155 for this…
Are you thinking something like “Clients are NOT RECOMMENDED to send a sendaddrv2 message if they are not interested in participating in address relay over the connection.”? It doesn’t seem necessary to me, but also doesn’t seem harmful.
though it’s not clear to me how a node would signal it doesn’t want to receive address messages at all.
Have you seen #21528? I’m proposing a “opt-in” approach vs an “opt-out” approach for propagating addr self-announcements to inbound peers.
re-ACK 7c141b77764359dedc8815ce8484f42f87727dbe
Rescinding this as it seems this change is contentious.
The current implemented behavior of sendaddrv2 is a clean binary signal: BIP155 addrv2 format support true/false. It may be good to update BIP155 to clarify that.
Consequences of the proposed widening of sendaddrv2 semantics (if my thinking isn’t muddled):
loss of signal: we can no longer know if a block-relay peer does not support addrv2
irreversibility: once propagated across the network in a release, reverting the change cannot recover the lost information from peers using that release, so if we need it for a reason that we don’t foresee today, we would need to introduce a new message mechanism again
It seems there should be a very compelling reason beyond “it’s harmless but currently irrelevant to block-relay peers” to irreversibly widen the semantics from the current implementation, and that the BIP could be clarified.
@jonatack this was discussed briefly in the p2p irc meeting last night: https://www.erisian.com.au/bitcoin-core-dev/log-2021-06-15.html#l-284
I agree with what @sipa said in that meeting:
0304 2021-06-15T21:16:13 <sipa> if you don't care about addresses, sending sendaddrv2 is just irrelevant
BIP 155 is explicit about what the sendaddrv2 message means:
Introduce a new message type sendaddrv2. Sending such a message indicates that a node can understand and prefers to receive addrv2 messages instead of addr messages. I.e. “Send me addrv2”.
If a node doesn’t wish to receive any addr messages, then it shouldn’t send a sendaddrv2 message to request its peer to “Send me addrv2”.
The sendaddrv2 message has a very clear purpose - to announce that the peer who emitted it supports the addrv2 format (and obviously prefers to receive addrv2 instead of addr). This is documented in BIP155. If the semantic of sendaddrv2 is to be changed, that should start by modifying BIP155. There must be a very compelling reason (as @jonatack mentions above) to change it, given that its already deployed and running on mainnet. Tweaks to address relay is not such a reason, IMO.
There already was a discussion whether to add extra meaning in sendaddrv2 before it was introduced (whether to tie it to relay/not-relay). It concluded like this.
On the organizational level - I think it is not a good idea to rush such a change to the code and the protocol near or past the feature freeze (this PR was opened a few hours before FF).
On the technical level - I think it would be most simple and flexible to introduce a new flag “I (don’t) want unrequested addresses”. This could be done with either a new message or a service bit. Having two separate flags for the supported format (A) and the desire to receive gossip (B) would allow all 4 combinations, let’s say:
A - support for addrv2 (currently equal to sending sendaddrv2)B - want unrequested addresses!A and !B (it follows that if you send me a reply to my getaddr it should be in the old addr format because I don’t support the new addrv2)!A and BA and !B (it follows that if you send me a reply to my getaddr I prefer it to be in the addrv2 format)A and BSignalling “I want unrequested addresses” (B) could mean that I either maintain an address database and/or participate in gossip. Which one it is (or both) is irrelevant to my peer who only needs to know whether to gossip to me or not.
#17194 is related, cc @naumenkogs
BIP 155 is explicit about what the sendaddrv2 message means:
Introduce a new message type sendaddrv2. Sending such a message indicates that a node can understand and prefers to receive addrv2 messages instead of addr messages. I.e. “Send me addrv2”.
If a node doesn’t wish to receive any addr messages, then it shouldn’t send a
sendaddrv2message to request its peer to “Send me addrv2”.
Oh, well, I was thinking that the BIP155 wording is clear enough, but apparently it is not. The intention is to say “send me red apples instead of green apples (if you are going to send me any apples at all)”, not to mandate whether I want apples or not. Edit: opened https://github.com/bitcoin/bips/pull/1134 to clarify the wording.
I don’t think that #21528 is strictly dependent on this PR, in that it would work less well without including SENDADDRV2 in the list of messages indicating willingness to participate in addr relay. There would still be ADDR/ADDRV2 and GETADDR left as an indication.
In the short term, it might even work more precisely, because black-hole peers that send out SENDADDRV2 without an intent to participate in addr relay on this particular connection (notably block-relay-only connections of v0.21 nodes) wouldn’t be incorrectly identified as peers interested in addr relay.
If the semantic of sendaddrv2 is to be changed, that should start by modifying BIP155
This PR is not changing the semantics of BIP155. It’s simply changing the Bitcoin Core node behavior to not send a message that is irrelevant. If we ignore all addr messages on a connection, then there’s absolutely no point in sending a message on that connection which says “please send addrs in such-and-such format”.
The intention is to say “send me red apples instead of green apples (if you are going to send me any apples at all)”, not to mandate whether I want apples or not.
This doesn’t make sense to me. Why would I say “send me red apples instead of green apples” when I don’t eat apples at all and I’m going to put all the apples in the trash?
To summarize what the current behavior is for block-relay-only connection:
sendaddrv2 messageaddrv2 messagesaddrv2 messagesafter this PR:
addr messagesaddr messagesThere’s essentially no change to our addr processing. The only change for the peer is that they’ll serialize addresses as addr instead of addrv2. There are no implications or changes to the protocol.
The semantic of sendaddrv2 is to announce support for the addrv2 format (if you find the text in BIP155 vague, refer to the code - it does exactly that and the code not vague).
If the semantic of sendaddrv2 is to be changed, that should start by modifying BIP155
This PR is not changing the semantics of BIP155.
This PR+https://github.com/bitcoin/bitcoin/pull/21528 change the semantic of not only BIP155 (sendaddrv2) but to the getaddr, addr and addrv2 messages. I think both PRs should be considered together. The change in semantic is because, e.g. getaddr is changed from “give me some addresses” to “give me some addresses and I participate in address relay, so if you send me addresses now or unrequested later I promise to further relay them to others, I am not a black hole”.
It’s simply changing the Bitcoin Core node behavior to not send a message that is irrelevant. If we ignore all addr messages on a connection, then there’s absolutely no point in sending a message on that connection which says “please send addrs in such-and-such format”.
sendaddrv2 does not say that, see the top of this post. But anyway - looking at this PR in isolation could be misleading as the big picture is in #21528.
The intention is to say “send me red apples instead of green apples (if you are going to send me any apples at all)”, not to mandate whether I want apples or not.
This doesn’t make sense to me. Why would I say “send me red apples instead of green apples” when I don’t eat apples at all and I’m going to put all the apples in the trash?
You may be eating apples after all. What if we look beyond the Bitcoin Core’s block-relay mode? There may be a case where one does not want unrequested address messages, but could send getaddr and wants replies to that to be in addrv2 format. This cannot be done with your interpretation of sendaddrv2, this PR and #21528.
To summarize what the current behavior is for block-relay-only connection: * we make a connection out to the peer * we send a
sendaddrv2message * the peer sends usaddrv2messages * we ignore thoseaddrv2messagesafter this PR: * we make a connection out to the peer * the peer sends us
addrmessages * we ignore thoseaddrmessagesThere’s essentially no change to our addr processing. The only change for the peer is that they’ll serialize addresses as
addrinstead ofaddrv2. There are no implications or changes to the protocol.
I agree that this PR alone looks innocent, but then if we are to look at this PR alone, the change is also pointless, so why do it?
@vasild Am I correct in interpreting your position this way: address gossip is considered to be a well-defined aspect of the P2P protocol, and given that #21528 is changing the semantics of a number of messages, including sendaddrv2, that change matters. It’s not specifically this PR you’re concerned about, but the fact that 21528 is changing semantics (which this PR relies on), and those semantics don’t match BIP155.
If so, I’d counter with my view that addr gossip is only a local policy choice that every client makes, and is (so far) not the subject of any BIP at all. Network rules are defined by more than just BIPs, and expectations of network nodes matter just as much. However, in this case, given the fact that lots of actually deployed nodes on the network don’t and never have participated in addr gossip at all, I think it’s fair to say that choosing to not do so isn’t much of a break. Because of that, I think that these PRs are best seen as only changing heuristics about what and where to relay addresses. Perhaps going further than earlier heuristics we had, but still, clients are free to relay whatever they want or nothing at all. It’s not related to BIP155, because that BIP specifies addrv2 relay and announcing support for it; the choice of gossip or not is outside of its scope.
I do think the lack of clarity in the situation is unfortunate, and could be improved if someone wants to put in the work of analyzing the goals and expectations of address gossip, by writing up a specification for messages for explicitly opting in or out of address relay. That would supersede all this. I don’t think it’s necessary to formalize this aspect of the protocol before being able to make improvements, though.
The change in semantic is because, e.g. getaddr is changed from “give me some addresses” to “give me some addresses and I participate in address relay, so if you send me addresses now or unrequested later I promise to further relay them to others, I am not a black hole”.
I think that interpretation is inaccurate. There is no expectation of not black holing. Even if you see it as a specification change, at best it means “I want to be served addresses” - the receiver is free to use them directly or not, put them in a database or not, relay them or not, or none at all and just drop them. The aim of 21528 is just not knowingly relaying to those who don’t care about addresses anyway. If an attacker wants to be an intentional black hole, they can keep claiming they want addresses and ignore them.
You may be eating apples after all. What if we look beyond the Bitcoin Core’s block-relay mode? There may be a case where one does not want unrequested address messages, but could send getaddr and wants replies to that to be in addrv2 format. This cannot be done with your interpretation of sendaddrv2, this PR and #21528.
I think that’s fine. It’s a heuristic and it is expected that there can be situations where it is overly conservative. Receiving gossip is the status quo, so when there isn’t a signal to infer the other party isn’t interested in it, we should assume that they want it. There is no harm done, because it’s the same as we do now. Nothing here prevents or worsens the ability for an actual protocol message from explicitly opting out of gossip, if that were desirable.
Would it ease your concerns if the interaction with sendaddrv2 was removed (i.e., this PR closed, and sendaddrv2 removed from triggering gossip in #21528)? My understanding is that it is only added for consistency reasons to 21528, and that that necessitates this PR. The advantages of the current approach are that the heuristic is more conservative, so (even) less likely to miss situations where gossip is desired, and more consistent (“you’re doing something related to addr relay, you probably care about gossip”). On the other hand, if sendaddrv2 is excluded, this PR isn’t necessary, and block-only connections from 0.21 nodes will also get the benefit. Perhaps that makes sense; it’s treating sendaddrv2 purely as a configuration setting then, and to opt into gossip, you have to actually ask or send addresses.
Would it ease your concerns if the interaction with
sendaddrv2was removed (i.e., this PR closed, andsendaddrv2removed from triggering gossip in #21528)? My understanding is that it is only added for consistency reasons to 21528, and that that necessitates this PR.
My take is that the approach in #21528 — of inferring addr relay heuristics from observed messages to improve network behavior — is conceptually fine, now that we’ve put the work in to minimize the chance that the changes there could break other software, but that changing the way we send sendaddrv2 to try to achieve some perceived consistency with the legacy addr messages is purely an aesthetic choice and not a functional one. So overall I think this choice doesn’t matter much, but it does seem both aesthetically and functionally worse to me that we’d stop sending this message as proposed here, because (a) there is some benefit as @jonatack mentioned for measuring deployment of BIP 155 by always sending and (b) even aesthetically I think that it’s better that signaling of support for addrv2 should be orthogonal to addr relay negotiation itself, so in the future when I hope someone proposes some more comprehensive addr relay protocol I think we will just want to uncouple these things anyway. In general I think we don’t want to have complicated interactions to reason about when different combinations of p2p messages are sent, and BIP 155 is nice and simple as it is.
Another way of putting it is if you think of the heuristics of #21528 as a sort of hack to achieve better addr behavior in the absence of protocol support for the functionality we want, then keeping that hack as small as possible seems best. I don’t think anyone believes that implicit behavior (that is undocumented in any BIP) is the right way for protocol development to proceed if starting from a clean slate — so eventually I think we’ll rewrite things.
Perhaps that makes sense; it’s treating sendaddrv2 purely as a configuration setting then
This is precisely how I see it. (Again though I think it doesn’t matter very much, this seems like mostly an aesthetic debate, though I do think this proposal is slightly worse than the alternative of leaving this logic unchanged.)
I do think the lack of clarity in the situation is unfortunate, and could be improved if someone wants to put in the work of analyzing the goals and expectations of address gossip, by writing up a specification for messages for explicitly opting in or out of address relay. That would supersede all this. I don’t think it’s necessary to formalize this aspect of the protocol before being able to make improvements, though.
I think this would be a worthy specification if addr relay is more relied on in the future by lightweight clients (for e.g a wider number of BIP157 filters consumers, that Core encourage the deployment of with its current network policy). An under-specified, buggy addr gossip mechanism would lure to a poor quality of their addrman diversity, or even ease malicious poisoning of their network view.
As for now, I believe the impact of our network policy changes on the rest of the ecosystem have been well-studied in #21528, enough to clear up the concern of breaking other Bitcoin clients, and letting code improvements happening in the lack of aforementioned specification.
(a) there is some benefit as @jonatack mentioned for measuring deployment of BIP 155 by always sending
On this specific point of gathering statistics or risk of “irreversible changes”, I don’t think that’s a blocker for this PR, as this only concerning our outbound block-relay peers. A bip155 deployment measuring tool could connect as an inbound full-relay peer, sends sendaddrv2 and wait for answer to assert the support ? Though it might be more hacky to write such software for edge-cases I’ve not weighted in…
From a procedural point, I agree with vasild that if this change is contentious, in the sense all technical concerns have been duly addressed, and if we’re beyond feature freeze it’s better to be conservative and slow down the pace, imho.
sendaddrv2 as signal from that PR.
Everybody, thanks for your comments! I read everything carefully, slept over this and the following scenario came to my mind:
A (outbound, blocks only) -> B (inbound)
A does not send sendaddrv2 (because it is blocks-only, due to this PR)A does not send getaddr (because it is blocks-only, as in master)B sends sendaddrv2So B thinks that A does not support addrv2. Later B requests some addresses from A via getaddr (yes, Bitcoin Core does not send getaddr in incoming connections, but another software, or a modified Bitcoin Core may do) and gets an addrv2 response, which comes as a surprise because A never announced support for addrv2. Currently this would not have happened because A, being in blocks-only mode would ignore the getaddr from B. I would say it is better to make sure we never send addrv2 if we did not announce our support for it via sendaddrv2, but I see this PR is now closed, so that is irrelevant.
More importantly, I came to the realization (or delusion) that just getaddr is enough for the purposes of #21528, will continue the discussion there.