Merge #16618: [Fix] Allow connection of a noban banned peer

d117f4541d4717e83c9396273e92960723622030 Add test for setban (nicolas.dorier)
dc7529abf0197dccb876dc4a93cbdd2ad9f03e5c [Fix] Allow connection of a noban banned peer (nicolas.dorier)

Pull request description:

  Reported by @MarcoFalke on https://github.com/bitcoin/bitcoin/pull/16248#discussion_r314026195

  The bug would mean that if the peer connecting to you is banned, but whitelisted without specific permissions, it would not be able to connect to the node.

  The solution is just to move the same line below.

ACKs for top commit:
  Sjors:
    Agree inline is more clear. utACK d117f45
  MarcoFalke:
    ACK d117f4541d4717e83c9396273e92960723622030

Tree-SHA512: 0fed39acb1e8db67bb0bf4c4de3ad034ae776f38d55bd661f1ae0e1a4c6becaf1824ab46ed8279f2f31df3f4b29ff56461d8b167d3e9cece62cfe58b5a912811

4 files changed, 50 insertions, 2 deletions

diff --git a/src/net.cpp b/src/net.cpp
index 0464a6e9ea..0391edadaa 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -906,7 +906,6 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
     NetPermissionFlags permissionFlags = NetPermissionFlags::PF_NONE;
     hListenSocket.AddSocketPermissionFlags(permissionFlags);
     AddWhitelistPermissionFlags(permissionFlags, addr);
-    const bool noban = NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN);
     bool legacyWhitelisted = false;
     if (NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_ISIMPLICIT)) {
         NetPermissions::ClearFlag(permissionFlags, PF_ISIMPLICIT);
@@ -953,7 +952,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
 
     // Don't accept connections from banned peers, but if our inbound slots aren't almost full, accept
     // if the only banning reason was an automatic misbehavior ban.
-    if (!noban && bannedlevel > ((nInbound + 1 < nMaxInbound) ? 1 : 0))
+    if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && bannedlevel > ((nInbound + 1 < nMaxInbound) ? 1 : 0))
     {
         LogPrint(BCLog::NET, "connection from %s dropped (banned)\n", addr.ToString());
         CloseSocket(hSocket);
diff --git a/test/functional/rpc_setban.py b/test/functional/rpc_setban.py
new file mode 100644
index 0000000000..a1a8196557
--- /dev/null
+++ b/test/functional/rpc_setban.py
@@ -0,0 +1,47 @@
+#!/usr/bin/env python3
+# Copyright (c) 2015-2019 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test the setban rpc call."""
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    connect_nodes,
+    p2p_port
+)
+
+class SetBanTests(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 2
+        self.setup_clean_chain = True
+        self.extra_args = [[],[]]
+
+    def run_test(self):
+        # Node 0 connects to Node 1, check that the noban permission is not granted
+        connect_nodes(self.nodes[0], 1)
+        peerinfo = self.nodes[1].getpeerinfo()[0]
+        assert(not 'noban' in peerinfo['permissions'])
+
+        # Node 0 get banned by Node 1
+        self.nodes[1].setban("127.0.0.1", "add")
+
+        # Node 0 should not be able to reconnect
+        with self.nodes[1].assert_debug_log(expected_msgs=['dropped (banned)\n']):
+            self.restart_node(1, [])
+            self.nodes[0].addnode("127.0.0.1:" + str(p2p_port(1)), "onetry")
+
+        # However, node 0 should be able to reconnect if it has noban permission
+        self.restart_node(1, ['-whitelist=127.0.0.1'])
+        connect_nodes(self.nodes[0], 1)
+        peerinfo = self.nodes[1].getpeerinfo()[0]
+        assert('noban' in peerinfo['permissions'])
+
+        # If we remove the ban, Node 0 should be able to reconnect even without noban permission
+        self.nodes[1].setban("127.0.0.1", "remove")
+        self.restart_node(1, [])
+        connect_nodes(self.nodes[0], 1)
+        peerinfo = self.nodes[1].getpeerinfo()[0]
+        assert(not 'noban' in peerinfo['permissions'])
+
+if __name__ == '__main__':
+    SetBanTests().main()
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index bca5ac644f..ad5673e03a 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -146,6 +146,7 @@ BASE_SCRIPTS = [
     'rpc_net.py',
     'wallet_keypool.py',
     'p2p_mempool.py',
+    'rpc_setban.py',
     'p2p_blocksonly.py',
     'mining_prioritisetransaction.py',
     'p2p_invalid_locator.py',
diff --git a/test/lint/lint-spelling.ignore-words.txt b/test/lint/lint-spelling.ignore-words.txt
index a25de2435b..b08837c1d4 100644
--- a/test/lint/lint-spelling.ignore-words.txt
+++ b/test/lint/lint-spelling.ignore-words.txt
@@ -12,3 +12,4 @@ cachable
 errorstring
 keyserver
 homogenous
+setban