test: remove duplicated ban test #29688

pull brunoerg wants to merge 1 commits into bitcoin:master from brunoerg:2024-03-ban-test changing 2 files +5 −13
  1. brunoerg commented at 10:33 AM on March 21, 2024: contributor

    Test the ban list is preserved through restart has been done by both rpc_setban and p2p_disconnect_ban. Since p2p_disconnect_ban does it in a more elegant way, we can keep only it and remove the other one.

    https://github.com/bitcoin/bitcoin/blob/bf1b6383dbbfdd0c96a161d4693a48bf3a6b6150/test/functional/p2p_disconnect_ban.py#L74-L110

  2. DrahtBot commented at 10:33 AM on March 21, 2024: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK tdb3, hernanmarino, BrandonOdiwuor, alfonsoromanz, achow101

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label Tests on Mar 21, 2024
  4. DrahtBot added the label CI failed on Mar 21, 2024
  5. brunoerg force-pushed on Mar 21, 2024
  6. DrahtBot removed the label CI failed on Mar 21, 2024
  7. in test/functional/rpc_setban.py:65 in 9cbfb82a61 outdated 
      59 | @@ -60,24 +60,6 @@ def run_test(self):
  60 |          assert not self.is_banned(node, tor_addr)
  61 |          assert not self.is_banned(node, ip_addr)
  62 |  
  63 | -        node.setban(tor_addr, "add")
  64 | -        assert self.is_banned(node, tor_addr)
  65 | -        assert not self.is_banned(node, ip_addr)
    tdb3 commented at 11:41 PM on March 24, 2024:

    Seems like this would also remove testing of banning an onion address, which doesn't seem ideal. Would recommend keeping this.

    brunoerg commented at 12:29 AM on March 25, 2024:

    Done.

  8. in test/functional/rpc_setban.py:75 in 9cbfb82a61 outdated 
      70 | -        assert self.is_banned(node, tor_addr)
  71 | -        assert not self.is_banned(node, ip_addr)
  72 | -
  73 | -        node.setban(tor_addr, "remove")
  74 | -        assert not self.is_banned(self.nodes[1], tor_addr)
  75 | -        assert not self.is_banned(node, ip_addr)
    tdb3 commented at 11:42 PM on March 24, 2024:

    Same for here. If this were kept, it would continue to test ban removal for onion addresses.

    brunoerg commented at 12:29 AM on March 25, 2024:

    Done.

  9. tdb3 commented at 11:48 PM on March 24, 2024: contributor

    Nice work taking a more holistic view of test coverage (avoiding duplicates).

    Inline comments provided. In a nutshell, unless it's covered elsewhere (and I'm missing something), I would recommend keeping lines focused on testing banning/unbanning onion addresses. Looks like the removal of restarting/checking lines can be done while keeping onion address banning/unbanning/checking.

    This would prevent the preceding lines from false advertising: https://github.com/bitcoin/bitcoin/blob/9cbfb82a610c3a40777ba372588564810e736d7c/test/functional/rpc_setban.py#L56-L61

  10. brunoerg force-pushed on Mar 25, 2024
  11. brunoerg commented at 12:31 AM on March 25, 2024: contributor

    Thanks, @tdb3 for reviewing it. Just changed it to keep the ban/unban of an onion addresses and moved the test persistence across node restart to p2p_disconnect_ban.

  12. in test/functional/p2p_disconnect_ban.py:89 in 605b1a8409 outdated 
      87 | @@ -87,7 +88,7 @@ def run_test(self):
  88 |  
  89 |          # Move time forward by 3 seconds so the third ban has expired
    tdb3 commented at 1:25 AM on March 25, 2024:

    nit:

    -        # Move time forward by 3 seconds so the third ban has expired
+        # Move time forward by 3 seconds so the forth ban has expired
    brunoerg commented at 9:38 AM on March 25, 2024:

    Done

  13. in test/functional/p2p_disconnect_ban.py:80 in 605b1a8409 outdated 
      76 | @@ -77,6 +77,7 @@ def run_test(self):
  77 |          self.nodes[1].setmocktime(old_time)
  78 |          self.nodes[1].setban("127.0.0.0/32", "add")
  79 |          self.nodes[1].setban("127.0.0.0/24", "add")
  80 | +        self.nodes[1].setban("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion", "add")
    tdb3 commented at 1:25 AM on March 25, 2024:

    nit: The line just after this has an index that isn't adjusted, but still passes.

    assert_equal("192.168.0.1/32", listBeforeShutdown[2]['address'])

    At first glance, this might be due to how the address types are organized in the list returned by listbanned() but if this organization changes then the test could fail in the future. Recommend this be updated to increase robustness. For example:

    addrs_before_shutdown = [a['address'] for a in self.nodes[1].listbanned()]
assert "192.168.0.1/32" in addrs_before_shutdown

    Author's call as to whether this is part of this PR or a new/future one. There are other parts of test that depend on specific ordering in the list (this isn't the only spot).

    brunoerg commented at 9:43 AM on March 25, 2024:

    I'll leave it as it for now. Perhaps we could change it if/when when the behaviour of listbanned changes.

  14. in test/functional/p2p_disconnect_ban.py:94 in 605b1a8409 outdated 
      87 | @@ -87,7 +88,7 @@ def run_test(self):
  88 |  
  89 |          # Move time forward by 3 seconds so the third ban has expired
  90 |          self.nodes[1].setmocktime(old_time + 3)
  91 | -        assert_equal(len(self.nodes[1].listbanned()), 4)
  92 | +        assert_equal(len(self.nodes[1].listbanned()), 5)
  93 |  
  94 |          self.log.info("Test ban_duration and time_remaining")
  95 |          for ban in self.nodes[1].listbanned():
    tdb3 commented at 1:29 AM on March 25, 2024:

    Recommend adding checks for the onion address in this loop.

    Also recommend adding check for the onion address the after shutdown (technically after restart) here: https://github.com/bitcoin/bitcoin/blob/605b1a84093f38a328940f96fcdc35866f5a7422/test/functional/p2p_disconnect_ban.py#L107-L111

    brunoerg commented at 9:41 AM on March 25, 2024:

    Nice idea, done!

  15. tdb3 commented at 1:30 AM on March 25, 2024: contributor

    Pulled, built, and ran all unit and functional tests (all passed). Updated comments for new commit 605b1a84093f38a328940f96fcdc35866f5a7422.

  16. brunoerg force-pushed on Mar 25, 2024
  17. brunoerg commented at 9:38 AM on March 25, 2024: contributor

    Force-pushed addressing #29688 (review) and #29688 (review)

  18. brunoerg force-pushed on Mar 25, 2024
  19. in test/functional/p2p_disconnect_ban.py:94 in 7cdef4bc11 outdated 
      91 |          self.nodes[1].setmocktime(old_time + 3)
  92 | -        assert_equal(len(self.nodes[1].listbanned()), 4)
  93 | +        assert_equal(len(self.nodes[1].listbanned()), 5)
  94 |  
  95 |          self.log.info("Test ban_duration and time_remaining")
  96 |          for ban in self.nodes[1].listbanned():
    tdb3 commented at 12:58 AM on March 26, 2024:

    Looking good! What if we button things up by adding checks for the onion address inside the for loop for ban in self.nodes[1].listbanned():?

    https://github.com/bitcoin/bitcoin/blob/7cdef4bc1148a6641ad6cfb2ade90cbe8fba82cb/test/functional/p2p_disconnect_ban.py#L93-L103

    brunoerg commented at 1:59 PM on March 26, 2024:

    Nice idea, done!

  20. tdb3 commented at 12:58 AM on March 26, 2024: contributor

    Inline comment left for 7cdef4bc1148a6641ad6cfb2ade90cbe8fba82cb. Pulled and re-ran p2p_disconnect_ban (passed).

  21. test: remove duplicated ban test
    Test the ban list is preserved through restart has been
done by both `rpc_setban` and `p2p_disconnect_ban`.
Since `p2p_disconnect_ban` does it in a more elegant
way, we can keep only it and remove the duplicated one.
    e30e8625bb
  22. brunoerg force-pushed on Mar 26, 2024
  23. brunoerg commented at 1:59 PM on March 26, 2024: contributor

    Force-pushed addressing #29688 (review)

  24. tdb3 commented at 1:20 AM on March 27, 2024: contributor

    ACK for e30e8625bbc42045b8b757a8d7e80c20cc61cebf. Comments addressed. Pulled latest commit and ran p2p_disconnect_ban (v1 and v2 variants). Passed.

  25. hernanmarino approved
  26. hernanmarino commented at 8:12 PM on April 9, 2024: contributor

    tested ACK e30e8625bbc42045b8b757a8d7e80c20cc61cebf

  27. BrandonOdiwuor approved
  28. BrandonOdiwuor commented at 1:47 PM on April 11, 2024: contributor

    ACK e30e8625bbc42045b8b757a8d7e80c20cc61cebf

    Looks good to me

  29. alfonsoromanz approved
  30. alfonsoromanz commented at 3:58 PM on April 15, 2024: contributor

    ACK e30e8625bbc42045b8b757a8d7e80c20cc61cebf

  31. achow101 commented at 10:42 PM on April 22, 2024: member

    It seems like these two tests primarily revolve around the setban RPC and do a couple of the same things, so perhaps they can be consolidated into one test?

  32. brunoerg commented at 10:55 PM on April 22, 2024: contributor

    It seems like these two tests primarily revolve around the setban RPC and do a couple of the same things, so perhaps they can be consolidated into one test?

    See #26863. Closed due to lack of interest.

  33. achow101 commented at 7:58 PM on April 23, 2024: member

    ACK e30e8625bbc42045b8b757a8d7e80c20cc61cebf

  34. achow101 merged this on Apr 23, 2024
  35. achow101 closed this on Apr 23, 2024
  36. bitcoin locked this on Apr 23, 2025
Contributors
brunoergDrahtBottdb3hernanmarinoBrandonOdiwuoralfonsoromanzachow101
Labels
Tests
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 03:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me