Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disconnect outbound peers on invalid chains #11568

Conversation

Projects
None yet
10 participants
@sdaftuar
Copy link
Member

commented Oct 26, 2017

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.

@laanwj laanwj added this to the 0.15.0.2 milestone Oct 26, 2017

@laanwj laanwj added this to Review priority 0.15.0.2 in High-priority for review Oct 26, 2017

moveonly: factor out headers processing into separate function
ProcessMessages will now return earlier when processing headers
messages, rather than continuing on (and do nothing).

@sdaftuar sdaftuar force-pushed the sdaftuar:2017-10-disconnect-outbound-peers-on-invalid-chains branch to 8ca0ffa Oct 26, 2017

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2017

Needed rebase

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2017

utACK 8ca0ffa

@@ -1264,6 +1264,9 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve
if (nDoS > 0) {
LOCK(cs_main);
Misbehaving(pfrom->GetId(), nDoS);
} else if (!pfrom->fInbound && !pfrom->m_manual_connection && punish_invalid) {

This comment has been minimized.

Copy link
@theuni

theuni Oct 27, 2017

Member

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.

This comment has been minimized.

Copy link
@achow101

achow101 Oct 27, 2017

Member

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

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 27, 2017

Contributor

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.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Oct 27, 2017

Author Member

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.

@@ -1264,6 +1264,9 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve
if (nDoS > 0) {
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 comment has been minimized.

Copy link
@theuni

theuni Oct 27, 2017

Member

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);

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Oct 27, 2017

Author Member

Sounds good.

@theuni

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

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

@ryanofsky
Copy link
Contributor

left a comment

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.

@@ -2349,7 +2352,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
return ProcessMessage(pfrom, NetMsgType::BLOCKTXN, blockTxnMsg, nTimeReceived, chainparams, connman, interruptMsgProc);

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 27, 2017

Contributor

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.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Oct 27, 2017

Author Member

Done

This comment has been minimized.

Copy link
@theuni

theuni Oct 27, 2017

Member

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

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Oct 27, 2017

Author Member

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

@@ -1264,6 +1264,9 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve
if (nDoS > 0) {
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 comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 27, 2017

Contributor

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.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Oct 27, 2017

Author Member

Reworked this and added an explanatory comment.

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2017

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

@achow101

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

utACK 4cde638

@jnewbery

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

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

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

utACK other than #11568 (comment)

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2017

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

@theuni

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

@sdaftuar Thanks!

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

@sdaftuar sdaftuar force-pushed the sdaftuar:2017-10-disconnect-outbound-peers-on-invalid-chains branch from bf5494b to 37886d5 Oct 27, 2017

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2017

@theuni

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

Verified same as unsquashed. utACK 37886d5.

@sipa
Copy link
Member

left a comment

utACK 37886d5

Verified move-only first commit.

@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 28, 2017

utACK 37886d5

// 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);

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 28, 2017

Member

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

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Oct 28, 2017

Merge #11568: Disconnect outbound peers on invalid chains
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

This comment has been minimized.

Copy link
Member

commented Oct 28, 2017

Posthumous utACK 37886d5

practicalswift added a commit to practicalswift/bitcoin that referenced this pull request Oct 30, 2017

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

laanwj added a commit that referenced this pull request Oct 31, 2017

Merge #11578: net: Add missing lock in ProcessHeadersMessage(...)
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

@fanquake fanquake removed this from Review priority 0.15.0.2 in High-priority for review Nov 1, 2017

@morcos morcos referenced this pull request Nov 2, 2017

Merged

0.15: Backports #11592

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Nov 2, 2017

moveonly: factor out headers processing into separate function
ProcessMessages will now return earlier when processing headers
messages, rather than continuing on (and do nothing).

Github-Pull: bitcoin#11568
Rebased-From: 4637f18

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Nov 2, 2017

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Nov 2, 2017

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

Github-Pull: bitcoin#11578
Rebased-From: 2530bf2

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Mar 12, 2018

net: 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.