test: check ban_duration and time_remaining after setting ban #23879

pull brunoerg wants to merge 1 commits into bitcoin:master from brunoerg:2021-12-test-ban_duration-time_remaining changing 1 files +9 −0

brunoerg commented at 4:02 PM on December 27, 2021: member

This PR adds functional test coverage for ban_duration and time_remaining introduced in #21602
test: check ban_duration and time_remaining after setting ban da349f131a
DrahtBot added the label Tests on Dec 27, 2021
shaavan approved
shaavan commented at 1:51 PM on December 28, 2021: contributor
ACK da349f131a57640340f32c62c5d9b06a415d74ec

The added test logically makes sense.

Summary:
- The test checks for the ban_duration and time_remaining for the subnets banned in the above code.
- time_remaining for each of them should be 3 seconds less than their ban_duration as mocktime has been moved up by 3 seconds in the above code.
```
self.nodes[1].setmocktime(old_time + 3)
```
I tested that the test passed successfully on the PR branch.
theStack approved
theStack commented at 9:06 PM on December 29, 2021: member

Tested ACK da349f131a57640340f32c62c5d9b06a415d74ec

For historical reference, these fields (ban_duration, time_remaining) were introduced in PR #21602, which also included unit tests (commit 60290d3f5ec8e7e3b8cb1ebae02d5d72f6005184). Can't hurt to also check these in the functional tests!
MarcoFalke merged this on Dec 30, 2021
MarcoFalke closed this on Dec 30, 2021
sidhujag referenced this in commit 0acb5b4d02 on Dec 30, 2021

in test/functional/p2p_disconnect_ban.py:77 in da349f131a

  69 | @@ -70,6 +70,15 @@ def run_test(self):
  70 |          self.nodes[1].setmocktime(old_time + 3)
  71 |          assert_equal(len(self.nodes[1].listbanned()), 3)
  72 |  
  73 | +        self.log.info("Test ban_duration and time_remaining")
  74 | +        for ban in self.nodes[1].listbanned():
  75 | +            if ban["address"] in ["127.0.0.0/32", "127.0.0.0/24"]:
  76 | +                assert_equal(ban["ban_duration"], 86400)
  77 | +                assert_equal(ban["time_remaining"], 86397)

MarcoFalke commented at 2:17 PM on January 6, 2022:

 test  2022-01-06T13:54:00.230000Z TestFramework (ERROR): Assertion failed 
                                   Traceback (most recent call last):
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
                                       self.run_test()
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_disconnect_ban.py", line 77, in run_test
                                       assert_equal(ban["time_remaining"], 86397)
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 50, in assert_equal
                                       raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
                                   AssertionError: not(86396 == 86397)

https://cirrus-ci.com/task/6754020390862848

MarcoFalke commented at 2:17 PM on January 6, 2022:

This is because the ban was set without mocktime

brunoerg commented at 2:53 PM on January 6, 2022:

# Set the mocktime so we can control when bans expire
old_time = int(time.time())
self.nodes[1].setmocktime(old_time)
self.nodes[1].setban("127.0.0.0/32", "add")
self.nodes[1].setban("127.0.0.0/24", "add")
self.nodes[1].setban("192.168.0.1", "add", 1)  # ban for 1 seconds

instead of

self.nodes[1].setban("127.0.0.0/32", "add")
self.nodes[1].setban("127.0.0.0/24", "add")
# Set the mocktime so we can control when bans expire
old_time = int(time.time())
self.nodes[1].setmocktime(old_time)
self.nodes[1].setban("192.168.0.1", "add", 1)  # ban for 1 seconds

could solve it?

vasild commented at 9:18 AM on January 7, 2022:

Fixed in https://github.com/bitcoin/bitcoin/pull/23996

DrahtBot locked this on Jan 7, 2023

Contributors

Labels

Linked (view graph)

#21602 rpc: add additional ban time fields to listbanned #23988 p2p_disconnect_ban.py issue