From 544131f3fba9ea07fee29f9d3ee0116cd5d8a5b2 Mon Sep 17 00:00:00 2001 From: ishaanam Date: Thu, 30 Nov 2023 16:12:24 -0500 Subject: [PATCH 001/116] rpc, test: test sendall spends unconfirmed change and unconfirmed inputs when specified --- src/wallet/rpc/spend.cpp | 2 +- test/functional/wallet_sendall.py | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 5a13b5ac8ecfa..0a43293b2258d 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -1292,7 +1292,7 @@ RPCHelpMan sendall() { return RPCHelpMan{"sendall", "EXPERIMENTAL warning: this call may be changed in future releases.\n" - "\nSpend the value of all (or specific) confirmed UTXOs in the wallet to one or more recipients.\n" + "\nSpend the value of all (or specific) confirmed UTXOs and unconfirmed change in the wallet to one or more recipients.\n" "Unconfirmed inbound UTXOs and locked UTXOs will not be spent. Sendall will respect the avoid_reuse wallet flag.\n" "If your wallet contains many small inputs, either because it received tiny payments or as a result of accumulating change, consider using `send_max` to exclude inputs that are worth less than the fees needed to spend them.\n", { diff --git a/test/functional/wallet_sendall.py b/test/functional/wallet_sendall.py index c2b800df2189e..70b52641c352e 100755 --- a/test/functional/wallet_sendall.py +++ b/test/functional/wallet_sendall.py @@ -379,6 +379,27 @@ def sendall_with_maxconf(self): assert_equal(len(self.wallet.listunspent()), 1) assert_equal(self.wallet.listunspent()[0]['confirmations'], 6) + @cleanup + def sendall_spends_unconfirmed_change(self): + self.log.info("Test that sendall spends unconfirmed change") + self.add_utxos([17]) + self.wallet.sendtoaddress(self.remainder_target, 10) + assert_greater_than(self.wallet.getbalances()["mine"]["trusted"], 6) + self.test_sendall_success(sendall_args = [self.remainder_target]) + + assert_equal(self.wallet.getbalance(), 0) + + @cleanup + def sendall_spends_unconfirmed_inputs_if_specified(self): + self.log.info("Test that sendall spends specified unconfirmed inputs") + self.def_wallet.sendtoaddress(self.wallet.getnewaddress(), 17) + self.wallet.syncwithvalidationinterfacequeue() + assert_equal(self.wallet.getbalances()["mine"]["untrusted_pending"], 17) + unspent = self.wallet.listunspent(minconf=0)[0] + + self.wallet.sendall(recipients=[self.remainder_target], inputs=[unspent]) + assert_equal(self.wallet.getbalance(), 0) + # This tests needs to be the last one otherwise @cleanup will fail with "Transaction too large" error def sendall_fails_with_transaction_too_large(self): self.log.info("Test that sendall fails if resulting transaction is too large") @@ -460,6 +481,12 @@ def run_test(self): # Sendall only uses outputs with less than a given number of confirmation when using minconf self.sendall_with_maxconf() + # Sendall spends unconfirmed change + self.sendall_spends_unconfirmed_change() + + # Sendall spends unconfirmed inputs if they are specified + self.sendall_spends_unconfirmed_inputs_if_specified() + # Sendall fails when many inputs result to too large transaction self.sendall_fails_with_transaction_too_large() From 36757941a05b65c2b61a83820afdf5effd8fc9a2 Mon Sep 17 00:00:00 2001 From: ishaanam Date: Sat, 9 Dec 2023 23:05:15 -0500 Subject: [PATCH 002/116] wallet, rpc: implement ancestor aware funding for sendall --- src/wallet/rpc/spend.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 0a43293b2258d..1474c4ef4ee7f 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -1467,10 +1467,18 @@ RPCHelpMan sendall() } } + std::vector outpoints_spent; + outpoints_spent.reserve(rawTx.vin.size()); + + for (const CTxIn& tx_in : rawTx.vin) { + outpoints_spent.push_back(tx_in.prevout); + } + // estimate final size of tx const TxSize tx_size{CalculateMaximumSignedTxSize(CTransaction(rawTx), pwallet.get())}; const CAmount fee_from_size{fee_rate.GetFee(tx_size.vsize)}; - const CAmount effective_value{total_input_value - fee_from_size}; + const std::optional total_bump_fees{pwallet->chain().calculateCombinedBumpFee(outpoints_spent, fee_rate)}; + CAmount effective_value = total_input_value - fee_from_size - total_bump_fees.value_or(0); if (fee_from_size > pwallet->m_default_max_tx_fee) { throw JSONRPCError(RPC_WALLET_ERROR, TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED).original); From 32c80413fdb063199f3bee719c4651bd63f05fce Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 20 Dec 2023 15:41:05 -0500 Subject: [PATCH 003/116] bench: add benchmark for checkblockindex --- src/Makefile.bench.include | 1 + src/bench/checkblockindex.cpp | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 src/bench/checkblockindex.cpp diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index 7ba0111fa686f..2ba72c9e76b7f 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -23,6 +23,7 @@ bench_bench_bitcoin_SOURCES = \ bench/ccoins_caching.cpp \ bench/chacha20.cpp \ bench/checkblock.cpp \ + bench/checkblockindex.cpp \ bench/checkqueue.cpp \ bench/crypto_hash.cpp \ bench/data.cpp \ diff --git a/src/bench/checkblockindex.cpp b/src/bench/checkblockindex.cpp new file mode 100644 index 0000000000000..e8a848dbd409b --- /dev/null +++ b/src/bench/checkblockindex.cpp @@ -0,0 +1,20 @@ +// Copyright (c) 2023-present 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 CheckBlockIndex(benchmark::Bench& bench) +{ + auto testing_setup{MakeNoLogFileContext()}; + // Mine some more blocks + testing_setup->mineBlocks(1000); + bench.run([&] { + testing_setup->m_node.chainman->CheckBlockIndex(); + }); +} + + +BENCHMARK(CheckBlockIndex, benchmark::PriorityLevel::HIGH); From d5a631b9597e5029a5048d9b8ad84ea4536bbac0 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 23 Aug 2023 14:05:42 -0400 Subject: [PATCH 004/116] validation: improve performance of CheckBlockIndex by not saving all indexes in a std::multimap, but only those that are not part of the best header chain. The indexes of the best header chain are stored in a vector, which, in the typical case of a mostly linear chain with a few forks, results in a much smaller multimap, and increases performance noticeably for long chains. This does not change the actual consistency checks that are being performed for each index, just the way the block index tree is stored and traversed. Co-authored-by: Ryan Ofsky --- src/validation.cpp | 50 +++++++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 14 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index b6d0c38f391da..9dad39e6579e5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5050,19 +5050,28 @@ void ChainstateManager::CheckBlockIndex() return; } - // Build forward-pointing map of the entire block tree. + // Build forward-pointing data structure for the entire block tree. + // For performance reasons, indexes of the best header chain are stored in a vector (within CChain). + // All remaining blocks are stored in a multimap. + // The best header chain can differ from the active chain: E.g. its entries may belong to blocks that + // are not yet validated. + CChain best_hdr_chain; + assert(m_best_header); + best_hdr_chain.SetTip(*m_best_header); + std::multimap forward; for (auto& [_, block_index] : m_blockman.m_block_index) { - forward.emplace(block_index.pprev, &block_index); + // Only save indexes in forward that are not part of the best header chain. + if (!best_hdr_chain.Contains(&block_index)) { + // Only genesis, which must be part of the best header chain, can have a nullptr parent. + assert(block_index.pprev); + forward.emplace(block_index.pprev, &block_index); + } } + assert(forward.size() + best_hdr_chain.Height() + 1 == m_blockman.m_block_index.size()); - assert(forward.size() == m_blockman.m_block_index.size()); - - std::pair::iterator,std::multimap::iterator> rangeGenesis = forward.equal_range(nullptr); - CBlockIndex *pindex = rangeGenesis.first->second; - rangeGenesis.first++; - assert(rangeGenesis.first == rangeGenesis.second); // There is only one index entry with parent nullptr. - + CBlockIndex* pindex = best_hdr_chain[0]; + assert(pindex); // Iterate over the entire block tree, using depth-first search. // Along the way, remember whether there are blocks on the path from genesis // block being explored which are the first to have certain properties. @@ -5274,14 +5283,21 @@ void ChainstateManager::CheckBlockIndex() // assert(pindex->GetBlockHash() == pindex->GetBlockHeader().GetHash()); // Perhaps too slow // End: actual consistency checks. - // Try descending into the first subnode. + + // Try descending into the first subnode. Always process forks first and the best header chain after. snap_update_firsts(); std::pair::iterator,std::multimap::iterator> range = forward.equal_range(pindex); if (range.first != range.second) { - // A subnode was found. + // A subnode not part of the best header chain was found. pindex = range.first->second; nHeight++; continue; + } else if (best_hdr_chain.Contains(pindex)) { + // Descend further into best header chain. + nHeight++; + pindex = best_hdr_chain[nHeight]; + if (!pindex) break; // we are finished, since the best header chain is always processed last + continue; } // This is a leaf node. // Move upwards until we reach a node of which we have not yet visited the last child. @@ -5307,9 +5323,15 @@ void ChainstateManager::CheckBlockIndex() // Proceed to the next one. rangePar.first++; if (rangePar.first != rangePar.second) { - // Move to the sibling. + // Move to a sibling not part of the best header chain. pindex = rangePar.first->second; break; + } else if (pindexPar == best_hdr_chain[nHeight - 1]) { + // Move to pindex's sibling on the best-chain, if it has one. + pindex = best_hdr_chain[nHeight]; + // There will not be a next block if (and only if) parent block is the best header. + assert((pindex == nullptr) == (pindexPar == best_hdr_chain.Tip())); + break; } else { // Move up further. pindex = pindexPar; @@ -5319,8 +5341,8 @@ void ChainstateManager::CheckBlockIndex() } } - // Check that we actually traversed the entire map. - assert(nNodes == forward.size()); + // Check that we actually traversed the entire block index. + assert(nNodes == forward.size() + best_hdr_chain.Height() + 1); } std::string Chainstate::ToString() From 5bc2077e8f592442b089affdf0b5795fbc053bb8 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Thu, 21 Dec 2023 17:24:07 -0500 Subject: [PATCH 005/116] validation: allow to specify frequency for -checkblockindex This makes it similar to -checkaddrman and -checkmempool, which also allow to run the check occasionally instead of always / never. Co-authored-by: Ryan Ofsky --- src/init.cpp | 2 +- src/kernel/chainstatemanager_opts.h | 2 +- src/node/chainstatemanager_args.cpp | 5 ++++- src/test/util/setup_common.cpp | 2 +- src/validation.cpp | 8 ++++++++ src/validation.h | 2 +- 6 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 0aa04755cbeba..927786e3d5950 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -604,7 +604,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-checkblocks=", strprintf("How many blocks to check at startup (default: %u, 0 = all)", DEFAULT_CHECKBLOCKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checklevel=", strprintf("How thorough the block verification of -checkblocks is: %s (0-4, default: %u)", Join(CHECKLEVEL_DOC, ", "), DEFAULT_CHECKLEVEL), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); - argsman.AddArg("-checkblockindex", strprintf("Do a consistency check for the block tree, chainstate, and other validation data structures occasionally. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-checkblockindex", strprintf("Do a consistency check for the block tree, chainstate, and other validation data structures every operations. Use 0 to disable. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checkaddrman=", strprintf("Run addrman consistency checks every operations. Use 0 to disable. (default: %u)", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checkmempool=", strprintf("Run mempool consistency checks every transactions. Use 0 to disable. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block %s (default: %u)", defaultChainParams->Checkpoints().GetHeight(), DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h index de5f78494a261..076841c3c9d4a 100644 --- a/src/kernel/chainstatemanager_opts.h +++ b/src/kernel/chainstatemanager_opts.h @@ -33,7 +33,7 @@ namespace kernel { struct ChainstateManagerOpts { const CChainParams& chainparams; fs::path datadir; - std::optional check_block_index{}; + std::optional check_block_index{}; bool checkpoints_enabled{DEFAULT_CHECKPOINTS_ENABLED}; //! If set, it will override the minimum work we will assume exists on some valid chain. std::optional minimum_chain_work{}; diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp index 1cc126cb051c3..bc4a815a3edf8 100644 --- a/src/node/chainstatemanager_args.cpp +++ b/src/node/chainstatemanager_args.cpp @@ -24,7 +24,10 @@ namespace node { util::Result ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts) { - if (auto value{args.GetBoolArg("-checkblockindex")}) opts.check_block_index = *value; + if (auto value{args.GetIntArg("-checkblockindex")}) { + // Interpret bare -checkblockindex argument as 1 instead of 0. + opts.check_block_index = args.GetArg("-checkblockindex")->empty() ? 1 : *value; + } if (auto value{args.GetBoolArg("-checkpoints")}) opts.checkpoints_enabled = *value; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2c18184261e48..666a7a03034fd 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -238,7 +238,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto const ChainstateManager::Options chainman_opts{ .chainparams = chainparams, .datadir = m_args.GetDataDirNet(), - .check_block_index = true, + .check_block_index = 1, .notifications = *m_node.notifications, .signals = m_node.validation_signals.get(), .worker_threads_num = 2, diff --git a/src/validation.cpp b/src/validation.cpp index 9dad39e6579e5..a547614fc140e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5034,6 +5034,14 @@ void ChainstateManager::LoadExternalBlockFile( LogPrintf("Loaded %i blocks from external file in %dms\n", nLoaded, Ticks(SteadyClock::now() - start)); } +bool ChainstateManager::ShouldCheckBlockIndex() const +{ + // Assert to verify Flatten() has been called. + if (!*Assert(m_options.check_block_index)) return false; + if (GetRand(*m_options.check_block_index) >= 1) return false; + return true; +} + void ChainstateManager::CheckBlockIndex() { if (!ShouldCheckBlockIndex()) { diff --git a/src/validation.h b/src/validation.h index bcf153719af9a..15a1cbdc8de85 100644 --- a/src/validation.h +++ b/src/validation.h @@ -933,7 +933,7 @@ class ChainstateManager const CChainParams& GetParams() const { return m_options.chainparams; } const Consensus::Params& GetConsensus() const { return m_options.chainparams.GetConsensus(); } - bool ShouldCheckBlockIndex() const { return *Assert(m_options.check_block_index); } + bool ShouldCheckBlockIndex() const; const arith_uint256& MinimumChainWork() const { return *Assert(m_options.minimum_chain_work); } const uint256& AssumedValidBlock() const { return *Assert(m_options.assumed_valid_block); } kernel::Notifications& GetNotifications() const { return m_options.notifications; }; From 07aba8dd215b23b06853b1a9fe04ac8b08f62932 Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Mon, 29 Apr 2024 11:53:04 -0400 Subject: [PATCH 006/116] functional test: ensure confirmed utxo being sourced for 2nd chain --- test/functional/mempool_package_onemore.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/mempool_package_onemore.py b/test/functional/mempool_package_onemore.py index 921c190668e77..281bb046b7ae9 100755 --- a/test/functional/mempool_package_onemore.py +++ b/test/functional/mempool_package_onemore.py @@ -41,7 +41,7 @@ def run_test(self): for _ in range(DEFAULT_ANCESTOR_LIMIT - 4): utxo, = self.chain_tx([utxo]) chain.append(utxo) - second_chain, = self.chain_tx([self.wallet.get_utxo()]) + second_chain, = self.chain_tx([self.wallet.get_utxo(confirmed_only=True)]) # Check mempool has DEFAULT_ANCESTOR_LIMIT + 1 transactions in it assert_equal(len(self.nodes[0].getrawmempool()), DEFAULT_ANCESTOR_LIMIT + 1) From d7290d662f494503f28e087dd728b492c0bb2c5f Mon Sep 17 00:00:00 2001 From: Ayush Singh Date: Thu, 13 Jul 2023 17:07:35 +0530 Subject: [PATCH 007/116] fuzz: wallet, add target for Crypter --- src/Makefile.test.include | 1 + src/wallet/test/fuzz/crypter.cpp | 92 ++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 src/wallet/test/fuzz/crypter.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 942e0bf69b34c..31d2bd2ddbb0c 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -203,6 +203,7 @@ endif FUZZ_WALLET_SRC = \ wallet/test/fuzz/coincontrol.cpp \ wallet/test/fuzz/coinselection.cpp \ + wallet/test/fuzz/crypter.cpp \ wallet/test/fuzz/fees.cpp \ wallet/test/fuzz/parse_iso8601.cpp diff --git a/src/wallet/test/fuzz/crypter.cpp b/src/wallet/test/fuzz/crypter.cpp new file mode 100644 index 0000000000000..62dd1bfde0a00 --- /dev/null +++ b/src/wallet/test/fuzz/crypter.cpp @@ -0,0 +1,92 @@ +// Copyright (c) 2022 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 +#include +#include + +namespace wallet { +namespace { + +const TestingSetup* g_setup; +void initialize_crypter() +{ + static const auto testing_setup = MakeNoLogFileContext(); + g_setup = testing_setup.get(); +} + +FUZZ_TARGET(crypter, .init = initialize_crypter) +{ + FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + bool good_data{true}; + + CCrypter crypt; + // These values are regularly updated within `CallOneOf` + std::vector cipher_text_ed; + CKeyingMaterial plain_text_ed; + const std::vector random_key = ConsumeRandomLengthByteVector(fuzzed_data_provider); + + LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10000) + { + CallOneOf( + fuzzed_data_provider, + [&] { + const std::string random_string = fuzzed_data_provider.ConsumeRandomLengthString(); + SecureString secure_string(random_string.begin(), random_string.end()); + + const unsigned int derivation_method = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral(); + + // Limiting the value of nRounds since it is otherwise uselessly expensive and causes a timeout when fuzzing. + crypt.SetKeyFromPassphrase(/*strKeyData=*/secure_string, + /*chSalt=*/ConsumeRandomLengthByteVector(fuzzed_data_provider), + /*nRounds=*/fuzzed_data_provider.ConsumeIntegralInRange(0, 25000), + /*nDerivationMethod=*/derivation_method); + }, + [&] { + const std::vector random_vector = ConsumeFixedLengthByteVector(fuzzed_data_provider, 32); + const CKeyingMaterial new_key(random_vector.begin(), random_vector.end()); + const std::vector& new_IV = ConsumeFixedLengthByteVector(fuzzed_data_provider, 16); + crypt.SetKey(new_key, new_IV); + }, + [&] { + const std::vector random_vector = ConsumeRandomLengthByteVector(fuzzed_data_provider); + plain_text_ed = CKeyingMaterial(random_vector.begin(), random_vector.end()); + }, + [&] { + cipher_text_ed = ConsumeRandomLengthByteVector(fuzzed_data_provider); + }, + [&] { + (void)crypt.Encrypt(plain_text_ed, cipher_text_ed); + }, + [&] { + (void)crypt.Decrypt(cipher_text_ed, plain_text_ed); + }, + [&] { + const CKeyingMaterial master_key(random_key.begin(), random_key.end()); + const uint256 iv = ConsumeUInt256(fuzzed_data_provider); + EncryptSecret(master_key, plain_text_ed, iv, cipher_text_ed); + }, + [&] { + const CKeyingMaterial master_key(random_key.begin(), random_key.end()); + const uint256 iv = ConsumeUInt256(fuzzed_data_provider); + DecryptSecret(master_key, cipher_text_ed, iv, plain_text_ed); + }, + [&] { + std::optional random_pub_key = ConsumeDeserializable(fuzzed_data_provider); + if (!random_pub_key) { + good_data = false; + return; + } + const CPubKey pub_key = *random_pub_key; + const CKeyingMaterial master_key(random_key.begin(), random_key.end()); + const std::vector crypted_secret = ConsumeRandomLengthByteVector(fuzzed_data_provider); + CKey key; + DecryptKey(master_key, crypted_secret, pub_key, key); + }); + } +} +} // namespace +} // namespace wallet From 3635d432681847313c098f9827483372a840e70f Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 19 Aug 2023 09:54:30 -0300 Subject: [PATCH 008/116] test: rpc_createmultisig, remove manual wallet initialization There is no need to manually initialize the wallets within the test case. The test framework already initializes them when `_requires_wallet` is true. --- test/functional/rpc_createmultisig.py | 2 +- test/functional/test_framework/test_framework.py | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index 65d7b4c4223f4..a540e7730bed2 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -32,6 +32,7 @@ def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 3 self.supports_cli = False + self.enable_wallet_if_possible() def get_keys(self): self.pub = [] @@ -51,7 +52,6 @@ def run_test(self): self.wallet = MiniWallet(test_node=node0) if self.is_bdb_compiled(): - self.import_deterministic_coinbase_privkeys() self.check_addmultisigaddress_errors() self.log.info('Generating blocks ...') diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index a2f767cc9898c..71551ffd7cb41 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -444,6 +444,10 @@ def init_wallet(self, *, node): n.createwallet(wallet_name=wallet_name, descriptors=self.options.descriptors, load_on_startup=True) n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase', rescan=True) + # Only enables wallet support when the module is available + def enable_wallet_if_possible(self): + self._requires_wallet = self.is_wallet_compiled() + def run_test(self): """Tests must override this method to define test logic""" raise NotImplementedError From b5a328943362cfac6e90fd4e1b167c357d53b7d4 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 19 Aug 2023 16:47:54 -0300 Subject: [PATCH 009/116] test: refactor, multiple cleanups in rpc_createmultisig.py Cleaning up the test in the following ways: * Generate priv-pub key pairs used for testing only once (instead of doing it 4 times). * Simplifies 'wmulti' wallet creation, load and unload process. * Removes confusing class members initialized and updated inside a nested for-loop. * Simplifies do_multisig() outpoint detection: The outpoint index information is already contained in MiniWallet's `send_to` return value dictionary as "sent_vout". Co-authored-by: Sebastian Falbesoner --- test/functional/rpc_createmultisig.py | 90 ++++++++++++--------------- 1 file changed, 40 insertions(+), 50 deletions(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index a540e7730bed2..4e6683182c3f8 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -10,9 +10,9 @@ from test_framework.address import address_to_scriptpubkey from test_framework.blocktools import COINBASE_MATURITY -from test_framework.authproxy import JSONRPCException from test_framework.descriptors import descsum_create, drop_origins from test_framework.key import ECPubKey +from test_framework.messages import COIN from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_raises_rpc_error, @@ -34,19 +34,22 @@ def set_test_params(self): self.supports_cli = False self.enable_wallet_if_possible() - def get_keys(self): + def create_keys(self, num_keys): self.pub = [] self.priv = [] - node0, node1, node2 = self.nodes - for _ in range(self.nkeys): + for _ in range(num_keys): privkey, pubkey = generate_keypair(wif=True) self.pub.append(pubkey.hex()) self.priv.append(privkey) if self.is_bdb_compiled(): - self.final = node2.getnewaddress() + self.final = self.nodes[2].getnewaddress() else: self.final = getnewdestination('bech32')[2] + def create_wallet(self, node, wallet_name): + node.createwallet(wallet_name=wallet_name, disable_private_keys=True) + return node.get_wallet_rpc(wallet_name) + def run_test(self): node0, node1, node2 = self.nodes self.wallet = MiniWallet(test_node=node0) @@ -57,12 +60,15 @@ def run_test(self): self.log.info('Generating blocks ...') self.generate(self.wallet, 149) + wallet_multi = self.create_wallet(node1, 'wmulti') if self._requires_wallet else None self.moved = 0 - for self.nkeys in [3, 5]: - for self.nsigs in [2, 3]: - for self.output_type in ["bech32", "p2sh-segwit", "legacy"]: - self.get_keys() - self.do_multisig() + self.create_keys(5) + for nkeys in [3, 5]: + for nsigs in [2, 3]: + for output_type in ["bech32", "p2sh-segwit", "legacy"]: + self.do_multisig(nkeys, nsigs, output_type, wallet_multi) + if wallet_multi is not None: + wallet_multi.unloadwallet() if self.is_bdb_compiled(): self.checkbalances() @@ -149,93 +155,77 @@ def checkbalances(self): assert bal2 == self.moved assert_equal(bal0 + bal1 + bal2 + balw, total) - def do_multisig(self): + def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): node0, node1, node2 = self.nodes - - if self.is_bdb_compiled(): - if 'wmulti' not in node1.listwallets(): - try: - node1.loadwallet('wmulti') - except JSONRPCException as e: - path = self.nodes[1].wallets_path / "wmulti" - if e.error['code'] == -18 and "Wallet file verification failed. Failed to load database path '{}'. Path does not exist.".format(path) in e.error['message']: - node1.createwallet(wallet_name='wmulti', disable_private_keys=True) - else: - raise - wmulti = node1.get_wallet_rpc('wmulti') + pub_keys = self.pub[0: nkeys] + priv_keys = self.priv[0: nkeys] # Construct the expected descriptor - desc = 'multi({},{})'.format(self.nsigs, ','.join(self.pub)) - if self.output_type == 'legacy': + desc = 'multi({},{})'.format(nsigs, ','.join(pub_keys)) + if output_type == 'legacy': desc = 'sh({})'.format(desc) - elif self.output_type == 'p2sh-segwit': + elif output_type == 'p2sh-segwit': desc = 'sh(wsh({}))'.format(desc) - elif self.output_type == 'bech32': + elif output_type == 'bech32': desc = 'wsh({})'.format(desc) desc = descsum_create(desc) - msig = node2.createmultisig(self.nsigs, self.pub, self.output_type) + msig = node2.createmultisig(nsigs, pub_keys, output_type) assert 'warnings' not in msig madd = msig["address"] mredeem = msig["redeemScript"] assert_equal(desc, msig['descriptor']) - if self.output_type == 'bech32': + if output_type == 'bech32': assert madd[0:4] == "bcrt" # actually a bech32 address - if self.is_bdb_compiled(): + if wallet_multi is not None: # compare against addmultisigaddress - msigw = wmulti.addmultisigaddress(self.nsigs, self.pub, None, self.output_type) + msigw = wallet_multi.addmultisigaddress(nsigs, pub_keys, None, output_type) maddw = msigw["address"] mredeemw = msigw["redeemScript"] assert_equal(desc, drop_origins(msigw['descriptor'])) # addmultisigiaddress and createmultisig work the same assert maddw == madd assert mredeemw == mredeem - wmulti.unloadwallet() spk = address_to_scriptpubkey(madd) - txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=spk, amount=1300)["txid"] - tx = node0.getrawtransaction(txid, True) - vout = [v["n"] for v in tx["vout"] if madd == v["scriptPubKey"]["address"]] - assert len(vout) == 1 - vout = vout[0] - scriptPubKey = tx["vout"][vout]["scriptPubKey"]["hex"] - value = tx["vout"][vout]["value"] - prevtxs = [{"txid": txid, "vout": vout, "scriptPubKey": scriptPubKey, "redeemScript": mredeem, "amount": value}] + value = decimal.Decimal("0.00001300") + tx = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=spk, amount=int(value * COIN)) + prevtxs = [{"txid": tx["txid"], "vout": tx["sent_vout"], "scriptPubKey": spk.hex(), "redeemScript": mredeem, "amount": value}] self.generate(node0, 1) outval = value - decimal.Decimal("0.00001000") - rawtx = node2.createrawtransaction([{"txid": txid, "vout": vout}], [{self.final: outval}]) + rawtx = node2.createrawtransaction([{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{self.final: outval}]) prevtx_err = dict(prevtxs[0]) del prevtx_err["redeemScript"] - assert_raises_rpc_error(-8, "Missing redeemScript/witnessScript", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + assert_raises_rpc_error(-8, "Missing redeemScript/witnessScript", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err]) # if witnessScript specified, all ok prevtx_err["witnessScript"] = prevtxs[0]["redeemScript"] - node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs-1], [prevtx_err]) # both specified, also ok prevtx_err["redeemScript"] = prevtxs[0]["redeemScript"] - node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs-1], [prevtx_err]) # redeemScript mismatch to witnessScript prevtx_err["redeemScript"] = "6a" # OP_RETURN - assert_raises_rpc_error(-8, "redeemScript does not correspond to witnessScript", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + assert_raises_rpc_error(-8, "redeemScript does not correspond to witnessScript", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err]) # redeemScript does not match scriptPubKey del prevtx_err["witnessScript"] - assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err]) # witnessScript does not match scriptPubKey prevtx_err["witnessScript"] = prevtx_err["redeemScript"] del prevtx_err["redeemScript"] - assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err]) - rawtx2 = node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs - 1], prevtxs) - rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [self.priv[-1]], prevtxs) + rawtx2 = node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs - 1], prevtxs) + rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [priv_keys[-1]], prevtxs) self.moved += outval tx = node0.sendrawtransaction(rawtx3["hex"], 0) @@ -243,7 +233,7 @@ def do_multisig(self): assert tx in node0.getblock(blk)["tx"] txinfo = node0.getrawtransaction(tx, True, blk) - self.log.info("n/m=%d/%d %s size=%d vsize=%d weight=%d" % (self.nsigs, self.nkeys, self.output_type, txinfo["size"], txinfo["vsize"], txinfo["weight"])) + self.log.info("n/m=%d/%d %s size=%d vsize=%d weight=%d" % (nsigs, nkeys, output_type, txinfo["size"], txinfo["vsize"], txinfo["weight"])) if __name__ == '__main__': From 25a81705d376e8c96dad45436ae3fca975b3daf5 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 19 Aug 2023 18:51:06 -0300 Subject: [PATCH 010/116] test: rpc_createmultisig, remove unnecessary checkbalances() The function exists merely to check that the node2's wallet received the transactions created during all the 'do_multisig()' calls. It was created as a standalone function because 'getbalance()' only returns something when transactions are confirmed. So, the rationale on that time was to have a method mining blocks to confirm the recently created transactions to be able to check the incoming balance. This is why we have the "moved" class field. This change removes all the hardcoded amounts and verifies node2 balance reception directly inside 'do_multisig()'. --- test/functional/rpc_createmultisig.py | 36 +++++++-------------------- 1 file changed, 9 insertions(+), 27 deletions(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index 4e6683182c3f8..c2163556b2a04 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -9,7 +9,6 @@ import os from test_framework.address import address_to_scriptpubkey -from test_framework.blocktools import COINBASE_MATURITY from test_framework.descriptors import descsum_create, drop_origins from test_framework.key import ECPubKey from test_framework.messages import COIN @@ -41,10 +40,6 @@ def create_keys(self, num_keys): privkey, pubkey = generate_keypair(wif=True) self.pub.append(pubkey.hex()) self.priv.append(privkey) - if self.is_bdb_compiled(): - self.final = self.nodes[2].getnewaddress() - else: - self.final = getnewdestination('bech32')[2] def create_wallet(self, node, wallet_name): node.createwallet(wallet_name=wallet_name, disable_private_keys=True) @@ -54,14 +49,13 @@ def run_test(self): node0, node1, node2 = self.nodes self.wallet = MiniWallet(test_node=node0) - if self.is_bdb_compiled(): + if self.is_wallet_compiled(): self.check_addmultisigaddress_errors() self.log.info('Generating blocks ...') self.generate(self.wallet, 149) wallet_multi = self.create_wallet(node1, 'wmulti') if self._requires_wallet else None - self.moved = 0 self.create_keys(5) for nkeys in [3, 5]: for nsigs in [2, 3]: @@ -69,8 +63,6 @@ def run_test(self): self.do_multisig(nkeys, nsigs, output_type, wallet_multi) if wallet_multi is not None: wallet_multi.unloadwallet() - if self.is_bdb_compiled(): - self.checkbalances() # Test mixed compressed and uncompressed pubkeys self.log.info('Mixed compressed and uncompressed multisigs are not allowed') @@ -139,22 +131,6 @@ def check_addmultisigaddress_errors(self): pubs = [self.nodes[1].getaddressinfo(addr)["pubkey"] for addr in addresses] assert_raises_rpc_error(-5, "Bech32m multisig addresses cannot be created with legacy wallets", self.nodes[0].addmultisigaddress, 2, pubs, "", "bech32m") - def checkbalances(self): - node0, node1, node2 = self.nodes - self.generate(node0, COINBASE_MATURITY) - - bal0 = node0.getbalance() - bal1 = node1.getbalance() - bal2 = node2.getbalance() - balw = self.wallet.get_balance() - - height = node0.getblockchaininfo()["blocks"] - assert 150 < height < 350 - total = 149 * 50 + (height - 149 - 100) * 25 - assert bal1 == 0 - assert bal2 == self.moved - assert_equal(bal0 + bal1 + bal2 + balw, total) - def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): node0, node1, node2 = self.nodes pub_keys = self.pub[0: nkeys] @@ -196,7 +172,10 @@ def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): self.generate(node0, 1) outval = value - decimal.Decimal("0.00001000") - rawtx = node2.createrawtransaction([{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{self.final: outval}]) + # send coins to node2 when wallet is enabled + node2_balance = node2.getbalances()['mine']['trusted'] if self.is_wallet_compiled() else 0 + out_addr = node2.getnewaddress() if self.is_wallet_compiled() else getnewdestination('bech32')[2] + rawtx = node2.createrawtransaction([{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{out_addr: outval}]) prevtx_err = dict(prevtxs[0]) del prevtx_err["redeemScript"] @@ -227,11 +206,14 @@ def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): rawtx2 = node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs - 1], prevtxs) rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [priv_keys[-1]], prevtxs) - self.moved += outval tx = node0.sendrawtransaction(rawtx3["hex"], 0) blk = self.generate(node0, 1)[0] assert tx in node0.getblock(blk)["tx"] + # When the wallet is enabled, assert node2 sees the incoming amount + if self.is_wallet_compiled(): + assert_equal(node2.getbalances()['mine']['trusted'], node2_balance + outval) + txinfo = node0.getrawtransaction(tx, True, blk) self.log.info("n/m=%d/%d %s size=%d vsize=%d weight=%d" % (nsigs, nkeys, output_type, txinfo["size"], txinfo["vsize"], txinfo["weight"])) From 4f33dbd8f8c0e29f37b04e6af6d2c7905ecceaf6 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 19 Aug 2023 19:18:00 -0300 Subject: [PATCH 011/116] test: rpc_createmultisig, decouple 'test_mixing_uncompressed_and_compressed_keys' And also, simplified the test a bit by re-using the already existing 'wallet_multi' (instead of creating a new one). Plus, removed the 'is_bdb_compiled()' calls which were there basically to check if the test has the wallet compiled or not. --- test/functional/rpc_createmultisig.py | 74 +++++++++++++-------------- 1 file changed, 36 insertions(+), 38 deletions(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index c2163556b2a04..2414dfd116bda 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -61,45 +61,8 @@ def run_test(self): for nsigs in [2, 3]: for output_type in ["bech32", "p2sh-segwit", "legacy"]: self.do_multisig(nkeys, nsigs, output_type, wallet_multi) - if wallet_multi is not None: - wallet_multi.unloadwallet() - - # Test mixed compressed and uncompressed pubkeys - self.log.info('Mixed compressed and uncompressed multisigs are not allowed') - pk0, pk1, pk2 = [getnewdestination('bech32')[0].hex() for _ in range(3)] - - # decompress pk2 - pk_obj = ECPubKey() - pk_obj.set(bytes.fromhex(pk2)) - pk_obj.compressed = False - pk2 = pk_obj.get_bytes().hex() - - if self.is_bdb_compiled(): - node0.createwallet(wallet_name='wmulti0', disable_private_keys=True) - wmulti0 = node0.get_wallet_rpc('wmulti0') - - # Check all permutations of keys because order matters apparently - for keys in itertools.permutations([pk0, pk1, pk2]): - # Results should be the same as this legacy one - legacy_addr = node0.createmultisig(2, keys, 'legacy')['address'] - - if self.is_bdb_compiled(): - result = wmulti0.addmultisigaddress(2, keys, '', 'legacy') - assert_equal(legacy_addr, result['address']) - assert 'warnings' not in result - - # Generate addresses with the segwit types. These should all make legacy addresses - err_msg = ["Unable to make chosen address type, please ensure no uncompressed public keys are present."] - for addr_type in ['bech32', 'p2sh-segwit']: - result = self.nodes[0].createmultisig(nrequired=2, keys=keys, address_type=addr_type) - assert_equal(legacy_addr, result['address']) - assert_equal(result['warnings'], err_msg) - - if self.is_bdb_compiled(): - result = wmulti0.addmultisigaddress(nrequired=2, keys=keys, address_type=addr_type) - assert_equal(legacy_addr, result['address']) - assert_equal(result['warnings'], err_msg) + self.test_mixing_uncompressed_and_compressed_keys(node0, wallet_multi) self.log.info('Testing sortedmulti descriptors with BIP 67 test vectors') with open(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data/rpc_bip67.json'), encoding='utf-8') as f: @@ -217,6 +180,41 @@ def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): txinfo = node0.getrawtransaction(tx, True, blk) self.log.info("n/m=%d/%d %s size=%d vsize=%d weight=%d" % (nsigs, nkeys, output_type, txinfo["size"], txinfo["vsize"], txinfo["weight"])) + def test_mixing_uncompressed_and_compressed_keys(self, node, wallet_multi): + self.log.info('Mixed compressed and uncompressed multisigs are not allowed') + pk0, pk1, pk2 = [getnewdestination('bech32')[0].hex() for _ in range(3)] + + # decompress pk2 + pk_obj = ECPubKey() + pk_obj.set(bytes.fromhex(pk2)) + pk_obj.compressed = False + pk2 = pk_obj.get_bytes().hex() + + # Check all permutations of keys because order matters apparently + for keys in itertools.permutations([pk0, pk1, pk2]): + # Results should be the same as this legacy one + legacy_addr = node.createmultisig(2, keys, 'legacy')['address'] + + if wallet_multi is not None: + # 'addmultisigaddress' should return the same address + result = wallet_multi.addmultisigaddress(2, keys, '', 'legacy') + assert_equal(legacy_addr, result['address']) + assert 'warnings' not in result + + # Generate addresses with the segwit types. These should all make legacy addresses + err_msg = ["Unable to make chosen address type, please ensure no uncompressed public keys are present."] + + for addr_type in ['bech32', 'p2sh-segwit']: + result = self.nodes[0].createmultisig(nrequired=2, keys=keys, address_type=addr_type) + assert_equal(legacy_addr, result['address']) + assert_equal(result['warnings'], err_msg) + + if wallet_multi is not None: + result = wallet_multi.addmultisigaddress(nrequired=2, keys=keys, address_type=addr_type) + assert_equal(legacy_addr, result['address']) + assert_equal(result['warnings'], err_msg) + + if __name__ == '__main__': RpcCreateMultiSigTest().main() From f7a173b5785cda460470df9a74a0e0f94d7f9a18 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 19 Aug 2023 19:20:04 -0300 Subject: [PATCH 012/116] test: rpc_createmultisig, decouple 'test_sortedmulti_descriptors_bip67' Move-only commit. No behavior change. --- test/functional/rpc_createmultisig.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index 2414dfd116bda..983aeafc1c5bc 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -63,18 +63,7 @@ def run_test(self): self.do_multisig(nkeys, nsigs, output_type, wallet_multi) self.test_mixing_uncompressed_and_compressed_keys(node0, wallet_multi) - - self.log.info('Testing sortedmulti descriptors with BIP 67 test vectors') - with open(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data/rpc_bip67.json'), encoding='utf-8') as f: - vectors = json.load(f) - - for t in vectors: - key_str = ','.join(t['keys']) - desc = descsum_create('sh(sortedmulti(2,{}))'.format(key_str)) - assert_equal(self.nodes[0].deriveaddresses(desc)[0], t['address']) - sorted_key_str = ','.join(t['sorted_keys']) - sorted_key_desc = descsum_create('sh(multi(2,{}))'.format(sorted_key_str)) - assert_equal(self.nodes[0].deriveaddresses(sorted_key_desc)[0], t['address']) + self.test_sortedmulti_descriptors_bip67() # Check that bech32m is currently not allowed assert_raises_rpc_error(-5, "createmultisig cannot create bech32m multisig addresses", self.nodes[0].createmultisig, 2, self.pub, "bech32m") @@ -214,6 +203,18 @@ def test_mixing_uncompressed_and_compressed_keys(self, node, wallet_multi): assert_equal(legacy_addr, result['address']) assert_equal(result['warnings'], err_msg) + def test_sortedmulti_descriptors_bip67(self): + self.log.info('Testing sortedmulti descriptors with BIP 67 test vectors') + with open(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data/rpc_bip67.json'), encoding='utf-8') as f: + vectors = json.load(f) + + for t in vectors: + key_str = ','.join(t['keys']) + desc = descsum_create('sh(sortedmulti(2,{}))'.format(key_str)) + assert_equal(self.nodes[0].deriveaddresses(desc)[0], t['address']) + sorted_key_str = ','.join(t['sorted_keys']) + sorted_key_desc = descsum_create('sh(multi(2,{}))'.format(sorted_key_str)) + assert_equal(self.nodes[0].deriveaddresses(sorted_key_desc)[0], t['address']) if __name__ == '__main__': From 0c9fedfc45fa7cbd6801ca5fd756863ec9a6911c Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Aug 2023 17:45:34 -0300 Subject: [PATCH 013/116] fix incorrect multisig redeem script size limit for segwit The multisig script generation process currently fails when the user exceeds 15 keys, even when it shouldn't. The maximum number of keys allowed for segwit redeem scripts (p2sh-segwit and bech32) is 20 keys. This is because the redeem script placed in the witness is not restricted by the item size limit. The reason behind this issue is the utilization of the legacy p2sh redeem script restrictions on segwit ones. Redeem scripts longer than 520 bytes are blocked from being inserted into the keystore, which causes the signing process and the descriptor inference process to fail. This occurs because the multisig generation flow uses the same keystore as the legacy spkm (FillableSigningProvider), which contains the 520-byte limit. --- src/outputtype.cpp | 8 ++++---- src/outputtype.h | 2 +- src/rpc/output_script.cpp | 3 +-- src/rpc/util.cpp | 2 +- src/rpc/util.h | 2 +- src/wallet/rpc/addresses.cpp | 12 ++++++++++-- 6 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/outputtype.cpp b/src/outputtype.cpp index c72d9deacb0df..8c2b76494bd87 100644 --- a/src/outputtype.cpp +++ b/src/outputtype.cpp @@ -81,11 +81,11 @@ std::vector GetAllDestinationsForKey(const CPubKey& key) } } -CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, const CScript& script, OutputType type) +CTxDestination AddAndGetDestinationForScript(FlatSigningProvider& keystore, const CScript& script, OutputType type) { // Add script to keystore - keystore.AddCScript(script); - // Note that scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are not yet supported. + keystore.scripts.emplace(CScriptID(script), script); + switch (type) { case OutputType::LEGACY: return ScriptHash(script); @@ -94,7 +94,7 @@ CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, CTxDestination witdest = WitnessV0ScriptHash(script); CScript witprog = GetScriptForDestination(witdest); // Add the redeemscript, so that P2WSH and P2SH-P2WSH outputs are recognized as ours. - keystore.AddCScript(witprog); + keystore.scripts.emplace(CScriptID(witprog), witprog); if (type == OutputType::BECH32) { return witdest; } else { diff --git a/src/outputtype.h b/src/outputtype.h index a2d5942320855..feef7991a60b2 100644 --- a/src/outputtype.h +++ b/src/outputtype.h @@ -46,7 +46,7 @@ std::vector GetAllDestinationsForKey(const CPubKey& key); * This function will automatically add the script (and any other * necessary scripts) to the keystore. */ -CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, const CScript& script, OutputType); +CTxDestination AddAndGetDestinationForScript(FlatSigningProvider& keystore, const CScript& script, OutputType); /** Get the OutputType for a CTxDestination */ std::optional OutputTypeFromDestination(const CTxDestination& dest); diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp index f9343f48a897e..91d4f283f0906 100644 --- a/src/rpc/output_script.cpp +++ b/src/rpc/output_script.cpp @@ -143,8 +143,7 @@ static RPCHelpMan createmultisig() output_type = parsed.value(); } - // Construct using pay-to-script-hash: - FillableSigningProvider keystore; + FlatSigningProvider keystore; CScript inner; const CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, keystore, inner); diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index f68387805494b..435801b45b99c 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -224,7 +224,7 @@ CPubKey AddrToPubKey(const FillableSigningProvider& keystore, const std::string& } // Creates a multisig address from a given list of public keys, number of signatures required, and the address type -CTxDestination AddAndGetMultisigDestination(const int required, const std::vector& pubkeys, OutputType type, FillableSigningProvider& keystore, CScript& script_out) +CTxDestination AddAndGetMultisigDestination(const int required, const std::vector& pubkeys, OutputType type, FlatSigningProvider& keystore, CScript& script_out) { // Gather public keys if (required < 1) { diff --git a/src/rpc/util.h b/src/rpc/util.h index 177af90c05ea2..6bfe41468878e 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -117,7 +117,7 @@ std::string HelpExampleRpcNamed(const std::string& methodname, const RPCArgList& CPubKey HexToPubKey(const std::string& hex_in); CPubKey AddrToPubKey(const FillableSigningProvider& keystore, const std::string& addr_in); -CTxDestination AddAndGetMultisigDestination(const int required, const std::vector& pubkeys, OutputType type, FillableSigningProvider& keystore, CScript& script_out); +CTxDestination AddAndGetMultisigDestination(const int required, const std::vector& pubkeys, OutputType type, FlatSigningProvider& keystore, CScript& script_out); UniValue DescribeAddress(const CTxDestination& dest); diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index bed9ec029aa04..bcc39b05b86c7 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -289,9 +289,17 @@ RPCHelpMan addmultisigaddress() output_type = parsed.value(); } - // Construct using pay-to-script-hash: + // Construct multisig scripts + FlatSigningProvider provider; CScript inner; - CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, spk_man, inner); + CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, provider, inner); + + // Import scripts into the wallet + for (const auto& [id, script] : provider.scripts) { + spk_man.AddCScript(script); + } + + // Store destination in the addressbook pwallet->SetAddressBook(dest, label, AddressPurpose::SEND); // Make the descriptor From 9d9a91c4ea6b3bb32ef4131bca86f1d6683fc901 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Aug 2023 17:50:09 -0300 Subject: [PATCH 014/116] rpc: bugfix, incorrect segwit redeem script size used in signrawtransactionwithkey The process currently fails to sign redeem scripts that are longer than 520 bytes. Even when it shouldn't. The 520 bytes redeem scripts limit is a legacy p2sh rule, and not a segwit limitation. Segwit redeem scripts are not restricted by the script item size limit. The reason why this occurs, is the usage of the same keystore used by the legacy spkm. Which contains blockage for any redeem scripts longer than the script item size limit. --- src/rpc/rawtransaction.cpp | 8 ++++++-- src/rpc/rawtransaction_util.cpp | 6 +++--- src/rpc/rawtransaction_util.h | 4 ++-- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 634be2f7fb9d6..ce71c052abceb 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -785,7 +785,7 @@ static RPCHelpMan signrawtransactionwithkey() throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); } - FillableSigningProvider keystore; + FlatSigningProvider keystore; const UniValue& keys = request.params[1].get_array(); for (unsigned int idx = 0; idx < keys.size(); ++idx) { UniValue k = keys[idx]; @@ -793,7 +793,11 @@ static RPCHelpMan signrawtransactionwithkey() if (!key.IsValid()) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key"); } - keystore.AddKey(key); + + CPubKey pubkey = key.GetPubKey(); + CKeyID key_id = pubkey.GetID(); + keystore.pubkeys.emplace(key_id, pubkey); + keystore.keys.emplace(key_id, key); } // Fetch previous transactions (inputs): diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp index a9e11622a7aea..a27e1be5446aa 100644 --- a/src/rpc/rawtransaction_util.cpp +++ b/src/rpc/rawtransaction_util.cpp @@ -181,7 +181,7 @@ static void TxInErrorToJSON(const CTxIn& txin, UniValue& vErrorsRet, const std:: vErrorsRet.push_back(entry); } -void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keystore, std::map& coins) +void ParsePrevouts(const UniValue& prevTxsUnival, FlatSigningProvider* keystore, std::map& coins) { if (!prevTxsUnival.isNull()) { const UniValue& prevTxs = prevTxsUnival.get_array(); @@ -247,11 +247,11 @@ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keyst // work from witnessScript when possible std::vector scriptData(!ws.isNull() ? ParseHexV(ws, "witnessScript") : ParseHexV(rs, "redeemScript")); CScript script(scriptData.begin(), scriptData.end()); - keystore->AddCScript(script); + keystore->scripts.emplace(CScriptID(script), script); // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH). // This is done for redeemScript only for compatibility, it is encouraged to use the explicit witnessScript field instead. CScript witness_output_script{GetScriptForDestination(WitnessV0ScriptHash(script))}; - keystore->AddCScript(witness_output_script); + keystore->scripts.emplace(CScriptID(witness_output_script), witness_output_script); if (!ws.isNull() && !rs.isNull()) { // if both witnessScript and redeemScript are provided, diff --git a/src/rpc/rawtransaction_util.h b/src/rpc/rawtransaction_util.h index 964d0b095b1c3..40d6bbba87372 100644 --- a/src/rpc/rawtransaction_util.h +++ b/src/rpc/rawtransaction_util.h @@ -12,7 +12,7 @@ #include struct bilingual_str; -class FillableSigningProvider; +struct FlatSigningProvider; class UniValue; struct CMutableTransaction; class Coin; @@ -38,7 +38,7 @@ void SignTransactionResultToJSON(CMutableTransaction& mtx, bool complete, const * @param keystore A pointer to the temporary keystore if there is one * @param coins Map of unspent outputs - coins in mempool and current chain UTXO set, may be extended by previous txns outputs after call */ -void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keystore, std::map& coins); +void ParsePrevouts(const UniValue& prevTxsUnival, FlatSigningProvider* keystore, std::map& coins); /** Normalize univalue-represented inputs and add them to the transaction */ void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, bool rbf); From 9be6065cc03f2408f290a332b203eef9c9cebf24 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Aug 2023 17:58:46 -0300 Subject: [PATCH 015/116] test: coverage for 16-20 segwit multisig scripts This exercises the bug fixed by previous commits, where we were unable to generate and sign for segwit redeem scripts (in this case multisig redeem scripts) longer than 520 bytes. and also, this adds coverage for legacy 15-15 multisig script generation and signing. --- test/functional/rpc_createmultisig.py | 31 +++++++++++++++++++++------ 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index 983aeafc1c5bc..e14dea70e616b 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -56,12 +56,13 @@ def run_test(self): self.generate(self.wallet, 149) wallet_multi = self.create_wallet(node1, 'wmulti') if self._requires_wallet else None - self.create_keys(5) - for nkeys in [3, 5]: - for nsigs in [2, 3]: - for output_type in ["bech32", "p2sh-segwit", "legacy"]: - self.do_multisig(nkeys, nsigs, output_type, wallet_multi) + self.create_keys(21) # max number of allowed keys + 1 + m_of_n = [(2, 3), (3, 3), (2, 5), (3, 5), (10, 15), (15, 15)] + for (sigs, keys) in m_of_n: + for output_type in ["bech32", "p2sh-segwit", "legacy"]: + self.do_multisig(keys, sigs, output_type, wallet_multi) + self.test_multisig_script_limit() self.test_mixing_uncompressed_and_compressed_keys(node0, wallet_multi) self.test_sortedmulti_descriptors_bip67() @@ -83,6 +84,21 @@ def check_addmultisigaddress_errors(self): pubs = [self.nodes[1].getaddressinfo(addr)["pubkey"] for addr in addresses] assert_raises_rpc_error(-5, "Bech32m multisig addresses cannot be created with legacy wallets", self.nodes[0].addmultisigaddress, 2, pubs, "", "bech32m") + def test_multisig_script_limit(self): + node1 = self.nodes[1] + pubkeys = self.pub[0:20] + + self.log.info('Test legacy redeem script max size limit') + assert_raises_rpc_error(-8, "redeemScript exceeds size limit: 684 > 520", node1.createmultisig, 16, pubkeys, 'legacy') + + self.log.info('Test valid 16-20 multisig p2sh-legacy and bech32 (no wallet)') + self.do_multisig(nkeys=20, nsigs=16, output_type="p2sh-segwit", wallet_multi=None) + self.do_multisig(nkeys=20, nsigs=16, output_type="bech32", wallet_multi=None) + + self.log.info('Test invalid 16-21 multisig p2sh-legacy and bech32 (no wallet)') + assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", node1.createmultisig, 16, self.pub, 'p2sh-segwit') + assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", node1.createmultisig, 16, self.pub, 'bech32') + def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): node0, node1, node2 = self.nodes pub_keys = self.pub[0: nkeys] @@ -117,13 +133,13 @@ def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): assert mredeemw == mredeem spk = address_to_scriptpubkey(madd) - value = decimal.Decimal("0.00001300") + value = decimal.Decimal("0.00004000") tx = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=spk, amount=int(value * COIN)) prevtxs = [{"txid": tx["txid"], "vout": tx["sent_vout"], "scriptPubKey": spk.hex(), "redeemScript": mredeem, "amount": value}] self.generate(node0, 1) - outval = value - decimal.Decimal("0.00001000") + outval = value - decimal.Decimal("0.00002000") # deduce fee (must be higher than the min relay fee) # send coins to node2 when wallet is enabled node2_balance = node2.getbalances()['mine']['trusted'] if self.is_wallet_compiled() else 0 out_addr = node2.getnewaddress() if self.is_wallet_compiled() else getnewdestination('bech32')[2] @@ -157,6 +173,7 @@ def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): rawtx2 = node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs - 1], prevtxs) rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [priv_keys[-1]], prevtxs) + assert rawtx3['complete'] tx = node0.sendrawtransaction(rawtx3["hex"], 0) blk = self.generate(node0, 1)[0] From 53302a09817e5b799d345dfea432546a55a9d727 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Aug 2023 18:16:41 -0300 Subject: [PATCH 016/116] bugfix: addmultisigaddress, add unsupported operation for redeem scripts over 520 bytes Due to a bug in the legacy wallet, the p2sh maximum script size limit is also imposed on 'p2sh-segwit' and 'bech32' redeem scripts. Although redeem scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are technically valid for segwit output types, we don't want to enable this feature in legacy wallets for the following reasons: 1) It introduces a compatibility-breaking change requiring downgrade protection; older wallets would be unable to interact with these "new" legacy wallets. 2) Considering the ongoing deprecation of the legacy spkm, this issue adds another good reason to transition towards descriptors. --- src/wallet/rpc/addresses.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index bcc39b05b86c7..62188249dae85 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -296,7 +296,20 @@ RPCHelpMan addmultisigaddress() // Import scripts into the wallet for (const auto& [id, script] : provider.scripts) { - spk_man.AddCScript(script); + // Due to a bug in the legacy wallet, the p2sh maximum script size limit is also imposed on 'p2sh-segwit' and 'bech32' redeem scripts. + // Even when redeem scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are valid for segwit output types, we don't want to + // enable it because: + // 1) It introduces a compatibility-breaking change requiring downgrade protection; older wallets would be unable to interact with these "new" legacy wallets. + // 2) Considering the ongoing deprecation of the legacy spkm, this issue adds another good reason to transition towards descriptors. + if (script.size() > MAX_SCRIPT_ELEMENT_SIZE) throw JSONRPCError(RPC_WALLET_ERROR, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts"); + + if (!spk_man.AddCScript(script)) { + if (CScript inner_script; spk_man.GetCScript(CScriptID(script), inner_script)) { + CHECK_NONFATAL(inner_script == script); // Nothing to add, script already contained by the wallet + continue; + } + throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Error importing script into the wallet")); + } } // Store destination in the addressbook From 2451a217dd2c21b6d2f2b2699ceddd0bf9073019 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Aug 2023 18:18:00 -0300 Subject: [PATCH 017/116] test: addmultisigaddress, coverage for script size limits --- test/functional/rpc_createmultisig.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index e14dea70e616b..fdac3623d3c0d 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -62,7 +62,7 @@ def run_test(self): for output_type in ["bech32", "p2sh-segwit", "legacy"]: self.do_multisig(keys, sigs, output_type, wallet_multi) - self.test_multisig_script_limit() + self.test_multisig_script_limit(wallet_multi) self.test_mixing_uncompressed_and_compressed_keys(node0, wallet_multi) self.test_sortedmulti_descriptors_bip67() @@ -84,7 +84,7 @@ def check_addmultisigaddress_errors(self): pubs = [self.nodes[1].getaddressinfo(addr)["pubkey"] for addr in addresses] assert_raises_rpc_error(-5, "Bech32m multisig addresses cannot be created with legacy wallets", self.nodes[0].addmultisigaddress, 2, pubs, "", "bech32m") - def test_multisig_script_limit(self): + def test_multisig_script_limit(self, wallet_multi): node1 = self.nodes[1] pubkeys = self.pub[0:20] @@ -99,6 +99,17 @@ def test_multisig_script_limit(self): assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", node1.createmultisig, 16, self.pub, 'p2sh-segwit') assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", node1.createmultisig, 16, self.pub, 'bech32') + # Check legacy wallet related command + self.log.info('Test legacy redeem script max size limit (with wallet)') + if wallet_multi is not None and not self.options.descriptors: + assert_raises_rpc_error(-8, "redeemScript exceeds size limit: 684 > 520", wallet_multi.addmultisigaddress, 16, pubkeys, '', 'legacy') + + self.log.info('Test legacy wallet unsupported operation. 16-20 multisig p2sh-legacy and bech32 generation') + # Due an internal limitation on legacy wallets, the redeem script limit also applies to p2sh-segwit and bech32 (even when the scripts are valid) + # We take this as a "good thing" to tell users to upgrade to descriptors. + assert_raises_rpc_error(-4, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts", wallet_multi.addmultisigaddress, 16, pubkeys, '', 'p2sh-segwit') + assert_raises_rpc_error(-4, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts", wallet_multi.addmultisigaddress, 16, pubkeys, '', 'bech32') + def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): node0, node1, node2 = self.nodes pub_keys = self.pub[0: nkeys] From faf3cd659a72473a1aa73c4367a145f4ec64f146 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 7 Feb 2024 11:30:21 +0100 Subject: [PATCH 018/116] test: Normalize struct.pack format * Add () around some int values * Remove b-prefix from strings This is needed for the scripted diff to work. --- test/functional/feature_addrman.py | 10 +++++----- test/functional/test_framework/script.py | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py index 95d33d62ead30..c17b697ec1eed 100755 --- a/test/functional/feature_addrman.py +++ b/test/functional/feature_addrman.py @@ -29,14 +29,14 @@ def serialize_addrman( INCOMPATIBILITY_BASE = 32 r = MAGIC_BYTES[net_magic] r += struct.pack("B", format) - r += struct.pack("B", INCOMPATIBILITY_BASE + lowest_compatible) + r += struct.pack("B", (INCOMPATIBILITY_BASE + lowest_compatible)) r += ser_uint256(bucket_key) - r += struct.pack(" Date: Wed, 7 Feb 2024 11:20:41 +0100 Subject: [PATCH 019/116] test: Use int.to_bytes over struct packing This is done in prepration for the scripted diff, which can not deal with those lines. --- test/functional/p2p_invalid_messages.py | 3 +-- test/functional/test_framework/script.py | 4 ++-- test/functional/wallet_balance.py | 5 ++--- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 40a69936bccca..adcbb4fd051f5 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -5,7 +5,6 @@ """Test node responses to invalid network messages.""" import random -import struct import time from test_framework.messages import ( @@ -233,7 +232,7 @@ def test_addrv2_too_long_address(self): '208d')) # port def test_addrv2_unrecognized_network(self): - now_hex = struct.pack(' Date: Wed, 7 Feb 2024 11:26:05 +0100 Subject: [PATCH 020/116] scripted-diff: test: Use int.to_bytes over struct packing -BEGIN VERIFY SCRIPT- sed -i --regexp-extended 's!struct.pack\(.H., (.*)\)!\1.to_bytes(2, "big")!g' $( git grep -l struct.pack ) -END VERIFY SCRIPT- --- contrib/seeds/generate-seeds.py | 6 ++--- contrib/signet/miner | 4 ++-- test/functional/feature_addrman.py | 12 +++++----- test/functional/p2p_segwit.py | 12 +++++----- test/functional/test_framework/p2p.py | 2 +- test/functional/test_framework/script.py | 28 ++++++++++++------------ 6 files changed, 32 insertions(+), 32 deletions(-) diff --git a/contrib/seeds/generate-seeds.py b/contrib/seeds/generate-seeds.py index e921757802adb..c84be1055da85 100755 --- a/contrib/seeds/generate-seeds.py +++ b/contrib/seeds/generate-seeds.py @@ -115,7 +115,7 @@ def parse_spec(s): def ser_compact_size(l): r = b"" if l < 253: - r = struct.pack("B", l) + r = l.to_bytes(1, "little") elif l < 0x10000: r = struct.pack("H', spec[2]) + r += spec[2].to_bytes(2, "big") return r def process_nodes(g, f, structname): diff --git a/contrib/signet/miner b/contrib/signet/miner index e5daf9f993eaa..7b7b3feb39b54 100755 --- a/contrib/signet/miner +++ b/contrib/signet/miner @@ -52,10 +52,10 @@ def signet_txs(block, challenge): mroot = block.get_merkle_root(hashes) sd = b"" - sd += struct.pack(" Date: Wed, 7 Feb 2024 13:19:37 +0100 Subject: [PATCH 021/116] test: Remove struct.pack from almost all places --- contrib/seeds/generate-seeds.py | 7 +++---- test/functional/feature_addrman.py | 1 - test/functional/feature_block.py | 3 +-- test/functional/p2p_segwit.py | 1 - test/functional/test_framework/script.py | 1 - 5 files changed, 4 insertions(+), 9 deletions(-) diff --git a/contrib/seeds/generate-seeds.py b/contrib/seeds/generate-seeds.py index c84be1055da85..f67e7b0f4ccec 100755 --- a/contrib/seeds/generate-seeds.py +++ b/contrib/seeds/generate-seeds.py @@ -29,7 +29,6 @@ from base64 import b32decode from enum import Enum -import struct import sys import os import re @@ -117,11 +116,11 @@ def ser_compact_size(l): if l < 253: r = l.to_bytes(1, "little") elif l < 0x10000: - r = struct.pack(" Date: Thu, 28 Dec 2023 16:50:13 -0500 Subject: [PATCH 022/116] test: test sendall does ancestor aware funding --- test/functional/wallet_sendall.py | 40 +++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/test/functional/wallet_sendall.py b/test/functional/wallet_sendall.py index 70b52641c352e..1d308c225d90b 100755 --- a/test/functional/wallet_sendall.py +++ b/test/functional/wallet_sendall.py @@ -400,6 +400,43 @@ def sendall_spends_unconfirmed_inputs_if_specified(self): self.wallet.sendall(recipients=[self.remainder_target], inputs=[unspent]) assert_equal(self.wallet.getbalance(), 0) + @cleanup + def sendall_does_ancestor_aware_funding(self): + self.log.info("Test that sendall does ancestor aware funding for unconfirmed inputs") + + # higher parent feerate + self.def_wallet.sendtoaddress(address=self.wallet.getnewaddress(), amount=17, fee_rate=20) + self.wallet.syncwithvalidationinterfacequeue() + + assert_equal(self.wallet.getbalances()["mine"]["untrusted_pending"], 17) + unspent = self.wallet.listunspent(minconf=0)[0] + + parent_txid = unspent["txid"] + assert_equal(self.wallet.gettransaction(parent_txid)["confirmations"], 0) + + res_1 = self.wallet.sendall(recipients=[self.def_wallet.getnewaddress()], inputs=[unspent], fee_rate=20, add_to_wallet=False, lock_unspents=True) + child_hex = res_1["hex"] + + child_tx = self.wallet.decoderawtransaction(child_hex) + higher_parent_feerate_amount = child_tx["vout"][0]["value"] + + # lower parent feerate + self.def_wallet.sendtoaddress(address=self.wallet.getnewaddress(), amount=17, fee_rate=10) + self.wallet.syncwithvalidationinterfacequeue() + assert_equal(self.wallet.getbalances()["mine"]["untrusted_pending"], 34) + unspent = self.wallet.listunspent(minconf=0)[0] + + parent_txid = unspent["txid"] + assert_equal(self.wallet.gettransaction(parent_txid)["confirmations"], 0) + + res_2 = self.wallet.sendall(recipients=[self.def_wallet.getnewaddress()], inputs=[unspent], fee_rate=20, add_to_wallet=False, lock_unspents=True) + child_hex = res_2["hex"] + + child_tx = self.wallet.decoderawtransaction(child_hex) + lower_parent_feerate_amount = child_tx["vout"][0]["value"] + + assert_greater_than(higher_parent_feerate_amount, lower_parent_feerate_amount) + # This tests needs to be the last one otherwise @cleanup will fail with "Transaction too large" error def sendall_fails_with_transaction_too_large(self): self.log.info("Test that sendall fails if resulting transaction is too large") @@ -487,6 +524,9 @@ def run_test(self): # Sendall spends unconfirmed inputs if they are specified self.sendall_spends_unconfirmed_inputs_if_specified() + # Sendall does ancestor aware funding when spending an unconfirmed UTXO + self.sendall_does_ancestor_aware_funding() + # Sendall fails when many inputs result to too large transaction self.sendall_fails_with_transaction_too_large() From 5676aec1e1a6d2c6fd3099e120e263a0a7def089 Mon Sep 17 00:00:00 2001 From: josibake Date: Tue, 18 Jul 2023 22:22:26 +0200 Subject: [PATCH 023/116] refactor: Model the bech32 charlimit as an Enum Bech32(m) was defined with a 90 character limit so that certain guarantees for error detection could be made for segwit addresses. However, there is nothing about the encoding scheme itself that requires a limit and in practice bech32(m) has been used without the 90 char limit (e.g. lightning invoices). Further, increasing the character limit doesn't do away with error detection, it simply lessons the guarantees. Model charlimit as an Enum, so that if a different address scheme is using bech32(m), the character limit for that address scheme can be used, rather than always using the 90 charlimit defined for segwit addresses. upate comment --- src/bech32.cpp | 13 +++++++------ src/bech32.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/bech32.cpp b/src/bech32.cpp index ba3c419d8b686..6a0956f1b6839 100644 --- a/src/bech32.cpp +++ b/src/bech32.cpp @@ -370,11 +370,12 @@ std::string Encode(Encoding encoding, const std::string& hrp, const data& values } /** Decode a Bech32 or Bech32m string. */ -DecodeResult Decode(const std::string& str) { +DecodeResult Decode(const std::string& str, CharLimit limit) { std::vector errors; if (!CheckCharacters(str, errors)) return {}; size_t pos = str.rfind('1'); - if (str.size() > 90 || pos == str.npos || pos == 0 || pos + 7 > str.size()) { + if (str.size() > limit) return {}; + if (pos == str.npos || pos == 0 || pos + 7 > str.size()) { return {}; } data values(str.size() - 1 - pos); @@ -397,12 +398,12 @@ DecodeResult Decode(const std::string& str) { } /** Find index of an incorrect character in a Bech32 string. */ -std::pair> LocateErrors(const std::string& str) { +std::pair> LocateErrors(const std::string& str, CharLimit limit) { std::vector error_locations{}; - if (str.size() > 90) { - error_locations.resize(str.size() - 90); - std::iota(error_locations.begin(), error_locations.end(), 90); + if (str.size() > limit) { + error_locations.resize(str.size() - limit); + std::iota(error_locations.begin(), error_locations.end(), static_cast(limit)); return std::make_pair("Bech32 string too long", std::move(error_locations)); } diff --git a/src/bech32.h b/src/bech32.h index 5e89e6efdaa94..fe2a276ae078f 100644 --- a/src/bech32.h +++ b/src/bech32.h @@ -28,6 +28,14 @@ enum class Encoding { BECH32M, //!< Bech32m encoding as defined in BIP350 }; +/** Character limits for Bech32(m) encoded strings. Character limits are how we provide error location guarantees. + * These values should never exceed 2^31 - 1 (max value for a 32-bit int), since there are places where we may need to + * convert the CharLimit::VALUE to an int. In practice, this should never happen since this CharLimit applies to an address encoding + * and we would never encode an address with such a massive value */ +enum CharLimit : size_t { + BECH32 = 90, //!< BIP173/350 imposed character limit for Bech32(m) encoded addresses. This guarantees finding up to 4 errors. +}; + /** Encode a Bech32 or Bech32m string. If hrp contains uppercase characters, this will cause an * assertion error. Encoding must be one of BECH32 or BECH32M. */ std::string Encode(Encoding encoding, const std::string& hrp, const std::vector& values); @@ -43,10 +51,10 @@ struct DecodeResult }; /** Decode a Bech32 or Bech32m string. */ -DecodeResult Decode(const std::string& str); +DecodeResult Decode(const std::string& str, CharLimit limit = CharLimit::BECH32); /** Return the positions of errors in a Bech32 string. */ -std::pair> LocateErrors(const std::string& str); +std::pair> LocateErrors(const std::string& str, CharLimit limit = CharLimit::BECH32); } // namespace bech32 From e208fb5d3bea4c1fb750cb0028819635ecdeb415 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Sat, 27 Apr 2024 18:09:12 -0400 Subject: [PATCH 024/116] cli: Sanitize ports in rpcconnect and rpcport Adds error handling of invalid ports to rpcconnect and rpcport, with associated functional tests. --- src/bitcoin-cli.cpp | 31 ++++++++++++++- test/functional/interface_bitcoin_cli.py | 49 ++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index c7ba2204c3fda..eb1ee6134a935 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -743,8 +743,35 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co // 2. port in -rpcconnect (ie following : in ipv4 or ]: in ipv6) // 3. default port for chain uint16_t port{BaseParams().RPCPort()}; - SplitHostPort(gArgs.GetArg("-rpcconnect", DEFAULT_RPCCONNECT), port, host); - port = static_cast(gArgs.GetIntArg("-rpcport", port)); + { + uint16_t rpcconnect_port{0}; + const std::string rpcconnect_str = gArgs.GetArg("-rpcconnect", DEFAULT_RPCCONNECT); + if (!SplitHostPort(rpcconnect_str, rpcconnect_port, host)) { + // Uses argument provided as-is + // (rather than value parsed) + // to aid the user in troubleshooting + throw std::runtime_error(strprintf("Invalid port provided in -rpcconnect: %s", rpcconnect_str)); + } else { + if (rpcconnect_port != 0) { + // Use the valid port provided in rpcconnect + port = rpcconnect_port; + } // else, no port was provided in rpcconnect (continue using default one) + } + + if (std::optional rpcport_arg = gArgs.GetArg("-rpcport")) { + // -rpcport was specified + const uint16_t rpcport_int{ToIntegral(rpcport_arg.value()).value_or(0)}; + if (rpcport_int == 0) { + // Uses argument provided as-is + // (rather than value parsed) + // to aid the user in troubleshooting + throw std::runtime_error(strprintf("Invalid port provided in -rpcport: %s", rpcport_arg.value())); + } + + // Use the valid port provided + port = rpcport_int; + } + } // Obtain event base raii_event_base base = obtain_event_base(); diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py index 83bb5121e5b4a..a6628dcbf3f88 100755 --- a/test/functional/interface_bitcoin_cli.py +++ b/test/functional/interface_bitcoin_cli.py @@ -8,6 +8,7 @@ import re from test_framework.blocktools import COINBASE_MATURITY +from test_framework.netutil import test_ipv6_local from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, @@ -15,6 +16,7 @@ assert_raises_process_error, assert_raises_rpc_error, get_auth_cookie, + rpc_port, ) import time @@ -107,6 +109,53 @@ def run_test(self): self.log.info("Test connecting to a non-existing server") assert_raises_process_error(1, "Could not connect to the server", self.nodes[0].cli('-rpcport=1').echo) + self.log.info("Test handling of invalid ports in rpcconnect") + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:notaport", self.nodes[0].cli("-rpcconnect=127.0.0.1:notaport").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:-1", self.nodes[0].cli("-rpcconnect=127.0.0.1:-1").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:0", self.nodes[0].cli("-rpcconnect=127.0.0.1:0").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:65536", self.nodes[0].cli("-rpcconnect=127.0.0.1:65536").echo) + + self.log.info("Checking for IPv6") + have_ipv6 = test_ipv6_local() + if not have_ipv6: + self.log.info("Skipping IPv6 tests") + + if have_ipv6: + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: [::1]:notaport", self.nodes[0].cli("-rpcconnect=[::1]:notaport").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: [::1]:-1", self.nodes[0].cli("-rpcconnect=[::1]:-1").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: [::1]:0", self.nodes[0].cli("-rpcconnect=[::1]:0").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcconnect: [::1]:65536", self.nodes[0].cli("-rpcconnect=[::1]:65536").echo) + + self.log.info("Test handling of invalid ports in rpcport") + assert_raises_process_error(1, "Invalid port provided in -rpcport: notaport", self.nodes[0].cli("-rpcport=notaport").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcport: -1", self.nodes[0].cli("-rpcport=-1").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcport: 0", self.nodes[0].cli("-rpcport=0").echo) + assert_raises_process_error(1, "Invalid port provided in -rpcport: 65536", self.nodes[0].cli("-rpcport=65536").echo) + + self.log.info("Test port usage preferences") + node_rpc_port = rpc_port(self.nodes[0].index) + # Prevent bitcoin-cli from using existing rpcport in conf + conf_rpcport = "rpcport=" + str(node_rpc_port) + self.nodes[0].replace_in_config([(conf_rpcport, "#" + conf_rpcport)]) + # prefer rpcport over rpcconnect + assert_raises_process_error(1, "Could not connect to the server 127.0.0.1:1", self.nodes[0].cli(f"-rpcconnect=127.0.0.1:{node_rpc_port}", "-rpcport=1").echo) + if have_ipv6: + assert_raises_process_error(1, "Could not connect to the server ::1:1", self.nodes[0].cli(f"-rpcconnect=[::1]:{node_rpc_port}", "-rpcport=1").echo) + + assert_equal(BLOCKS, self.nodes[0].cli("-rpcconnect=127.0.0.1:18999", f'-rpcport={node_rpc_port}').getblockcount()) + if have_ipv6: + assert_equal(BLOCKS, self.nodes[0].cli("-rpcconnect=[::1]:18999", f'-rpcport={node_rpc_port}').getblockcount()) + + # prefer rpcconnect port over default + assert_equal(BLOCKS, self.nodes[0].cli(f"-rpcconnect=127.0.0.1:{node_rpc_port}").getblockcount()) + if have_ipv6: + assert_equal(BLOCKS, self.nodes[0].cli(f"-rpcconnect=[::1]:{node_rpc_port}").getblockcount()) + + # prefer rpcport over default + assert_equal(BLOCKS, self.nodes[0].cli(f'-rpcport={node_rpc_port}').getblockcount()) + # Re-enable rpcport in conf if present + self.nodes[0].replace_in_config([("#" + conf_rpcport, conf_rpcport)]) + self.log.info("Test connecting with non-existing RPC cookie file") assert_raises_process_error(1, "Could not locate RPC credentials", self.nodes[0].cli('-rpccookiefile=does-not-exist', '-rpcpassword=').echo) From 24bc46c83b39149f4845a575a82337eb46d91bdb Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Sat, 27 Apr 2024 18:10:57 -0400 Subject: [PATCH 025/116] cli: Add warning for duplicate port definition Adds a warning when both -rpcconnect and -rpcport define a port to be used. --- src/bitcoin-cli.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index eb1ee6134a935..6352044ba7340 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -770,6 +770,12 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co // Use the valid port provided port = rpcport_int; + + // If there was a valid port provided in rpcconnect, + // rpcconnect_port is non-zero. + if (rpcconnect_port != 0) { + tfm::format(std::cerr, "Warning: Port specified in both -rpcconnect and -rpcport. Using -rpcport %u\n", port); + } } } From fa3169b0732d7eb4b9166e7ecc6b7cfb669a9b54 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 29 Apr 2024 17:39:07 +0200 Subject: [PATCH 026/116] rpc: Remove index-based Arg accessor --- src/rpc/mempool.cpp | 6 +++--- src/rpc/mining.cpp | 2 +- src/rpc/net.cpp | 2 +- src/rpc/util.cpp | 2 +- src/rpc/util.h | 36 ++++++++++-------------------------- src/test/rpc_tests.cpp | 34 ++++++++++------------------------ src/wallet/rpc/wallet.cpp | 4 ++-- 7 files changed, 28 insertions(+), 58 deletions(-) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index e599c7dc92553..dc2755766d0fe 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -82,7 +82,7 @@ static RPCHelpMan sendrawtransaction() CTransactionRef tx(MakeTransactionRef(std::move(mtx))); - const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg(1))}; + const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg("maxfeerate"))}; int64_t virtual_size = GetVirtualTransactionSize(*tx); CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size); @@ -162,7 +162,7 @@ static RPCHelpMan testmempoolaccept() "Array must contain between 1 and " + ToString(MAX_PACKAGE_COUNT) + " transactions."); } - const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg(1))}; + const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg("maxfeerate"))}; std::vector txns; txns.reserve(raw_transactions.size()); @@ -873,7 +873,7 @@ static RPCHelpMan submitpackage() } // Fee check needs to be run with chainstate and package context - const CFeeRate max_raw_tx_fee_rate = ParseFeeRate(self.Arg(1)); + const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg("maxfeerate"))}; std::optional client_maxfeerate{max_raw_tx_fee_rate}; // 0-value is special; it's mapped to no sanity check if (max_raw_tx_fee_rate == CFeeRate(0)) { diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 2391392bd752c..88e731d92cff5 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -485,7 +485,7 @@ static RPCHelpMan prioritisetransaction() LOCK(cs_main); uint256 hash(ParseHashV(request.params[0], "txid")); - const auto dummy{self.MaybeArg(1)}; + const auto dummy{self.MaybeArg("dummy")}; CAmount nAmount = request.params[2].getInt(); if (dummy && *dummy != 0) { diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 59397aa84dd0c..636129c9800d6 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -401,7 +401,7 @@ static RPCHelpMan addconnection() } else { throw JSONRPCError(RPC_INVALID_PARAMETER, self.ToString()); } - bool use_v2transport = self.Arg(2); + bool use_v2transport{self.Arg("v2transport")}; NodeContext& node = EnsureAnyNodeContext(request.context); CConnman& connman = EnsureConnman(node); diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 9a7c731afe903..7bf78f150a466 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -677,7 +677,7 @@ static const UniValue* DetailMaybeArg(CheckFn* check, const std::vector& static void CheckRequiredOrDefault(const RPCArg& param) { - // Must use `Arg(i)` to get the argument or its default value. + // Must use `Arg(key)` to get the argument or its default value. const bool required{ std::holds_alternative(param.m_fallback) && RPCArg::Optional::NO == std::get(param.m_fallback), }; diff --git a/src/rpc/util.h b/src/rpc/util.h index 0e4dcc27b5e97..394a429a29a1b 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -414,19 +414,16 @@ class RPCHelpMan * argument isNull() and parses (from JSON) and returns the user-passed argument, * or the default value derived from the RPCArg documentation. * - * There are two overloads of this function: - * - Use Arg(size_t i) to get the argument (or the default value) by index. - * - Use Arg(const std::string& key) to get the argument (or the default value) by key. + * The instantiation of this helper for type R must match the corresponding RPCArg::Type. * - * The Type passed to this helper must match the corresponding RPCArg::Type. - * - * @return The value of the RPC argument (or the default value) cast to type Type. + * @return The value of the RPC argument (or the default value) cast to type R. * * @see MaybeArg for handling optional arguments without default values. */ template - auto Arg(size_t i) const + auto Arg(std::string_view key) const { + auto i{GetParamIndex(key)}; // Return argument (required or with default value). if constexpr (std::is_integral_v || std::is_floating_point_v) { // Return numbers by value. @@ -436,11 +433,6 @@ class RPCHelpMan return ArgValue(i); } } - template - auto Arg(std::string_view key) const - { - return Arg(GetParamIndex(key)); - } /** * @brief Helper to get an optional request argument. * @@ -452,21 +444,18 @@ class RPCHelpMan * argument isNull() and parses (from JSON) and returns the user-passed argument, * or a falsy value if no argument was passed. * - * There are two overloads of this function: - * - Use MaybeArg(size_t i) to get the optional argument by index. - * - Use MaybeArg(const std::string& key) to get the optional argument by key. + * The instantiation of this helper for type R must match the corresponding RPCArg::Type. * - * The Type passed to this helper must match the corresponding RPCArg::Type. + * @return For integral and floating-point types, a std::optional is returned. + * For other types, a R* pointer to the argument is returned. If the + * argument is not provided, std::nullopt or a null pointer is returned. * - * @return For integral and floating-point types, a std::optional is returned. - * For other types, a Type* pointer to the argument is returned. If the - * argument is not provided, std::nullopt or a null pointer is returned. - * * @see Arg for handling arguments that are required or have a default value. */ template - auto MaybeArg(size_t i) const + auto MaybeArg(std::string_view key) const { + auto i{GetParamIndex(key)}; // Return optional argument (without default). if constexpr (std::is_integral_v || std::is_floating_point_v) { // Return numbers by value, wrapped in optional. @@ -476,11 +465,6 @@ class RPCHelpMan return ArgValue(i); } } - template - auto MaybeArg(std::string_view key) const - { - return MaybeArg(GetParamIndex(key)); - } std::string ToString() const; /** Return the named args that need to be converted from string to another JSON type */ UniValue GetArgMap() const; diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index acacb6257d549..e64e50bb9d925 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -614,40 +614,26 @@ BOOST_AUTO_TEST_CASE(rpc_arg_helper) //! Check that `self.Arg` returns the same value as the `request.params` accessors RPCHelpMan::RPCMethodImpl check_positional = [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - BOOST_CHECK_EQUAL(self.Arg(0), request.params[0].getInt()); - BOOST_CHECK_EQUAL(self.Arg(1), request.params[1].get_str()); - BOOST_CHECK_EQUAL(self.Arg(2), request.params[2].isNull() ? DEFAULT_UINT64_T : request.params[2].getInt()); - BOOST_CHECK_EQUAL(self.Arg(3), request.params[3].isNull() ? DEFAULT_STRING : request.params[3].get_str()); - BOOST_CHECK_EQUAL(self.Arg(4), request.params[4].isNull() ? DEFAULT_BOOL : request.params[4].get_bool()); + BOOST_CHECK_EQUAL(self.Arg("req_int"), request.params[0].getInt()); + BOOST_CHECK_EQUAL(self.Arg("req_str"), request.params[1].get_str()); + BOOST_CHECK_EQUAL(self.Arg("def_uint64_t"), request.params[2].isNull() ? DEFAULT_UINT64_T : request.params[2].getInt()); + BOOST_CHECK_EQUAL(self.Arg("def_string"), request.params[3].isNull() ? DEFAULT_STRING : request.params[3].get_str()); + BOOST_CHECK_EQUAL(self.Arg("def_bool"), request.params[4].isNull() ? DEFAULT_BOOL : request.params[4].get_bool()); if (!request.params[5].isNull()) { - BOOST_CHECK_EQUAL(self.MaybeArg(5).value(), request.params[5].get_real()); + BOOST_CHECK_EQUAL(self.MaybeArg("opt_double").value(), request.params[5].get_real()); } else { - BOOST_CHECK(!self.MaybeArg(5)); + BOOST_CHECK(!self.MaybeArg("opt_double")); } if (!request.params[6].isNull()) { - BOOST_CHECK(self.MaybeArg(6)); - BOOST_CHECK_EQUAL(*self.MaybeArg(6), request.params[6].get_str()); + BOOST_CHECK(self.MaybeArg("opt_string")); + BOOST_CHECK_EQUAL(*self.MaybeArg("opt_string"), request.params[6].get_str()); } else { - BOOST_CHECK(!self.MaybeArg(6)); + BOOST_CHECK(!self.MaybeArg("opt_string")); } return UniValue{}; }; CheckRpc(params, UniValue{JSON(R"([5, "hello", null, null, null, null, null])")}, check_positional); CheckRpc(params, UniValue{JSON(R"([5, "hello", 4, "test", true, 1.23, "world"])")}, check_positional); - - //! Check that `self.Arg` returns the same value when using index and key - RPCHelpMan::RPCMethodImpl check_named = [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - BOOST_CHECK_EQUAL(self.Arg(0), self.Arg("req_int")); - BOOST_CHECK_EQUAL(self.Arg(1), self.Arg("req_str")); - BOOST_CHECK_EQUAL(self.Arg(2), self.Arg("def_uint64_t")); - BOOST_CHECK_EQUAL(self.Arg(3), self.Arg("def_string")); - BOOST_CHECK_EQUAL(self.Arg(4), self.Arg("def_bool")); - BOOST_CHECK(self.MaybeArg(5) == self.MaybeArg("opt_double")); - BOOST_CHECK(self.MaybeArg(6) == self.MaybeArg("opt_string")); - return UniValue{}; - }; - CheckRpc(params, UniValue{JSON(R"([5, "hello", null, null, null, null, null])")}, check_named); - CheckRpc(params, UniValue{JSON(R"([5, "hello", 4, "test", true, 1.23, "world"])")}, check_named); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index f1cb595271e5e..2c64d09808469 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -395,7 +395,7 @@ static RPCHelpMan createwallet() if (!request.params[4].isNull() && request.params[4].get_bool()) { flags |= WALLET_FLAG_AVOID_REUSE; } - if (self.Arg(5)) { + if (self.Arg("descriptors")) { #ifndef USE_SQLITE throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without sqlite support (required for descriptor wallets)"); #endif @@ -489,7 +489,7 @@ static RPCHelpMan unloadwallet() // Release the "main" shared pointer and prevent further notifications. // Note that any attempt to load the same wallet would fail until the wallet // is destroyed (see CheckUniqueFileid). - std::optional load_on_start{self.MaybeArg(1)}; + std::optional load_on_start{self.MaybeArg("load_on_startup")}; if (!RemoveWallet(context, wallet, load_on_start, warnings)) { throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded"); } From ffa27af24da81a97d6c4912ae0e10bc5b6f17f69 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 10 May 2024 16:06:03 -0400 Subject: [PATCH 027/116] test: Add check-deps.sh script to check for unexpected library dependencies --- contrib/devtools/check-deps.sh | 203 +++++++++++++++++++++++++++++++++ 1 file changed, 203 insertions(+) create mode 100755 contrib/devtools/check-deps.sh diff --git a/contrib/devtools/check-deps.sh b/contrib/devtools/check-deps.sh new file mode 100755 index 0000000000000..9d2eebe14d8de --- /dev/null +++ b/contrib/devtools/check-deps.sh @@ -0,0 +1,203 @@ +#!/usr/bin/env bash + +export LC_ALL=C +set -Eeuo pipefail + +# Declare paths to libraries +declare -A LIBS +LIBS[cli]="libbitcoin_cli.a" +LIBS[common]="libbitcoin_common.a" +LIBS[consensus]="libbitcoin_consensus.a" +LIBS[crypto]="crypto/.libs/libbitcoin_crypto_base.a crypto/.libs/libbitcoin_crypto_x86_shani.a crypto/.libs/libbitcoin_crypto_sse41.a crypto/.libs/libbitcoin_crypto_avx2.a" +LIBS[node]="libbitcoin_node.a" +LIBS[util]="libbitcoin_util.a" +LIBS[wallet]="libbitcoin_wallet.a" +LIBS[wallet_tool]="libbitcoin_wallet_tool.a" + +# Declare allowed dependencies "X Y" where X is allowed to depend on Y. This +# list is taken from doc/design/libraries.md. +ALLOWED_DEPENDENCIES=( + "cli common" + "cli util" + "common consensus" + "common crypto" + "common util" + "consensus crypto" + "node common" + "node consensus" + "node crypto" + "node kernel" + "node util" + "util crypto" + "wallet common" + "wallet crypto" + "wallet util" + "wallet_tool util" + "wallet_tool wallet" +) + +# Add minor dependencies omitted from doc/design/libraries.md to keep the +# dependency diagram simple. +ALLOWED_DEPENDENCIES+=( + "wallet consensus" + "wallet_tool common" + "wallet_tool crypto" +) + +# Declare list of known errors that should be suppressed. +declare -A SUPPRESS +# init.cpp file currently calls Berkeley DB sanity check function on startup, so +# there is an undocumented dependency of the node library on the wallet library. +SUPPRESS["libbitcoin_node_a-init.o libbitcoin_wallet_a-bdb.o _ZN6wallet27BerkeleyDatabaseSanityCheckEv"]=1 +# init/common.cpp file calls InitError and InitWarning from interface_ui which +# is currently part of the node library. interface_ui should just be part of the +# common library instead, and is moved in +# https://github.com/bitcoin/bitcoin/issues/10102 +SUPPRESS["libbitcoin_common_a-common.o libbitcoin_node_a-interface_ui.o _Z11InitWarningRK13bilingual_str"]=1 +SUPPRESS["libbitcoin_common_a-common.o libbitcoin_node_a-interface_ui.o _Z9InitErrorRK13bilingual_str"]=1 +# rpc/external_signer.cpp adds defines node RPC methods but is built as part of the +# common library. It should be moved to the node library instead. +SUPPRESS["libbitcoin_common_a-external_signer.o libbitcoin_node_a-server.o _ZN9CRPCTable13appendCommandERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEPK11CRPCCommand"]=1 + +usage() { + echo "Usage: $(basename "${BASH_SOURCE[0]}") [BUILD_DIR]" +} + +# Output makefile targets, converting library .a paths to libtool .la targets +lib_targets() { + for lib in "${!LIBS[@]}"; do + for lib_path in ${LIBS[$lib]}; do + # shellcheck disable=SC2001 + sed 's:/.libs/\(.*\)\.a$:/\1.la:g' <<<"$lib_path" + done + done +} + +# Extract symbol names and object names and write to text files +extract_symbols() { + local temp_dir="$1" + for lib in "${!LIBS[@]}"; do + for lib_path in ${LIBS[$lib]}; do + nm -o "$lib_path" | grep ' T ' | awk '{print $3, $1}' >> "${temp_dir}/${lib}_exports.txt" + nm -o "$lib_path" | grep ' U ' | awk '{print $3, $1}' >> "${temp_dir}/${lib}_imports.txt" + awk '{print $1}' "${temp_dir}/${lib}_exports.txt" | sort -u > "${temp_dir}/${lib}_exported_symbols.txt" + awk '{print $1}' "${temp_dir}/${lib}_imports.txt" | sort -u > "${temp_dir}/${lib}_imported_symbols.txt" + done + done +} + +# Lookup object name(s) corresponding to symbol name in text file +obj_names() { + local symbol="$1" + local txt_file="$2" + sed -n "s/^$symbol [^:]\\+:\\([^:]\\+\\):[^:]*\$/\\1/p" "$txt_file" | sort -u +} + +# Iterate through libraries and find disallowed dependencies +check_libraries() { + local temp_dir="$1" + local result=0 + for src in "${!LIBS[@]}"; do + for dst in "${!LIBS[@]}"; do + if [ "$src" != "$dst" ] && ! is_allowed "$src" "$dst"; then + if ! check_disallowed "$src" "$dst" "$temp_dir"; then + result=1 + fi + fi + done + done + check_not_suppressed + return $result +} + +# Return whether src library is allowed to depend on dst. +is_allowed() { + local src="$1" + local dst="$2" + for allowed in "${ALLOWED_DEPENDENCIES[@]}"; do + if [ "$src $dst" = "$allowed" ]; then + return 0 + fi + done + return 1 +} + +# Return whether src library imports any symbols from dst, assuming src is not +# allowed to depend on dst. +check_disallowed() { + local src="$1" + local dst="$2" + local temp_dir="$3" + local result=0 + + # Loop over symbol names exported by dst and imported by src + while read symbol; do + local dst_obj + dst_obj=$(obj_names "$symbol" "${temp_dir}/${dst}_exports.txt") + while read src_obj; do + if ! check_suppress "$src_obj" "$dst_obj" "$symbol"; then + echo "Error: $src_obj depends on $dst_obj symbol '$(c++filt "$symbol")', can suppess with:" + echo " SUPPRESS[\"$src_obj $dst_obj $symbol\"]=1" + result=1 + fi + done < <(obj_names "$symbol" "${temp_dir}/${src}_imports.txt") + done < <(comm -12 "${temp_dir}/${dst}_exported_symbols.txt" "${temp_dir}/${src}_imported_symbols.txt") + return $result +} + +# Declare array to track errors which were suppressed. +declare -A SUPPRESSED + +# Return whether error should be suppressed and record suppresssion in +# SUPPRESSED array. +check_suppress() { + local src_obj="$1" + local dst_obj="$2" + local symbol="$3" + for suppress in "${!SUPPRESS[@]}"; do + read suppress_src suppress_dst suppress_pattern <<<"$suppress" + if [[ "$src_obj" == "$suppress_src" && "$dst_obj" == "$suppress_dst" && "$symbol" =~ $suppress_pattern ]]; then + SUPPRESSED["$suppress"]=1 + return 0 + fi + done + return 1 +} + +# Warn about error which were supposed to be suppress, but were not encountered. +check_not_suppressed() { + for suppress in "${!SUPPRESS[@]}"; do + if [[ ! -v SUPPRESSED[$suppress] ]]; then + echo >&2 "Warning: suppression '$suppress' was ignored, consider deleting." + fi + done +} + +# Check arguments. +if [ "$#" = 0 ]; then + BUILD_DIR="$(dirname "${BASH_SOURCE[0]}")/../../src" +elif [ "$#" = 1 ]; then + BUILD_DIR="$1" +else + echo >&2 "Error: wrong number of arguments." + usage >&2 + exit 1 +fi +if [ ! -f "$BUILD_DIR/Makefile" ]; then + echo >&2 "Error: directory '$BUILD_DIR' does not contain a makefile, please specify path to build directory for library targets." + usage >&2 + exit 1 +fi + +# Build libraries and run checks. +cd "$BUILD_DIR" +# shellcheck disable=SC2046 +make -j"$(nproc)" $(lib_targets) +TEMP_DIR="$(mktemp -d)" +extract_symbols "$TEMP_DIR" +if check_libraries "$TEMP_DIR"; then + echo "Success! No unexpected dependencies were detected." +else + echo >&2 "Error: Unexpected dependencies were detected. Check previous output." +fi +rm -r "$TEMP_DIR" From 5b9309420cc9721a0d5745b6ad3166a4bdbd1508 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 10 May 2024 15:45:07 -0400 Subject: [PATCH 028/116] build: move chainparamsbase from util to common Move chainparamsbase from util to common, because util library should not depend on the common library and chainparamsbase uses the ArgsManager class in common. --- src/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Makefile.am b/src/Makefile.am index b749651b726d2..1fd0cdd7a80cc 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -669,6 +669,7 @@ libbitcoin_common_a_SOURCES = \ addresstype.cpp \ base58.cpp \ bech32.cpp \ + chainparamsbase.cpp \ chainparams.cpp \ coins.cpp \ common/args.cpp \ @@ -719,7 +720,6 @@ libbitcoin_util_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) libbitcoin_util_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) libbitcoin_util_a_SOURCES = \ support/lockedpool.cpp \ - chainparamsbase.cpp \ clientversion.cpp \ logging.cpp \ random.cpp \ From cc5f29fbea15d33e4d1aa95591253c6b86953fe7 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 10 May 2024 15:45:07 -0400 Subject: [PATCH 029/116] build: move memory_cleanse from util to crypto Move memory_cleanse from util to crypto because the crypto library should not depend on other libraries, and it calls memory_cleanse. --- src/Makefile.am | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 1fd0cdd7a80cc..bb61f2d0276b0 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -587,7 +587,8 @@ crypto_libbitcoin_crypto_base_la_SOURCES = \ crypto/sha512.cpp \ crypto/sha512.h \ crypto/siphash.cpp \ - crypto/siphash.h + crypto/siphash.h \ + support/cleanse.cpp # See explanation for -static in crypto_libbitcoin_crypto_base_la's LDFLAGS and # CXXFLAGS above @@ -725,7 +726,6 @@ libbitcoin_util_a_SOURCES = \ random.cpp \ randomenv.cpp \ streams.cpp \ - support/cleanse.cpp \ sync.cpp \ util/asmap.cpp \ util/batchpriority.cpp \ @@ -968,7 +968,6 @@ libbitcoinkernel_la_SOURCES = \ script/solver.cpp \ signet.cpp \ streams.cpp \ - support/cleanse.cpp \ support/lockedpool.cpp \ sync.cpp \ txdb.cpp \ From 6861f954f8ff42c87ad638037adae86a5bd89600 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 10 May 2024 15:45:07 -0400 Subject: [PATCH 030/116] util: move util/message to common/signmessage Move util/message to common/signmessage so it is named more clearly, and because the util library is not supposed to depend on other libraries besides the crypto library. The signmessage functions use CKey, CPubKey, PKHash, and DecodeDestination functions in the consensus and common libraries. --- src/Makefile.am | 4 ++-- src/{util/message.cpp => common/signmessage.cpp} | 2 +- src/{util/message.h => common/signmessage.h} | 6 +++--- src/interfaces/wallet.h | 2 +- src/qt/signverifymessagedialog.cpp | 2 +- src/rpc/signmessage.cpp | 2 +- src/test/fuzz/message.cpp | 2 +- src/test/util_tests.cpp | 2 +- src/wallet/rpc/signmessage.cpp | 2 +- src/wallet/scriptpubkeyman.h | 2 +- src/wallet/wallet.cpp | 2 +- 11 files changed, 14 insertions(+), 14 deletions(-) rename src/{util/message.cpp => common/signmessage.cpp} (98%) rename src/{util/message.h => common/signmessage.h} (95%) diff --git a/src/Makefile.am b/src/Makefile.am index bb61f2d0276b0..54000a7eb19d6 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -144,6 +144,7 @@ BITCOIN_CORE_H = \ compat/cpuid.h \ compat/endian.h \ common/settings.h \ + common/signmessage.h \ common/system.h \ compressor.h \ consensus/consensus.h \ @@ -308,7 +309,6 @@ BITCOIN_CORE_H = \ util/hasher.h \ util/insert.h \ util/macros.h \ - util/message.h \ util/moneystr.h \ util/overflow.h \ util/overloaded.h \ @@ -680,6 +680,7 @@ libbitcoin_common_a_SOURCES = \ common/interfaces.cpp \ common/run_command.cpp \ common/settings.cpp \ + common/signmessage.cpp \ common/system.cpp \ common/url.cpp \ compressor.cpp \ @@ -742,7 +743,6 @@ libbitcoin_util_a_SOURCES = \ util/hasher.cpp \ util/sock.cpp \ util/syserror.cpp \ - util/message.cpp \ util/moneystr.cpp \ util/rbf.cpp \ util/readwritefile.cpp \ diff --git a/src/util/message.cpp b/src/common/signmessage.cpp similarity index 98% rename from src/util/message.cpp rename to src/common/signmessage.cpp index 1afb28cc10dc5..1612751e44dd6 100644 --- a/src/util/message.cpp +++ b/src/common/signmessage.cpp @@ -3,12 +3,12 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include #include #include #include #include -#include #include #include diff --git a/src/util/message.h b/src/common/signmessage.h similarity index 95% rename from src/util/message.h rename to src/common/signmessage.h index d0e2422574b04..215b563bbb215 100644 --- a/src/util/message.h +++ b/src/common/signmessage.h @@ -3,8 +3,8 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#ifndef BITCOIN_UTIL_MESSAGE_H -#define BITCOIN_UTIL_MESSAGE_H +#ifndef BITCOIN_COMMON_SIGNMESSAGE_H +#define BITCOIN_COMMON_SIGNMESSAGE_H #include @@ -74,4 +74,4 @@ uint256 MessageHash(const std::string& message); std::string SigningResultString(const SigningResult res); -#endif // BITCOIN_UTIL_MESSAGE_H +#endif // BITCOIN_COMMON_SIGNMESSAGE_H diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index c41f35829d7e7..f1bfffa7d465f 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -6,13 +6,13 @@ #define BITCOIN_INTERFACES_WALLET_H #include +#include #include #include #include #include