banlist updates

Diapolo commented at 7:49 am on July 3, 2015: none

banlist: update set dirty to be more fine grained

move the SetBannedSetDirty(false) call from DumpData() into DumpBanlist()
ensure we only set false, if the write succeeded

banlist: better handling of banlist in StartNode()

only start working on/with banlist data, if reading in the banlist from disk didn’t fail
as CNode::setBannedIsDirty is false (default) when reading fails, we don’t need to explicitly set it to false to prevent writing banlist.dat in that case either

banlist: add more banlist infos to log / add GUI signal

to match the peers.dat handling also supply a debug.log entry for how many entries were loaded from banlist.dat and how long it took
add a GUI init message for loading the banlist (same as with peers.dat)
move the same message for peers.dat upwards in the code, to be able to reuse the timing variable nStart and also just log, if our read from peers.dat didn’t fail

banlist (bugfix): allow CNode::SweepBanned() to run on interval

allows CNode::SweepBanned() to run, even if !CNode::BannedSetIsDirty(), because if nBanUntil is over we want the ban to be disabled for these nodes

in src/net.cpp: in 860da409cc outdated

1722-    if (!bandb.Read(banmap))
1723+    if (!bandb.Read(banmap)) {
1724         LogPrintf("Invalid or missing banlist.dat; recreating\n");
1725+    } else {
1726+        CNode::SetBanned(banmap); // thread save setter
1727+        CNode::SetBannedSetDirty(false); // no need to write down, just read data

jonasschnelli commented at 7:52 am on July 3, 2015:

This line CNode::SetBannedSetDirty(false); should be called even if bandb.Read(ban map) failed.

Diapolo commented at 7:54 am on July 3, 2015:

As written in the code, it is false as default, so it isn’t dirty, because no functions were called that change the flag :).

jonasschnelli commented at 8:00 am on July 3, 2015:

Why is it’s default false (net.cppL448)? I would recommend to callCNode::SetBannedSetDirty(false);` when we start up a node. There is no case where the list can be “dirty” in this state.

Diapolo commented at 8:10 am on July 3, 2015:

IMHO an not explicitly initialized bool defaults to false (0). This call here (where the comment is), is necessary because we just read and didn’t change anything, as the CNode::SetBanned() call sets setBannedIsDirty = true.

jonasschnelli commented at 7:54 am on July 3, 2015: contributor

utACK (mind the nit above).

in src/net.cpp: in 860da409cc outdated

1712+        else
1713+            LogPrintf("Loaded %i addresses from peers.dat  %dms\n", addrman.size(), GetTimeMillis() - nStart);
1714     }
1715 
1716-    //try to read stored banlist
1717+    uiInterface.InitMessage(_("Loading banned addresses..."));

Diapolo commented at 7:55 am on July 3, 2015:

@jonasschnelli Should this be ips/subnets rather than addresses?

jonasschnelli commented at 8:01 am on July 3, 2015:

Hmm…. it could be _("Loading banned addresses/subnets...") but if we be particular about this: a subnet defines an amount of “addresses”. But i don’t care about that.

laanwj commented at 8:02 am on July 3, 2015:

"Loading ban list..."

laanwj added the label P2P on Jul 3, 2015

Diapolo commented at 3:59 pm on July 3, 2015: none

@jonasschnelli Would be nice if you could test this pull and review commit 4, so this can get merged soon. I’ve got further small fixes for this but don’t want to blow up this pull ;).

Diapolo commented at 7:17 pm on July 21, 2015: none

@jonasschnelli Ping

Diapolo commented at 9:25 am on July 29, 2015: none

@jonasschnelli @laanwj Can we get this in please?

Diapolo commented at 12:14 pm on August 10, 2015: none

@jonasschnelli Too sad you couldn’t yet ACK this, mind taking a look?

in src/net.cpp: in 2678c7cc93 outdated

2507 
2508-    LogPrint("net", "Flushed %d banned node ips/subnets to banlist.dat  %dms\n",
2509-             banmap.size(), GetTimeMillis() - nStart);
2510+        LogPrint("net", "Flushed %d banned node ips/subnets to banlist.dat  %dms\n",
2511+            banmap.size(), GetTimeMillis() - nStart);
2512+    }

jgarzik commented at 1:31 pm on September 16, 2015:

General principle: Code like “if (!dirty) return;” reduces the indentation level for the standard case, and makes the code more readable.

Diapolo commented at 2:03 pm on September 16, 2015:

So you would have written?

0if (bandb.Write(banmap)) CNode::SetBannedSetDirty(false);

Or are you talking about:

0LogPrint("net", "Flushed %d banned node ips/subnets to banlist.dat  %dms\n", banmap.size(), GetTimeMillis() - Start);

MarcoFalke commented at 3:57 pm on September 16, 2015:

I think it was if (!CNode::BannedSetIsDirty()) return;

Diapolo commented at 4:44 pm on September 16, 2015:

LOL, got it now ;)… thanks for that hint.

dcousens commented at 1:34 am on September 19, 2015:

@Diapolo any chance you’ll do the early exit as @jgarzik suggested?

jgarzik commented at 1:31 pm on September 16, 2015: contributor

ut ACK

Diapolo commented at 2:04 pm on September 16, 2015: none

@jgarzik Thanks for review :).

in src/net.cpp: in 2678c7cc93 outdated

554@@ -555,11 +555,13 @@ void CNode::SweepBanned()
555     banmap_t::iterator it = setBanned.begin();
556     while(it != setBanned.end())
557     {
558+        CSubNet subNet = (*it).first;

dcousens commented at 1:01 am on September 19, 2015:

const?

Diapolo commented at 11:28 am on September 19, 2015:

Not used a single time currently in net.cpp with (*it), so I’ll leave that. But I’m open for further comments on this.

in src/net.cpp: in 2678c7cc93 outdated

1897@@ -1901,31 +1898,37 @@ void static Discover(boost::thread_group& threadGroup)
1898 void StartNode(boost::thread_group& threadGroup, CScheduler& scheduler)
1899 {
1900     uiInterface.InitMessage(_("Loading addresses..."));
1901-    // Load addresses for peers.dat
1902+    // Load addresses from peers.dat
1903     int64_t nStart = GetTimeMillis();
1904     {
1905         CAddrDB adb;
1906         if (!adb.Read(addrman))

dcousens commented at 1:05 am on September 19, 2015:

nit: Avoid the negation in logic here now that we handle both cases?

Diapolo commented at 11:32 am on September 19, 2015:

OK, will push a new version.

in src/net.cpp: in 2678c7cc93 outdated

1914+    // Load addresses from banlist.dat
1915+    nStart = GetTimeMillis();
1916     CBanDB bandb;
1917     banmap_t banmap;
1918-    if (!bandb.Read(banmap))
1919+    if (!bandb.Read(banmap)) {

dcousens commented at 1:12 am on September 19, 2015:

nit: same here?

Diapolo commented at 11:36 am on September 19, 2015:

Yeah, will also push a new version ;).

in src/net.cpp: in 2678c7cc93 outdated

2487 }
2488 
2489 void DumpBanlist()
2490 {
2491-    int64_t nStart = GetTimeMillis();
2492+    CNode::SweepBanned(); // clean unused entires (if bantime has expired)

dcousens commented at 1:27 am on September 19, 2015:

s/entires/entries

dcousens commented at 1:50 am on September 19, 2015: contributor

utACK

Diapolo commented at 11:58 am on September 19, 2015: none

@dcousens @jgarzik Reworked to fix your valuable nits, can you re-check and re-ACK please?

dcousens commented at 3:21 pm on September 19, 2015: contributor

@Diapolo re-reviewed commit-by-commit. utACK / concept ACK (683275e)

Diapolo commented at 5:58 am on September 21, 2015: none

Seems unrelated!?

0Running 161 test cases...
1
2No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.
3
4The build has been terminated

banlist: update set dirty to be more fine grained

- move the SetBannedSetDirty(false) call from DumpData() into DumpBanlist()
- ensure we only set false, if the write succeeded

57c77fe4d3

banlist: better handling of banlist in StartNode()

- only start working on/with banlist data, if reading in the banlist from
  disk didn't fail
- as CNode::setBannedIsDirty is false (default) when reading fails, we
  don't need to explicitly set it to false to prevent writing
  banlist.dat in that case either

ce479aaada

banlist: add more banlist infos to log / add GUI signal

- to match the peers.dat handling also supply a debug.log entry for how
  many entries were loaded from banlist.dat and how long it took
- add a GUI init message for loading the banlist (same as with peers.dat)

- move the same message for peers.dat upwards in the code, to be able to
  reuse the timing variable nStart and also just log, if our read from
  peers.dat didn't fail

2977c243ef

banlist (bugfix): allow CNode::SweepBanned() to run on interval

- allows CNode::SweepBanned() to run, even if !CNode::BannedSetIsDirty(),
  because if nBanUntil is over we want the ban to be disabled for these
  nodes

e8600c924d

Diapolo commented at 7:16 am on October 9, 2015: none

What is holding up the merge here? @jgarzik Ping :)

dcousens commented at 11:56 pm on October 12, 2015: contributor

utACK

luke-jr referenced this in commit d1997d64c6 on Oct 15, 2015

laanwj commented at 8:14 am on October 27, 2015: member

All utACK but I can’t really see from the code changes whether it’s correct. No one has tested this yet? Or is this covered by the current tests?

Diapolo commented at 4:57 pm on October 27, 2015: none

Well, I tested it :-D and I also was aware of these things while @jonasschnelli implemented it, but AFAIK no one was interrested in fixing/improving at that time…

jonasschnelli commented at 5:05 pm on October 27, 2015: contributor

utACK. A unittest would be nice (or extending the rpc test with some banlist.dat checking on file basis).

Diapolo closed this on Oct 31, 2015

Diapolo deleted the branch on Oct 31, 2015

DrahtBot locked this on Sep 8, 2021

banlist updates #6371