From 709dc77a7053c6fff1503e92e8b3cdd1d369f355 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 17 Jun 2024 14:54:08 -0400 Subject: [PATCH 1/5] Merge bitcoin/bitcoin#30291: test: write functional test results to csv MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ad06e68399da71c615db0dbf5304d0cd46bc1f40 test: write functional test results to csv (tdb3) Pull request description: Adds argument `--resultsfile` to test_runner.py. Enables functional test results to be written to a (csv) file for processing by other applications (or for historical archiving). Test name, status, and duration are written to the file provided with the argument. Since `test_runner.py` is being touched, also fixes a misspelling (linter warning). Can split into its own commit if desired. #### Notes - Total runtime of functional tests has seemed to have increased on my development machines over the past few months (more tests added, individual test runtime increase, etc.). Was interested in recording test runtime data over time to detect trends. Initially searched `doc/benchmarking.md`, existing PRs, and Issues, but didn't immediately see this type of capability or alternate solutions (please chime in if you know of one!). Thought it would be beneficial to add this capability to `test_runner` to facilitate this type of data analysis (and potentially other use cases) - Saw https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#benchmarking-with-perf, and this PR's higher level data seems complimentary. - Was on the fence as to whether to expand `print_results()` (i.e. take advantage of the same loop over `test_results`) or implement in a separate `write_results()` function. Decided on the latter for now, but interested in reviewers' thoughts. #### Example 1: all tests pass ``` $ test/functional/test_runner.py --resultsfile functional_test_results.csv --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp feature_blocksdir wallet_startup feature_config_args mempool_accept Temporary test directory at /mnt/tmp/test_runner_₿_🏃_20240614_201625 Test results will be written to functional_test_results.csv ... $ cat functional_test_results.csv test,status,duration(seconds) feature_blocksdir.py,Passed,1 feature_config_args.py,Passed,29 mempool_accept.py,Passed,9 wallet_startup.py,Passed,2 ALL,Passed,29 ``` #### Example 2: one test failure ``` $ cat functional_test_results.csv test,status,duration(seconds) feature_blocksdir.py,Passed,1 feature_config_args.py,Passed,28 wallet_startup.py,Passed,2 mempool_accept.py,Failed,1 ALL,Failed,28 ``` ACKs for top commit: maflcko: re-ACK ad06e68399da71c615db0dbf5304d0cd46bc1f40 kevkevinpal: tACK [ad06e68](https://github.com/bitcoin/bitcoin/pull/30291/commits/ad06e68399da71c615db0dbf5304d0cd46bc1f40) achow101: ACK ad06e68399da71c615db0dbf5304d0cd46bc1f40 rkrux: tACK [ad06e68](https://github.com/bitcoin/bitcoin/pull/30291/commits/ad06e68399da71c615db0dbf5304d0cd46bc1f40) brunoerg: ACK ad06e68399da71c615db0dbf5304d0cd46bc1f40 marcofleon: Good idea, tested ACK ad06e68399da71c615db0dbf5304d0cd46bc1f40 Tree-SHA512: 561194406cc744905518aa5ac6850c07c4aaecdaf5d4d8b250671b6e90093d4fc458f050e8a85374e66359cc0e0eaceba5eb24092c55f0d8f349d744a32ef76c --- test/functional/test_runner.py | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 026ed289447c..c7d735dc3397 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -15,8 +15,10 @@ import argparse from collections import deque import configparser +import csv import datetime import os +import pathlib import time import shutil import signal @@ -448,6 +450,7 @@ def main(): parser.add_argument('--failfast', '-F', action='store_true', help='stop execution after the first test failure') parser.add_argument('--filter', help='filter scripts to run by regular expression') parser.add_argument('--skipunit', '-u', action='store_true', help='skip unit tests for the test framework') + parser.add_argument('--resultsfile', '-r', help='store test results (as CSV) to the provided file') args, unknown_args = parser.parse_known_args() @@ -480,6 +483,13 @@ def main(): logging.debug("Temporary test directory at %s" % tmpdir) + results_filepath = None + if args.resultsfile: + results_filepath = pathlib.Path(args.resultsfile) + # Stop early if the parent directory doesn't exist + assert results_filepath.parent.exists(), "Results file parent directory does not exist" + logging.debug("Test results will be written to " + str(results_filepath)) + enable_bitcoind = config["components"].getboolean("ENABLE_BITCOIND") if not enable_bitcoind: @@ -561,9 +571,10 @@ def main(): failfast=args.failfast, use_term_control=args.ansi, skipunit=args.skipunit, + results_filepath=results_filepath, ) -def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, attempts=1, enable_coverage=False, args=None, combined_logs_len=0, failfast=False, use_term_control, skipunit=False): +def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, attempts=1, enable_coverage=False, args=None, combined_logs_len=0, failfast=False, use_term_control, skipunit=False, results_filepath=None): args = args or [] # Warn if dashd is already running @@ -659,7 +670,10 @@ def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, attempts=1, enab logging.debug("Early exiting after test failure") break - print_results(test_results, max_len_name, (int(time.time() - start_time))) + runtime = int(time.time() - start_time) + print_results(test_results, max_len_name, runtime) + if results_filepath: + write_results(test_results, results_filepath, runtime) if coverage: coverage_passed = coverage.report_rpc_coverage() @@ -706,6 +720,17 @@ def print_results(test_results, max_len_name, runtime): results += "Runtime: %s s\n" % (runtime) print(results) + +def write_results(test_results, filepath, total_runtime): + with open(filepath, mode="w", encoding="utf8") as results_file: + results_writer = csv.writer(results_file) + results_writer.writerow(['test', 'status', 'duration(seconds)']) + all_passed = True + for test_result in test_results: + all_passed = all_passed and test_result.was_successful + results_writer.writerow([test_result.name, test_result.status, str(test_result.time)]) + results_writer.writerow(['ALL', ("Passed" if all_passed else "Failed"), str(total_runtime)]) + class TestHandler: """ Trigger the test scripts passed in via the list. From 757ae1fa272d54dcf6f490b870ddf0b9de4795ac Mon Sep 17 00:00:00 2001 From: merge-script Date: Thu, 11 Jul 2024 18:37:19 +0100 Subject: [PATCH 2/5] Merge bitcoin/bitcoin#30397: refactor: Use designated initializer in test/util/net.cpp e233ec036dc972a5847ab769ad22d418fd9404d1 refactor: Use designated initializer (Hodlinator) Pull request description: Block was recently touched (e2d1f84858485650ff743753ffa5c679f210a992) and the codebase recently switched to C++20 which allows this to improve robustness. Follow-up suggested in https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664818014 ACKs for top commit: maflcko: ACK e233ec036dc972a5847ab769ad22d418fd9404d1 Tree-SHA512: ce3a18f513421e923710a43c8f97db1badb7ff5c6bdbfd62d9543312d2225731db5c14bef16feb47c43b84fad4dc24485086634b680feba422d2b7b363e13fa6 --- src/test/util/net.cpp | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 963abca9631f..b271bed147b8 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -118,20 +118,20 @@ std::vector GetRandomNodeEvictionCandidates(int n_candida std::vector candidates; for (int id = 0; id < n_candidates; ++id) { candidates.push_back({ - /*id=*/id, - /*m_connected=*/std::chrono::seconds{random_context.randrange(100)}, - /*m_min_ping_time=*/std::chrono::microseconds{random_context.randrange(100)}, - /*m_last_block_time=*/std::chrono::seconds{random_context.randrange(100)}, - /*m_last_tx_time=*/std::chrono::seconds{random_context.randrange(100)}, - /*fRelevantServices=*/random_context.randbool(), - /*m_relay_txs=*/random_context.randbool(), - /*fBloomFilter=*/random_context.randbool(), - /*nKeyedNetGroup=*/random_context.randrange(100), - /*prefer_evict=*/random_context.randbool(), - /*m_is_local=*/random_context.randbool(), - /*m_network=*/ALL_NETWORKS[random_context.randrange(ALL_NETWORKS.size())], - /*m_noban=*/false, - /*m_conn_type=*/ConnectionType::INBOUND, + .id=id, + .m_connected=std::chrono::seconds{random_context.randrange(100)}, + .m_min_ping_time=std::chrono::microseconds{random_context.randrange(100)}, + .m_last_block_time=std::chrono::seconds{random_context.randrange(100)}, + .m_last_tx_time=std::chrono::seconds{random_context.randrange(100)}, + .fRelevantServices=random_context.randbool(), + .m_relay_txs=random_context.randbool(), + .fBloomFilter=random_context.randbool(), + .nKeyedNetGroup=random_context.randrange(100), + .prefer_evict=random_context.randbool(), + .m_is_local=random_context.randbool(), + .m_network=ALL_NETWORKS[random_context.randrange(ALL_NETWORKS.size())], + .m_noban=false, + .m_conn_type=ConnectionType::INBOUND, }); } return candidates; From feaedf680a1d475603c102040e7828efd00394fb Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 16 Jul 2024 16:27:24 -0400 Subject: [PATCH 3/5] Merge bitcoin/bitcoin#22729: Make it possible to disable Tor binds and abort startup on bind failure bca346a97056748f1e3fb899f336d56d9fd45a64 net: require P2P binds to succeed (Vasil Dimov) af552534ab83c572d3bc3f93ccaee5c1961ccab5 net: report an error if unable to bind on the Tor listening addr:port (Vasil Dimov) 9a7e5f4d68dadc64a675f32d1e91199d6b1aa095 net: don't extra bind for Tor if binds are restricted (Vasil Dimov) Pull request description: Make it possible to disable the Tor binding on `127.0.0.1:8334` and stop startup if any P2P bind fails instead of "if all P2P binds fail". Fixes https://github.com/bitcoin/bitcoin/issues/22726 Fixes https://github.com/bitcoin/bitcoin/issues/22727 ACKs for top commit: achow101: ACK bca346a97056748f1e3fb899f336d56d9fd45a64 cbergqvist: ACK bca346a97056748f1e3fb899f336d56d9fd45a64 pinheadmz: ACK bca346a97056748f1e3fb899f336d56d9fd45a64 Tree-SHA512: fabef89a957191eea4f3e3b6109d2b8389f27ecc74440a920b0c10f31fff00a85bcfd1eb3c91826c7169c618f4de8a8d0a260e2caf40fd854f07ea9a980d8603 --- src/init.cpp | 2 ++ src/net.cpp | 28 +++++++++++++++------ test/functional/feature_bind_extra.py | 23 ++++++++++------- test/functional/test-shell.md | 2 +- test/functional/test_framework/test_node.py | 15 +++++++++++ test/functional/test_framework/util.py | 10 +++++--- 6 files changed, 59 insertions(+), 21 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 113bd4c9f025..7dd8548fe53a 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -2653,6 +2653,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) CService onion_service_target; if (!connOptions.onion_binds.empty()) { onion_service_target = connOptions.onion_binds.front(); + } else if (!connOptions.vBinds.empty()) { + onion_service_target = connOptions.vBinds.front(); } else { onion_service_target = DefaultOnionServiceTarget(); connOptions.onion_binds.push_back(onion_service_target); diff --git a/src/net.cpp b/src/net.cpp index 21648dbd01c5..f2f016678469 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3942,24 +3942,36 @@ bool CConnman::Bind(const CService& addr_, unsigned int flags, NetPermissionFlag bool CConnman::InitBinds(const Options& options) { - bool fBound = false; for (const auto& addrBind : options.vBinds) { - fBound |= Bind(addrBind, BF_REPORT_ERROR, NetPermissionFlags::None); + if (!Bind(addrBind, BF_REPORT_ERROR, NetPermissionFlags::None)) { + return false; + } } for (const auto& addrBind : options.vWhiteBinds) { - fBound |= Bind(addrBind.m_service, BF_REPORT_ERROR, addrBind.m_flags); + if (!Bind(addrBind.m_service, BF_REPORT_ERROR, addrBind.m_flags)) { + return false; + } } for (const auto& addr_bind : options.onion_binds) { - fBound |= Bind(addr_bind, BF_DONT_ADVERTISE, NetPermissionFlags::None); + if (!Bind(addr_bind, BF_REPORT_ERROR | BF_DONT_ADVERTISE, NetPermissionFlags::None)) { + return false; + } } if (options.bind_on_any) { + // Don't consider errors to bind on IPv6 "::" fatal because the host OS + // may not have IPv6 support and the user did not explicitly ask us to + // bind on that. + const CService ipv6_any{in6_addr(IN6ADDR_ANY_INIT), GetListenPort()}; // :: + Bind(ipv6_any, BF_NONE, NetPermissionFlags::None); + struct in_addr inaddr_any; inaddr_any.s_addr = htonl(INADDR_ANY); - struct in6_addr inaddr6_any = IN6ADDR_ANY_INIT; - fBound |= Bind(CService(inaddr6_any, GetListenPort()), BF_NONE, NetPermissionFlags::None); - fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE, NetPermissionFlags::None); + const CService ipv4_any{inaddr_any, GetListenPort()}; // 0.0.0.0 + if (!Bind(ipv4_any, BF_REPORT_ERROR, NetPermissionFlags::None)) { + return false; + } } - return fBound; + return true; } bool CConnman::Start(CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman, CMasternodeSync& mn_sync, diff --git a/test/functional/feature_bind_extra.py b/test/functional/feature_bind_extra.py index 5de9ff203cc2..994fbbf0c27a 100755 --- a/test/functional/feature_bind_extra.py +++ b/test/functional/feature_bind_extra.py @@ -30,7 +30,7 @@ def set_test_params(self): # Avoid any -bind= on the command line. Force the framework to avoid # adding -bind=127.0.0.1. self.bind_to_localhost_only = False - self.num_nodes = 2 + self.num_nodes = 3 def setup_network(self): # Due to OS-specific network stats queries, we only run on Linux. @@ -64,14 +64,21 @@ def setup_network(self): ) port += 2 + # Node2, no -bind=...=onion, thus no extra port for Tor target. + self.expected.append( + [ + [f"-bind=127.0.0.1:{port}"], + [(loopback_ipv4, port)] + ], + ) + port += 1 + self.extra_args = list(map(lambda e: e[0], self.expected)) - self.add_nodes(self.num_nodes, self.extra_args) - # Don't start the nodes, as some of them would collide trying to bind on the same port. + self.setup_nodes() def run_test(self): - for i in range(len(self.expected)): - self.log.info(f"Starting node {i} with {self.expected[i][0]}") - self.start_node(i) + for i, (args, expected_services) in enumerate(self.expected): + self.log.info(f"Checking listening ports of node {i} with {args}") pid = self.nodes[i].process.pid binds = set(get_bind_addrs(pid)) # Remove IPv6 addresses because on some CI environments "::1" is not configured @@ -82,9 +89,7 @@ def run_test(self): binds = set(filter(lambda e: len(e[0]) != ipv6_addr_len_bytes, binds)) # Remove RPC ports. They are not relevant for this test. binds = set(filter(lambda e: e[1] != rpc_port(i), binds)) - assert_equal(binds, set(self.expected[i][1])) - self.stop_node(i) - self.log.info(f"Stopped node {i}") + assert_equal(binds, set(expected_services)) if __name__ == '__main__': BindExtraTest().main() diff --git a/test/functional/test-shell.md b/test/functional/test-shell.md index b89b40f13dd2..4b968296d5c2 100644 --- a/test/functional/test-shell.md +++ b/test/functional/test-shell.md @@ -169,7 +169,7 @@ can be called after the TestShell is shut down. | Test parameter key | Default Value | Description | |---|---|---| -| `bind_to_localhost_only` | `True` | Binds bitcoind RPC services to `127.0.0.1` if set to `True`.| +| `bind_to_localhost_only` | `True` | Binds bitcoind P2P services to `127.0.0.1` if set to `True`.| | `cachedir` | `"/path/to/bitcoin/test/cache"` | Sets the bitcoind datadir directory. | | `chain` | `"regtest"` | Sets the chain-type for the underlying test bitcoind processes. | | `configfile` | `"/path/to/bitcoin/test/config.ini"` | Sets the location of the test framework config file. | diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index cbe42cd1de88..db76cf5b699b 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -40,6 +40,7 @@ wait_until_helper, p2p_port, get_chain_folder, + tor_port, ) BITCOIND_PROC_WAIT_TIMEOUT = 60 @@ -90,8 +91,11 @@ def __init__(self, i, datadir, extra_args_from_options, *, chain, rpchost, timew self.cwd = cwd self.mocktime = mocktime self.descriptors = descriptors + self.has_explicit_bind = False if extra_conf is not None: append_config(datadir, extra_conf) + # Remember if there is bind=... in the config file. + self.has_explicit_bind = any(e.startswith("bind=") for e in extra_conf) # Most callers will just need to add extra args to the standard list below. # For those callers that need more flexibity, they can just set the args property directly. # Note that common args are set in the config file (see initialize_datadir) @@ -227,6 +231,17 @@ def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, **kwargs if extra_args is None: extra_args = self.extra_args + # If listening and no -bind is given, then bitcoind would bind P2P ports on + # 0.0.0.0:P and 127.0.0.1:18445 (for incoming Tor connections), where P is + # a unique port chosen by the test framework and configured as port=P in + # bitcoin.conf. To avoid collisions on 127.0.0.1:18445, change it to + # 127.0.0.1:tor_port(). + will_listen = all(e != "-nolisten" and e != "-listen=0" for e in extra_args) + has_explicit_bind = self.has_explicit_bind or any(e.startswith("-bind=") for e in extra_args) + if will_listen and not has_explicit_bind: + extra_args.append(f"-bind=0.0.0.0:{p2p_port(self.index)}") + extra_args.append(f"-bind=127.0.0.1:{tor_port(self.index)}=onion") + self.use_v2transport = "-v2transport=1" in extra_args or (self.default_to_v2 and "-v2transport=0" not in extra_args) # Add a new stdout and stderr file each time dashd is started diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 7cdcbc8983d8..144a3b9993da 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -320,9 +320,9 @@ def random_bytes(n): # The maximum number of nodes a single test can spawn MAX_NODES = 20 -# Don't assign rpc or p2p ports lower than this +# Don't assign p2p, rpc or tor ports lower than this PORT_MIN = int(os.getenv('TEST_RUNNER_PORT_MIN', default=11000)) -# The number of ports to "reserve" for p2p and rpc, each +# The number of ports to "reserve" for p2p, rpc and tor, each PORT_RANGE = 10000 @@ -362,7 +362,11 @@ def p2p_port(n): def rpc_port(n): - return PORT_MIN + PORT_RANGE + n + (MAX_NODES * PortSeed.n) % (PORT_RANGE - 1 - MAX_NODES) + return p2p_port(n) + PORT_RANGE + + +def tor_port(n): + return p2p_port(n) + PORT_RANGE * 2 def rpc_url(datadir, i, chain, rpchost=None): From 5077d4b8865fb0d15afcac48cf83d86b1a027c17 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 12 Mar 2024 18:43:13 +0000 Subject: [PATCH 4/5] Merge bitcoin/bitcoin#29633: log: Remove error() reference d0e6564240857994db53d06f66ea09da0edbaf0f log: Remove error() reference (Fabian Jahr) Pull request description: Mini-followup to #29236 that was just merged. Removes a reference to `error()` that was missed in a comment. ACKs for top commit: ryanofsky: Code review ACK d0e6564240857994db53d06f66ea09da0edbaf0f. Just dropped LogPrintf reference since last review stickies-v: ACK d0e6564240857994db53d06f66ea09da0edbaf0f Empact: ACK https://github.com/bitcoin/bitcoin/pull/29633/commits/d0e6564240857994db53d06f66ea09da0edbaf0f Tree-SHA512: 8abe4895951013c2ceca9a57743aacabaf8af831d07eee9ae8372c121c16e88b7226f0e537200c3464792e19ac7e03b57ba0be31f43add8802753972b0aefc48 --- src/logging.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/logging.h b/src/logging.h index d0b2069319f0..cc37a65a8449 100644 --- a/src/logging.h +++ b/src/logging.h @@ -257,7 +257,7 @@ std::string SafeStringFormat(const std::string& fmt, const Args&... args) } } -// Be conservative when using LogPrintf/error or other things which +// Be conservative when using functions that // unconditionally log to debug.log! It should not be the case that an inbound // peer can fill up a user's disk with debug.log entries. From 314e0a3b8195bd31d5df8728c3f244469ecf3b71 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 14 Mar 2024 11:14:56 +0000 Subject: [PATCH 5/5] Merge bitcoin/bitcoin#29459: test: check_mempool_result negative feerate bf264e05981e3809715f34f548138d53991db6f2 test: check_mempool_result negative feerate (kevkevin) Pull request description: Adds test coverage in `mempool_accept.py` to check if a negative `maxfeerate` is input into `check_mempool_result` Asserts "Amount out of range" error message and `-3` error code Motivated by this [comment](https://github.com/bitcoin/bitcoin/pull/29434/files#r1491112250) ACKs for top commit: maflcko: lgtm ACK bf264e05981e3809715f34f548138d53991db6f2 brunoerg: nice, utACK bf264e05981e3809715f34f548138d53991db6f2 davidgumberg: Looks great, ACK https://github.com/bitcoin/bitcoin/pull/29459/commits/bf264e05981e3809715f34f548138d53991db6f2 Tree-SHA512: 58931b774cc887c616f2fd91af3ee65cc5db55acd8e2875c76de448c80bd4e020b057c5f4f85556431377f0d0e7553771fb285d1ec20cf64f64ec92a47776b78 --- test/functional/mempool_accept.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index f424f19edead..66c3f9e72ec6 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -77,6 +77,13 @@ def run_test(self): txid_in_block = self.wallet.sendrawtransaction(from_node=node, tx_hex=raw_tx_in_block) self.generate(node, 1) self.mempool_size = 0 + # Check negative feerate + assert_raises_rpc_error(-3, "Amount out of range", lambda: self.check_mempool_result( + result_expected=None, + rawtxs=[raw_tx_in_block], + maxfeerate=-0.01, + )) + # ... 0.99 passes self.check_mempool_result( result_expected=[{'txid': txid_in_block, 'allowed': False, 'reject-reason': 'txn-already-known'}], rawtxs=[raw_tx_in_block],