From bf9669af9ccc33dcade09bceb27d6745e9d9a75a Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Tue, 30 Jan 2024 19:27:21 +0530 Subject: [PATCH 01/60] test: Rename early key response test and move random_bitflip to util Early key response test is a special kind of test which requires modified v2 handshake functions. More such tests can be added where v2 handshake functions send incorrect garbage terminator, excess garbage bytes etc.. Hence, rename p2p_v2_earlykey.py to a general test file name - p2p_v2_misbehaving.py. random_bitflip function (used in signature tests prior to this commit) can be used in p2p_v2_misbehaving test to generate wrong garbage terminator, wrong garbage bytes etc.. So, move the function to util. --- .../{p2p_v2_earlykeyresponse.py => p2p_v2_misbehaving.py} | 0 test/functional/test_framework/key.py | 6 +----- test/functional/test_framework/util.py | 7 +++++++ test/functional/test_runner.py | 2 +- 4 files changed, 9 insertions(+), 6 deletions(-) rename test/functional/{p2p_v2_earlykeyresponse.py => p2p_v2_misbehaving.py} (100%) diff --git a/test/functional/p2p_v2_earlykeyresponse.py b/test/functional/p2p_v2_misbehaving.py similarity index 100% rename from test/functional/p2p_v2_earlykeyresponse.py rename to test/functional/p2p_v2_misbehaving.py diff --git a/test/functional/test_framework/key.py b/test/functional/test_framework/key.py index 06252f8996..939c7cbef6 100644 --- a/test/functional/test_framework/key.py +++ b/test/functional/test_framework/key.py @@ -14,6 +14,7 @@ import unittest from test_framework.crypto import secp256k1 +from test_framework.util import random_bitflip # Point with no known discrete log. H_POINT = "50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0" @@ -292,11 +293,6 @@ def sign_schnorr(key, msg, aux=None, flip_p=False, flip_r=False): class TestFrameworkKey(unittest.TestCase): def test_ecdsa_and_schnorr(self): """Test the Python ECDSA and Schnorr implementations.""" - def random_bitflip(sig): - sig = list(sig) - sig[random.randrange(len(sig))] ^= (1 << (random.randrange(8))) - return bytes(sig) - byte_arrays = [generate_privkey() for _ in range(3)] + [v.to_bytes(32, 'big') for v in [0, ORDER - 1, ORDER, 2**256 - 1]] keys = {} for privkey_bytes in byte_arrays: # build array of key/pubkey pairs diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index b4b05b1597..53f4a8ee4d 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -14,6 +14,7 @@ import os import pathlib import platform +import random import re import time @@ -230,6 +231,12 @@ def ceildiv(a, b): return -(-a // b) +def random_bitflip(data): + data = list(data) + data[random.randrange(len(data))] ^= (1 << (random.randrange(8))) + return bytes(data) + + def get_fee(tx_size, feerate_btc_kvb): """Calculate the fee in BTC given a feerate is BTC/kvB. Reflects CFeeRate::GetFee""" feerate_sat_kvb = int(feerate_btc_kvb * Decimal(1e8)) # Fee in sat/kvb as an int to avoid float precision errors diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 4d66ea97c8..964b7ab6aa 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -264,7 +264,7 @@ 'p2p_invalid_tx.py --v2transport', 'p2p_v2_transport.py', 'p2p_v2_encrypted.py', - 'p2p_v2_earlykeyresponse.py', + 'p2p_v2_misbehaving.py', 'example_test.py', 'mempool_accept_v3.py', 'wallet_txn_doublespend.py --legacy-wallet', From 86cca2cba230c10324c6aedd12ae9655b83b2856 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Tue, 13 Feb 2024 10:37:23 +0530 Subject: [PATCH 02/60] test: Support disconnect waiting for add_p2p_connection Adds a new boolean parameter `expect_success` to the `add_p2p_connection` method. If set, the node under test doesn't wait for connection to be established and is useful for testing scenarios when disconnection is expected. Without this parameter, intermittent test failures can happen if the disconnection happens before wait_until for is_connected is hit inside `add_p2p_connection`. Co-Authored-By: Sebastian Falbesoner --- test/functional/test_framework/test_node.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 3baa78fd79..d6cb5262ba 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=None, 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, expect_success=True, **kwargs): """Add an inbound p2p connection to the node. This method adds the p2p connection to the self.p2ps list and also @@ -687,7 +687,6 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, send_version=Tru if supports_v2_p2p is None: supports_v2_p2p = self.use_v2transport - p2p_conn.p2p_connected_to_node = True if self.use_v2transport: kwargs['services'] = kwargs.get('services', P2P_SERVICES) | NODE_P2P_V2 @@ -695,6 +694,8 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, send_version=Tru p2p_conn.peer_connect(**kwargs, send_version=send_version, net=self.chain, timeout_factor=self.timeout_factor, supports_v2_p2p=supports_v2_p2p)() self.p2ps.append(p2p_conn) + if not expect_success: + return 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) From 9e13ccc50eec9d2efe0f472e6d50dc822df70d84 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 11 Apr 2024 10:51:43 -0400 Subject: [PATCH 03/60] psbt: Check non witness utxo outpoint early A common issue that our fuzzers keep finding is that outpoints don't exist in the non witness utxos. Instead of trying to track this down and checking in various individual places, do the check early during deserialization. --- src/psbt.h | 9 +++++++-- test/functional/data/rpc_psbt.json | 4 +++- test/functional/rpc_psbt.py | 6 ++---- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/psbt.h b/src/psbt.h index 3f74083717..a6837fda49 100644 --- a/src/psbt.h +++ b/src/psbt.h @@ -1173,8 +1173,13 @@ struct PartiallySignedTransaction inputs.push_back(input); // Make sure the non-witness utxo matches the outpoint - if (input.non_witness_utxo && input.non_witness_utxo->GetHash() != tx->vin[i].prevout.hash) { - throw std::ios_base::failure("Non-witness UTXO does not match outpoint hash"); + if (input.non_witness_utxo) { + if (input.non_witness_utxo->GetHash() != tx->vin[i].prevout.hash) { + throw std::ios_base::failure("Non-witness UTXO does not match outpoint hash"); + } + if (tx->vin[i].prevout.n >= input.non_witness_utxo->vout.size()) { + throw std::ios_base::failure("Input specifies output index that does not exist"); + } } ++i; } diff --git a/test/functional/data/rpc_psbt.json b/test/functional/data/rpc_psbt.json index 3127350872..1ccc5e0ba0 100644 --- a/test/functional/data/rpc_psbt.json +++ b/test/functional/data/rpc_psbt.json @@ -38,7 +38,9 @@ "cHNidP8BAF4CAAAAAZvUh2UjC/mnLmYgAflyVW5U8Mb5f+tWvLVgDYF/aZUmAQAAAAD/////AUjmBSoBAAAAIlEgAw2k/OT32yjCyylRYx4ANxOFZZf+ljiCy1AOaBEsymMAAAAAAAEBKwDyBSoBAAAAIlEgwiR++/2SrEf29AuNQtFpF1oZ+p+hDkol1/NetN2FtpJBFCyxOsaCSN6AaqajZZzzwD62gh0JyBFKToaP696GW7bSzZcOFfU/wMgvlQ/VYP+pGbdhcr4Bc2iomROvB09ACwlCiXVqo3OczGiewPzzo2C+MswLWbFuk6Hou0YFcmssp6P/cGxBdmSWMrLMaOH5ErileONxnOdxCIXHqWb0m81DywEBAAA=", "cHNidP8BAF4CAAAAAZvUh2UjC/mnLmYgAflyVW5U8Mb5f+tWvLVgDYF/aZUmAQAAAAD/////AUjmBSoBAAAAIlEgAw2k/OT32yjCyylRYx4ANxOFZZf+ljiCy1AOaBEsymMAAAAAAAEBKwDyBSoBAAAAIlEgwiR++/2SrEf29AuNQtFpF1oZ+p+hDkol1/NetN2FtpJBFCyxOsaCSN6AaqajZZzzwD62gh0JyBFKToaP696GW7bSzZcOFfU/wMgvlQ/VYP+pGbdhcr4Bc2iomROvB09ACwk5iXVqo3OczGiewPzzo2C+MswLWbFuk6Hou0YFcmssp6P/cGxBdmSWMrLMaOH5ErileONxnOdxCIXHqWb0m81DywAA", "cHNidP8BAF4CAAAAAZvUh2UjC/mnLmYgAflyVW5U8Mb5f+tWvLVgDYF/aZUmAQAAAAD/////AUjmBSoBAAAAIlEgAw2k/OT32yjCyylRYx4ANxOFZZf+ljiCy1AOaBEsymMAAAAAAAEBKwDyBSoBAAAAIlEgwiR++/2SrEf29AuNQtFpF1oZ+p+hDkol1/NetN2FtpJjFcFQkpt0waBJVLeLS2A16XpeB4paDyjsltVHv+6azoA6wG99YgWelJehpKJnVp2YdtpgEBr/OONSm5uTnOf5GulwEV8uSQr3zEXE94UR82BXzlxaXFYyWin7RN/CA/NW4fgAIyAssTrGgkjegGqmo2Wc88A+toIdCcgRSk6Gj+vehlu20qzAAAA=", - "cHNidP8BAF4CAAAAAZvUh2UjC/mnLmYgAflyVW5U8Mb5f+tWvLVgDYF/aZUmAQAAAAD/////AUjmBSoBAAAAIlEgAw2k/OT32yjCyylRYx4ANxOFZZf+ljiCy1AOaBEsymMAAAAAAAEBKwDyBSoBAAAAIlEgwiR++/2SrEf29AuNQtFpF1oZ+p+hDkol1/NetN2FtpJhFcFQkpt0waBJVLeLS2A16XpeB4paDyjsltVHv+6azoA6wG99YgWelJehpKJnVp2YdtpgEBr/OONSm5uTnOf5GulwEV8uSQr3zEXE94UR82BXzlxaXFYyWin7RN/CA/NW4SMgLLE6xoJI3oBqpqNlnPPAPraCHQnIEUpOho/r3oZbttKswAAA" + "cHNidP8BAF4CAAAAAZvUh2UjC/mnLmYgAflyVW5U8Mb5f+tWvLVgDYF/aZUmAQAAAAD/////AUjmBSoBAAAAIlEgAw2k/OT32yjCyylRYx4ANxOFZZf+ljiCy1AOaBEsymMAAAAAAAEBKwDyBSoBAAAAIlEgwiR++/2SrEf29AuNQtFpF1oZ+p+hDkol1/NetN2FtpJhFcFQkpt0waBJVLeLS2A16XpeB4paDyjsltVHv+6azoA6wG99YgWelJehpKJnVp2YdtpgEBr/OONSm5uTnOf5GulwEV8uSQr3zEXE94UR82BXzlxaXFYyWin7RN/CA/NW4SMgLLE6xoJI3oBqpqNlnPPAPraCHQnIEUpOho/r3oZbttKswAAA", + "cHNidP8BAHUCAAAAAQCBcTce3/KF6Tet7qSze3gADAVmy7OtZGQXE8pCFxv2AAAAAAD+////AtPf9QUAAAAAGXapFNDFmQPFusKGh2DpD9UhpGZap2UgiKwA4fUFAAAAABepFDVF5uM7gyxHBQ8k0+65PJwDlIvHh7MuEwAAAQD9pQEBAAAAAAECiaPHHqtNIOA3G7ukzGmPopXJRjr6Ljl/hTPMti+VZ+UBAAAAFxYAFL4Y0VKpsBIDna89p95PUzSe7LmF/////4b4qkOnHf8USIk6UwpyN+9rRgi7st0tAXHmOuxqSJC0AQAAABcWABT+Pp7xp0XpdNkCxDVZQ6vLNL1TU/////8CAMLrCwAAAAAZdqkUhc/xCX/Z4Ai7NK9wnGIZeziXikiIrHL++E4sAAAAF6kUM5cluiHv1irHU6m80GfWx6ajnQWHAkcwRAIgJxK+IuAnDzlPVoMR3HyppolwuAJf3TskAinwf4pfOiQCIAGLONfc0xTnNMkna9b7QPZzMlvEuqFEyADS8vAtsnZcASED0uFWdJQbrUqZY3LLh+GFbTZSYG2YVi/jnF6efkE/IQUCSDBFAiEA0SuFLYXc2WHS9fSrZgZU327tzHlMDDPOXMMJ/7X85Y0CIGczio4OFyXBl/saiK9Z9R5E5CVbIBZ8hoQDHAXR8lkqASECI7cr7vCWXRC+B3jv7NYfysb3mk6haTkzgHNEZPhPKrMAAAAAAAAA", + "cHNidP8BAHUCAAAAASaBcTce3/KF6Tet7qSze3gADAVmy7OtZGQXE8pCFxv2AAAAAgD+////AtPf9QUAAAAAGXapFNDFmQPFusKGh2DpD9UhpGZap2UgiKwA4fUFAAAAABepFDVF5uM7gyxHBQ8k0+65PJwDlIvHh7MuEwAAAQD9pQEBAAAAAAECiaPHHqtNIOA3G7ukzGmPopXJRjr6Ljl/hTPMti+VZ+UBAAAAFxYAFL4Y0VKpsBIDna89p95PUzSe7LmF/////4b4qkOnHf8USIk6UwpyN+9rRgi7st0tAXHmOuxqSJC0AQAAABcWABT+Pp7xp0XpdNkCxDVZQ6vLNL1TU/////8CAMLrCwAAAAAZdqkUhc/xCX/Z4Ai7NK9wnGIZeziXikiIrHL++E4sAAAAF6kUM5cluiHv1irHU6m80GfWx6ajnQWHAkcwRAIgJxK+IuAnDzlPVoMR3HyppolwuAJf3TskAinwf4pfOiQCIAGLONfc0xTnNMkna9b7QPZzMlvEuqFEyADS8vAtsnZcASED0uFWdJQbrUqZY3LLh+GFbTZSYG2YVi/jnF6efkE/IQUCSDBFAiEA0SuFLYXc2WHS9fSrZgZU327tzHlMDDPOXMMJ/7X85Y0CIGczio4OFyXBl/saiK9Z9R5E5CVbIBZ8hoQDHAXR8lkqASECI7cr7vCWXRC+B3jv7NYfysb3mk6haTkzgHNEZPhPKrMAAAAAAAAA" ], "invalid_with_msg": [ [ diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 016aa3ba11..4426221006 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -701,11 +701,9 @@ def test_psbt_input_keys(psbt_input, keys): assert_equal(analysis['next'], 'creator') assert_equal(analysis['error'], 'PSBT is not valid. Output amount invalid') - analysis = self.nodes[0].analyzepsbt('cHNidP8BAJoCAAAAAkvEW8NnDtdNtDpsmze+Ht2LH35IJcKv00jKAlUs21RrAwAAAAD/////S8Rbw2cO1020OmybN74e3Ysffkglwq/TSMoCVSzbVGsBAAAAAP7///8CwLYClQAAAAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQD5ApUAAAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIAAAAAAAEAnQIAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP7///9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/v///wIA+QKVAAAAABl2qRT9zXUVA8Ls5iVqynLHe5/vSe1XyYisQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAAAAAQEfQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==') - assert_equal(analysis['next'], 'creator') - assert_equal(analysis['error'], 'PSBT is not valid. Input 0 specifies invalid prevout') + assert_raises_rpc_error(-22, "TX decode failed", self.nodes[0].analyzepsbt, "cHNidP8BAJoCAAAAAkvEW8NnDtdNtDpsmze+Ht2LH35IJcKv00jKAlUs21RrAwAAAAD/////S8Rbw2cO1020OmybN74e3Ysffkglwq/TSMoCVSzbVGsBAAAAAP7///8CwLYClQAAAAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQD5ApUAAAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIAAAAAAAEAnQIAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP7///9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/v///wIA+QKVAAAAABl2qRT9zXUVA8Ls5iVqynLHe5/vSe1XyYisQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAAAAAQEfQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==") - assert_raises_rpc_error(-25, 'Inputs missing or spent', self.nodes[0].walletprocesspsbt, 'cHNidP8BAJoCAAAAAkvEW8NnDtdNtDpsmze+Ht2LH35IJcKv00jKAlUs21RrAwAAAAD/////S8Rbw2cO1020OmybN74e3Ysffkglwq/TSMoCVSzbVGsBAAAAAP7///8CwLYClQAAAAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQD5ApUAAAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIAAAAAAAEAnQIAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP7///9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/v///wIA+QKVAAAAABl2qRT9zXUVA8Ls5iVqynLHe5/vSe1XyYisQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAAAAAQEfQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==') + assert_raises_rpc_error(-22, "TX decode failed", self.nodes[0].walletprocesspsbt, "cHNidP8BAJoCAAAAAkvEW8NnDtdNtDpsmze+Ht2LH35IJcKv00jKAlUs21RrAwAAAAD/////S8Rbw2cO1020OmybN74e3Ysffkglwq/TSMoCVSzbVGsBAAAAAP7///8CwLYClQAAAAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQD5ApUAAAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIAAAAAAAEAnQIAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP7///9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/v///wIA+QKVAAAAABl2qRT9zXUVA8Ls5iVqynLHe5/vSe1XyYisQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAAAAAQEfQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==") self.log.info("Test that we can fund psbts with external inputs specified") From c642b08c4e45cb3a625a867ebd66c0ae51bde212 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Fri, 17 May 2024 09:40:38 +0530 Subject: [PATCH 04/60] test: Log when the garbage is actually sent to transport layer Currently, we log the number of bytes of garbage when it is generated. The log is a better fit for when the garbage actually gets sent to the transport layer. --- test/functional/test_framework/p2p.py | 2 ++ test/functional/test_framework/v2_p2p.py | 3 --- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py index dc04696114..f11a3dd893 100755 --- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -223,6 +223,7 @@ def connection_made(self, transport): # send the initial handshake immediately if self.supports_v2_p2p and self.v2_state.initiating and not self.v2_state.tried_v2_handshake: send_handshake_bytes = self.v2_state.initiate_v2_handshake() + logger.debug(f"sending {len(self.v2_state.sent_garbage)} bytes of garbage data") self.send_raw_message(send_handshake_bytes) # for v1 outbound connections, send version message immediately after opening # (for v2 outbound connections, send it after the initial v2 handshake) @@ -262,6 +263,7 @@ def _on_data_v2_handshake(self): self.v2_state = None return elif send_handshake_bytes: + logger.debug(f"sending {len(self.v2_state.sent_garbage)} bytes of garbage data") self.send_raw_message(send_handshake_bytes) elif send_handshake_bytes == b"": return # only after send_handshake_bytes are sent can `complete_handshake()` be done diff --git a/test/functional/test_framework/v2_p2p.py b/test/functional/test_framework/v2_p2p.py index 8f79623bd8..8b061fcd61 100644 --- a/test/functional/test_framework/v2_p2p.py +++ b/test/functional/test_framework/v2_p2p.py @@ -4,7 +4,6 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Class for v2 P2P protocol (see BIP 324)""" -import logging import random from .crypto.bip324_cipher import FSChaCha20Poly1305 @@ -14,7 +13,6 @@ from .key import TaggedHash from .messages import MAGIC_BYTES -logger = logging.getLogger("TestFramework.v2_p2p") CHACHA20POLY1305_EXPANSION = 16 HEADER_LEN = 1 @@ -116,7 +114,6 @@ def generate_keypair_and_garbage(self): self.privkey_ours, self.ellswift_ours = ellswift_create() garbage_len = random.randrange(MAX_GARBAGE_LEN + 1) self.sent_garbage = random.randbytes(garbage_len) - logger.debug(f"sending {garbage_len} bytes of garbage data") return self.ellswift_ours + self.sent_garbage def initiate_v2_handshake(self): From d93b79470916b1e6f85c55cc6beb1e41b382196f Mon Sep 17 00:00:00 2001 From: Michael Dietz Date: Fri, 29 Dec 2023 14:25:19 -0600 Subject: [PATCH 05/60] tests: improve wallet multisig descriptor test and docs It is best to store all key origin information (master key fingerprint and all derivation steps) in the multisig descriptor. Being explicit with this information should be beneficial if this approach is used with other wallets/signers (whether hardware or software). There is no harm including all of this with xpubs (if anything it simplifies the test code) and makes this example/docs more complete and safer incase it is referenced by others. --- doc/descriptors.md | 5 ++-- .../wallet_multisig_descriptor_psbt.py | 25 ++++++++----------- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/doc/descriptors.md b/doc/descriptors.md index 3b94ec03e4..5e8e4a24b0 100644 --- a/doc/descriptors.md +++ b/doc/descriptors.md @@ -63,6 +63,7 @@ Output descriptors currently support: - `wsh(sortedmulti(1,xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/1/0/*,xpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3ZuxV27CprR9LgpeyGmXUbC6wb7ERfvrnKZjXoUmmDznezpbZb7ap6r1D3tgFxHmwMkQTPH/0/0/*))` describes a set of *1-of-2* P2WSH multisig outputs where one multisig key is the *1/0/`i`* child of the first specified xpub and the other multisig key is the *0/0/`i`* child of the second specified xpub, and `i` is any number in a configurable range (`0-1000` by default). The order of public keys in the resulting witnessScripts is determined by the lexicographic order of the public keys at that index. - `tr(c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5,{pk(fff97bd5755eeea420453a14355235d382f6472f8568a18b2f057a1460297556),pk(e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd13)})` describes a P2TR output with the `c6...` x-only pubkey as internal key, and two script paths. - `tr(c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5,sortedmulti_a(2,2f8bde4d1a07209355b4a7250a5c5128e88b84bddc619ab7cba8d569b240efe4,5cbdf0646e5db4eaa398f365f2ea7a0e3d419b7e0330e39ce92bddedcac4f9bc))` describes a P2TR output with the `c6...` x-only pubkey as internal key, and a single `multi_a` script that needs 2 signatures with 2 specified x-only keys, which will be sorted lexicographically. +- `wsh(sortedmulti(2,[6f53d49c/44h/1h/0h]tpubDDjsCRDQ9YzyaAq9rspCfq8RZFrWoBpYnLxK6sS2hS2yukqSczgcYiur8Scx4Hd5AZatxTuzMtJQJhchufv1FRFanLqUP7JHwusSSpfcEp2/0/*,[e6807791/44h/1h/0h]tpubDDAfvogaaAxaFJ6c15ht7Tq6ZmiqFYfrSmZsHu7tHXBgnjMZSHAeHSwhvjARNA6Qybon4ksPksjRbPDVp7yXA1KjTjSd5x18KHqbppnXP1s/0/*,[367c9cfa/44h/1h/0h]tpubDDtPnSgWYk8dDnaDwnof4ehcnjuL5VoUt1eW2MoAed1grPHuXPDnkX1fWMvXfcz3NqFxPbhqNZ3QBdYjLz2hABeM9Z2oqMR1Gt2HHYDoCgh/0/*))#av0kxgw0` describes a *2-of-3* multisig. For brevity, the internal "change" descriptor accompanying the above external "receiving" descriptor is not included here, but it typically differs only in the xpub derivation steps, ending in `/1/*` for change addresses. ## Reference @@ -167,9 +168,9 @@ The basic steps are: the participant's signer wallet. Avoid reusing this wallet for any purpose other than signing transactions from the corresponding multisig we are about to create. Hint: extract the wallet's xpubs using `listdescriptors` and pick the one from the `pkh` descriptor since it's least likely to be accidentally reused (legacy addresses) - 2. Create a watch-only descriptor wallet (blank, private keys disabled). Now the multisig is created by importing the two descriptors: + 2. Create a watch-only descriptor wallet (blank, private keys disabled). Now the multisig is created by importing the external and internal descriptors: `wsh(sortedmulti(,XPUB1/0/*,XPUB2/0/*,…,XPUBN/0/*))` and `wsh(sortedmulti(,XPUB1/1/*,XPUB2/1/*,…,XPUBN/1/*))` - (one descriptor w/ `0` for receiving addresses and another w/ `1` for change). Every participant does this + (one descriptor w/ `0` for receiving addresses and another w/ `1` for change). Every participant does this. All key origin information (master key fingerprint and all derivation steps) should be included with xpubs for proper support of hardware devices / external signers 3. A receiving address is generated for the multisig. As a check to ensure step 2 was done correctly, every participant should verify they get the same addresses 4. Funds are sent to the resulting address diff --git a/test/functional/wallet_multisig_descriptor_psbt.py b/test/functional/wallet_multisig_descriptor_psbt.py index 68bf45f7e3..145912025f 100755 --- a/test/functional/wallet_multisig_descriptor_psbt.py +++ b/test/functional/wallet_multisig_descriptor_psbt.py @@ -7,7 +7,6 @@ This is meant to be documentation as much as functional tests, so it is kept as simple and readable as possible. """ -from test_framework.address import base58_to_byte from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_approx, @@ -30,10 +29,12 @@ def skip_test_if_missing_module(self): self.skip_if_no_sqlite() @staticmethod - def _get_xpub(wallet): + def _get_xpub(wallet, internal): """Extract the wallet's xpubs using `listdescriptors` and pick the one from the `pkh` descriptor since it's least likely to be accidentally reused (legacy addresses).""" - descriptor = next(filter(lambda d: d["desc"].startswith("pkh"), wallet.listdescriptors()["descriptors"])) - return descriptor["desc"].split("]")[-1].split("/")[0] + pkh_descriptor = next(filter(lambda d: d["desc"].startswith("pkh(") and d["internal"] == internal, wallet.listdescriptors()["descriptors"])) + # Keep all key origin information (master key fingerprint and all derivation steps) for proper support of hardware devices + # See section 'Key origin identification' in 'doc/descriptors.md' for more details... + return pkh_descriptor["desc"].split("pkh(")[1].split(")")[0] @staticmethod def _check_psbt(psbt, to, value, multisig): @@ -47,19 +48,13 @@ def _check_psbt(psbt, to, value, multisig): amount += vout["value"] assert_approx(amount, float(value), vspan=0.001) - def participants_create_multisigs(self, xpubs): + def participants_create_multisigs(self, external_xpubs, internal_xpubs): """The multisig is created by importing the following descriptors. The resulting wallet is watch-only and every participant can do this.""" - # some simple validation - assert_equal(len(xpubs), self.N) - # a sanity-check/assertion, this will throw if the base58 checksum of any of the provided xpubs are invalid - for xpub in xpubs: - base58_to_byte(xpub) - for i, node in enumerate(self.nodes): node.createwallet(wallet_name=f"{self.name}_{i}", blank=True, descriptors=True, disable_private_keys=True) multisig = node.get_wallet_rpc(f"{self.name}_{i}") - external = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f'/0/*,'.join(xpubs)}/0/*))") - internal = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f'/1/*,'.join(xpubs)}/1/*))") + external = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f','.join(external_xpubs)}))") + internal = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f','.join(internal_xpubs)}))") result = multisig.importdescriptors([ { # receiving addresses (internal: False) "desc": external["descriptor"], @@ -93,10 +88,10 @@ def run_test(self): } self.log.info("Generate and exchange xpubs...") - xpubs = [self._get_xpub(signer) for signer in participants["signers"]] + external_xpubs, internal_xpubs = [[self._get_xpub(signer, internal) for signer in participants["signers"]] for internal in [False, True]] self.log.info("Every participant imports the following descriptors to create the watch-only multisig...") - participants["multisigs"] = list(self.participants_create_multisigs(xpubs)) + participants["multisigs"] = list(self.participants_create_multisigs(external_xpubs, internal_xpubs)) self.log.info("Check that every participant's multisig generates the same addresses...") for _ in range(10): # we check that the first 10 generated addresses are the same for all participant's multisigs From 34c9cee380e7276cf3f85f2ce56a293e57afd676 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 21 May 2024 15:55:26 +0000 Subject: [PATCH 06/60] clang-tidy: add check for non-trivial thread_local vars Do not allow thread_local vars with non-trivial destructors --- contrib/devtools/bitcoin-tidy/CMakeLists.txt | 4 +- .../devtools/bitcoin-tidy/bitcoin-tidy.cpp | 2 + .../example_nontrivial-threadlocal.cpp | 2 + .../bitcoin-tidy/nontrivial-threadlocal.cpp | 44 +++++++++++++++++++ .../bitcoin-tidy/nontrivial-threadlocal.h | 29 ++++++++++++ 5 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 contrib/devtools/bitcoin-tidy/example_nontrivial-threadlocal.cpp create mode 100644 contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp create mode 100644 contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.h diff --git a/contrib/devtools/bitcoin-tidy/CMakeLists.txt b/contrib/devtools/bitcoin-tidy/CMakeLists.txt index 1260c71423..95345b4782 100644 --- a/contrib/devtools/bitcoin-tidy/CMakeLists.txt +++ b/contrib/devtools/bitcoin-tidy/CMakeLists.txt @@ -25,7 +25,7 @@ find_program(CLANG_TIDY_EXE NAMES "clang-tidy-${LLVM_VERSION_MAJOR}" "clang-tidy message(STATUS "Found LLVM ${LLVM_PACKAGE_VERSION}") message(STATUS "Found clang-tidy: ${CLANG_TIDY_EXE}") -add_library(bitcoin-tidy MODULE bitcoin-tidy.cpp logprintf.cpp) +add_library(bitcoin-tidy MODULE bitcoin-tidy.cpp logprintf.cpp nontrivial-threadlocal.cpp) target_include_directories(bitcoin-tidy SYSTEM PRIVATE ${LLVM_INCLUDE_DIRS}) # Disable RTTI and exceptions as necessary @@ -58,7 +58,7 @@ else() endif() # Create a dummy library that runs clang-tidy tests as a side-effect of building -add_library(bitcoin-tidy-tests OBJECT EXCLUDE_FROM_ALL example_logprintf.cpp) +add_library(bitcoin-tidy-tests OBJECT EXCLUDE_FROM_ALL example_logprintf.cpp example_nontrivial-threadlocal.cpp) add_dependencies(bitcoin-tidy-tests bitcoin-tidy) set_target_properties(bitcoin-tidy-tests PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND}") diff --git a/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp b/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp index 0f34d37793..1ef4494973 100644 --- a/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp +++ b/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include "logprintf.h" +#include "nontrivial-threadlocal.h" #include #include @@ -13,6 +14,7 @@ class BitcoinModule final : public clang::tidy::ClangTidyModule void addCheckFactories(clang::tidy::ClangTidyCheckFactories& CheckFactories) override { CheckFactories.registerCheck("bitcoin-unterminated-logprintf"); + CheckFactories.registerCheck("bitcoin-nontrivial-threadlocal"); } }; diff --git a/contrib/devtools/bitcoin-tidy/example_nontrivial-threadlocal.cpp b/contrib/devtools/bitcoin-tidy/example_nontrivial-threadlocal.cpp new file mode 100644 index 0000000000..2b74df5d0e --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/example_nontrivial-threadlocal.cpp @@ -0,0 +1,2 @@ +#include +thread_local std::string foo; diff --git a/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp b/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp new file mode 100644 index 0000000000..d2bc78a31b --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp @@ -0,0 +1,44 @@ +// Copyright (c) 2023 Bitcoin Developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include "nontrivial-threadlocal.h" + +#include +#include + + +// Copied from clang-tidy's UnusedRaiiCheck +namespace { +AST_MATCHER(clang::CXXRecordDecl, hasNonTrivialDestructor) { + // TODO: If the dtor is there but empty we don't want to warn either. + return Node.hasDefinition() && Node.hasNonTrivialDestructor(); +} +} // namespace + +namespace bitcoin { + +void NonTrivialThreadLocal::registerMatchers(clang::ast_matchers::MatchFinder* finder) +{ + using namespace clang::ast_matchers; + + /* + thread_local std::string foo; + */ + + finder->addMatcher( + varDecl( + hasThreadStorageDuration(), + hasType(hasCanonicalType(recordType(hasDeclaration(cxxRecordDecl(hasNonTrivialDestructor()))))) + ).bind("nontrivial_threadlocal"), + this); +} + +void NonTrivialThreadLocal::check(const clang::ast_matchers::MatchFinder::MatchResult& Result) +{ + if (const clang::VarDecl* var = Result.Nodes.getNodeAs("nontrivial_threadlocal")) { + const auto user_diag = diag(var->getBeginLoc(), "Variable with non-trivial destructor cannot be thread_local."); + } +} + +} // namespace bitcoin diff --git a/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.h b/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.h new file mode 100644 index 0000000000..c853073467 --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.h @@ -0,0 +1,29 @@ +// Copyright (c) 2023 Bitcoin Developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef NONTRIVIAL_THREADLOCAL_CHECK_H +#define NONTRIVIAL_THREADLOCAL_CHECK_H + +#include + +namespace bitcoin { + +// Warn about any thread_local variable with a non-trivial destructor. +class NonTrivialThreadLocal final : public clang::tidy::ClangTidyCheck +{ +public: + NonTrivialThreadLocal(clang::StringRef Name, clang::tidy::ClangTidyContext* Context) + : clang::tidy::ClangTidyCheck(Name, Context) {} + + bool isLanguageVersionSupported(const clang::LangOptions& LangOpts) const override + { + return LangOpts.CPlusPlus; + } + void registerMatchers(clang::ast_matchers::MatchFinder* Finder) override; + void check(const clang::ast_matchers::MatchFinder::MatchResult& Result) override; +}; + +} // namespace bitcoin + +#endif // NONTRIVIAL_THREADLOCAL_CHECK_H From d4a1da8543522a213ac75761131d878eedfd4a5b Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Fri, 17 May 2024 10:06:59 +0530 Subject: [PATCH 07/60] test: Make global TRANSPORT_VERSION variable an instance variable Currently, transport version is a global variable declared as TRANSPORT_VERSION in v2_p2p.py. Making it an instance variable would help in sending non empty transport version packets for testing purposes. It might also help EncryptedP2PState be more extensible in far future protocol upgrades. --- test/functional/test_framework/v2_p2p.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/test_framework/v2_p2p.py b/test/functional/test_framework/v2_p2p.py index 8b061fcd61..0d2f7f6ad2 100644 --- a/test/functional/test_framework/v2_p2p.py +++ b/test/functional/test_framework/v2_p2p.py @@ -19,7 +19,6 @@ IGNORE_BIT_POS = 7 LENGTH_FIELD_LEN = 3 MAX_GARBAGE_LEN = 4095 -TRANSPORT_VERSION = b'' SHORTID = { 1: b"addr", @@ -93,6 +92,7 @@ def __init__(self, *, initiating, net): # has been decrypted. set to -1 if decryption hasn't been done yet. self.contents_len = -1 self.found_garbage_terminator = False + self.transport_version = b'' @staticmethod def v2_ecdh(priv, ellswift_theirs, ellswift_ours, initiating): @@ -169,7 +169,7 @@ def complete_handshake(self, response): msg_to_send += self.v2_enc_packet(decoy_content_len * b'\x00', aad=aad, ignore=True) aad = b'' # Send version packet. - msg_to_send += self.v2_enc_packet(TRANSPORT_VERSION, aad=aad) + msg_to_send += self.v2_enc_packet(self.transport_version, aad=aad) return 64 - len(self.received_prefix), msg_to_send def authenticate_handshake(self, response): From 7d07daa62311bdb0e2ce23d0b55f711f5088bd28 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Tue, 28 May 2024 11:32:04 +0530 Subject: [PATCH 08/60] log: Add V2 handshake timeout --- src/net.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/net.cpp b/src/net.cpp index 7c82f01d75..b3f2a15709 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1988,7 +1988,11 @@ bool CConnman::InactivityCheck(const CNode& node) const } if (!node.fSuccessfullyConnected) { - LogPrint(BCLog::NET, "version handshake timeout peer=%d\n", node.GetId()); + if (node.m_transport->GetInfo().transport_type == TransportProtocolType::DETECTING) { + LogPrint(BCLog::NET, "V2 handshake timeout peer=%d\n", node.GetId()); + } else { + LogPrint(BCLog::NET, "version handshake timeout peer=%d\n", node.GetId()); + } return true; } From 32b1d1379258aa57c2a7e278f60d1117fcf17953 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Wed, 5 Jun 2024 17:42:27 +0000 Subject: [PATCH 09/60] refactor: add self-assign checks to classes which violate the clang-tidy check Both of these cases appear to be harmless, but adding the tests allows us to turn on the aggressive clang-tidy checks. --- src/arith_uint256.h | 6 ++++-- src/key.h | 14 ++++++++------ src/test/util_tests.cpp | 6 ++++-- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/arith_uint256.h b/src/arith_uint256.h index ba36cebbdc..955fd65563 100644 --- a/src/arith_uint256.h +++ b/src/arith_uint256.h @@ -43,8 +43,10 @@ class base_uint base_uint& operator=(const base_uint& b) { - for (int i = 0; i < WIDTH; i++) - pn[i] = b.pn[i]; + if (this != &b) { + for (int i = 0; i < WIDTH; i++) + pn[i] = b.pn[i]; + } return *this; } diff --git a/src/key.h b/src/key.h index c802e1ebb8..bbca2c4dee 100644 --- a/src/key.h +++ b/src/key.h @@ -75,13 +75,15 @@ class CKey CKey& operator=(const CKey& other) { - if (other.keydata) { - MakeKeyData(); - *keydata = *other.keydata; - } else { - ClearKeyData(); + if (this != &other) { + if (other.keydata) { + MakeKeyData(); + *keydata = *other.keydata; + } else { + ClearKeyData(); + } + fCompressed = other.fCompressed; } - fCompressed = other.fCompressed; return *this; } diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index a371753adf..1b66496505 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -1508,8 +1508,10 @@ struct Tracker Tracker(Tracker&& t) noexcept : origin(t.origin), copies(t.copies) {} Tracker& operator=(const Tracker& t) noexcept { - origin = t.origin; - copies = t.copies + 1; + if (this != &t) { + origin = t.origin; + copies = t.copies + 1; + } return *this; } }; From 26a7f70b5d2b1cbfbf03e0b57b9b5ea92dafbb19 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Wed, 5 Jun 2024 17:47:21 +0000 Subject: [PATCH 10/60] ci: enable self-assignment clang-tidy check --- src/.clang-tidy | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/.clang-tidy b/src/.clang-tidy index 61adce1d50..8e511480ef 100644 --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -6,6 +6,7 @@ bugprone-move-forwarding-reference, bugprone-string-constructor, bugprone-use-after-move, bugprone-lambda-function-name, +bugprone-unhandled-self-assignment, misc-unused-using-decls, misc-no-recursion, modernize-use-default-member-init, @@ -23,8 +24,10 @@ readability-const-return-type, readability-redundant-declaration, readability-redundant-string-init, ' +HeaderFilterRegex: '.' WarningsAsErrors: '*' CheckOptions: - key: performance-move-const-arg.CheckTriviallyCopyableMove value: false -HeaderFilterRegex: '.' + - key: bugprone-unhandled-self-assignment.WarnOnlyIfThisHasSuspiciousField + value: false From e075fd131d668d9d1ba3c8566624481c4a57032d Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Tue, 30 Jan 2024 19:50:12 +0530 Subject: [PATCH 11/60] test: Introduce test types and modify v2 handshake function accordingly Prior to this commit, TestEncryptedP2PState would always send initial_v2_handshake bytes in 2 parts (as required by early key response test). For generalising this test and having different v2 handshake behaviour based on the test type, special behaviours like sending initial_v2_handshake bytes in 2 parts are executed only if test_type is set to EARLY_KEY_RESPONSE. --- test/functional/p2p_v2_misbehaving.py | 91 ++++++++++++--------------- 1 file changed, 41 insertions(+), 50 deletions(-) diff --git a/test/functional/p2p_v2_misbehaving.py b/test/functional/p2p_v2_misbehaving.py index 32d2e1148a..a3a5abdb6d 100755 --- a/test/functional/p2p_v2_misbehaving.py +++ b/test/functional/p2p_v2_misbehaving.py @@ -3,87 +3,78 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. -import random +from enum import Enum -from test_framework.test_framework import BitcoinTestFramework -from test_framework.crypto.ellswift import ellswift_create +from test_framework.messages import MAGIC_BYTES from test_framework.p2p import P2PInterface +from test_framework.test_framework import BitcoinTestFramework from test_framework.v2_p2p import EncryptedP2PState -class TestEncryptedP2PState(EncryptedP2PState): - """ Modify v2 P2P protocol functions for testing that "The responder waits until one byte is received which does - not match the 16 bytes consisting of the network magic followed by "version\x00\x00\x00\x00\x00"." (see BIP 324) +class TestType(Enum): + """ Scenarios to be tested: - - if `send_net_magic` is True, send first 4 bytes of ellswift (match network magic) else send remaining 60 bytes - - `can_data_be_received` is a variable used to assert if data is received on recvbuf. - - v2 TestNode shouldn't respond back if we send V1_PREFIX and data shouldn't be received on recvbuf. - This state is represented using `can_data_be_received` = False. - - v2 TestNode responds back when mismatch from V1_PREFIX happens and data can be received on recvbuf. - This state is represented using `can_data_be_received` = True. + 1. EARLY_KEY_RESPONSE - The responder needs to wait until one byte is received which does not match the 16 bytes + consisting of network magic followed by "version\x00\x00\x00\x00\x00" before sending out its ellswift + garbage bytes """ + EARLY_KEY_RESPONSE = 0 - def __init__(self): - super().__init__(initiating=True, net='regtest') - self.send_net_magic = True - self.can_data_be_received = False - - def initiate_v2_handshake(self, garbage_len=random.randrange(4096)): - """Initiator begins the v2 handshake by sending its ellswift bytes and garbage. - Here, the 64 bytes ellswift is assumed to have it's 4 bytes match network magic bytes. It is sent in 2 phases: - 1. when `send_network_magic` = True, send first 4 bytes of ellswift (matches network magic bytes) - 2. when `send_network_magic` = False, send remaining 60 bytes of ellswift - """ - if self.send_net_magic: - self.privkey_ours, self.ellswift_ours = ellswift_create() - self.sent_garbage = random.randbytes(garbage_len) - self.send_net_magic = False - return b"\xfa\xbf\xb5\xda" - else: - self.can_data_be_received = True - return self.ellswift_ours[4:] + self.sent_garbage +class EarlyKeyResponseState(EncryptedP2PState): + """ Modify v2 P2P protocol functions for testing EARLY_KEY_RESPONSE scenario""" + def __init__(self, initiating, net): + super().__init__(initiating=initiating, net=net) + self.can_data_be_received = False # variable used to assert if data is received on recvbuf. + + def initiate_v2_handshake(self): + """Send ellswift and garbage bytes in 2 parts when TestType = (EARLY_KEY_RESPONSE)""" + self.generate_keypair_and_garbage() + return b"" -class PeerEarlyKey(P2PInterface): + +class MisbehavingV2Peer(P2PInterface): """Custom implementation of P2PInterface which uses modified v2 P2P protocol functions for testing purposes.""" - def __init__(self): + def __init__(self, test_type): super().__init__() - self.v2_state = None - self.connection_opened = False + self.test_type = test_type def connection_made(self, transport): - """64 bytes ellswift is sent in 2 parts during `initial_v2_handshake()`""" - self.v2_state = TestEncryptedP2PState() + if self.test_type == TestType.EARLY_KEY_RESPONSE: + self.v2_state = EarlyKeyResponseState(initiating=True, net='regtest') super().connection_made(transport) def data_received(self, t): - # check that data can be received on recvbuf only when mismatch from V1_PREFIX happens (send_net_magic = False) - assert self.v2_state.can_data_be_received and not self.v2_state.send_net_magic + if self.test_type == TestType.EARLY_KEY_RESPONSE: + # check that data can be received on recvbuf only when mismatch from V1_PREFIX happens + assert self.v2_state.can_data_be_received + else: + super().data_received(t) - def on_open(self): - self.connection_opened = True -class P2PEarlyKey(BitcoinTestFramework): +class EncryptedP2PMisbehaving(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 self.extra_args = [["-v2transport=1", "-peertimeout=3"]] def run_test(self): + self.test_earlykeyresponse() + + def test_earlykeyresponse(self): self.log.info('Sending ellswift bytes in parts to ensure that response from responder is received only when') self.log.info('ellswift bytes have a mismatch from the 16 bytes(network magic followed by "version\\x00\\x00\\x00\\x00\\x00")') node0 = self.nodes[0] 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, wait_for_v2_handshake=False) - self.wait_until(lambda: peer1.connection_opened) + peer1 = node0.add_p2p_connection(MisbehavingV2Peer(TestType.EARLY_KEY_RESPONSE), wait_for_verack=False, send_version=False, supports_v2_p2p=True, wait_for_v2_handshake=False) + peer1.send_raw_message(MAGIC_BYTES['regtest']) 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') - ellswift_and_garbage_data = peer1.v2_state.initiate_v2_handshake() - peer1.send_raw_message(ellswift_and_garbage_data) - peer1.wait_for_disconnect(timeout=5) - self.log.info('successful disconnection when MITM happens in the key exchange phase') + peer1.v2_state.can_data_be_received = True + peer1.send_raw_message(peer1.v2_state.ellswift_ours[4:] + peer1.v2_state.sent_garbage) + with node0.assert_debug_log(['V2 handshake timeout peer=0']): + peer1.wait_for_disconnect(timeout=5) + self.log.info('successful disconnection since modified ellswift was sent as response') if __name__ == '__main__': - P2PEarlyKey().main() + EncryptedP2PMisbehaving().main() From de669a883bf65c576cc596b20a576d7bb1618182 Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Mon, 17 Jun 2024 13:49:01 -0400 Subject: [PATCH 12/60] doc: replace mention of V3 with TRUC --- doc/policy/packages.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/policy/packages.md b/doc/policy/packages.md index a220bdd17f..e8295ed4d7 100644 --- a/doc/policy/packages.md +++ b/doc/policy/packages.md @@ -56,7 +56,7 @@ The following rules are enforced for all packages: - *Rationale*: Basic support for package RBF can be used by wallets by making chains of no longer than two, then directly conflicting - those chains when needed. Combined with V3 transactions this can + those chains when needed. Combined with TRUC transactions this can result in more robust fee bumping. More general package RBF may be enabled in the future. From ff4558d441f377a372647972d35b5476b94579c9 Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Mon, 17 Jun 2024 13:50:23 -0400 Subject: [PATCH 13/60] doc: reword package RBF documentation --- doc/policy/packages.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/policy/packages.md b/doc/policy/packages.md index e8295ed4d7..9b321799f1 100644 --- a/doc/policy/packages.md +++ b/doc/policy/packages.md @@ -38,11 +38,11 @@ The following rules are enforced for all packages: * Only limited package replacements are currently considered. (#28984) - - All direct conflicts must signal replacement (or have `-mempoolfullrbf=1` set). + - All direct conflicts must signal replacement (or the node must have `-mempoolfullrbf=1` set). - Packages are 1-parent-1-child, with no in-mempool ancestors of the package. - - All conflicting clusters(connected components of mempool transactions) must be clusters of up to size 2. + - All conflicting clusters (connected components of mempool transactions) must be clusters of up to size 2. - No more than MAX_REPLACEMENT_CANDIDATES transactions can be replaced, analogous to regular [replacement rule](./mempool-replacements.md) 5). From bbde6ffefea9587b07a9f8d4043b2dd23ef8c3c5 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Thu, 20 Jun 2024 17:43:37 +0200 Subject: [PATCH 14/60] add node interface method for getting maximum mempool size --- src/interfaces/node.h | 3 +++ src/node/interfaces.cpp | 1 + 2 files changed, 4 insertions(+) diff --git a/src/interfaces/node.h b/src/interfaces/node.h index 2bb895dd47..040f30470a 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -162,6 +162,9 @@ class Node //! Get mempool dynamic usage. virtual size_t getMempoolDynamicUsage() = 0; + //! Get mempool maximum memory usage. + virtual size_t getMempoolMaxUsage() = 0; + //! Get header tip height and time. virtual bool getHeaderTip(int& height, int64_t& block_time) = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 2b36f4ceae..b8880d46db 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -273,6 +273,7 @@ class NodeImpl : public Node int64_t getTotalBytesSent() override { return m_context->connman ? m_context->connman->GetTotalBytesSent() : 0; } size_t getMempoolSize() override { return m_context->mempool ? m_context->mempool->size() : 0; } size_t getMempoolDynamicUsage() override { return m_context->mempool ? m_context->mempool->DynamicMemoryUsage() : 0; } + size_t getMempoolMaxUsage() override { return m_context->mempool ? m_context->mempool->m_opts.max_size_bytes : 0; } bool getHeaderTip(int& height, int64_t& block_time) override { LOCK(::cs_main); From 4a028cf54c0502bc9ba0804bf1ae413b20a007cb Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Thu, 20 Jun 2024 18:01:58 +0200 Subject: [PATCH 15/60] gui: show maximum mempool size in information window --- src/qt/clientmodel.cpp | 2 +- src/qt/clientmodel.h | 2 +- src/qt/rpcconsole.cpp | 15 ++++++++------- src/qt/rpcconsole.h | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp index 2f3bad37e6..0b03e3071c 100644 --- a/src/qt/clientmodel.cpp +++ b/src/qt/clientmodel.cpp @@ -53,7 +53,7 @@ ClientModel::ClientModel(interfaces::Node& node, OptionsModel *_optionsModel, QO connect(timer, &QTimer::timeout, [this] { // no locking required at this point // the following calls will acquire the required lock - Q_EMIT mempoolSizeChanged(m_node.getMempoolSize(), m_node.getMempoolDynamicUsage()); + Q_EMIT mempoolSizeChanged(m_node.getMempoolSize(), m_node.getMempoolDynamicUsage(), m_node.getMempoolMaxUsage()); Q_EMIT bytesChanged(m_node.getTotalBytesRecv(), m_node.getTotalBytesSent()); }); connect(m_thread, &QThread::finished, timer, &QObject::deleteLater); diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h index 624056b5df..7727359f99 100644 --- a/src/qt/clientmodel.h +++ b/src/qt/clientmodel.h @@ -113,7 +113,7 @@ class ClientModel : public QObject Q_SIGNALS: void numConnectionsChanged(int count); void numBlocksChanged(int count, const QDateTime& blockDate, double nVerificationProgress, SyncType header, SynchronizationState sync_state); - void mempoolSizeChanged(long count, size_t mempoolSizeInBytes); + void mempoolSizeChanged(long count, size_t mempoolSizeInBytes, size_t mempoolMaxSizeInBytes); void networkActiveChanged(bool networkActive); void alertsChanged(const QString &warnings); void bytesChanged(quint64 totalBytesIn, quint64 totalBytesOut); diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index edf417a7cb..fb731e4e90 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1000,15 +1000,16 @@ void RPCConsole::setNumBlocks(int count, const QDateTime& blockDate, double nVer } } -void RPCConsole::setMempoolSize(long numberOfTxs, size_t dynUsage) +void RPCConsole::setMempoolSize(long numberOfTxs, size_t dynUsage, size_t maxUsage) { ui->mempoolNumberTxs->setText(QString::number(numberOfTxs)); - if (dynUsage < 1000000) { - ui->mempoolSize->setText(QObject::tr("%1 kB").arg(dynUsage / 1000.0, 0, 'f', 2)); - } else { - ui->mempoolSize->setText(QObject::tr("%1 MB").arg(dynUsage / 1000000.0, 0, 'f', 2)); - } + const auto cur_usage_str = dynUsage < 1000000 ? + QObject::tr("%1 kB").arg(dynUsage / 1000.0, 0, 'f', 2) : + QObject::tr("%1 MB").arg(dynUsage / 1000000.0, 0, 'f', 2); + const auto max_usage_str = QObject::tr("%1 MB").arg(maxUsage / 1000000.0, 0, 'f', 2); + + ui->mempoolSize->setText(cur_usage_str + " / " + max_usage_str); } void RPCConsole::on_lineEdit_returnPressed() @@ -1400,4 +1401,4 @@ void RPCConsole::updateWindowTitle() const QString chainType = QString::fromStdString(Params().GetChainTypeString()); const QString title = tr("Node window - [%1]").arg(chainType); this->setWindowTitle(title); -} \ No newline at end of file +} diff --git a/src/qt/rpcconsole.h b/src/qt/rpcconsole.h index d6a5035c33..4747e611d0 100644 --- a/src/qt/rpcconsole.h +++ b/src/qt/rpcconsole.h @@ -121,7 +121,7 @@ public Q_SLOTS: /** Set number of blocks and last block date shown in the UI */ void setNumBlocks(int count, const QDateTime& blockDate, double nVerificationProgress, SyncType synctype); /** Set size (number of transactions and memory usage) of the mempool in the UI */ - void setMempoolSize(long numberOfTxs, size_t dynUsage); + void setMempoolSize(long numberOfTxs, size_t dynUsage, size_t maxUsage); /** Go forward or back in history */ void browseHistory(int offset); /** Scroll console view to end */ From 96b4facc912927305b06a233cb8b36e7e5964c08 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Sun, 17 Mar 2024 15:38:04 +0100 Subject: [PATCH 16/60] refactor, blockstorage: Generalize GetFirstStoredBlock GetFirstStoredBlock is generalized to check for any data status with a status mask that needs to be passed as a parameter. To reflect this the function is also renamed to GetFirstBlock. Co-authored-by: stickies-v --- src/node/blockstorage.cpp | 8 ++++---- src/node/blockstorage.h | 31 ++++++++++++++++++++++++++---- src/rpc/blockchain.cpp | 25 ++++++++++++++++++++---- src/rpc/blockchain.h | 4 ++++ src/test/blockchain_tests.cpp | 34 +++++++++++++++++++++++++++++++++ src/test/blockmanager_tests.cpp | 5 +++-- 6 files changed, 93 insertions(+), 14 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 576c07a833..6dbc111eb6 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -595,12 +595,12 @@ bool BlockManager::IsBlockPruned(const CBlockIndex& block) return m_have_pruned && !(block.nStatus & BLOCK_HAVE_DATA) && (block.nTx > 0); } -const CBlockIndex* BlockManager::GetFirstStoredBlock(const CBlockIndex& upper_block, const CBlockIndex* lower_block) +const CBlockIndex* BlockManager::GetFirstBlock(const CBlockIndex& upper_block, uint32_t status_mask, const CBlockIndex* lower_block) const { AssertLockHeld(::cs_main); const CBlockIndex* last_block = &upper_block; - assert(last_block->nStatus & BLOCK_HAVE_DATA); // 'upper_block' must have data - while (last_block->pprev && (last_block->pprev->nStatus & BLOCK_HAVE_DATA)) { + assert((last_block->nStatus & status_mask) == status_mask); // 'upper_block' must satisfy the status mask + while (last_block->pprev && ((last_block->pprev->nStatus & status_mask) == status_mask)) { if (lower_block) { // Return if we reached the lower_block if (last_block == lower_block) return lower_block; @@ -617,7 +617,7 @@ const CBlockIndex* BlockManager::GetFirstStoredBlock(const CBlockIndex& upper_bl bool BlockManager::CheckBlockDataAvailability(const CBlockIndex& upper_block, const CBlockIndex& lower_block) { if (!(upper_block.nStatus & BLOCK_HAVE_DATA)) return false; - return GetFirstStoredBlock(upper_block, &lower_block) == &lower_block; + return GetFirstBlock(upper_block, BLOCK_HAVE_DATA, &lower_block) == &lower_block; } // If we're using -prune with -reindex, then delete block files that will be ignored by the diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index ce514cc645..d1e46fde2b 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -335,10 +335,33 @@ class BlockManager //! (part of the same chain). bool CheckBlockDataAvailability(const CBlockIndex& upper_block LIFETIMEBOUND, const CBlockIndex& lower_block LIFETIMEBOUND) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! Find the first stored ancestor of start_block immediately after the last - //! pruned ancestor. Return value will never be null. Caller is responsible - //! for ensuring that start_block has data is not pruned. - const CBlockIndex* GetFirstStoredBlock(const CBlockIndex& start_block LIFETIMEBOUND, const CBlockIndex* lower_block=nullptr) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + /** + * @brief Returns the earliest block with specified `status_mask` flags set after + * the latest block _not_ having those flags. + * + * This function starts from `upper_block`, which must have all + * `status_mask` flags set, and iterates backwards through its ancestors. It + * continues as long as each block has all `status_mask` flags set, until + * reaching the oldest ancestor or `lower_block`. + * + * @pre `upper_block` must have all `status_mask` flags set. + * @pre `lower_block` must be null or an ancestor of `upper_block` + * + * @param upper_block The starting block for the search, which must have all + * `status_mask` flags set. + * @param status_mask Bitmask specifying required status flags. + * @param lower_block The earliest possible block to return. If null, the + * search can extend to the genesis block. + * + * @return A non-null pointer to the earliest block between `upper_block` + * and `lower_block`, inclusive, such that every block between the + * returned block and `upper_block` has `status_mask` flags set. + */ + const CBlockIndex* GetFirstBlock( + const CBlockIndex& upper_block LIFETIMEBOUND, + uint32_t status_mask, + const CBlockIndex* lower_block = nullptr + ) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** True if any block files have ever been pruned. */ bool m_have_pruned = false; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index a1135c27d4..822498c6d0 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -782,6 +782,24 @@ static RPCHelpMan getblock() }; } +//! Return height of highest block that has been pruned, or std::nullopt if no blocks have been pruned +std::optional GetPruneHeight(const BlockManager& blockman, const CChain& chain) { + AssertLockHeld(::cs_main); + + const CBlockIndex* chain_tip{chain.Tip()}; + if (!(chain_tip->nStatus & BLOCK_HAVE_DATA)) return chain_tip->nHeight; + + // Get first block with data, after the last block without data. + // This is the start of the unpruned range of blocks. + const auto& first_unpruned{*Assert(blockman.GetFirstBlock(*chain_tip, /*status_mask=*/BLOCK_HAVE_DATA))}; + if (!first_unpruned.pprev) { + // No block before the first unpruned block means nothing is pruned. + return std::nullopt; + } + // Block before the first unpruned block is the last pruned block. + return Assert(first_unpruned.pprev)->nHeight; +} + static RPCHelpMan pruneblockchain() { return RPCHelpMan{"pruneblockchain", "", @@ -834,8 +852,7 @@ static RPCHelpMan pruneblockchain() } PruneBlockFilesManual(active_chainstate, height); - const CBlockIndex& block{*CHECK_NONFATAL(active_chain.Tip())}; - return block.nStatus & BLOCK_HAVE_DATA ? active_chainstate.m_blockman.GetFirstStoredBlock(block)->nHeight - 1 : block.nHeight; + return GetPruneHeight(chainman.m_blockman, active_chain).value_or(-1); }, }; } @@ -1288,8 +1305,8 @@ RPCHelpMan getblockchaininfo() obj.pushKV("size_on_disk", chainman.m_blockman.CalculateCurrentUsage()); obj.pushKV("pruned", chainman.m_blockman.IsPruneMode()); if (chainman.m_blockman.IsPruneMode()) { - bool has_tip_data = tip.nStatus & BLOCK_HAVE_DATA; - obj.pushKV("pruneheight", has_tip_data ? chainman.m_blockman.GetFirstStoredBlock(tip)->nHeight : tip.nHeight + 1); + const auto prune_height{GetPruneHeight(chainman.m_blockman, active_chainstate.m_chain)}; + obj.pushKV("pruneheight", prune_height ? prune_height.value() + 1 : 0); const bool automatic_pruning{chainman.m_blockman.GetPruneTarget() != BlockManager::PRUNE_TARGET_MANUAL}; obj.pushKV("automatic_pruning", automatic_pruning); diff --git a/src/rpc/blockchain.h b/src/rpc/blockchain.h index c2021c3608..f6a7fe236c 100644 --- a/src/rpc/blockchain.h +++ b/src/rpc/blockchain.h @@ -21,6 +21,7 @@ class CBlockIndex; class Chainstate; class UniValue; namespace node { +class BlockManager; struct NodeContext; } // namespace node @@ -57,4 +58,7 @@ UniValue CreateUTXOSnapshot( const fs::path& path, const fs::path& tmppath); +//! Return height of highest block that has been pruned, or std::nullopt if no blocks have been pruned +std::optional GetPruneHeight(const node::BlockManager& blockman, const CChain& chain) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + #endif // BITCOIN_RPC_BLOCKCHAIN_H diff --git a/src/test/blockchain_tests.cpp b/src/test/blockchain_tests.cpp index be515a9eac..2a63d09795 100644 --- a/src/test/blockchain_tests.cpp +++ b/src/test/blockchain_tests.cpp @@ -5,7 +5,9 @@ #include #include +#include #include +#include #include #include @@ -74,4 +76,36 @@ BOOST_AUTO_TEST_CASE(get_difficulty_for_very_high_target) TestDifficulty(0x12345678, 5913134931067755359633408.0); } +//! Prune chain from height down to genesis block and check that +//! GetPruneHeight returns the correct value +static void CheckGetPruneHeight(node::BlockManager& blockman, CChain& chain, int height) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) +{ + AssertLockHeld(::cs_main); + + // Emulate pruning all blocks from `height` down to the genesis block + // by unsetting the `BLOCK_HAVE_DATA` flag from `nStatus` + for (CBlockIndex* it{chain[height]}; it != nullptr && it->nHeight > 0; it = it->pprev) { + it->nStatus &= ~BLOCK_HAVE_DATA; + } + + const auto prune_height{GetPruneHeight(blockman, chain)}; + BOOST_REQUIRE(prune_height.has_value()); + BOOST_CHECK_EQUAL(*prune_height, height); +} + +BOOST_FIXTURE_TEST_CASE(get_prune_height, TestChain100Setup) +{ + LOCK(::cs_main); + auto& chain = m_node.chainman->ActiveChain(); + auto& blockman = m_node.chainman->m_blockman; + + // Fresh chain of 100 blocks without any pruned blocks, so std::nullopt should be returned + BOOST_CHECK(!GetPruneHeight(blockman, chain).has_value()); + + // Start pruning + CheckGetPruneHeight(blockman, chain, 1); + CheckGetPruneHeight(blockman, chain, 99); + CheckGetPruneHeight(blockman, chain, 100); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index d7ac0bf823..12f3257700 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -2,6 +2,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include #include #include @@ -113,7 +114,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup) }; // 1) Return genesis block when all blocks are available - BOOST_CHECK_EQUAL(blockman.GetFirstStoredBlock(tip), chainman->ActiveChain()[0]); + BOOST_CHECK_EQUAL(blockman.GetFirstBlock(tip, BLOCK_HAVE_DATA), chainman->ActiveChain()[0]); BOOST_CHECK(blockman.CheckBlockDataAvailability(tip, *chainman->ActiveChain()[0])); // 2) Check lower_block when all blocks are available @@ -127,7 +128,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup) func_prune_blocks(last_pruned_block); // 3) The last block not pruned is in-between upper-block and the genesis block - BOOST_CHECK_EQUAL(blockman.GetFirstStoredBlock(tip), first_available_block); + BOOST_CHECK_EQUAL(blockman.GetFirstBlock(tip, BLOCK_HAVE_DATA), first_available_block); BOOST_CHECK(blockman.CheckBlockDataAvailability(tip, *first_available_block)); BOOST_CHECK(!blockman.CheckBlockDataAvailability(tip, *last_pruned_block)); } From e351576862471fc77b1e798a16833439e23ff0b4 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Tue, 13 Feb 2024 20:47:45 +0530 Subject: [PATCH 17/60] test: Check that disconnection happens when >4095 garbage bytes is sent This test type is represented using EXCESS_GARBAGE. --- test/functional/p2p_v2_misbehaving.py | 33 +++++++++++++++++++++++- test/functional/test_framework/v2_p2p.py | 5 ++-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/test/functional/p2p_v2_misbehaving.py b/test/functional/p2p_v2_misbehaving.py index a3a5abdb6d..ae325fa182 100755 --- a/test/functional/p2p_v2_misbehaving.py +++ b/test/functional/p2p_v2_misbehaving.py @@ -3,12 +3,16 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. +import random from enum import Enum from test_framework.messages import MAGIC_BYTES from test_framework.p2p import P2PInterface from test_framework.test_framework import BitcoinTestFramework -from test_framework.v2_p2p import EncryptedP2PState +from test_framework.v2_p2p import ( + EncryptedP2PState, + MAX_GARBAGE_LEN, +) class TestType(Enum): @@ -16,8 +20,10 @@ class TestType(Enum): 1. EARLY_KEY_RESPONSE - The responder needs to wait until one byte is received which does not match the 16 bytes consisting of network magic followed by "version\x00\x00\x00\x00\x00" before sending out its ellswift + garbage bytes + 2. EXCESS_GARBAGE - Disconnection happens when > MAX_GARBAGE_LEN bytes garbage is sent """ EARLY_KEY_RESPONSE = 0 + EXCESS_GARBAGE = 1 class EarlyKeyResponseState(EncryptedP2PState): @@ -32,6 +38,13 @@ def initiate_v2_handshake(self): return b"" +class ExcessGarbageState(EncryptedP2PState): + """Generate > MAX_GARBAGE_LEN garbage bytes""" + def generate_keypair_and_garbage(self): + garbage_len = MAX_GARBAGE_LEN + random.randrange(1, MAX_GARBAGE_LEN + 1) + return super().generate_keypair_and_garbage(garbage_len) + + class MisbehavingV2Peer(P2PInterface): """Custom implementation of P2PInterface which uses modified v2 P2P protocol functions for testing purposes.""" def __init__(self, test_type): @@ -41,6 +54,8 @@ def __init__(self, test_type): def connection_made(self, transport): if self.test_type == TestType.EARLY_KEY_RESPONSE: self.v2_state = EarlyKeyResponseState(initiating=True, net='regtest') + elif self.test_type == TestType.EXCESS_GARBAGE: + self.v2_state = ExcessGarbageState(initiating=True, net='regtest') super().connection_made(transport) def data_received(self, t): @@ -58,6 +73,7 @@ def set_test_params(self): def run_test(self): self.test_earlykeyresponse() + self.test_v2disconnection() def test_earlykeyresponse(self): self.log.info('Sending ellswift bytes in parts to ensure that response from responder is received only when') @@ -75,6 +91,21 @@ def test_earlykeyresponse(self): peer1.wait_for_disconnect(timeout=5) self.log.info('successful disconnection since modified ellswift was sent as response') + def test_v2disconnection(self): + # test v2 disconnection scenarios + node0 = self.nodes[0] + expected_debug_message = [ + [], # EARLY_KEY_RESPONSE + ["V2 transport error: missing garbage terminator, peer=1"], # EXCESS_GARBAGE + ] + for test_type in TestType: + if test_type == TestType.EARLY_KEY_RESPONSE: + continue + with node0.assert_debug_log(expected_debug_message[test_type.value], timeout=5): + peer = node0.add_p2p_connection(MisbehavingV2Peer(test_type), wait_for_verack=False, send_version=False, supports_v2_p2p=True, expect_success=False) + peer.wait_for_disconnect() + self.log.info(f"Expected disconnection for {test_type.name}") + if __name__ == '__main__': EncryptedP2PMisbehaving().main() diff --git a/test/functional/test_framework/v2_p2p.py b/test/functional/test_framework/v2_p2p.py index 0d2f7f6ad2..87600c36de 100644 --- a/test/functional/test_framework/v2_p2p.py +++ b/test/functional/test_framework/v2_p2p.py @@ -109,10 +109,11 @@ def v2_ecdh(priv, ellswift_theirs, ellswift_ours, initiating): # Responding, place their public key encoding first. return TaggedHash("bip324_ellswift_xonly_ecdh", ellswift_theirs + ellswift_ours + ecdh_point_x32) - def generate_keypair_and_garbage(self): + def generate_keypair_and_garbage(self, garbage_len=None): """Generates ellswift keypair and 4095 bytes garbage at max""" self.privkey_ours, self.ellswift_ours = ellswift_create() - garbage_len = random.randrange(MAX_GARBAGE_LEN + 1) + if garbage_len is None: + garbage_len = random.randrange(MAX_GARBAGE_LEN + 1) self.sent_garbage = random.randbytes(garbage_len) return self.ellswift_ours + self.sent_garbage From ad1482d5a20e6b155184a43d0724d2dcd950ce52 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Tue, 13 Feb 2024 20:51:51 +0530 Subject: [PATCH 18/60] test: Check that disconnection happens when wrong garbage terminator is sent This test type is represented using WRONG_GARBAGE_TERMINATOR. since the wrong garbage terminator is sent to TestNode, TestNode will interpret all of the gabage bytes, wrong garbage terminator, decoy messages and version packet it receives as garbage bytes. If the length of all these is more than 4095 + 16, it will result in a missing garbage terminator error. otherwise, it will result in a V2 handshake timeout error. Send only MAX_GARBAGE_LEN//2 bytes of garbage data to TestNode so that the total length received by the TestNode is at max = (MAX_GARBAGE_LEN//2) + 16 + 10*120 + 20 = 3283 bytes (which is less than 4095 + 16 bytes) and we get a consistent V2 handshake timeout error message. If we do not limit the garbage length sent, we will intermittently get both missing garbage terminator error and V2 handshake timeout error based on the garbage length and decoy packets length which are chosen at random. --- test/functional/p2p_v2_misbehaving.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/functional/p2p_v2_misbehaving.py b/test/functional/p2p_v2_misbehaving.py index ae325fa182..d0e64f57ac 100755 --- a/test/functional/p2p_v2_misbehaving.py +++ b/test/functional/p2p_v2_misbehaving.py @@ -9,6 +9,7 @@ from test_framework.messages import MAGIC_BYTES from test_framework.p2p import P2PInterface from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import random_bitflip from test_framework.v2_p2p import ( EncryptedP2PState, MAX_GARBAGE_LEN, @@ -21,9 +22,11 @@ class TestType(Enum): 1. EARLY_KEY_RESPONSE - The responder needs to wait until one byte is received which does not match the 16 bytes consisting of network magic followed by "version\x00\x00\x00\x00\x00" before sending out its ellswift + garbage bytes 2. EXCESS_GARBAGE - Disconnection happens when > MAX_GARBAGE_LEN bytes garbage is sent + 3. WRONG_GARBAGE_TERMINATOR - Disconnection happens when incorrect garbage terminator is sent """ EARLY_KEY_RESPONSE = 0 EXCESS_GARBAGE = 1 + WRONG_GARBAGE_TERMINATOR = 2 class EarlyKeyResponseState(EncryptedP2PState): @@ -45,6 +48,19 @@ def generate_keypair_and_garbage(self): return super().generate_keypair_and_garbage(garbage_len) +class WrongGarbageTerminatorState(EncryptedP2PState): + """Add option for sending wrong garbage terminator""" + def generate_keypair_and_garbage(self): + garbage_len = random.randrange(MAX_GARBAGE_LEN//2) + return super().generate_keypair_and_garbage(garbage_len) + + def complete_handshake(self, response): + length, handshake_bytes = super().complete_handshake(response) + # first 16 bytes returned by complete_handshake() is the garbage terminator + wrong_garbage_terminator = random_bitflip(handshake_bytes[:16]) + return length, wrong_garbage_terminator + handshake_bytes[16:] + + class MisbehavingV2Peer(P2PInterface): """Custom implementation of P2PInterface which uses modified v2 P2P protocol functions for testing purposes.""" def __init__(self, test_type): @@ -56,6 +72,8 @@ def connection_made(self, transport): self.v2_state = EarlyKeyResponseState(initiating=True, net='regtest') elif self.test_type == TestType.EXCESS_GARBAGE: self.v2_state = ExcessGarbageState(initiating=True, net='regtest') + elif self.test_type == TestType.WRONG_GARBAGE_TERMINATOR: + self.v2_state = WrongGarbageTerminatorState(initiating=True, net='regtest') super().connection_made(transport) def data_received(self, t): @@ -97,6 +115,7 @@ def test_v2disconnection(self): expected_debug_message = [ [], # EARLY_KEY_RESPONSE ["V2 transport error: missing garbage terminator, peer=1"], # EXCESS_GARBAGE + ["V2 handshake timeout peer=2"], # WRONG_GARBAGE_TERMINATOR ] for test_type in TestType: if test_type == TestType.EARLY_KEY_RESPONSE: From b5e6238fdbba5c777a5adfa4477dac51a82f4448 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Tue, 13 Feb 2024 20:57:17 +0530 Subject: [PATCH 19/60] test: Check that disconnection happens when garbage sent/received are different This test type is represented using WRONG_GARBAGE. Here, garbage bytes sent to TestNode are assumed to be tampered with and do not correspond to the garbage bytes which P2PInterface calculated and uses. --- test/functional/p2p_v2_misbehaving.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/functional/p2p_v2_misbehaving.py b/test/functional/p2p_v2_misbehaving.py index d0e64f57ac..e0633f25e3 100755 --- a/test/functional/p2p_v2_misbehaving.py +++ b/test/functional/p2p_v2_misbehaving.py @@ -23,10 +23,12 @@ class TestType(Enum): consisting of network magic followed by "version\x00\x00\x00\x00\x00" before sending out its ellswift + garbage bytes 2. EXCESS_GARBAGE - Disconnection happens when > MAX_GARBAGE_LEN bytes garbage is sent 3. WRONG_GARBAGE_TERMINATOR - Disconnection happens when incorrect garbage terminator is sent + 4. WRONG_GARBAGE - Disconnection happens when garbage bytes that is sent is different from what the peer receives """ EARLY_KEY_RESPONSE = 0 EXCESS_GARBAGE = 1 WRONG_GARBAGE_TERMINATOR = 2 + WRONG_GARBAGE = 3 class EarlyKeyResponseState(EncryptedP2PState): @@ -61,6 +63,15 @@ def complete_handshake(self, response): return length, wrong_garbage_terminator + handshake_bytes[16:] +class WrongGarbageState(EncryptedP2PState): + """Generate tampered garbage bytes""" + def generate_keypair_and_garbage(self): + garbage_len = random.randrange(1, MAX_GARBAGE_LEN) + ellswift_garbage_bytes = super().generate_keypair_and_garbage(garbage_len) + # assume that garbage bytes sent to TestNode were tampered with + return ellswift_garbage_bytes[:64] + random_bitflip(ellswift_garbage_bytes[64:]) + + class MisbehavingV2Peer(P2PInterface): """Custom implementation of P2PInterface which uses modified v2 P2P protocol functions for testing purposes.""" def __init__(self, test_type): @@ -74,6 +85,8 @@ def connection_made(self, transport): self.v2_state = ExcessGarbageState(initiating=True, net='regtest') elif self.test_type == TestType.WRONG_GARBAGE_TERMINATOR: self.v2_state = WrongGarbageTerminatorState(initiating=True, net='regtest') + elif self.test_type == TestType.WRONG_GARBAGE: + self.v2_state = WrongGarbageState(initiating=True, net='regtest') super().connection_made(transport) def data_received(self, t): @@ -116,6 +129,7 @@ def test_v2disconnection(self): [], # EARLY_KEY_RESPONSE ["V2 transport error: missing garbage terminator, peer=1"], # EXCESS_GARBAGE ["V2 handshake timeout peer=2"], # WRONG_GARBAGE_TERMINATOR + ["V2 transport error: packet decryption failure"], # WRONG_GARBAGE ] for test_type in TestType: if test_type == TestType.EARLY_KEY_RESPONSE: From 997cc00b950a7d1b7f2a3971282685f4e81d87d2 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Wed, 14 Feb 2024 13:43:52 +0530 Subject: [PATCH 20/60] test: Check that disconnection happens when AAD isn't filled This test type is represented using SEND_NO_AAD. If AAD of the first encrypted packet sent after the garbage terminator (optional decoy packet/version packet) hasn't been filled, disconnection happens. --- test/functional/p2p_v2_misbehaving.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/functional/p2p_v2_misbehaving.py b/test/functional/p2p_v2_misbehaving.py index e0633f25e3..51bf580b9d 100755 --- a/test/functional/p2p_v2_misbehaving.py +++ b/test/functional/p2p_v2_misbehaving.py @@ -24,11 +24,13 @@ class TestType(Enum): 2. EXCESS_GARBAGE - Disconnection happens when > MAX_GARBAGE_LEN bytes garbage is sent 3. WRONG_GARBAGE_TERMINATOR - Disconnection happens when incorrect garbage terminator is sent 4. WRONG_GARBAGE - Disconnection happens when garbage bytes that is sent is different from what the peer receives + 5. SEND_NO_AAD - Disconnection happens when AAD of first encrypted packet after the garbage terminator is not filled """ EARLY_KEY_RESPONSE = 0 EXCESS_GARBAGE = 1 WRONG_GARBAGE_TERMINATOR = 2 WRONG_GARBAGE = 3 + SEND_NO_AAD = 4 class EarlyKeyResponseState(EncryptedP2PState): @@ -72,6 +74,17 @@ def generate_keypair_and_garbage(self): return ellswift_garbage_bytes[:64] + random_bitflip(ellswift_garbage_bytes[64:]) +class NoAADState(EncryptedP2PState): + """Add option for not filling first encrypted packet after garbage terminator with AAD""" + def generate_keypair_and_garbage(self): + garbage_len = random.randrange(1, MAX_GARBAGE_LEN) + return super().generate_keypair_and_garbage(garbage_len) + + def complete_handshake(self, response): + self.sent_garbage = b'' # do not authenticate the garbage which is sent + return super().complete_handshake(response) + + class MisbehavingV2Peer(P2PInterface): """Custom implementation of P2PInterface which uses modified v2 P2P protocol functions for testing purposes.""" def __init__(self, test_type): @@ -87,6 +100,8 @@ def connection_made(self, transport): self.v2_state = WrongGarbageTerminatorState(initiating=True, net='regtest') elif self.test_type == TestType.WRONG_GARBAGE: self.v2_state = WrongGarbageState(initiating=True, net='regtest') + elif self.test_type == TestType.SEND_NO_AAD: + self.v2_state = NoAADState(initiating=True, net='regtest') super().connection_made(transport) def data_received(self, t): @@ -130,6 +145,7 @@ def test_v2disconnection(self): ["V2 transport error: missing garbage terminator, peer=1"], # EXCESS_GARBAGE ["V2 handshake timeout peer=2"], # WRONG_GARBAGE_TERMINATOR ["V2 transport error: packet decryption failure"], # WRONG_GARBAGE + ["V2 transport error: packet decryption failure"], # SEND_NO_AAD ] for test_type in TestType: if test_type == TestType.EARLY_KEY_RESPONSE: From c9dacd958d7c1e98b08a7083c299d981e4c11193 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Tue, 13 Feb 2024 21:01:21 +0530 Subject: [PATCH 21/60] test: Check that non empty version packet is ignored and no disconnection happens This test type is represented using SEND_NON_EMPTY_VERSION_PACKET. --- test/functional/p2p_v2_misbehaving.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/test/functional/p2p_v2_misbehaving.py b/test/functional/p2p_v2_misbehaving.py index 51bf580b9d..e45a63b3b0 100755 --- a/test/functional/p2p_v2_misbehaving.py +++ b/test/functional/p2p_v2_misbehaving.py @@ -25,12 +25,14 @@ class TestType(Enum): 3. WRONG_GARBAGE_TERMINATOR - Disconnection happens when incorrect garbage terminator is sent 4. WRONG_GARBAGE - Disconnection happens when garbage bytes that is sent is different from what the peer receives 5. SEND_NO_AAD - Disconnection happens when AAD of first encrypted packet after the garbage terminator is not filled + 6. SEND_NON_EMPTY_VERSION_PACKET - non-empty version packet is simply ignored """ EARLY_KEY_RESPONSE = 0 EXCESS_GARBAGE = 1 WRONG_GARBAGE_TERMINATOR = 2 WRONG_GARBAGE = 3 SEND_NO_AAD = 4 + SEND_NON_EMPTY_VERSION_PACKET = 5 class EarlyKeyResponseState(EncryptedP2PState): @@ -85,6 +87,13 @@ def complete_handshake(self, response): return super().complete_handshake(response) +class NonEmptyVersionPacketState(EncryptedP2PState): + """"Add option for sending non-empty transport version packet.""" + def complete_handshake(self, response): + self.transport_version = random.randbytes(5) + return super().complete_handshake(response) + + class MisbehavingV2Peer(P2PInterface): """Custom implementation of P2PInterface which uses modified v2 P2P protocol functions for testing purposes.""" def __init__(self, test_type): @@ -102,6 +111,8 @@ def connection_made(self, transport): self.v2_state = WrongGarbageState(initiating=True, net='regtest') elif self.test_type == TestType.SEND_NO_AAD: self.v2_state = NoAADState(initiating=True, net='regtest') + elif TestType.SEND_NON_EMPTY_VERSION_PACKET: + self.v2_state = NonEmptyVersionPacketState(initiating=True, net='regtest') super().connection_made(transport) def data_received(self, t): @@ -146,14 +157,19 @@ def test_v2disconnection(self): ["V2 handshake timeout peer=2"], # WRONG_GARBAGE_TERMINATOR ["V2 transport error: packet decryption failure"], # WRONG_GARBAGE ["V2 transport error: packet decryption failure"], # SEND_NO_AAD + [], # SEND_NON_EMPTY_VERSION_PACKET ] for test_type in TestType: if test_type == TestType.EARLY_KEY_RESPONSE: continue - with node0.assert_debug_log(expected_debug_message[test_type.value], timeout=5): - peer = node0.add_p2p_connection(MisbehavingV2Peer(test_type), wait_for_verack=False, send_version=False, supports_v2_p2p=True, expect_success=False) - peer.wait_for_disconnect() - self.log.info(f"Expected disconnection for {test_type.name}") + elif test_type == TestType.SEND_NON_EMPTY_VERSION_PACKET: + node0.add_p2p_connection(MisbehavingV2Peer(test_type), wait_for_verack=True, send_version=True, supports_v2_p2p=True) + self.log.info(f"No disconnection for {test_type.name}") + else: + with node0.assert_debug_log(expected_debug_message[test_type.value], timeout=5): + peer = node0.add_p2p_connection(MisbehavingV2Peer(test_type), wait_for_verack=False, send_version=False, supports_v2_p2p=True, expect_success=False) + peer.wait_for_disconnect() + self.log.info(f"Expected disconnection for {test_type.name}") if __name__ == '__main__': From 4a1975008b602aeacdad9a74d1837a7455148074 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Sun, 17 Mar 2024 15:57:38 +0100 Subject: [PATCH 22/60] rpc: Make pruneheight also reflect undo data presence --- src/rpc/blockchain.cpp | 22 +++++++++++++++------- test/functional/feature_pruning.py | 18 ++++++++++++++++++ 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 822498c6d0..fd1d1e4e87 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -786,16 +786,24 @@ static RPCHelpMan getblock() std::optional GetPruneHeight(const BlockManager& blockman, const CChain& chain) { AssertLockHeld(::cs_main); + // Search for the last block missing block data or undo data. Don't let the + // search consider the genesis block, because the genesis block does not + // have undo data, but should not be considered pruned. + const CBlockIndex* first_block{chain[1]}; const CBlockIndex* chain_tip{chain.Tip()}; - if (!(chain_tip->nStatus & BLOCK_HAVE_DATA)) return chain_tip->nHeight; - // Get first block with data, after the last block without data. - // This is the start of the unpruned range of blocks. - const auto& first_unpruned{*Assert(blockman.GetFirstBlock(*chain_tip, /*status_mask=*/BLOCK_HAVE_DATA))}; - if (!first_unpruned.pprev) { - // No block before the first unpruned block means nothing is pruned. - return std::nullopt; + // If there are no blocks after the genesis block, or no blocks at all, nothing is pruned. + if (!first_block || !chain_tip) return std::nullopt; + + // If the chain tip is pruned, everything is pruned. + if (!((chain_tip->nStatus & BLOCK_HAVE_MASK) == BLOCK_HAVE_MASK)) return chain_tip->nHeight; + + const auto& first_unpruned{*Assert(blockman.GetFirstBlock(*chain_tip, /*status_mask=*/BLOCK_HAVE_MASK, first_block))}; + if (&first_unpruned == first_block) { + // All blocks between first_block and chain_tip have data, so nothing is pruned. + return std::nullopt; } + // Block before the first unpruned block is the last pruned block. return Assert(first_unpruned.pprev)->nHeight; } diff --git a/test/functional/feature_pruning.py b/test/functional/feature_pruning.py index 4b548ef0f3..5f99b8dee8 100755 --- a/test/functional/feature_pruning.py +++ b/test/functional/feature_pruning.py @@ -25,6 +25,7 @@ assert_equal, assert_greater_than, assert_raises_rpc_error, + try_rpc, ) # Rescans start at the earliest block up to 2 hours before a key timestamp, so @@ -479,8 +480,12 @@ def run_test(self): self.log.info("Test invalid pruning command line options") self.test_invalid_command_line_options() + self.log.info("Test scanblocks can not return pruned data") self.test_scanblocks_pruned() + self.log.info("Test pruneheight reflects the presence of block and undo data") + self.test_pruneheight_undo_presence() + self.log.info("Done") def test_scanblocks_pruned(self): @@ -494,5 +499,18 @@ def test_scanblocks_pruned(self): assert_raises_rpc_error(-1, "Block not available (pruned data)", node.scanblocks, "start", [{"desc": f"raw({false_positive_spk.hex()})"}], 0, 0, "basic", {"filter_false_positives": True}) + def test_pruneheight_undo_presence(self): + node = self.nodes[2] + pruneheight = node.getblockchaininfo()["pruneheight"] + fetch_block = node.getblockhash(pruneheight - 1) + + self.connect_nodes(1, 2) + peers = node.getpeerinfo() + node.getblockfrompeer(fetch_block, peers[0]["id"]) + self.wait_until(lambda: not try_rpc(-1, "Block not available (pruned data)", node.getblock, fetch_block), timeout=5) + + new_pruneheight = node.getblockchaininfo()["pruneheight"] + assert_equal(pruneheight, new_pruneheight) + if __name__ == '__main__': PruneTest().main() From 8789dc8f315a9d9ad7142d831bc9412f780248e7 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Sun, 17 Mar 2024 17:41:17 +0100 Subject: [PATCH 23/60] doc: Add note to getblockfrompeer on missing undo data --- src/rpc/blockchain.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index fd1d1e4e87..9a838ab77d 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -429,6 +429,7 @@ static RPCHelpMan getblockfrompeer() "getblockfrompeer", "Attempt to fetch block from a given peer.\n\n" "We must have the header for this block, e.g. using submitheader.\n" + "The block will not have any undo data which can limit the usage of the block data in a context where the undo data is needed.\n" "Subsequent calls for the same block may cause the response from the previous peer to be ignored.\n" "Peers generally ignore requests for a stale block that they never fully verified, or one that is more than a month old.\n" "When a peer does not respond with a block, we will disconnect.\n" From f170fe04ca03fe4021cbff7c5450ce3cc7fda17f Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 25 Jun 2024 16:57:14 +0100 Subject: [PATCH 24/60] depends: update doc in Qt pwd patch Now that upstream has gotten around to fixing this. We don't need any more of the patch, and it likely wont apply to our version of Qt in any case. See: https://github.com/qt/qtbase/commit/3388de698bfb9bbc456c08f03e83bf3e749df35c. --- depends/patches/qt/dont_hardcode_pwd.patch | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/depends/patches/qt/dont_hardcode_pwd.patch b/depends/patches/qt/dont_hardcode_pwd.patch index a74e9cb098..f6955b2f20 100644 --- a/depends/patches/qt/dont_hardcode_pwd.patch +++ b/depends/patches/qt/dont_hardcode_pwd.patch @@ -1,13 +1,13 @@ -commit 0e953866fc4672486e29e1ba6d83b4207e7b2f0b -Author: fanquake -Date: Tue Aug 18 15:09:06 2020 +0800 +Do not assume FHS in scripts - Don't hardcode pwd path +On systems that do not follow the Filesystem Hierarchy Standard, such as +guix, the hardcoded `/bin/pwd` will fail to be found so that the script +will fail. - Let a man use his builtins if he wants to! Also, removes the unnecessary - assumption that pwd lives under /bin/pwd. +Use `pwd`, instead, so that the command can be found through the normal +path search mechanism. - See #15581. +See https://github.com/qt/qtbase/commit/3388de698bfb9bbc456c08f03e83bf3e749df35c. diff --git a/qtbase/configure b/qtbase/configure index 08b49a8d..faea5b55 100755 From e9bfbb5414ab14ca14d8edcfdf77f28c9ed67c33 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 18 Jan 2024 17:31:27 +0100 Subject: [PATCH 25/60] ci: forks can opt-out of CI branch push (Cirrus only) --- .cirrus.yml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.cirrus.yml b/.cirrus.yml index b37fce7002..f5874744b5 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -27,6 +27,13 @@ env: # Global defaults # The above machine types are matched to each task by their label. Refer to the # Cirrus CI docs for more details. # +# When a contributor maintains a fork of the repo, any pull request they make +# to their own fork, or to the main repository, will trigger two CI runs: +# one for the branch push and one for the pull request. +# This can be avoided by setting SKIP_BRANCH_PUSH=true as a custom env variable +# in Cirrus repository settings, accessible from +# https://cirrus-ci.com/github/my-organization/my-repository +# # On machines that are persisted between CI jobs, RESTART_CI_DOCKER_BEFORE_RUN=1 # ensures that previous containers and artifacts are cleared before each run. # This requires installing Podman instead of Docker. @@ -59,7 +66,10 @@ env: # Global defaults # https://cirrus-ci.org/guide/tips-and-tricks/#sharing-configuration-between-tasks filter_template: &FILTER_TEMPLATE - skip: $CIRRUS_REPO_FULL_NAME == "bitcoin-core/gui" && $CIRRUS_PR == "" # No need to run on the read-only mirror, unless it is a PR. https://cirrus-ci.org/guide/writing-tasks/#conditional-task-execution + # Allow forks to specify SKIP_BRANCH_PUSH=true and skip CI runs when a branch is pushed, + # but still run CI when a PR is created. + # https://cirrus-ci.org/guide/writing-tasks/#conditional-task-execution + skip: $SKIP_BRANCH_PUSH == "true" && $CIRRUS_PR == "" stateful: false # https://cirrus-ci.org/guide/writing-tasks/#stateful-tasks base_template: &BASE_TEMPLATE From 576828e732bacb61b608cd89f669a2936d3e8d00 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 25 Jun 2024 14:42:20 +0200 Subject: [PATCH 26/60] ci: test-each-commit merge base optional The ci "test-each-commit" job fetches the PR branch being tested with a depth of (# of commits in PR + 2), and then tries to run tests on commits after the most recent merge commit. When a PR is opened against a bitcoin core branch, a merge commit is always guaranteed to exist within the fetch depth, because bitcoin core branches always point at merge commits. However, in fork repositories, pull requests can be opened based on other branches that don't contain recent merge commits, and this will currently cause git rev-list to fail with fatal: bad revision '^^@'. Work around this problem by not requiring a recent merge commit, and just testing on all fetched commits if a merge commit can't be found. Co-authored-by: Ryan Ofsky --- .github/workflows/ci.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 54795332e8..ab9704c0af 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -58,7 +58,13 @@ jobs: # and the ^ prefix is used to exclude these parents and all their # ancestors from the rev-list output as described in: # https://git-scm.com/docs/git-rev-list - echo "TEST_BASE=$(git rev-list -n$((${{ env.MAX_COUNT }} + 1)) --reverse HEAD ^$(git rev-list -n1 --merges HEAD)^@ | head -1)" >> "$GITHUB_ENV" + MERGE_BASE=$(git rev-list -n1 --merges HEAD) + EXCLUDE_MERGE_BASE_ANCESTORS= + # MERGE_BASE can be empty due to limited fetch-depth + if test -n "$MERGE_BASE"; then + EXCLUDE_MERGE_BASE_ANCESTORS=^${MERGE_BASE}^@ + fi + echo "TEST_BASE=$(git rev-list -n$((${{ env.MAX_COUNT }} + 1)) --reverse HEAD $EXCLUDE_MERGE_BASE_ANCESTORS | head -1)" >> "$GITHUB_ENV" - run: | sudo apt-get update sudo apt-get install clang ccache build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq libevent-dev libboost-dev libsqlite3-dev libdb++-dev systemtap-sdt-dev libminiupnpc-dev libnatpmp-dev qtbase5-dev qttools5-dev qttools5-dev-tools qtwayland5 libqrencode-dev -y From fa7462c67ab9b6d45484ce92b44d03f812627d6e Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 10 Jun 2024 18:21:21 +0200 Subject: [PATCH 27/60] build: Bump clang minimum supported version to 16 --- ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh | 8 ++++---- doc/dependencies.md | 2 +- doc/release-notes-29091-29165.md | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh b/ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh index 6425120afb..49660aac0c 100755 --- a/ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh +++ b/ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh @@ -7,9 +7,9 @@ export LC_ALL=C.UTF-8 export CONTAINER_NAME=ci_native_nowallet_libbitcoinkernel -export CI_IMAGE_NAME_TAG="docker.io/ubuntu:22.04" -# Use minimum supported python3.9 (or best-effort 3.10) and clang-15, see doc/dependencies.md -export PACKAGES="python3-zmq clang-15 llvm-15 libc++abi-15-dev libc++-15-dev" -export DEP_OPTS="NO_WALLET=1 CC=clang-15 CXX='clang++-15 -stdlib=libc++'" +export CI_IMAGE_NAME_TAG="docker.io/debian:bullseye" +# Use minimum supported python3.9 and clang-16, see doc/dependencies.md +export PACKAGES="python3-zmq clang-16 llvm-16 libc++abi-16-dev libc++-16-dev" +export DEP_OPTS="NO_WALLET=1 CC=clang-16 CXX='clang++-16 -stdlib=libc++'" export GOAL="install" export BITCOIN_CONFIG="--enable-reduce-exports --enable-experimental-util-chainstate --with-experimental-kernel-lib --enable-shared" diff --git a/doc/dependencies.md b/doc/dependencies.md index 63c505c9cb..3bc931e8f6 100644 --- a/doc/dependencies.md +++ b/doc/dependencies.md @@ -8,7 +8,7 @@ You can find installation instructions in the `build-*.md` file for your platfor | --- | --- | | [Autoconf](https://www.gnu.org/software/autoconf/) | [2.69](https://github.com/bitcoin/bitcoin/pull/17769) | | [Automake](https://www.gnu.org/software/automake/) | [1.13](https://github.com/bitcoin/bitcoin/pull/18290) | -| [Clang](https://clang.llvm.org) | [15.0](https://github.com/bitcoin/bitcoin/pull/29165) | +| [Clang](https://clang.llvm.org) | [16.0](https://github.com/bitcoin/bitcoin/pull/30263) | | [GCC](https://gcc.gnu.org) | [11.1](https://github.com/bitcoin/bitcoin/pull/29091) | | [Python](https://www.python.org) (scripts, tests) | [3.9](https://github.com/bitcoin/bitcoin/pull/28211) | | [systemtap](https://sourceware.org/systemtap/) ([tracing](tracing.md))| N/A | diff --git a/doc/release-notes-29091-29165.md b/doc/release-notes-29091-29165.md index 9c9f8e4e50..e13d29adc6 100644 --- a/doc/release-notes-29091-29165.md +++ b/doc/release-notes-29091-29165.md @@ -1,5 +1,5 @@ Build ----- -GCC 11.1 or later, or Clang 15+ or later, +GCC 11.1 or later, or Clang 16.0 or later, are now required to compile Bitcoin Core. From 9999dbc1bd171931f02266d7c1a5cfd39f49238e Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 12 Jun 2024 13:07:14 +0200 Subject: [PATCH 28/60] fuzz: Clarify Apple-Clang-16 workaround --- src/test/fuzz/fuzz.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp index c1c9945a04..502f5c2b11 100644 --- a/src/test/fuzz/fuzz.cpp +++ b/src/test/fuzz/fuzz.cpp @@ -72,8 +72,8 @@ auto& FuzzTargets() void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target, FuzzTargetOptions opts) { - const auto it_ins{FuzzTargets().try_emplace(name, FuzzTarget /* temporary can be dropped after clang-16 */ {std::move(target), std::move(opts)})}; - Assert(it_ins.second); + const auto [it, ins]{FuzzTargets().try_emplace(name, FuzzTarget /* temporary can be dropped after Apple-Clang-16 ? */ {std::move(target), std::move(opts)})}; + Assert(ins); } static std::string_view g_fuzz_target; From fa8f53273c7e5965620d31a8c3fe5f223cb76888 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 26 Jun 2024 18:51:42 +0200 Subject: [PATCH 29/60] refactor: Remove no longer needed clang-15 workaround for std::span --- src/httpserver.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/httpserver.h b/src/httpserver.h index 991081bab8..907ed1a72b 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -131,7 +131,7 @@ class HTTPRequest */ void WriteReply(int nStatus, std::string_view reply = "") { - WriteReply(nStatus, std::as_bytes(std::span{reply.data(), reply.size()})); + WriteReply(nStatus, std::as_bytes(std::span{reply})); } void WriteReply(int nStatus, std::span reply); }; From 517e204bacd9dcea6612aae57d5a2813b89cd01d Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 5 Jan 2024 18:36:19 -0500 Subject: [PATCH 30/60] Change MigrateLegacyToDescriptor to reopen wallet as BERKELEY_RO When we reopen the wallet to do the migration, instead of opening using BDB, open it using the BerkeleyRO implementation. --- src/wallet/wallet.cpp | 33 +++++++++++++++++++++++++---- test/functional/wallet_migration.py | 6 +++++- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8a79cf730b..ea7928ab4f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2923,7 +2923,7 @@ bool CWallet::EraseAddressReceiveRequest(WalletBatch& batch, const CTxDestinatio return true; } -std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error_string) +static util::Result GetWalletPath(const std::string& name) { // Do some checking on wallet path. It should be either a: // @@ -2936,15 +2936,24 @@ std::unique_ptr MakeWalletDatabase(const std::string& name, cons if (!(path_type == fs::file_type::not_found || path_type == fs::file_type::directory || (path_type == fs::file_type::symlink && fs::is_directory(wallet_path)) || (path_type == fs::file_type::regular && fs::PathFromString(name).filename() == fs::PathFromString(name)))) { - error_string = Untranslated(strprintf( + return util::Error{Untranslated(strprintf( "Invalid -wallet path '%s'. -wallet path should point to a directory where wallet.dat and " "database/log.?????????? files can be stored, a location where such a directory could be created, " "or (for backwards compatibility) the name of an existing data file in -walletdir (%s)", - name, fs::quoted(fs::PathToString(GetWalletDir())))); + name, fs::quoted(fs::PathToString(GetWalletDir()))))}; + } + return wallet_path; +} + +std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error_string) +{ + const auto& wallet_path = GetWalletPath(name); + if (!wallet_path) { + error_string = util::ErrorString(wallet_path); status = DatabaseStatus::FAILED_BAD_PATH; return nullptr; } - return MakeDatabase(wallet_path, options, status, error_string); + return MakeDatabase(*wallet_path, options, status, error_string); } std::shared_ptr CWallet::Create(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings) @@ -4346,11 +4355,24 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle // If the wallet is still loaded, unload it so that nothing else tries to use it while we're changing it bool was_loaded = false; if (auto wallet = GetWallet(context, wallet_name)) { + if (wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { + return util::Error{_("Error: This wallet is already a descriptor wallet")}; + } + if (!RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings)) { return util::Error{_("Unable to unload the wallet before migrating")}; } UnloadWallet(std::move(wallet)); was_loaded = true; + } else { + // Check if the wallet is BDB + const auto& wallet_path = GetWalletPath(wallet_name); + if (!wallet_path) { + return util::Error{util::ErrorString(wallet_path)}; + } + if (!IsBDBFile(BDBDataFile(*wallet_path))) { + return util::Error{_("Error: This wallet is already a descriptor wallet")}; + } } // Load the wallet but only in the context of this function. @@ -4359,6 +4381,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle empty_context.args = context.args; DatabaseOptions options; options.require_existing = true; + options.require_format = DatabaseFormat::BERKELEY_RO; DatabaseStatus status; std::unique_ptr database = MakeWalletDatabase(wallet_name, options, status, error); if (!database) { @@ -4373,6 +4396,8 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle // Helper to reload as normal for some of our exit scenarios const auto& reload_wallet = [&](std::shared_ptr& to_reload) { + // Reset options.require_format as wallets of any format may be reloaded. + options.require_format = std::nullopt; assert(to_reload.use_count() == 1); std::string name = to_reload->GetName(); to_reload.reset(); diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 890b6a5c1b..aa0aefa7b5 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -205,9 +205,13 @@ def test_basic(self): self.assert_list_txs_equal(basic2.listtransactions(), basic2_txs) # Now test migration on a descriptor wallet - self.log.info("Test \"nothing to migrate\" when the user tries to migrate a wallet with no legacy data") + self.log.info("Test \"nothing to migrate\" when the user tries to migrate a loaded wallet with no legacy data") assert_raises_rpc_error(-4, "Error: This wallet is already a descriptor wallet", basic2.migratewallet) + self.log.info("Test \"nothing to migrate\" when the user tries to migrate an unloaded wallet with no legacy data") + basic2.unloadwallet() + assert_raises_rpc_error(-4, "Error: This wallet is already a descriptor wallet", self.nodes[0].migratewallet, "basic2") + def test_multisig(self): default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) From 46819f5df6de2a860c3ec87d55527b617a3b128f Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Thu, 2 May 2024 19:09:33 +1000 Subject: [PATCH 31/60] wallet: use LogTrace for walletdb log messages at trace level --- src/wallet/sqlite.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index 5e3a8179a2..f2110ea3f7 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -52,7 +52,7 @@ static int TraceSqlCallback(unsigned code, void* context, void* param1, void* pa // in the log file, only expand statements that query the database, not // statements that update the database. char* expanded{sqlite3_stmt_readonly(stmt) ? sqlite3_expanded_sql(stmt) : nullptr}; - LogPrintf("[%s] SQLite Statement: %s\n", db->Filename(), expanded ? expanded : sqlite3_sql(stmt)); + LogTrace(BCLog::WALLETDB, "[%s] SQLite Statement: %s\n", db->Filename(), expanded ? expanded : sqlite3_sql(stmt)); if (expanded) sqlite3_free(expanded); } return SQLITE_OK; From d35efe1efc0dbeae0667baade2a40be08511c13e Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 10 Jun 2024 18:20:15 -0400 Subject: [PATCH 32/60] p2p: Start downloading historical blocks from common ancestor Otherwise, if the background tip is not an ancestor of the snapshot, blocks in between that ancestor up to the height of the background tip will never be requested. Co-authored-by: Martin Zumsande Co-authored-by: Alfonso Roman Zubeldia <19962151+alfonsoromanz@users.noreply.github.com> --- src/net_processing.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6374cb52c1..babff0796f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -6293,10 +6293,13 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // before the background chainstate to prioritize getting to network tip. FindNextBlocksToDownload(*peer, get_inflight_budget(), vToDownload, staller); if (m_chainman.BackgroundSyncInProgress() && !IsLimitedPeer(*peer)) { + // If the background tip is not an ancestor of the snapshot block, + // we need to start requesting blocks from their last common ancestor. + const CBlockIndex *from_tip = LastCommonAncestor(m_chainman.GetBackgroundSyncTip(), m_chainman.GetSnapshotBaseBlock()); TryDownloadingHistoricalBlocks( *peer, get_inflight_budget(), - vToDownload, m_chainman.GetBackgroundSyncTip(), + vToDownload, from_tip, Assert(m_chainman.GetSnapshotBaseBlock())); } for (const CBlockIndex *pindex : vToDownload) { From fa360b047fc578fd855b8f7420d84dc967884f4a Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 1 Jul 2024 17:47:43 +0200 Subject: [PATCH 33/60] util: Use SteadyClock in RandAddSeedPerfmon --- src/randomenv.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/randomenv.cpp b/src/randomenv.cpp index 49033deef2..93d30a27fd 100644 --- a/src/randomenv.cpp +++ b/src/randomenv.cpp @@ -69,10 +69,10 @@ void RandAddSeedPerfmon(CSHA512& hasher) // This can take up to 2 seconds, so only do it every 10 minutes. // Initialize last_perfmon to 0 seconds, we don't skip the first call. - static std::atomic last_perfmon{0s}; + static std::atomic last_perfmon{SteadyClock::time_point{0s}}; auto last_time = last_perfmon.load(); - auto current_time = GetTime(); - if (current_time < last_time + std::chrono::minutes{10}) return; + auto current_time = SteadyClock::now(); + if (current_time < last_time + 10min) return; last_perfmon = current_time; std::vector vData(250000, 0); From 7461d0c006c92ede2f2595b79a5509eaf3509fb7 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 5 Jan 2024 18:36:21 -0500 Subject: [PATCH 34/60] wallet: Move LegacySPKM data storage and handling to LegacyDataSPKM In order to load the necessary data for migrating a legacy wallet without the full LegacyScriptPubKeyMan, move the data storage and loading components to LegacyDataSPKM. LegacyScriptPubKeyMan now subclasses that. --- src/wallet/scriptpubkeyman.cpp | 62 ++++++++++----- src/wallet/scriptpubkeyman.h | 141 +++++++++++++++++++-------------- src/wallet/wallet.h | 2 +- src/wallet/walletdb.cpp | 6 +- 4 files changed, 125 insertions(+), 86 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index b42275fe4b..f642dc1ab7 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -80,7 +80,7 @@ bool PermitsUncompressed(IsMineSigVersion sigversion) return sigversion == IsMineSigVersion::TOP || sigversion == IsMineSigVersion::P2SH; } -bool HaveKeys(const std::vector& pubkeys, const LegacyScriptPubKeyMan& keystore) +bool HaveKeys(const std::vector& pubkeys, const LegacyDataSPKM& keystore) { for (const valtype& pubkey : pubkeys) { CKeyID keyID = CPubKey(pubkey).GetID(); @@ -227,7 +227,7 @@ isminetype LegacyScriptPubKeyMan::IsMine(const CScript& script) const assert(false); } -bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key) +bool LegacyDataSPKM::CheckDecryptionKey(const CKeyingMaterial& master_key) { { LOCK(cs_KeyStore); @@ -581,7 +581,7 @@ int64_t LegacyScriptPubKeyMan::GetTimeFirstKey() const return nTimeFirstKey; } -std::unique_ptr LegacyScriptPubKeyMan::GetSolvingProvider(const CScript& script) const +std::unique_ptr LegacyDataSPKM::GetSolvingProvider(const CScript& script) const { return std::make_unique(*this); } @@ -717,7 +717,7 @@ void LegacyScriptPubKeyMan::UpdateTimeFirstKey(int64_t nCreateTime) NotifyFirstKeyTimeChanged(this, nTimeFirstKey); } -bool LegacyScriptPubKeyMan::LoadKey(const CKey& key, const CPubKey &pubkey) +bool LegacyDataSPKM::LoadKey(const CKey& key, const CPubKey &pubkey) { return AddKeyPubKeyInner(key, pubkey); } @@ -769,7 +769,7 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyWithDB(WalletBatch& batch, const CKey& s return true; } -bool LegacyScriptPubKeyMan::LoadCScript(const CScript& redeemScript) +bool LegacyDataSPKM::LoadCScript(const CScript& redeemScript) { /* A sanity check was added in pull #3843 to avoid adding redeemScripts * that never can be redeemed. However, old wallets may still contain @@ -784,18 +784,36 @@ bool LegacyScriptPubKeyMan::LoadCScript(const CScript& redeemScript) return FillableSigningProvider::AddCScript(redeemScript); } +void LegacyDataSPKM::LoadKeyMetadata(const CKeyID& keyID, const CKeyMetadata& meta) +{ + LOCK(cs_KeyStore); + mapKeyMetadata[keyID] = meta; +} + void LegacyScriptPubKeyMan::LoadKeyMetadata(const CKeyID& keyID, const CKeyMetadata& meta) { LOCK(cs_KeyStore); + LegacyDataSPKM::LoadKeyMetadata(keyID, meta); UpdateTimeFirstKey(meta.nCreateTime); - mapKeyMetadata[keyID] = meta; +} + +void LegacyDataSPKM::LoadScriptMetadata(const CScriptID& script_id, const CKeyMetadata& meta) +{ + LOCK(cs_KeyStore); + m_script_metadata[script_id] = meta; } void LegacyScriptPubKeyMan::LoadScriptMetadata(const CScriptID& script_id, const CKeyMetadata& meta) { LOCK(cs_KeyStore); + LegacyDataSPKM::LoadScriptMetadata(script_id, meta); UpdateTimeFirstKey(meta.nCreateTime); - m_script_metadata[script_id] = meta; +} + +bool LegacyDataSPKM::AddKeyPubKeyInner(const CKey& key, const CPubKey& pubkey) +{ + LOCK(cs_KeyStore); + return FillableSigningProvider::AddKeyPubKey(key, pubkey); } bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey) @@ -823,7 +841,7 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pu return true; } -bool LegacyScriptPubKeyMan::LoadCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret, bool checksum_valid) +bool LegacyDataSPKM::LoadCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret, bool checksum_valid) { // Set fDecryptionThoroughlyChecked to false when the checksum is invalid if (!checksum_valid) { @@ -833,7 +851,7 @@ bool LegacyScriptPubKeyMan::LoadCryptedKey(const CPubKey &vchPubKey, const std:: return AddCryptedKeyInner(vchPubKey, vchCryptedSecret); } -bool LegacyScriptPubKeyMan::AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret) +bool LegacyDataSPKM::AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret) { LOCK(cs_KeyStore); assert(mapKeys.empty()); @@ -861,13 +879,13 @@ bool LegacyScriptPubKeyMan::AddCryptedKey(const CPubKey &vchPubKey, } } -bool LegacyScriptPubKeyMan::HaveWatchOnly(const CScript &dest) const +bool LegacyDataSPKM::HaveWatchOnly(const CScript &dest) const { LOCK(cs_KeyStore); return setWatchOnly.count(dest) > 0; } -bool LegacyScriptPubKeyMan::HaveWatchOnly() const +bool LegacyDataSPKM::HaveWatchOnly() const { LOCK(cs_KeyStore); return (!setWatchOnly.empty()); @@ -901,12 +919,12 @@ bool LegacyScriptPubKeyMan::RemoveWatchOnly(const CScript &dest) return true; } -bool LegacyScriptPubKeyMan::LoadWatchOnly(const CScript &dest) +bool LegacyDataSPKM::LoadWatchOnly(const CScript &dest) { return AddWatchOnlyInMem(dest); } -bool LegacyScriptPubKeyMan::AddWatchOnlyInMem(const CScript &dest) +bool LegacyDataSPKM::AddWatchOnlyInMem(const CScript &dest) { LOCK(cs_KeyStore); setWatchOnly.insert(dest); @@ -950,7 +968,7 @@ bool LegacyScriptPubKeyMan::AddWatchOnly(const CScript& dest, int64_t nCreateTim return AddWatchOnly(dest); } -void LegacyScriptPubKeyMan::LoadHDChain(const CHDChain& chain) +void LegacyDataSPKM::LoadHDChain(const CHDChain& chain) { LOCK(cs_KeyStore); m_hd_chain = chain; @@ -971,14 +989,14 @@ void LegacyScriptPubKeyMan::AddHDChain(const CHDChain& chain) m_hd_chain = chain; } -void LegacyScriptPubKeyMan::AddInactiveHDChain(const CHDChain& chain) +void LegacyDataSPKM::AddInactiveHDChain(const CHDChain& chain) { LOCK(cs_KeyStore); assert(!chain.seed_id.IsNull()); m_inactive_hd_chains[chain.seed_id] = chain; } -bool LegacyScriptPubKeyMan::HaveKey(const CKeyID &address) const +bool LegacyDataSPKM::HaveKey(const CKeyID &address) const { LOCK(cs_KeyStore); if (!m_storage.HasEncryptionKeys()) { @@ -987,7 +1005,7 @@ bool LegacyScriptPubKeyMan::HaveKey(const CKeyID &address) const return mapCryptedKeys.count(address) > 0; } -bool LegacyScriptPubKeyMan::GetKey(const CKeyID &address, CKey& keyOut) const +bool LegacyDataSPKM::GetKey(const CKeyID &address, CKey& keyOut) const { LOCK(cs_KeyStore); if (!m_storage.HasEncryptionKeys()) { @@ -1006,7 +1024,7 @@ bool LegacyScriptPubKeyMan::GetKey(const CKeyID &address, CKey& keyOut) const return false; } -bool LegacyScriptPubKeyMan::GetKeyOrigin(const CKeyID& keyID, KeyOriginInfo& info) const +bool LegacyDataSPKM::GetKeyOrigin(const CKeyID& keyID, KeyOriginInfo& info) const { CKeyMetadata meta; { @@ -1026,7 +1044,7 @@ bool LegacyScriptPubKeyMan::GetKeyOrigin(const CKeyID& keyID, KeyOriginInfo& inf return true; } -bool LegacyScriptPubKeyMan::GetWatchPubKey(const CKeyID &address, CPubKey &pubkey_out) const +bool LegacyDataSPKM::GetWatchPubKey(const CKeyID &address, CPubKey &pubkey_out) const { LOCK(cs_KeyStore); WatchKeyMap::const_iterator it = mapWatchKeys.find(address); @@ -1037,7 +1055,7 @@ bool LegacyScriptPubKeyMan::GetWatchPubKey(const CKeyID &address, CPubKey &pubke return false; } -bool LegacyScriptPubKeyMan::GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const +bool LegacyDataSPKM::GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const { LOCK(cs_KeyStore); if (!m_storage.HasEncryptionKeys()) { @@ -1156,7 +1174,7 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata& throw std::runtime_error(std::string(__func__) + ": writing HD chain model failed"); } -void LegacyScriptPubKeyMan::LoadKeyPool(int64_t nIndex, const CKeyPool &keypool) +void LegacyDataSPKM::LoadKeyPool(int64_t nIndex, const CKeyPool &keypool) { LOCK(cs_KeyStore); if (keypool.m_pre_split) { @@ -1677,7 +1695,7 @@ std::set LegacyScriptPubKeyMan::GetKeys() const return set_address; } -std::unordered_set LegacyScriptPubKeyMan::GetScriptPubKeys() const +std::unordered_set LegacyDataSPKM::GetScriptPubKeys() const { LOCK(cs_KeyStore); std::unordered_set spks; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 2c1ab8d44a..470b2dbf77 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -276,31 +276,97 @@ static const std::unordered_set LEGACY_OUTPUT_TYPES { class DescriptorScriptPubKeyMan; -class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider +// Manages the data for a LegacyScriptPubKeyMan. +// This is the minimum necessary to load a legacy wallet so that it can be migrated. +class LegacyDataSPKM : public ScriptPubKeyMan, public FillableSigningProvider { -private: - //! keeps track of whether Unlock has run a thorough check before - bool fDecryptionThoroughlyChecked = true; - +protected: using WatchOnlySet = std::set; using WatchKeyMap = std::map; - - WalletBatch *encrypted_batch GUARDED_BY(cs_KeyStore) = nullptr; - using CryptedKeyMap = std::map>>; CryptedKeyMap mapCryptedKeys GUARDED_BY(cs_KeyStore); WatchOnlySet setWatchOnly GUARDED_BY(cs_KeyStore); WatchKeyMap mapWatchKeys GUARDED_BY(cs_KeyStore); + /* the HD chain data model (external chain counters) */ + CHDChain m_hd_chain; + std::unordered_map m_inactive_hd_chains; + + //! keeps track of whether Unlock has run a thorough check before + bool fDecryptionThoroughlyChecked = true; + + bool AddWatchOnlyInMem(const CScript &dest); + virtual bool AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey); + bool AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret); + +public: + using ScriptPubKeyMan::ScriptPubKeyMan; + + // Map from Key ID to key metadata. + std::map mapKeyMetadata GUARDED_BY(cs_KeyStore); + + // Map from Script ID to key metadata (for watch-only keys). + std::map m_script_metadata GUARDED_BY(cs_KeyStore); + + // ScriptPubKeyMan overrides + bool CheckDecryptionKey(const CKeyingMaterial& master_key) override; + std::unordered_set GetScriptPubKeys() const override; + std::unique_ptr GetSolvingProvider(const CScript& script) const override; + uint256 GetID() const override { return uint256::ONE; } + + // FillableSigningProvider overrides + bool HaveKey(const CKeyID &address) const override; + bool GetKey(const CKeyID &address, CKey& keyOut) const override; + bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const override; + bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override; + + std::set setInternalKeyPool GUARDED_BY(cs_KeyStore); + std::set setExternalKeyPool GUARDED_BY(cs_KeyStore); + std::set set_pre_split_keypool GUARDED_BY(cs_KeyStore); + int64_t m_max_keypool_index GUARDED_BY(cs_KeyStore) = 0; + std::map m_pool_key_to_index; + + //! Load metadata (used by LoadWallet) + virtual void LoadKeyMetadata(const CKeyID& keyID, const CKeyMetadata &metadata); + virtual void LoadScriptMetadata(const CScriptID& script_id, const CKeyMetadata &metadata); + + //! Adds a watch-only address to the store, without saving it to disk (used by LoadWallet) + bool LoadWatchOnly(const CScript &dest); + //! Returns whether the watch-only script is in the wallet + bool HaveWatchOnly(const CScript &dest) const; + //! Returns whether there are any watch-only things in the wallet + bool HaveWatchOnly() const; + //! Adds a key to the store, without saving it to disk (used by LoadWallet) + bool LoadKey(const CKey& key, const CPubKey &pubkey); + //! Adds an encrypted key to the store, without saving it to disk (used by LoadWallet) + bool LoadCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret, bool checksum_valid); + //! Adds a CScript to the store + bool LoadCScript(const CScript& redeemScript); + //! Load a HD chain model (used by LoadWallet) + void LoadHDChain(const CHDChain& chain); + void AddInactiveHDChain(const CHDChain& chain); + const CHDChain& GetHDChain() const { return m_hd_chain; } + //! Load a keypool entry + void LoadKeyPool(int64_t nIndex, const CKeyPool &keypool); + + //! Fetches a pubkey from mapWatchKeys if it exists there + bool GetWatchPubKey(const CKeyID &address, CPubKey &pubkey_out) const; +}; + +// Implements the full legacy wallet behavior +class LegacyScriptPubKeyMan : public LegacyDataSPKM +{ +private: + WalletBatch *encrypted_batch GUARDED_BY(cs_KeyStore) = nullptr; + // By default, do not scan any block until keys/scripts are generated/imported int64_t nTimeFirstKey GUARDED_BY(cs_KeyStore) = UNKNOWN_TIME; //! Number of pre-generated keys/scripts (part of the look-ahead process, used to detect payments) int64_t m_keypool_size GUARDED_BY(cs_KeyStore){DEFAULT_KEYPOOL_SIZE}; - bool AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey); - bool AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret); + bool AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey) override; /** * Private version of AddWatchOnly method which does not accept a @@ -313,7 +379,6 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv */ bool AddWatchOnly(const CScript& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore); bool AddWatchOnlyWithDB(WalletBatch &batch, const CScript& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore); - bool AddWatchOnlyInMem(const CScript &dest); //! Adds a watch-only address to the store, and saves it to disk. bool AddWatchOnlyWithDB(WalletBatch &batch, const CScript& dest, int64_t create_time) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore); @@ -328,18 +393,9 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv /** Add a KeyOriginInfo to the wallet */ bool AddKeyOriginWithDB(WalletBatch& batch, const CPubKey& pubkey, const KeyOriginInfo& info); - /* the HD chain data model (external chain counters) */ - CHDChain m_hd_chain; - std::unordered_map m_inactive_hd_chains; - /* HD derive new child key (on internal or external chain) */ void DeriveNewChildKey(WalletBatch& batch, CKeyMetadata& metadata, CKey& secret, CHDChain& hd_chain, bool internal = false) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore); - std::set setInternalKeyPool GUARDED_BY(cs_KeyStore); - std::set setExternalKeyPool GUARDED_BY(cs_KeyStore); - std::set set_pre_split_keypool GUARDED_BY(cs_KeyStore); - int64_t m_max_keypool_index GUARDED_BY(cs_KeyStore) = 0; - std::map m_pool_key_to_index; // Tracks keypool indexes to CKeyIDs of keys that have been taken out of the keypool but may be returned to it std::map m_index_to_reserved_key; @@ -376,12 +432,11 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv bool TopUpChain(WalletBatch& batch, CHDChain& chain, unsigned int size); public: - LegacyScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) : ScriptPubKeyMan(storage), m_keypool_size(keypool_size) {} + LegacyScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) : LegacyDataSPKM(storage), m_keypool_size(keypool_size) {} util::Result GetNewDestination(const OutputType type) override; isminetype IsMine(const CScript& script) const override; - bool CheckDecryptionKey(const CKeyingMaterial& master_key) override; bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override; util::Result GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) override; @@ -415,8 +470,6 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv bool CanGetAddresses(bool internal = false) const override; - std::unique_ptr GetSolvingProvider(const CScript& script) const override; - bool CanProvide(const CScript& script, SignatureData& sigdata) override; bool SignTransaction(CMutableTransaction& tx, const std::map& coins, int sighash, std::map& input_errors) const override; @@ -425,58 +478,27 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv uint256 GetID() const override; - // Map from Key ID to key metadata. - std::map mapKeyMetadata GUARDED_BY(cs_KeyStore); - - // Map from Script ID to key metadata (for watch-only keys). - std::map m_script_metadata GUARDED_BY(cs_KeyStore); - //! Adds a key to the store, and saves it to disk. bool AddKeyPubKey(const CKey& key, const CPubKey &pubkey) override; - //! Adds a key to the store, without saving it to disk (used by LoadWallet) - bool LoadKey(const CKey& key, const CPubKey &pubkey); //! Adds an encrypted key to the store, and saves it to disk. bool AddCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret); - //! Adds an encrypted key to the store, without saving it to disk (used by LoadWallet) - bool LoadCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret, bool checksum_valid); void UpdateTimeFirstKey(int64_t nCreateTime) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore); - //! Adds a CScript to the store - bool LoadCScript(const CScript& redeemScript); //! Load metadata (used by LoadWallet) - void LoadKeyMetadata(const CKeyID& keyID, const CKeyMetadata &metadata); - void LoadScriptMetadata(const CScriptID& script_id, const CKeyMetadata &metadata); + void LoadKeyMetadata(const CKeyID& keyID, const CKeyMetadata &metadata) override; + void LoadScriptMetadata(const CScriptID& script_id, const CKeyMetadata &metadata) override; //! Generate a new key CPubKey GenerateNewKey(WalletBatch& batch, CHDChain& hd_chain, bool internal = false) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore); /* Set the HD chain model (chain child index counters) and writes it to the database */ void AddHDChain(const CHDChain& chain); - //! Load a HD chain model (used by LoadWallet) - void LoadHDChain(const CHDChain& chain); - const CHDChain& GetHDChain() const { return m_hd_chain; } - void AddInactiveHDChain(const CHDChain& chain); - //! Adds a watch-only address to the store, without saving it to disk (used by LoadWallet) - bool LoadWatchOnly(const CScript &dest); - //! Returns whether the watch-only script is in the wallet - bool HaveWatchOnly(const CScript &dest) const; - //! Returns whether there are any watch-only things in the wallet - bool HaveWatchOnly() const; //! Remove a watch only script from the keystore bool RemoveWatchOnly(const CScript &dest); bool AddWatchOnly(const CScript& dest, int64_t nCreateTime) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore); - //! Fetches a pubkey from mapWatchKeys if it exists there - bool GetWatchPubKey(const CKeyID &address, CPubKey &pubkey_out) const; - /* SigningProvider overrides */ - bool HaveKey(const CKeyID &address) const override; - bool GetKey(const CKeyID &address, CKey& keyOut) const override; - bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const override; bool AddCScript(const CScript& redeemScript) override; - bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override; - //! Load a keypool entry - void LoadKeyPool(int64_t nIndex, const CKeyPool &keypool); bool NewKeyPool(); void MarkPreSplitKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore); @@ -525,7 +547,6 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv const std::map& GetAllReserveKeys() const { return m_pool_key_to_index; } std::set GetKeys() const override; - std::unordered_set GetScriptPubKeys() const override; /** * Retrieves scripts that were imported by bugs into the legacy spkm and are @@ -544,9 +565,9 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv class LegacySigningProvider : public SigningProvider { private: - const LegacyScriptPubKeyMan& m_spk_man; + const LegacyDataSPKM& m_spk_man; public: - explicit LegacySigningProvider(const LegacyScriptPubKeyMan& spk_man) : m_spk_man(spk_man) {} + explicit LegacySigningProvider(const LegacyDataSPKM& spk_man) : m_spk_man(spk_man) {} bool GetCScript(const CScriptID &scriptid, CScript& script) const override { return m_spk_man.GetCScript(scriptid, script); } bool HaveCScript(const CScriptID &scriptid) const override { return m_spk_man.HaveCScript(scriptid); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 6a998fa398..c4e3e9b14e 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -962,7 +962,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati LegacyScriptPubKeyMan* GetLegacyScriptPubKeyMan() const; LegacyScriptPubKeyMan* GetOrCreateLegacyScriptPubKeyMan(); - //! Make a LegacyScriptPubKeyMan and set it for all types, internal, and external. + //! Make a Legacy(Data)SPKM and set it for all types, internal, and external. void SetupLegacyScriptPubKeyMan(); bool WithEncryptionKey(std::function cb) const override; diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index f34fcfc3fd..29267c681b 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -356,7 +356,7 @@ bool LoadKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, std::stri } if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadKey(key, vchPubKey)) { - strErr = "Error reading wallet database: LegacyScriptPubKeyMan::LoadKey failed"; + strErr = "Error reading wallet database: LegacyDataSPKM::LoadKey failed"; return false; } } catch (const std::exception& e) { @@ -395,7 +395,7 @@ bool LoadCryptedKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, st if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadCryptedKey(vchPubKey, vchPrivKey, checksum_valid)) { - strErr = "Error reading wallet database: LegacyScriptPubKeyMan::LoadCryptedKey failed"; + strErr = "Error reading wallet database: LegacyDataSPKM::LoadCryptedKey failed"; return false; } } catch (const std::exception& e) { @@ -586,7 +586,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, value >> script; if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadCScript(script)) { - strErr = "Error reading wallet database: LegacyScriptPubKeyMan::LoadCScript failed"; + strErr = "Error reading wallet database: LegacyDataSPKM::LoadCScript failed"; return DBErrors::NONCRITICAL_ERROR; } return DBErrors::LOAD_OK; From b231f4d556876ae70305e8710e31d53525ded8ae Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 24 Jun 2024 12:15:00 -0400 Subject: [PATCH 35/60] wallet: Move LegacyScriptPubKeyMan::IsMine to LegacyDataSPKM IsMine is necessary for migration. It should be inlined with migration when the legacy wallet is removed. --- src/wallet/scriptpubkeyman.cpp | 4 ++-- src/wallet/scriptpubkeyman.h | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index f642dc1ab7..73f45f5fa7 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -98,7 +98,7 @@ bool HaveKeys(const std::vector& pubkeys, const LegacyDataSPKM& keystor //! scripts or simply treat any script that has been //! stored in the keystore as spendable // NOLINTNEXTLINE(misc-no-recursion) -IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion, bool recurse_scripthash=true) +IsMineResult IsMineInner(const LegacyDataSPKM& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion, bool recurse_scripthash=true) { IsMineResult ret = IsMineResult::NO; @@ -213,7 +213,7 @@ IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& s } // namespace -isminetype LegacyScriptPubKeyMan::IsMine(const CScript& script) const +isminetype LegacyDataSPKM::IsMine(const CScript& script) const { switch (IsMineInner(*this, script, IsMineSigVersion::TOP)) { case IsMineResult::INVALID: diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 470b2dbf77..6e29e4668f 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -314,6 +314,8 @@ class LegacyDataSPKM : public ScriptPubKeyMan, public FillableSigningProvider std::unordered_set GetScriptPubKeys() const override; std::unique_ptr GetSolvingProvider(const CScript& script) const override; uint256 GetID() const override { return uint256::ONE; } + // TODO: Remove IsMine when deleting LegacyScriptPubKeyMan + isminetype IsMine(const CScript& script) const override; // FillableSigningProvider overrides bool HaveKey(const CKeyID &address) const override; @@ -435,7 +437,6 @@ class LegacyScriptPubKeyMan : public LegacyDataSPKM LegacyScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) : LegacyDataSPKM(storage), m_keypool_size(keypool_size) {} util::Result GetNewDestination(const OutputType type) override; - isminetype IsMine(const CScript& script) const override; bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override; From 61d872f1b3d0dbfd0de4ff9b7deff40eeffa6a14 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 5 Jan 2024 18:36:22 -0500 Subject: [PATCH 36/60] wallet: Move MigrateToDescriptor and DeleteRecords to LegacyDataSPKM --- src/wallet/scriptpubkeyman.cpp | 12 ++++++------ src/wallet/scriptpubkeyman.h | 24 ++++++++++++------------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 73f45f5fa7..3e7548aad8 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1753,7 +1753,7 @@ std::unordered_set LegacyDataSPKM::GetScriptPubKeys() return spks; } -std::unordered_set LegacyScriptPubKeyMan::GetNotMineScriptPubKeys() const +std::unordered_set LegacyDataSPKM::GetNotMineScriptPubKeys() const { LOCK(cs_KeyStore); std::unordered_set spks; @@ -1763,7 +1763,7 @@ std::unordered_set LegacyScriptPubKeyMan::GetNotMineSc return spks; } -std::optional LegacyScriptPubKeyMan::MigrateToDescriptor() +std::optional LegacyDataSPKM::MigrateToDescriptor() { LOCK(cs_KeyStore); if (m_storage.IsLocked()) { @@ -1830,7 +1830,7 @@ std::optional LegacyScriptPubKeyMan::MigrateToDescriptor() WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0); // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys - auto desc_spk_man = std::unique_ptr(new DescriptorScriptPubKeyMan(m_storage, w_desc, m_keypool_size)); + auto desc_spk_man = std::make_unique(m_storage, w_desc, /*keypool_size=*/0); desc_spk_man->AddDescriptorKey(key, key.GetPubKey()); desc_spk_man->TopUp(); auto desc_spks = desc_spk_man->GetScriptPubKeys(); @@ -1875,7 +1875,7 @@ std::optional LegacyScriptPubKeyMan::MigrateToDescriptor() WalletDescriptor w_desc(std::move(desc), 0, 0, chain_counter, 0); // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys - auto desc_spk_man = std::unique_ptr(new DescriptorScriptPubKeyMan(m_storage, w_desc, m_keypool_size)); + auto desc_spk_man = std::make_unique(m_storage, w_desc, /*keypool_size=*/0); desc_spk_man->AddDescriptorKey(master_key.key, master_key.key.GetPubKey()); desc_spk_man->TopUp(); auto desc_spks = desc_spk_man->GetScriptPubKeys(); @@ -1937,7 +1937,7 @@ std::optional LegacyScriptPubKeyMan::MigrateToDescriptor() } else { // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0); - auto desc_spk_man = std::unique_ptr(new DescriptorScriptPubKeyMan(m_storage, w_desc, m_keypool_size)); + auto desc_spk_man = std::make_unique(m_storage, w_desc, /*keypool_size=*/0); for (const auto& keyid : privkeyids) { CKey key; if (!GetKey(keyid, key)) { @@ -2015,7 +2015,7 @@ std::optional LegacyScriptPubKeyMan::MigrateToDescriptor() return out; } -bool LegacyScriptPubKeyMan::DeleteRecords() +bool LegacyDataSPKM::DeleteRecords() { LOCK(cs_KeyStore); WalletBatch batch(m_storage.GetDatabase()); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 6e29e4668f..9c1f4b0a1a 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -354,6 +354,18 @@ class LegacyDataSPKM : public ScriptPubKeyMan, public FillableSigningProvider //! Fetches a pubkey from mapWatchKeys if it exists there bool GetWatchPubKey(const CKeyID &address, CPubKey &pubkey_out) const; + + /** + * Retrieves scripts that were imported by bugs into the legacy spkm and are + * simply invalid, such as a sh(sh(pkh())) script, or not watched. + */ + std::unordered_set GetNotMineScriptPubKeys() const; + + /** Get the DescriptorScriptPubKeyMans (with private keys) that have the same scriptPubKeys as this LegacyScriptPubKeyMan. + * Does not modify this ScriptPubKeyMan. */ + std::optional MigrateToDescriptor(); + /** Delete all the records ofthis LegacyScriptPubKeyMan from disk*/ + bool DeleteRecords(); }; // Implements the full legacy wallet behavior @@ -548,18 +560,6 @@ class LegacyScriptPubKeyMan : public LegacyDataSPKM const std::map& GetAllReserveKeys() const { return m_pool_key_to_index; } std::set GetKeys() const override; - - /** - * Retrieves scripts that were imported by bugs into the legacy spkm and are - * simply invalid, such as a sh(sh(pkh())) script, or not watched. - */ - std::unordered_set GetNotMineScriptPubKeys() const; - - /** Get the DescriptorScriptPubKeyMans (with private keys) that have the same scriptPubKeys as this LegacyScriptPubKeyMan. - * Does not modify this ScriptPubKeyMan. */ - std::optional MigrateToDescriptor(); - /** Delete all the records ofthis LegacyScriptPubKeyMan from disk*/ - bool DeleteRecords(); }; /** Wraps a LegacyScriptPubKeyMan so that it can be returned in a new unique_ptr. Does not provide privkeys */ From 771bc60f134c002a027e799639f0fed59ae8123d Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 5 Jan 2024 18:36:28 -0500 Subject: [PATCH 37/60] wallet: Use LegacyDataSPKM when loading In SetupLegacyScriptPubKeyMan, a base LegacyDataSPKM will be created if the database has the format "bdb_ro" (i.e. the wallet was opened only for migration purposes). All of the loading functions are now called with a LegacyDataSPKM object instead of LegacyScriptPubKeyMan. --- src/wallet/wallet.cpp | 25 ++++++++++++++++++++++--- src/wallet/wallet.h | 2 ++ src/wallet/walletdb.cpp | 20 ++++++++++---------- 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ea7928ab4f..01258bd45f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3611,6 +3611,16 @@ LegacyScriptPubKeyMan* CWallet::GetLegacyScriptPubKeyMan() const return dynamic_cast(it->second); } +LegacyDataSPKM* CWallet::GetLegacyDataSPKM() const +{ + if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { + return nullptr; + } + auto it = m_internal_spk_managers.find(OutputType::LEGACY); + if (it == m_internal_spk_managers.end()) return nullptr; + return dynamic_cast(it->second); +} + LegacyScriptPubKeyMan* CWallet::GetOrCreateLegacyScriptPubKeyMan() { SetupLegacyScriptPubKeyMan(); @@ -3627,13 +3637,22 @@ void CWallet::AddScriptPubKeyMan(const uint256& id, std::unique_ptrGetTimeFirstKey()); } +LegacyDataSPKM* CWallet::GetOrCreateLegacyDataSPKM() +{ + SetupLegacyScriptPubKeyMan(); + return GetLegacyDataSPKM(); +} + void CWallet::SetupLegacyScriptPubKeyMan() { if (!m_internal_spk_managers.empty() || !m_external_spk_managers.empty() || !m_spk_managers.empty() || IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { return; } - auto spk_manager = std::unique_ptr(new LegacyScriptPubKeyMan(*this, m_keypool_size)); + std::unique_ptr spk_manager = m_database->Format() == "bdb_ro" ? + std::make_unique(*this) : + std::make_unique(*this, m_keypool_size); + for (const auto& type : LEGACY_OUTPUT_TYPES) { m_internal_spk_managers[type] = spk_manager.get(); m_external_spk_managers[type] = spk_manager.get(); @@ -4001,7 +4020,7 @@ std::optional CWallet::GetDescriptorsForLegacy(bilingual_str& err { AssertLockHeld(cs_wallet); - LegacyScriptPubKeyMan* legacy_spkm = GetLegacyScriptPubKeyMan(); + LegacyDataSPKM* legacy_spkm = GetLegacyDataSPKM(); if (!Assume(legacy_spkm)) { // This shouldn't happen error = Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing")); @@ -4020,7 +4039,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) { AssertLockHeld(cs_wallet); - LegacyScriptPubKeyMan* legacy_spkm = GetLegacyScriptPubKeyMan(); + LegacyDataSPKM* legacy_spkm = GetLegacyDataSPKM(); if (!Assume(legacy_spkm)) { // This shouldn't happen error = Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing")); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c4e3e9b14e..e2fd8ad46b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -961,6 +961,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati //! Get the LegacyScriptPubKeyMan which is used for all types, internal, and external. LegacyScriptPubKeyMan* GetLegacyScriptPubKeyMan() const; LegacyScriptPubKeyMan* GetOrCreateLegacyScriptPubKeyMan(); + LegacyDataSPKM* GetLegacyDataSPKM() const; + LegacyDataSPKM* GetOrCreateLegacyDataSPKM(); //! Make a Legacy(Data)SPKM and set it for all types, internal, and external. void SetupLegacyScriptPubKeyMan(); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 29267c681b..61cc9dbc78 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -354,7 +354,7 @@ bool LoadKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, std::stri strErr = "Error reading wallet database: CPrivKey corrupt"; return false; } - if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadKey(key, vchPubKey)) + if (!pwallet->GetOrCreateLegacyDataSPKM()->LoadKey(key, vchPubKey)) { strErr = "Error reading wallet database: LegacyDataSPKM::LoadKey failed"; return false; @@ -393,7 +393,7 @@ bool LoadCryptedKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, st } } - if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadCryptedKey(vchPubKey, vchPrivKey, checksum_valid)) + if (!pwallet->GetOrCreateLegacyDataSPKM()->LoadCryptedKey(vchPubKey, vchPrivKey, checksum_valid)) { strErr = "Error reading wallet database: LegacyDataSPKM::LoadCryptedKey failed"; return false; @@ -440,7 +440,7 @@ bool LoadHDChain(CWallet* pwallet, DataStream& ssValue, std::string& strErr) try { CHDChain chain; ssValue >> chain; - pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadHDChain(chain); + pwallet->GetOrCreateLegacyDataSPKM()->LoadHDChain(chain); } catch (const std::exception& e) { if (strErr.empty()) { strErr = e.what(); @@ -584,7 +584,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, key >> hash; CScript script; value >> script; - if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadCScript(script)) + if (!pwallet->GetOrCreateLegacyDataSPKM()->LoadCScript(script)) { strErr = "Error reading wallet database: LegacyDataSPKM::LoadCScript failed"; return DBErrors::NONCRITICAL_ERROR; @@ -607,7 +607,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, key >> vchPubKey; CKeyMetadata keyMeta; value >> keyMeta; - pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadKeyMetadata(vchPubKey.GetID(), keyMeta); + pwallet->GetOrCreateLegacyDataSPKM()->LoadKeyMetadata(vchPubKey.GetID(), keyMeta); // Extract some CHDChain info from this metadata if it has any if (keyMeta.nVersion >= CKeyMetadata::VERSION_WITH_HDDATA && !keyMeta.hd_seed_id.IsNull() && keyMeta.hdKeypath.size() > 0) { @@ -674,7 +674,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, // Set inactive chains if (!hd_chains.empty()) { - LegacyScriptPubKeyMan* legacy_spkm = pwallet->GetLegacyScriptPubKeyMan(); + LegacyDataSPKM* legacy_spkm = pwallet->GetLegacyDataSPKM(); if (legacy_spkm) { for (const auto& [hd_seed_id, chain] : hd_chains) { if (hd_seed_id != legacy_spkm->GetHDChain().seed_id) { @@ -695,7 +695,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, uint8_t fYes; value >> fYes; if (fYes == '1') { - pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadWatchOnly(script); + pwallet->GetOrCreateLegacyDataSPKM()->LoadWatchOnly(script); } return DBErrors::LOAD_OK; }); @@ -708,7 +708,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, key >> script; CKeyMetadata keyMeta; value >> keyMeta; - pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadScriptMetadata(CScriptID(script), keyMeta); + pwallet->GetOrCreateLegacyDataSPKM()->LoadScriptMetadata(CScriptID(script), keyMeta); return DBErrors::LOAD_OK; }); result = std::max(result, watch_meta_res.m_result); @@ -720,7 +720,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, key >> nIndex; CKeyPool keypool; value >> keypool; - pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadKeyPool(nIndex, keypool); + pwallet->GetOrCreateLegacyDataSPKM()->LoadKeyPool(nIndex, keypool); return DBErrors::LOAD_OK; }); result = std::max(result, pool_res.m_result); @@ -763,7 +763,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, // nTimeFirstKey is only reliable if all keys have metadata if (pwallet->IsLegacy() && (key_res.m_records + ckey_res.m_records + watch_script_res.m_records) != (keymeta_res.m_records + watch_meta_res.m_records)) { - auto spk_man = pwallet->GetOrCreateLegacyScriptPubKeyMan(); + auto spk_man = pwallet->GetLegacyScriptPubKeyMan(); if (spk_man) { LOCK(spk_man->cs_KeyStore); spk_man->UpdateTimeFirstKey(1); From 8ce3739edbcf6437bf2695087e0ebe8c633df19b Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 26 Jun 2024 16:44:44 -0300 Subject: [PATCH 38/60] test: verify wallet is still active post-migration failure The migration process reloads the wallet after all failures. This commit tests the behavior by trying to obtain a new address after a decryption failure during migration. --- test/functional/wallet_migration.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index aa0aefa7b5..8fdc284d24 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -471,6 +471,12 @@ def test_encrypted(self): assert_raises_rpc_error(-4, "Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect", wallet.migratewallet, None, "badpass") assert_raises_rpc_error(-4, "The passphrase contains a null character", wallet.migratewallet, None, "pass\0with\0null") + # Check the wallet is still active post-migration failure. + # If not, it will throw an exception and abort the test. + wallet.walletpassphrase("pass", 99999) + wallet.getnewaddress() + + # Verify we can properly migrate the encrypted wallet self.migrate_wallet(wallet, passphrase="pass") info = wallet.getwalletinfo() From ab14d1d6a4a8ef5fe5013150e6c5ebcb5f5e4ea9 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Mon, 20 May 2024 22:32:32 +0200 Subject: [PATCH 39/60] validation: Don't error if maxsigcachesize exceeds uint32::max Instead clamp it to uint32::max if it exceeds it. Co-authored-by: Anthony Towns --- src/cuckoocache.h | 14 ++++++-------- src/init.cpp | 7 ++----- src/script/sigcache.cpp | 8 ++------ src/script/sigcache.h | 1 - src/validation.cpp | 5 +---- 5 files changed, 11 insertions(+), 24 deletions(-) diff --git a/src/cuckoocache.h b/src/cuckoocache.h index df320ed465..8370179395 100644 --- a/src/cuckoocache.h +++ b/src/cuckoocache.h @@ -14,7 +14,6 @@ #include #include #include -#include #include #include @@ -360,16 +359,15 @@ class cache * structure * @returns A pair of the maximum number of elements storable (see setup() * documentation for more detail) and the approximate total size of these - * elements in bytes or std::nullopt if the size requested is too large. + * elements in bytes. */ - std::optional> setup_bytes(size_t bytes) + std::pair setup_bytes(size_t bytes) { - size_t requested_num_elems = bytes / sizeof(Element); - if (std::numeric_limits::max() < requested_num_elems) { - return std::nullopt; - } + uint32_t requested_num_elems(std::min( + bytes / sizeof(Element), + std::numeric_limits::max())); - auto num_elems = setup(bytes/sizeof(Element)); + auto num_elems = setup(requested_num_elems); size_t approx_size_bytes = num_elems * sizeof(Element); return std::make_pair(num_elems, approx_size_bytes); diff --git a/src/init.cpp b/src/init.cpp index cb294ba84f..985b8d3d63 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1156,11 +1156,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) ValidationCacheSizes validation_cache_sizes{}; ApplyArgsManOptions(args, validation_cache_sizes); - if (!InitSignatureCache(validation_cache_sizes.signature_cache_bytes) - || !InitScriptExecutionCache(validation_cache_sizes.script_execution_cache_bytes)) - { - return InitError(strprintf(_("Unable to allocate memory for -maxsigcachesize: '%s' MiB"), args.GetIntArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_BYTES >> 20))); - } + (void)InitSignatureCache(validation_cache_sizes.signature_cache_bytes); + (void)InitScriptExecutionCache(validation_cache_sizes.script_execution_cache_bytes); assert(!node.scheduler); node.scheduler = std::make_unique(); diff --git a/src/script/sigcache.cpp b/src/script/sigcache.cpp index 7c6c282cc4..666601d174 100644 --- a/src/script/sigcache.cpp +++ b/src/script/sigcache.cpp @@ -15,7 +15,6 @@ #include #include -#include #include #include @@ -77,7 +76,7 @@ class CSignatureCache std::unique_lock lock(cs_sigcache); setValid.insert(entry); } - std::optional> setup_bytes(size_t n) + std::pair setup_bytes(size_t n) { return setValid.setup_bytes(n); } @@ -96,10 +95,7 @@ static CSignatureCache signatureCache; // signatureCache. bool InitSignatureCache(size_t max_size_bytes) { - auto setup_results = signatureCache.setup_bytes(max_size_bytes); - if (!setup_results) return false; - - const auto [num_elems, approx_size_bytes] = *setup_results; + const auto [num_elems, approx_size_bytes] = signatureCache.setup_bytes(max_size_bytes); LogPrintf("Using %zu MiB out of %zu MiB requested for signature cache, able to store %zu elements\n", approx_size_bytes >> 20, max_size_bytes >> 20, num_elems); return true; diff --git a/src/script/sigcache.h b/src/script/sigcache.h index d33d60d5bc..26b8b9cd2a 100644 --- a/src/script/sigcache.h +++ b/src/script/sigcache.h @@ -10,7 +10,6 @@ #include #include -#include #include // DoS prevention: limit cache size to 32MiB (over 1000000 entries on 64-bit diff --git a/src/validation.cpp b/src/validation.cpp index 3e9ba08bb1..a9d26daf99 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2100,10 +2100,7 @@ bool InitScriptExecutionCache(size_t max_size_bytes) g_scriptExecutionCacheHasher.Write(nonce.begin(), 32); g_scriptExecutionCacheHasher.Write(nonce.begin(), 32); - auto setup_results = g_scriptExecutionCache.setup_bytes(max_size_bytes); - if (!setup_results) return false; - - const auto [num_elems, approx_size_bytes] = *setup_results; + const auto [num_elems, approx_size_bytes] = g_scriptExecutionCache.setup_bytes(max_size_bytes); LogPrintf("Using %zu MiB out of %zu MiB requested for script execution cache, able to store %zu elements\n", approx_size_bytes >> 20, max_size_bytes >> 20, num_elems); return true; From 13a3661aba95b54b822c99ecbb695b14a22536d2 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 17 May 2024 23:33:25 +0200 Subject: [PATCH 40/60] kernel: De-globalize script execution cache Move its ownership to the ChainstateManager class. Next to simplifying usage of the kernel library by no longer requiring manual setup of the cache prior to using validation code, it also slims down the amount of memory allocated by BasicTestingSetup. --- src/bitcoin-chainstate.cpp | 1 - src/init.cpp | 1 - src/kernel/chainstatemanager_opts.h | 2 ++ src/kernel/validation_cache_sizes.h | 1 - src/node/chainstatemanager_args.cpp | 9 +++++++ src/node/validation_cache_args.cpp | 1 - src/script/sigcache.h | 1 + src/test/txvalidationcache_tests.cpp | 36 ++++++++++++++------------- src/test/util/setup_common.cpp | 1 - src/validation.cpp | 37 +++++++++++++++++----------- src/validation.h | 21 +++++++++++++--- 11 files changed, 70 insertions(+), 41 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index ecbdcd48bb..d11d793d21 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -68,7 +68,6 @@ int main(int argc, char* argv[]) // performing the check with the signature cache. kernel::ValidationCacheSizes validation_cache_sizes{}; Assert(InitSignatureCache(validation_cache_sizes.signature_cache_bytes)); - Assert(InitScriptExecutionCache(validation_cache_sizes.script_execution_cache_bytes)); ValidationSignals validation_signals{std::make_unique()}; diff --git a/src/init.cpp b/src/init.cpp index 985b8d3d63..5accc63f37 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1157,7 +1157,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) ValidationCacheSizes validation_cache_sizes{}; ApplyArgsManOptions(args, validation_cache_sizes); (void)InitSignatureCache(validation_cache_sizes.signature_cache_bytes); - (void)InitScriptExecutionCache(validation_cache_sizes.script_execution_cache_bytes); assert(!node.scheduler); node.scheduler = std::make_unique(); diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h index 076841c3c9..1b08eeeca7 100644 --- a/src/kernel/chainstatemanager_opts.h +++ b/src/kernel/chainstatemanager_opts.h @@ -9,6 +9,7 @@ #include #include +#include