From c578d661c01c51c6cdd94d7f6577b059519634b8 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 28 Apr 2022 15:51:48 -0400 Subject: [PATCH 1/4] Merge bitcoin/bitcoin#18554: wallet: ensure wallet files are not reused across chains 5f213213cb17429353ef7ec3e97b185af06d236f tests: add tests for cross-chain wallet use prevention (Seibart Nedor) 968765973b5bfde1ee4ad2fb5c19e24bce63ad0e wallet: ensure wallet files are not reused across chains (Seibart Nedor) Pull request description: This implements a proposal in #12805 and is a rebase of #14533. This seems to be a working approach, but I'm not sure why the `p2p_segwit.py` functional test needed a change, so I'll look into it more. ACKs for top commit: achow101: ACK 5f213213cb17429353ef7ec3e97b185af06d236f dongcarl: Code Review ACK 5f213213cb17429353ef7ec3e97b185af06d236f [deleted]: tACK https://github.com/bitcoin/bitcoin/pull/18554/commits/5f213213cb17429353ef7ec3e97b185af06d236f Tree-SHA512: 2c934300f113e772fc31c16ef5588526300bbc36e4dcef7d77bd0760c5c8f0ec77f766b1bed5503eb0157fa26dc900ed54d2ad1b41863c1f736ce5c1f3b67bec --- src/dummywallet.cpp | 1 + src/wallet/init.cpp | 1 + src/wallet/wallet.cpp | 14 +++++++ src/wallet/wallet.h | 1 + test/functional/test_runner.py | 1 + test/functional/wallet_crosschain.py | 60 ++++++++++++++++++++++++++++ 6 files changed, 78 insertions(+) create mode 100755 test/functional/wallet_crosschain.py diff --git a/src/dummywallet.cpp b/src/dummywallet.cpp index 2cadbb003b210..72f691322c3a1 100644 --- a/src/dummywallet.cpp +++ b/src/dummywallet.cpp @@ -74,6 +74,7 @@ void DummyWalletInit::AddWalletOptions(ArgsManager& argsman) const "-flushwallet", "-privdb", "-walletrejectlongchains", + "-walletcrosschain", "-unsafesqlitesync" }); } diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index 563bdf8f61de7..e229c5988d60c 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -120,6 +120,7 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const #endif argsman.AddArg("-walletrejectlongchains", strprintf("Wallet will not create transactions that violate mempool chain limits (default: %u)", DEFAULT_WALLET_REJECT_LONG_CHAINS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::WALLET_DEBUG_TEST); + argsman.AddArg("-walletcrosschain", strprintf("Allow reusing wallet files across chains (default: %u)", DEFAULT_WALLETCROSSCHAIN), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::WALLET_DEBUG_TEST); argsman.AddHiddenArgs({"-zapwallettxes"}); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b27db42923ade..680dff156e211 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3018,6 +3018,20 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf assert(!walletInstance->m_chain || walletInstance->m_chain == &chain); walletInstance->m_chain = &chain; + // Unless allowed, ensure wallet files are not reused across chains: + if (!gArgs.GetBoolArg("-walletcrosschain", DEFAULT_WALLETCROSSCHAIN)) { + WalletBatch batch(walletInstance->GetDatabase()); + CBlockLocator locator; + if (batch.ReadBestBlock(locator) && locator.vHave.size() > 0 && chain.getHeight()) { + // Wallet is assumed to be from another chain, if genesis block in the active + // chain differs from the genesis block known to the wallet. + if (chain.getBlockHash(0) != locator.vHave.back()) { + error = Untranslated("Wallet files should not be reused across chains. Restart bitcoind with -walletcrosschain to override."); + return false; + } + } + } + // Register wallet with validationinterface. It's done before rescan to avoid // missing block connections between end of rescan and validation subscribing. // Because of wallet lock being hold, block connection notifications are going to diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 78e3d4a5dbc5d..4c030e8078587 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -99,6 +99,7 @@ static const bool DEFAULT_WALLET_REJECT_LONG_CHAINS{true}; static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 6; static const bool DEFAULT_WALLETBROADCAST = true; static const bool DEFAULT_DISABLE_WALLET = false; +static const bool DEFAULT_WALLETCROSSCHAIN = false; //! -maxtxfee default static const CAmount DEFAULT_TRANSACTION_MAXFEE = COIN / 10; //! Discourage users to set fees higher than this amount (in satoshis) per kB diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index ff0da91247fc8..0c5e22b4002da 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -303,6 +303,7 @@ 'rpc_bind.py --ipv4', 'rpc_bind.py --ipv6', 'rpc_bind.py --nonloopback', + 'wallet_crosschain.py', 'mining_basic.py', 'rpc_named_arguments.py', 'feature_startupnotify.py', diff --git a/test/functional/wallet_crosschain.py b/test/functional/wallet_crosschain.py new file mode 100755 index 0000000000000..b6d0c87985fa0 --- /dev/null +++ b/test/functional/wallet_crosschain.py @@ -0,0 +1,60 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +import os + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_raises_rpc_error + +class WalletCrossChain(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 2 + self.setup_clean_chain = True + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + + def setup_network(self): + self.add_nodes(self.num_nodes) + + # Switch node 1 to testnet before starting it. + self.nodes[1].chain = 'testnet3' + self.nodes[1].extra_args = ['-maxconnections=0'] # disable testnet sync + with open(self.nodes[1].bitcoinconf, 'r', encoding='utf8') as conf: + conf_data = conf.read() + with open (self.nodes[1].bitcoinconf, 'w', encoding='utf8') as conf: + conf.write(conf_data.replace('regtest=', 'testnet=').replace('[regtest]', '[test]')) + + self.start_nodes() + + def run_test(self): + self.log.info("Creating wallets") + + node0_wallet = os.path.join(self.nodes[0].datadir, 'node0_wallet') + self.nodes[0].createwallet(node0_wallet) + self.nodes[0].unloadwallet(node0_wallet) + node1_wallet = os.path.join(self.nodes[1].datadir, 'node1_wallet') + self.nodes[1].createwallet(node1_wallet) + self.nodes[1].unloadwallet(node1_wallet) + + self.log.info("Loading wallets into nodes with a different genesis blocks") + + if self.options.descriptors: + assert_raises_rpc_error(-18, 'Wallet file verification failed.', self.nodes[0].loadwallet, node1_wallet) + assert_raises_rpc_error(-18, 'Wallet file verification failed.', self.nodes[1].loadwallet, node0_wallet) + else: + assert_raises_rpc_error(-4, 'Wallet files should not be reused across chains.', self.nodes[0].loadwallet, node1_wallet) + assert_raises_rpc_error(-4, 'Wallet files should not be reused across chains.', self.nodes[1].loadwallet, node0_wallet) + + if not self.options.descriptors: + self.log.info("Override cross-chain wallet load protection") + self.stop_nodes() + self.start_nodes([['-walletcrosschain']] * self.num_nodes) + self.nodes[0].loadwallet(node1_wallet) + self.nodes[1].loadwallet(node0_wallet) + + +if __name__ == '__main__': + WalletCrossChain().main() From 91567819b44c69b74db9f21de4e68169fec560ea Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 16 Mar 2022 16:56:18 +0100 Subject: [PATCH 2/4] Merge bitcoin/bitcoin#18815: bench: Add logging benchmark fafe06c379316f165e88b8de7300a716cef25d0a bench: Sort bench_bench_bitcoin_SOURCES (MarcoFalke) fa31dc9b714401b67480232ef552d1479f5e6902 bench: Add logging benchmark (MarcoFalke) Pull request description: Might make finding performance bottlenecks or regressions (https://github.com/bitcoin/bitcoin/pull/17218) easier. For example, fuzzing relies on disabled logging to be as fast as possible. ACKs for top commit: dergoegge: ACK fafe06c379316f165e88b8de7300a716cef25d0a Tree-SHA512: dd858f3234a4dfb00bd7dec4398eb076370a4b9746aa24eecee7da86f6882398a2d086e5ab0b7c9f7321abcb135e7ffc54cc78e60d18b90379c6dba6d613b3f7 --- src/Makefile.bench.include | 28 +++++++++++----------- src/bench/logging.cpp | 48 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 13 deletions(-) create mode 100644 src/bench/logging.cpp diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index 3aca46268b34b..87d4ed0ad5265 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -14,44 +14,46 @@ GENERATED_BENCH_FILES = $(RAW_BENCH_FILES:.raw=.raw.h) bench_bench_dash_SOURCES = \ $(RAW_BENCH_FILES) \ bench/addrman.cpp \ - bench/bench_bitcoin.cpp \ + bench/base58.cpp \ + bench/bech32.cpp \ bench/bench.cpp \ bench/bench.h \ bench/bip324_ecdh.cpp \ + bench/bench_bitcoin.cpp \ bench/block_assemble.cpp \ + bench/ccoins_caching.cpp \ + bench/chacha20.cpp \ + bench/chacha_poly_aead.cpp \ bench/bls.cpp \ bench/bls_dkg.cpp \ bench/checkblock.cpp \ bench/checkqueue.cpp \ - bench/data.h \ + bench/crypto_hash.cpp \ bench/data.cpp \ + bench/data.h \ bench/duplicate_inputs.cpp \ bench/ecdsa.cpp \ bench/ellswift.cpp \ bench/examples.cpp \ - bench/rollingbloom.cpp \ - bench/chacha20.cpp \ - bench/crypto_hash.cpp \ - bench/ccoins_caching.cpp \ bench/gcs_filter.cpp \ bench/hashpadding.cpp \ bench/load_external.cpp \ - bench/merkle_root.cpp \ + bench/lockedpool.cpp \ + bench/logging.cpp \ bench/mempool_eviction.cpp \ bench/mempool_stress.cpp \ - bench/nanobench.h \ + bench/merkle_root.cpp \ bench/nanobench.cpp \ + bench/nanobench.h \ bench/peer_eviction.cpp \ bench/pool.cpp \ + bench/poly1305.cpp \ + bench/prevector.cpp \ + bench/rollingbloom.cpp \ bench/rpc_blockchain.cpp \ bench/rpc_mempool.cpp \ bench/strencodings.cpp \ bench/util_time.cpp \ - bench/base58.cpp \ - bench/bech32.cpp \ - bench/lockedpool.cpp \ - bench/poly1305.cpp \ - bench/prevector.cpp \ bench/string_cast.cpp \ bench/verify_script.cpp diff --git a/src/bench/logging.cpp b/src/bench/logging.cpp new file mode 100644 index 0000000000000..d28777df9e20f --- /dev/null +++ b/src/bench/logging.cpp @@ -0,0 +1,48 @@ +// Copyright (c) 2020 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include + + +static void Logging(benchmark::Bench& bench, const std::vector& extra_args, const std::function& log) +{ + TestingSetup test_setup{ + CBaseChainParams::REGTEST, + extra_args, + }; + + bench.run([&] { log(); }); +} + +static void LoggingYoThreadNames(benchmark::Bench& bench) +{ + Logging(bench, {"-logthreadnames=1"}, [] { LogPrintf("%s\n", "test"); }); +} +static void LoggingNoThreadNames(benchmark::Bench& bench) +{ + Logging(bench, {"-logthreadnames=0"}, [] { LogPrintf("%s\n", "test"); }); +} +static void LoggingYoCategory(benchmark::Bench& bench) +{ + Logging(bench, {"-logthreadnames=0", "-debug=net"}, [] { LogPrint(BCLog::NET, "%s\n", "test"); }); +} +static void LoggingNoCategory(benchmark::Bench& bench) +{ + Logging(bench, {"-logthreadnames=0", "-debug=0"}, [] { LogPrint(BCLog::NET, "%s\n", "test"); }); +} +static void LoggingNoFile(benchmark::Bench& bench) +{ + Logging(bench, {"-nodebuglogfile", "-debug=1"}, [] { + LogPrintf("%s\n", "test"); + LogPrint(BCLog::NET, "%s\n", "test"); + }); +} + +BENCHMARK(LoggingYoThreadNames); +BENCHMARK(LoggingNoThreadNames); +BENCHMARK(LoggingYoCategory); +BENCHMARK(LoggingNoCategory); +BENCHMARK(LoggingNoFile); From fc941e186c197214972a7654fbeb7dd947862b6a Mon Sep 17 00:00:00 2001 From: MacroFake Date: Tue, 5 Jul 2022 18:55:52 +0200 Subject: [PATCH 3/4] Merge bitcoin/bitcoin#19393: test: Add more tests for orphan tx handling c0a5fceee9858afd24fe0bf655b7b30728e96e78 test: Add test for erase orphan tx conflicted by block (Hennadii Stepanov) fa45bb21193ae0c220cfc224d5e3ea0e7f3ec988 test: Add test for erase orphan tx included by block (Hennadii Stepanov) 5c049780c8b310428cf72fb304bf0c1071742785 test: Add test for erase orphan tx from peer (Hennadii Stepanov) Pull request description: This PR adds test coverage for the following cases: - erase orphan transactions when a peer is disconnected - erase an orphan transaction when it is included in a new tip block - erase an orphan transaction when it is conflicted with other transactions included in a new tip block Found useful while working on #19374. ACKs for top commit: aureleoules: tACK c0a5fceee9858afd24fe0bf655b7b30728e96e78 (`make check` and `test/functional/test_runner.py`). kouloumos: ACK c0a5fceee9858afd24fe0bf655b7b30728e96e78 with a nit per https://github.com/bitcoin/bitcoin/pull/19393#discussion_r899156623. pg156: Reviewed to https://github.com/bitcoin/bitcoin/pull/19393/commits/c0a5fceee9858afd24fe0bf655b7b30728e96e78. Concept ACK. Agree due to the lack of RPC calls to inspect orphan pool, using `assert_debug_log` to match strings in log is a reasonable way to test. Tree-SHA512: 98f8deeee2d1c588c7e28a82e513d4a18655084198369db33fe2710458251eeaffed030626940072d7576f57fcbf7d856d761990129e2ca9e372d2ccbd86d07d --- test/functional/p2p_invalid_tx.py | 68 ++++++++++++++++++++++++++++--- 1 file changed, 63 insertions(+), 5 deletions(-) diff --git a/test/functional/p2p_invalid_tx.py b/test/functional/p2p_invalid_tx.py index f7a6b644c7e05..eb132b4e73a8d 100755 --- a/test/functional/p2p_invalid_tx.py +++ b/test/functional/p2p_invalid_tx.py @@ -72,7 +72,6 @@ def run_test(self): block.solve() # Save the coinbase for later block2 = block - tip = block.sha256 node.p2ps[0].send_blocks_and_test([block1, block2], node, success=True) self.log.info("Mature the block.") @@ -116,24 +115,24 @@ def test_orphan_tx_handling(self, base_tx, resolve_via_block): SCRIPT_PUB_KEY_OP_TRUE = b'\x51\x75' * 15 + b'\x51' tx_withhold = CTransaction() tx_withhold.vin.append(CTxIn(outpoint=COutPoint(base_tx, 0))) - tx_withhold.vout.append(CTxOut(nValue=50 * COIN - 12000, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_withhold.vout= [CTxOut(nValue=25 * COIN - 12000, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)] * 2 tx_withhold.calc_sha256() # Our first orphan tx with some outputs to create further orphan txs tx_orphan_1 = CTransaction() tx_orphan_1.vin.append(CTxIn(outpoint=COutPoint(tx_withhold.sha256, 0))) - tx_orphan_1.vout = [CTxOut(nValue=10 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)] * 3 + tx_orphan_1.vout = [CTxOut(nValue=8 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)] * 3 tx_orphan_1.calc_sha256() # A valid transaction with low fee tx_orphan_2_no_fee = CTransaction() tx_orphan_2_no_fee.vin.append(CTxIn(outpoint=COutPoint(tx_orphan_1.sha256, 0))) - tx_orphan_2_no_fee.vout.append(CTxOut(nValue=10 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_orphan_2_no_fee.vout.append(CTxOut(nValue=8 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) # A valid transaction with sufficient fee tx_orphan_2_valid = CTransaction() tx_orphan_2_valid.vin.append(CTxIn(outpoint=COutPoint(tx_orphan_1.sha256, 1))) - tx_orphan_2_valid.vout.append(CTxOut(nValue=10 * COIN - 12000, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_orphan_2_valid.vout.append(CTxOut(nValue=8 * COIN - 12000, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) tx_orphan_2_valid.calc_sha256() # An invalid transaction with negative fee @@ -197,6 +196,7 @@ def test_orphan_tx_handling(self, base_tx, resolve_via_block): with node.assert_debug_log(['orphanage overflow, removed 1 tx']): node.p2ps[0].send_txs_and_test(orphan_tx_pool, node, success=False) + self.log.info('Test orphan with rejected parents') rejected_parent = CTransaction() rejected_parent.vin.append(CTxIn(outpoint=COutPoint(tx_orphan_2_invalid.sha256, 0))) rejected_parent.vout.append(CTxOut(nValue=11 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) @@ -205,6 +205,64 @@ def test_orphan_tx_handling(self, base_tx, resolve_via_block): #with node.assert_debug_log(['not keeping orphan with rejected parents {}'.format(rejected_parent.hash)]): node.p2ps[0].send_txs_and_test([rejected_parent], node, success=False) + self.log.info('Test that a peer disconnection causes erase its transactions from the orphan pool') + with node.assert_debug_log(['Erased 100 orphan tx from peer=25']): + self.reconnect_p2p(num_connections=1) + + self.log.info('Test that a transaction in the orphan pool is included in a new tip block causes erase this transaction from the orphan pool') + tx_withhold_until_block_A = CTransaction() + tx_withhold_until_block_A.vin.append(CTxIn(outpoint=COutPoint(tx_withhold.sha256, 1))) + tx_withhold_until_block_A.vout = [CTxOut(nValue=12 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)] * 2 + tx_withhold_until_block_A.calc_sha256() + + tx_orphan_include_by_block_A = CTransaction() + tx_orphan_include_by_block_A.vin.append(CTxIn(outpoint=COutPoint(tx_withhold_until_block_A.sha256, 0))) + tx_orphan_include_by_block_A.vout.append(CTxOut(nValue=12 * COIN - 12000, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_orphan_include_by_block_A.calc_sha256() + + self.log.info('Send the orphan ... ') + node.p2ps[0].send_txs_and_test([tx_orphan_include_by_block_A], node, success=False) + + tip = int(node.getbestblockhash(), 16) + height = node.getblockcount() + 1 + block_A = create_block(tip, create_coinbase(height)) + block_A.vtx.extend([tx_withhold, tx_withhold_until_block_A, tx_orphan_include_by_block_A]) + block_A.hashMerkleRoot = block_A.calc_merkle_root() + block_A.solve() + + self.log.info('Send the block that includes the previous orphan ... ') + with node.assert_debug_log(["Erased 1 orphan tx included or conflicted by block"]): + node.p2ps[0].send_blocks_and_test([block_A], node, success=True) + + self.log.info('Test that a transaction in the orphan pool conflicts with a new tip block causes erase this transaction from the orphan pool') + tx_withhold_until_block_B = CTransaction() + tx_withhold_until_block_B.vin.append(CTxIn(outpoint=COutPoint(tx_withhold_until_block_A.sha256, 1))) + tx_withhold_until_block_B.vout.append(CTxOut(nValue=11 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_withhold_until_block_B.calc_sha256() + + tx_orphan_include_by_block_B = CTransaction() + tx_orphan_include_by_block_B.vin.append(CTxIn(outpoint=COutPoint(tx_withhold_until_block_B.sha256, 0))) + tx_orphan_include_by_block_B.vout.append(CTxOut(nValue=10 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_orphan_include_by_block_B.calc_sha256() + + tx_orphan_conflict_by_block_B = CTransaction() + tx_orphan_conflict_by_block_B.vin.append(CTxIn(outpoint=COutPoint(tx_withhold_until_block_B.sha256, 0))) + tx_orphan_conflict_by_block_B.vout.append(CTxOut(nValue=9 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_orphan_conflict_by_block_B.calc_sha256() + self.log.info('Send the orphan ... ') + node.p2ps[0].send_txs_and_test([tx_orphan_conflict_by_block_B], node, success=False) + + tip = int(node.getbestblockhash(), 16) + height = node.getblockcount() + 1 + block_B = create_block(tip, create_coinbase(height)) + block_B.vtx.extend([tx_withhold_until_block_B, tx_orphan_include_by_block_B]) + block_B.hashMerkleRoot = block_B.calc_merkle_root() + block_B.solve() + + self.log.info('Send the block that includes a transaction which conflicts with the previous orphan ... ') + with node.assert_debug_log(["Erased 1 orphan tx included or conflicted by block"]): + node.p2ps[0].send_blocks_and_test([block_B], node, success=True) + if __name__ == '__main__': InvalidTxRequestTest().main() From 6424b6ac7a4fa69112c8502a794b9a836ab18d7d Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 5 Aug 2022 15:21:23 -0400 Subject: [PATCH 4/4] Merge bitcoin/bitcoin#24699: wallet: Improve AvailableCoins performance by reducing duplicated operations bc886fcb31e1afa7bbf7b86bfd93e51da7076ccf Change mapWallet to be a std::unordered_map (Andrew Chow) 272356024db978c92112167f8d8e4cc62adad63d Change getWalletTxs to return a set instead of a vector (Andrew Chow) 97532867cf51db3e941231fbdc60f9f4fa0012a0 Change mapTxSpends to be a std::unordered_multimap (Andrew Chow) 1f798fe85ba952273005f68e36ed48cfc36f4c9d wallet: Cache SigningProviders (Andrew Chow) 8a105ecd1aeff15f84c3883e2762bf71ad59d920 wallet: Use CalculateMaximumSignedInputSize to indicate solvability (Andrew Chow) Pull request description: While running my coin selection simulations, I noticed that towards the end of the simulation, the wallet would become slow to make new transactions. The wallet generally performs much more slowly when there are a large number of transactions and/or a large number of keys. The improvements here are focused on wallets with a large number of transactions as that is what the simulations produce. Most of the slowdown I observed was due to `DescriptorScriptPubKeyMan::GetSigningProvider` re-deriving keys every time it is called. To avoid this, it will now cache the `SigningProvider` produced so that repeatedly fetching the `SigningProvider` for the same script will not result in the same key being derived over and over. This has a side effect of making the function non-const, which makes a lot of other functions non-const as well. This helps with wallets with lots of address reuse (as my coin selection simulations are), but not if addresses are not reused as keys will end up needing to be derived the first time `GetSigningProvider` is called for a script. The `GetSigningProvider` problem was also exacerbated by unnecessarily fetching a `SigningProvider` for the same script multiple times. A `SigningProvider` is retrieved to be used inside of `IsSolvable`. A few lines later, we use `GetTxSpendSize` which fetches a `SigningProvider` and then calls `CalculateMaximumSignedInputSize`. We can avoid a second call to `GetSigningProvider` by using `CalculateMaximumSignedInputSize` directly with the `SigningProvider` already retrieved for `IsSolvable`. There is an additional slowdown where `ProduceSignature` with a dummy signer is called twice for each output. The first time is `IsSolvable` checks that `ProduceSignature` succeeds, thereby informing whether we have solving data. The second is `CalculateMaximumSignedInputSize` which returns -1 if `ProduceSignature` fails, and returns the input size otherwise. We can reduce this to one call of `ProduceSignature` by using `CalculateMaximumSignedInputSize`'s result to set `solvable`. Lastly, a lot of time is spent looking in `mapWallet` and `mapTxSpends` to determine whether an output is already spent. The performance of these lookups is slightly improved by changing those maps to use `std::unordered_map` and `std::unordered_multimap` respectively. ACKs for top commit: Xekyo: ACK bc886fcb31e1afa7bbf7b86bfd93e51da7076ccf furszy: diff re-reACK bc886fcb Tree-SHA512: fd710fe1224ef67d2bb83d6ac9e7428d9f76a67f14085915f9d80e1a492d2c51cb912edfcaad1db11c2edf8d2d97eb7ddd95bfb364587fb1f143490fd72c9ec1 --- src/interfaces/wallet.h | 4 ++- src/wallet/interfaces.cpp | 7 +++-- src/wallet/scriptpubkeyman.cpp | 17 +++++++++--- src/wallet/scriptpubkeyman.h | 2 ++ src/wallet/spend.cpp | 6 +++-- src/wallet/test/wallet_tests.cpp | 6 ++--- src/wallet/wallet.cpp | 46 +++++++++++++++++++------------- src/wallet/wallet.h | 8 +++--- 8 files changed, 61 insertions(+), 35 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index c0fbd86cfc2f1..ac23e5a373a0e 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -189,7 +189,7 @@ class Wallet virtual WalletTx getWalletTx(const uint256& txid) = 0; //! Get list of all wallet transactions. - virtual std::vector getWalletTxs() = 0; + virtual std::set getWalletTxs() = 0; //! Try to get updated status for a particular transaction, if possible without blocking. virtual bool tryGetTxStatus(const uint256& txid, @@ -423,6 +423,8 @@ struct WalletTx bool is_coinbase; bool is_platform_transfer{false}; bool is_denominate; + + bool operator<(const WalletTx& a) const { return tx->GetHash() < a.tx->GetHash(); } }; //! Updated transaction status. diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 05607fba25f4b..acf917f6cd11b 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -338,13 +338,12 @@ class WalletImpl : public Wallet } return {}; } - std::vector getWalletTxs() override + std::set getWalletTxs() override { LOCK(m_wallet->cs_wallet); - std::vector result; - result.reserve(m_wallet->mapWallet.size()); + std::set result; for (const auto& entry : m_wallet->mapWallet) { - result.emplace_back(MakeWalletTx(*m_wallet, entry.second)); + result.emplace(MakeWalletTx(*m_wallet, entry.second)); } return result; } diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index b676ab9a1183d..93586fc2cda17 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -2161,10 +2161,21 @@ std::unique_ptr DescriptorScriptPubKeyMan::GetSigningProvid std::unique_ptr DescriptorScriptPubKeyMan::GetSigningProvider(int32_t index, bool include_private) const { AssertLockHeld(cs_desc_man); - // Get the scripts, keys, and key origins for this script + std::unique_ptr out_keys = std::make_unique(); - std::vector scripts_temp; - if (!m_wallet_descriptor.descriptor->ExpandFromCache(index, m_wallet_descriptor.cache, scripts_temp, *out_keys)) return nullptr; + + // Fetch SigningProvider from cache to avoid re-deriving + auto it = m_map_signing_providers.find(index); + if (it != m_map_signing_providers.end()) { + *out_keys = Merge(*out_keys, it->second); + } else { + // Get the scripts, keys, and key origins for this script + std::vector scripts_temp; + if (!m_wallet_descriptor.descriptor->ExpandFromCache(index, m_wallet_descriptor.cache, scripts_temp, *out_keys)) return nullptr; + + // Cache SigningProvider so we don't need to re-derive if we need this SigningProvider again + m_map_signing_providers[index] = *out_keys; + } if (HavePrivateKeys() && include_private) { FlatSigningProvider master_provider; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 2686ce4d41ea6..c5cdab127fd94 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -524,6 +524,8 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan KeyMap GetKeys() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); + // Cached FlatSigningProviders to avoid regenerating them each time they are needed. + mutable std::map m_map_signing_providers; // Fetch the SigningProvider for the given script and optionally include private keys std::unique_ptr GetSigningProvider(const CScript& script, bool include_private = false) const; // Fetch the SigningProvider for the given pubkey and always include private keys. This should only be called by signing code. diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index fadc5b9cf29af..1e5e1bec13a8c 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -171,9 +171,11 @@ void AvailableCoins(const CWallet& wallet, std::vector& vCoins, const C std::unique_ptr provider = wallet.GetSolvingProvider(wtx.tx->vout[i].scriptPubKey); - bool solvable = provider ? IsSolvable(*provider, wtx.tx->vout[i].scriptPubKey) : false; + int input_bytes = CalculateMaximumSignedInputSize(wtx.tx->vout[i], COutPoint(), provider.get(), coinControl); + // Because CalculateMaximumSignedInputSize just uses ProduceSignature and makes a dummy signature, + // it is safe to assume that this input is solvable if input_bytes is greater -1. + bool solvable = input_bytes > -1; bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); - int input_bytes = GetTxSpendSize(wallet, wtx, i, (coinControl && coinControl->fAllowWatchOnly)); vCoins.emplace_back(COutPoint(wtx.GetHash(), i), wtx.tx->vout.at(i), nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 8bfa5339ee781..fba6699298450 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -850,16 +850,16 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) { auto block_hash = block_tx.GetHash(); - auto prev_hash = m_coinbase_txns[0]->GetHash(); + auto prev_tx = m_coinbase_txns[0]; LOCK(wallet->cs_wallet); - BOOST_CHECK(wallet->HasWalletSpend(prev_hash)); + BOOST_CHECK(wallet->HasWalletSpend(prev_tx)); BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 1u); std::vector vHashIn{ block_hash }, vHashOut; BOOST_CHECK_EQUAL(wallet->ZapSelectTx(vHashIn, vHashOut), DBErrors::LOAD_OK); - BOOST_CHECK(!wallet->HasWalletSpend(prev_hash)); + BOOST_CHECK(!wallet->HasWalletSpend(prev_tx)); BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 0u); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 680dff156e211..fe6ec1721c162 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -411,7 +411,7 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const { AssertLockHeld(cs_wallet); - std::map::const_iterator it = mapWallet.find(hash); + const auto it = mapWallet.find(hash); if (it == mapWallet.end()) return nullptr; return &(it->second); @@ -525,7 +525,7 @@ std::set CWallet::GetConflicts(const uint256& txid) const std::set result; AssertLockHeld(cs_wallet); - std::map::const_iterator it = mapWallet.find(txid); + const auto it = mapWallet.find(txid); if (it == mapWallet.end()) return result; const CWalletTx& wtx = it->second; @@ -543,11 +543,17 @@ std::set CWallet::GetConflicts(const uint256& txid) const return result; } -bool CWallet::HasWalletSpend(const uint256& txid) const +bool CWallet::HasWalletSpend(const CTransactionRef& tx) const { AssertLockHeld(cs_wallet); - auto iter = mapTxSpends.lower_bound(COutPoint(txid, 0)); - return (iter != mapTxSpends.end() && iter->first.hash == txid); + const uint256& txid = tx->GetHash(); + for (unsigned int i = 0; i < tx->vout.size(); ++i) { + auto iter = mapTxSpends.find(COutPoint(txid, i)); + if (iter != mapTxSpends.end()) { + return true; + } + } + return false; } void CWallet::Flush() @@ -612,7 +618,7 @@ bool CWallet::IsSpent(const uint256& hash, unsigned int n) const for (TxSpends::const_iterator it = range.first; it != range.second; ++it) { const uint256& wtxid = it->second; - std::map::const_iterator mit = mapWallet.find(wtxid); + const auto mit = mapWallet.find(wtxid); if (mit != mapWallet.end()) { int depth = GetTxDepthInMainChain(mit->second); if (depth > 0 || (depth == 0 && !mit->second.isAbandoned())) @@ -1154,12 +1160,13 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) batch.WriteTx(wtx); NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED); // Iterate over all its outputs, and mark transactions in the wallet that spend them abandoned too - TxSpends::const_iterator iter = mapTxSpends.lower_bound(COutPoint(now, 0)); - while (iter != mapTxSpends.end() && iter->first.hash == now) { - if (!done.count(iter->second)) { - todo.insert(iter->second); + for (unsigned int i = 0; i < wtx.tx->vout.size(); ++i) { + std::pair range = mapTxSpends.equal_range(COutPoint(now, i)); + for (TxSpends::const_iterator iter = range.first; iter != range.second; ++iter) { + if (!done.count(iter->second)) { + todo.insert(iter->second); + } } - iter++; } // If a transaction changes 'conflicted' state, that changes the balance // available of the outputs it spends. So force those to be recomputed @@ -1220,12 +1227,13 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c wtx.MarkDirty(); batch.WriteTx(wtx); // Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too - TxSpends::const_iterator iter = mapTxSpends.lower_bound(COutPoint(now, 0)); - while (iter != mapTxSpends.end() && iter->first.hash == now) { - if (!done.count(iter->second)) { - todo.insert(iter->second); - } - iter++; + for (unsigned int i = 0; i < wtx.tx->vout.size(); ++i) { + std::pair range = mapTxSpends.equal_range(COutPoint(now, i)); + for (TxSpends::const_iterator iter = range.first; iter != range.second; ++iter) { + if (!done.count(iter->second)) { + todo.insert(iter->second); + } + } } // If a transaction changes 'conflicted' state, that changes the balance // available of the outputs it spends. So force those to be recomputed @@ -1362,7 +1370,7 @@ CAmount CWallet::GetDebit(const CTxIn &txin, const isminefilter& filter) const { { LOCK(cs_wallet); - std::map::const_iterator mi = mapWallet.find(txin.prevout.hash); + const auto mi = mapWallet.find(txin.prevout.hash); if (mi != mapWallet.end()) { const CWalletTx& prev = (*mi).second; @@ -1934,7 +1942,7 @@ bool CWallet::SignTransaction(CMutableTransaction& tx) const // Build coins map std::map coins; for (auto& input : tx.vin) { - std::map::const_iterator mi = mapWallet.find(input.prevout.hash); + const auto mi = mapWallet.find(input.prevout.hash); if(mi == mapWallet.end() || input.prevout.n >= mi->second.tx->vout.size()) { return false; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 4c030e8078587..97b2ec91c6a08 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -42,6 +43,7 @@ #include #include #include +#include #include #include @@ -287,7 +289,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati * detect and report conflicts (double-spends or * mutated transactions where the mutant gets mined). */ - typedef std::multimap TxSpends; + typedef std::unordered_multimap TxSpends; TxSpends mapTxSpends GUARDED_BY(cs_wallet); void AddToSpends(const COutPoint& outpoint, const uint256& wtxid, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void AddToSpends(const CWalletTx& wtx, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); @@ -469,7 +471,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati /** Map from txid to CWalletTx for all transactions this wallet is * interested in, including received and sent transactions. */ - std::map mapWallet GUARDED_BY(cs_wallet); + std::unordered_map mapWallet GUARDED_BY(cs_wallet); typedef std::multimap TxItems; TxItems wtxOrdered; @@ -820,7 +822,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati std::set GetConflicts(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Check if a given transaction has any of its outputs spent by another transaction in the wallet - bool HasWalletSpend(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool HasWalletSpend(const CTransactionRef& tx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Flush wallet (bitdb flush) void Flush();