From 8f1cf1027ede8cd0372c0620a81d54ce5512f226 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 11 Mar 2024 10:13:04 -0400 Subject: [PATCH] Merge bitcoin/bitcoin#29007: test: create deterministic addrman in the functional tests 2cc8ca19f4185490f30a49516c890b2289fbab71 [test] Use deterministic addrman in addrman info tests (stratospher) a8978661093500df8d8dcf2a9c90837afacd0aab [test] Restart a node with empty addrman (stratospher) 71c19915c0c716d6f8a539dd92b8ad41e8c447ee [test] Use deterministic addrman in addpeeraddress test (stratospher) 7b868e6b678502e86571976d696c0e3cb72c0884 Revert "test: avoid non-determinism in asmap-addrman test" (stratospher) 69e091f3e1f65b12cf1804e7d39651f55a01827a [init] Create deterministic addrman in tests using -test=addrman (stratospher) be25ac3092b7755e26e1ec6c33a27cd0e3dd9eac [init] Remove -addrmantest command line arg (stratospher) 802e6e128bba5ffa6d4ec53ff45acccb7cb28f21 [init] Add new command line arg for use only in functional tests (stratospher) Pull request description: An address is placed in a `[bucket,position]` in the addrman table (new table or tried table) using the `addpeeraddress` RPC. This `[bucket,position]` is calculated using `nKey`(and other metrics) for the addrman which is chosen randomly during every run. Supposing there are 2 addresses to be placed in an addrman table. During every test run, a different `[bucket,position]` would be calculated for each address.These calculated `[bucket,position]` could even be the same for the 2 addresses in some test runs and result in collisions in the addrman. We wouldn't be able to predict when the collisions are going to happen because we can't predict the `nKey` value which is chosen at random. This can cause flaky tests. Because of these non deterministic collisions, we are limited in what we can do to test addrman functionality. Currently in our tests don't add a second address to prevent these collisions from happening - we only keep 1 address in the new table and 1 address in the tried table. See https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1091145647, https://github.com/bitcoin/bitcoin/pull/23084, [#22831(comment)](https://github.com/bitcoin/bitcoin/pull/22831/files#r708302639). This PR lets us create a deterministic addrman with fixed `nKey` so that we can know the `[bucket,position]` collisions beforehand, safely add more addresses in an addrman table and write more extensive tests. ACKs for top commit: amitiuttarwar: ACK 2cc8ca19f4185490f30a49516c890b2289fbab71 achow101: ACK 2cc8ca19f4185490f30a49516c890b2289fbab71 0xB10C: ACK 2cc8ca19f4185490f30a49516c890b2289fbab71 mzumsande: Code Review ACK 2cc8ca19f4185490f30a49516c890b2289fbab71 Tree-SHA512: 8acd9bdfe7de1eb44d22373bf13533d8ecf602df966fdd5b8b78afcd8cc35a286c95d2712f67a89473a0d68dded7d38f5599f6e4bf95a6589475444545bfb189 # Conflicts: # src/addrdb.cpp # src/common/args.cpp # src/init.cpp # src/net.cpp # test/functional/feature_asmap.py # test/functional/rpc_net.py # test/functional/test_framework/test_framework.py --- src/addrdb.cpp | 10 +- src/init.cpp | 31 +++ src/net.cpp | 3 +- src/util/system.cpp | 12 ++ src/util/system.h | 5 + test/functional/feature_addrman.py | 7 +- test/functional/feature_asmap.py | 18 +- test/functional/rpc_net.py | 176 +++++++++++++++++- .../test_framework/test_framework.py | 10 +- 9 files changed, 244 insertions(+), 28 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 83bc6192ff7b..97eea9a993fd 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -185,7 +185,9 @@ void ReadFromStream(AddrMan& addr, CDataStream& ssPeers) std::optional LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args, std::unique_ptr& addrman) { auto check_addrman = std::clamp(args.GetIntArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000); - addrman = std::make_unique(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman); + bool deterministic = HasTestOption(args, "addrman"); // use a deterministic addrman only for tests + + addrman = std::make_unique(netgroupman, /*deterministic=*/deterministic, /*consistency_check_ratio=*/check_addrman); const auto start{SteadyClock::now()}; const auto path_addr{gArgs.GetDataDirNet() / "peers.dat"}; @@ -194,7 +196,7 @@ std::optional LoadAddrman(const NetGroupManager& netgroupman, con 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=*/false, /*consistency_check_ratio=*/check_addrman); + addrman = std::make_unique(netgroupman, /*deterministic=*/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 DbInconsistentError& e) { @@ -203,7 +205,7 @@ std::optional LoadAddrman(const NetGroupManager& netgroupman, con // with frequent corruption, we are restoring old behaviour that does the same, silently. // // TODO: Evaluate cause and fix, revert this change at some point. - addrman = std::make_unique(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman); + addrman = std::make_unique(netgroupman, /*deterministic=*/deterministic, /*consistency_check_ratio=*/check_addrman); LogPrintf("Creating peers.dat because of invalid or corrupt file (%s)\n", e.what()); DumpPeerAddresses(args, *addrman); } catch (const InvalidAddrManVersionError&) { @@ -212,7 +214,7 @@ std::optional LoadAddrman(const NetGroupManager& netgroupman, con return 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=*/false, /*consistency_check_ratio=*/check_addrman); + addrman = std::make_unique(netgroupman, /*deterministic=*/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) { diff --git a/src/init.cpp b/src/init.cpp index e5bcc2470fb0..aba4ca8a522e 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -719,6 +719,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-stopafterblockimport", strprintf("Stop running after importing blocks from disk (default: %u)", DEFAULT_STOPAFTERBLOCKIMPORT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-stopatheight", strprintf("Stop running after reaching the given height in the main chain (default: %u)", DEFAULT_STOPATHEIGHT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-watchquorums=", strprintf("Watch and validate quorum communication (default: %u)", llmq::DEFAULT_WATCH_QUORUMS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-test=