From edbb656d7f8f40b6b331d4584fc3bc9dab004ce7 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 2 Feb 2024 12:02:17 -0500 Subject: [PATCH 1/3] Merge bitcoin/bitcoin#29361: refactor: Fix timedata includes fad0fafd5aca699cfab7673f8eb18211139aeb18 refactor: Fix timedata includes (MarcoFalke) Pull request description: Remove unused includes. Also, fixup comments, see https://github.com/bitcoin/bitcoin/pull/28956/files#r1464827885. Also, add missing includes to `chain.h` while touching it. ACKs for top commit: achow101: ACK fad0fafd5aca699cfab7673f8eb18211139aeb18 dergoegge: utACK fad0fafd5aca699cfab7673f8eb18211139aeb18 stickies-v: ACK fad0fafd5aca699cfab7673f8eb18211139aeb18 Tree-SHA512: 45e86f2eb90f0e37012bd83bf30259719e0e58ede18b31f51ca8a6f6d23e6ca4d060fc0f56f821a711cbdb45792b82cf780f5ae3226680d7a966471990f352bc --- src/chain.h | 7 ++++++- src/net_processing.cpp | 2 +- src/node/miner.cpp | 2 +- src/rpc/mining.cpp | 1 + src/timedata.h | 3 --- 5 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/chain.h b/src/chain.h index 4236e5a7adff..2f2de9243f5e 100644 --- a/src/chain.h +++ b/src/chain.h @@ -10,14 +10,19 @@ #include #include #include +#include #include #include +#include +#include +#include +#include #include /** * Maximum amount of time that a block timestamp is allowed to exceed the - * current network-adjusted time before the block will be accepted. + * current time before the block will be accepted. */ static constexpr int64_t MAX_FUTURE_BLOCK_TIME = 2 * 60 * 60; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0adbc77a685e..3af39f45ca0b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -36,6 +36,7 @@ #include #include #include +#include #include #include #include @@ -70,7 +71,6 @@ #include #include -#include #include #include #include diff --git a/src/node/miner.cpp b/src/node/miner.cpp index d93c320eb195..8b61f7284a8e 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -19,9 +19,9 @@ #include #include #include -#include #include #include +#include #include #include diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index ea13095ed37f..2a673a8f4293 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include diff --git a/src/timedata.h b/src/timedata.h index 3779486c1f88..0b6fcc883293 100644 --- a/src/timedata.h +++ b/src/timedata.h @@ -5,11 +5,8 @@ #ifndef BITCOIN_TIMEDATA_H #define BITCOIN_TIMEDATA_H -#include - #include #include -#include #include #include From a6978b2611f389f53b9ce2e8fdd53db4b390210e Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 11 Dec 2023 10:56:00 +0100 Subject: [PATCH 2/3] Merge bitcoin/bitcoin#29041: test: fix intermittent error in rpc_net.py (#29030) ea00f982d21aab51001d422225f00626a74db298 test: fix intermittent error in rpc_net.py (#29030) (Sebastian Falbesoner) Pull request description: Asserting for the debug log message "Added connection peer=" is insufficient for ensuring that this new connection will show up in a following getpeerinfo() call, as the debug message is written in the CNode ctor, which means it hasn't necessarily been added to CConnman.m_nodes at this point. Solve this by using the recently introduced `wait_for_new_peer` helper (see #29006, commit 00e0658e77f66103ebdeb29def99dc9f937c049d), which is more robust. Fixes #29030. ACKs for top commit: maflcko: lgtm ACK ea00f982d21aab51001d422225f00626a74db298 Tree-SHA512: dda307949a466fb3b24408a8c213d307e0af2155f2e8b4e52c836a22397f9d218bf9d8c54ca55bae62a96d7566f27167db9311dd8801785c327234783af5ed00 --- test/functional/rpc_net.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 5813ebc3dab5..a95722000f35 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -118,7 +118,7 @@ def test_getpeerinfo(self): self.log.info("Check getpeerinfo output before a version message was sent") no_version_peer_id = 3 no_version_peer_conntime = self.mocktime - with self.nodes[0].assert_debug_log([f"Added connection peer={no_version_peer_id}"]): + with self.nodes[0].wait_for_new_peer(): no_version_peer = self.nodes[0].add_p2p_connection(P2PInterface(), send_version=False, wait_for_verack=False) if self.options.v2transport: self.wait_until(lambda: self.nodes[0].getpeerinfo()[no_version_peer_id]["transport_protocol_type"] == "v2") From 7e96446e7234796b3a49c403e63f21bc0dd34f96 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 11 Jan 2024 12:08:55 -0500 Subject: [PATCH 3/3] Merge bitcoin/bitcoin#29034: test: detect OS in functional tests consistently using `platform.system()` 878d914777a03a04ecb84217152e8b7fd73a5062 doc: test: mention OS detection preferences in style guideline (Sebastian Falbesoner) 4c65ac96f8b021c107783adce3e8afe4f8edee6e test: detect OS consistently using `platform.system()` (Sebastian Falbesoner) 37324ae3dfb0e50daaf752dc863a880559fa4637 test: use `skip_if_platform_not_linux` helper where possible (Sebastian Falbesoner) Pull request description: There are at least three ways to detect the operating system in Python3: - `os.name` (https://docs.python.org/3.9/library/os.html#os.name) - `sys.platform` (https://docs.python.org/3.9/library/sys.html#sys.platform) - `platform.system()` (https://docs.python.org/3.9/library/platform.html#platform.system) We are currently using all of them in functional tests (both in individual tests and shared test framework code), which seems a bit messy. This PR consolidates into using `platform.system()`, as it appears to be one most consistent and easy to read (see also [IRC discussion](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-12-08#989301;) and table below). `sys.platform` is inconsistent as it has the major version number encoded for BSD systems, which doesn't make much sense for e.g. OpenBSD, where there is no concept of major versions, but instead the version is simply increased by 0.1 on each release. Note that `os.name` is still useful to detect whether we are running a POSIX system (see `BitcoinTestFramework.skip_if_platform_not_posix`), so for this use-case it is kept as only exception. The following table shows values for common operating systems, found via ``` $ python3 -c "import os; import sys; import platform; print(os.name, sys.platform, platform.system())" ``` | OS | os.name | sys.platform | platform.system() | |--------------|---------|--------------|--------------------| | Linux 6.2.0 | posix | linux | Linux | | MacOS* | posix | darwin | Darwin | | OpenBSD 7.4 | posix | openbsd7 | OpenBSD | | Windows* | nt | win32 | Windows | \* = I neither have a MacOS nor a Windows machine available, so I extracted the values from documentation and our current code. Also I'm relying on CI for testing the relevant code-paths. Having reviewers to this this locally would be very appreciated, if this gets Concept ACKed. ACKs for top commit: kevkevinpal: ACK [878d914](https://github.com/bitcoin/bitcoin/pull/29034/commits/878d914777a03a04ecb84217152e8b7fd73a5062) achow101: ACK 878d914777a03a04ecb84217152e8b7fd73a5062 hebasto: ACK 878d914777a03a04ecb84217152e8b7fd73a5062, I have reviewed the code and it looks OK. pablomartin4btc: tACK 878d914777a03a04ecb84217152e8b7fd73a5062 Tree-SHA512: 24513d493e47f572028c843260b81c47c2c29bfb701991050255c9f9529cd19065ecbc7b3b6e15619da7f3f608b4825c345ce6fee30d8fd1eaadbd08cff400fc --- test/functional/README.md | 4 ++++ test/functional/feature_bind_extra.py | 10 +++------- test/functional/feature_init.py | 4 ++-- test/functional/feature_notifications.py | 9 +++++---- test/functional/rpc_bind.py | 11 ++++------- test/functional/test_framework/p2p.py | 3 ++- test/functional/test_framework/test_node.py | 4 ++-- test/functional/test_runner.py | 3 ++- test/functional/wallet_multiwallet.py | 4 ++-- 9 files changed, 26 insertions(+), 26 deletions(-) diff --git a/test/functional/README.md b/test/functional/README.md index 2586c66c8368..76e5d16cceb9 100644 --- a/test/functional/README.md +++ b/test/functional/README.md @@ -37,6 +37,10 @@ don't have test cases for. `set_test_params()`, `add_options()` and `setup_xxxx()` methods at the top of the subclass, then locally-defined helper methods, then the `run_test()` method. - Use `f'{x}'` for string formatting in preference to `'{}'.format(x)` or `'%s' % x`. +- Use `platform.system()` for detecting the running operating system and `os.name` to + check whether it's a POSIX system (see also the `skip_if_platform_not_{linux,posix}` + methods in the `BitcoinTestFramework` class, which can be used to skip a whole test + depending on the platform). #### Naming guidelines diff --git a/test/functional/feature_bind_extra.py b/test/functional/feature_bind_extra.py index 5de9ff203cc2..a6d76854a6d6 100755 --- a/test/functional/feature_bind_extra.py +++ b/test/functional/feature_bind_extra.py @@ -7,15 +7,12 @@ that bind happens on the expected ports. """ -import sys - from test_framework.netutil import ( addr_to_hex, get_bind_addrs, ) from test_framework.test_framework import ( BitcoinTestFramework, - SkipTest, ) from test_framework.util import ( assert_equal, @@ -32,12 +29,11 @@ def set_test_params(self): self.bind_to_localhost_only = False self.num_nodes = 2 - def setup_network(self): + def skip_test_if_missing_module(self): # Due to OS-specific network stats queries, we only run on Linux. - self.log.info("Checking for Linux") - if not sys.platform.startswith('linux'): - raise SkipTest("This test can only be run on Linux.") + self.skip_if_platform_not_linux() + def setup_network(self): loopback_ipv4 = addr_to_hex("127.0.0.1") # Start custom ports by reusing unused p2p ports diff --git a/test/functional/feature_init.py b/test/functional/feature_init.py index ce12319933de..89d5c1e645df 100755 --- a/test/functional/feature_init.py +++ b/test/functional/feature_init.py @@ -3,9 +3,9 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Stress tests related to node initialization.""" -import os from pathlib import Path from random import randint +import platform import shutil from test_framework.test_framework import BitcoinTestFramework, SkipTest @@ -37,7 +37,7 @@ def run_test(self): # and other approaches (like below) don't work: # # os.kill(node.process.pid, signal.CTRL_C_EVENT) - if os.name == 'nt': + if platform.system() == 'Windows': raise SkipTest("can't SIGTERM on Windows") self.stop_node(0) diff --git a/test/functional/feature_notifications.py b/test/functional/feature_notifications.py index a91d15afe981..224b2a2f6ce1 100755 --- a/test/functional/feature_notifications.py +++ b/test/functional/feature_notifications.py @@ -5,6 +5,7 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test the -alertnotify, -blocknotify, -chainlocknotify, -instantsendnotify and -walletnotify options.""" import os +import platform from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE @@ -16,13 +17,13 @@ # Linux allow all characters other than \x00 # Windows disallow control characters (0-31) and /\?%:|"<> -FILE_CHAR_START = 32 if os.name == 'nt' else 1 +FILE_CHAR_START = 32 if platform.system() == 'Windows' else 1 FILE_CHAR_END = 128 -FILE_CHARS_DISALLOWED = '/\\?%*:|"<>' if os.name == 'nt' else '/' +FILE_CHARS_DISALLOWED = '/\\?%*:|"<>' if platform.system() == 'Windows' else '/' UNCONFIRMED_HASH_STRING = 'unconfirmed' def notify_outputname(walletname, txid): - return txid if os.name == 'nt' else f'{walletname}_{txid}' + return txid if platform.system() == 'Windows' else f'{walletname}_{txid}' class NotificationsTest(DashTestFramework): @@ -170,7 +171,7 @@ def expect_wallet_notify(self, tx_details): # Universal newline ensures '\n' on 'nt' assert_equal(text[-1], '\n') text = text[:-1] - if os.name == 'nt': + if platform.system() == 'Windows': # On Windows, echo as above will append a whitespace assert_equal(text[-1], ' ') text = text[:-1] diff --git a/test/functional/rpc_bind.py b/test/functional/rpc_bind.py index 91c624222dc7..9a8990161568 100755 --- a/test/functional/rpc_bind.py +++ b/test/functional/rpc_bind.py @@ -4,8 +4,6 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test running dashd with the -rpcbind and -rpcallowip options.""" -import sys - from test_framework.netutil import all_interfaces, addr_to_hex, get_bind_addrs, test_ipv6_local from test_framework.test_framework import BitcoinTestFramework, SkipTest from test_framework.util import assert_equal, assert_raises_rpc_error, get_rpc_proxy, rpc_port, rpc_url @@ -17,6 +15,10 @@ def set_test_params(self): self.num_nodes = 1 self.supports_cli = False + def skip_test_if_missing_module(self): + # due to OS-specific network stats queries, this test works only on Linux + self.skip_if_platform_not_linux() + def setup_network(self): self.add_nodes(self.num_nodes, None) @@ -61,14 +63,9 @@ def run_allowip_test(self, allow_ips, rpchost, rpcport): self.stop_nodes() def run_test(self): - # due to OS-specific network stats queries, this test works only on Linux if sum([self.options.run_ipv4, self.options.run_ipv6, self.options.run_nonloopback]) > 1: raise AssertionError("Only one of --ipv4, --ipv6 and --nonloopback can be set") - self.log.info("Check for linux") - if not sys.platform.startswith('linux'): - raise SkipTest("This test can only be run on linux.") - self.log.info("Check for ipv6") have_ipv6 = test_ipv6_local() if not have_ipv6 and not (self.options.run_ipv4 or self.options.run_nonloopback): diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py index d0d79f5ebc8e..bb96bdf34352 100755 --- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -24,6 +24,7 @@ from collections import defaultdict from io import BytesIO import logging +import platform import struct import sys import threading @@ -788,7 +789,7 @@ def __init__(self): NetworkThread.listeners = {} NetworkThread.protos = {} - if sys.platform == 'win32': + if platform.system() == 'Windows': asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy()) NetworkThread.network_event_loop = asyncio.new_event_loop() diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 52f785c14ce5..3dd3596a1889 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -12,13 +12,13 @@ import json import logging import os.path +import platform import re import subprocess import tempfile import time import urllib.parse import shlex -import sys import collections from pathlib import Path @@ -573,7 +573,7 @@ def test_success(cmd): cmd, shell=True, stderr=subprocess.DEVNULL, stdout=subprocess.DEVNULL) == 0 - if not sys.platform.startswith('linux'): + if platform.system() != 'Linux': self.log.warning("Can't profile with perf; only available on Linux platforms") return None diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 3b5375eb83cd..79da3d466776 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -17,6 +17,7 @@ import configparser import datetime import os +import platform import time import shutil import signal @@ -42,7 +43,7 @@ CROSS = "x " CIRCLE = "o " -if os.name == 'nt': #type:ignore +if platform.system() == 'Windows': #type:ignore import ctypes kernel32 = ctypes.windll.kernel32 # type: ignore ENABLE_VIRTUAL_TERMINAL_PROCESSING = 4 diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index b610347496e2..423cd9efb22c 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -9,9 +9,9 @@ from threading import Thread from decimal import Decimal import os +import platform import shutil import stat -import sys import time from test_framework.authproxy import JSONRPCException @@ -143,7 +143,7 @@ def wallet_file(name): # should raise rpc error if wallet path can't be created err_code = -4 if self.options.descriptors else -1 - assert_raises_rpc_error(err_code, "filesystem error:" if sys.platform != 'win32' else "create_directories:", self.nodes[0].createwallet, "w8/bad") + assert_raises_rpc_error(err_code, "filesystem error:" if platform.system() != 'Windows' else "create_directories:", self.nodes[0].createwallet, "w8/bad") # check that all requested wallets were created self.stop_node(0)