banlist updates #6371

Diapolo · 2015-07-03T07:49:07Z

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

jonasschnelli · 2015-07-03T07:52:18Z

src/net.cpp

        LogPrintf("Invalid or missing banlist.dat; recreating\n");
+    } else {
+        CNode::SetBanned(banmap); // thread save setter
+        CNode::SetBannedSetDirty(false); // no need to write down, just read data


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

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

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.

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 · 2015-07-03T07:54:01Z

utACK (mind the nit above).

Diapolo · 2015-07-03T07:55:18Z

src/net.cpp

    }

-    //try to read stored banlist
+    uiInterface.InitMessage(_("Loading banned addresses..."));


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

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.

"Loading ban list..."

Diapolo · 2015-07-03T15:59:16Z

@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 · 2015-07-21T19:17:17Z

@jonasschnelli Ping

Diapolo · 2015-07-29T09:25:18Z

@jonasschnelli @laanwj Can we get this in please?

Diapolo · 2015-08-10T12:14:05Z

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

jgarzik · 2015-09-16T13:31:06Z

src/net.cpp

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


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

So you would have written?

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

Or are you talking about:

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

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

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

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

jgarzik · 2015-09-16T13:31:23Z

ut ACK

Diapolo · 2015-09-16T14:04:32Z

@jgarzik Thanks for review :).

dcousens · 2015-09-19T01:01:47Z

src/net.cpp

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


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.

dcousens · 2015-09-19T01:50:21Z

utACK

Diapolo · 2015-09-19T11:58:57Z

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

dcousens · 2015-09-19T15:21:11Z

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

Diapolo · 2015-09-21T05:58:28Z

Seems unrelated!?

Running 161 test cases...

No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

The build has been terminated

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

- 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

- 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

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

Diapolo · 2015-10-09T07:16:47Z

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

dcousens · 2015-10-12T23:56:12Z

utACK

laanwj · 2015-10-27T08:14:38Z

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 · 2015-10-27T16:57:48Z

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 · 2015-10-27T17:05:00Z

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

jonasschnelli reviewed Jul 3, 2015
View reviewed changes

Diapolo reviewed Jul 3, 2015
View reviewed changes

laanwj added the P2P label Jul 3, 2015

jgarzik reviewed Sep 16, 2015
View reviewed changes

dcousens reviewed Sep 19, 2015
View reviewed changes

Diapolo mentioned this pull request Sep 22, 2015

[Qt] allow banning and unbanning over UI->peers table #6315

Merged

Philip Kaufmann added 4 commits October 2, 2015 11:38

banlist: update set dirty to be more fine grained

57c77fe

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

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

e8600c9

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

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Oct 15, 2015

banlist: Update to final PR bitcoin#6371 merged upstream

d1997d6

Diapolo closed this Oct 31, 2015

Diapolo deleted the banlist branch October 31, 2015 11:27

luke-jr mentioned this pull request Jan 15, 2016

Banlist updates #7350

Merged

OlegGirko mentioned this pull request Jul 4, 2017

Backport Bitcoin PR#7350: Banlist updates dashpay/dash#1505

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

banlist updates #6371

banlist updates #6371

Uh oh!

Conversation

Diapolo commented Jul 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jul 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Jul 3, 2015

Uh oh!

Diapolo commented Jul 21, 2015

Uh oh!

Diapolo commented Jul 29, 2015

Uh oh!

Diapolo commented Aug 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgarzik commented Sep 16, 2015

Uh oh!

Diapolo commented Sep 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens commented Sep 19, 2015

Uh oh!

Diapolo commented Sep 19, 2015

Uh oh!

dcousens commented Sep 19, 2015

Uh oh!

Diapolo commented Sep 21, 2015

Uh oh!

Diapolo commented Oct 9, 2015

Uh oh!

dcousens commented Oct 12, 2015

Uh oh!

laanwj commented Oct 27, 2015

Uh oh!

Diapolo commented Oct 27, 2015

Uh oh!

jonasschnelli commented Oct 27, 2015

Uh oh!

Uh oh!