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

banlist.dat: store banlist on disk #6310

Merged
merged 4 commits into from Jul 2, 2015

Conversation

Projects
None yet
7 participants
@jonasschnelli
Member

jonasschnelli commented Jun 19, 2015

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.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jun 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.

Contributor

jgarzik commented Jun 19, 2015

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.

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 19, 2015

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

Diapolo commented Jun 19, 2015

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

@Diapolo

View changes

Show outdated Hide outdated src/net.cpp
return error("%s: Failed to open file %s", __func__, pathBanlist.string());
// use file size to size memory buffer
int fileSize = boost::filesystem::file_size(pathBanlist);

This comment has been minimized.

@Diapolo

Diapolo Jun 19, 2015

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

@Diapolo

Diapolo Jun 19, 2015

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 19, 2015

Member

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.

@jonasschnelli

jonasschnelli Jun 19, 2015

Member

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.

This comment has been minimized.

@Diapolo

Diapolo Jun 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.

@Diapolo

Diapolo Jun 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.

This comment has been minimized.

@laanwj

laanwj Jun 26, 2015

Member

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.

@laanwj

laanwj Jun 26, 2015

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 26, 2015

Member

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

@jonasschnelli

jonasschnelli Jun 26, 2015

Member

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

@Diapolo

View changes

Show outdated Hide outdated src/net.cpp
// Don't try to resize to a negative number if file is small
if (dataSize < 0)
dataSize = 0;
vector<unsigned char> vchData;

This comment has been minimized.

@Diapolo

Diapolo Jun 19, 2015

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

@Diapolo

Diapolo Jun 19, 2015

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

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Jun 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.

Contributor

dexX7 commented Jun 26, 2015

@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.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 26, 2015

Member
Member

sipa commented Jun 26, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 26, 2015

Member

@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.

Member

jonasschnelli commented Jun 26, 2015

@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.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 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?

Member

laanwj commented Jun 26, 2015

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?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 26, 2015

Member

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

Member

jonasschnelli commented Jun 26, 2015

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 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).

Member

laanwj commented Jun 26, 2015

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

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 26, 2015

@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.

Diapolo commented Jun 26, 2015

@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.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 26, 2015

Member

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

Member

laanwj commented Jun 26, 2015

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

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 26, 2015

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

Diapolo commented Jun 26, 2015

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 26, 2015

Member

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.

Member

jonasschnelli commented Jun 26, 2015

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.

@laanwj

View changes

Show outdated Hide outdated src/rpcnet.cpp
UniValue rec(UniValue::VOBJ);
rec.push_back(Pair("address", (*it).first.ToString()));
rec.push_back(Pair("banned_untill", (*it).second));
rec.push_back(Pair("banned_untill", banEntry.nBanUntil));

This comment has been minimized.

@laanwj

laanwj Jun 29, 2015

Member

s/untill/until/ I suppose?

@laanwj

laanwj Jun 29, 2015

Member

s/untill/until/ I suppose?

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 29, 2015

Member

s/untill/until/ I suppose?

Fixed typo.

@jonasschnelli

jonasschnelli Jun 29, 2015

Member

s/untill/until/ I suppose?

Fixed typo.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 29, 2015

Member

utACK

Member

laanwj commented Jun 29, 2015

utACK

@laanwj

View changes

Show outdated Hide outdated src/net.cpp
fResult = true;
}
}
return fResult;
}
void CNode::Ban(const CNetAddr& addr, int64_t bantimeoffset, bool sinceUnixEpoch) {
void CNode::Ban(const CNetAddr& addr, const BanReason &banReason, int64_t bantimeoffset, bool sinceUnixEpoch) {
CSubNet subNet(addr.ToString()+(addr.IsIPv4() ? "/32" : "/128"));

This comment has been minimized.

@laanwj

laanwj Jun 29, 2015

Member

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.

@laanwj

laanwj Jun 29, 2015

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 29, 2015

Member

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.

@jonasschnelli

jonasschnelli Jun 29, 2015

Member

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.

@laanwj

View changes

Show outdated Hide outdated src/net.cpp
if (bantimeoffset > 0)
banTime = (sinceUnixEpoch ? 0 : GetTime() )+bantimeoffset;
banEntry.nBanUntil = (sinceUnixEpoch ? 0 : GetTime() )+bantimeoffset;

This comment has been minimized.

@laanwj

laanwj Jun 29, 2015

Member

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)

@laanwj

laanwj Jun 29, 2015

Member

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)

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 29, 2015

Member

Yes. This makes more sense. Fixed and pushed.

@jonasschnelli

jonasschnelli Jun 29, 2015

Member

Yes. This makes more sense. Fixed and pushed.

@ajweiss

This comment has been minimized.

Show comment
Hide comment
@ajweiss

ajweiss Jun 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...

Contributor

ajweiss commented Jun 29, 2015

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 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.

Member

laanwj commented Jul 2, 2015

@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.

@laanwj

View changes

Show outdated Hide outdated src/netbase.cpp
memset(netmask, 255, sizeof(netmask));
std::vector<CNetAddr> vIP;
if (!LookupHost(addr.ToString().c_str(), vIP, 1, fAllowLookup))

This comment has been minimized.

@laanwj

laanwj Jul 2, 2015

Member

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

@laanwj

laanwj Jul 2, 2015

Member

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 2, 2015

Member

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

@jonasschnelli

jonasschnelli Jul 2, 2015

Member

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 2, 2015

Member

Right! The isValid must be set explicit. Fixed.

@jonasschnelli

jonasschnelli Jul 2, 2015

Member

Right! The isValid must be set explicit. Fixed.

@laanwj laanwj merged commit 177a0e4 into bitcoin:master Jul 2, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Jul 2, 2015

Merge pull request #6310
177a0e4 Adding CSubNet constructor over a single CNetAddr (Jonas Schnelli)
409bccf use CBanEntry as object container for banned nodes (Jonas Schnelli)
dfa174c CAddrDB/CBanDB: change filesize variables from int to uint64_t (Jonas Schnelli)
f581d3d banlist.dat: store banlist on disk (Jonas Schnelli)

zkbot added a commit to zcash/zcash that referenced this pull request Apr 5, 2018

Auto merge of #2812 - str4d:2074-store-banlist, r=<try>
banlist.dat: store banlist on disk

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6310
- bitcoin/bitcoin#6315
  - Only the new signal and net changes, no QT
- bitcoin/bitcoin#7350
- bitcoin/bitcoin#7458

Part of #2074.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment