From 89b84ea91ae40876a52879c509c63d0bacbfaade Mon Sep 17 00:00:00 2001 From: 0xb10c Date: Tue, 12 Mar 2024 16:17:01 +0100 Subject: [PATCH 1/3] test: check that addrman seeding is successful The addpeeraddress calls can fail due to collisions. As we are using a deteministic addrman, they won't fail with the current bucket/position calculation. However, if the calculation is changed, they might collide and fail silently causing tests using `seed_addrman()` to fail. Assert that the addpeeraddress calls are successful. --- test/functional/rpc_net.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 48d86ab59dd..2701d2471d0 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -45,14 +45,18 @@ def seed_addrman(node): """ Populate the addrman with addresses from different networks. Here 2 ipv4, 2 ipv6, 1 cjdns, 2 onion and 1 i2p addresses are added. """ - node.addpeeraddress(address="1.2.3.4", tried=True, port=8333) - node.addpeeraddress(address="2.0.0.0", port=8333) - node.addpeeraddress(address="1233:3432:2434:2343:3234:2345:6546:4534", tried=True, port=8333) - node.addpeeraddress(address="2803:0:1234:abcd::1", port=45324) - node.addpeeraddress(address="fc00:1:2:3:4:5:6:7", port=8333) - node.addpeeraddress(address="pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion", tried=True, port=8333) - node.addpeeraddress(address="nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion", port=45324, tried=True) - node.addpeeraddress(address="c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p", port=8333) + # These addresses currently don't collide with a deterministic addrman. + # If the addrman positioning/bucketing is changed, these might collide + # and adding them fails. + success = { "success": True } + assert_equal(node.addpeeraddress(address="1.2.3.4", tried=True, port=8333), success) + assert_equal(node.addpeeraddress(address="2.0.0.0", port=8333), success) + assert_equal(node.addpeeraddress(address="1233:3432:2434:2343:3234:2345:6546:4534", tried=True, port=8333), success) + assert_equal(node.addpeeraddress(address="2803:0:1234:abcd::1", port=45324), success) + assert_equal(node.addpeeraddress(address="fc00:1:2:3:4:5:6:7", port=8333), success) + assert_equal(node.addpeeraddress(address="pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion", tried=True, port=8333), success) + assert_equal(node.addpeeraddress(address="nrfj6inpyf73gpkyool35hcmne5zwfmse3jl3aw23vk7chdemalyaqad.onion", port=45324, tried=True), success) + assert_equal(node.addpeeraddress(address="c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p", port=8333), success) class NetTest(BitcoinTestFramework): From 3047c3e3a99112c38f118034daa672db70fa4a60 Mon Sep 17 00:00:00 2001 From: 0xb10c Date: Tue, 12 Mar 2024 16:26:42 +0100 Subject: [PATCH 2/3] addrman: drop /*deterministic=*/ comment Just having deterministic is enough. See https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1488241966 --- src/addrdb.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index f8d4240f3fe..14dc314c365 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -195,7 +195,7 @@ util::Result> LoadAddrman(const NetGroupManager& netgro auto check_addrman = std::clamp(args.GetIntArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000); bool deterministic = HasTestOption(args, "addrman"); // use a deterministic addrman only for tests - auto addrman{std::make_unique(netgroupman, /*deterministic=*/deterministic, /*consistency_check_ratio=*/check_addrman)}; + auto addrman{std::make_unique(netgroupman, deterministic, /*consistency_check_ratio=*/check_addrman)}; const auto start{SteadyClock::now()}; const auto path_addr{args.GetDataDirNet() / "peers.dat"}; @@ -204,7 +204,7 @@ util::Result> LoadAddrman(const NetGroupManager& netgro LogPrintf("Loaded %i addresses from peers.dat %dms\n", addrman->Size(), Ticks(SteadyClock::now() - start)); } catch (const DbNotFoundError&) { // Addrman can be in an inconsistent state after failure, reset it - addrman = std::make_unique(netgroupman, /*deterministic=*/deterministic, /*consistency_check_ratio=*/check_addrman); + addrman = std::make_unique(netgroupman, deterministic, /*consistency_check_ratio=*/check_addrman); LogPrintf("Creating peers.dat because the file was not found (%s)\n", fs::quoted(fs::PathToString(path_addr))); DumpPeerAddresses(args, *addrman); } catch (const InvalidAddrManVersionError&) { @@ -212,7 +212,7 @@ util::Result> LoadAddrman(const NetGroupManager& netgro return util::Error{strprintf(_("Failed to rename invalid peers.dat file. Please move or delete it and try again."))}; } // Addrman can be in an inconsistent state after failure, reset it - addrman = std::make_unique(netgroupman, /*deterministic=*/deterministic, /*consistency_check_ratio=*/check_addrman); + addrman = std::make_unique(netgroupman, deterministic, /*consistency_check_ratio=*/check_addrman); LogPrintf("Creating new peers.dat because the file version was not compatible (%s). Original backed up to peers.dat.bak\n", fs::quoted(fs::PathToString(path_addr))); DumpPeerAddresses(args, *addrman); } catch (const std::exception& e) { From 9a44a20fb790f3be5d5d5d8f5d0f48aac633b2a4 Mon Sep 17 00:00:00 2001 From: 0xb10c Date: Tue, 12 Mar 2024 16:31:39 +0100 Subject: [PATCH 3/3] init: clarify -test error See https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1469388717 --- src/init.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index 349d4ff1abc..885c0673ddb 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1031,7 +1031,7 @@ bool AppInitParameterInteraction(const ArgsManager& args) if (args.IsArgSet("-test")) { if (chainparams.GetChainType() != ChainType::REGTEST) { - return InitError(Untranslated("-test=