net: cbfilters: Don't disconnect peers for requesting unknown blocks. #34933

Some BIP157 light clients are not complying with the following requirement:

StopHash MUST be known to belong to a block accepted by the receiving peer. This is the case if the peer had previously sent a headers or inv message with that block or any descendents. A node that receives [getcf(ilter|header|checkpt)s]with an unknown StopHash SHOULD NOT respond.

https://github.com/bitcoin/bips/blob/master/bip-0157.mediawiki#getcfilters

During the recent 2-block reorg I observed on one node's logs that it disconnected BIP157 peers with the version string /btcwire:0.5.0/neutrino:0.12.0-beta/ for asking for filters from the other side of the block race:

[net] peer requested invalid block hash: 95b0d0f836f18d78dfbae435fb37bf6fff2e37a4fc01c20be8495bc3ea4fbb58, disconnecting peer=8502032, [...]

(https://bnoc.xyz/t/raw-data-request-block-filter-peers-being-disconnected-due-to-invalid-block-requests/101)

The broken implementations should fix themselves, but disconnecting is too harsh of a punishment for this mistake, this PR keeps the log message and returns early without responding when this happens, but doesn't set fDisconnect=true.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

#29655 looks related (not a reorg there, but a race in which peers asked for the filter before the node had connected the block). Looks like Neutrino may still not be fully compliant with BIP157?

I don't know if not disconnecting the peer is necessarily better though - in some cases it may be preferable if a non-compliant peer gets disconnected (so that they could 1) ask another peer and 2) fix their software)? If we just ignore the request, this may result in them unnecessary waiting for an answer that never comes, plus never noticing that there is a problem on their side.

I've made an issue on their repo if you want to chime in with more details: https://github.com/lightninglabs/neutrino/issues/338.

I've followed @davidgumberg's request for data on this and posted it in https://bnoc.xyz/t/raw-data-request-block-filter-peers-being-disconnected-due-to-invalid-block-requests/101/2

but disconnecting is too harsh of a punishment for this mistake,

I don't see why this is the case? They can still reconnect, and I don't think they're marked as discouraged let alone banned or anything?

Given some of the discussion in https://github.com/lightninglabs/neutrino/issues/338:

Yeah, so it's an race condition, that cannot be avoided unless both sides make strict assumptions about the concrete protocol implementation of the other.

There'll always be a gap in which I think you have the block (either explicit or implicit), but while my request is in flight, you re-org away from it.

It does seem like even though some nodes are actually misbehaving (sending filter requests for blocks to peers that haven't INV'ed or HEADERS'ed the blocks), because the protocol is underspecified, there are lots of ways that nodes can be "compliant" with the BIP, but send requests for blocks a node does not have filters for at the time of receiving the request.

If we just ignore the request, this may result in them unnecessary waiting for an answer that never comes, plus never noticing that there is a problem on their side.

I don't see why this is the case? They can still reconnect, and I don't think they're marked as discouraged let alone banned or anything?

Maybe I am overestimating this, but isn't it a bit dangerous to disconnect peers for behaviors that almost exclusively concur with reorgs and block races since this increases the likelihood of the network splitting? I mean I realize that light client nodes don't propagate to others so some of the feedback effect of disconnecting a full node is not present, but it would still be bad if light clients ended up peered with only one side of a race / fork because of disconnect logic?

So ideally it seems that we should disambiguate between two cases:

1. a request for a block that was never INV'ed or HEADERS'ed - disconnection is the punishment?
2. a request for a block that was INV'ed or HEADERS'ed but which is no longer part of a node's active chain - ignore the request?

I'm really not sure, looking back at the BIP it says:

A node that receives [getcf(ilter|header|checkpt)s] with an unknown StopHash SHOULD NOT respond.

It kind of seems reasonable for light client implementations to interpret non-response as a sign that the node does not have the block, so it does seem like this is a reasonable response for 2), and honestly I don't think it's that bad of a way to handle 1).

Maybe I am overestimating this, but isn't it a bit dangerous to disconnect peers for behaviors that almost exclusively concur with reorgs and block races since this increases the likelihood of the network splitting? I mean I realize that light client nodes don't propagate to others so some of the feedback effect of disconnecting a full node is not present, but it would still be bad if light clients ended up peered with only one side of a race / fork because of disconnect logic?

Why not just fix the bug in the light client? If a buggy client gets stuck one one side of a fork, but a non-buggy one doesn't, that seems like a good reason to fix the bug to me. For clients that don't participate in forwarding/validating blocks I don't think there's reason to be concerned about a network split -- only the buggy clients are impacted, not the network as a whole. If the fork resolves itself, then the light client will be fine; and if it doesn't resolve itself, I doubt the light client will behave well anyway -- if you're testing in fork environments you'd likely catch this bug already, and if you're not, you probably haven't written much code to cope with a sustained fork?

a request for a block that was INV'ed or HEADERS'ed but which is no longer part of a node's active chain - ignore the request?

Outside of compact blocks, we should only be announcing blocks we've validated and included in our active chain, so I think it would be fair to respond to requests related to such blocks, even if they've since been reorged out. My impression is that the code already handles this fine though?

because the protocol is underspecified, there are lots of ways that nodes can be "compliant" with the BIP, but send requests for blocks a node does not have filters for at the time of receiving the request.

I don't see how this could actually happen, see https://github.com/lightninglabs/neutrino/issues/338:https://github.com/lightninglabs/neutrino/issues/338 and #29655 (comment):

Ah, my mistake, I did not look carefully at BlockRequestAllowed, closing for now, discussion should continue in #29655 and lightninglabs/neutrino#338.