From faa30a4c566c5b720c7994c55f276352a119129f Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 29 Jan 2024 16:22:19 +0100 Subject: [PATCH 01/50] rpc: Do not wait for headers inside loadtxoutset --- contrib/devtools/test_utxo_snapshots.sh | 4 ++-- src/rpc/blockchain.cpp | 30 +++++-------------------- 2 files changed, 7 insertions(+), 27 deletions(-) diff --git a/contrib/devtools/test_utxo_snapshots.sh b/contrib/devtools/test_utxo_snapshots.sh index 93a4cd1683..d7c019f4a6 100755 --- a/contrib/devtools/test_utxo_snapshots.sh +++ b/contrib/devtools/test_utxo_snapshots.sh @@ -183,8 +183,8 @@ echo "-- Initial state of the client:" client_rpc getchainstates echo -echo "-- Loading UTXO snapshot into client..." -client_rpc loadtxoutset "$UTXO_DAT_FILE" +echo "-- Loading UTXO snapshot into client. Calling RPC in a loop..." +while ! client_rpc loadtxoutset "$UTXO_DAT_FILE" ; do sleep 10; done watch -n 0.3 "( tail -n 14 $CLIENT_DATADIR/debug.log ; echo ; ./src/bitcoin-cli -rpcport=$CLIENT_RPC_PORT -datadir=$CLIENT_DATADIR getchainstates) | cat" diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 34d308211b..50908e9f96 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -2571,7 +2571,7 @@ static RPCHelpMan dumptxoutset() { return RPCHelpMan{ "dumptxoutset", - "Write the serialized UTXO set to disk.", + "Write the serialized UTXO set to a file.", { {"path", RPCArg::Type::STR, RPCArg::Optional::NO, "Path to the output file. If relative, will be prefixed by datadir."}, }, @@ -2699,7 +2699,7 @@ static RPCHelpMan loadtxoutset() { return RPCHelpMan{ "loadtxoutset", - "Load the serialized UTXO set from disk.\n" + "Load the serialized UTXO set from a file.\n" "Once this snapshot is loaded, its contents will be " "deserialized into a second chainstate data structure, which is then used to sync to " "the network's tip. " @@ -2753,34 +2753,14 @@ static RPCHelpMan loadtxoutset() throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("Unable to load UTXO snapshot, " "assumeutxo block hash in snapshot metadata not recognized (%s)", base_blockhash.ToString())); } - int max_secs_to_wait_for_headers = 60 * 10; - CBlockIndex* snapshot_start_block = nullptr; - - LogPrintf("[snapshot] waiting to see blockheader %s in headers chain before snapshot activation\n", - base_blockhash.ToString()); - - while (max_secs_to_wait_for_headers > 0) { - snapshot_start_block = WITH_LOCK(::cs_main, + CBlockIndex* snapshot_start_block = WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(base_blockhash)); - max_secs_to_wait_for_headers -= 1; - - if (!IsRPCRunning()) { - throw JSONRPCError(RPC_CLIENT_NOT_CONNECTED, "Shutting down"); - } - - if (!snapshot_start_block) { - std::this_thread::sleep_for(std::chrono::seconds(1)); - } else { - break; - } - } if (!snapshot_start_block) { - LogPrintf("[snapshot] timed out waiting for snapshot start blockheader %s\n", - base_blockhash.ToString()); throw JSONRPCError( RPC_INTERNAL_ERROR, - "Timed out waiting for base block header to appear in headers chain"); + strprintf("The base block header (%s) must appear in the headers chain. Make sure all headers are syncing, and call this RPC again.", + base_blockhash.ToString())); } if (!chainman.ActivateSnapshot(afile, metadata, false)) { throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to load UTXO snapshot " + fs::PathToString(path)); From fa4e396e1da8e5b04a5f906b95017b969ea37bae Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 28 Jul 2023 10:44:44 +0200 Subject: [PATCH 02/50] fuzz: Generate with random libFuzzer settings --- test/fuzz/test_runner.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/fuzz/test_runner.py b/test/fuzz/test_runner.py index 4e24c07699..f4815a5f74 100755 --- a/test/fuzz/test_runner.py +++ b/test/fuzz/test_runner.py @@ -11,6 +11,7 @@ import configparser import logging import os +import random import subprocess import sys @@ -264,9 +265,12 @@ def job(command, t, t_env): for target, t_env in targets: target_corpus_dir = corpus_dir / target os.makedirs(target_corpus_dir, exist_ok=True) + use_value_profile = int(random.random() < .3) command = [ os.path.join(build_dir, 'src', 'test', 'fuzz', 'fuzz'), - "-runs=100000", + "-max_total_time=6000", + "-reload=0", + f"-use_value_profile={use_value_profile}", target_corpus_dir, ] futures.append(fuzz_pool.submit(job, command, target, t_env)) From fa3a4102ef0ae06d8930d7a7b567759e2a5b5fde Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 5 Feb 2024 16:25:57 +0100 Subject: [PATCH 03/50] fuzz: Set -rss_limit_mb=8000 for generate as well This is set by merge, so set it here as well, to avoid OOM. --- test/fuzz/test_runner.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/fuzz/test_runner.py b/test/fuzz/test_runner.py index f4815a5f74..ff3b6e6b6d 100755 --- a/test/fuzz/test_runner.py +++ b/test/fuzz/test_runner.py @@ -268,6 +268,7 @@ def job(command, t, t_env): use_value_profile = int(random.random() < .3) command = [ os.path.join(build_dir, 'src', 'test', 'fuzz', 'fuzz'), + "-rss_limit_mb=8000", "-max_total_time=6000", "-reload=0", f"-use_value_profile={use_value_profile}", From a8c3454ba137dfac20b3c89bc558374de0524114 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Tue, 6 Feb 2024 00:40:26 +0100 Subject: [PATCH 04/50] test: speedup bip324_cipher.py unit test Executing the unit tests for the bip324_cipher.py module currently takes quite long (>60 seconds on my notebook). Most time here is spent in empty plaintext/ciphertext encryption/decryption loops: .... for _ in range(msg_idx): enc_aead.encrypt(b"", b"") ... for _ in range(msg_idx): enc_aead.decrypt(b"", bytes(16)) ... Their sole purpose is increasing the FSChaCha20Poly1305 packet counters in order to trigger rekeying, i.e. the actual encryption/decryption is not relevant, as the result is thrown away. This commit speeds up the tests by supporting to pass "None" as plaintext/ciphertext, indicating to the routines that no actual encryption/decryption should be done. master branch: $ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py .. ---------------------------------------------------------------------- Ran 2 tests in 64.658s PR branch: $ python3 -m unittest ./test/functional/test_framework/crypto/bip324_cipher.py .. ---------------------------------------------------------------------- Ran 2 tests in 0.822s --- test/functional/test_framework/crypto/bip324_cipher.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/functional/test_framework/crypto/bip324_cipher.py b/test/functional/test_framework/crypto/bip324_cipher.py index 56190647f2..c9f0fa0151 100644 --- a/test/functional/test_framework/crypto/bip324_cipher.py +++ b/test/functional/test_framework/crypto/bip324_cipher.py @@ -25,6 +25,8 @@ def pad16(x): def aead_chacha20_poly1305_encrypt(key, nonce, aad, plaintext): """Encrypt a plaintext using ChaCha20Poly1305.""" + if plaintext is None: + return None ret = bytearray() msg_len = len(plaintext) for i in range((msg_len + 63) // 64): @@ -42,7 +44,7 @@ def aead_chacha20_poly1305_encrypt(key, nonce, aad, plaintext): def aead_chacha20_poly1305_decrypt(key, nonce, aad, ciphertext): """Decrypt a ChaCha20Poly1305 ciphertext.""" - if len(ciphertext) < 16: + if ciphertext is None or len(ciphertext) < 16: return None msg_len = len(ciphertext) - 16 poly1305 = Poly1305(chacha20_block(key, nonce, 0)[:32]) @@ -191,11 +193,11 @@ def test_fschacha20poly1305aead(self): dec_aead = FSChaCha20Poly1305(key) for _ in range(msg_idx): - enc_aead.encrypt(b"", b"") + enc_aead.encrypt(b"", None) ciphertext = enc_aead.encrypt(aad, plain) self.assertEqual(hex_cipher, ciphertext.hex()) for _ in range(msg_idx): - dec_aead.decrypt(b"", bytes(16)) + dec_aead.decrypt(b"", None) plaintext = dec_aead.decrypt(aad, ciphertext) self.assertEqual(plain, plaintext) From 5fc9db504b9ac784019e7e8c215c31abfccb62b6 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Fri, 17 Nov 2023 17:00:20 -0500 Subject: [PATCH 05/50] test: enable p2p_sendtxrcncl.py with v2transport By adding to the test framework a wait until the v2 handshake is completed, so that p2p_sendtxrcncl.py (which doesn't need to be changed itself) doesnt't send out any other messages before that. --- test/functional/p2p_v2_earlykeyresponse.py | 2 +- test/functional/test_framework/test_node.py | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/test/functional/p2p_v2_earlykeyresponse.py b/test/functional/p2p_v2_earlykeyresponse.py index 1f570e6010..32d2e1148a 100755 --- a/test/functional/p2p_v2_earlykeyresponse.py +++ b/test/functional/p2p_v2_earlykeyresponse.py @@ -75,7 +75,7 @@ def run_test(self): self.log.info('Sending first 4 bytes of ellswift which match network magic') self.log.info('If a response is received, assertion failure would happen in our custom data_received() function') # send happens in `initiate_v2_handshake()` in `connection_made()` - peer1 = node0.add_p2p_connection(PeerEarlyKey(), wait_for_verack=False, send_version=False, supports_v2_p2p=True) + peer1 = node0.add_p2p_connection(PeerEarlyKey(), wait_for_verack=False, send_version=False, supports_v2_p2p=True, wait_for_v2_handshake=False) self.wait_until(lambda: peer1.connection_opened) self.log.info('Sending remaining ellswift and garbage which are different from V1_PREFIX. Since a response is') self.log.info('expected now, our custom data_received() function wouldn\'t result in assertion failure') diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 838dcba141..660f8f90cc 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -667,7 +667,7 @@ def assert_start_raises_init_error(self, extra_args=None, expected_msg=None, mat assert_msg += "with expected error " + expected_msg self._raise_assertion_error(assert_msg) - def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, send_version=True, supports_v2_p2p=False, **kwargs): + def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, send_version=True, supports_v2_p2p=False, wait_for_v2_handshake=True, **kwargs): """Add an inbound p2p connection to the node. This method adds the p2p connection to the self.p2ps list and also @@ -693,6 +693,8 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, send_version=Tru self.p2ps.append(p2p_conn) p2p_conn.wait_until(lambda: p2p_conn.is_connected, check_connected=False) + if supports_v2_p2p and wait_for_v2_handshake: + p2p_conn.wait_until(lambda: p2p_conn.v2_state.tried_v2_handshake) if send_version: p2p_conn.wait_until(lambda: not p2p_conn.on_connection_send_msg) if wait_for_verack: @@ -771,6 +773,8 @@ def addconnection_callback(address, port): p2p_conn.wait_for_connect() self.p2ps.append(p2p_conn) + if supports_v2_p2p: + p2p_conn.wait_until(lambda: p2p_conn.v2_state.tried_v2_handshake) p2p_conn.wait_until(lambda: not p2p_conn.on_connection_send_msg) if wait_for_verack: p2p_conn.wait_for_verack() From 87549c8f89fe8c9f404b74c5a40b5ee54c5a966d Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 20 Nov 2023 16:17:42 -0500 Subject: [PATCH 06/50] test: enable p2p_invalid_messages.py with v2transport by disabling some sub-tests that test v1-specific features, and adapting others to v2. --- test/functional/p2p_invalid_messages.py | 39 +++++++++++++++++++------ 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 4916d36ab7..762e6c4688 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -109,6 +109,9 @@ def test_duplicate_version_msg(self): self.nodes[0].disconnect_p2ps() def test_magic_bytes(self): + # Skip with v2, magic bytes are v1-specific + if self.options.v2transport: + return self.log.info("Test message with invalid magic bytes disconnects peer") conn = self.nodes[0].add_p2p_connection(P2PDataStore()) with self.nodes[0].assert_debug_log(['Header error: Wrong MessageStart ffffffff received']): @@ -120,6 +123,9 @@ def test_magic_bytes(self): self.nodes[0].disconnect_p2ps() def test_checksum(self): + # Skip with v2, the checksum is v1-specific + if self.options.v2transport: + return self.log.info("Test message with invalid checksum logs an error") conn = self.nodes[0].add_p2p_connection(P2PDataStore()) with self.nodes[0].assert_debug_log(['Header error: Wrong checksum (badmsg, 2 bytes), expected 78df0a04 was ffffffff']): @@ -137,7 +143,11 @@ def test_checksum(self): def test_size(self): self.log.info("Test message with oversized payload disconnects peer") conn = self.nodes[0].add_p2p_connection(P2PDataStore()) - with self.nodes[0].assert_debug_log(['Header error: Size too large (badmsg, 4000001 bytes)']): + error_msg = ( + ['V2 transport error: packet too large (4000014 bytes)'] if self.options.v2transport + else ['Header error: Size too large (badmsg, 4000001 bytes)'] + ) + with self.nodes[0].assert_debug_log(error_msg): msg = msg_unrecognized(str_data="d" * (VALID_DATA_LIMIT + 1)) msg = conn.build_message(msg) conn.send_raw_message(msg) @@ -147,15 +157,26 @@ def test_size(self): def test_msgtype(self): self.log.info("Test message with invalid message type logs an error") conn = self.nodes[0].add_p2p_connection(P2PDataStore()) - with self.nodes[0].assert_debug_log(['Header error: Invalid message type']): + if self.options.v2transport: + msgtype = 99 # not defined msg = msg_unrecognized(str_data="d") - msg = conn.build_message(msg) - # Modify msgtype - msg = msg[:7] + b'\x00' + msg[7 + 1:] - conn.send_raw_message(msg) - conn.sync_with_ping(timeout=1) - # Check that traffic is accounted for (24 bytes header + 2 bytes payload) - assert_equal(self.nodes[0].getpeerinfo()[0]['bytesrecv_per_msg']['*other*'], 26) + contents = msgtype.to_bytes(1, 'big') + msg.serialize() + tmsg = conn.v2_state.v2_enc_packet(contents, ignore=False) + with self.nodes[0].assert_debug_log(['V2 transport error: invalid message type']): + conn.send_raw_message(tmsg) + conn.sync_with_ping(timeout=1) + # Check that traffic is accounted for (20 bytes plus 3 bytes contents) + assert_equal(self.nodes[0].getpeerinfo()[0]['bytesrecv_per_msg']['*other*'], 23) + else: + with self.nodes[0].assert_debug_log(['Header error: Invalid message type']): + msg = msg_unrecognized(str_data="d") + msg = conn.build_message(msg) + # Modify msgtype + msg = msg[:7] + b'\x00' + msg[7 + 1:] + conn.send_raw_message(msg) + conn.sync_with_ping(timeout=1) + # Check that traffic is accounted for (24 bytes header + 2 bytes payload) + assert_equal(self.nodes[0].getpeerinfo()[0]['bytesrecv_per_msg']['*other*'], 26) self.nodes[0].disconnect_p2ps() def test_addrv2(self, label, required_log_messages, raw_addrv2): From 6e9e39da434f8dafacee4cba068daf499bdb4cc2 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Tue, 16 Jan 2024 11:28:10 -0500 Subject: [PATCH 07/50] test: Don't use v2transport when it's too slow. Sending multiple large messages is rather slow with the non-optimized python implementation of ChaCha20. Apart from the slowness, these tests would also run successfully with v2. --- test/functional/feature_block.py | 4 ++++ test/functional/feature_maxuploadtarget.py | 5 +++-- test/functional/p2p_invalid_messages.py | 6 ++++-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/test/functional/feature_block.py b/test/functional/feature_block.py index 58ef1e761d..8a95975184 100755 --- a/test/functional/feature_block.py +++ b/test/functional/feature_block.py @@ -1263,6 +1263,10 @@ def run_test(self): b89a = self.update_block("89a", [tx]) self.send_blocks([b89a], success=False, reject_reason='bad-txns-inputs-missingorspent', reconnect=True) + # Don't use v2transport for the large reorg, which is too slow with the unoptimized python ChaCha20 implementation + if self.options.v2transport: + self.nodes[0].disconnect_p2ps() + self.helper_peer = self.nodes[0].add_p2p_connection(P2PDataStore(), supports_v2_p2p=False) self.log.info("Test a re-org of one week's worth of blocks (1088 blocks)") self.move_tip(88) diff --git a/test/functional/feature_maxuploadtarget.py b/test/functional/feature_maxuploadtarget.py index c551c0b449..ffb60d1918 100755 --- a/test/functional/feature_maxuploadtarget.py +++ b/test/functional/feature_maxuploadtarget.py @@ -67,7 +67,8 @@ def run_test(self): p2p_conns = [] for _ in range(3): - p2p_conns.append(self.nodes[0].add_p2p_connection(TestP2PConn())) + # Don't use v2transport in this test (too slow with the unoptimized python ChaCha20 implementation) + p2p_conns.append(self.nodes[0].add_p2p_connection(TestP2PConn(), supports_v2_p2p=False)) # Now mine a big block mine_large_block(self, self.wallet, self.nodes[0]) @@ -148,7 +149,7 @@ def run_test(self): self.restart_node(0, ["-whitelist=download@127.0.0.1", "-maxuploadtarget=1"]) # Reconnect to self.nodes[0] - peer = self.nodes[0].add_p2p_connection(TestP2PConn()) + peer = self.nodes[0].add_p2p_connection(TestP2PConn(), supports_v2_p2p=False) #retrieve 20 blocks which should be enough to break the 1MB limit getdata_request.inv = [CInv(MSG_BLOCK, big_new_block)] diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 762e6c4688..40a69936bc 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -327,8 +327,10 @@ def test_noncontinuous_headers_msg(self): def test_resource_exhaustion(self): self.log.info("Test node stays up despite many large junk messages") - conn = self.nodes[0].add_p2p_connection(P2PDataStore()) - conn2 = self.nodes[0].add_p2p_connection(P2PDataStore()) + # Don't use v2 here - the non-optimised encryption would take too long to encrypt + # the large messages + conn = self.nodes[0].add_p2p_connection(P2PDataStore(), supports_v2_p2p=False) + conn2 = self.nodes[0].add_p2p_connection(P2PDataStore(), supports_v2_p2p=False) msg_at_size = msg_unrecognized(str_data="b" * VALID_DATA_LIMIT) assert len(msg_at_size.serialize()) == MAX_PROTOCOL_MESSAGE_LENGTH From bf5662c678455fb47c402f8520215726ddfe7a94 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 20 Nov 2023 15:10:29 -0500 Subject: [PATCH 08/50] test: enable v2 for python p2p depending on global --v2transport flag This changes the default behavior, individual tests can overwrite this option. As a result, it is possible to run the entire test suite with --v2transport, and all connections to the python p2p will then use it. Also adjust several tests that are already running with --v2transport in the test runner (although they actually made v1 connection before this change). This is done in the same commit so that there isn't an intermediate commit in which the CI fails. --- test/functional/p2p_ibd_stalling.py | 3 ++- test/functional/p2p_timeouts.py | 29 ++++++++++++--------- test/functional/rpc_net.py | 13 ++++----- test/functional/test_framework/test_node.py | 12 +++++++-- 4 files changed, 35 insertions(+), 22 deletions(-) diff --git a/test/functional/p2p_ibd_stalling.py b/test/functional/p2p_ibd_stalling.py index 0eb37fa92f..830f374d63 100755 --- a/test/functional/p2p_ibd_stalling.py +++ b/test/functional/p2p_ibd_stalling.py @@ -80,7 +80,8 @@ def run_test(self): # Need to wait until 1023 blocks are received - the magic total bytes number is a workaround in lack of an rpc # returning the number of downloaded (but not connected) blocks. - self.wait_until(lambda: self.total_bytes_recv_for_blocks() == 172761) + bytes_recv = 172761 if not self.options.v2transport else 169692 + self.wait_until(lambda: self.total_bytes_recv_for_blocks() == bytes_recv) self.all_sync_send_with_ping(peers) # If there was a peer marked for stalling, it would get disconnected diff --git a/test/functional/p2p_timeouts.py b/test/functional/p2p_timeouts.py index b4fa5099d8..80d7b6e9ae 100755 --- a/test/functional/p2p_timeouts.py +++ b/test/functional/p2p_timeouts.py @@ -69,11 +69,8 @@ def run_test(self): with self.nodes[0].assert_debug_log(['Unsupported message "ping" prior to verack from peer=0']): no_verack_node.send_message(msg_ping()) - # With v2, non-version messages before the handshake would be interpreted as part of the key exchange. - # Therefore, don't execute this part of the test if v2transport is chosen. - if not self.options.v2transport: - with self.nodes[0].assert_debug_log(['non-version message before version handshake. Message "ping" from peer=1']): - no_version_node.send_message(msg_ping()) + with self.nodes[0].assert_debug_log(['non-version message before version handshake. Message "ping" from peer=1']): + no_version_node.send_message(msg_ping()) self.mock_forward(1) assert "version" in no_verack_node.last_message @@ -83,14 +80,20 @@ def run_test(self): assert no_send_node.is_connected no_verack_node.send_message(msg_ping()) - if not self.options.v2transport: - no_version_node.send_message(msg_ping()) - - expected_timeout_logs = [ - "version handshake timeout peer=0", - f"socket no message in first 3 seconds, {'0' if self.options.v2transport else '1'} 0 peer=1", - "socket no message in first 3 seconds, 0 0 peer=2", - ] + no_version_node.send_message(msg_ping()) + + if self.options.v2transport: + expected_timeout_logs = [ + "version handshake timeout peer=0", + "version handshake timeout peer=1", + "version handshake timeout peer=2", + ] + else: + expected_timeout_logs = [ + "version handshake timeout peer=0", + "socket no message in first 3 seconds, 1 0 peer=1", + "socket no message in first 3 seconds, 0 0 peer=2", + ] with self.nodes[0].assert_debug_log(expected_msgs=expected_timeout_logs): self.mock_forward(2) diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index afb75ab208..b4a58df5b2 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -117,6 +117,9 @@ def test_getpeerinfo(self): peer_info = self.nodes[0].getpeerinfo()[no_version_peer_id] peer_info.pop("addr") peer_info.pop("addrbind") + # The next two fields will vary for v2 connections because we send a rng-based number of decoy messages + peer_info.pop("bytesrecv") + peer_info.pop("bytessent") assert_equal( peer_info, { @@ -125,9 +128,7 @@ def test_getpeerinfo(self): "addr_relay_enabled": False, "bip152_hb_from": False, "bip152_hb_to": False, - "bytesrecv": 0, "bytesrecv_per_msg": {}, - "bytessent": 0, "bytessent_per_msg": {}, "connection_type": "inbound", "conntime": no_version_peer_conntime, @@ -136,8 +137,8 @@ def test_getpeerinfo(self): "inflight": [], "last_block": 0, "last_transaction": 0, - "lastrecv": 0, - "lastsend": 0, + "lastrecv": 0 if not self.options.v2transport else no_version_peer_conntime, + "lastsend": 0 if not self.options.v2transport else no_version_peer_conntime, "minfeefilter": Decimal("0E-8"), "network": "not_publicly_routable", "permissions": [], @@ -145,13 +146,13 @@ def test_getpeerinfo(self): "relaytxes": False, "services": "0000000000000000", "servicesnames": [], - "session_id": "", + "session_id": "" if not self.options.v2transport else no_version_peer.v2_state.peer['session_id'].hex(), "startingheight": -1, "subver": "", "synced_blocks": -1, "synced_headers": -1, "timeoffset": 0, - "transport_protocol_type": "v1" if not self.options.v2transport else "detecting", + "transport_protocol_type": "v1" if not self.options.v2transport else "v2", "version": 0, }, ) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 660f8f90cc..3baa78fd79 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -667,7 +667,7 @@ def assert_start_raises_init_error(self, extra_args=None, expected_msg=None, mat assert_msg += "with expected error " + expected_msg self._raise_assertion_error(assert_msg) - def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, send_version=True, supports_v2_p2p=False, wait_for_v2_handshake=True, **kwargs): + def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, send_version=True, supports_v2_p2p=None, wait_for_v2_handshake=True, **kwargs): """Add an inbound p2p connection to the node. This method adds the p2p connection to the self.p2ps list and also @@ -684,6 +684,9 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, send_version=Tru kwargs['dstport'] = p2p_port(self.index) if 'dstaddr' not in kwargs: kwargs['dstaddr'] = '127.0.0.1' + if supports_v2_p2p is None: + supports_v2_p2p = self.use_v2transport + p2p_conn.p2p_connected_to_node = True if self.use_v2transport: @@ -723,7 +726,7 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, send_version=Tru return p2p_conn - def add_outbound_p2p_connection(self, p2p_conn, *, wait_for_verack=True, p2p_idx, connection_type="outbound-full-relay", supports_v2_p2p=False, advertise_v2_p2p=False, **kwargs): + def add_outbound_p2p_connection(self, p2p_conn, *, wait_for_verack=True, p2p_idx, connection_type="outbound-full-relay", supports_v2_p2p=None, advertise_v2_p2p=None, **kwargs): """Add an outbound p2p connection from node. Must be an "outbound-full-relay", "block-relay-only", "addr-fetch" or "feeler" connection. @@ -751,6 +754,11 @@ def addconnection_callback(address, port): self.addconnection('%s:%d' % (address, port), connection_type, advertise_v2_p2p) p2p_conn.p2p_connected_to_node = False + if supports_v2_p2p is None: + supports_v2_p2p = self.use_v2transport + if advertise_v2_p2p is None: + advertise_v2_p2p = self.use_v2transport + if advertise_v2_p2p: kwargs['services'] = kwargs.get('services', P2P_SERVICES) | NODE_P2P_V2 assert self.use_v2transport # only a v2 TestNode could make a v2 outbound connection From 0fbf051fec723f86f49ab14ea15c91bb1435c656 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Sun, 18 Feb 2024 01:57:16 +0100 Subject: [PATCH 09/50] depends: fix BDB compilation on OpenBSD Compiling C++ code with `-D_XOPEN_SOURCE=600` causes problems on OpenBSD. If that define is set, the C++ standard header detection routine in BDB's configure script fails. This results in `HAVE_CXX_STDHEADERS` not being defined, which then it turn leads to the inclusion of `` (rather than ``), which doesn't exist. According to a mailing list post discussing a similar problem [1], "OpenBSD provides the POSIX APIs by default", so we don't need this define anyway and can remove it. This fixes the BDB build problem as described in issue #28963. Tested on OpenBSD 7.4 with clang 13.0.0. [1] https://www.mail-archive.com/tech@openbsd.org/msg63386.html --- depends/packages/bdb.mk | 1 - 1 file changed, 1 deletion(-) diff --git a/depends/packages/bdb.mk b/depends/packages/bdb.mk index 9f5a925015..1a21238152 100644 --- a/depends/packages/bdb.mk +++ b/depends/packages/bdb.mk @@ -17,7 +17,6 @@ $(package)_config_opts_android=--with-pic $(package)_cflags+=-Wno-error=implicit-function-declaration -Wno-error=format-security -Wno-error=implicit-int $(package)_cppflags_freebsd=-D_XOPEN_SOURCE=600 -D__BSD_VISIBLE=1 $(package)_cppflags_netbsd=-D_XOPEN_SOURCE=600 -$(package)_cppflags_openbsd=-D_XOPEN_SOURCE=600 $(package)_cppflags_mingw32=-DUNICODE -D_UNICODE endef From fa100512677587b4e84a8be2235cf6d49b6a0134 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 8 Feb 2024 13:43:38 +0100 Subject: [PATCH 10/50] lint: Add get_subtrees() helper This is needed for a future change. --- test/lint/test_runner/src/main.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index 1dc79e97bd..28644f839c 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -34,17 +34,22 @@ fn get_git_root() -> PathBuf { PathBuf::from(check_output(git().args(["rev-parse", "--show-toplevel"])).unwrap()) } +/// Return all subtree paths +fn get_subtrees() -> Vec<&'static str> { + vec![ + "src/crc32c", + "src/crypto/ctaes", + "src/leveldb", + "src/minisketch", + "src/secp256k1", + ] +} + fn lint_subtree() -> LintResult { // This only checks that the trees are pure subtrees, it is not doing a full // check with -r to not have to fetch all the remotes. let mut good = true; - for subtree in [ - "src/crypto/ctaes", - "src/secp256k1", - "src/minisketch", - "src/leveldb", - "src/crc32c", - ] { + for subtree in get_subtrees() { good &= Command::new("test/lint/git-subtree-check.sh") .arg(subtree) .status() From fa770fd368e32950dd727e111a5d66e1dbb93716 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 9 Feb 2024 10:31:40 +0100 Subject: [PATCH 11/50] doc: Add missing RUST_BACKTRACE=1 This will print a backtrace when an internal code error happened. --- test/lint/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/lint/README.md b/test/lint/README.md index 1fba41d9ea..1bfcb75327 100644 --- a/test/lint/README.md +++ b/test/lint/README.md @@ -19,7 +19,7 @@ test runner To run all the lint checks in the test runner outside the docker, use: ```sh -( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && cargo run ) +( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && RUST_BACKTRACE=1 cargo run ) ``` #### Dependencies From fa63b0e351dee4782ee19ad46603957ef8d020eb Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Sun, 11 Feb 2024 15:21:47 +0100 Subject: [PATCH 12/50] lint: Make lint error easier to spot in output --- test/lint/test_runner/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index 28644f839c..3207538e6f 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -139,7 +139,7 @@ fn main() -> ExitCode { // chdir to root before each lint test env::set_current_dir(&git_root).unwrap(); if let Err(err) = lint_fn() { - println!("{err}\n^---- Failure generated from {lint_name}!"); + println!("{err}\n^---- ⚠️ Failure generated from {lint_name}!"); test_failed = true; } } From fa31908ea848488ff842f1b9fce6235bb8855ec7 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 8 Feb 2024 13:58:44 +0100 Subject: [PATCH 13/50] lint: Check for missing or redundant bitcoin-config.h includes --- ci/lint/04_install.sh | 3 +- test/lint/test_runner/src/main.rs | 107 +++++++++++++++++++++++++++++- 2 files changed, 108 insertions(+), 2 deletions(-) diff --git a/ci/lint/04_install.sh b/ci/lint/04_install.sh index 476417d04b..47cb8864c2 100755 --- a/ci/lint/04_install.sh +++ b/ci/lint/04_install.sh @@ -10,10 +10,11 @@ export PATH=$PWD/ci/retry:$PATH ${CI_RETRY_EXE} apt-get update # Lint dependencies: +# - automake pkg-config libtool (for lint_includes_build_config) # - curl/xz-utils (to install shellcheck) # - git (used in many lint scripts) # - gpg (used by verify-commits) -${CI_RETRY_EXE} apt-get install -y curl xz-utils git gpg +${CI_RETRY_EXE} apt-get install -y automake pkg-config libtool curl xz-utils git gpg PYTHON_PATH="/python_build" if [ ! -d "${PYTHON_PATH}/bin" ]; then diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index 3207538e6f..b97e822484 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -4,7 +4,7 @@ use std::env; use std::fs; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::Command; use std::process::ExitCode; @@ -45,6 +45,14 @@ fn get_subtrees() -> Vec<&'static str> { ] } +/// Return the pathspecs to exclude all subtrees +fn get_pathspecs_exclude_subtrees() -> Vec { + get_subtrees() + .iter() + .map(|s| format!(":(exclude){}", s)) + .collect() +} + fn lint_subtree() -> LintResult { // This only checks that the trees are pure subtrees, it is not doing a full // check with -r to not have to fetch all the remotes. @@ -87,6 +95,102 @@ fs:: namespace, which has unsafe filesystem functions marked as deleted. } } +fn lint_includes_build_config() -> LintResult { + let config_path = "./src/config/bitcoin-config.h.in"; + let include_directive = "#include "; + if !Path::new(config_path).is_file() { + assert!(Command::new("./autogen.sh") + .status() + .expect("command error") + .success()); + } + let defines_regex = format!( + r"^\s*(?!//).*({})", + check_output(Command::new("grep").args(["undef ", "--", config_path])) + .expect("grep failed") + .lines() + .map(|line| { + line.split("undef ") + .nth(1) + .unwrap_or_else(|| panic!("Could not extract name in line: {line}")) + }) + .collect::>() + .join("|") + ); + let print_affected_files = |mode: bool| { + // * mode==true: Print files which use the define, but lack the include + // * mode==false: Print files which lack the define, but use the include + let defines_files = check_output( + git() + .args([ + "grep", + "--perl-regexp", + if mode { + "--files-with-matches" + } else { + "--files-without-match" + }, + &defines_regex, + "--", + "*.cpp", + "*.h", + ]) + .args(get_pathspecs_exclude_subtrees()) + .args([ + // These are exceptions which don't use bitcoin-config.h, rather the Makefile.am adds + // these cppflags manually. + ":(exclude)src/crypto/sha256_arm_shani.cpp", + ":(exclude)src/crypto/sha256_avx2.cpp", + ":(exclude)src/crypto/sha256_sse41.cpp", + ":(exclude)src/crypto/sha256_x86_shani.cpp", + ]), + ) + .expect("grep failed"); + git() + .args([ + "grep", + if mode { + "--files-without-match" + } else { + "--files-with-matches" + }, + include_directive, + "--", + ]) + .args(defines_files.lines()) + .status() + .expect("command error") + .success() + }; + let missing = print_affected_files(true); + if missing { + return Err(format!( + r#" +^^^ +One or more files use a symbol declared in the bitcoin-config.h header. However, they are not +including the header. This is problematic, because the header may or may not be indirectly +included. If the indirect include were to be intentionally or accidentally removed, the build could +still succeed, but silently be buggy. For example, a slower fallback algorithm could be picked, +even though bitcoin-config.h indicates that a faster feature is available and should be used. + +If you are unsure which symbol is used, you can find it with this command: +git grep --perl-regexp '{}' -- file_name + "#, + defines_regex + )); + } + let redundant = print_affected_files(false); + if redundant { + return Err(r#" +^^^ +None of the files use a symbol declared in the bitcoin-config.h header. However, they are including +the header. Consider removing the unused include. + "# + .to_string()); + } + Ok(()) +} + fn lint_doc() -> LintResult { if Command::new("test/lint/check-doc.py") .status() @@ -128,6 +232,7 @@ fn main() -> ExitCode { let test_list: Vec<(&str, LintFn)> = vec![ ("subtree check", lint_subtree), ("std::filesystem check", lint_std_filesystem), + ("build config includes check", lint_includes_build_config), ("-help=1 documentation check", lint_doc), ("lint-*.py scripts", lint_all), ]; From fa58ae74ea67485495dbc2d003adfbca68d6869b Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 Feb 2024 15:16:03 +0100 Subject: [PATCH 14/50] refactor: Add missing include for USE_BDB, USE_SQLITE to bench/wallet_ismine.cpp It was included indirectly via src/wallet/test/util.h, however it is better to include what you use. --- src/bench/wallet_ismine.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/bench/wallet_ismine.cpp b/src/bench/wallet_ismine.cpp index 261c95c7c8..3f922e18a5 100644 --- a/src/bench/wallet_ismine.cpp +++ b/src/bench/wallet_ismine.cpp @@ -2,6 +2,9 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#if defined(HAVE_CONFIG_H) +#include +#endif // HAVE_CONFIG_H #include #include #include @@ -11,9 +14,9 @@ #include #include #include +#include #include #include -#include namespace wallet { static void WalletIsMine(benchmark::Bench& bench, bool legacy_wallet, int num_combo = 0) From 84388c942cb035fed546eda360ae6b40c6cfac09 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 21 Feb 2024 10:02:21 -0500 Subject: [PATCH 15/50] ci: avoid running git diff after patching Drop `git diff` command so it is easier to run CI locally if git checkout is a worktree. Currently it fails because the directory is not recognized as a git repository. The `git diff` command was added recently in #28359 commit fa07ac48d804beac38a98d23be2167f78cadefae and can be avoided just by teeing the patch to stdout --- ci/test/03_test_script.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ci/test/03_test_script.sh b/ci/test/03_test_script.sh index cdcd731528..786cb08bf6 100755 --- a/ci/test/03_test_script.sh +++ b/ci/test/03_test_script.sh @@ -36,8 +36,8 @@ export HOST=${HOST:-$("$BASE_ROOT_DIR/depends/config.guess")} # CI, so as a temporary minimal fix to work around UB and CI failures, leave # bytes_written unmodified. # See https://github.com/bitcoin/bitcoin/pull/28359#issuecomment-1698694748 - echo 'diff --git a/src/leveldb/db/db_impl.cc b/src/leveldb/db/db_impl.cc -index 65e31724bc..f61b471953 100644 + # Tee patch to stdout to make it clear CI is testing modified code. + tee >(patch -p1) <<'EOF' --- a/src/leveldb/db/db_impl.cc +++ b/src/leveldb/db/db_impl.cc @@ -1028,9 +1028,6 @@ Status DBImpl::DoCompactionWork(CompactionState* compact) { @@ -49,8 +49,8 @@ index 65e31724bc..f61b471953 100644 - } mutex_.Lock(); - stats_[compact->compaction->level() + 1].Add(stats);' | patch -p1 - git diff + stats_[compact->compaction->level() + 1].Add(stats); +EOF ) if [ "$RUN_FUZZ_TESTS" = "true" ]; then From faeed91c0be6e5dda4790522d0dc999afd869d11 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 22 Feb 2024 19:37:23 +0100 Subject: [PATCH 16/50] test: Fix intermittent issue in interface_rest.py --- test/functional/interface_rest.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py index b81eae2506..05aa40bbfe 100755 --- a/test/functional/interface_rest.py +++ b/test/functional/interface_rest.py @@ -337,6 +337,9 @@ def run_test(self): assert_greater_than(json_obj['bytes'], 300) mempool_info = self.nodes[0].getmempoolinfo() + # pop unstable unbroadcastcount before check + for obj in [json_obj, mempool_info]: + obj.pop("unbroadcastcount") assert_equal(json_obj, mempool_info) # Check that there are our submitted transactions in the TX memory pool From eb5d78c649c9ad55b3809473b0d5ec4b88ed923d Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Fri, 23 Feb 2024 13:14:41 -0500 Subject: [PATCH 17/50] doc: document preference for list-initialization --- doc/developer-notes.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index ece36cb088..8c3845c66c 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -113,6 +113,8 @@ code. between integer types, use functional casts such as `int(x)` or `int{x}` instead of `(int) x`. When casting between more complex types, use `static_cast`. Use `reinterpret_cast` and `const_cast` as appropriate. + - Prefer [`list initialization ({})`](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-list) where possible. + For example `int x{0};` instead of `int x = 0;` or `int x(0);` For function calls a namespace should be specified explicitly, unless such functions have been declared within it. Otherwise, [argument-dependent lookup](https://en.cppreference.com/w/cpp/language/adl), also known as ADL, could be @@ -138,7 +140,7 @@ int main() Block style example: ```c++ -int g_count = 0; +int g_count{0}; namespace foo { class Class @@ -150,7 +152,7 @@ public: { // Comment summarising what this section of code does for (int i = 0; i < n; ++i) { - int total_sum = 0; + int total_sum{0}; // When something fails, return early if (!Something()) return false; ... From b03b20685a3a85c9664a7c1b4d37a49d27b056de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Fri, 23 Feb 2024 20:25:59 +0100 Subject: [PATCH 18/50] Fix CI-detected codespell warnings --- src/test/span_tests.cpp | 2 +- src/wallet/scriptpubkeyman.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/span_tests.cpp b/src/test/span_tests.cpp index f6cac10b09..aae61990f7 100644 --- a/src/test/span_tests.cpp +++ b/src/test/span_tests.cpp @@ -53,7 +53,7 @@ BOOST_AUTO_TEST_SUITE(span_tests) // aren't compatible with Spans at compile time. // // Previously there was a bug where writing a SFINAE check for vector was -// not possible, because in libstdc++ vector has a data() memeber +// not possible, because in libstdc++ vector has a data() member // returning void*, and the Span template guide ignored the data() return value, // so the template substitution would succeed, but the constructor would fail, // resulting in a fatal compile error, rather than a SFINAE error that could be diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 04f7f89d68..2d83ae556f 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -52,7 +52,7 @@ class WalletStorage virtual bool WithEncryptionKey(std::function cb) const = 0; virtual bool HasEncryptionKeys() const = 0; virtual bool IsLocked() const = 0; - //! Callback function for after TopUp completes containining any scripts that were added by a SPKMan + //! Callback function for after TopUp completes containing any scripts that were added by a SPKMan virtual void TopUpCallback(const std::set&, ScriptPubKeyMan*) = 0; }; From 5f240ab2e89fb20286fbaf9a1f00346bb1cad5a1 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Fri, 23 Feb 2024 14:08:32 -0500 Subject: [PATCH 19/50] test: Add option to skip unit tests for the test runner --- test/functional/test_runner.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index e438a60edc..9f69fd898d 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -434,6 +434,8 @@ def main(): parser.add_argument('--tmpdirprefix', '-t', default=tempfile.gettempdir(), help="Root directory for datadirs") 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') + args, unknown_args = parser.parse_known_args() if not args.ansi: @@ -544,9 +546,10 @@ def main(): combined_logs_len=args.combinedlogslen, failfast=args.failfast, use_term_control=args.ansi, + skipunit=args.skipunit, ) -def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, enable_coverage=False, args=None, combined_logs_len=0, failfast=False, use_term_control): +def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, enable_coverage=False, args=None, combined_logs_len=0, failfast=False, use_term_control, skipunit=False): args = args or [] # Warn if bitcoind is already running @@ -563,20 +566,20 @@ def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, enable_coverage= if os.path.isdir(cache_dir): print("%sWARNING!%s There is a cache directory here: %s. If tests fail unexpectedly, try deleting the cache directory." % (BOLD[1], BOLD[0], cache_dir)) - # Test Framework Tests - print("Running Unit Tests for Test Framework Modules") tests_dir = src_dir + '/test/functional/' # This allows `test_runner.py` to work from an out-of-source build directory using a symlink, # a hard link or a copy on any platform. See https://github.com/bitcoin/bitcoin/pull/27561. sys.path.append(tests_dir) - test_framework_tests = unittest.TestSuite() - for module in TEST_FRAMEWORK_MODULES: - test_framework_tests.addTest(unittest.TestLoader().loadTestsFromName("test_framework.{}".format(module))) - result = unittest.TextTestRunner(verbosity=1, failfast=True).run(test_framework_tests) - if not result.wasSuccessful(): - sys.exit("Early exiting after failure in TestFramework unit tests") + if not skipunit: + print("Running Unit Tests for Test Framework Modules") + test_framework_tests = unittest.TestSuite() + for module in TEST_FRAMEWORK_MODULES: + test_framework_tests.addTest(unittest.TestLoader().loadTestsFromName("test_framework.{}".format(module))) + result = unittest.TextTestRunner(verbosity=1, failfast=True).run(test_framework_tests) + if not result.wasSuccessful(): + sys.exit("Early exiting after failure in TestFramework unit tests") flags = ['--cachedir={}'.format(cache_dir)] + args From d2fe90571e98e02617682561ea82acb1e2647827 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Wed, 31 Jan 2024 17:14:52 +0000 Subject: [PATCH 20/50] test: Drop `x` modifier in `fsbridge::fopen` call for mingw builds The MinGW-w64 toolchain links executables to the old msvcrt C Runtime Library that does not support the `x` modifier for the _wfopen() function. --- src/test/streams_tests.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index 7d1ac5a19a..0903f987f6 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -29,7 +29,14 @@ BOOST_AUTO_TEST_CASE(xor_file) BOOST_CHECK_EXCEPTION(xor_file.ignore(1), std::ios_base::failure, HasReason{"AutoFile::ignore: file handle is nullpt"}); } { - AutoFile xor_file{raw_file("wbx"), xor_pat}; +#ifdef __MINGW64__ + // Our usage of mingw-w64 and the msvcrt runtime does not support + // the x modifier for the _wfopen(). + const char* mode = "wb"; +#else + const char* mode = "wbx"; +#endif + AutoFile xor_file{raw_file(mode), xor_pat}; xor_file << test1 << test2; } { From fccfdb25b2c337bbd8b283c27493f10d5e02b5d4 Mon Sep 17 00:00:00 2001 From: Jesse Barton Date: Mon, 26 Feb 2024 14:32:52 +0000 Subject: [PATCH 21/50] doc: Update OpenBSD build docs to 7.4 Tested and used all build options on OpenBSD 7.4 with no issues. Added a note about referring to depends/README.md for detailed instructions on required dependencies. This was added in reference to a conversation in #29443 --- doc/build-openbsd.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/build-openbsd.md b/doc/build-openbsd.md index 96ee714341..7ed83853a8 100644 --- a/doc/build-openbsd.md +++ b/doc/build-openbsd.md @@ -1,6 +1,6 @@ # OpenBSD Build Guide -**Updated for OpenBSD [7.3](https://www.openbsd.org/73.html)** +**Updated for OpenBSD [7.4](https://www.openbsd.org/74.html)** This guide describes how to build bitcoind, command-line utilities, and GUI on OpenBSD. @@ -43,6 +43,8 @@ BerkeleyDB is only required to support legacy wallets. It is recommended to use Berkeley DB 4.8. You cannot use the BerkeleyDB library from ports. However you can build it yourself, [using depends](/depends). +Refer to [depends/README.md](/depends/README.md) for detailed instructions. + ```bash gmake -C depends NO_BOOST=1 NO_LIBEVENT=1 NO_QT=1 NO_SQLITE=1 NO_NATPMP=1 NO_UPNP=1 NO_ZMQ=1 NO_USDT=1 ... From 52f9bba889fd9b50a0543fd9fedc389592cdc7e5 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Fri, 8 Dec 2023 19:10:18 +0000 Subject: [PATCH 22/50] crypto: replace non-standard CLZ builtins with c++20's bit_width Also some header cleanups. --- src/crypto/common.h | 28 +++++----------------------- 1 file changed, 5 insertions(+), 23 deletions(-) diff --git a/src/crypto/common.h b/src/crypto/common.h index 6ae5d4cd24..18d986f429 100644 --- a/src/crypto/common.h +++ b/src/crypto/common.h @@ -5,15 +5,12 @@ #ifndef BITCOIN_CRYPTO_COMMON_H #define BITCOIN_CRYPTO_COMMON_H -#if defined(HAVE_CONFIG_H) -#include -#endif - -#include -#include - #include +#include +#include +#include + uint16_t static inline ReadLE16(const unsigned char* ptr) { uint16_t x; @@ -89,22 +86,7 @@ void static inline WriteBE64(unsigned char* ptr, uint64_t x) /** Return the smallest number n such that (x >> n) == 0 (or 64 if the highest bit in x is set. */ uint64_t static inline CountBits(uint64_t x) { -#if HAVE_BUILTIN_CLZL - if (sizeof(unsigned long) >= sizeof(uint64_t)) { - return x ? 8 * sizeof(unsigned long) - __builtin_clzl(x) : 0; - } -#endif -#if HAVE_BUILTIN_CLZLL - if (sizeof(unsigned long long) >= sizeof(uint64_t)) { - return x ? 8 * sizeof(unsigned long long) - __builtin_clzll(x) : 0; - } -#endif - int ret = 0; - while (x) { - x >>= 1; - ++ret; - } - return ret; + return std::bit_width(x); } #endif // BITCOIN_CRYPTO_COMMON_H From 297367b3bb062c57142747719ac9bf2e12717ce9 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 12 Dec 2023 18:44:49 +0000 Subject: [PATCH 23/50] crypto: replace CountBits with std::bit_width bit_width is a drop-in replacement with an exact meaning in c++, so there is no need to continue testing/fuzzing/benchmarking. --- src/crypto/common.h | 7 ------- src/random.h | 3 ++- src/test/crypto_tests.cpp | 22 ---------------------- src/test/fuzz/integer.cpp | 1 - src/util/asmap.cpp | 6 +++--- 5 files changed, 5 insertions(+), 34 deletions(-) diff --git a/src/crypto/common.h b/src/crypto/common.h index 18d986f429..42f72a6fa8 100644 --- a/src/crypto/common.h +++ b/src/crypto/common.h @@ -7,7 +7,6 @@ #include -#include #include #include @@ -83,10 +82,4 @@ void static inline WriteBE64(unsigned char* ptr, uint64_t x) memcpy(ptr, &v, 8); } -/** Return the smallest number n such that (x >> n) == 0 (or 64 if the highest bit in x is set. */ -uint64_t static inline CountBits(uint64_t x) -{ - return std::bit_width(x); -} - #endif // BITCOIN_CRYPTO_COMMON_H diff --git a/src/random.h b/src/random.h index 76bae5838d..f7c20ee4b0 100644 --- a/src/random.h +++ b/src/random.h @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -203,7 +204,7 @@ class FastRandomContext { assert(range); --range; - int bits = CountBits(range); + int bits = std::bit_width(range); while (true) { uint64_t ret = randbits(bits); if (ret <= range) return ret; diff --git a/src/test/crypto_tests.cpp b/src/test/crypto_tests.cpp index 0a6378adf4..46acc6fc9f 100644 --- a/src/test/crypto_tests.cpp +++ b/src/test/crypto_tests.cpp @@ -1060,28 +1060,6 @@ BOOST_AUTO_TEST_CASE(hkdf_hmac_sha256_l32_tests) "8da4e775a563c18f715f802a063c5a31b8a11f5c5ee1879ec3454e5f3c738d2d"); } -BOOST_AUTO_TEST_CASE(countbits_tests) -{ - FastRandomContext ctx; - for (unsigned int i = 0; i <= 64; ++i) { - if (i == 0) { - // Check handling of zero. - BOOST_CHECK_EQUAL(CountBits(0), 0U); - } else if (i < 10) { - for (uint64_t j = uint64_t{1} << (i - 1); (j >> i) == 0; ++j) { - // Exhaustively test up to 10 bits - BOOST_CHECK_EQUAL(CountBits(j), i); - } - } else { - for (int k = 0; k < 1000; k++) { - // Randomly test 1000 samples of each length above 10 bits. - uint64_t j = (uint64_t{1}) << (i - 1) | ctx.randbits(i - 1); - BOOST_CHECK_EQUAL(CountBits(j), i); - } - } - } -} - BOOST_AUTO_TEST_CASE(sha256d64) { for (int i = 0; i <= 32; ++i) { diff --git a/src/test/fuzz/integer.cpp b/src/test/fuzz/integer.cpp index 2577f9e97a..db246bb84e 100644 --- a/src/test/fuzz/integer.cpp +++ b/src/test/fuzz/integer.cpp @@ -80,7 +80,6 @@ FUZZ_TARGET(integer, .init = initialize_integer) static const uint256 u256_max(uint256S("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")); const std::vector v256{u256, u256_min, u256_max}; (void)ComputeMerkleRoot(v256); - (void)CountBits(u64); (void)DecompressAmount(u64); { if (std::optional parsed = ParseMoney(FormatMoney(i64))) { diff --git a/src/util/asmap.cpp b/src/util/asmap.cpp index 360573cbae..f50cd8a28c 100644 --- a/src/util/asmap.cpp +++ b/src/util/asmap.cpp @@ -5,13 +5,13 @@ #include #include -#include #include #include #include #include #include +#include #include #include #include @@ -111,7 +111,7 @@ uint32_t Interpret(const std::vector &asmap, const std::vector &ip) } else if (opcode == Instruction::MATCH) { match = DecodeMatch(pos, endpos); if (match == INVALID) break; // Match bits straddle EOF - matchlen = CountBits(match) - 1; + matchlen = std::bit_width(match) - 1; if (bits < matchlen) break; // Not enough input bits for (uint32_t bit = 0; bit < matchlen; bit++) { if ((ip[ip.size() - bits]) != ((match >> (matchlen - 1 - bit)) & 1)) { @@ -175,7 +175,7 @@ bool SanityCheckASMap(const std::vector& asmap, int bits) } else if (opcode == Instruction::MATCH) { uint32_t match = DecodeMatch(pos, endpos); if (match == INVALID) return false; // Match bits straddle EOF - int matchlen = CountBits(match) - 1; + int matchlen = std::bit_width(match) - 1; if (prevopcode != Instruction::MATCH) had_incomplete_match = false; if (matchlen < 8 && had_incomplete_match) return false; // Within a sequence of matches only at most one should be incomplete had_incomplete_match = (matchlen < 8); From b052b2d1f2b220582a933eb5fa6a28144bed07d8 Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 17 Jul 2023 13:48:31 +0100 Subject: [PATCH 24/50] build: remove -Wdocumentation conditional Now that --enable-suppress-external-warnings is on by default, we can drop it. --- configure.ac | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/configure.ac b/configure.ac index 50e6870dd9..4a7c02c1e1 100644 --- a/configure.ac +++ b/configure.ac @@ -432,10 +432,7 @@ if test "$CXXFLAGS_overridden" = "no"; then AX_CHECK_COMPILE_FLAG([-Wsuggest-override], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsuggest-override"], [], [$CXXFLAG_WERROR]) AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wimplicit-fallthrough"], [], [$CXXFLAG_WERROR]) AX_CHECK_COMPILE_FLAG([-Wunreachable-code], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunreachable-code"], [], [$CXXFLAG_WERROR]) - - if test "$suppress_external_warnings" != "no" ; then - AX_CHECK_COMPILE_FLAG([-Wdocumentation], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdocumentation"], [], [$CXXFLAG_WERROR]) - fi + AX_CHECK_COMPILE_FLAG([-Wdocumentation], [WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdocumentation"], [], [$CXXFLAG_WERROR]) dnl Some compilers (gcc) ignore unknown -Wno-* options, but warn about all dnl unknown options if any other warning is produced. Test the -Wfoo case, and From 95bddb930aa72edd40fdff52cf447202995b0dce Mon Sep 17 00:00:00 2001 From: dergoegge Date: Fri, 19 Jan 2024 11:49:48 +0000 Subject: [PATCH 25/50] [validation] Isolate merkle root checks --- src/validation.cpp | 90 ++++++++++++++++++++++++++-------------------- 1 file changed, 52 insertions(+), 38 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index caa4ff3115..98383cd133 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3639,6 +3639,54 @@ static bool CheckBlockHeader(const CBlockHeader& block, BlockValidationState& st return true; } +static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state) +{ + bool mutated; + uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated); + if (block.hashMerkleRoot != hashMerkleRoot2) + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txnmrklroot", "hashMerkleRoot mismatch"); + + // Check for merkle tree malleability (CVE-2012-2459): repeating sequences + // of transactions in a block without affecting the merkle root of a block, + // while still invalidating it. + if (mutated) + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txns-duplicate", "duplicate transaction"); + + return true; +} + +static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_commitment, BlockValidationState& state) +{ + if (expect_witness_commitment) { + int commitpos = GetWitnessCommitmentIndex(block); + if (commitpos != NO_WITNESS_COMMITMENT) { + bool malleated = false; + uint256 hashWitness = BlockWitnessMerkleRoot(block, &malleated); + // The malleation check is ignored; as the transaction tree itself + // already does not permit it, it is impossible to trigger in the + // witness tree. + if (block.vtx[0]->vin[0].scriptWitness.stack.size() != 1 || block.vtx[0]->vin[0].scriptWitness.stack[0].size() != 32) { + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-nonce-size", strprintf("%s : invalid witness reserved value size", __func__)); + } + CHash256().Write(hashWitness).Write(block.vtx[0]->vin[0].scriptWitness.stack[0]).Finalize(hashWitness); + if (memcmp(hashWitness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) { + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-merkle-match", strprintf("%s : witness merkle commitment mismatch", __func__)); + } + + return true; + } + } + + // No witness data is allowed in blocks that don't commit to witness data, as this would otherwise leave room for spam + for (const auto& tx : block.vtx) { + if (tx->HasWitness()) { + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "unexpected-witness", strprintf("%s : unexpected witness data found", __func__)); + } + } + + return true; +} + bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot) { // These are checks that are independent of context. @@ -3657,17 +3705,8 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu } // Check the merkle root. - if (fCheckMerkleRoot) { - bool mutated; - uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated); - if (block.hashMerkleRoot != hashMerkleRoot2) - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txnmrklroot", "hashMerkleRoot mismatch"); - - // Check for merkle tree malleability (CVE-2012-2459): repeating sequences - // of transactions in a block without affecting the merkle root of a block, - // while still invalidating it. - if (mutated) - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txns-duplicate", "duplicate transaction"); + if (fCheckMerkleRoot && !CheckMerkleRoot(block, state)) { + return false; } // All potential-corruption validation must be done before we do any @@ -3866,33 +3905,8 @@ static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& stat // * There must be at least one output whose scriptPubKey is a single 36-byte push, the first 4 bytes of which are // {0xaa, 0x21, 0xa9, 0xed}, and the following 32 bytes are SHA256^2(witness root, witness reserved value). In case there are // multiple, the last one is used. - bool fHaveWitness = false; - if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_SEGWIT)) { - int commitpos = GetWitnessCommitmentIndex(block); - if (commitpos != NO_WITNESS_COMMITMENT) { - bool malleated = false; - uint256 hashWitness = BlockWitnessMerkleRoot(block, &malleated); - // The malleation check is ignored; as the transaction tree itself - // already does not permit it, it is impossible to trigger in the - // witness tree. - if (block.vtx[0]->vin[0].scriptWitness.stack.size() != 1 || block.vtx[0]->vin[0].scriptWitness.stack[0].size() != 32) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-nonce-size", strprintf("%s : invalid witness reserved value size", __func__)); - } - CHash256().Write(hashWitness).Write(block.vtx[0]->vin[0].scriptWitness.stack[0]).Finalize(hashWitness); - if (memcmp(hashWitness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-merkle-match", strprintf("%s : witness merkle commitment mismatch", __func__)); - } - fHaveWitness = true; - } - } - - // No witness data is allowed in blocks that don't commit to witness data, as this would otherwise leave room for spam - if (!fHaveWitness) { - for (const auto& tx : block.vtx) { - if (tx->HasWitness()) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "unexpected-witness", strprintf("%s : unexpected witness data found", __func__)); - } - } + if (!CheckWitnessMalleation(block, DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_SEGWIT), state)) { + return false; } // After the coinbase witness reserved value and commitment are verified, From e7669e1343aec4298fd30d539847963e6fa5619c Mon Sep 17 00:00:00 2001 From: dergoegge Date: Thu, 22 Feb 2024 11:27:49 +0000 Subject: [PATCH 26/50] [refactor] Cleanup merkle root checks --- src/validation.cpp | 55 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 98383cd133..e02ce2ad1d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3642,35 +3642,59 @@ static bool CheckBlockHeader(const CBlockHeader& block, BlockValidationState& st static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state) { bool mutated; - uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated); - if (block.hashMerkleRoot != hashMerkleRoot2) - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txnmrklroot", "hashMerkleRoot mismatch"); + uint256 merkle_root = BlockMerkleRoot(block, &mutated); + if (block.hashMerkleRoot != merkle_root) { + return state.Invalid( + /*result=*/BlockValidationResult::BLOCK_MUTATED, + /*reject_reason=*/"bad-txnmrklroot", + /*debug_message=*/"hashMerkleRoot mismatch"); + } // Check for merkle tree malleability (CVE-2012-2459): repeating sequences // of transactions in a block without affecting the merkle root of a block, // while still invalidating it. - if (mutated) - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txns-duplicate", "duplicate transaction"); + if (mutated) { + return state.Invalid( + /*result=*/BlockValidationResult::BLOCK_MUTATED, + /*reject_reason=*/"bad-txns-duplicate", + /*debug_message=*/"duplicate transaction"); + } return true; } +/** CheckWitnessMalleation performs checks for block malleation with regard to + * its witnesses. + * + * Note: If the witness commitment is expected (i.e. `expect_witness_commitment + * = true`), then the block is required to have at least one transaction and the + * first transaction needs to have at least one input. */ static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_commitment, BlockValidationState& state) { if (expect_witness_commitment) { int commitpos = GetWitnessCommitmentIndex(block); if (commitpos != NO_WITNESS_COMMITMENT) { - bool malleated = false; - uint256 hashWitness = BlockWitnessMerkleRoot(block, &malleated); + assert(!block.vtx.empty() && !block.vtx[0]->vin.empty()); + const auto& witness_stack{block.vtx[0]->vin[0].scriptWitness.stack}; + + if (witness_stack.size() != 1 || witness_stack[0].size() != 32) { + return state.Invalid( + /*result=*/BlockValidationResult::BLOCK_MUTATED, + /*reject_reason=*/"bad-witness-nonce-size", + /*debug_message=*/strprintf("%s : invalid witness reserved value size", __func__)); + } + // The malleation check is ignored; as the transaction tree itself // already does not permit it, it is impossible to trigger in the // witness tree. - if (block.vtx[0]->vin[0].scriptWitness.stack.size() != 1 || block.vtx[0]->vin[0].scriptWitness.stack[0].size() != 32) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-nonce-size", strprintf("%s : invalid witness reserved value size", __func__)); - } - CHash256().Write(hashWitness).Write(block.vtx[0]->vin[0].scriptWitness.stack[0]).Finalize(hashWitness); - if (memcmp(hashWitness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-merkle-match", strprintf("%s : witness merkle commitment mismatch", __func__)); + uint256 hash_witness = BlockWitnessMerkleRoot(block, /*mutated=*/nullptr); + + CHash256().Write(hash_witness).Write(witness_stack[0]).Finalize(hash_witness); + if (memcmp(hash_witness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) { + return state.Invalid( + /*result=*/BlockValidationResult::BLOCK_MUTATED, + /*reject_reason=*/"bad-witness-merkle-match", + /*debug_message=*/strprintf("%s : witness merkle commitment mismatch", __func__)); } return true; @@ -3680,7 +3704,10 @@ static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_comm // No witness data is allowed in blocks that don't commit to witness data, as this would otherwise leave room for spam for (const auto& tx : block.vtx) { if (tx->HasWitness()) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "unexpected-witness", strprintf("%s : unexpected witness data found", __func__)); + return state.Invalid( + /*result=*/BlockValidationResult::BLOCK_MUTATED, + /*reject_reason=*/"unexpected-witness", + /*debug_message=*/strprintf("%s : unexpected witness data found", __func__)); } } From 66abce1d98115e41f394bc4f4f52341960ddc839 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Fri, 19 Jan 2024 15:09:28 +0000 Subject: [PATCH 27/50] [validation] Introduce IsBlockMutated --- src/validation.cpp | 20 ++++++++++++++++++++ src/validation.h | 3 +++ 2 files changed, 23 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index e02ce2ad1d..e1133138df 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3824,6 +3824,26 @@ bool HasValidProofOfWork(const std::vector& headers, const Consens [&](const auto& header) { return CheckProofOfWork(header.GetHash(), header.nBits, consensusParams);}); } +bool IsBlockMutated(const CBlock& block, bool check_witness_root) +{ + BlockValidationState state; + if (!CheckMerkleRoot(block, state)) { + LogDebug(BCLog::VALIDATION, "Block mutated: %s\n", state.ToString()); + return true; + } + + if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { + return false; + } + + if (!CheckWitnessMalleation(block, check_witness_root, state)) { + LogDebug(BCLog::VALIDATION, "Block mutated: %s\n", state.ToString()); + return true; + } + + return false; +} + arith_uint256 CalculateHeadersWork(const std::vector& headers) { arith_uint256 total_work{0}; diff --git a/src/validation.h b/src/validation.h index fd9b53df8f..566153c150 100644 --- a/src/validation.h +++ b/src/validation.h @@ -383,6 +383,9 @@ bool TestBlockValidity(BlockValidationState& state, /** Check with the proof of work on each blockheader matches the value in nBits */ bool HasValidProofOfWork(const std::vector& headers, const Consensus::Params& consensusParams); +/** Check if a block has been mutated (with respect to its merkle root and witness commitments). */ +bool IsBlockMutated(const CBlock& block, bool check_witness_root); + /** Return the sum of the work on a given set of headers */ arith_uint256 CalculateHeadersWork(const std::vector& headers); From 2d8495e0800f5332748cd50eaad801ff77671bba Mon Sep 17 00:00:00 2001 From: dergoegge Date: Tue, 6 Feb 2024 17:07:48 +0000 Subject: [PATCH 28/50] [validation] Merkle root malleation should be caught by IsBlockMutated --- src/test/validation_tests.cpp | 5 +++++ src/validation.cpp | 13 ++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index 14440571eb..cb7d1a312f 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -4,12 +4,17 @@ #include #include +#include +#include +#include #include #include #include #include #include +#include + #include #include diff --git a/src/validation.cpp b/src/validation.cpp index e1133138df..120663bfc0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3833,7 +3833,18 @@ bool IsBlockMutated(const CBlock& block, bool check_witness_root) } if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { - return false; + // Consider the block mutated if any transaction is 64 bytes in size (see 3.1 + // in "Weaknesses in Bitcoin’s Merkle Root Construction": + // https://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20190225/a27d8837/attachment-0001.pdf). + // + // Note: This is not a consensus change as this only applies to blocks that + // don't have a coinbase transaction and would therefore already be invalid. + return std::any_of(block.vtx.begin(), block.vtx.end(), + [](auto& tx) { return GetSerializeSize(TX_NO_WITNESS(tx)) == 64; }); + } else { + // Theoretically it is still possible for a block with a 64 byte + // coinbase transaction to be mutated but we neglect that possibility + // here as it requires at least 224 bits of work. } if (!CheckWitnessMalleation(block, check_witness_root, state)) { From 49257c0304828a185c273fcb99742c54bbef0c8e Mon Sep 17 00:00:00 2001 From: dergoegge Date: Fri, 19 Jan 2024 15:25:01 +0000 Subject: [PATCH 29/50] [net processing] Don't process mutated blocks We preemptively perform a block mutation check before further processing a block message (similar to early sanity checks on other messsage types). The main reasons for this change are as follows: - `CBlock::GetHash()` is a foot-gun without a prior mutation check, as the hash returned only commits to the header but not to the actual transactions (`CBlock::vtx`) contained in the block. - We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. https://github.com/bitcoin/bitcoin/pull/27608). --- src/net_processing.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c8da927763..5c3ec5f700 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4719,6 +4719,16 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, LogPrint(BCLog::NET, "received block %s peer=%d\n", pblock->GetHash().ToString(), pfrom.GetId()); + const CBlockIndex* prev_block{WITH_LOCK(m_chainman.GetMutex(), return m_chainman.m_blockman.LookupBlockIndex(pblock->hashPrevBlock))}; + + if (IsBlockMutated(/*block=*/*pblock, + /*check_witness_root=*/DeploymentActiveAfter(prev_block, m_chainman, Consensus::DEPLOYMENT_SEGWIT))) { + LogDebug(BCLog::NET, "Received mutated block from peer=%d\n", peer->m_id); + Misbehaving(*peer, 100, "mutated block"); + WITH_LOCK(cs_main, RemoveBlockRequest(pblock->GetHash(), peer->m_id)); + return; + } + bool forceProcessing = false; const uint256 hash(pblock->GetHash()); bool min_pow_checked = false; @@ -4734,7 +4744,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, mapBlockSource.emplace(hash, std::make_pair(pfrom.GetId(), true)); // Check work on this block against our anti-dos thresholds. - const CBlockIndex* prev_block = m_chainman.m_blockman.LookupBlockIndex(pblock->hashPrevBlock); if (prev_block && prev_block->nChainWork + CalculateHeadersWork({pblock->GetBlockHeader()}) >= GetAntiDoSWorkThreshold()) { min_pow_checked = true; } From 5bf4f5ba32da4627f152b54d266df9b2aa930457 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Fri, 2 Feb 2024 16:16:55 +0000 Subject: [PATCH 30/50] [test] Add regression test for #27608 --- test/functional/p2p_mutated_blocks.py | 96 +++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 97 insertions(+) create mode 100755 test/functional/p2p_mutated_blocks.py diff --git a/test/functional/p2p_mutated_blocks.py b/test/functional/p2p_mutated_blocks.py new file mode 100755 index 0000000000..20f618dc6e --- /dev/null +++ b/test/functional/p2p_mutated_blocks.py @@ -0,0 +1,96 @@ +#!/usr/bin/env python3 +# Copyright (c) The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +""" +Test that an attacker can't degrade compact block relay by sending unsolicited +mutated blocks to clear in-flight blocktxn requests from other honest peers. +""" + +from test_framework.p2p import P2PInterface +from test_framework.messages import ( + BlockTransactions, + msg_cmpctblock, + msg_block, + msg_blocktxn, + HeaderAndShortIDs, +) +from test_framework.test_framework import BitcoinTestFramework +from test_framework.blocktools import ( + COINBASE_MATURITY, + create_block, + add_witness_commitment, + NORMAL_GBT_REQUEST_PARAMS, +) +from test_framework.util import assert_equal +from test_framework.wallet import MiniWallet +import copy + +class MutatedBlocksTest(BitcoinTestFramework): + def set_test_params(self): + self.setup_clean_chain = True + self.num_nodes = 1 + + def run_test(self): + self.wallet = MiniWallet(self.nodes[0]) + self.generate(self.wallet, COINBASE_MATURITY) + + honest_relayer = self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=0, connection_type="outbound-full-relay") + attacker = self.nodes[0].add_p2p_connection(P2PInterface()) + + # Create new block with two transactions (coinbase + 1 self-transfer). + # The self-transfer transaction is needed to trigger a compact block + # `getblocktxn` roundtrip. + tx = self.wallet.create_self_transfer()["tx"] + block = create_block(tmpl=self.nodes[0].getblocktemplate(NORMAL_GBT_REQUEST_PARAMS), txlist=[tx]) + add_witness_commitment(block) + block.solve() + + # Create mutated version of the block by changing the transaction + # version on the self-transfer. + mutated_block = copy.deepcopy(block) + mutated_block.vtx[1].nVersion = 4 + + # Announce the new block via a compact block through the honest relayer + cmpctblock = HeaderAndShortIDs() + cmpctblock.initialize_from_block(block, use_witness=True) + honest_relayer.send_message(msg_cmpctblock(cmpctblock.to_p2p())) + + # Wait for a `getblocktxn` that attempts to fetch the self-transfer + def self_transfer_requested(): + if not honest_relayer.last_message.get('getblocktxn'): + return False + + get_block_txn = honest_relayer.last_message['getblocktxn'] + return get_block_txn.block_txn_request.blockhash == block.sha256 and \ + get_block_txn.block_txn_request.indexes == [1] + honest_relayer.wait_until(self_transfer_requested, timeout=5) + + # Block at height 101 should be the only one in flight from peer 0 + peer_info_prior_to_attack = self.nodes[0].getpeerinfo() + assert_equal(peer_info_prior_to_attack[0]['id'], 0) + assert_equal([101], peer_info_prior_to_attack[0]["inflight"]) + + # Attempt to clear the honest relayer's download request by sending the + # mutated block (as the attacker). + with self.nodes[0].assert_debug_log(expected_msgs=["bad-txnmrklroot, hashMerkleRoot mismatch"]): + attacker.send_message(msg_block(mutated_block)) + # Attacker should get disconnected for sending a mutated block + attacker.wait_for_disconnect(timeout=5) + + # Block at height 101 should *still* be the only block in-flight from + # peer 0 + peer_info_after_attack = self.nodes[0].getpeerinfo() + assert_equal(peer_info_after_attack[0]['id'], 0) + assert_equal([101], peer_info_after_attack[0]["inflight"]) + + # The honest relayer should be able to complete relaying the block by + # sending the blocktxn that was requested. + block_txn = msg_blocktxn() + block_txn.block_transactions = BlockTransactions(blockhash=block.sha256, transactions=[tx]) + honest_relayer.send_and_ping(block_txn) + assert_equal(self.nodes[0].getbestblockhash(), block.hash) + +if __name__ == '__main__': + MutatedBlocksTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 9bfc9aa7d4..a852a0d65c 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -306,6 +306,7 @@ 'wallet_crosschain.py', 'mining_basic.py', 'feature_signet.py', + 'p2p_mutated_blocks.py', 'wallet_implicitsegwit.py --legacy-wallet', 'rpc_named_arguments.py', 'feature_startupnotify.py', From 1ec6bbeb8d27d31647d1433ccb87b362f6d81f90 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Tue, 6 Feb 2024 10:07:18 +0000 Subject: [PATCH 31/50] [validation] Cache merkle root and witness commitment checks Slight performance improvement by avoiding duplicate work. --- src/primitives/block.h | 8 ++++++-- src/validation.cpp | 6 ++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/primitives/block.h b/src/primitives/block.h index 99accfc7dd..832f8a03f7 100644 --- a/src/primitives/block.h +++ b/src/primitives/block.h @@ -71,8 +71,10 @@ class CBlock : public CBlockHeader // network and disk std::vector vtx; - // memory only - mutable bool fChecked; + // Memory-only flags for caching expensive checks + mutable bool fChecked; // CheckBlock() + mutable bool m_checked_witness_commitment{false}; // CheckWitnessCommitment() + mutable bool m_checked_merkle_root{false}; // CheckMerkleRoot() CBlock() { @@ -95,6 +97,8 @@ class CBlock : public CBlockHeader CBlockHeader::SetNull(); vtx.clear(); fChecked = false; + m_checked_witness_commitment = false; + m_checked_merkle_root = false; } CBlockHeader GetBlockHeader() const diff --git a/src/validation.cpp b/src/validation.cpp index 120663bfc0..b03cc7f78a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3641,6 +3641,8 @@ static bool CheckBlockHeader(const CBlockHeader& block, BlockValidationState& st static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state) { + if (block.m_checked_merkle_root) return true; + bool mutated; uint256 merkle_root = BlockMerkleRoot(block, &mutated); if (block.hashMerkleRoot != merkle_root) { @@ -3660,6 +3662,7 @@ static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state) /*debug_message=*/"duplicate transaction"); } + block.m_checked_merkle_root = true; return true; } @@ -3672,6 +3675,8 @@ static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state) static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_commitment, BlockValidationState& state) { if (expect_witness_commitment) { + if (block.m_checked_witness_commitment) return true; + int commitpos = GetWitnessCommitmentIndex(block); if (commitpos != NO_WITNESS_COMMITMENT) { assert(!block.vtx.empty() && !block.vtx[0]->vin.empty()); @@ -3697,6 +3702,7 @@ static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_comm /*debug_message=*/strprintf("%s : witness merkle commitment mismatch", __func__)); } + block.m_checked_witness_commitment = true; return true; } } From 1ed2c9829700054526156297552bb47230406098 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Tue, 27 Feb 2024 11:57:26 +0000 Subject: [PATCH 32/50] Add transaction_identifier::size to allow Span conversion --- src/util/transaction_identifier.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h index 89e10dee01..d4a0ede25a 100644 --- a/src/util/transaction_identifier.h +++ b/src/util/transaction_identifier.h @@ -44,6 +44,7 @@ class transaction_identifier constexpr void SetNull() { m_wrapped.SetNull(); } std::string GetHex() const { return m_wrapped.GetHex(); } std::string ToString() const { return m_wrapped.ToString(); } + static constexpr auto size() { return decltype(m_wrapped)::size(); } constexpr const std::byte* data() const { return reinterpret_cast(m_wrapped.data()); } constexpr const std::byte* begin() const { return reinterpret_cast(m_wrapped.begin()); } constexpr const std::byte* end() const { return reinterpret_cast(m_wrapped.end()); } From d8087adc7ebd4ea05dd3843e5a92e8115fd7bbcc Mon Sep 17 00:00:00 2001 From: dergoegge Date: Fri, 23 Feb 2024 15:50:36 +0000 Subject: [PATCH 33/50] [test] IsBlockMutated unit tests --- src/test/validation_tests.cpp | 211 ++++++++++++++++++++++++++++++++++ 1 file changed, 211 insertions(+) diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index cb7d1a312f..c6f6ac3bdb 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -150,4 +150,215 @@ BOOST_AUTO_TEST_CASE(test_assumeutxo) BOOST_CHECK_EQUAL(out110_2.nChainTx, 111U); } +BOOST_AUTO_TEST_CASE(block_malleation) +{ + // Test utilities that calls `IsBlockMutated` and then clears the validity + // cache flags on `CBlock`. + auto is_mutated = [](CBlock& block, bool check_witness_root) { + bool mutated{IsBlockMutated(block, check_witness_root)}; + block.fChecked = false; + block.m_checked_witness_commitment = false; + block.m_checked_merkle_root = false; + return mutated; + }; + auto is_not_mutated = [&is_mutated](CBlock& block, bool check_witness_root) { + return !is_mutated(block, check_witness_root); + }; + + // Test utilities to create coinbase transactions and insert witness + // commitments. + // + // Note: this will not include the witness stack by default to avoid + // triggering the "no witnesses allowed for blocks that don't commit to + // witnesses" rule when testing other malleation vectors. + auto create_coinbase_tx = [](bool include_witness = false) { + CMutableTransaction coinbase; + coinbase.vin.resize(1); + if (include_witness) { + coinbase.vin[0].scriptWitness.stack.resize(1); + coinbase.vin[0].scriptWitness.stack[0] = std::vector(32, 0x00); + } + + coinbase.vout.resize(1); + coinbase.vout[0].scriptPubKey.resize(MINIMUM_WITNESS_COMMITMENT); + coinbase.vout[0].scriptPubKey[0] = OP_RETURN; + coinbase.vout[0].scriptPubKey[1] = 0x24; + coinbase.vout[0].scriptPubKey[2] = 0xaa; + coinbase.vout[0].scriptPubKey[3] = 0x21; + coinbase.vout[0].scriptPubKey[4] = 0xa9; + coinbase.vout[0].scriptPubKey[5] = 0xed; + + auto tx = MakeTransactionRef(coinbase); + assert(tx->IsCoinBase()); + return tx; + }; + auto insert_witness_commitment = [](CBlock& block, uint256 commitment) { + assert(!block.vtx.empty() && block.vtx[0]->IsCoinBase() && !block.vtx[0]->vout.empty()); + + CMutableTransaction mtx{*block.vtx[0]}; + CHash256().Write(commitment).Write(std::vector(32, 0x00)).Finalize(commitment); + memcpy(&mtx.vout[0].scriptPubKey[6], commitment.begin(), 32); + block.vtx[0] = MakeTransactionRef(mtx); + }; + + { + CBlock block; + + // Empty block is expected to have merkle root of 0x0. + BOOST_CHECK(block.vtx.empty()); + block.hashMerkleRoot = uint256{1}; + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + block.hashMerkleRoot = uint256{}; + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false)); + + // Block with a single coinbase tx is mutated if the merkle root is not + // equal to the coinbase tx's hash. + block.vtx.push_back(create_coinbase_tx()); + BOOST_CHECK(block.vtx[0]->GetHash() != block.hashMerkleRoot); + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + block.hashMerkleRoot = block.vtx[0]->GetHash(); + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false)); + + // Block with two transactions is mutated if the merkle root does not + // match the double sha256 of the concatenation of the two transaction + // hashes. + block.vtx.push_back(MakeTransactionRef(CMutableTransaction{})); + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + HashWriter hasher; + hasher.write(block.vtx[0]->GetHash()); + hasher.write(block.vtx[1]->GetHash()); + block.hashMerkleRoot = hasher.GetHash(); + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false)); + + // Block with two transactions is mutated if any node is duplicate. + { + block.vtx[1] = block.vtx[0]; + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + HashWriter hasher; + hasher.write(block.vtx[0]->GetHash()); + hasher.write(block.vtx[1]->GetHash()); + block.hashMerkleRoot = hasher.GetHash(); + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + } + + // Blocks with 64-byte coinbase transactions are not considered mutated + block.vtx.clear(); + { + CMutableTransaction mtx; + mtx.vin.resize(1); + mtx.vout.resize(1); + mtx.vout[0].scriptPubKey.resize(4); + block.vtx.push_back(MakeTransactionRef(mtx)); + block.hashMerkleRoot = block.vtx.back()->GetHash(); + assert(block.vtx.back()->IsCoinBase()); + assert(GetSerializeSize(TX_NO_WITNESS(block.vtx.back())) == 64); + } + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false)); + } + + { + // Test merkle root malleation + + // Pseudo code to mine transactions tx{1,2,3}: + // + // ``` + // loop { + // tx1 = random_tx() + // tx2 = random_tx() + // tx3 = deserialize_tx(txid(tx1) || txid(tx2)); + // if serialized_size_without_witness(tx3) == 64 { + // print(hex(tx3)) + // break + // } + // } + // ``` + // + // The `random_tx` function used to mine the txs below simply created + // empty transactions with a random version field. + CMutableTransaction tx1; + BOOST_CHECK(DecodeHexTx(tx1, "ff204bd0000000000000", /*try_no_witness=*/true, /*try_witness=*/false)); + CMutableTransaction tx2; + BOOST_CHECK(DecodeHexTx(tx2, "8ae53c92000000000000", /*try_no_witness=*/true, /*try_witness=*/false)); + CMutableTransaction tx3; + BOOST_CHECK(DecodeHexTx(tx3, "cdaf22d00002c6a7f848f8ae4d30054e61dcf3303d6fe01d282163341f06feecc10032b3160fcab87bdfe3ecfb769206ef2d991b92f8a268e423a6ef4d485f06", /*try_no_witness=*/true, /*try_witness=*/false)); + { + // Verify that double_sha256(txid1||txid2) == txid3 + HashWriter hasher; + hasher.write(tx1.GetHash()); + hasher.write(tx2.GetHash()); + assert(hasher.GetHash() == tx3.GetHash()); + // Verify that tx3 is 64 bytes in size (without witness). + assert(GetSerializeSize(TX_NO_WITNESS(tx3)) == 64); + } + + CBlock block; + block.vtx.push_back(MakeTransactionRef(tx1)); + block.vtx.push_back(MakeTransactionRef(tx2)); + uint256 merkle_root = block.hashMerkleRoot = BlockMerkleRoot(block); + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false)); + + // Mutate the block by replacing the two transactions with one 64-byte + // transaction that serializes into the concatenation of the txids of + // the transactions in the unmutated block. + block.vtx.clear(); + block.vtx.push_back(MakeTransactionRef(tx3)); + BOOST_CHECK(!block.vtx.back()->IsCoinBase()); + BOOST_CHECK(BlockMerkleRoot(block) == merkle_root); + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + } + + { + CBlock block; + block.vtx.push_back(create_coinbase_tx(/*include_witness=*/true)); + { + CMutableTransaction mtx; + mtx.vin.resize(1); + mtx.vin[0].scriptWitness.stack.resize(1); + mtx.vin[0].scriptWitness.stack[0] = {0}; + block.vtx.push_back(MakeTransactionRef(mtx)); + } + block.hashMerkleRoot = BlockMerkleRoot(block); + // Block with witnesses is considered mutated if the witness commitment + // is not validated. + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + // Block with invalid witness commitment is considered mutated. + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/true)); + + // Block with valid commitment is not mutated + { + auto commitment{BlockWitnessMerkleRoot(block)}; + insert_witness_commitment(block, commitment); + block.hashMerkleRoot = BlockMerkleRoot(block); + } + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/true)); + + // Malleating witnesses should be caught by `IsBlockMutated`. + { + CMutableTransaction mtx{*block.vtx[1]}; + assert(!mtx.vin[0].scriptWitness.stack[0].empty()); + ++mtx.vin[0].scriptWitness.stack[0][0]; + block.vtx[1] = MakeTransactionRef(mtx); + } + // Without also updating the witness commitment, the merkle root should + // not change when changing one of the witnesses. + BOOST_CHECK(block.hashMerkleRoot == BlockMerkleRoot(block)); + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/true)); + { + auto commitment{BlockWitnessMerkleRoot(block)}; + insert_witness_commitment(block, commitment); + block.hashMerkleRoot = BlockMerkleRoot(block); + } + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/true)); + + // Test malleating the coinbase witness reserved value + { + CMutableTransaction mtx{*block.vtx[0]}; + mtx.vin[0].scriptWitness.stack.resize(0); + block.vtx[0] = MakeTransactionRef(mtx); + block.hashMerkleRoot = BlockMerkleRoot(block); + } + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/true)); + } +} + BOOST_AUTO_TEST_SUITE_END() From 51bc1c7126d6e130bc40c529fb71ae6486da0492 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 27 Feb 2024 15:59:05 +0000 Subject: [PATCH 34/50] test: Remove Windows-specific code from `system_tests/run_command` This code has been dead since https://github.com/bitcoin/bitcoin/pull/28967. Required as a precondition for replacing Boost.Process with cpp-subprocess to make diff for this code meaningful and reviewable. The plan is to reintroduce Windows-specific code in this test simultaneously with enabling Windows support in cpp-subprocess. --- src/test/system_tests.cpp | 32 +------------------------------- 1 file changed, 1 insertion(+), 31 deletions(-) diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp index 76a8f80ba1..90fce9adf9 100644 --- a/src/test/system_tests.cpp +++ b/src/test/system_tests.cpp @@ -29,23 +29,12 @@ BOOST_AUTO_TEST_CASE(dummy) BOOST_AUTO_TEST_CASE(run_command) { -#ifdef WIN32 - // https://www.winehq.org/pipermail/wine-devel/2008-September/069387.html - auto hntdll = GetModuleHandleA("ntdll.dll"); - assert(hntdll); - const bool wine_runtime = GetProcAddress(hntdll, "wine_get_version"); -#endif - { const UniValue result = RunCommandParseJSON(""); BOOST_CHECK(result.isNull()); } { -#ifdef WIN32 - const UniValue result = RunCommandParseJSON("cmd.exe /c echo {\"success\": true}"); -#else const UniValue result = RunCommandParseJSON("echo \"{\"success\": true}\""); -#endif BOOST_CHECK(result.isObject()); const UniValue& success = result.find_value("success"); BOOST_CHECK(!success.isNull()); @@ -53,11 +42,7 @@ BOOST_AUTO_TEST_CASE(run_command) } { // An invalid command is handled by Boost -#ifdef WIN32 - const int expected_error{wine_runtime ? 6 : 2}; -#else const int expected_error{2}; -#endif BOOST_CHECK_EXCEPTION(RunCommandParseJSON("invalid_command"), boost::process::process_error, [&](const boost::process::process_error& e) { BOOST_CHECK(std::string(e.what()).find("RunCommandParseJSON error:") == std::string::npos); BOOST_CHECK_EQUAL(e.code().value(), expected_error); @@ -66,11 +51,7 @@ BOOST_AUTO_TEST_CASE(run_command) } { // Return non-zero exit code, no output to stderr -#ifdef WIN32 - const std::string command{"cmd.exe /c exit 1"}; -#else const std::string command{"false"}; -#endif BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) { const std::string what{e.what()}; BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned 1: \n", command)) != std::string::npos); @@ -79,13 +60,8 @@ BOOST_AUTO_TEST_CASE(run_command) } { // Return non-zero exit code, with error message for stderr -#ifdef WIN32 - const std::string command{"cmd.exe /c dir nosuchfile"}; - const std::string expected{wine_runtime ? "File not found." : "File Not Found"}; -#else const std::string command{"ls nosuchfile"}; const std::string expected{"No such file or directory"}; -#endif BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) { const std::string what(e.what()); BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned", command)) != std::string::npos); @@ -95,15 +71,10 @@ BOOST_AUTO_TEST_CASE(run_command) } { // Unable to parse JSON -#ifdef WIN32 - const std::string command{"cmd.exe /c echo {"}; -#else const std::string command{"echo {"}; -#endif BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, HasReason("Unable to parse JSON: {")); } - // Test std::in, except for Windows -#ifndef WIN32 + // Test std::in { const UniValue result = RunCommandParseJSON("cat", "{\"success\": true}"); BOOST_CHECK(result.isObject()); @@ -111,7 +82,6 @@ BOOST_AUTO_TEST_CASE(run_command) BOOST_CHECK(!success.isNull()); BOOST_CHECK_EQUAL(success.get_bool(), true); } -#endif } #endif // ENABLE_EXTERNAL_SIGNER From 5d45552fd4303f8d668ffbc50cce1053485aeead Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 27 Feb 2024 18:28:19 +0000 Subject: [PATCH 35/50] Squashed 'src/crc32c/' changes from 0bac72c455..b60d2b7334 b60d2b7334 Merge bitcoin-core/crc32c-subtree#6: Fix UBSan "misaligned-pointer-use" warning on aarch64 1ac401e32b Fix UBSan "misaligned-pointer-use" warning on aarch64 git-subtree-dir: src/crc32c git-subtree-split: b60d2b733406cc64025095c6c2cb3933e222b529 --- src/crc32c_arm64.cc | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/crc32c_arm64.cc b/src/crc32c_arm64.cc index 1da04ed34a..711616cd2f 100644 --- a/src/crc32c_arm64.cc +++ b/src/crc32c_arm64.cc @@ -12,6 +12,7 @@ #include #include +#include #include "./crc32c_internal.h" #ifdef CRC32C_HAVE_CONFIG_H @@ -29,14 +30,14 @@ // compute 8bytes for each segment parallelly #define CRC32C32BYTES(P, IND) \ do { \ - crc1 = __crc32cd( \ - crc1, *((const uint64_t *)(P) + (SEGMENTBYTES / 8) * 1 + (IND))); \ - crc2 = __crc32cd( \ - crc2, *((const uint64_t *)(P) + (SEGMENTBYTES / 8) * 2 + (IND))); \ - crc3 = __crc32cd( \ - crc3, *((const uint64_t *)(P) + (SEGMENTBYTES / 8) * 3 + (IND))); \ - crc0 = __crc32cd( \ - crc0, *((const uint64_t *)(P) + (SEGMENTBYTES / 8) * 0 + (IND))); \ + std::memcpy(&d64, (P) + SEGMENTBYTES * 1 + (IND) * 8, sizeof(d64)); \ + crc1 = __crc32cd(crc1, d64); \ + std::memcpy(&d64, (P) + SEGMENTBYTES * 2 + (IND) * 8, sizeof(d64)); \ + crc2 = __crc32cd(crc2, d64); \ + std::memcpy(&d64, (P) + SEGMENTBYTES * 3 + (IND) * 8, sizeof(d64)); \ + crc3 = __crc32cd(crc3, d64); \ + std::memcpy(&d64, (P) + SEGMENTBYTES * 0 + (IND) * 8, sizeof(d64)); \ + crc0 = __crc32cd(crc0, d64); \ } while (0); // compute 8*8 bytes for each segment parallelly @@ -68,6 +69,9 @@ uint32_t ExtendArm64(uint32_t crc, const uint8_t *data, size_t size) { int64_t length = size; uint32_t crc0, crc1, crc2, crc3; uint64_t t0, t1, t2; + uint16_t d16; + uint32_t d32; + uint64_t d64; // k0=CRC(x^(3*SEGMENTBYTES*8)), k1=CRC(x^(2*SEGMENTBYTES*8)), // k2=CRC(x^(SEGMENTBYTES*8)) @@ -88,7 +92,8 @@ uint32_t ExtendArm64(uint32_t crc, const uint8_t *data, size_t size) { t2 = (uint64_t)vmull_p64(crc2, k2); t1 = (uint64_t)vmull_p64(crc1, k1); t0 = (uint64_t)vmull_p64(crc0, k0); - crc = __crc32cd(crc3, *(uint64_t *)data); + std::memcpy(&d64, data, sizeof(d64)); + crc = __crc32cd(crc3, d64); data += sizeof(uint64_t); crc ^= __crc32cd(0, t2); crc ^= __crc32cd(0, t1); @@ -98,18 +103,21 @@ uint32_t ExtendArm64(uint32_t crc, const uint8_t *data, size_t size) { } while (length >= 8) { - crc = __crc32cd(crc, *(uint64_t *)data); + std::memcpy(&d64, data, sizeof(d64)); + crc = __crc32cd(crc, d64); data += 8; length -= 8; } if (length & 4) { - crc = __crc32cw(crc, *(uint32_t *)data); + std::memcpy(&d32, data, sizeof(d32)); + crc = __crc32cw(crc, d32); data += 4; } if (length & 2) { - crc = __crc32ch(crc, *(uint16_t *)data); + std::memcpy(&d16, data, sizeof(d16)); + crc = __crc32ch(crc, d16); data += 2; } From ad7584d8b60119ca3717117a1eb6a16d753c5d74 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Mon, 26 Feb 2024 19:52:14 +0000 Subject: [PATCH 36/50] serialization: replace char-is-int8_t autoconf detection with c++20 concept This removes the only remaining autoconf macro in our serialization code, so it can now be used trivially and safely out-of-tree. --- configure.ac | 8 -------- src/serialize.h | 20 ++++++++++---------- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/configure.ac b/configure.ac index 50e6870dd9..45c114688b 100644 --- a/configure.ac +++ b/configure.ac @@ -1174,14 +1174,6 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include [ AC_MSG_RESULT([no])] ) -AC_MSG_CHECKING([for if type char equals int8_t]) -AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include - #include ]], - [[ static_assert(std::is_same::value, ""); ]])], - [ AC_MSG_RESULT([yes]); AC_DEFINE([CHAR_EQUALS_INT8], [1], [Define this symbol if type char equals int8_t]) ], - [ AC_MSG_RESULT([no])] -) - AC_MSG_CHECKING([for fdatasync]) AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include ]], [[ fdatasync(0); ]])], diff --git a/src/serialize.h b/src/serialize.h index 7b336ce1af..ab57376a57 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -6,10 +6,6 @@ #ifndef BITCOIN_SERIALIZE_H #define BITCOIN_SERIALIZE_H -#if defined(HAVE_CONFIG_H) -#include -#endif - #include #include // IWYU pragma: keep #include @@ -17,6 +13,7 @@ #include #include +#include #include #include #include @@ -263,9 +260,14 @@ const Out& AsBase(const In& x) // i.e. anything that supports .read(Span) and .write(Span) // // clang-format off -#ifndef CHAR_EQUALS_INT8 -template void Serialize(Stream&, char) = delete; // char serialization forbidden. Use uint8_t or int8_t -#endif + +// Typically int8_t and char are distinct types, but some systems may define int8_t +// in terms of char. Forbid serialization of char in the typical case, but allow it if +// it's the only way to describe an int8_t. +template +concept CharNotInt8 = std::same_as && !std::same_as; + +template void Serialize(Stream&, V) = delete; // char serialization forbidden. Use uint8_t or int8_t template void Serialize(Stream& s, std::byte a) { ser_writedata8(s, uint8_t(a)); } template inline void Serialize(Stream& s, int8_t a ) { ser_writedata8(s, a); } template inline void Serialize(Stream& s, uint8_t a ) { ser_writedata8(s, a); } @@ -279,9 +281,7 @@ template void Serialize(Stream& s, const B template void Serialize(Stream& s, const std::array& a) { s.write(MakeByteSpan(a)); } template void Serialize(Stream& s, Span span) { s.write(AsBytes(span)); } -#ifndef CHAR_EQUALS_INT8 -template void Unserialize(Stream&, char) = delete; // char serialization forbidden. Use uint8_t or int8_t -#endif +template void Unserialize(Stream&, V) = delete; // char serialization forbidden. Use uint8_t or int8_t template void Unserialize(Stream& s, std::byte& a) { a = std::byte{ser_readdata8(s)}; } template inline void Unserialize(Stream& s, int8_t& a ) { a = ser_readdata8(s); } template inline void Unserialize(Stream& s, uint8_t& a ) { a = ser_readdata8(s); } From 6fa61e35320ac2bc623a9c9ca11b270b34e2d05a Mon Sep 17 00:00:00 2001 From: Justin Dhillon Date: Tue, 27 Feb 2024 13:56:23 -0800 Subject: [PATCH 37/50] doc: Fix Broken Links --- test/functional/test_framework/netutil.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/test_framework/netutil.py b/test/functional/test_framework/netutil.py index 838f40fcaa..30a4a58d6f 100644 --- a/test/functional/test_framework/netutil.py +++ b/test/functional/test_framework/netutil.py @@ -4,7 +4,7 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Linux network utilities. -Roughly based on http://voorloopnul.com/blog/a-python-netstat-in-less-than-100-lines-of-code/ by Ricardo Pascal +Roughly based on https://web.archive.org/web/20190424172231/http://voorloopnul.com/blog/a-python-netstat-in-less-than-100-lines-of-code/ by Ricardo Pascal """ import sys From 367bb7a80cc71130995672c853d4a6e0134721d6 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 28 Feb 2024 13:00:00 +0300 Subject: [PATCH 38/50] wallet: Avoid updating `ReserveDestination::nIndex` when `GetReservedDestination` fails --- src/wallet/wallet.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 26c5256f6f..3ac09430d8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2607,8 +2607,10 @@ util::Result ReserveDestination::GetReservedDestination(bool int if (nIndex == -1) { CKeyPool keypool; - auto op_address = m_spk_man->GetReservedDestination(type, internal, nIndex, keypool); + int64_t index; + auto op_address = m_spk_man->GetReservedDestination(type, internal, index, keypool); if (!op_address) return op_address; + nIndex = index; address = *op_address; fInternal = keypool.fInternal; } From e073f1dfda7a2a2cb2be9fe2a1d576f122596021 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 28 Feb 2024 13:04:48 +0300 Subject: [PATCH 39/50] test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures --- test/functional/wallet_keypool.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/test/functional/wallet_keypool.py b/test/functional/wallet_keypool.py index d2341fb12e..6ed8572347 100755 --- a/test/functional/wallet_keypool.py +++ b/test/functional/wallet_keypool.py @@ -103,11 +103,18 @@ def run_test(self): nodes[0].getrawchangeaddress() nodes[0].getrawchangeaddress() nodes[0].getrawchangeaddress() - addr = set() + # remember keypool sizes + wi = nodes[0].getwalletinfo() + kp_size_before = [wi['keypoolsize_hd_internal'], wi['keypoolsize']] # the next one should fail assert_raises_rpc_error(-12, "Keypool ran out", nodes[0].getrawchangeaddress) + # check that keypool sizes did not change + wi = nodes[0].getwalletinfo() + kp_size_after = [wi['keypoolsize_hd_internal'], wi['keypoolsize']] + assert_equal(kp_size_before, kp_size_after) # drain the external keys + addr = set() addr.add(nodes[0].getnewaddress(address_type="bech32")) addr.add(nodes[0].getnewaddress(address_type="bech32")) addr.add(nodes[0].getnewaddress(address_type="bech32")) @@ -115,8 +122,15 @@ def run_test(self): addr.add(nodes[0].getnewaddress(address_type="bech32")) addr.add(nodes[0].getnewaddress(address_type="bech32")) assert len(addr) == 6 + # remember keypool sizes + wi = nodes[0].getwalletinfo() + kp_size_before = [wi['keypoolsize_hd_internal'], wi['keypoolsize']] # the next one should fail assert_raises_rpc_error(-12, "Error: Keypool ran out, please call keypoolrefill first", nodes[0].getnewaddress) + # check that keypool sizes did not change + wi = nodes[0].getwalletinfo() + kp_size_after = [wi['keypoolsize_hd_internal'], wi['keypoolsize']] + assert_equal(kp_size_before, kp_size_after) # refill keypool with three new addresses nodes[0].walletpassphrase('test', 1) From 432b18ca8d0654318a8d882b28b20af2cb2d2e5d Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 27 Feb 2024 18:29:24 +0000 Subject: [PATCH 40/50] serialization: detect byteswap builtins without autoconf tests Rather than a complicated set of tests to decide which bswap functions to use, always prefer the compiler built-ins when available. These builtins and fallbacks can all be removed once we're using c++23, which adds std::byteswap. --- configure.ac | 7 +---- src/compat/byteswap.h | 66 ++++++++++++++++++++++++++-------------- src/compat/endian.h | 24 +++++++-------- src/test/bswap_tests.cpp | 6 ++-- 4 files changed, 59 insertions(+), 44 deletions(-) diff --git a/configure.ac b/configure.ac index 50e6870dd9..882bd01075 100644 --- a/configure.ac +++ b/configure.ac @@ -975,7 +975,7 @@ if test "$TARGET_OS" = "darwin"; then AX_CHECK_LINK_FLAG([-Wl,-fixup_chains], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,-fixup_chains"], [], [$LDFLAG_WERROR]) fi -AC_CHECK_HEADERS([endian.h sys/endian.h byteswap.h sys/select.h sys/prctl.h sys/sysctl.h vm/vm_param.h sys/vmmeter.h sys/resources.h]) +AC_CHECK_HEADERS([endian.h sys/endian.h sys/select.h sys/prctl.h sys/sysctl.h vm/vm_param.h sys/vmmeter.h sys/resources.h]) AC_CHECK_DECLS([getifaddrs, freeifaddrs],[CHECK_SOCKET],, [#include @@ -997,11 +997,6 @@ AC_CHECK_DECLS([le16toh, le32toh, le64toh, htole16, htole32, htole64, be16toh, b #include #endif]) -AC_CHECK_DECLS([bswap_16, bswap_32, bswap_64],,, - [#if HAVE_BYTESWAP_H - #include - #endif]) - AC_MSG_CHECKING([for __builtin_clzl]) AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ ]], [[ (void) __builtin_clzl(0); diff --git a/src/compat/byteswap.h b/src/compat/byteswap.h index 9ee71ef267..0d0b079230 100644 --- a/src/compat/byteswap.h +++ b/src/compat/byteswap.h @@ -5,44 +5,66 @@ #ifndef BITCOIN_COMPAT_BYTESWAP_H #define BITCOIN_COMPAT_BYTESWAP_H -#if defined(HAVE_CONFIG_H) -#include +#include +#ifdef _MSC_VER +#include #endif -#include -#if defined(HAVE_BYTESWAP_H) -#include -#endif +// All internal_bswap_* functions can be replaced with std::byteswap once we +// require c++23. Both libstdc++ and libc++ implement std::byteswap via these +// builtins. -#if defined(MAC_OSX) +#ifndef DISABLE_BUILTIN_BSWAPS +# if defined __has_builtin +# if __has_builtin(__builtin_bswap16) +# define bitcoin_builtin_bswap16(x) __builtin_bswap16(x) +# endif +# if __has_builtin(__builtin_bswap32) +# define bitcoin_builtin_bswap32(x) __builtin_bswap32(x) +# endif +# if __has_builtin(__builtin_bswap64) +# define bitcoin_builtin_bswap64(x) __builtin_bswap64(x) +# endif +# elif defined(_MSC_VER) +# define bitcoin_builtin_bswap16(x) _byteswap_ushort(x) +# define bitcoin_builtin_bswap32(x) _byteswap_ulong(x) +# define bitcoin_builtin_bswap64(x) _byteswap_uint64(x) +# endif +#endif -#include -#define bswap_16(x) OSSwapInt16(x) -#define bswap_32(x) OSSwapInt32(x) -#define bswap_64(x) OSSwapInt64(x) +// MSVC's _byteswap_* functions are not constexpr +#ifndef _MSC_VER +#define BSWAP_CONSTEXPR constexpr #else -// Non-MacOS / non-Darwin +#define BSWAP_CONSTEXPR +#endif -#if HAVE_DECL_BSWAP_16 == 0 -inline uint16_t bswap_16(uint16_t x) +inline BSWAP_CONSTEXPR uint16_t internal_bswap_16(uint16_t x) { +#ifdef bitcoin_builtin_bswap16 + return bitcoin_builtin_bswap16(x); +#else return (x >> 8) | (x << 8); +#endif } -#endif // HAVE_DECL_BSWAP16 == 0 -#if HAVE_DECL_BSWAP_32 == 0 -inline uint32_t bswap_32(uint32_t x) +inline BSWAP_CONSTEXPR uint32_t internal_bswap_32(uint32_t x) { +#ifdef bitcoin_builtin_bswap32 + return bitcoin_builtin_bswap32(x); +#else return (((x & 0xff000000U) >> 24) | ((x & 0x00ff0000U) >> 8) | ((x & 0x0000ff00U) << 8) | ((x & 0x000000ffU) << 24)); +#endif } -#endif // HAVE_DECL_BSWAP32 == 0 -#if HAVE_DECL_BSWAP_64 == 0 -inline uint64_t bswap_64(uint64_t x) +inline BSWAP_CONSTEXPR uint64_t internal_bswap_64(uint64_t x) { +#ifdef bitcoin_builtin_bswap64 + return bitcoin_builtin_bswap64(x); +#else return (((x & 0xff00000000000000ull) >> 56) | ((x & 0x00ff000000000000ull) >> 40) | ((x & 0x0000ff0000000000ull) >> 24) @@ -51,9 +73,7 @@ inline uint64_t bswap_64(uint64_t x) | ((x & 0x0000000000ff0000ull) << 24) | ((x & 0x000000000000ff00ull) << 40) | ((x & 0x00000000000000ffull) << 56)); +#endif } -#endif // HAVE_DECL_BSWAP64 == 0 - -#endif // defined(MAC_OSX) #endif // BITCOIN_COMPAT_BYTESWAP_H diff --git a/src/compat/endian.h b/src/compat/endian.h index 882de2dbf0..70161409f8 100644 --- a/src/compat/endian.h +++ b/src/compat/endian.h @@ -76,7 +76,7 @@ inline uint16_t htobe16(uint16_t host_16bits) #if HAVE_DECL_HTOLE16 == 0 inline uint16_t htole16(uint16_t host_16bits) { - return bswap_16(host_16bits); + return internal_bswap_16(host_16bits); } #endif // HAVE_DECL_HTOLE16 @@ -90,7 +90,7 @@ inline uint16_t be16toh(uint16_t big_endian_16bits) #if HAVE_DECL_LE16TOH == 0 inline uint16_t le16toh(uint16_t little_endian_16bits) { - return bswap_16(little_endian_16bits); + return internal_bswap_16(little_endian_16bits); } #endif // HAVE_DECL_LE16TOH @@ -104,7 +104,7 @@ inline uint32_t htobe32(uint32_t host_32bits) #if HAVE_DECL_HTOLE32 == 0 inline uint32_t htole32(uint32_t host_32bits) { - return bswap_32(host_32bits); + return internal_bswap_32(host_32bits); } #endif // HAVE_DECL_HTOLE32 @@ -118,7 +118,7 @@ inline uint32_t be32toh(uint32_t big_endian_32bits) #if HAVE_DECL_LE32TOH == 0 inline uint32_t le32toh(uint32_t little_endian_32bits) { - return bswap_32(little_endian_32bits); + return internal_bswap_32(little_endian_32bits); } #endif // HAVE_DECL_LE32TOH @@ -132,7 +132,7 @@ inline uint64_t htobe64(uint64_t host_64bits) #if HAVE_DECL_HTOLE64 == 0 inline uint64_t htole64(uint64_t host_64bits) { - return bswap_64(host_64bits); + return internal_bswap_64(host_64bits); } #endif // HAVE_DECL_HTOLE64 @@ -146,7 +146,7 @@ inline uint64_t be64toh(uint64_t big_endian_64bits) #if HAVE_DECL_LE64TOH == 0 inline uint64_t le64toh(uint64_t little_endian_64bits) { - return bswap_64(little_endian_64bits); + return internal_bswap_64(little_endian_64bits); } #endif // HAVE_DECL_LE64TOH @@ -155,7 +155,7 @@ inline uint64_t le64toh(uint64_t little_endian_64bits) #if HAVE_DECL_HTOBE16 == 0 inline uint16_t htobe16(uint16_t host_16bits) { - return bswap_16(host_16bits); + return internal_bswap_16(host_16bits); } #endif // HAVE_DECL_HTOBE16 @@ -169,7 +169,7 @@ inline uint16_t htole16(uint16_t host_16bits) #if HAVE_DECL_BE16TOH == 0 inline uint16_t be16toh(uint16_t big_endian_16bits) { - return bswap_16(big_endian_16bits); + return internal_bswap_16(big_endian_16bits); } #endif // HAVE_DECL_BE16TOH @@ -183,7 +183,7 @@ inline uint16_t le16toh(uint16_t little_endian_16bits) #if HAVE_DECL_HTOBE32 == 0 inline uint32_t htobe32(uint32_t host_32bits) { - return bswap_32(host_32bits); + return internal_bswap_32(host_32bits); } #endif // HAVE_DECL_HTOBE32 @@ -197,7 +197,7 @@ inline uint32_t htole32(uint32_t host_32bits) #if HAVE_DECL_BE32TOH == 0 inline uint32_t be32toh(uint32_t big_endian_32bits) { - return bswap_32(big_endian_32bits); + return internal_bswap_32(big_endian_32bits); } #endif // HAVE_DECL_BE32TOH @@ -211,7 +211,7 @@ inline uint32_t le32toh(uint32_t little_endian_32bits) #if HAVE_DECL_HTOBE64 == 0 inline uint64_t htobe64(uint64_t host_64bits) { - return bswap_64(host_64bits); + return internal_bswap_64(host_64bits); } #endif // HAVE_DECL_HTOBE64 @@ -225,7 +225,7 @@ inline uint64_t htole64(uint64_t host_64bits) #if HAVE_DECL_BE64TOH == 0 inline uint64_t be64toh(uint64_t big_endian_64bits) { - return bswap_64(big_endian_64bits); + return internal_bswap_64(big_endian_64bits); } #endif // HAVE_DECL_BE64TOH diff --git a/src/test/bswap_tests.cpp b/src/test/bswap_tests.cpp index 2be7122fc1..fe48e39c41 100644 --- a/src/test/bswap_tests.cpp +++ b/src/test/bswap_tests.cpp @@ -16,9 +16,9 @@ BOOST_AUTO_TEST_CASE(bswap_tests) uint16_t e1 = 0x3412; uint32_t e2 = 0xbc9a7856; uint64_t e3 = 0xbc9a78563412f0de; - BOOST_CHECK(bswap_16(u1) == e1); - BOOST_CHECK(bswap_32(u2) == e2); - BOOST_CHECK(bswap_64(u3) == e3); + BOOST_CHECK(internal_bswap_16(u1) == e1); + BOOST_CHECK(internal_bswap_32(u2) == e2); + BOOST_CHECK(internal_bswap_64(u3) == e3); } BOOST_AUTO_TEST_SUITE_END() From 86b7f28d6c507155a9d3a15487ee883989b88943 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 27 Feb 2024 18:39:22 +0000 Subject: [PATCH 41/50] serialization: use internal endian conversion functions These replace our platform-specific mess in favor of c++20 endian detection via std::endian and internal byteswap functions when necessary. They no longer rely on autoconf detection. --- configure.ac | 9 +- src/compat/endian.h | 241 +++++++------------------------------------- src/crypto/common.h | 22 ++-- src/i2p.cpp | 2 +- src/serialize.h | 28 ++--- 5 files changed, 64 insertions(+), 238 deletions(-) diff --git a/configure.ac b/configure.ac index 882bd01075..439fe10cde 100644 --- a/configure.ac +++ b/configure.ac @@ -975,7 +975,7 @@ if test "$TARGET_OS" = "darwin"; then AX_CHECK_LINK_FLAG([-Wl,-fixup_chains], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,-fixup_chains"], [], [$LDFLAG_WERROR]) fi -AC_CHECK_HEADERS([endian.h sys/endian.h sys/select.h sys/prctl.h sys/sysctl.h vm/vm_param.h sys/vmmeter.h sys/resources.h]) +AC_CHECK_HEADERS([sys/select.h sys/prctl.h sys/sysctl.h vm/vm_param.h sys/vmmeter.h sys/resources.h]) AC_CHECK_DECLS([getifaddrs, freeifaddrs],[CHECK_SOCKET],, [#include @@ -990,13 +990,6 @@ AC_CHECK_DECLS([pipe2]) AC_CHECK_FUNCS([timingsafe_bcmp]) -AC_CHECK_DECLS([le16toh, le32toh, le64toh, htole16, htole32, htole64, be16toh, be32toh, be64toh, htobe16, htobe32, htobe64],,, - [#if HAVE_ENDIAN_H - #include - #elif HAVE_SYS_ENDIAN_H - #include - #endif]) - AC_MSG_CHECKING([for __builtin_clzl]) AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ ]], [[ (void) __builtin_clzl(0); diff --git a/src/compat/endian.h b/src/compat/endian.h index 70161409f8..4f58428153 100644 --- a/src/compat/endian.h +++ b/src/compat/endian.h @@ -5,237 +5,70 @@ #ifndef BITCOIN_COMPAT_ENDIAN_H #define BITCOIN_COMPAT_ENDIAN_H -#if defined(HAVE_CONFIG_H) -#include -#endif - #include +#include #include -#if defined(HAVE_ENDIAN_H) -#include -#elif defined(HAVE_SYS_ENDIAN_H) -#include -#endif - -#ifndef HAVE_CONFIG_H -// While not technically a supported configuration, defaulting to defining these -// DECLs when we were compiled without autotools makes it easier for other build -// systems to build things like libbitcoinconsensus for strange targets. -#ifdef htobe16 -#define HAVE_DECL_HTOBE16 1 -#endif -#ifdef htole16 -#define HAVE_DECL_HTOLE16 1 -#endif -#ifdef be16toh -#define HAVE_DECL_BE16TOH 1 -#endif -#ifdef le16toh -#define HAVE_DECL_LE16TOH 1 -#endif - -#ifdef htobe32 -#define HAVE_DECL_HTOBE32 1 -#endif -#ifdef htole32 -#define HAVE_DECL_HTOLE32 1 -#endif -#ifdef be32toh -#define HAVE_DECL_BE32TOH 1 -#endif -#ifdef le32toh -#define HAVE_DECL_LE32TOH 1 -#endif - -#ifdef htobe64 -#define HAVE_DECL_HTOBE64 1 -#endif -#ifdef htole64 -#define HAVE_DECL_HTOLE64 1 -#endif -#ifdef be64toh -#define HAVE_DECL_BE64TOH 1 -#endif -#ifdef le64toh -#define HAVE_DECL_LE64TOH 1 -#endif - -#endif // HAVE_CONFIG_H - -#if defined(WORDS_BIGENDIAN) - -#if HAVE_DECL_HTOBE16 == 0 -inline uint16_t htobe16(uint16_t host_16bits) -{ - return host_16bits; -} -#endif // HAVE_DECL_HTOBE16 - -#if HAVE_DECL_HTOLE16 == 0 -inline uint16_t htole16(uint16_t host_16bits) -{ - return internal_bswap_16(host_16bits); -} -#endif // HAVE_DECL_HTOLE16 - -#if HAVE_DECL_BE16TOH == 0 -inline uint16_t be16toh(uint16_t big_endian_16bits) -{ - return big_endian_16bits; -} -#endif // HAVE_DECL_BE16TOH - -#if HAVE_DECL_LE16TOH == 0 -inline uint16_t le16toh(uint16_t little_endian_16bits) -{ - return internal_bswap_16(little_endian_16bits); -} -#endif // HAVE_DECL_LE16TOH - -#if HAVE_DECL_HTOBE32 == 0 -inline uint32_t htobe32(uint32_t host_32bits) -{ - return host_32bits; -} -#endif // HAVE_DECL_HTOBE32 - -#if HAVE_DECL_HTOLE32 == 0 -inline uint32_t htole32(uint32_t host_32bits) -{ - return internal_bswap_32(host_32bits); -} -#endif // HAVE_DECL_HTOLE32 - -#if HAVE_DECL_BE32TOH == 0 -inline uint32_t be32toh(uint32_t big_endian_32bits) -{ - return big_endian_32bits; -} -#endif // HAVE_DECL_BE32TOH - -#if HAVE_DECL_LE32TOH == 0 -inline uint32_t le32toh(uint32_t little_endian_32bits) +inline BSWAP_CONSTEXPR uint16_t htobe16_internal(uint16_t host_16bits) { - return internal_bswap_32(little_endian_32bits); + if constexpr (std::endian::native == std::endian::little) return internal_bswap_16(host_16bits); + else return host_16bits; } -#endif // HAVE_DECL_LE32TOH - -#if HAVE_DECL_HTOBE64 == 0 -inline uint64_t htobe64(uint64_t host_64bits) -{ - return host_64bits; -} -#endif // HAVE_DECL_HTOBE64 - -#if HAVE_DECL_HTOLE64 == 0 -inline uint64_t htole64(uint64_t host_64bits) -{ - return internal_bswap_64(host_64bits); -} -#endif // HAVE_DECL_HTOLE64 - -#if HAVE_DECL_BE64TOH == 0 -inline uint64_t be64toh(uint64_t big_endian_64bits) -{ - return big_endian_64bits; -} -#endif // HAVE_DECL_BE64TOH - -#if HAVE_DECL_LE64TOH == 0 -inline uint64_t le64toh(uint64_t little_endian_64bits) -{ - return internal_bswap_64(little_endian_64bits); -} -#endif // HAVE_DECL_LE64TOH - -#else // WORDS_BIGENDIAN - -#if HAVE_DECL_HTOBE16 == 0 -inline uint16_t htobe16(uint16_t host_16bits) +inline BSWAP_CONSTEXPR uint16_t htole16_internal(uint16_t host_16bits) { - return internal_bswap_16(host_16bits); + if constexpr (std::endian::native == std::endian::big) return internal_bswap_16(host_16bits); + else return host_16bits; } -#endif // HAVE_DECL_HTOBE16 - -#if HAVE_DECL_HTOLE16 == 0 -inline uint16_t htole16(uint16_t host_16bits) +inline BSWAP_CONSTEXPR uint16_t be16toh_internal(uint16_t big_endian_16bits) { - return host_16bits; + if constexpr (std::endian::native == std::endian::little) return internal_bswap_16(big_endian_16bits); + else return big_endian_16bits; } -#endif // HAVE_DECL_HTOLE16 - -#if HAVE_DECL_BE16TOH == 0 -inline uint16_t be16toh(uint16_t big_endian_16bits) +inline BSWAP_CONSTEXPR uint16_t le16toh_internal(uint16_t little_endian_16bits) { - return internal_bswap_16(big_endian_16bits); + if constexpr (std::endian::native == std::endian::big) return internal_bswap_16(little_endian_16bits); + else return little_endian_16bits; } -#endif // HAVE_DECL_BE16TOH - -#if HAVE_DECL_LE16TOH == 0 -inline uint16_t le16toh(uint16_t little_endian_16bits) +inline BSWAP_CONSTEXPR uint32_t htobe32_internal(uint32_t host_32bits) { - return little_endian_16bits; + if constexpr (std::endian::native == std::endian::little) return internal_bswap_32(host_32bits); + else return host_32bits; } -#endif // HAVE_DECL_LE16TOH - -#if HAVE_DECL_HTOBE32 == 0 -inline uint32_t htobe32(uint32_t host_32bits) -{ - return internal_bswap_32(host_32bits); -} -#endif // HAVE_DECL_HTOBE32 - -#if HAVE_DECL_HTOLE32 == 0 -inline uint32_t htole32(uint32_t host_32bits) +inline BSWAP_CONSTEXPR uint32_t htole32_internal(uint32_t host_32bits) { - return host_32bits; + if constexpr (std::endian::native == std::endian::big) return internal_bswap_32(host_32bits); + else return host_32bits; } -#endif // HAVE_DECL_HTOLE32 - -#if HAVE_DECL_BE32TOH == 0 -inline uint32_t be32toh(uint32_t big_endian_32bits) +inline BSWAP_CONSTEXPR uint32_t be32toh_internal(uint32_t big_endian_32bits) { - return internal_bswap_32(big_endian_32bits); + if constexpr (std::endian::native == std::endian::little) return internal_bswap_32(big_endian_32bits); + else return big_endian_32bits; } -#endif // HAVE_DECL_BE32TOH - -#if HAVE_DECL_LE32TOH == 0 -inline uint32_t le32toh(uint32_t little_endian_32bits) +inline BSWAP_CONSTEXPR uint32_t le32toh_internal(uint32_t little_endian_32bits) { - return little_endian_32bits; + if constexpr (std::endian::native == std::endian::big) return internal_bswap_32(little_endian_32bits); + else return little_endian_32bits; } -#endif // HAVE_DECL_LE32TOH - -#if HAVE_DECL_HTOBE64 == 0 -inline uint64_t htobe64(uint64_t host_64bits) +inline BSWAP_CONSTEXPR uint64_t htobe64_internal(uint64_t host_64bits) { - return internal_bswap_64(host_64bits); + if constexpr (std::endian::native == std::endian::little) return internal_bswap_64(host_64bits); + else return host_64bits; } -#endif // HAVE_DECL_HTOBE64 - -#if HAVE_DECL_HTOLE64 == 0 -inline uint64_t htole64(uint64_t host_64bits) +inline BSWAP_CONSTEXPR uint64_t htole64_internal(uint64_t host_64bits) { - return host_64bits; + if constexpr (std::endian::native == std::endian::big) return internal_bswap_64(host_64bits); + else return host_64bits; } -#endif // HAVE_DECL_HTOLE64 - -#if HAVE_DECL_BE64TOH == 0 -inline uint64_t be64toh(uint64_t big_endian_64bits) +inline BSWAP_CONSTEXPR uint64_t be64toh_internal(uint64_t big_endian_64bits) { - return internal_bswap_64(big_endian_64bits); + if constexpr (std::endian::native == std::endian::little) return internal_bswap_64(big_endian_64bits); + else return big_endian_64bits; } -#endif // HAVE_DECL_BE64TOH - -#if HAVE_DECL_LE64TOH == 0 -inline uint64_t le64toh(uint64_t little_endian_64bits) +inline BSWAP_CONSTEXPR uint64_t le64toh_internal(uint64_t little_endian_64bits) { - return little_endian_64bits; + if constexpr (std::endian::native == std::endian::big) return internal_bswap_64(little_endian_64bits); + else return little_endian_64bits; } -#endif // HAVE_DECL_LE64TOH - -#endif // WORDS_BIGENDIAN #endif // BITCOIN_COMPAT_ENDIAN_H diff --git a/src/crypto/common.h b/src/crypto/common.h index 42f72a6fa8..1dc4f3f55c 100644 --- a/src/crypto/common.h +++ b/src/crypto/common.h @@ -14,38 +14,38 @@ uint16_t static inline ReadLE16(const unsigned char* ptr) { uint16_t x; memcpy(&x, ptr, 2); - return le16toh(x); + return le16toh_internal(x); } uint32_t static inline ReadLE32(const unsigned char* ptr) { uint32_t x; memcpy(&x, ptr, 4); - return le32toh(x); + return le32toh_internal(x); } uint64_t static inline ReadLE64(const unsigned char* ptr) { uint64_t x; memcpy(&x, ptr, 8); - return le64toh(x); + return le64toh_internal(x); } void static inline WriteLE16(unsigned char* ptr, uint16_t x) { - uint16_t v = htole16(x); + uint16_t v = htole16_internal(x); memcpy(ptr, &v, 2); } void static inline WriteLE32(unsigned char* ptr, uint32_t x) { - uint32_t v = htole32(x); + uint32_t v = htole32_internal(x); memcpy(ptr, &v, 4); } void static inline WriteLE64(unsigned char* ptr, uint64_t x) { - uint64_t v = htole64(x); + uint64_t v = htole64_internal(x); memcpy(ptr, &v, 8); } @@ -53,32 +53,32 @@ uint16_t static inline ReadBE16(const unsigned char* ptr) { uint16_t x; memcpy(&x, ptr, 2); - return be16toh(x); + return be16toh_internal(x); } uint32_t static inline ReadBE32(const unsigned char* ptr) { uint32_t x; memcpy(&x, ptr, 4); - return be32toh(x); + return be32toh_internal(x); } uint64_t static inline ReadBE64(const unsigned char* ptr) { uint64_t x; memcpy(&x, ptr, 8); - return be64toh(x); + return be64toh_internal(x); } void static inline WriteBE32(unsigned char* ptr, uint32_t x) { - uint32_t v = htobe32(x); + uint32_t v = htobe32_internal(x); memcpy(ptr, &v, 4); } void static inline WriteBE64(unsigned char* ptr, uint64_t x) { - uint64_t v = htobe64(x); + uint64_t v = htobe64_internal(x); memcpy(ptr, &v, 8); } diff --git a/src/i2p.cpp b/src/i2p.cpp index c891562d00..4b79a6826b 100644 --- a/src/i2p.cpp +++ b/src/i2p.cpp @@ -392,7 +392,7 @@ Binary Session::MyDestination() const } memcpy(&cert_len, &m_private_key.at(CERT_LEN_POS), sizeof(cert_len)); - cert_len = be16toh(cert_len); + cert_len = be16toh_internal(cert_len); const size_t dest_len = DEST_LEN_BASE + cert_len; diff --git a/src/serialize.h b/src/serialize.h index 7b336ce1af..5ae701191c 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -60,27 +60,27 @@ template inline void ser_writedata8(Stream &s, uint8_t obj) } template inline void ser_writedata16(Stream &s, uint16_t obj) { - obj = htole16(obj); + obj = htole16_internal(obj); s.write(AsBytes(Span{&obj, 1})); } template inline void ser_writedata16be(Stream &s, uint16_t obj) { - obj = htobe16(obj); + obj = htobe16_internal(obj); s.write(AsBytes(Span{&obj, 1})); } template inline void ser_writedata32(Stream &s, uint32_t obj) { - obj = htole32(obj); + obj = htole32_internal(obj); s.write(AsBytes(Span{&obj, 1})); } template inline void ser_writedata32be(Stream &s, uint32_t obj) { - obj = htobe32(obj); + obj = htobe32_internal(obj); s.write(AsBytes(Span{&obj, 1})); } template inline void ser_writedata64(Stream &s, uint64_t obj) { - obj = htole64(obj); + obj = htole64_internal(obj); s.write(AsBytes(Span{&obj, 1})); } template inline uint8_t ser_readdata8(Stream &s) @@ -93,31 +93,31 @@ template inline uint16_t ser_readdata16(Stream &s) { uint16_t obj; s.read(AsWritableBytes(Span{&obj, 1})); - return le16toh(obj); + return le16toh_internal(obj); } template inline uint16_t ser_readdata16be(Stream &s) { uint16_t obj; s.read(AsWritableBytes(Span{&obj, 1})); - return be16toh(obj); + return be16toh_internal(obj); } template inline uint32_t ser_readdata32(Stream &s) { uint32_t obj; s.read(AsWritableBytes(Span{&obj, 1})); - return le32toh(obj); + return le32toh_internal(obj); } template inline uint32_t ser_readdata32be(Stream &s) { uint32_t obj; s.read(AsWritableBytes(Span{&obj, 1})); - return be32toh(obj); + return be32toh_internal(obj); } template inline uint64_t ser_readdata64(Stream &s) { uint64_t obj; s.read(AsWritableBytes(Span{&obj, 1})); - return le64toh(obj); + return le64toh_internal(obj); } @@ -548,10 +548,10 @@ struct CustomUintFormatter { if (v < 0 || v > MAX) throw std::ios_base::failure("CustomUintFormatter value out of range"); if (BigEndian) { - uint64_t raw = htobe64(v); + uint64_t raw = htobe64_internal(v); s.write(AsBytes(Span{&raw, 1}).last(Bytes)); } else { - uint64_t raw = htole64(v); + uint64_t raw = htole64_internal(v); s.write(AsBytes(Span{&raw, 1}).first(Bytes)); } } @@ -563,10 +563,10 @@ struct CustomUintFormatter uint64_t raw = 0; if (BigEndian) { s.read(AsWritableBytes(Span{&raw, 1}).last(Bytes)); - v = static_cast(be64toh(raw)); + v = static_cast(be64toh_internal(raw)); } else { s.read(AsWritableBytes(Span{&raw, 1}).first(Bytes)); - v = static_cast(le64toh(raw)); + v = static_cast(le64toh_internal(raw)); } } }; From 1484998b6b08c93714325952bd94dd8a2de446ae Mon Sep 17 00:00:00 2001 From: Max Edwards Date: Wed, 28 Feb 2024 13:40:08 +0000 Subject: [PATCH 42/50] ci: print python version on win64 native job --- .github/workflows/ci.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5857753e14..2559e1012c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -148,14 +148,15 @@ jobs: with: arch: x64 - - name: Check MSBuild and Qt + - name: Get tool information run: | msbuild -version | Out-File -FilePath "$env:GITHUB_WORKSPACE\msbuild_version" Get-Content -Path "$env:GITHUB_WORKSPACE\msbuild_version" $env:VCToolsVersion | Out-File -FilePath "$env:GITHUB_WORKSPACE\toolset_version" - Get-Content -Path "$env:GITHUB_WORKSPACE\toolset_version" + Write-Host "VCToolsVersion $(Get-Content -Path "$env:GITHUB_WORKSPACE\toolset_version")" $env:CI_QT_URL | Out-File -FilePath "$env:GITHUB_WORKSPACE\qt_url" $env:CI_QT_CONF | Out-File -FilePath "$env:GITHUB_WORKSPACE\qt_conf" + py -3 --version - name: Restore static Qt cache id: static-qt-cache From 0487f91a2046c0d6f91ccaedeb00fbefa635c66d Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Thu, 29 Feb 2024 10:39:40 +0530 Subject: [PATCH 43/50] test: Fix intermittent failure in rpc_net.py --v2transport Make sure that v2 handshake is complete before comparing getpeerinfo outputs so that `transport_protocol_type` isn't stuck at 'detecting'. - on the python side, this is ensured by default `wait_for_handshake = True` inside `add_p2p_connection()`. - on the c++ side, add a wait_until statement till `transport_protocol_type = v2` so that v2 handshake is complete. Co-Authored-By: Martin Zumsande --- test/functional/rpc_net.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index b4a58df5b2..accb2439f2 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -113,6 +113,8 @@ def test_getpeerinfo(self): self.nodes[0].setmocktime(no_version_peer_conntime) 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") self.nodes[0].setmocktime(0) peer_info = self.nodes[0].getpeerinfo()[no_version_peer_id] peer_info.pop("addr") From 6ee3997d03e456655e3c44abf1e15270c423ed41 Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Thu, 29 Feb 2024 09:54:47 -0500 Subject: [PATCH 44/50] test: removes unnecessary check from validation_tests An unnecessary check was added to the block mutation tests in #29412 where IsBlockMutated is returning true for the invalid reasons: we try to check mutation via transaction duplication, but the merkle root is not updated before the check, therefore the check fails because the provided root and the computed root differ, but not because the block contains the same transaction twice. The check is meaningless so it can be removed. --- src/test/validation_tests.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index c6f6ac3bdb..93a884be6d 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -233,7 +233,6 @@ BOOST_AUTO_TEST_CASE(block_malleation) // Block with two transactions is mutated if any node is duplicate. { block.vtx[1] = block.vtx[0]; - BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); HashWriter hasher; hasher.write(block.vtx[0]->GetHash()); hasher.write(block.vtx[1]->GetHash()); From 25eab523897e790f4f4d7b49cdbf19d13e3b0fcc Mon Sep 17 00:00:00 2001 From: brunoerg Date: Mon, 26 Feb 2024 18:06:19 -0300 Subject: [PATCH 45/50] fuzz: add target for local addresses --- src/test/fuzz/net.cpp | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index c882bd766a..e8b1480c5b 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -77,3 +77,40 @@ FUZZ_TARGET(net, .init = initialize_net) (void)node.HasPermission(net_permission_flags); (void)node.ConnectedThroughNetwork(); } + +FUZZ_TARGET(local_address, .init = initialize_net) +{ + FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + CService service{ConsumeService(fuzzed_data_provider)}; + CNode node{ConsumeNode(fuzzed_data_provider)}; + { + LOCK(g_maplocalhost_mutex); + mapLocalHost.clear(); + } + LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) { + CallOneOf( + fuzzed_data_provider, + [&] { + service = ConsumeService(fuzzed_data_provider); + }, + [&] { + const bool added{AddLocal(service, fuzzed_data_provider.ConsumeIntegralInRange(0, LOCAL_MAX - 1))}; + if (!added) return; + assert(service.IsRoutable()); + assert(IsLocal(service)); + assert(SeenLocal(service)); + }, + [&] { + (void)RemoveLocal(service); + }, + [&] { + (void)SeenLocal(service); + }, + [&] { + (void)IsLocal(service); + }, + [&] { + (void)GetLocalAddress(node); + }); + } +} From 376f0f6d0798c10f09266d609afea3ada1b99f9b Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Wed, 7 Feb 2024 20:04:01 +0000 Subject: [PATCH 46/50] build: remove confusing and inconsistent disable-asm option 1. It didn't actually disable asm usage in our code. Regardless of the setting, asm is used in random.cpp and support/cleanse.cpp. 2. The value wasn't forwarded to libsecp as a user might have reasonably expected. 3. We now have the DISABLE_OPTIMIZED_SHA256 define which is what disable-asm actually did in practice. If there is any desire, we can hook DISABLE_OPTIMIZED_SHA256 up to a new configure option that actually does what it says. --- configure.ac | 16 ---------------- src/Makefile.am | 2 -- src/crypto/sha256.cpp | 8 +++----- 3 files changed, 3 insertions(+), 23 deletions(-) diff --git a/configure.ac b/configure.ac index 078d968b0c..65c390651f 100644 --- a/configure.ac +++ b/configure.ac @@ -249,16 +249,6 @@ AC_ARG_ENABLE([threadlocal], [use_thread_local=$enableval], [use_thread_local=auto]) -AC_ARG_ENABLE([asm], - [AS_HELP_STRING([--disable-asm], - [disable assembly routines (enabled by default)])], - [use_asm=$enableval], - [use_asm=yes]) - -if test "$use_asm" = "yes"; then - AC_DEFINE([USE_ASM], [1], [Define this symbol to build in assembly routines]) -fi - AC_ARG_ENABLE([zmq], [AS_HELP_STRING([--disable-zmq], [disable ZMQ notifications])], @@ -460,8 +450,6 @@ enable_sse41=no enable_avx2=no enable_x86_shani=no -if test "$use_asm" = "yes"; then - dnl Check for optional instruction set support. Enabling these does _not_ imply that all code will dnl be compiled with them, rather that specific objects/libs may use them after checking for runtime dnl compatibility. @@ -600,8 +588,6 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ ) CXXFLAGS="$TEMP_CXXFLAGS" -fi - CORE_CPPFLAGS="$CORE_CPPFLAGS -DHAVE_BUILD_INFO" AC_ARG_WITH([utils], @@ -1817,7 +1803,6 @@ AM_CONDITIONAL([ENABLE_AVX2], [test "$enable_avx2" = "yes"]) AM_CONDITIONAL([ENABLE_X86_SHANI], [test "$enable_x86_shani" = "yes"]) AM_CONDITIONAL([ENABLE_ARM_CRC], [test "$enable_arm_crc" = "yes"]) AM_CONDITIONAL([ENABLE_ARM_SHANI], [test "$enable_arm_shani" = "yes"]) -AM_CONDITIONAL([USE_ASM], [test "$use_asm" = "yes"]) AM_CONDITIONAL([WORDS_BIGENDIAN], [test "$ac_cv_c_bigendian" = "yes"]) AM_CONDITIONAL([USE_NATPMP], [test "$use_natpmp" = "yes"]) AM_CONDITIONAL([USE_UPNP], [test "$use_upnp" = "yes"]) @@ -1972,7 +1957,6 @@ echo " with fuzz binary = $enable_fuzz_binary" echo " with bench = $use_bench" echo " with upnp = $use_upnp" echo " with natpmp = $use_natpmp" -echo " use asm = $use_asm" echo " USDT tracing = $use_usdt" echo " sanitizers = $use_sanitizers" echo " debug enabled = $enable_debug" diff --git a/src/Makefile.am b/src/Makefile.am index 3e8870c828..3e24ea5a7e 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -50,10 +50,8 @@ LIBBITCOIN_WALLET_TOOL=libbitcoin_wallet_tool.a endif LIBBITCOIN_CRYPTO = $(LIBBITCOIN_CRYPTO_BASE) -if USE_ASM LIBBITCOIN_CRYPTO_SSE4 = crypto/libbitcoin_crypto_sse4.la LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_SSE4) -endif if ENABLE_SSE41 LIBBITCOIN_CRYPTO_SSE41 = crypto/libbitcoin_crypto_sse41.la LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_SSE41) diff --git a/src/crypto/sha256.cpp b/src/crypto/sha256.cpp index 36ef6d9a1a..4c7bb6f20f 100644 --- a/src/crypto/sha256.cpp +++ b/src/crypto/sha256.cpp @@ -26,13 +26,11 @@ #endif #if defined(__x86_64__) || defined(__amd64__) || defined(__i386__) -#if defined(USE_ASM) namespace sha256_sse4 { void Transform(uint32_t* s, const unsigned char* chunk, size_t blocks); } #endif -#endif namespace sha256d64_sse41 { @@ -574,7 +572,7 @@ bool SelfTest() { } #if !defined(DISABLE_OPTIMIZED_SHA256) -#if defined(USE_ASM) && (defined(__x86_64__) || defined(__amd64__) || defined(__i386__)) +#if (defined(__x86_64__) || defined(__amd64__) || defined(__i386__)) /** Check whether the OS has enabled AVX registers. */ bool AVXEnabled() { @@ -597,7 +595,7 @@ std::string SHA256AutoDetect(sha256_implementation::UseImplementation use_implem TransformD64_8way = nullptr; #if !defined(DISABLE_OPTIMIZED_SHA256) -#if defined(USE_ASM) && defined(HAVE_GETCPUID) +#if defined(HAVE_GETCPUID) bool have_sse4 = false; bool have_xsave = false; bool have_avx = false; @@ -654,7 +652,7 @@ std::string SHA256AutoDetect(sha256_implementation::UseImplementation use_implem ret += ",avx2(8way)"; } #endif -#endif // defined(USE_ASM) && defined(HAVE_GETCPUID) +#endif // defined(HAVE_GETCPUID) #if defined(ENABLE_ARM_SHANI) bool have_arm_shani = false; From f8a06f7a02be83e9b76a1b31f1b66a965dbedfce Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Mon, 26 Feb 2024 16:07:19 +0000 Subject: [PATCH 47/50] doc: remove references to disable-asm option now that it's gone The comment about sha256_sse4::Transform is believed to be old and stale. --- doc/developer-notes.md | 7 ------- doc/fuzzing.md | 7 +------ 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 8c3845c66c..13b9016d40 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -577,13 +577,6 @@ export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:prin See the CI config for more examples, and upstream documentation for more information about any additional options. -There are a number of known problems when using the `address` sanitizer. The -address sanitizer is known to fail in -[sha256_sse4::Transform](/src/crypto/sha256_sse4.cpp) which makes it unusable -unless you also use `--disable-asm` when running configure. We would like to fix -sanitizer issues, so please send pull requests if you can fix any errors found -by the address sanitizer (or any other sanitizer). - Not all sanitizer options can be enabled at the same time, e.g. trying to build with `--with-sanitizers=address,thread` will fail in the configure script as these sanitizers are mutually incompatible. Refer to your compiler manual to diff --git a/doc/fuzzing.md b/doc/fuzzing.md index a4b0198dd9..c9fb918c8f 100644 --- a/doc/fuzzing.md +++ b/doc/fuzzing.md @@ -127,11 +127,6 @@ The default Clang/LLVM version supplied by Apple on macOS does not include fuzzing libraries, so macOS users will need to install a full version, for example using `brew install llvm`. -Should you run into problems with the address sanitizer, it is possible you -may need to run `./configure` with `--disable-asm` to avoid errors -with certain assembly code from Bitcoin Core's code. See [developer notes on sanitizers](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#sanitizers) -for more information. - You may also need to take care of giving the correct path for `clang` and `clang++`, like `CC=/path/to/clang CXX=/path/to/clang++` if the non-systems `clang` does not come first in your path. @@ -139,7 +134,7 @@ You may also need to take care of giving the correct path for `clang` and Full configure that was tested on macOS with `brew` installed `llvm`: ```sh -./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined --disable-asm CC=$(brew --prefix llvm)/bin/clang CXX=$(brew --prefix llvm)/bin/clang++ +./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined CC=$(brew --prefix llvm)/bin/clang CXX=$(brew --prefix llvm)/bin/clang++ ``` Read the [libFuzzer documentation](https://llvm.org/docs/LibFuzzer.html) for more information. This [libFuzzer tutorial](https://github.com/google/fuzzing/blob/master/tutorial/libFuzzerTutorial.md) might also be of interest. From efb70cd6452ce1f0d9f5464bec837b09ed5c2a78 Mon Sep 17 00:00:00 2001 From: jrakibi Date: Thu, 29 Feb 2024 20:50:50 +0100 Subject: [PATCH 48/50] doc: correct function name in AssumeUTXO design docs --- doc/design/assumeutxo.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/design/assumeutxo.md b/doc/design/assumeutxo.md index 66962a629d..abb623fc69 100644 --- a/doc/design/assumeutxo.md +++ b/doc/design/assumeutxo.md @@ -21,7 +21,7 @@ minimum and uses at least 1100 MiB. As the background sync continues there will be temporarily two chainstate directories, each multiple gigabytes in size (likely growing larger than the -the downloaded snapshot). +downloaded snapshot). ### Indexes @@ -145,7 +145,7 @@ sequentially. Once the tip of the background chainstate hits the base block of the snapshot chainstate, we stop use of the background chainstate by setting `m_disabled`, in -`CompleteSnapshotValidation()`, which is checked in `ActivateBestChain()`). We hash the +`MaybeCompleteSnapshotValidation()`, which is checked in `ActivateBestChain()`). We hash the background chainstate's UTXO set contents and ensure it matches the compiled value in `CMainParams::m_assumeutxo_data`. From a1fbde0ef7cf6c94d4a5181f8ceb327096713160 Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Thu, 29 Feb 2024 16:38:58 -0500 Subject: [PATCH 49/50] p2p: Don't consider blocks mutated if they don't connect to known prev block --- src/net_processing.cpp | 3 ++- test/functional/p2p_mutated_blocks.py | 22 +++++++++++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5c3ec5f700..99ae0e8fa1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4721,7 +4721,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, const CBlockIndex* prev_block{WITH_LOCK(m_chainman.GetMutex(), return m_chainman.m_blockman.LookupBlockIndex(pblock->hashPrevBlock))}; - if (IsBlockMutated(/*block=*/*pblock, + // Check for possible mutation if it connects to something we know so we can check for DEPLOYMENT_SEGWIT being active + if (prev_block && IsBlockMutated(/*block=*/*pblock, /*check_witness_root=*/DeploymentActiveAfter(prev_block, m_chainman, Consensus::DEPLOYMENT_SEGWIT))) { LogDebug(BCLog::NET, "Received mutated block from peer=%d\n", peer->m_id); Misbehaving(*peer, 100, "mutated block"); diff --git a/test/functional/p2p_mutated_blocks.py b/test/functional/p2p_mutated_blocks.py index 20f618dc6e..737edaf5bf 100755 --- a/test/functional/p2p_mutated_blocks.py +++ b/test/functional/p2p_mutated_blocks.py @@ -31,6 +31,11 @@ class MutatedBlocksTest(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 1 + self.extra_args = [ + [ + "-testactivationheight=segwit@1", # causes unconnected headers/blocks to not have segwit considered deployed + ], + ] def run_test(self): self.wallet = MiniWallet(self.nodes[0]) @@ -74,7 +79,7 @@ def self_transfer_requested(): # Attempt to clear the honest relayer's download request by sending the # mutated block (as the attacker). - with self.nodes[0].assert_debug_log(expected_msgs=["bad-txnmrklroot, hashMerkleRoot mismatch"]): + with self.nodes[0].assert_debug_log(expected_msgs=["Block mutated: bad-txnmrklroot, hashMerkleRoot mismatch"]): attacker.send_message(msg_block(mutated_block)) # Attacker should get disconnected for sending a mutated block attacker.wait_for_disconnect(timeout=5) @@ -92,5 +97,20 @@ def self_transfer_requested(): honest_relayer.send_and_ping(block_txn) assert_equal(self.nodes[0].getbestblockhash(), block.hash) + # Check that unexpected-witness mutation check doesn't trigger on a header that doesn't connect to anything + assert_equal(len(self.nodes[0].getpeerinfo()), 1) + attacker = self.nodes[0].add_p2p_connection(P2PInterface()) + block_missing_prev = copy.deepcopy(block) + block_missing_prev.hashPrevBlock = 123 + block_missing_prev.solve() + + # Attacker gets a DoS score of 10, not immediately disconnected, so we do it 10 times to get to 100 + for _ in range(10): + assert_equal(len(self.nodes[0].getpeerinfo()), 2) + with self.nodes[0].assert_debug_log(expected_msgs=["AcceptBlock FAILED (prev-blk-not-found)"]): + attacker.send_message(msg_block(block_missing_prev)) + attacker.wait_for_disconnect(timeout=5) + + if __name__ == '__main__': MutatedBlocksTest().main() From 521693378b86aaae5af1646c3a18a902cc835c69 Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 1 Mar 2024 11:52:00 -0500 Subject: [PATCH 50/50] build: move sha256_sse4 into libbitcoin_crypto_base Followup to discussion in #29407. Drops LIBBITCOIN_CRYPTO_SSE4. --- src/Makefile.am | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 3e24ea5a7e..b5d5c4652a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -50,8 +50,6 @@ LIBBITCOIN_WALLET_TOOL=libbitcoin_wallet_tool.a endif LIBBITCOIN_CRYPTO = $(LIBBITCOIN_CRYPTO_BASE) -LIBBITCOIN_CRYPTO_SSE4 = crypto/libbitcoin_crypto_sse4.la -LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_SSE4) if ENABLE_SSE41 LIBBITCOIN_CRYPTO_SSE41 = crypto/libbitcoin_crypto_sse41.la LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_SSE41) @@ -582,6 +580,7 @@ crypto_libbitcoin_crypto_base_la_SOURCES = \ crypto/sha1.h \ crypto/sha256.cpp \ crypto/sha256.h \ + crypto/sha256_sse4.cpp \ crypto/sha3.cpp \ crypto/sha3.h \ crypto/sha512.cpp \ @@ -589,13 +588,6 @@ crypto_libbitcoin_crypto_base_la_SOURCES = \ crypto/siphash.cpp \ crypto/siphash.h -# See explanation for -static in crypto_libbitcoin_crypto_base_la's LDFLAGS and -# CXXFLAGS above -crypto_libbitcoin_crypto_sse4_la_LDFLAGS = $(AM_LDFLAGS) -static -crypto_libbitcoin_crypto_sse4_la_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) -static -crypto_libbitcoin_crypto_sse4_la_CPPFLAGS = $(AM_CPPFLAGS) -crypto_libbitcoin_crypto_sse4_la_SOURCES = crypto/sha256_sse4.cpp - # See explanation for -static in crypto_libbitcoin_crypto_base_la's LDFLAGS and # CXXFLAGS above crypto_libbitcoin_crypto_sse41_la_LDFLAGS = $(AM_LDFLAGS) -static