Skip to content

test: test banlist database recreation #26794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
fanquake merged 1 commit into from
Apr 27, 2023
Merged

test: test banlist database recreation #26794

fanquake merged 1 commit into from
Apr 27, 2023

Conversation

@brunoerg
Copy link
Contributor

@brunoerg brunoerg commented Jan 2, 2023

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.

bitcoin/src/banman.cpp

Lines 28 to 45 in d8bdee0

void BanMan::LoadBanlist()
{
LOCK(m_cs_banned);
if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated);
const auto start{SteadyClock::now()};
if (m_ban_db.Read(m_banned)) {
SweepBanned(); // sweep out unused entries
LogPrint(BCLog::NET, "Loaded %d banned node addresses/subnets %dms\n", m_banned.size(),
Ticks<std::chrono::milliseconds>(SteadyClock::now() - start));
} else {
LogPrintf("Recreating the banlist database\n");
m_banned = {};
m_is_dirty = true;
}
}

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 2, 2023 •
edited
Loading

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 DrahtBot added the Tests label Jan 2, 2023
@kevkevinpal
Copy link
Contributor

kevkevinpal commented Jan 2, 2023

utACK, would we also want to test that the recreated db can properly add and read a record to the db?

maflcko
maflcko reviewed Jan 3, 2023
View reviewed changes
@brunoerg brunoerg force-pushed the 2023-01-test-banlist-database-recreation branch from 3d6f2ee to 869ab3f Compare January 3, 2023 12:52
@brunoerg
Copy link
Contributor Author

brunoerg commented Jan 3, 2023

Force-pushed addressing @MarcoFalke's review. Replaced os stuff for Path

vasild
vasild reviewed Jan 4, 2023
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 brunoerg force-pushed the 2023-01-test-banlist-database-recreation branch from 869ab3f to 7087241 Compare January 5, 2023 19:40
@brunoerg brunoerg force-pushed the 2023-01-test-banlist-database-recreation branch from 7087241 to 4bdcf57 Compare January 5, 2023 20:43
@brunoerg
Copy link
Contributor Author

brunoerg commented Jan 5, 2023

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.

@DrahtBot DrahtBot mentioned this pull request Jan 10, 2023
Closed
@achow101 achow101 requested a review from maflcko April 25, 2023 15:49
@maflcko
Copy link
Member

maflcko commented Apr 26, 2023

lgtm ACK 4bdcf57

@DrahtBot DrahtBot removed the request for review from maflcko April 26, 2023 14:23
@fanquake fanquake merged commit 03cb2fc into bitcoin:master Apr 27, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 28, 2023
@fanquake
Merge bitcoin#26794: test: test banlist database recreation
5e2ffb0 
4bdcf57 test: test banlist database recreation (brunoerg)

Pull request description:

  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

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 4bdcf57

Tree-SHA512: d9d0cd0c4b3797189dff00d3a634878188e7cf51e78346601fc97e2bf78c495561705214062bb42ab8e491e0d111f8bfcf74dbc801768bc02cf2b45f162aad85
@bitcoin bitcoin locked and limited conversation to collaborators Apr 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@vasild vasild vasild left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@fanquake fanquake

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@brunoerg @DrahtBot @kevkevinpal @maflcko @vasild @fanquake