test: test banlist database recreation #26794

pull brunoerg wants to merge 1 commits into bitcoin:master from brunoerg:2023-01-test-banlist-database-recreation changing 1 files +12 −0

brunoerg commented at 6:51 PM on January 2, 2023: contributor

This PR adds test coverage for 'banlist database recreation'. If it wasn't able to read ban db (in LoadBanlist), so it should create a new (an empty, ofc) one. https://github.com/bitcoin/bitcoin/blob/d8bdee0fc889def7c5f5076da13db4fce0a3728a/src/banman.cpp#L28-L45
DrahtBot commented at 6:51 PM on January 2, 2023: contributor


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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK MarcoFalke

Concept ACK kevkevinpal

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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #26863 (test: merge banning test from p2p_disconnect_ban to rpc_setban by brunoerg)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot added the label Tests on Jan 2, 2023
kevkevinpal commented at 11:51 PM on January 2, 2023: contributor

utACK, would we also want to test that the recreated db can properly add and read a record to the db?
in test/functional/p2p_disconnect_ban.py:7 in 3d6f2ee39a outdated
```
   3 | @@ -4,6 +4,7 @@
   4 |  # file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 |  """Test node disconnect and ban behavior"""
   6 |  import time
   7 | +import os
```
maflcko commented at 8:22 AM on January 3, 2023:

Would be nice to use the Path interface for new code, given that chain_path is already Path?

https://docs.python.org/3/library/pathlib.html#correspondence-to-tools-in-the-os-module

brunoerg commented at 11:50 AM on January 3, 2023:

Sounds good, gonna address it!
brunoerg force-pushed on Jan 3, 2023
brunoerg commented at 12:53 PM on January 3, 2023: contributor

Force-pushed addressing @MarcoFalke's review. Replaced os stuff for Path
vasild commented at 4:34 PM on January 4, 2023: contributor

The test looks correct, but maybe p2p_disconnect_ban.py - "Test node disconnect and ban behavior" is not the best place to add it. To me it seems that this is more appropriate for a unit test by extending src/test/banman_tests.cpp.
brunoerg force-pushed on Jan 5, 2023
test: test banlist database recreation 4bdcf57158
brunoerg force-pushed on Jan 5, 2023
brunoerg commented at 8:52 PM on January 5, 2023: contributor

would we also want to test that the recreated db can properly add and read a record to the db? @kevkevinpal Sounds good, I just force-pushed reordering the code to leave the great part of the ban test after the recreation.

The test looks correct, but maybe p2p_disconnect_ban.py - "Test node disconnect and ban behavior" is not the best place to add it. To me it seems that this is more appropriate for a unit test by extending src/test/banman_tests.cpp. @vasild I'm thinking about it, make sense. I put it in a functional test because here we can test the behavior of the whole process of starting/stopping the node and check how it deals recreating the db as well as see additions to the db after the recreation.
achow101 requested review from maflcko on Apr 25, 2023
maflcko commented at 2:23 PM on April 26, 2023: member

lgtm ACK 4bdcf571584cbe44ad5533a3c991d3b0b4b2c84f
DrahtBot removed review request from maflcko on Apr 26, 2023
maflcko assigned fanquake on Apr 27, 2023
fanquake merged this on Apr 27, 2023
fanquake closed this on Apr 27, 2023
sidhujag referenced this in commit 5e2ffb065a on Apr 28, 2023
bitcoin locked this on Apr 26, 2024

Type	Reviewers
ACK	MarcoFalke
Concept ACK	kevkevinpal

Contributors

Labels

test: test banlist database recreation #26794

Reviews

Conflicts