net: Allow connections from misbehavior banned peers #14929

1. 
   gmaxwell commented at 9:14 pm on December 11, 2018: contributor

   This allows incoming connections from peers which are only banned due to an automatic misbehavior ban if doing so won’t fill inbound.

   These peers are preferred for eviction when inbound fills, but may still be kept if they fall into the protected classes. This eviction preference lasts the entire life of the connection even if the ban expires.

   If they misbehave again they’ll still get disconnected.

   The main purpose of banning on misbehavior is to prevent our connections from being wasted on unhelpful peers such as ones running incompatible consensus rules. For inbound peers this can be better accomplished with eviction preferences.

   A secondary purpose was to reduce resource waste from repeated abuse but virtually any attacker can get a nearly unlimited supply of addresses, so disconnection is about the best we can do.

   This can reduce the potential from negative impact due to incorrect misbehaviour bans.

2. 
   gmaxwell commented at 9:16 pm on December 11, 2018: contributor

   @pstratem @TheBlueMatt
3. in src/net.cpp:509 in 1539257d3f outdated

   505@@ -506,6 +506,26 @@ void CConnman::ClearBanned()
   506         clientInterface->BannedListChanged();
   507 }
   508 
   509+int CConnman::IsBannedLevel(CNetAddr ip)
   

   ---

   ---

   

   promag commented at 9:24 pm on December 11, 2018:

   const CNetAddr&
4. in src/net.cpp:518 in 1539257d3f outdated

   513+    // 1 - Automatic misbehavior ban
   514+    // 2 - Any other ban
   515+    int level = 0;
   516+    LOCK(cs_setBanned);
   517+    for (const auto& it : setBanned) {
   518+        CSubNet subNet = it.first;
   

   ---

   ---

   

   promag commented at 9:25 pm on December 11, 2018:

   const CSubNet&
5. in src/net.cpp:519 in 1539257d3f outdated

   514+    // 2 - Any other ban
   515+    int level = 0;
   516+    LOCK(cs_setBanned);
   517+    for (const auto& it : setBanned) {
   518+        CSubNet subNet = it.first;
   519+        CBanEntry banEntry = it.second;
   

   ---

   ---

   

   promag commented at 9:25 pm on December 11, 2018:

   const CBanEntry&

   ---

   

   promag commented at 4:14 pm on December 12, 2018:

   Forgot this one?
6. in src/net.cpp:521 in 1539257d3f outdated

   516+    LOCK(cs_setBanned);
   517+    for (const auto& it : setBanned) {
   518+        CSubNet subNet = it.first;
   519+        CBanEntry banEntry = it.second;
   520+
   521+        if (subNet.Match(ip) && GetTime() < banEntry.nBanUntil) {
   

   ---

   ---

   

   promag commented at 9:29 pm on December 11, 2018:

   Cache GetTime() before loop? nit, maybe swap conditions, the 2nd is cheaper?
7. in src/net.cpp:1060 in 1539257d3f outdated

   1055@@ -1034,6 +1056,12 @@ bool CConnman::AttemptToEvictConnection()
   1056 
   1057     if (vEvictionCandidates.empty()) return false;
   1058 
   1059+    // If any remaining peers are preferred for eviction consider only them.
   1060+    if (std::any_of(vEvictionCandidates.begin(),vEvictionCandidates.end(), [](NodeEvictionCandidate const & n) { return n.prefevict;})) {
   

   ---

   ---

   

   promag commented at 9:30 pm on December 11, 2018:

   Should fix formatting.

   ---

   

   gmaxwell commented at 10:35 pm on December 11, 2018:

   It isn’t clear what you’re asking for here.

   ---

   

   promag commented at 10:43 pm on December 11, 2018:

   Mainly spaces inconsistencies.
8. in src/net.h:675 in 1539257d3f outdated

   671@@ -671,6 +672,7 @@ class CNode
   672     // the network or wire types and the cleaned string used when displayed or logged.
   673     std::string strSubVer GUARDED_BY(cs_SubVer), cleanSubVer GUARDED_BY(cs_SubVer);
   674     CCriticalSection cs_SubVer; // used for both cleanSubVer and strSubVer
   675+    bool m_prefevict; // This peer is preferred for eviction.
   

   ---

   ---

   

   promag commented at 9:32 pm on December 11, 2018:

   nit, initialize here bool m_prefevict{false};.

   ---

   

   gmaxwell commented at 10:37 pm on December 11, 2018:

   There isn’t a single other one initialized there. I think it would not be good to behave inconsistently in this respect.
9. 
   promag commented at 9:34 pm on December 11, 2018: member

   The obvious code review..
10. MarcoFalke renamed this:
    Allow connections from misbehavior banned peers.
    net: Allow connections from misbehavior banned peers
    on Dec 11, 2018
11. MarcoFalke added the label P2P on Dec 11, 2018
12. MarcoFalke added this to the milestone 0.18.0 on Dec 11, 2018
13. 
    DrahtBot commented at 11:22 pm on December 11, 2018: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    No conflicts as of last run.

14. MarcoFalke added the label Needs gitian build on Dec 12, 2018
15. in src/net.cpp:511 in 27614369b8 outdated

    505@@ -506,21 +506,43 @@ void CConnman::ClearBanned()
    506         clientInterface->BannedListChanged();
    507 }
    508 
    509-bool CConnman::IsBanned(CNetAddr ip)
    510+int CConnman::IsBannedLevel(const CNetAddr& ip)
    511+{
    512+    // Retuns the most severe level of banning that applies to this address.
    

    ---

    ---

    

    practicalswift commented at 8:04 am on December 12, 2018:

    Should be “returns” :-)
16. in src/net.cpp:529 in 27614369b8 outdated

    532 {
    533+    auto current_time = GetTime();
    534     LOCK(cs_setBanned);
    535     for (const auto& it : setBanned) {
    536-        CSubNet subNet = it.first;
    537+        const CSubNet &subNet = it.first;
    

    ---

    ---

    

    promag commented at 4:15 pm on December 12, 2018:

    const CSubNet& subNet 🏃
17. DrahtBot removed the label Needs gitian build on Dec 14, 2018
18. MarcoFalke deleted a comment on Dec 14, 2018
19. 
    gmaxwell commented at 9:21 pm on December 31, 2018: contributor

    can someone at least give me a concept ack?
20. 
    sdaftuar commented at 9:26 pm on December 31, 2018: member

    Yep, Concept ACK.
21. in src/net.h:675 in 006a07de4e outdated

    671@@ -671,6 +672,7 @@ class CNode
    672     // the network or wire types and the cleaned string used when displayed or logged.
    673     std::string strSubVer GUARDED_BY(cs_SubVer), cleanSubVer GUARDED_BY(cs_SubVer);
    674     CCriticalSection cs_SubVer; // used for both cleanSubVer and strSubVer
    675+    bool m_prefer_evict; // This peer is preferred for eviction.
    

    ---

    ---

    

    laanwj commented at 11:30 am on January 2, 2019:

    One idea I had would be to make this an ’eviction priority’ integer instead of a boolean, then choose one of the peers with lowest priority when an eviction is needed. But not necessary unless this is going to be used for other things, as well.

    ---

    

    gmaxwell commented at 6:25 am on January 3, 2019:

    I thought of that too! but thought it was simpler to make it a bool until we had something else to trigger it on.
22. 
    laanwj commented at 11:30 am on January 2, 2019: member

    Concept ACK
23. 
    sdaftuar commented at 7:13 pm on January 7, 2019: member

    Perhaps we should go further and not bother disconnecting inbound peers who misbehave, and just increase their likelihood of eviction? One of the changes I’ve been meaning to propose is to not disconnect inbound peers for relaying an invalid block or header, so that in the event of a future softfork, old peers don’t get disconnected for not knowing about the new rules (which risks network partition).

    The only benefit to disconnecting that I can think of is if we’re using disconnection as a rate limit on the bad behavior itself – but that seems like a poor remedy for limiting a determined adversary that we assume has access to an ~infinite IP range.

    On further thought: this seems like it would be a more involved change to make than I realized (with all the associated testing changes), so we can postpone discussion to a separate PR.

24. in src/net.cpp:862 in 006a07de4e outdated

    1057@@ -1034,6 +1058,12 @@ bool CConnman::AttemptToEvictConnection()
    1058 
    1059     if (vEvictionCandidates.empty()) return false;
    1060 
    1061+    // If any remaining peers are preferred for eviction consider only them.
    

    ---

    ---

    

    sdaftuar commented at 8:10 pm on January 7, 2019:

    It’s not entirely clear to me why we might allow preferred eviction peers to be protected under one of the above criteria – can you elaborate on that design choice in this comment?

    ---

    

    gmaxwell commented at 8:25 pm on January 7, 2019:

    My thought was along the lines of “we don’t ever want to disconnect the last inbound peer to give us a block we accepted, regardless of who it is”– beyond that, I didn’t think it mattered much if they got protected under other criteria. I’m fully open to changing it, if you have a suggestion for a better behaviour.

    ---

    

    sdaftuar commented at 9:35 pm on January 7, 2019:

    Seems like fine reasoning, maybe just leave a comment to that effect so that someone could refine this later without wondering why it’s done the current way?
25. in src/net.cpp:1151 in 006a07de4e outdated

    1143@@ -1114,7 +1144,11 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
    1144     // on all platforms.  Set it again here just to be sure.
    1145     SetSocketNoDelay(hSocket);
    1146 
    1147-    if (IsBanned(addr) && !whitelisted)
    1148+    int bannedlevel = IsBannedLevel(addr);
    1149+
    1150+    // Don't accept connections from banned peers, but if our inbound slots aren't almost full, accept
    1151+    // if the only banning reason was an automatic misbehavior ban.
    1152+    if (!whitelisted && bannedlevel > (nInbound + 1 < nMaxInbound))
    

    ---

    ---

    

    sdaftuar commented at 8:17 pm on January 7, 2019:

    nit: bannedlevel > (nInbound + 1 < nMaxInbound) is not the most readable code, but I do appreciate the comment.
26. 
    sdaftuar commented at 8:19 pm on January 7, 2019: member

    Looks reasonable, will test.
27. 
    gmaxwell commented at 8:23 pm on January 7, 2019: contributor

    @sdaftuar I think in the long run we should move in that direction, but it’s a bigger change that requires more careful consideration.

    Consider: disconnecting on an invalid block / header also potentially helps protect the peer when we’re the one that is in the wrong (e.g. rejecting blocks due to corrupt state). Not a reason to not do it, but perhaps it’s a reason to be first really confident that our outbound management of consensus rule incompatibilities is really good. Likewise, we might want to implement more holistic rate limiting or prioritized message processing (e.g. queue messages from peers, handle messages from helpful, outbound, peers first).

28. 
    sdaftuar commented at 4:30 pm on January 9, 2019: member

    ACK

    My only observation from testing is that the interaction between whitelisting and m_prefer_evict is not entirely obvious, but I think this is mostly due to the pre-existing not-well-defined behavior of whitelisting (for instance, I think a whitelisted peer should probably be exempt from eviction, which if I read correctly is not currently the case? But instead after this PR a peer could conceivably be whitelisted and have the prefer_evict flag set – though that would only be in a strange situation where a such a peer received an automatic misbehavior ban prior to being whitelisted).

29. 
    gmaxwell commented at 8:20 pm on January 10, 2019: contributor

    @sdaftuar pre-existing code unconditionally exempts whitelisted peers from consideration for eviction: https://github.com/bitcoin/bitcoin/blob/006a07de4e5c3de36ef84d186315a4a5f23da158/src/net.cpp#L1025

    I think the remaining interaction isn’t that weird, like if a peer gets eviction pref then gets whitelisted it won’t get evicted while whitelisted, but if the whitelisting is removed, the remaining pref will still apply.

30. 
    sipa commented at 8:29 pm on January 11, 2019: member

    utACK 006a07de4e5c3de36ef84d186315a4a5f23da158
31. DrahtBot added the label Needs rebase on Jan 14, 2019
32. in src/net.cpp:2771 in 006a07de4e outdated

    2767@@ -2733,6 +2768,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
    2768     addrName = addrNameIn == "" ? addr.ToStringIPPort() : addrNameIn;
    2769     nVersion = 0;
    2770     strSubVer = "";
    2771+    m_prefer_evict = false;
    

    ---

    ---

    

    MarcoFalke commented at 3:40 pm on January 14, 2019:

    nit: Should use a C++11 default member initializer instead (That will also solve the merge conflict)

    ---

    

    gmaxwell commented at 0:28 am on January 15, 2019:

    @MarcoFalke Please see: github.com/bitcoin/bitcoin/pull/14929/#discussion_r240818366 (sorry github screws up any effort I make to directly link the comment)

    ---

    

    gmaxwell commented at 5:40 am on January 15, 2019:

    oh I see, it was conflicted by a PR that went through and changed initialization, though, strangely, incompletely (e.g. strSubVer)
33. DrahtBot removed the label Needs rebase on Jan 15, 2019
34. 
    naumenkogs commented at 5:46 pm on January 16, 2019: member

    Won’t this be a good opportunity to use the same variable for banned and whitelisted? Say IsBannedLevel == -1 for whitelisted, or better some other naming
35. 
    gmaxwell commented at 6:43 pm on January 16, 2019: contributor

    @naumenkogs I don’t think there is any conceptual conflict with having a peer which is both banned and whitelisted.
36. 
    naumenkogs commented at 7:04 pm on January 16, 2019: member

    @gmaxwell but being whitelisted just disables all bans, isn’t it?

    It always makes me a bit confusing and I have to go look in the code how exactly these 2 work together. Perhaps enforcing a choice would make it more straightforward, not sure.

37. DrahtBot added the label Needs rebase on Jan 21, 2019
38. Allow connections from misbehavior banned peers.

    This allows incoming connections from peers which are only banned
     due to an automatic misbehavior ban if doing so won't fill inbound.
    
    These peers are preferred for eviction when inbound fills, but may
     still be kept if they fall into the protected classes.  This
     eviction preference lasts the entire life of the connection even
     if the ban expires.
    
    If they misbehave again they'll still get disconnected.
    
    The main purpose of banning on misbehavior is to prevent our
     connections from being wasted on unhelpful peers such as ones
     running incompatible consensus rules.  For inbound peers this
     can be better accomplished with eviction preferences.
    
    A secondary purpose was to reduce resource waste from repeated
     abuse but virtually any attacker can get a nearly unlimited
     supply of addresses, so disconnection is about the best we can
     do.

    0297be61ac
39. DrahtBot removed the label Needs rebase on Jan 22, 2019
40. 
    gmaxwell commented at 7:40 pm on January 24, 2019: contributor

    @naumenkogs It disables bans applying, but it doesn’t disable being banned, and shouldn’t – unwhitelisting should reveal the underlying banned (or not) state, because of this I don’t think they should share state.
41. laanwj added this to the "Blockers" column in a project

    ---

42. 
    sdaftuar commented at 7:50 pm on January 24, 2019: member

    utACK 0297be61acdf1cdd5f56c8371d1718d08229d9b3
43. in src/banman.cpp:75 in 0297be61ac

    66@@ -67,14 +67,36 @@ void BanMan::ClearBanned()
    67     if (m_client_interface) m_client_interface->BannedListChanged();
    68 }
    69 
    70+int BanMan::IsBannedLevel(CNetAddr net_addr)
    71+{
    72+    // Returns the most severe level of banning that applies to this address.
    73+    // 0 - Not banned
    74+    // 1 - Automatic misbehavior ban
    75+    // 2 - Any other ban
    

    ---

    ---

    

    Sjors commented at 7:58 pm on January 24, 2019:

    Nit: maybe swap 1 and 2 so we can add other / more detailed specific reasons later?

    ---

    

    gmaxwell commented at 9:36 pm on January 27, 2019:

    meh. Trivial to do if there is a need. Without a need otherwise the current order makes more sense to me.

    ---

    

    Sjors commented at 5:19 pm on January 29, 2019:

    This data is ephemeral, right? Not saved into banlist.dat (which would make it more tedious to change later).
44. 
    Sjors commented at 7:59 pm on January 24, 2019: member

    Concept ACK
45. 
    jonasschnelli commented at 7:59 pm on January 24, 2019: contributor

    utACK 0297be61acdf1cdd5f56c8371d1718d08229d9b3 A functional test-script would be nice.
46. 
    naumenkogs commented at 8:00 pm on January 24, 2019: member

    utACK https://github.com/bitcoin/bitcoin/commit/0297be61acdf1cdd5f56c8371d1718d08229d9b3
47. jonasschnelli merged this on Jan 29, 2019
48. jonasschnelli closed this on Jan 29, 2019

    ---

49. jonasschnelli referenced this in commit 2d790e82c8 on Jan 29, 2019
50. jnewbery removed this from the "Blockers" column in a project

    ---

51. jasonbcox referenced this in commit f4ccfea4ad on Dec 20, 2019
52. jonspock referenced this in commit 74494ed0f3 on Jan 10, 2020
53. proteanx referenced this in commit 48dd7dbde1 on Jan 10, 2020
54. sipa referenced this in commit abdfd2d0e3 on Jul 7, 2020
55. sidhujag referenced this in commit aec880260b on Jul 8, 2020
56. PastaPastaPasta referenced this in commit d859a99d4c on Jul 18, 2021
57. PastaPastaPasta referenced this in commit 6c442523ee on Jul 18, 2021
58. PastaPastaPasta referenced this in commit 03ba73e8e6 on Jul 19, 2021
59. PastaPastaPasta referenced this in commit ee0afc6c32 on Jul 19, 2021
60. PastaPastaPasta referenced this in commit 134c11e9f1 on Jul 20, 2021
61. PastaPastaPasta referenced this in commit d953148375 on Jul 20, 2021
62. DrahtBot locked this on Dec 16, 2021

Contributors
gmaxwell promag DrahtBot practicalswift sdaftuar laanwj sipa MarcoFalke naumenkogs Sjors jonasschnelli

Labels
P2P

Milestone
0.18.0