test: remove duplicated ban test

[brunoerg]

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.

bitcoin/test/functional/p2p_disconnect_ban.py

Lines 74 to 110 in bf1b638

	self.log.info("setban: test persistence across node restart")
	# 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
	self.nodes[1].setban("2001:4d48:ac57:400:cacf:e9ff:fe1d:9c63/19", "add", 1000) # ban for 1000 seconds
	listBeforeShutdown = self.nodes[1].listbanned()
	assert_equal("192.168.0.1/32", listBeforeShutdown[2]['address'])
	self.log.info("setban: test banning with absolute timestamp")
	self.nodes[1].setban("192.168.0.2", "add", old_time + 120, True)
	# Move time forward by 3 seconds so the third ban has expired
	self.nodes[1].setmocktime(old_time + 3)
	assert_equal(len(self.nodes[1].listbanned()), 4)
	self.log.info("Test ban_duration and time_remaining")
	for ban in self.nodes[1].listbanned():
	if ban["address"] in ["127.0.0.0/32", "127.0.0.0/24"]:
	assert_equal(ban["ban_duration"], 86400)
	assert_equal(ban["time_remaining"], 86397)
	elif ban["address"] == "2001:4d48:ac57:400:cacf:e9ff:fe1d:9c63/19":
	assert_equal(ban["ban_duration"], 1000)
	assert_equal(ban["time_remaining"], 997)
	elif ban["address"] == "192.168.0.2/32":
	assert_equal(ban["ban_duration"], 120)
	assert_equal(ban["time_remaining"], 117)
	self.restart_node(1)
	listAfterShutdown = self.nodes[1].listbanned()
	assert_equal("127.0.0.0/24", listAfterShutdown[0]['address'])
	assert_equal("127.0.0.0/32", listAfterShutdown[1]['address'])
	assert_equal("192.168.0.2/32", listAfterShutdown[2]['address'])
	assert_equal("/19" in listAfterShutdown[3]['address'], True)

[DrahtBot]

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

Code Coverage

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

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.

[tdb3]

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:

bitcoin/test/functional/rpc_setban.py

Lines 56 to 61 in 9cbfb82

	self.log.info("Test that a non-IP address can be banned/unbanned")
	node = self.nodes[1]
	tor_addr = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"
	ip_addr = "1.2.3.4"
	assert not self.is_banned(node, tor_addr)
	assert not self.is_banned(node, ip_addr)

[brunoerg]

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.

[tdb3]

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

[brunoerg]

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

[tdb3]

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

[brunoerg]

Force-pushed addressing #29688 (comment)

[tdb3]

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

[hernanmarino]

tested ACK e30e862

[BrandonOdiwuor]

ACK e30e862

Looks good to me

[alfonsoromanz]

ACK e30e862

[achow101]

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?

[brunoerg]

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.

[achow101]

ACK e30e862