Btcd disconnects upon receiving sendaddrv2 #1661
Comments
|
Thanks for reporting this! The easiest way to resolve this is to permit unknown messages rather than disconnecting as we do now. However, what's puzzling to me is why a new message was introduced ( |
It depends on who you ask, but I think philosophically there are many folks (many of whom work in Core) who argue a p2p protocol with many implementations should never be dependent on a single version number to describe all the features supported. This is why Core has used empty messages to communicate protocol support for years.
Can you elaborate more on this? Throwing away bytes received should never be worse than receiving a lot of pings/other useless data, so it seems strange that there would be DoS issues here. |
|
FWIW bumping the protocol version was my initial proposal, but everyone preferred the solution to send a message for signalling instead for the sake of other implementations (e.g. having only part of the feature set but still wanting to use TorV3). For the record I agree with @TheBlueMatt's reasoning about permissionless in introducing features, but I am extremely disappointed that it turns out to be the method that is most problematic for other implementations. |
|
If bitcoin/bitcoin#20564 is merged, it will act as a temporary workaround to give btcd a bit of time to write and deploy a fix. It looks like there are already plans to implement a solution:
|
This would make it possible for old versions (that contain this change) to talk with newer versions without requiring a protocol version increment. Fixes btcsuite#1661
@Roasbeef feel free to correct me here, but I think the concern is you could have peer(s) that keep sending garbage commands that would otherwise be dropped |
|
@jakesyl Of course, peers could be sending garbage. But if that's a problem, then peers spamming you with ping commands or tx messages would be even more of a problem. For that reason disconnecting on unknown messages doesn't protect against attackers - they'd just use an easier mechanism anyway - but it does make it harder to use send* style messages for feature negotiation. |
|
got it- thanks for the clarification @sipa! |
Consider that before this "add a message to intro another message" approach, it was possible to ascertain the capabilities of a node purely based on the advertised node version + services. IMO this property is useful for implementations that prefer to maintain a stricter set of peers for functionality and discovery purposes. With this new approach, an implementation is now only able to ascertain the capabilities of a given node by parsing a combination of its version+services along with the user agent string (assuming we don't want to require the node to first attempt to connect to any other potential node to obtain this information). User agent strings themselves re finicky since users can change them to arbitrary values for most implementations, so node implementations are now forced to always connect out to a node, wait for w/e meta-extension message, before deciding to hang up or keep the connection or actually add the address to their local address manager. As a result, the protocol version + services no longer uniquely describe what set of messages a node is able to send and understand. Interactive probing is required to obtain the same information which was previously obtainable given only an addr message and/or initial version handshake. |
That mechanism could've been the services bit field available in the legacy addr message and the version handshake. Consider that if I want to make a new implementation that only prefers Alternatively, I could've just only connected out to nodes that advertised in their |
With the proposed change in bitcoin/bips#1043 that's actually still possible, as the sendaddrv2 will be sent before verack, so after the initial handshake you know what the peer supports. The same is done in BIP339 (wtxid relay).
That's a separate discussion. You're arguing here that there may be reasons why addrv2 support should be discoverable. If that's the case, it certainly should have been a service bit. But I believe the BIPs' authors intentionally chose not to make the feature discoverable as service bits are a very limited resource - addrv2 is simply an extension of addrv1. |
|
It looks like this draft BIP attempts to codify bitcoind's new approach going forward which is great so other implementations can know how to interpret/handle its new choice of mechanism to extend the protocol. Re ignoring messages or not: IMO it really depends on if it's reasonable (and IMO it is) for an implementation to expect that the protocol version+service define a subset of the existing Bitcoin P2P protocol. Why should I maintain a connection to a peer which is sending random messages I don't know the significance Stepping back this also begs the question: is sending "unknown" messages to another implementation compliant with the "Bitcoin P2P Protocol"? AFAICT, it's unclear since the "defacto" specification of the p2p protocol appears to be either the Bitcoin wiki or Bitcoin developer site. So it seems this behavior itself undefined, though I may be missing something. |
This is...dubious. I know I've written P2P clients that will happily send a high version number so that they get certain feature support, but don't implement other, older-version-number features. This has always worked as intended and is precisely the reason for the "empty message as feature negotiation" stuff - sure, it may not be the most elegant way to negotiate protocol features, but it works. Just sending a version number has never been enough to turn on certain feature in the P2P protocol (including in Bitcoin Core), many features have required, since day one, an additional negotiation step, and happily functioned without completing it.
As sipa indicated, this, sadly, just isn't very workable - service bits is a rather limited resource (there's only 32 of them), and as a result many protocol features do not have a feature bit assigned to them. As noted above, given that, there is no way to build a "only implement some of the features implied by protocol version X" client without building an additional version negotiation protocol, hence the use of dummy protocol negotiation messages. |
I think the answer depends on your perspective. Assuming the sender is not malicious, they are presumably using a protocol extension you don't know about - and the extension should have been chosen in such a way that it doesn't affect anyone who doesn't understand it. That has been the case for all protocol changes that I know of since 2014 (including sendheaders, feefilter, compact blocks, and wtxid relay). Those happened to always be accompanied with a version bump, but it really serves no purpose - the protocols are effectively negotiated using new messages, and the bump just determines whether that negotiation is even tried. I believe people felt this also doesn't belong in a protocol that should be permissionlessly extensible.
I think part of this debate is because apparently there was no common understanding about what the protocol was. That's unfortunate, and messy, but it's probably inevitable in any protocol between multiple systems without central authority to fix a specification. Bitcoin Core has always ignored unknown messages, so this issue comes a bit as a surprise. |
Other than Rationale earlier in this thread for this change was "it's easier on other impls" but it doesn't appear that other impls were consulted. I scanned the past year of bitcoin dev mailing list posts (for 2020), and the only tangential mention of this new p2p feature rollout is the mail by suhas that linked to that document, tho the motivation in the email was the upcoming wtxid change. It may have been discussed in the list in 2019, but I would think if a new p2p feature were close to being rolled out, there'd be another mail to give people a heads up.
Yeah I'm familiar with this opinion re service bit scarcity, to which my response usually is: why not just add more? The |
You're absolutely right about that. I think people incorrectly assumed that others would have been paying attention to the discussion on BIP155 over the past months, but looking back I don't think any of that made itself to the mailing list. |
feefilter (technically, I mean hard which way you really call this one), wtxidrelay, and compact blocks as well, and that's just from sipa's list above.
Right, I believe that rationale was selected about 10 years ago, maybe more :). Indeed, the issue is that these things largely weren't documented, only decided and then folks moved on. When others actually wrote other implementations, they didn't come up again.
Sure, but that's besides the point - there's a perfectly fine, even if inelegant, way by which features have been negotiated for many years - send a dummy message to turn it on, and only send that message if the protocol version is high. |
It applies to every message type, except Just to give some examples for illustration, but really it applies to every message type: For example, Another example, Another example, |
The de facto specification for the protocol was initially the Satoshi implementation. From the initial v0.1 release, unknown message types have been permitted as a mechanism to extend the p2p protocol: https://github.com/benjyz/bitcoinArchive/blob/master/bitcoin0.1/src/main.cpp#L2035-L2039. This has remained true for the Bitcoin Core implementation since that time. |
Bitcoin Core master, soon to be released, introduces the new
sendaddrv2message (BIP 155). When such a Bitcoin Core node connects to a btcd node (tag v0.21.0-beta) it immediately disconnects:On bitcoind side:
I tried with and without the new
peerblockfiltersfeature, but that doesn't seem to matter.This may lead to node loneliness. It also happens to Lnd nodes, see breez/breezmobile#408
This issue did not occur as of a slightly older Bitcoin Core commit, e.g. bitcoin/bitcoin@1d3ec2a
The text was updated successfully, but these errors were encountered: