Nice find. Though I can't think of a good reason why we'd ever want a stale list coming out of GetBanned(). Why not just sweep there?

utACK ea2c925c99969d06320495afbf1188f54e25e015
Calling sweep from within GetBanned() looks simpler in the first place but would break the assumption that GetBanned() can never change the setBannedIsDirty to true.
No strong opinion though.

Calling sweep from within GetBanned() looks simpler in the first place but would break the assumption that GetBanned() can never change the setBannedIsDirty to true.

Right. CConnman::DumpBanlist() sets SetBannedSetDirty and then calls GetBanned() (I believe making the assumption that GetBanned() wouldn't change SetBannedSetDirty). I didn't know whether it was safe to change that.

If you think that's ok, then calling sweep from with GetBanned() is obviously tidier, and means I don't need to make SweepBanned() public.

Needs rebase

Discussion on IRC: the disconnect_ban.py test is slow until this is merged, due to https://github.com/bitcoin/bitcoin/blob/master/test/functional/disconnect_ban.py#L67

rebased with fix to disconnect_ban.py test suite.

@jnewbery Yea, I'd prefer to see this moved to GetBanned(). Otherwise correct usage just isn't obvious enough (as evidenced by this bug).

Ideally the sweep would be a static function operating on a banlist_t rather than this, but I don't think it's worth worrying about.

Looks like the only other change needed would be:

@@ -496,10 +499,9 @@ void CConnman::DumpBanlist()
 
     CBanDB bandb;
     banmap_t banmap;
-    SetBannedSetDirty(false);
     GetBanned(banmap);
-    if (!bandb.Write(banmap))
-        SetBannedSetDirty(true);
+    if (bandb.Write(banmap))
+        SetBannedSetDirty(false);

Note that this code is pretty racy either way. We should fix that as a follow-up.

thanks @theuni - that's definitely cleaner. I've changed this PR to use your fix.

It fails locally here (every time, not just intermittently):

disconnect_ban.py failed, Duration: 26 s

stdout:
2017-05-01 08:21:38.048000 TestFramework (INFO): Initializing test directory /tmp/testh_zvdgd7/435
2017-05-01 08:21:39.087000 TestFramework (INFO): Test setban and listbanned RPCs
2017-05-01 08:21:39.087000 TestFramework (INFO): setban: successfully ban single IP address
2017-05-01 08:21:39.148000 TestFramework (INFO): clearbanned: successfully clear ban list
2017-05-01 08:21:39.179000 TestFramework (INFO): setban: fail to ban an already banned subnet
2017-05-01 08:21:39.184000 TestFramework (INFO): setban: fail to ban an invalid subnet
2017-05-01 08:21:39.189000 TestFramework (INFO): setban remove: fail to unban a non-banned subnet
2017-05-01 08:21:39.194000 TestFramework (INFO): setban remove: successfully unban subnet
2017-05-01 08:21:39.243000 TestFramework (INFO): setban: test persistence across node restart
2017-05-01 08:21:49.801000 TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/store/orion/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 151, in main
    self.run_test()
  File "/home/orion/projects/bitcoin/bitcoin/test/functional/disconnect_ban.py", line 67, in run_test
    assert wait_until(lambda: len(self.nodes[1].listbanned()) == 3, timeout=10)
AssertionError
2017-05-01 08:21:49.819000 TestFramework (INFO): Stopping nodes
2017-05-01 08:22:04.002000 TestFramework (WARNING): Not cleaning up dir /tmp/testh_zvdgd7/435
2017-05-01 08:22:04.003000 TestFramework (ERROR): Test failed. Test logging available at /tmp/testh_zvdgd7/435/test_
framework.log

@laanwj that's very strange. The test passes for me consistently, and also passes on travis. Can you try rebuilding and clearing the functional test cache. If it's still failing, can you send me the test logs?

Seems to work now. Strange.