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

1. 
   NicolasDorier commented at 7:37 AM on August 15, 2019: contributor

   Reported by @MarcoFalke on #16248 (review)

   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.

2. fanquake added the label P2P on Aug 15, 2019
3. 
   GChuf commented at 10:34 AM on August 15, 2019: contributor

   Is this safe to do? What are the implications of connecting to a peer that would otherwise be banned?

4. 
   Sjors commented at 10:52 AM on August 15, 2019: member

   @gapeman afaik that was the behavior before #16248 (which was not supposed to change existing behavior). @NicolasDorier would be nice to have a test for this.

5. 
   NicolasDorier commented at 1:12 PM on August 15, 2019: contributor

   @gapeman if your config specifically say that you don't want to ban a peer, then it should not be banned.

   As @Sjors says, I just want to keep current behavior working as before.

   I can take a look to see if I can make a test for that.

6. NicolasDorier force-pushed on Aug 15, 2019
7. NicolasDorier force-pushed on Aug 15, 2019
8. 
   NicolasDorier commented at 1:28 PM on August 15, 2019: contributor

   I refactored it to properly scope some variables. Writing a test so such regression can't happen.

9. [Fix] Allow connection of a noban banned peer dc7529abf0
10. NicolasDorier force-pushed on Aug 15, 2019
11. NicolasDorier force-pushed on Aug 15, 2019
12. 
    NicolasDorier commented at 2:02 PM on August 15, 2019: contributor

    Fixed the bug, and added an anti regression test for setban

13. NicolasDorier force-pushed on Aug 15, 2019
14. 
    naumenkogs commented at 2:08 PM on August 15, 2019: member

    utACK a0d2e2b

15. MarcoFalke added this to the milestone 0.19.0 on Aug 15, 2019
16. in test/functional/rpc_setban.py:7 in a0d2e2b1bd outdated

       0 | @@ -0,0 +1,44 @@
       1 | +#!/usr/bin/env python3
       2 | +# Copyright (c) 2015-2019 The Bitcoin Core developers
       3 | +# Distributed under the MIT software license, see the accompanying
       4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
       5 | +"""Test the setban rpc call."""
       6 | +
       7 | +from test_framework.test_node import ErrorMatch
    

    ---

    ---

    

    MarcoFalke commented at 2:14 PM on August 15, 2019:

    test/functional/rpc_setban.py:7:1: F401 'test_framework.test_node.ErrorMatch' imported but unused

    test/functional/rpc_setban.py:39:78: F821 undefined name 'p'

    test/functional/rpc_setban.py:41:75: F821 undefined name 'p'

17. in test/functional/rpc_setban.py:34 in a0d2e2b1bd outdated

      30 | +        # Node 1 should be able to reconnect if it has noban permission
      31 | +        self.restart_node(1, ['-whitelist=127.0.0.1'])
      32 | +        connect_nodes(self.nodes[0], 1)
      33 | +        self.assert_has_permission('noban', True)
      34 | +
      35 | +    def assert_has_permission(self, permission, contains):
    

    ---

    ---

    

    Sjors commented at 2:23 PM on August 15, 2019:

    Maybe pass node in directly and split into assert_has_permission(self, node, permission) and assert_has_no_permission(self, node, permission)

    ---

    

    NicolasDorier commented at 3:18 PM on August 15, 2019:

    what about assert_equal(True, self.hasPermision(...)) ?

    That said, I don't think it should pass the nodes to this method. It is confusing (there is two nodes: the node that is permissioned, and the node on which we want to check permissions).

    ---

    

    Sjors commented at 4:19 PM on August 15, 2019:

    assert(self.hasPermision(...)) is even simpler.

    You could pass both nodes. Or you could document at the top of the test file which hardcoded nodes do what.

    ---

    

    promag commented at 12:02 AM on August 16, 2019:

    Sorry but I think you could just inline this - it's longer as it is with the contains arg.

18. 
    Sjors commented at 2:25 PM on August 15, 2019: member

    Travis failed due to lint issue. Maybe rename rpc_setban.py to p2p_whitelist.py?

19. NicolasDorier force-pushed on Aug 15, 2019
20. 
    promag commented at 12:05 AM on August 16, 2019: member

    ACK

21. NicolasDorier force-pushed on Aug 16, 2019
22. 
    NicolasDorier commented at 4:19 AM on August 16, 2019: contributor

    As @promag suggested, I instead just inlined the permission checked, it is actually more readable.

    I also check in the logs that we really prevent connection because of the ban. I also added a test to setban x remove.

23. NicolasDorier force-pushed on Aug 16, 2019
24. NicolasDorier force-pushed on Aug 16, 2019
25. NicolasDorier force-pushed on Aug 16, 2019
26. 
    NicolasDorier commented at 4:23 AM on August 16, 2019: contributor

    @Sjors rpc_setban is the right name, I really want to test setban, so I added it in the whitelist of authorized term.

27. Add test for setban d117f4541d
28. NicolasDorier force-pushed on Aug 16, 2019
29. 
    Sjors commented at 8:22 AM on August 16, 2019: member

    Agree inline is more clear. utACK d117f45

30. 
    MarcoFalke commented at 2:09 PM on August 16, 2019: member

    ACK d117f4541d4717e83c9396273e92960723622030

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA512
    
    ACK d117f4541d4717e83c9396273e92960723622030
    -----BEGIN PGP SIGNATURE-----
    
    iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    pUin2gv9GZKDSUbn62676zj0G6Igb8tzgC2M1Sf2KyEuIR+heCoVYGlyp3rXDbuc
    22c5dyDXDnktsyMR3mwo3KLzT/MBY/wc8nDgdzkRUzWt6j+/q8Ex8NTnfbgyGXzr
    vBE3bJOfA1y3DCCzpSar4uU/sPUAK+oJ3qF1+62rijDYpC0Jh1UaYMy7AdYQdYQ/
    kyO3jorAiwIk0sE7SzJDq3rE7wlHzQ4LCZauCnVft/Y1rOY1IkmYfke3uapR4EVr
    Zw7D40jY6RsEDLNh/1odaRqjm01v1uJ5d7mFnqOFCKA7RwdS2NQ0ElbfQ3fAUowm
    V7Oola76XE8OvGsj9y+qOwBkX5iMgeNQANb+Pf6poB0ANDh0AKpIkxjSIGGbhKi2
    XN9KGRaHLpNBeU69lQDrvsXEzyfl4kSLyO9/k7+qZAtgUTqyLDhi9beqS9Ta0OKF
    X7RHymgGIn1zvTOY4WSoPiHIF5ceVDu9S5r/fxr9DHmwPWWEzOE9ztfcSiQw68tY
    HRrWUvc+
    =qI3j
    -----END PGP SIGNATURE-----
    

    Timestamp of file with hash 74a14b73630691da8ffa64e5b597d7fd7246787327bba54579da5c03c881ec01 -

    </details>

31. MarcoFalke referenced this in commit b80cdfec9a on Aug 16, 2019
32. MarcoFalke merged this on Aug 16, 2019
33. MarcoFalke closed this on Aug 16, 2019

    ---

34. luke-jr referenced this in commit 358051e7cd on Sep 3, 2019
35. luke-jr referenced this in commit 7a0fe3848b on Sep 3, 2019
36. deadalnix referenced this in commit 82899c9ef4 on May 2, 2020
37. ftrader referenced this in commit cad767a4ac on Aug 17, 2020
38. PastaPastaPasta referenced this in commit a67cacae29 on Jul 18, 2021
39. PastaPastaPasta referenced this in commit a8ed0d4de2 on Jul 18, 2021
40. PastaPastaPasta referenced this in commit 6641befcf7 on Jul 19, 2021
41. PastaPastaPasta referenced this in commit 9d2d8c1d91 on Jul 19, 2021
42. PastaPastaPasta referenced this in commit ce8b04fc38 on Jul 20, 2021
43. PastaPastaPasta referenced this in commit ec608315d8 on Jul 20, 2021
44. DrahtBot locked this on Dec 16, 2021

Contributors

NicolasDorierGChufSjorsnaumenkogsMarcoFalkepromag

Milestone
0.19.0