Disconnect outbound peers on invalid chains #11568

sdaftuar · 2017-10-26T19:07:48Z

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.

ProcessMessages will now return earlier when processing headers messages, rather than continuing on (and do nothing).

sdaftuar · 2017-10-26T20:40:31Z

Needed rebase

TheBlueMatt · 2017-10-26T21:57:32Z

utACK 8ca0ffa

theuni · 2017-10-27T06:55:15Z

src/net_processing.cpp

            if (nDoS > 0) {
                LOCK(cs_main);
                Misbehaving(pfrom->GetId(), nDoS);
+            } else if (!pfrom->fInbound && !pfrom->m_manual_connection && punish_invalid) {


Carrying forward my cranky-old-man-rant from: #11446 (comment)

This is crazy-confusing to read. If the header was bad enough to earn them some ban points, put the points on the score-board and carry on. But if it wasn't bad enough for points, kick them immediately.

If we're punishing for invalid, I think the disconnect needs to always happen if (state.IsInvalid). Using points as a proxy for some certain invalid condition(s) is really non-obvious.

I agree with @theuni. Think this should be if ... not else if ... so that we always disconnect on invalid blocks.

Obviously this is very, very much a stop-gap for 0.15.0.2. Next up is refactoring the CValidationState logic to have a concept of what went wrong (bad-by-soft-fork, bad-by-prev-block, bad-by-....) and then just move all of the DoS logic into net_processing. I believe a switch to if instead of else if may break the "prev block not found" handling, so I'm happy to commit to pushing for a DoS overhaul by 0.16 in the coming months.

After further analysis, I think the logic I've presented here is undesirable, because there is at least one situation where we should not disconnect an outbound peer for an invalid block header, which is when the block timestamp is too far in the future. This is a problem whether or not we break this else if out into its own if clause.

I'm going to add a commit that reworks this to explicitly look for the case where the first invalid block header processed is one for which we have the entry in our mapBlockIndex, and only disconnect in that case. I think the review burden will be somewhat (much?) lower for such an approach, and it still accomplishes the overall goal of disconnecting peers on (some, but not all) known-invalid headers chains.

theuni · 2017-10-27T06:57:37Z

src/net_processing.cpp

                LOCK(cs_main);
                Misbehaving(pfrom->GetId(), nDoS);
+            } else if (!pfrom->fInbound && !pfrom->m_manual_connection && punish_invalid) {
+                // Don't keep outbound peers that are on an invalid chain


It's weird to have a punish_invalid param that only punishes based on its own criteria. That means that both sites have to consider all constraints.

Instead, from the ::HEADERS handler, how about something like:

bool punish_invalid = !pfrom->fInbound && !pfrom->m_manual_connection; ProcessHeadersMessage(..., should_punish);

Sounds good.

theuni · 2017-10-27T07:05:19Z

Concept ACK. No hostility intended, I'm just still having a hard time understanding the intended ban/disconnect behavior.

ryanofsky

utACK 8ca0ffa. Confirmed moveonly commit and that the change does seem to "Only disconnect outbound (non-manual) peers that serve us invalid blocks, and exempt compact block announcements from such disconnects." I do think it would be nice if the reasoning was explained more or the logic simplified as other reviewers suggested. It would also be nice to have a unit test to make sure this doesn't break in the future, especially if there's going to be a refactoring like Matt mentioned.

ryanofsky · 2017-10-27T15:46:15Z

src/net_processing.cpp


        if (fRevertToHeaderProcessing)
-            return ProcessMessage(pfrom, NetMsgType::HEADERS, vHeadersMsg, nTimeReceived, chainparams, connman, interruptMsgProc);
+            return ProcessHeadersMessage(pfrom, connman, {cmpctblock.header}, chainparams, /*punish_invalid=*/false);


Can you add a comment saying why this avoids disconnecting on compact block messages with invalid headers? It's unclear whether it's important not to disconnect or whether it's an arbitrary decision and you're just being conservative. You could also consider renaming punish_invalid to disconnect_invalid or similar to be more specific.

I think the hacky vHeadersMsg serialization stuff can be removed now that ProcessHeadersMessage is factored out.

Thanks, I had been looking forward to nuking that, and then forgot! Done.

ryanofsky · 2017-10-27T15:50:41Z

src/net_processing.cpp

                LOCK(cs_main);
                Misbehaving(pfrom->GetId(), nDoS);
+            } else if (!pfrom->fInbound && !pfrom->m_manual_connection && punish_invalid) {
+                // Don't keep outbound peers that are on an invalid chain


This suggestion doesn't apply if you make the changes Cory and Andrew are asking, but otherwise it would be nice if this comment said something about why it avoids disconnecting when nDoS > 0.

Reworked this and added an explanatory comment.

sdaftuar · 2017-10-27T18:14:02Z

I reworked the disconnect logic to (hopefully) be much clearer, and narrowly tailored to the situation that we're interested in.

achow101 · 2017-10-27T18:25:20Z

utACK 4cde638

jnewbery · 2017-10-27T20:10:02Z

Somewhat tested ACK 4cde638.

I have a test case that proves a peer gets kicked for sending an invalid header here: https://github.com/jnewbery/bitcoin/tree/pr11568.2 so I can verify that this PR works in the positive case (ie it disconnects when it's supposed to). I don't think that there are corner cases where this PR might cause other changes in existing behaviour, but I haven't been able to convince myself of that.

The test case in https://github.com/jnewbery/bitcoin/tree/pr11568.2 should be reviewed and merged separately and shouldn't hold up merging or backporting this PR. It builds on #10160 which isn't yet merged, and the test itself is potentially racey so could be improved.

theuni · 2017-10-27T20:18:11Z

utACK other than #11568 (comment)

TheBlueMatt · 2017-10-27T20:24:11Z

ewww-ewww-ewww-ewww-ewww-I've-already-started-working-on-rewriting-DoS-interface utACK bf5494b

theuni · 2017-10-27T20:27:37Z

@sdaftuar Thanks!

@TheBlueMatt Please consider working on top of #11457 (will rebase) which allows for much more straightforward banning.

sdaftuar · 2017-10-27T20:31:42Z

Squashed https://github.com/sdaftuar/bitcoin/commits/11568.0 -> 37886d5

theuni · 2017-10-27T20:41:21Z

Verified same as unsquashed. utACK 37886d5.

sipa

utACK 37886d5

Verified move-only first commit.

laanwj · 2017-10-28T16:41:20Z

utACK 37886d5

laanwj · 2017-10-28T16:43:57Z

src/net_processing.cpp

+            // the peer if the header turns out to be for an invalid block.
+            // Note that if a peer tries to build on an invalid chain, that
+            // will be detected and the peer will be banned.
+            return ProcessHeadersMessage(pfrom, connman, {cmpctblock.header}, chainparams, /*punish_duplicate_invalid=*/false);


Thanks for adding a comment before the argument, that's a good way to avoid boolean arguments turning into an unreadable blob of arguments.

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

meshcollider · 2017-10-28T20:15:53Z

Posthumous utACK 37886d5

Reading the variable mapBlockIndex requires holding the mutex cs_main. The new "Disconnect outbound peers relaying invalid headers" code added in commit 37886d5 and merged as part of bitcoin#11568 two days ago did not lock cs_main prior to accessing mapBlockIndex.

2530bf2 net: Add missing lock in ProcessHeadersMessage(...) (practicalswift) Pull request description: Add missing lock in `ProcessHeadersMessage(...)`. Reading the variable `mapBlockIndex` requires holding the mutex `cs_main`. The new "Disconnect outbound peers relaying invalid headers" code added in commit 37886d5 and merged as part of #11568 two days ago did not lock `cs_main` prior to accessing `mapBlockIndex`. Tree-SHA512: b799c234be8043d036183a00bc7867bbf3bd7ffe3baa94c88529da3b3cd0571c31ed11dadfaf29c5b8498341d6d0a3c928029a43b69f3267ef263682c91563a3

ProcessMessages will now return earlier when processing headers messages, rather than continuing on (and do nothing). Github-Pull: bitcoin#11568 Rebased-From: 4637f18

Github-Pull: bitcoin#11568 Rebased-From: 37886d5

Reading the variable mapBlockIndex requires holding the mutex cs_main. The new "Disconnect outbound peers relaying invalid headers" code added in commit 37886d5 and merged as part of bitcoin#11568 two days ago did not lock cs_main prior to accessing mapBlockIndex. Github-Pull: bitcoin#11578 Rebased-From: 2530bf2

Reading the variable mapBlockIndex requires holding the mutex cs_main. The new "Disconnect outbound peers relaying invalid headers" code added in commit 37886d5 and merged as part of bitcoin#11568 two days ago did not lock cs_main prior to accessing mapBlockIndex.

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

2530bf2 net: Add missing lock in ProcessHeadersMessage(...) (practicalswift) Pull request description: Add missing lock in `ProcessHeadersMessage(...)`. Reading the variable `mapBlockIndex` requires holding the mutex `cs_main`. The new "Disconnect outbound peers relaying invalid headers" code added in commit 37886d5 and merged as part of bitcoin#11568 two days ago did not lock `cs_main` prior to accessing `mapBlockIndex`. Tree-SHA512: b799c234be8043d036183a00bc7867bbf3bd7ffe3baa94c88529da3b3cd0571c31ed11dadfaf29c5b8498341d6d0a3c928029a43b69f3267ef263682c91563a3

laanwj added this to the 0.15.0.2 milestone Oct 26, 2017

laanwj added P2P Needs backport labels Oct 26, 2017

moveonly: factor out headers processing into separate function

4637f18

ProcessMessages will now return earlier when processing headers messages, rather than continuing on (and do nothing).

sdaftuar force-pushed the 2017-10-disconnect-outbound-peers-on-invalid-chains branch from 94742b1 to 8ca0ffa Compare October 26, 2017 20:40

theuni reviewed Oct 27, 2017

View reviewed changes

achow101 mentioned this pull request Oct 27, 2017

Disconnect Peers for Duplicate Invalid blocks. #11446

Closed

ryanofsky reviewed Oct 27, 2017

View reviewed changes

Disconnect outbound peers relaying invalid headers

37886d5

sdaftuar force-pushed the 2017-10-disconnect-outbound-peers-on-invalid-chains branch from bf5494b to 37886d5 Compare October 27, 2017 20:30

sipa reviewed Oct 28, 2017

View reviewed changes

laanwj reviewed Oct 28, 2017

View reviewed changes

sipa merged commit 37886d5 into bitcoin:master Oct 28, 2017

practicalswift mentioned this pull request Oct 30, 2017

net: Add missing lock in ProcessHeadersMessage(...) #11578

Merged

maflcko removed the Needs backport label Nov 1, 2017

morcos mentioned this pull request Nov 2, 2017

0.15: Backports #11592

Merged

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 2, 2017

Disconnect outbound peers relaying invalid headers

59b210d

Github-Pull: bitcoin#11568 Rebased-From: 37886d5

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

Disconnect outbound peers on invalid chains #11568

Disconnect outbound peers on invalid chains #11568

Uh oh!

Conversation

sdaftuar commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdaftuar commented Oct 26, 2017

Uh oh!

TheBlueMatt commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Oct 27, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Oct 27, 2017

Uh oh!

achow101 commented Oct 27, 2017

Uh oh!

jnewbery commented Oct 27, 2017

Uh oh!

theuni commented Oct 27, 2017

Uh oh!

TheBlueMatt commented Oct 27, 2017

Uh oh!

theuni commented Oct 27, 2017

Uh oh!

sdaftuar commented Oct 27, 2017

Uh oh!

theuni commented Oct 27, 2017

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Oct 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

sdaftuar commented Oct 26, 2017 •

edited

Loading

sdaftuar Oct 27, 2017 •

edited

Loading