Disconnect Peers for Duplicate Invalid blocks.

achow101 commented at 6:25 pm on October 3, 2017: member

I have updated this to be as we discussed in the IRC meeting. If we see a block header for a block we already know is invalid, we disconnect from the node that sent it. The only exception are our HB CB nodes.

~~This is an implementation of the bad block interrogation idea that @gmaxwell has talked about.~~

~~To ensure that the nodes we are connected to are following the same consens rules as we are, ban all nodes that relay us an invalid block or block header instead of only banning the first peer.~~

Then when we receive an invalid block, we want to check that all of our peers also found that block to be invalid, so we ask all of our peers for the header of that invalid block. If they found it valid, the header will be relayed and we will then ban them. Otherwise we keep the connection.

To ensure that our newly connected nodes are also following the same consensus rules, we ask them for the headers of blocks that we know are invalid when they connect after the version handshake. If they respond with a header, then we will ban them.

~~This still needs tests, but I’m not sure how we should do that. If you have any suggestions, I’m all ears.~~

~~Note, I know the implementation is slightly wrong, still working on it.~~

MarcoFalke added the label P2P on Oct 3, 2017

in src/net_processing.cpp:1381 in e41ceb2edf outdated

1374@@ -1370,6 +1375,17 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
1375             connman->PushMessage(pfrom, CNetMsgMaker(nSendVersion).Make("alert", finalAlert));
1376         }
1377 
1378+        // Ask this peer for the block headers of all of the blocks we know are invalid
1379+        // If we receive headers messages for these invalid blocks, we will ban them.
1380+        std::vector<uint256> invalid_block_hashes;
1381+        for (const std::pair<const uint256, CBlockIndex*>& item : mapBlockIndex)

theuni commented at 7:04 pm on October 3, 2017:

Should probably wait until VERACK for this.

Also, walking mapBlockIndex seems a little heavy here, no?

achow101 commented at 7:13 pm on October 3, 2017:

Besides walking mapBlockIndex, what do you suggest? I was considering making something to just store the hashes of the invalid blocks instead.

theuni commented at 7:47 pm on October 3, 2017:

Yea, I’d say the fork-points need to be cached. Unsure if it’d be best to just do it here and be done with it, or maintain something in validation.cpp that (for ex) getchaintips could also use.

achow101 commented at 7:56 pm on October 3, 2017:

Well it would have to be something that is loaded/generated on startup too.

achow101 force-pushed on Oct 3, 2017

theuni commented at 7:43 pm on October 3, 2017: member

Is it really necessary to request as many headers as possible on each bad chain?

What about something like sending a request for non-deterministic headers from m to n, wherem = forkheight - rand(x), n = max(m + rand(2x), tip) from all chains (including the active one), that way we can also interpret ignored valid requests?

achow101 force-pushed on Oct 3, 2017

in src/net_processing.cpp:1995 in ad944f3eed outdated

1988@@ -1972,6 +1989,11 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
1989                     Misbehaving(pfrom->GetId(), nDoS);
1990                 }
1991                 LogPrintf("Peer %d sent us invalid header via cmpctblock\n", pfrom->GetId());
1992+                // Send a getheaders message with the hash of the invalid block to all peers.
1993+                // Those who respond to this message will be banned.
1994+                connman->ForEachNode([cmpctblock, connman](CNode* pnode) {
1995+                    connman->PushMessage(pnode, CNetMsgMaker(pnode->GetSendVersion()).Make(NetMsgType::GETHEADERS, CBlockLocator({cmpctblock.header.GetHash()}), uint256()));

theuni commented at 7:56 pm on October 3, 2017:

Feedback loop here.

achow101 commented at 7:57 pm on October 3, 2017:

how so?

theuni commented at 8:05 pm on October 3, 2017:

Node sends you bad header, you request that header from all nodes (including the one that just sent it). Same node replies with same header, you request it again from all nodes… repeat until they get banned.

achow101 commented at 8:07 pm on October 3, 2017:

Isn’t the node that sent it to you in the first place already banned by the time this runs?

theuni commented at 8:42 pm on October 3, 2017:

No, not necessarily. See “high-hash” for example. But even still, the ban/disconnect should not be assumed. What if they’re whitelisted?

in src/net_processing.cpp:2319 in ad944f3eed outdated

2312@@ -2289,6 +2313,11 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2313                     LOCK(cs_main);
2314                     Misbehaving(pfrom->GetId(), nDoS);
2315                 }
2316+                // Send a getheaders message with the hash of the invalid block to all peers.
2317+                // Those who respond to this message will be banned.
2318+                connman->ForEachNode([header_hashes, connman](CNode* pnode) {
2319+                    connman->PushMessage(pnode, CNetMsgMaker(pnode->GetSendVersion()).Make(NetMsgType::GETHEADERS, CBlockLocator(header_hashes), uint256()));

theuni commented at 7:56 pm on October 3, 2017:

feedback loop here too :)

achow101 commented at 8:01 pm on October 3, 2017: member

Is it really necessary to request as many headers as possible on each bad chain?

No.

that way we can also interpret ignored valid requests?

I’m not sure if that is useful or necessary. How do you know the difference between a node not having a block, rejected the block, or is just slow to respond?

theuni commented at 8:46 pm on October 3, 2017: member

Cautious concept ack on doing this once, sometimes, post-handshake, but I think this needs to be thought through a bit more.

Amplification bugs seem easy to introduce here. I’d be more comfortable if the testing was non-deterministic, and done more like ~50% of the time.
Nodes can just ignore any block locator containing different chains.
It seems unnecessary/wasteful to do this for each block that comes in. Once you’ve been connected to a peer for a while and you’re likely on the same chain, if you diverge, it should be already observable. What does this offer over the request done after the handshake?
I think this makes it trivial to construct a network-wide connection graph?

gmaxwell commented at 8:46 pm on October 3, 2017: contributor

I don’t understand why this is generating any headers requests at all, we already will synchronize all headers on the peers best chain (obviously not transferring any we already know about). We just need to see that they connect to an invalid block and disconnect.

JeremyRubin commented at 9:06 pm on October 3, 2017: contributor

I don’t really like this policy, I wouldn’t go as far as a Concept NACK though because I think it can be adjusted to do what you want.

I think returning an invalid block when requested is a weak indicator of peer correctness overall. Nodes to assuming “you get what you ask for” and feeding you invalid data – if you ask for it and they have it – seems like a semi-sane behavior. (e.g., the bitcoin-wiki says “Keep in mind that some clients may provide headers of blocks which are invalid if the block locator object contains a hash on the invalid branch. " of getheaders)

Of course, this isn’t quite what Bitcoin Core assumes currently, but introducing more dependency on it seems like questionable design.

A safer alternative design would be to do a getheaders on the highest known valid previous header on that chain. Then, see if the invalid block is relayed. That way we’re only ever querying for valid data.

For example, if we have a block A and A is valid, and then B comes out with parent A, but B is invalid, doing GetHeaders on A and disconnecting if B is returned is safer.

JeremyRubin commented at 9:08 pm on October 3, 2017: contributor

Interleaving error (was taking my sweet time writing it up), but what @gmaxwell said should cover my suggestion as well.

achow101 commented at 9:52 pm on October 3, 2017: member

It seems unnecessary/wasteful to do this for each block that comes in.

I doesn’t. It does this for each invalid block.

What does this offer over the request done after the handshake?

I suppose it doesn’t do that much after the handshake. The part that makes a difference with peers that have been connected to a while is the banning on duplicate invalid headers.

we already will synchronize all headers on the peers best chain (obviously not transferring any we already know about).

Even with synchronizing all headers, I’m not sure that we necessarily know what a peer’s best chain is. I thought that with compact blocks blocks can be relayed before a node has accepted it and made it part of its best chain. So we may not necessarily know whether they actually accepted an invalid block even though it was relayed to us.

Also, I don’t think we necessarily know the headers chain of a newly connected peer.

A safer alternative design would be to do a getheaders on the highest known valid previous header on that chain. Then, see if the invalid block is relayed. That way we’re only ever querying for valid data.

Right, that seems like a better way of checking. I think then it is harder to tell whether you are actually syncing the blockchain and whether you are testing the peer for correctness.

Edit: I gave this a bit more thought. We may want to directly request an invalid block because the invalid block may not be part of their best chain yet they still could have accepted it as valid.

gmaxwell commented at 10:43 pm on October 3, 2017: contributor

I think returning an invalid block when requested is a weak indicator of peer correctness overall. Nodes to assuming “you get what you ask for” and feeding you invalid data – if you ask for it and they have it – seems like a semi-sane behavior.

I don’t agree that what you describe is a sane behavior and it is not a behavior of the Bitcoin protocol in the past. It’s prudent and important to that we’re able to punt peers that send us invalid data. Usually when you ask for something you do not know it will be invalid. If a peer will send you invalid data because you asked for it, then they’ll send you invalid data when you were not expecting it either. :)

In general, being willing to forward something we know is invalid is begging for a transitive fault, cases where you send something invalid to a node, it forwards it on to a third party, and the third party really can’t handle it and punts them.

achow101 renamed this:
~~Bad block interrogation~~
[WIP] Bad block interrogation
on Oct 3, 2017

gmaxwell commented at 0:54 am on October 4, 2017: contributor

I think then it is harder to tell whether you are actually syncing the blockchain and whether you are testing the peer for correctness.

Yes, well and I think it can actually be done with basically no extra network traffic, just responding differently to messages we already get (punt a peer if they give you a header whos parent is something we’ve marked invalid).

achow101 commented at 0:58 am on October 4, 2017: member

punt a peer if they give you a header whos parent is something we’ve marked invalid

We already do that. I’m more concerned about how you get a peer to tell you what blocks are in its best chain without having to wait for a new block to be found.

achow101 commented at 3:22 pm on October 4, 2017: member

After giving this some more thought and sleeping on it, I have a slightly different propsal:

Ban all nodes who give us an invalid block even after we have already seen the block (Note: I’m not sure how this interacts with compact blocks).
After the version handshake, send each node a getheaders request with the parent block of each invalid block that we have (i.e. a getheaders for each invalid block) and no stop block. If we get an invalid header, we will ban them per part 1. We won’t send getheaders for blocks that are older than some time period (maybe 2000 blocks so we only need one getheaders since I think it is also unlikely that we wouldn’t disconnect and ban a node that diverged from us several thousand blocks ago). The actual block requested in the getheaders can be fuzzed a bit too. @gmaxwell @theuni @JeremyRubin What do you think?

gmaxwell commented at 5:08 pm on October 5, 2017: contributor

For peers that are sending us HB mode compact blocks we can only kick things off for a subset of invalidity reasons.

achow101 force-pushed on Oct 7, 2017

achow101 commented at 5:46 pm on October 7, 2017: member

I have updated this to be as we discussed in the IRC meeting. If we see a block header for a block we already know is invalid, we disconnect from the node that sent it. The only exception are our HB CB nodes.

laanwj added this to the milestone 0.15.1 on Oct 12, 2017

laanwj removed this from the milestone 0.15.1 on Oct 12, 2017

laanwj added this to the milestone 0.15.0.2 on Oct 12, 2017

achow101 renamed this:
~~[WIP] Bad block interrogation~~
Disconnect Peers for Duplicate Invalid blocks.
on Oct 12, 2017

laanwj added this to the "Review priority 0.15.0.2" column in a project

sipa commented at 0:33 am on October 13, 2017: member

Going to test this.

in src/net_processing.cpp:1973 in eb1aa2c27a outdated

1969@@ -1970,6 +1970,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
1970                 if (nDoS > 0) {
1971                     LOCK(cs_main);
1972                     Misbehaving(pfrom->GetId(), nDoS);
1973+                } else if (std::find(lNodesAnnouncingHeaderAndIDs.begin(), lNodesAnnouncingHeaderAndIDs.end(), pfrom->GetId()) == lNodesAnnouncingHeaderAndIDs.end()) {

theuni commented at 8:50 pm on October 13, 2017:

Do we not want to disconnect if ban points were assigned?

achow101 commented at 9:12 pm on October 13, 2017:

Most of the time when ban points are assigned, 100 points are assigned and the peer is banned and disconnected. However there are a few cases (e.g. https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L3060) where ban points are assigned but not 100 of them, and we don’t necessarily want to disconnect on those.

theuni commented at 9:39 pm on October 13, 2017:

Mmm, that’s not at all obvious. For now at least, ban points are rather arbitrary. Not completely, of course, but I don’t think their values can be relied on to convey any particular meaning. ~~Besides that, they could’ve already gotten some ban points for some other message.~~

Anyway, Misbehaving() already does what you’ve described. So this actually seems backwards. Now, they’re disconnected immediately if no points were assigned, but giving 10 points keeps them connected…

achow101 commented at 9:44 pm on October 13, 2017:

Do you think we should just disconnect always when something is invalid?

TheBlueMatt commented at 9:28 pm on October 17, 2017:

This is racy. There is no way to check if a peer might have been in compact-fast-relay mode when you receive a compact block header.

achow101 commented at 7:38 pm on October 18, 2017:

Would it be better to just always disconnect on an invalid block, regardless?

TheBlueMatt commented at 10:03 pm on October 18, 2017:

I think we should just ignore this case. My first reaction was to suggest you do a getheaders request, but then I went down a rabbit hole trying to decide whether the peer is required to respond with the header if they decided the block was valid (it turns out BIP 152 explicitly answers this question with a no). Per BIP 152, I believe the way to “correctly” handle this would be to reply with a getdata for the same compact block that you just received, and then handle that differently (!) but that seems absolutely crazy, and not worth it given that compact blocks are only allowed to build on the best chain so we should disconnect them immediately when they get the next block (see https://github.com/bitcoin/bips/pull/601)

achow101 commented at 7:07 pm on October 19, 2017:

@TheBlueMatt

I think we should just ignore this case.

Do you mean just disconnect regardless or just don’t do anything here and let invalid blocks through here?

achow101 commented at 8:31 pm on October 19, 2017:

Ok, I just removed this check and disconnect. After reviewing BIP 152 again, the low bandwidth peers should already be disconnected by the disconnection in handling the headers message so our high bandwidth peers are still unaffected.

in src/net_processing.cpp:2294 in eb1aa2c27a outdated

2289@@ -2288,6 +2290,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2290                 if (nDoS > 0) {
2291                     LOCK(cs_main);
2292                     Misbehaving(pfrom->GetId(), nDoS);
2293+                } else {
2294+                    pfrom->fDisconnect = true;

theuni commented at 8:52 pm on October 13, 2017:

Same here.

achow101 commented at 8:32 pm on October 19, 2017:

Removed the else statement so it will always disconnect on an invalid block header.

laanwj added the label Needs backport on Oct 19, 2017

achow101 force-pushed on Oct 19, 2017

Disconnect on duplicate invalid blocks

If we receive a block header for a block that we already know is
invalid, disconnect the peer that sent it to us unless the peer
sent it with a compact block

23b7af0ac4

achow101 force-pushed on Oct 19, 2017

meshcollider commented at 10:34 pm on October 19, 2017: contributor

utACK https://github.com/bitcoin/bitcoin/pull/11446/commits/23b7af0ac484ce4a7f46060c06e3dca5a8c26d0d

TheBlueMatt commented at 0:11 am on October 20, 2017: member

I’m not sure how I feel about this - the current disconnection/ban logic on invalid headers is proabbly too conservative, but it helps in the case of soft forks where there are occasional invalid blocks relayed preventing us from banning all of our un-upgraded peers. Maybe we should ratchet up the agressiveness here, but only for outbound peers? I think just that change would be sufficient for 0.15.0.2, though I’d really like to see some refactoring of the CValidationState stuff happen so ProcessNewBlockHeaders could inform us of more granular error messages for net_processing to use here.

sdaftuar commented at 1:41 am on October 20, 2017: member

I think there’s a small race condition in the CMPCTBLOCK handler – there’s a circumstance where we would revert to headers processing, and we release cs_main before doing so. If the block header is learned to be invalid in between initial processing and re-processing, then I think this patch would disconnect an HB compact block peer inappropriately.

I’m not really sure what the best remedy is; if we moved headers processing to its own function in net_processing, then I’d say we could just pass in whether this is coming from a compact block or not. Not sure that suggestion makes sense as long as it’s living in ProcessMessages(), though.

achow101 commented at 5:57 pm on October 23, 2017: member

I think there’s a small race condition in the CMPCTBLOCK handler – there’s a circumstance where we would revert to headers processing, and we release cs_main before doing so. If the block header is learned to be invalid in between initial processing and re-processing, then I think this patch would disconnect an HB compact block peer inappropriately.

Why would it revert to headers processing?

sdaftuar commented at 7:03 pm on October 23, 2017: member

@achow101 See https://github.com/bitcoin/bitcoin/blob/57ee73990f1ce29916adfd99f93eae1ccea1a43b/src/net_processing.cpp#L2130

laanwj removed this from the milestone 0.15.0.2 on Oct 26, 2017

laanwj removed the label Needs backport on Oct 26, 2017

laanwj removed this from the "Review priority 0.15.0.2" column in a project

achow101 commented at 3:41 pm on October 27, 2017: member

superseded by #11568

achow101 closed this on Oct 27, 2017

sipa referenced this in commit ba216b5fa6 on Oct 28, 2017

codablock referenced this in commit 433cdccdde on Sep 26, 2019

codablock referenced this in commit 2e980018cf on Sep 29, 2019

barrystyle referenced this in commit ce73e18655 on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021

Disconnect Peers for Duplicate Invalid blocks. #11446