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

[net] listbanned RPC and QT should show correct banned subnets #10234

Merged
merged 2 commits into from May 2, 2017

Conversation

Projects
None yet
6 participants
@jnewbery
Member

jnewbery commented Apr 19, 2017

The listbanned RPC and QT show entries in the connman.setBanned set, even if the entry has expired. Expired entries in the set are only swept in the following circumstances:

  • bitcoind is stopped/started
  • the setban or clearbanned RPC is called

That means that the list of banned subnets returned by listbanned is inconsistent. If the node has been stop-started or the setban/clearbanned RPC has been called, then stale entries won't be shown. If the node hasn't been stop-started and those RPCs haven't been called, then stale entries will be shown.

This PR calls SweepBanned() before GetBanned() in the listbanned RPC and QT, so all calls to GetBanned() return an up-to-date list of banned subnets.

This PR also adds a test to nodehandling to test the behaviour.

@jonasschnelli @sdaftuar

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 19, 2017

Member

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?

Member

theuni commented Apr 19, 2017

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?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 19, 2017

Member

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

Member

jonasschnelli commented Apr 19, 2017

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

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Apr 19, 2017

Member

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.

Member

jnewbery commented Apr 19, 2017

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.

@fanquake fanquake added the P2P label Apr 19, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Apr 21, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Apr 21, 2017

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Apr 23, 2017

Member

Needs rebase

Member

MarcoFalke commented Apr 23, 2017

Needs rebase

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 25, 2017

Member

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

Member

laanwj commented Apr 25, 2017

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

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Apr 25, 2017

Member

rebased with fix to disconnect_ban.py test suite.

Member

jnewbery commented Apr 25, 2017

rebased with fix to disconnect_ban.py test suite.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 25, 2017

Member

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

Member

theuni commented Apr 25, 2017

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

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Apr 28, 2017

Member

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

Member

jnewbery commented Apr 28, 2017

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 1, 2017

Member

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
Member

laanwj commented May 1, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery May 1, 2017

Member

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

Member

jnewbery commented May 1, 2017

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 2, 2017

Member

Seems to work now. Strange.

Member

laanwj commented May 2, 2017

Seems to work now. Strange.

@laanwj laanwj merged commit d6732d8 into bitcoin:master May 2, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 2, 2017

Merge #10234: [net] listbanned RPC and QT should show correct banned …
…subnets

d6732d8 [tests] update disconnect_ban.py test case to work with listbanned (John Newbery)
77c54b2 [net] listbanned RPC and QT should show correct banned subnets (John Newbery)

Tree-SHA512: edd0e43377d456260d2697213c2829f8483630f3a668b6707d52605faefa610d951d10e6f22a95eff483cbd14faa8ac9b69fa7d3c0b5735c5f3df23fd71282e0

@jnewbery jnewbery deleted the jnewbery:list_banned_correctly branch May 2, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Jul 17, 2017

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Jul 17, 2017

karel-3d added a commit to karel-3d/bitcoin that referenced this pull request Oct 30, 2017

karel-3d added a commit to karel-3d/bitcoin that referenced this pull request Oct 30, 2017

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