Disconnect outbound peers on invalid chains #11568

pull sdaftuar wants to merge 2 commits into bitcoin:master from sdaftuar:2017-10-disconnect-outbound-peers-on-invalid-chains changing 3 files +226 −172
  1. sdaftuar commented at 7:07 pm on October 26, 2017: member

    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.

  2. laanwj added this to the milestone 0.15.0.2 on Oct 26, 2017
  3. laanwj added the label P2P on Oct 26, 2017
  4. laanwj added the label Needs backport on Oct 26, 2017
  5. laanwj added this to the "Review priority 0.15.0.2" column in a project
  6. moveonly: factor out headers processing into separate function 
    ProcessMessages will now return earlier when processing headers
messages, rather than continuing on (and do nothing).
    4637f18522
  7. sdaftuar force-pushed on Oct 26, 2017
  8. sdaftuar commented at 8:40 pm on October 26, 2017: member
    Needed rebase
  9. TheBlueMatt commented at 9:57 pm on October 26, 2017: member
    utACK 8ca0ffa127e1a11dca3a17f6d9b3079e92fd3771
  10. in src/net_processing.cpp:1267 in 8ca0ffa127 outdated 
    1263@@ -1264,6 +1264,9 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve
1264             if (nDoS > 0) {
1265                 LOCK(cs_main);
1266                 Misbehaving(pfrom->GetId(), nDoS);
1267+            } else if (!pfrom->fInbound && !pfrom->m_manual_connection && punish_invalid) {
    theuni commented at 6:55 am on October 27, 2017:

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

    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.

    achow101 commented at 3:31 pm on October 27, 2017:
    I agree with @theuni. Think this should be if ... not else if ... so that we always disconnect on invalid blocks.
    TheBlueMatt commented at 3:38 pm on October 27, 2017:
    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.
    sdaftuar commented at 6:00 pm on October 27, 2017:

    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.

  11. in src/net_processing.cpp:1268 in 8ca0ffa127 outdated 
    1263@@ -1264,6 +1264,9 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve
1264             if (nDoS > 0) {
1265                 LOCK(cs_main);
1266                 Misbehaving(pfrom->GetId(), nDoS);
1267+            } else if (!pfrom->fInbound && !pfrom->m_manual_connection && punish_invalid) {
1268+                // Don't keep outbound peers that are on an invalid chain
    theuni commented at 6:57 am on October 27, 2017:

    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:

    0bool punish_invalid = !pfrom->fInbound && !pfrom->m_manual_connection;
1ProcessHeadersMessage(..., should_punish);
    ryanofsky commented at 3:50 pm on October 27, 2017:
    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.
    sdaftuar commented at 4:19 pm on October 27, 2017:
    Sounds good.
    sdaftuar commented at 6:47 pm on October 27, 2017:
    Reworked this and added an explanatory comment.
  12. theuni commented at 7:05 am on October 27, 2017: member
    Concept ACK. No hostility intended, I’m just still having a hard time understanding the intended ban/disconnect behavior.
  13. in src/net_processing.cpp:2355 in 8ca0ffa127 outdated 
    2351@@ -2349,7 +2352,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2352             return ProcessMessage(pfrom, NetMsgType::BLOCKTXN, blockTxnMsg, nTimeReceived, chainparams, connman, interruptMsgProc);
2353 
2354         if (fRevertToHeaderProcessing)
2355-            return ProcessMessage(pfrom, NetMsgType::HEADERS, vHeadersMsg, nTimeReceived, chainparams, connman, interruptMsgProc);
2356+            return ProcessHeadersMessage(pfrom, connman, {cmpctblock.header}, chainparams, /*punish_invalid=*/false);
    ryanofsky commented at 3:46 pm on October 27, 2017:
    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.
    sdaftuar commented at 6:47 pm on October 27, 2017:
    Done
    theuni commented at 7:13 pm on October 27, 2017:
    I think the hacky vHeadersMsg serialization stuff can be removed now that ProcessHeadersMessage is factored out.
    sdaftuar commented at 8:18 pm on October 27, 2017:
    Thanks, I had been looking forward to nuking that, and then forgot! Done.
  14. ryanofsky commented at 3:58 pm on October 27, 2017: member
    utACK 8ca0ffa127e1a11dca3a17f6d9b3079e92fd3771. 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.
  15. sdaftuar commented at 6:14 pm on October 27, 2017: member
    I reworked the disconnect logic to (hopefully) be much clearer, and narrowly tailored to the situation that we’re interested in.
  16. achow101 commented at 6:25 pm on October 27, 2017: member
    utACK 4cde638d4ce054ffdadc3bf65ae5dfa81b01aee2
  17. jnewbery commented at 8:10 pm on October 27, 2017: member

    Somewhat tested ACK 4cde638d4ce054ffdadc3bf65ae5dfa81b01aee2.

    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.

  18. theuni commented at 8:18 pm on October 27, 2017: member
    utACK other than #11568 (review)
  19. TheBlueMatt commented at 8:24 pm on October 27, 2017: member
    ewww-ewww-ewww-ewww-ewww-I’ve-already-started-working-on-rewriting-DoS-interface utACK bf5494badac903af51802e9aeb1ca1ca0f83a59c
  20. theuni commented at 8:27 pm on October 27, 2017: member
    @sdaftuar Thanks! @TheBlueMatt Please consider working on top of #11457 (will rebase) which allows for much more straightforward banning.
  21. Disconnect outbound peers relaying invalid headers 37886d5e2f
  22. sdaftuar force-pushed on Oct 27, 2017
  23. sdaftuar commented at 8:31 pm on October 27, 2017: member
    Squashed https://github.com/sdaftuar/bitcoin/commits/11568.0 -> 37886d5e2f9992678dea4b1bd893f4f10d61d3ad
  24. theuni commented at 8:41 pm on October 27, 2017: member
    Verified same as unsquashed. utACK 37886d5.
  25. sipa commented at 9:45 am on October 28, 2017: member

    utACK 37886d5e2f9992678dea4b1bd893f4f10d61d3ad

    Verified move-only first commit.

  26. laanwj commented at 4:41 pm on October 28, 2017: member
    utACK 37886d5
  27. in src/net_processing.cpp:2387 in 37886d5e2f 
    2384+            // Headers received from HB compact block peers are permitted to be
2385+            // relayed before full validation (see BIP 152), so we don't want to disconnect
2386+            // the peer if the header turns out to be for an invalid block.
2387+            // Note that if a peer tries to build on an invalid chain, that
2388+            // will be detected and the peer will be banned.
2389+            return ProcessHeadersMessage(pfrom, connman, {cmpctblock.header}, chainparams, /*punish_duplicate_invalid=*/false);
    laanwj commented at 4:43 pm on October 28, 2017:
    Thanks for adding a comment before the argument, that’s a good way to avoid boolean arguments turning into an unreadable blob of arguments.
  28. sipa merged this on Oct 28, 2017
  29. sipa closed this on Oct 28, 2017
  30. sipa referenced this in commit ba216b5fa6 on Oct 28, 2017
  31. meshcollider commented at 8:15 pm on October 28, 2017: contributor
    Posthumous utACK https://github.com/bitcoin/bitcoin/commit/37886d5e2f9992678dea4b1bd893f4f10d61d3ad
  32. practicalswift referenced this in commit 042831e741 on Oct 30, 2017
  33. practicalswift referenced this in commit 2530bf27b7 on Oct 30, 2017
  34. practicalswift referenced this in commit 07aff81353 on Oct 30, 2017
  35. laanwj referenced this in commit 8335cb4781 on Oct 31, 2017
  36. fanquake removed this from the "Review priority 0.15.0.2" column in a project
  37. MarcoFalke referenced this in commit 7d774414b8 on Nov 1, 2017
  38. MarcoFalke referenced this in commit 691bad4142 on Nov 1, 2017
  39. MarcoFalke removed the label Needs backport on Nov 1, 2017
  40. MarcoFalke referenced this in commit 3ea950d215 on Nov 1, 2017
  41. MarcoFalke referenced this in commit fc966bbd2b on Nov 2, 2017
  42. MarcoFalke referenced this in commit 59b210d9a7 on Nov 2, 2017
  43. MarcoFalke referenced this in commit ec8dedff46 on Nov 2, 2017
  44. HashUnlimited referenced this in commit a0de28a9ad on Mar 12, 2018
  45. codablock referenced this in commit 433cdccdde on Sep 26, 2019
  46. codablock referenced this in commit 39cf1d3944 on Sep 26, 2019
  47. codablock referenced this in commit 2e980018cf on Sep 29, 2019
  48. codablock referenced this in commit c743111c66 on Sep 29, 2019
  49. barrystyle referenced this in commit ce73e18655 on Jan 22, 2020
  50. barrystyle referenced this in commit 3ac71cebf3 on Jan 22, 2020
  51. MarcoFalke locked this on Sep 8, 2021


sdaftuar TheBlueMatt theuni achow101 ryanofsky jnewbery sipa laanwj meshcollider

Labels
P2P

Milestone
0.15.1

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me