banlist.dat: store banlist on disk #6310

pull jonasschnelli wants to merge 4 commits into bitcoin:master from jonasschnelli:2015/06/store_ban_list changing 9 files +359 −36
  1. jonasschnelli commented at 3:50 PM on June 19, 2015: contributor

    Currently, the list of banned nodes are only held in memory. A restart of bitcoind would reset the set of banned nodes.

    This PR introduces a new filestore (banlist.dat) which stores the IPs/Subnets of the banned nodes more or less identical to the CAddrDB (peers.dat).

    Also added within this PR is a feature that removes unused banned node entries because the banned_till time has expired.

    includes tests for the banlist disk storage.

    Unsure if we should introduce a new cmd arg -storebanlist to optionally allow storing of banned node data. It could be, that some users expecting a sweep of all banned nodes when they restart bitcoind.

  2. jgarzik commented at 3:53 PM on June 19, 2015: contributor

    Nice feature. RE -storebanlist, the system should default to storing. Users may disable.

    Or don't create an option at all -- same as CAddrDB, user may delete file to clear the list.

  3. Diapolo commented at 7:52 PM on June 19, 2015: none

    I'd also vote for treating this the same as peers.dat and don't add an option for it.

  4. in src/net.cpp:None in 9c4b16b843 outdated 
    2228 | +    CAutoFile filein(file, SER_DISK, CLIENT_VERSION);
2229 | +    if (filein.IsNull())
2230 | +        return error("%s: Failed to open file %s", __func__, pathBanlist.string());
2231 | +
2232 | +    // use file size to size memory buffer
2233 | +    int fileSize = boost::filesystem::file_size(pathBanlist);
    Diapolo commented at 7:55 PM on June 19, 2015:

    Not sure if this is best to use an int here... what is file_size returning?

    jonasschnelli commented at 7:58 PM on June 19, 2015:

    Do you expect users with more then 2GB of banned node data? I have taken this approach from CAddrDB 1:1. But i agree file sizes should be held in uint64_t or in size_t if they are not serialized/shared anywhere.

    Diapolo commented at 8:00 PM on June 19, 2015:

    I think it's plenty of space, but we should be 100% sure we don't overflow IMHO? Yeah there are more parts in the code that don't do that.

    laanwj commented at 11:20 AM on June 26, 2015:

    Right: to be precise, file sizes should be uint64_t, not size_t. Remember that size_t depends on the address width, it is for in-memory sizes.

    jonasschnelli commented at 1:22 PM on June 26, 2015:

    Agreed. Changed from int to uint64_t (also in CAddDB).

  5. in src/net.cpp:None in 9c4b16b843 outdated 
    2233 | +    int fileSize = boost::filesystem::file_size(pathBanlist);
2234 | +    int dataSize = fileSize - sizeof(uint256);
2235 | +    // Don't try to resize to a negative number if file is small
2236 | +    if (dataSize < 0)
2237 | +        dataSize = 0;
2238 | +    vector<unsigned char> vchData;
    Diapolo commented at 7:56 PM on June 19, 2015:

    Maybe you could use std:: so we can get rid of using namespace std; easier :)?

  6. jonasschnelli cross-referenced this on Jun 21, 2015 from issue [Qt] allow banning and unbanning over UI->peers table by jonasschnelli
  7. laanwj added the label Feature on Jun 22, 2015
  8. dexX7 commented at 12:08 AM on June 26, 2015: contributor

    @jgarzik: RE -storebanlist, the system should default to storing. Users may disable.

    This seems risky in my opinion, because not all IPs are static, and they don't necessarily always belong to the same node.

  9. sipa commented at 12:47 AM on June 26, 2015: member

    dexX7: that is why all bans are temporary.

    jonasschnelli: I think defaulting to storing them is perfectly fine (or even not offering the option).

  10. jonasschnelli commented at 7:15 AM on June 26, 2015: contributor

    @jgarziks way of resetting the banlist (by removing banlist.dat) is very straightforward and doesn't need any explanation. I'd like to avoid another cmd arg option to not end up in having thousands of tiny options.

    Node misbehaving bans are by default 24h. All other bans needs conscious actions from the users. The chance that your node accidentally get an IP out of a dynamic range, where the previous owner managed to add his bitcoin-core on a ban-list, is very rar. And then, the chance that all nodes did ban this IP or that you connect to a node which conscious added your new IP to the banlist is even rarer We have 3,706,452,992 assignable public IPv4 addresses.

  11. laanwj commented at 11:21 AM on June 26, 2015: member

    Concept ACK, also agree that defaulting to store bans is perfectly fine (so is not offering an option).

    As any kind of persistence commits to a format it is good to think forward a bit what should be included: would it make sense to add e.g. a "ban source" enum field, that specifies whether the ban is automatic or manual?

  12. jonasschnelli force-pushed on Jun 26, 2015
  13. jonasschnelli commented at 1:52 PM on June 26, 2015: contributor

    I like the idea of storing the "ban source". Because we now keep/dump a serialized std::map<CSubNet, int64_t> everything beyond that would be a bigger change. We should probably introduce a simple class for the banlist entry like class CBanEntry { int64_t bantime, enum banSource }. This might also be capable of allowing ban distinction between multiple bans on the same IP but with different ports (but would mean moving from std::map to a vector).

  14. laanwj commented at 3:20 PM on June 26, 2015: member

    We should probably introduce a simple class for the banlist entry like class CBanEntry { int64_t bantime, enum banSource }

    Right. That would also allow for versioning in serialization.

    This might also be capable of allowing ban distinction between multiple bans on the same IP but with different ports

    This might be overdoing it; at least I don't see much added value in per-port bans, at the least they're easily circumvented (for incoming connections: just reconnect and you get a different source port, for outgoing connections simply rebind to a different port).

  15. Diapolo commented at 3:27 PM on June 26, 2015: none

    @laanwj Think about per port bans with incoming Tor/proxy connections, they're all 127.0.0.1 and when I currently ban a single 127.0.0.1 with a specific port all further connection attempts are blocked until bantime is over. Not sure if this is to solve in any way.

  16. laanwj commented at 3:33 PM on June 26, 2015: member

    @Diapolo That simply won't work. As I try to explain above, source ports aren't unique nor identifying.

  17. Diapolo commented at 3:55 PM on June 26, 2015: none

    @laanwj Indeed, my fault... but should be warned when banning 127.0.0.1 or do you assume people know what they are banning?

  18. jonasschnelli force-pushed on Jun 26, 2015
  19. jonasschnelli force-pushed on Jun 26, 2015
  20. jonasschnelli commented at 7:58 PM on June 26, 2015: contributor

    Followed and implemented @laanwj s proposal. Added CBanEntry as ban metadata container which uses enum BanReason (BanReasonNodeMisbehaving, BanReasonManuallyAdded). Also added nCreateTime within the new metadata class, to allow to backtrack when a ban was added/created. Using now everywhere the typedef std::map<CSubNet, CBanEntry> banmap_t.

    Now the banlist.dat file is extendable over the possibility to detect different versions (CBanEntry->nVersion) of entries and allow possible migrations during deserialization.

  21. jonasschnelli force-pushed on Jun 26, 2015
  22. in src/rpcnet.cpp:None in 0386ba4b62 outdated 
     552 |      {
 553 | +        CBanEntry banEntry = (*it).second;
 554 |          UniValue rec(UniValue::VOBJ);
 555 |          rec.push_back(Pair("address", (*it).first.ToString()));
 556 | -        rec.push_back(Pair("banned_untill", (*it).second));
 557 | +        rec.push_back(Pair("banned_untill", banEntry.nBanUntil));
    laanwj commented at 2:49 PM on June 29, 2015:

    s/untill/until/ I suppose?

    jonasschnelli commented at 6:04 PM on June 29, 2015:

    s/untill/until/ I suppose?

    Fixed typo.

  23. laanwj commented at 2:50 PM on June 29, 2015: member

    utACK

  24. in src/net.cpp:None in 0386ba4b62 outdated 
     490 |      return fResult;
 491 |  }
 492 |  
 493 | -void CNode::Ban(const CNetAddr& addr, int64_t bantimeoffset, bool sinceUnixEpoch) {
 494 | +void CNode::Ban(const CNetAddr& addr, const BanReason &banReason, int64_t bantimeoffset, bool sinceUnixEpoch) {
 495 |      CSubNet subNet(addr.ToString()+(addr.IsIPv4() ? "/32" : "/128"));
    laanwj commented at 2:53 PM on June 29, 2015:

    Hhmm at some point we should add a CSubNet constructor or factory function that makes a subnet with one unique CNetAddr (netmask all ones), and use that, instead of going through a string. Not a big deal, though.

    jonasschnelli commented at 6:40 PM on June 29, 2015:

    Hhmm at some point we should add a CSubNet constructor or factory function that makes a subnet with one unique CNetAddr (netmask all ones), and use that, instead of going through a string. Not a big deal, though.

    Right. Added (see latest commit). If we don't add it now, it probably will never be added.

  25. in src/net.cpp:None in 0386ba4b62 outdated 
     503 | +    CBanEntry banEntry(GetTime());
 504 | +    banEntry.banReason = banReason;
 505 | +    banEntry.nBanUntil = GetTime()+GetArg("-bantime", 60*60*24);  // Default 24-hour ban
 506 |      if (bantimeoffset > 0)
 507 | -        banTime = (sinceUnixEpoch ? 0 : GetTime() )+bantimeoffset;
 508 | +        banEntry.nBanUntil = (sinceUnixEpoch ? 0 : GetTime() )+bantimeoffset;
    laanwj commented at 3:04 PM on June 29, 2015:

    maybe

    if (bantimeoffset <= 0)
{
    bantimeoffset = GetArg("-bantime", 60*60*24); // Default 24-hour ban
    sinceUnixEpoch = false;
}
banEntry.nBanUntil = (sinceUnixEpoch ? 0 : GetTime() )+bantimeoffset;

    (slightly cleaner, not calling GetTime() twice)

    jonasschnelli commented at 6:05 PM on June 29, 2015:

    Yes. This makes more sense. Fixed and pushed.

  26. jonasschnelli force-pushed on Jun 29, 2015
  27. ajweiss commented at 6:33 PM on June 29, 2015: contributor

    I have an unsubmitted patch that also adds "remembering" of ban scores associated by IP address across disconnects over a time window (24h I think). That is, it prevents malicious nodes from carrying out sawtooth attacks where they can reset their ban score by disconnecting and reconnecting before being banned. There are pros (prevents malicious behavior) and cons (tightens up the rules and increases the likelihood of NAT exit hosts getting banned due to lots of small accumulations of ban points over time.) Is there any interest in this? I can open a new pull if so...

  28. jonasschnelli force-pushed on Jun 29, 2015
  29. jonasschnelli cross-referenced this on Jul 2, 2015 from issue Bugfix: RPC net: Correct spelling of "until" in listbanned by luke-jr
  30. laanwj commented at 6:17 PM on July 2, 2015: member

    @ajweiss Not sure about that. At least make sure that the associated structure is size-limited. Especially with IPv6 it will be extremely easy to fill memory that way. But this needs to be discussed somewhere else as it is not related to this pull.

  31. banlist.dat: store banlist on disk f581d3d656
  32. CAddrDB/CBanDB: change filesize variables from int to uint64_t dfa174c295
  33. use CBanEntry as object container for banned nodes
    - added a reason enum for a ban
- added creation time for a ban

Using CBanEntry as container will keep banlist.dat extenable.
    409bccfbf5
  34. in src/netbase.cpp:None in 79f375fc05 outdated 
    1299 | +
1300 | +    //set all netmask bits to 1
1301 | +    memset(netmask, 255, sizeof(netmask));
1302 | +
1303 | +    std::vector<CNetAddr> vIP;
1304 | +    if (!LookupHost(addr.ToString().c_str(), vIP, 1, fAllowLookup))
    laanwj commented at 6:22 PM on July 2, 2015:

    Thanks. But this seems a bit circuitous. Let's do just:

    CSubNet::CSubNet(const CNetAddr &addr, bool fAllowLookup):
    valid(addr.IsValid())
{
    memset(netmask, 255, sizeof(netmask));
    network = addr;
}

    Also the fAllowLookup parameter is unnecessary. It should never be needed to look up anything when going from an already binary address.

    Edit: added initialization for valid()

    jonasschnelli commented at 6:38 PM on July 2, 2015:

    Right. Wasn't aware of the LookupHost() in the CNetAddr::CNetAddr() constructor. Fixed and pushed.

    jonasschnelli commented at 6:44 PM on July 2, 2015:

    Right! The isValid must be set explicit. Fixed.

  35. jonasschnelli force-pushed on Jul 2, 2015
  36. jonasschnelli force-pushed on Jul 2, 2015
  37. jonasschnelli force-pushed on Jul 2, 2015
  38. Adding CSubNet constructor over a single CNetAddr 177a0e4914
  39. jonasschnelli force-pushed on Jul 2, 2015
  40. laanwj merged this on Jul 2, 2015
  41. laanwj closed this on Jul 2, 2015
  42. laanwj referenced this in commit 66e5465773 on Jul 2, 2015
  43. laanwj cross-referenced this on Jul 3, 2015 from issue Anti DoS: give Tor exits a lower priority than clearnet peers by mikehearn
  44. paveljanik cross-referenced this on Aug 6, 2016 from issue Do not shadow member variables in serialization by paveljanik
  45. str4d cross-referenced this on Jul 14, 2017 from issue Bitcoin 0.12 P2P/Net PRs 1 by str4d
  46. str4d cross-referenced this on Dec 19, 2017 from issue banlist.dat: store banlist on disk by str4d
  47. zkbot referenced this in commit d0b67191e2 on Apr 5, 2018
  48. hebasto cross-referenced this on Oct 29, 2019 from issue refactor: Remove addrdb.h dependency from node.h by hebasto
  49. zkbot referenced this in commit bf0f1cbee9 on Mar 6, 2020
  50. jnewbery cross-referenced this on Aug 17, 2020 from issue Deprecate banlist.dat by jnewbery
  51. zkbot referenced this in commit 2d9a9aaa83 on Nov 11, 2020
  52. zkbot referenced this in commit f40121446d on Nov 12, 2020
  53. zkbot referenced this in commit 049951dc45 on Feb 11, 2021
  54. zkbot referenced this in commit b3a6729944 on Feb 16, 2021
  55. zkbot referenced this in commit e85265fbd5 on Feb 17, 2021
  56. zkbot referenced this in commit b4b07a1bbd on Feb 17, 2021
  57. bitcoin locked this on Sep 8, 2021
Contributors
jonasschnellijgarzikDiapololaanwjdexX7sipaajweiss
Labels
Feature
Linked (view graph)
#6315 [Qt] allow banning and unbanning over UI->peers table#6363 Bugfix: RPC net: Correct spelling of "until" in listbanned#6364 Anti DoS: give Tor exits a lower priority than clearnet peers#8468 Do not shadow member variables in serialization#17297 refactor: Remove addrdb.h dependency from node.h #19748 Deprecate banlist.dat
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: 2026-05-20 06:55 UTC