Disconnect Peers for Duplicate Invalid blocks. #11446

achow101 · 2017-10-03T18:25:55Z

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

theuni · 2017-10-03T19:04:08Z

src/net_processing.cpp

+        // Ask this peer for the block headers of all of the blocks we know are invalid
+        // If we receive headers messages for these invalid blocks, we will ban them.
+        std::vector<uint256> invalid_block_hashes;
+        for (const std::pair<const uint256, CBlockIndex*>& item : mapBlockIndex)


Should probably wait until VERACK for this.

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

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

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.

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

theuni · 2017-10-03T19:43:24Z

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?

theuni · 2017-10-03T19:56:06Z

src/net_processing.cpp

+                // Send a getheaders message with the hash of the invalid block to all peers.
+                // Those who respond to this message will be banned.
+                connman->ForEachNode([cmpctblock, connman](CNode* pnode) {
+                    connman->PushMessage(pnode, CNetMsgMaker(pnode->GetSendVersion()).Make(NetMsgType::GETHEADERS, CBlockLocator({cmpctblock.header.GetHash()}), uint256()));


Feedback loop here.

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.

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

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

theuni · 2017-10-03T19:56:48Z

src/net_processing.cpp

+                // Send a getheaders message with the hash of the invalid block to all peers.
+                // Those who respond to this message will be banned.
+                connman->ForEachNode([header_hashes, connman](CNode* pnode) {
+                    connman->PushMessage(pnode, CNetMsgMaker(pnode->GetSendVersion()).Make(NetMsgType::GETHEADERS, CBlockLocator(header_hashes), uint256()));


feedback loop here too :)

achow101 · 2017-10-03T20:01:51Z

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 · 2017-10-03T20:46:14Z

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 · 2017-10-03T20:46:15Z

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 · 2017-10-03T21:06:18Z

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 · 2017-10-03T21:08:22Z

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

achow101 · 2017-10-03T21:52:09Z

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 · 2017-10-03T22:43:32Z

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.

gmaxwell · 2017-10-04T00:54:20Z

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 · 2017-10-04T00:58:43Z

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 · 2017-10-04T15:22:25Z

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 · 2017-10-05T17:08:28Z

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

achow101 · 2017-10-07T17:46:45Z

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.

sipa · 2017-10-13T00:33:23Z

Going to test this.

theuni · 2017-10-13T20:50:08Z

src/net_processing.cpp

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


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

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.

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

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

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.

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

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 bitcoin/bips#601)

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

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.

theuni · 2017-10-13T20:52:45Z

src/net_processing.cpp

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


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

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

meshcollider · 2017-10-19T22:34:25Z

utACK 23b7af0

TheBlueMatt · 2017-10-20T00:11:45Z

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 · 2017-10-20T01:41:58Z

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 · 2017-10-23T17:57:31Z

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 · 2017-10-23T19:03:36Z

@achow101 See

bitcoin/src/net_processing.cpp

Line 2130 in 57ee739

fRevertToHeaderProcessing = true;

achow101 · 2017-10-27T15:41:18Z

superseded by #11568

37886d5 Disconnect outbound peers relaying invalid headers (Suhas Daftuar) 4637f18 moveonly: factor out headers processing into separate function (Suhas Daftuar) Pull request description: Alternate to #11446. Disconnect outbound (non-manual) peers that serve us block headers that are already known to be invalid, but exempt compact block announcements from such disconnects. We restrict disconnection to outbound peers that are using up an outbound connection slot, because we rely on those peers to give us connectivity to the honest network (our inbound peers are not chosen by us and hence could all be from an attacker/sybil). Maintaining connectivity to peers that serve us invalid headers is sometimes desirable, eg after a soft-fork, to protect unupgraded software from being partitioned off the honest network, so we prefer to only disconnect when necessary. Compact block announcements are exempted from this logic to comply with BIP 152, which explicitly permits nodes to relay compact blocks before fully validating them. Tree-SHA512: 3ea88e4ccc1184f292a85b17f800d401d2c3806fefc7ad5429d05d6872c53acfa5751e3df83ce6b9c0060ab289511ed70ae1323d140ccc5b12e3c8da6de49936

37886d5 Disconnect outbound peers relaying invalid headers (Suhas Daftuar) 4637f18 moveonly: factor out headers processing into separate function (Suhas Daftuar) Pull request description: Alternate to bitcoin#11446. Disconnect outbound (non-manual) peers that serve us block headers that are already known to be invalid, but exempt compact block announcements from such disconnects. We restrict disconnection to outbound peers that are using up an outbound connection slot, because we rely on those peers to give us connectivity to the honest network (our inbound peers are not chosen by us and hence could all be from an attacker/sybil). Maintaining connectivity to peers that serve us invalid headers is sometimes desirable, eg after a soft-fork, to protect unupgraded software from being partitioned off the honest network, so we prefer to only disconnect when necessary. Compact block announcements are exempted from this logic to comply with BIP 152, which explicitly permits nodes to relay compact blocks before fully validating them. Tree-SHA512: 3ea88e4ccc1184f292a85b17f800d401d2c3806fefc7ad5429d05d6872c53acfa5751e3df83ce6b9c0060ab289511ed70ae1323d140ccc5b12e3c8da6de49936

maflcko added the P2P label Oct 3, 2017

theuni reviewed Oct 3, 2017

View reviewed changes

achow101 force-pushed the bad-block-interrogation branch 2 times, most recently from 3ddf32e to eed5dad Compare October 3, 2017 19:37

achow101 force-pushed the bad-block-interrogation branch from eed5dad to ad944f3 Compare October 3, 2017 19:45

theuni reviewed Oct 3, 2017

View reviewed changes

achow101 changed the title ~~Bad block interrogation~~ [WIP] Bad block interrogation Oct 3, 2017

Sjors mentioned this pull request Oct 4, 2017

Adding command-line arg to hide segwit2x service bit btc1/bitcoin#109

Closed

achow101 mentioned this pull request Oct 7, 2017

Relax punishment for peers relaying invalid blocks and headers #10593

Closed

achow101 force-pushed the bad-block-interrogation branch from ad944f3 to eb1aa2c Compare October 7, 2017 17:45

laanwj modified the milestones: 0.15.1, 0.15.0.2 Oct 12, 2017

achow101 changed the title ~~[WIP] Bad block interrogation~~ Disconnect Peers for Duplicate Invalid blocks. Oct 12, 2017

theuni reviewed Oct 13, 2017

View reviewed changes

laanwj added the Needs backport label Oct 19, 2017

achow101 force-pushed the bad-block-interrogation branch from eb1aa2c to 3943a00 Compare October 19, 2017 20:32

Disconnect on duplicate invalid blocks

23b7af0

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

achow101 force-pushed the bad-block-interrogation branch from 3943a00 to 23b7af0 Compare October 19, 2017 20:33

sdaftuar mentioned this pull request Oct 26, 2017

Disconnect outbound peers on invalid chains #11568

Merged

laanwj removed this from the 0.15.0.2 milestone Oct 26, 2017

laanwj removed the Needs backport label Oct 26, 2017

achow101 closed this Oct 27, 2017

meshcollider mentioned this pull request Nov 14, 2017

External wallet files #11687

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Disconnect Peers for Duplicate Invalid blocks. #11446

Disconnect Peers for Duplicate Invalid blocks. #11446

Conversation

achow101 commented Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

achow101 Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Oct 3, 2017

theuni commented Oct 3, 2017

gmaxwell commented Oct 3, 2017

JeremyRubin commented Oct 3, 2017

JeremyRubin commented Oct 3, 2017

achow101 commented Oct 3, 2017 • edited Loading

gmaxwell commented Oct 3, 2017

gmaxwell commented Oct 4, 2017

achow101 commented Oct 4, 2017

achow101 commented Oct 4, 2017

gmaxwell commented Oct 5, 2017

achow101 commented Oct 7, 2017

sipa commented Oct 13, 2017

Choose a reason for hiding this comment

achow101 Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

theuni Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meshcollider commented Oct 19, 2017

TheBlueMatt commented Oct 20, 2017

sdaftuar commented Oct 20, 2017

achow101 commented Oct 23, 2017

sdaftuar commented Oct 23, 2017

achow101 commented Oct 27, 2017

achow101 commented Oct 3, 2017 •

edited

Loading

achow101 Oct 3, 2017 •

edited

Loading

achow101 commented Oct 3, 2017 •

edited

Loading

achow101 Oct 13, 2017 •

edited

Loading

theuni Oct 13, 2017 •

edited

Loading