From cca4f82b828669ae23f6ac64fb83e068b81ae189 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Fri, 26 Aug 2022 15:23:21 -0300 Subject: [PATCH 001/272] test: add coverage for rpc error when trying to rescan beyond pruned data --- test/functional/feature_pruning.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/functional/feature_pruning.py b/test/functional/feature_pruning.py index 7dbeccbc0935f..9f37335850180 100755 --- a/test/functional/feature_pruning.py +++ b/test/functional/feature_pruning.py @@ -143,6 +143,10 @@ def test_invalid_command_line_options(self): extra_args=['-prune=550', '-reindex-chainstate'], ) + def test_rescan_blockchain(self): + self.restart_node(0, ["-prune=550"]) + assert_raises_rpc_error(-1, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height.", self.nodes[0].rescanblockchain) + def test_height_min(self): assert os.path.isfile(os.path.join(self.prunedir, "blk00000.dat")), "blk00000.dat is missing, pruning too early" self.log.info("Success") @@ -477,6 +481,9 @@ def run_test(self): self.log.info("Test wallet re-scan") self.wallet_test() + self.log.info("Test it's not possible to rescan beyond pruned data") + self.test_rescan_blockchain() + self.log.info("Test invalid pruning command line options") self.test_invalid_command_line_options() From 33fdfc7986455191df8ce339261bc0561115cf7f Mon Sep 17 00:00:00 2001 From: brunoerg Date: Fri, 14 Oct 2022 12:02:19 -0300 Subject: [PATCH 002/272] test: perturb anchors.dat to test it doesn't throw an error during initialization --- test/functional/feature_anchors.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/test/functional/feature_anchors.py b/test/functional/feature_anchors.py index 713c0826d39ff..468ad1eafaab2 100755 --- a/test/functional/feature_anchors.py +++ b/test/functional/feature_anchors.py @@ -63,17 +63,25 @@ def run_test(self): self.log.info("Check the addresses in anchors.dat") with open(node_anchors_path, "rb") as file_handler: - anchors = file_handler.read().hex() + anchors = file_handler.read() + anchors_hex = anchors.hex() for port in block_relay_nodes_port: ip_port = ip + port - assert ip_port in anchors + assert ip_port in anchors_hex for port in inbound_nodes_port: ip_port = ip + port - assert ip_port not in anchors + assert ip_port not in anchors_hex - self.log.info("Start node") - self.start_node(0) + self.log.info("Perturb anchors.dat to test it doesn't throw an error during initialization") + with self.nodes[0].assert_debug_log(["0 block-relay-only anchors will be tried for connections."]): + with open(node_anchors_path, "wb") as out_file_handler: + tweaked_contents = bytearray(anchors) + tweaked_contents[20:20] = b'1' + out_file_handler.write(bytes(tweaked_contents)) + + self.log.info("Start node") + self.start_node(0) self.log.info("When node starts, check if anchors.dat doesn't exist anymore") assert not os.path.exists(node_anchors_path) From 9141e4395a5f923059ad61ac6ba42a1a89e92cb0 Mon Sep 17 00:00:00 2001 From: Yusuf Sahin HAMZA Date: Tue, 26 Jul 2022 11:05:39 +0300 Subject: [PATCH 003/272] rpc, docs: Add note for commands that supports only legacy wallets Note is added for following rpc commands: importprivkey, importpubkey, importwallet, dumpprivkey, dumpwallet, importmulti, addmultisigaddress, sethdseed --- src/wallet/rpc/addresses.cpp | 3 ++- src/wallet/rpc/backup.cpp | 20 +++++++++++++------- src/wallet/rpc/wallet.cpp | 4 ++-- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 903a569cb9d70..67d5037fd25ce 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -220,7 +220,8 @@ RPCHelpMan addmultisigaddress() "Each key is a Bitcoin address or hex-encoded public key.\n" "This functionality is only intended for use with non-watchonly addresses.\n" "See `importaddress` for watchonly p2sh address support.\n" - "If 'label' is specified, assign address to that label.\n", + "If 'label' is specified, assign address to that label.\n" + "Note: This command is only compatible with legacy wallets.\n", { {"nrequired", RPCArg::Type::NUM, RPCArg::Optional::NO, "The number of required signatures out of the n keys or addresses."}, {"keys", RPCArg::Type::ARR, RPCArg::Optional::NO, "The bitcoin addresses or hex-encoded public keys", diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 1d2d7d2a10dcc..3eb8e2ddcb023 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -102,7 +102,8 @@ RPCHelpMan importprivkey() "may report that the imported key exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.\n" "The rescan parameter can be set to false if the key was never used to create transactions. If it is set to false,\n" "but the key was used to create transactions, rescanblockchain needs to be called with the appropriate block range.\n" - "Note: Use \"getwalletinfo\" to query the scanning progress.\n", + "Note: Use \"getwalletinfo\" to query the scanning progress.\n" + "Note: This command is only compatible with legacy wallets. Use \"importdescriptors\" with \"combo(X)\" for descriptor wallets.\n", { {"privkey", RPCArg::Type::STR, RPCArg::Optional::NO, "The private key (see dumpprivkey)"}, {"label", RPCArg::Type::STR, RPCArg::DefaultHint{"current label if address exists, otherwise \"\""}, "An optional label"}, @@ -210,7 +211,7 @@ RPCHelpMan importaddress() "\nNote: If you import a non-standard raw script in hex form, outputs sending to it will be treated\n" "as change, and not show up in many RPCs.\n" "Note: Use \"getwalletinfo\" to query the scanning progress.\n" - "Note: This command is only compatible with legacy wallets. Use \"importdescriptors\" with \"addr(X)\" for descriptor wallets.\n", + "Note: This command is only compatible with legacy wallets. Use \"importdescriptors\" for descriptor wallets.\n", { {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The Bitcoin address (or hex-encoded script)"}, {"label", RPCArg::Type::STR, RPCArg::Default{""}, "An optional label"}, @@ -404,7 +405,8 @@ RPCHelpMan importpubkey() "may report that the imported pubkey exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.\n" "The rescan parameter can be set to false if the key was never used to create transactions. If it is set to false,\n" "but the key was used to create transactions, rescanblockchain needs to be called with the appropriate block range.\n" - "Note: Use \"getwalletinfo\" to query the scanning progress.\n", + "Note: Use \"getwalletinfo\" to query the scanning progress.\n" + "Note: This command is only compatible with legacy wallets. Use \"importdescriptors\" with \"combo(X)\" for descriptor wallets.\n", { {"pubkey", RPCArg::Type::STR, RPCArg::Optional::NO, "The hex-encoded public key"}, {"label", RPCArg::Type::STR, RPCArg::Default{""}, "An optional label"}, @@ -484,7 +486,8 @@ RPCHelpMan importwallet() { return RPCHelpMan{"importwallet", "\nImports keys from a wallet dump file (see dumpwallet). Requires a new wallet backup to include imported keys.\n" - "Note: Blockchain and Mempool will be rescanned after a successful import. Use \"getwalletinfo\" to query the scanning progress.\n", + "Note: Blockchain and Mempool will be rescanned after a successful import. Use \"getwalletinfo\" to query the scanning progress.\n" + "Note: This command is only compatible with legacy wallets.\n", { {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The wallet file"}, }, @@ -640,7 +643,8 @@ RPCHelpMan dumpprivkey() { return RPCHelpMan{"dumpprivkey", "\nReveals the private key corresponding to 'address'.\n" - "Then the importprivkey can be used with this output\n", + "Then the importprivkey can be used with this output\n" + "Note: This command is only compatible with legacy wallets.\n", { {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The bitcoin address for the private key"}, }, @@ -688,7 +692,8 @@ RPCHelpMan dumpwallet() "\nDumps all wallet keys in a human-readable format to a server-side file. This does not allow overwriting existing files.\n" "Imported scripts are included in the dumpfile, but corresponding BIP173 addresses, etc. may not be added automatically by importwallet.\n" "Note that if your wallet contains keys which are not derived from your HD seed (e.g. imported keys), these are not covered by\n" - "only backing up the seed itself, and must be backed up too (e.g. ensure you back up the whole dumpfile).\n", + "only backing up the seed itself, and must be backed up too (e.g. ensure you back up the whole dumpfile).\n" + "Note: This command is only compatible with legacy wallets.\n", { {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The filename with path (absolute path recommended)"}, }, @@ -1252,7 +1257,8 @@ RPCHelpMan importmulti() "may report that the imported keys, addresses or scripts exist but related transactions are still missing.\n" "The rescan parameter can be set to false if the key was never used to create transactions. If it is set to false,\n" "but the key was used to create transactions, rescanblockchain needs to be called with the appropriate block range.\n" - "Note: Use \"getwalletinfo\" to query the scanning progress.\n", + "Note: Use \"getwalletinfo\" to query the scanning progress.\n" + "Note: This command is only compatible with legacy wallets. Use \"importdescriptors\" for descriptor wallets.\n", { {"requests", RPCArg::Type::ARR, RPCArg::Optional::NO, "Data to be imported", { diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 675c4a759d00b..cd74f672c1fe4 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -460,8 +460,8 @@ static RPCHelpMan sethdseed() return RPCHelpMan{"sethdseed", "\nSet or generate a new HD wallet seed. Non-HD wallets will not be upgraded to being a HD wallet. Wallets that are already\n" "HD will have a new HD seed set so that new keys added to the keypool will be derived from this new seed.\n" - "\nNote that you will need to MAKE A NEW BACKUP of your wallet after setting the HD wallet seed." + - HELP_REQUIRING_PASSPHRASE, + "\nNote that you will need to MAKE A NEW BACKUP of your wallet after setting the HD wallet seed." + HELP_REQUIRING_PASSPHRASE + + "Note: This command is only compatible with legacy wallets.\n", { {"newkeypool", RPCArg::Type::BOOL, RPCArg::Default{true}, "Whether to flush old unused addresses, including change addresses, from the keypool and regenerate it.\n" "If true, the next address from getnewaddress and change address from getrawchangeaddress will be from this new seed.\n" From c371cae07a7ba045130568b6abc470eaa4f95ef4 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Wed, 7 Dec 2022 13:50:33 -0300 Subject: [PATCH 004/272] test, init: perturb file to ensure failure instead of only deleting them --- test/functional/feature_init.py | 48 ++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/test/functional/feature_init.py b/test/functional/feature_init.py index cf626bc7c6256..634e08e9e28d7 100755 --- a/test/functional/feature_init.py +++ b/test/functional/feature_init.py @@ -45,6 +45,13 @@ def sigterm_node(): node.process.terminate() node.process.wait() + def start_expecting_error(err_fragment): + node.assert_start_raises_init_error( + extra_args=['-txindex=1', '-blockfilterindex=1', '-coinstatsindex=1'], + expected_msg=err_fragment, + match=ErrorMatch.PARTIAL_REGEX, + ) + def check_clean_start(): """Ensure that node restarts successfully after various interrupts.""" node.start() @@ -87,36 +94,27 @@ def check_clean_start(): self.log.info("Test startup errors after removing certain essential files") - files_to_disturb = { + files_to_delete = { 'blocks/index/*.ldb': 'Error opening block database.', 'chainstate/*.ldb': 'Error opening block database.', 'blocks/blk*.dat': 'Error loading block database.', } - for file_patt, err_fragment in files_to_disturb.items(): + files_to_perturb = { + 'blocks/index/*.ldb': 'Error opening block database.', + 'chainstate/*.ldb': 'Error opening block database.', + 'blocks/blk*.dat': 'Error opening block database.', + } + + for file_patt, err_fragment in files_to_delete.items(): target_files = list(node.chain_path.glob(file_patt)) for target_file in target_files: - self.log.info(f"Tweaking file to ensure failure {target_file}") + self.log.info(f"Deleting file to ensure failure {target_file}") bak_path = str(target_file) + ".bak" target_file.rename(bak_path) - # TODO: at some point, we should test perturbing the files instead of removing - # them, e.g. - # - # contents = target_file.read_bytes() - # tweaked_contents = bytearray(contents) - # tweaked_contents[50:250] = b'1' * 200 - # target_file.write_bytes(bytes(tweaked_contents)) - # - # At the moment I can't get this to work (bitcoind loads successfully?) so - # investigate doing this later. - - node.assert_start_raises_init_error( - extra_args=['-txindex=1', '-blockfilterindex=1', '-coinstatsindex=1'], - expected_msg=err_fragment, - match=ErrorMatch.PARTIAL_REGEX, - ) + start_expecting_error(err_fragment) for target_file in target_files: bak_path = str(target_file) + ".bak" @@ -126,6 +124,18 @@ def check_clean_start(): check_clean_start() self.stop_node(0) + for file_patt, err_fragment in files_to_perturb.items(): + target_files = list(node.chain_path.glob(file_patt)) + + for target_file in target_files: + self.log.info(f"Perturbing file to ensure failure {target_file}") + with open(target_file, "rb") as tf_read, open(target_file, "wb") as tf_write: + contents = tf_read.read() + tweaked_contents = bytearray(contents) + tweaked_contents[50:250] = b'1' * 200 + tf_write.write(bytes(tweaked_contents)) + + start_expecting_error(err_fragment) if __name__ == '__main__': InitStressTest().main() From 3cc989da5c750e740705131bed05bbf93bfdf169 Mon Sep 17 00:00:00 2001 From: Yusuf Sahin HAMZA Date: Sat, 10 Dec 2022 19:30:28 +0300 Subject: [PATCH 005/272] Fix checking bad dns seeds without casting Since seed lines comes with 'str' type, comparing it directly with 0 ('int' type) in the if statement was not working at all. This is fixed by casting 'int' type to the values in the 'good' column of seeds text file. Lines that starts with comment in the seeds text file are now ignored. If statement for checking bad seeds are moved to the top of the 'parseline' function as if seed is bad, there is no point of going forward from there. --- contrib/seeds/makeseeds.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/contrib/seeds/makeseeds.py b/contrib/seeds/makeseeds.py index eda58c370f293..09b6ceb785c39 100755 --- a/contrib/seeds/makeseeds.py +++ b/contrib/seeds/makeseeds.py @@ -46,10 +46,16 @@ def parseline(line: str) -> Union[dict, None]: """ Parses a line from `seeds_main.txt` into a dictionary of details for that line. or `None`, if the line could not be parsed. """ + if line.startswith('#'): + # Ignore line that starts with comment + return None sline = line.split() if len(sline) < 11: # line too short to be valid, skip it. return None + # Skip bad results. + if int(sline[1]) == 0: + return None m = PATTERN_IPV4.match(sline[0]) sortkey = None ip = None @@ -83,9 +89,6 @@ def parseline(line: str) -> Union[dict, None]: sortkey = ip ipstr = m.group(1) port = int(m.group(6)) - # Skip bad results. - if sline[1] == 0: - return None # Extract uptime %. uptime30 = float(sline[7][:-1]) # Extract Unix timestamp of last success. From 53552affca381cdb5103ecdbcc7f3fb562e66ac4 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Mon, 12 Dec 2022 21:05:33 +0000 Subject: [PATCH 006/272] [headerssync] Make m_commit_offset protected --- src/headerssync.cpp | 2 +- src/headerssync.h | 13 +++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/headerssync.cpp b/src/headerssync.cpp index 757b942cd9697..a3adfb4f701d9 100644 --- a/src/headerssync.cpp +++ b/src/headerssync.cpp @@ -24,11 +24,11 @@ static_assert(sizeof(CompressedHeader) == 48); HeadersSyncState::HeadersSyncState(NodeId id, const Consensus::Params& consensus_params, const CBlockIndex* chain_start, const arith_uint256& minimum_required_work) : + m_commit_offset(GetRand(HEADER_COMMITMENT_PERIOD)), m_id(id), m_consensus_params(consensus_params), m_chain_start(chain_start), m_minimum_required_work(minimum_required_work), m_current_chain_work(chain_start->nChainWork), - m_commit_offset(GetRand(HEADER_COMMITMENT_PERIOD)), m_last_header_received(m_chain_start->GetBlockHeader()), m_current_height(chain_start->nHeight) { diff --git a/src/headerssync.h b/src/headerssync.h index 16da9642462d4..e93f67e6da1fb 100644 --- a/src/headerssync.h +++ b/src/headerssync.h @@ -175,6 +175,13 @@ class HeadersSyncState { */ CBlockLocator NextHeadersRequestLocator() const; +protected: + /** The (secret) offset on the heights for which to create commitments. + * + * m_header_commitments entries are created at any height h for which + * (h % HEADER_COMMITMENT_PERIOD) == m_commit_offset. */ + const unsigned m_commit_offset; + private: /** Clear out all download state that might be in progress (freeing any used * memory), and mark this object as no longer usable. @@ -222,12 +229,6 @@ class HeadersSyncState { /** A queue of commitment bits, created during the 1st phase, and verified during the 2nd. */ bitdeque<> m_header_commitments; - /** The (secret) offset on the heights for which to create commitments. - * - * m_header_commitments entries are created at any height h for which - * (h % HEADER_COMMITMENT_PERIOD) == m_commit_offset. */ - const unsigned m_commit_offset; - /** m_max_commitments is a bound we calculate on how long an honest peer's chain could be, * given the MTP rule. * From 3153e7d779ac284f86e433af033d63f13f361b6f Mon Sep 17 00:00:00 2001 From: dergoegge Date: Thu, 8 Dec 2022 12:47:24 +0000 Subject: [PATCH 007/272] [fuzz] Add HeadersSyncState target --- src/Makefile.test.include | 1 + src/test/fuzz/headerssync.cpp | 117 ++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+) create mode 100644 src/test/fuzz/headerssync.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 74c30f1caf9ce..c82e29031110f 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -269,6 +269,7 @@ test_fuzz_fuzz_SOURCES = \ test/fuzz/flatfile.cpp \ test/fuzz/float.cpp \ test/fuzz/golomb_rice.cpp \ + test/fuzz/headerssync.cpp \ test/fuzz/hex.cpp \ test/fuzz/http_request.cpp \ test/fuzz/i2p.cpp \ diff --git a/src/test/fuzz/headerssync.cpp b/src/test/fuzz/headerssync.cpp new file mode 100644 index 0000000000000..521afadb51d3d --- /dev/null +++ b/src/test/fuzz/headerssync.cpp @@ -0,0 +1,117 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +static void initialize_headers_sync_state_fuzz() +{ + static const auto testing_setup = MakeNoLogFileContext<>( + /*chain_name=*/CBaseChainParams::MAIN); +} + +void MakeHeadersContinuous( + const CBlockHeader& genesis_header, + const std::vector& all_headers, + std::vector& new_headers) +{ + Assume(!new_headers.empty()); + + const CBlockHeader* prev_header{ + all_headers.empty() ? &genesis_header : &all_headers.back()}; + + for (auto& header : new_headers) { + header.hashPrevBlock = prev_header->GetHash(); + + prev_header = &header; + } +} + +class FuzzedHeadersSyncState : public HeadersSyncState +{ +public: + FuzzedHeadersSyncState(const unsigned commit_offset, const CBlockIndex* chain_start, const arith_uint256& minimum_required_work) + : HeadersSyncState(/*id=*/0, Params().GetConsensus(), chain_start, minimum_required_work) + { + const_cast(m_commit_offset) = commit_offset; + } +}; + +FUZZ_TARGET_INIT(headers_sync_state, initialize_headers_sync_state_fuzz) +{ + FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + auto mock_time{ConsumeTime(fuzzed_data_provider)}; + + CBlockHeader genesis_header{Params().GenesisBlock()}; + CBlockIndex start_index(genesis_header); + + if (mock_time < start_index.GetMedianTimePast()) return; + SetMockTime(mock_time); + + const uint256 genesis_hash = genesis_header.GetHash(); + start_index.phashBlock = &genesis_hash; + + arith_uint256 min_work{UintToArith256(ConsumeUInt256(fuzzed_data_provider))}; + FuzzedHeadersSyncState headers_sync( + /*commit_offset=*/fuzzed_data_provider.ConsumeIntegralInRange(1, 1024), + /*chain_start=*/&start_index, + /*minimum_required_work=*/min_work); + + // Store headers for potential redownload phase. + std::vector all_headers; + std::vector::const_iterator redownloaded_it; + bool presync{true}; + bool requested_more{true}; + + while (requested_more) { + std::vector headers; + + // Consume headers from fuzzer or maybe replay headers if we got to the + // redownload phase. + if (presync || fuzzed_data_provider.ConsumeBool()) { + auto deser_headers = ConsumeDeserializable>(fuzzed_data_provider); + if (!deser_headers || deser_headers->empty()) return; + + if (fuzzed_data_provider.ConsumeBool()) { + MakeHeadersContinuous(genesis_header, all_headers, *deser_headers); + } + + headers.swap(*deser_headers); + } else if (auto num_headers_left{std::distance(redownloaded_it, all_headers.cend())}; num_headers_left > 0) { + // Consume some headers from the redownload buffer (At least one + // header is consumed). + auto begin_it{redownloaded_it}; + std::advance(redownloaded_it, fuzzed_data_provider.ConsumeIntegralInRange(1, num_headers_left)); + headers.insert(headers.cend(), begin_it, redownloaded_it); + } + + if (headers.empty()) return; + auto result = headers_sync.ProcessNextHeaders(headers, fuzzed_data_provider.ConsumeBool()); + requested_more = result.request_more; + + if (result.request_more) { + if (presync) { + all_headers.insert(all_headers.cend(), headers.cbegin(), headers.cend()); + + if (headers_sync.GetState() == HeadersSyncState::State::REDOWNLOAD) { + presync = false; + redownloaded_it = all_headers.cbegin(); + + // If we get to redownloading, the presynced headers need + // to have the min amount of work on them. + assert(CalculateHeadersWork(all_headers) >= min_work); + } + } + + (void)headers_sync.NextHeadersRequestLocator(); + } + } +} From 057057a2d7e23c2e29cbfd29a5124b3947a33757 Mon Sep 17 00:00:00 2001 From: Yusuf Sahin HAMZA Date: Wed, 21 Dec 2022 00:18:35 +0300 Subject: [PATCH 008/272] Add test for `sendmany` rpc that uses `subtractfeefrom` parameter --- test/functional/wallet_basic.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 86cfd4a230bcb..2e10c0f0bfc65 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -267,6 +267,20 @@ def run_test(self): assert_equal(self.nodes[2].getbalance(), node_2_bal) node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), node_0_bal + Decimal('10'), fee_per_byte, self.get_vsize(self.nodes[2].gettransaction(txid)['hex'])) + # Sendmany 5 BTC to two addresses with subtracting fee from both addresses + a0 = self.nodes[0].getnewaddress() + a1 = self.nodes[0].getnewaddress() + txid = self.nodes[2].sendmany(dummy='', amounts={a0: 5, a1: 5}, subtractfeefrom=[a0, a1]) + self.generate(self.nodes[2], 1, sync_fun=lambda: self.sync_all(self.nodes[0:3])) + node_2_bal -= Decimal('10') + assert_equal(self.nodes[2].getbalance(), node_2_bal) + tx = self.nodes[2].gettransaction(txid) + node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), node_0_bal + Decimal('10'), fee_per_byte, self.get_vsize(tx['hex'])) + assert_equal(self.nodes[0].getbalance(), node_0_bal) + expected_bal = Decimal('5') + (tx['fee'] / 2) + assert_equal(self.nodes[0].getreceivedbyaddress(a0), expected_bal) + assert_equal(self.nodes[0].getreceivedbyaddress(a1), expected_bal) + self.log.info("Test sendmany with fee_rate param (explicit fee rate in sat/vB)") fee_rate_sat_vb = 2 fee_rate_btc_kvb = fee_rate_sat_vb * 1e3 / 1e8 From 4bdcf571584cbe44ad5533a3c991d3b0b4b2c84f Mon Sep 17 00:00:00 2001 From: brunoerg Date: Mon, 2 Jan 2023 15:26:23 -0300 Subject: [PATCH 009/272] test: test banlist database recreation --- test/functional/p2p_disconnect_ban.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/functional/p2p_disconnect_ban.py b/test/functional/p2p_disconnect_ban.py index 91c2a4393234f..cbaeb3ce65a35 100755 --- a/test/functional/p2p_disconnect_ban.py +++ b/test/functional/p2p_disconnect_ban.py @@ -4,6 +4,7 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test node disconnect and ban behavior""" import time +from pathlib import Path from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -36,6 +37,17 @@ def run_test(self): self.log.info("clearbanned: successfully clear ban list") self.nodes[1].clearbanned() assert_equal(len(self.nodes[1].listbanned()), 0) + + self.log.info('Test banlist database recreation') + self.stop_node(1) + target_file = self.nodes[1].chain_path / "banlist.json" + Path.unlink(target_file) + with self.nodes[1].assert_debug_log(["Recreating the banlist database"]): + self.start_node(1) + + assert Path.exists(target_file) + assert_equal(self.nodes[1].listbanned(), []) + self.nodes[1].setban("127.0.0.0/24", "add") self.log.info("setban: fail to ban an already banned subnet") From 9a1d73fdffa4f35e33bc187ac9b3afbba28b1950 Mon Sep 17 00:00:00 2001 From: John Moffett Date: Fri, 30 Dec 2022 16:09:56 -0500 Subject: [PATCH 010/272] Fix segfault when shutdown during wallet open If you open a wallet and send a shutdown signal during that process, the GUI will segfault due to some queued wallet events happening after the wallet controller is deleted. This is a minimal fix for those issues. --- src/qt/bitcoingui.cpp | 8 ++++++-- src/qt/sendcoinsdialog.cpp | 13 +++++++++---- src/qt/test/wallettests.cpp | 2 ++ 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index a0731b337aa4e..47e4f9978df81 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -680,6 +680,10 @@ void BitcoinGUI::setWalletController(WalletController* wallet_controller) GUIUtil::ExceptionSafeConnect(wallet_controller, &WalletController::walletAdded, this, &BitcoinGUI::addWallet); connect(wallet_controller, &WalletController::walletRemoved, this, &BitcoinGUI::removeWallet); + connect(wallet_controller, &WalletController::destroyed, this, [this] { + // wallet_controller gets destroyed manually, but it leaves our member copy dangling + m_wallet_controller = nullptr; + }); auto activity = new LoadWalletsActivity(m_wallet_controller, this); activity->load(); @@ -692,7 +696,7 @@ WalletController* BitcoinGUI::getWalletController() void BitcoinGUI::addWallet(WalletModel* walletModel) { - if (!walletFrame) return; + if (!walletFrame || !m_wallet_controller) return; WalletView* wallet_view = new WalletView(walletModel, platformStyle, walletFrame); if (!walletFrame->addView(wallet_view)) return; @@ -742,7 +746,7 @@ void BitcoinGUI::removeWallet(WalletModel* walletModel) void BitcoinGUI::setCurrentWallet(WalletModel* wallet_model) { - if (!walletFrame) return; + if (!walletFrame || !m_wallet_controller) return; walletFrame->setCurrentWallet(wallet_model); for (int index = 0; index < m_wallet_selector->count(); ++index) { if (m_wallet_selector->itemData(index).value() == wallet_model) { diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 249e3f2101c72..70f34038b6e47 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -600,10 +600,15 @@ SendCoinsEntry *SendCoinsDialog::addEntry() entry->clear(); entry->setFocus(); ui->scrollAreaWidgetContents->resize(ui->scrollAreaWidgetContents->sizeHint()); - qApp->processEvents(); - QScrollBar* bar = ui->scrollArea->verticalScrollBar(); - if(bar) - bar->setSliderPosition(bar->maximum()); + + // Scroll to the newly added entry on a QueuedConnection because Qt doesn't + // adjust the scroll area and scrollbar immediately when the widget is added. + // Invoking on a DirectConnection will only scroll to the second-to-last entry. + QMetaObject::invokeMethod(ui->scrollArea, [this] { + if (ui->scrollArea->verticalScrollBar()) { + ui->scrollArea->verticalScrollBar()->setValue(ui->scrollArea->verticalScrollBar()->maximum()); + } + }, Qt::QueuedConnection); updateTabsAndLabels(); return entry; diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 59a59348906e5..34f5e78fc28ea 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -212,6 +212,8 @@ void TestGUI(interfaces::Node& node) QCOMPARE(transactionTableModel->rowCount({}), 105); uint256 txid1 = SendCoins(*wallet.get(), sendCoinsDialog, PKHash(), 5 * COIN, /*rbf=*/false); uint256 txid2 = SendCoins(*wallet.get(), sendCoinsDialog, PKHash(), 10 * COIN, /*rbf=*/true); + // Transaction table model updates on a QueuedConnection, so process events to ensure it's updated. + qApp->processEvents(); QCOMPARE(transactionTableModel->rowCount({}), 107); QVERIFY(FindTx(*transactionTableModel, txid1).isValid()); QVERIFY(FindTx(*transactionTableModel, txid2).isValid()); From a1aaa7f51f4205ae4d27fbceb2c3a97bc114e828 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Mon, 16 May 2022 17:58:19 -0300 Subject: [PATCH 011/272] rpc, wallet: add `abandoned` field for all categories of transactions in ListTransactions --- src/wallet/rpc/transactions.cpp | 10 ++++------ test/functional/wallet_bumpfee.py | 5 +++++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index c257af13c4f26..d2b33a8b700f0 100644 --- a/src/wallet/rpc/transactions.cpp +++ b/src/wallet/rpc/transactions.cpp @@ -389,6 +389,7 @@ static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nM entry.pushKV("label", label); } entry.pushKV("vout", r.vout); + entry.pushKV("abandoned", wtx.isAbandoned()); if (fLong) WalletTxToJSON(wallet, wtx, entry); ret.push_back(entry); @@ -462,8 +463,7 @@ RPCHelpMan listtransactions() }, TransactionDescriptionString()), { - {RPCResult::Type::BOOL, "abandoned", /*optional=*/true, "'true' if the transaction has been abandoned (inputs are respendable). Only available for the \n" - "'send' category of transactions."}, + {RPCResult::Type::BOOL, "abandoned", "'true' if the transaction has been abandoned (inputs are respendable)."}, })}, } }, @@ -576,8 +576,7 @@ RPCHelpMan listsinceblock() }, TransactionDescriptionString()), { - {RPCResult::Type::BOOL, "abandoned", /*optional=*/true, "'true' if the transaction has been abandoned (inputs are respendable). Only available for the \n" - "'send' category of transactions."}, + {RPCResult::Type::BOOL, "abandoned", "'true' if the transaction has been abandoned (inputs are respendable)."}, {RPCResult::Type::STR, "label", /*optional=*/true, "A comment for the address/transaction, if any"}, })}, }}, @@ -722,8 +721,7 @@ RPCHelpMan gettransaction() {RPCResult::Type::NUM, "vout", "the vout value"}, {RPCResult::Type::STR_AMOUNT, "fee", /*optional=*/true, "The amount of the fee in " + CURRENCY_UNIT + ". This is negative and only available for the \n" "'send' category of transactions."}, - {RPCResult::Type::BOOL, "abandoned", /*optional=*/true, "'true' if the transaction has been abandoned (inputs are respendable). Only available for the \n" - "'send' category of transactions."}, + {RPCResult::Type::BOOL, "abandoned", "'true' if the transaction has been abandoned (inputs are respendable)."}, {RPCResult::Type::ARR, "parent_descs", /*optional=*/true, "Only if 'category' is 'received'. List of parent descriptors for the scriptPubKey of this coin.", { {RPCResult::Type::STR, "desc", "The descriptor string."}, }}, diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index 1cb3f434e871d..f1e7869d914d1 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -539,6 +539,11 @@ def test_unconfirmed_not_spendable(self, rbf_node, rbf_node_address): # Call abandon to make sure the wallet doesn't attempt to resubmit # the bump tx and hope the wallet does not rebroadcast before we call. rbf_node.abandontransaction(bumpid) + + tx_bump_abandoned = rbf_node.gettransaction(bumpid) + for tx in tx_bump_abandoned['details']: + assert_equal(tx['abandoned'], True) + assert bumpid not in rbf_node.getrawmempool() assert rbfid in rbf_node.getrawmempool() From 0c520679ab5f0ba99584cb356ec28ef089f14735 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Mon, 16 May 2022 18:03:02 -0300 Subject: [PATCH 012/272] doc: add release notes for `abandoned` field in `gettransaction` and `listtransactions` --- doc/release-notes-25158.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 doc/release-notes-25158.md diff --git a/doc/release-notes-25158.md b/doc/release-notes-25158.md new file mode 100644 index 0000000000000..ce8ab53ddd835 --- /dev/null +++ b/doc/release-notes-25158.md @@ -0,0 +1,6 @@ +RPC Wallet +---------- + +- The `gettransaction`, `listtransactions`, `listsinceblock` RPCs now return + the `abandoned` field for all transactions. Previously, the "abandoned" field + was only returned for sent transactions. (#25158) \ No newline at end of file From a5b4883fb43d01bfef1244df62c64bf8691ca63a Mon Sep 17 00:00:00 2001 From: ishaanam Date: Wed, 27 Jul 2022 14:24:19 +0530 Subject: [PATCH 013/272] rpc: extract psbt updating logic into ProcessPSBT This function is called from utxoupdatepsbt and will be modified in a following commit to allow for updating inputs with the `non_witness_utxo` as well. --- src/rpc/rawtransaction.cpp | 115 ++++++++++++++++++++----------------- 1 file changed, 62 insertions(+), 53 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 981dead3b85bb..011983a158c42 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -169,6 +169,63 @@ static std::vector CreateTxDoc() }; } +// Update PSBT with information from the mempool, the UTXO set, and the provided descriptors +PartiallySignedTransaction ProcessPSBT(const std::string& psbt_string, const std::any& context, const HidingSigningProvider& provider) +{ + // Unserialize the transactions + PartiallySignedTransaction psbtx; + std::string error; + if (!DecodeBase64PSBT(psbtx, psbt_string, error)) { + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed %s", error)); + } + + // Fetch previous transactions (inputs): + CCoinsView viewDummy; + CCoinsViewCache view(&viewDummy); + { + NodeContext& node = EnsureAnyNodeContext(context); + const CTxMemPool& mempool = EnsureMemPool(node); + ChainstateManager& chainman = EnsureChainman(node); + LOCK2(cs_main, mempool.cs); + CCoinsViewCache &viewChain = chainman.ActiveChainstate().CoinsTip(); + CCoinsViewMemPool viewMempool(&viewChain, mempool); + view.SetBackend(viewMempool); // temporarily switch cache backend to db+mempool view + + for (const CTxIn& txin : psbtx.tx->vin) { + view.AccessCoin(txin.prevout); // Load entries from viewChain into view; can fail. + } + + view.SetBackend(viewDummy); // switch back to avoid locking mempool for too long + } + + // Fill the inputs + const PrecomputedTransactionData txdata = PrecomputePSBTData(psbtx); + for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) { + PSBTInput& input = psbtx.inputs.at(i); + + if (input.non_witness_utxo || !input.witness_utxo.IsNull()) { + continue; + } + + const Coin& coin = view.AccessCoin(psbtx.tx->vin[i].prevout); + + if (IsSegWitOutput(provider, coin.out.scriptPubKey)) { + input.witness_utxo = coin.out; + } + + // Update script/keypath information using descriptor data. + // Note that SignPSBTInput does a lot more than just constructing ECDSA signatures + // we don't actually care about those here, in fact. + SignPSBTInput(provider, psbtx, i, &txdata, /*sighash=*/1); + } + + // Update script/keypath information using descriptor data. + for (unsigned int i = 0; i < psbtx.tx->vout.size(); ++i) { + UpdatePSBTOutput(provider, psbtx, i); + } + return psbtx; +} + static RPCHelpMan getrawtransaction() { return RPCHelpMan{ @@ -1594,13 +1651,6 @@ static RPCHelpMan utxoupdatepsbt() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - // Unserialize the transactions - PartiallySignedTransaction psbtx; - std::string error; - if (!DecodeBase64PSBT(psbtx, request.params[0].get_str(), error)) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed %s", error)); - } - // Parse descriptors, if any. FlatSigningProvider provider; if (!request.params[1].isNull()) { @@ -1609,53 +1659,12 @@ static RPCHelpMan utxoupdatepsbt() EvalDescriptorStringOrObject(descs[i], provider); } } - // We don't actually need private keys further on; hide them as a precaution. - HidingSigningProvider public_provider(&provider, /*hide_secret=*/true, /*hide_origin=*/false); - - // Fetch previous transactions (inputs): - CCoinsView viewDummy; - CCoinsViewCache view(&viewDummy); - { - NodeContext& node = EnsureAnyNodeContext(request.context); - const CTxMemPool& mempool = EnsureMemPool(node); - ChainstateManager& chainman = EnsureChainman(node); - LOCK2(cs_main, mempool.cs); - CCoinsViewCache &viewChain = chainman.ActiveChainstate().CoinsTip(); - CCoinsViewMemPool viewMempool(&viewChain, mempool); - view.SetBackend(viewMempool); // temporarily switch cache backend to db+mempool view - - for (const CTxIn& txin : psbtx.tx->vin) { - view.AccessCoin(txin.prevout); // Load entries from viewChain into view; can fail. - } - - view.SetBackend(viewDummy); // switch back to avoid locking mempool for too long - } - - // Fill the inputs - const PrecomputedTransactionData txdata = PrecomputePSBTData(psbtx); - for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) { - PSBTInput& input = psbtx.inputs.at(i); - - if (input.non_witness_utxo || !input.witness_utxo.IsNull()) { - continue; - } - - const Coin& coin = view.AccessCoin(psbtx.tx->vin[i].prevout); - - if (IsSegWitOutput(provider, coin.out.scriptPubKey)) { - input.witness_utxo = coin.out; - } - - // Update script/keypath information using descriptor data. - // Note that SignPSBTInput does a lot more than just constructing ECDSA signatures - // we don't actually care about those here, in fact. - SignPSBTInput(public_provider, psbtx, i, &txdata, /*sighash=*/1); - } - // Update script/keypath information using descriptor data. - for (unsigned int i = 0; i < psbtx.tx->vout.size(); ++i) { - UpdatePSBTOutput(public_provider, psbtx, i); - } + // We don't actually need private keys further on; hide them as a precaution. + const PartiallySignedTransaction& psbtx = ProcessPSBT( + request.params[0].get_str(), + request.context, + HidingSigningProvider(&provider, /*hide_secret=*/true, /*hide_origin=*/false)); CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION); ssTx << psbtx; From c4981e7f63a3e0aeec1ef3dec040261e993dd724 Mon Sep 17 00:00:00 2001 From: mruddy <6440430+mruddy@users.noreply.github.com> Date: Sun, 24 Apr 2022 08:00:38 -0400 Subject: [PATCH 014/272] prune, import: fixes #23852 allows pruning to work during the loadblock import process. --- src/validation.cpp | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index c839647b29533..3714335b39869 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4474,6 +4474,9 @@ void Chainstate::LoadExternalBlockFile( // next block, but it's still possible to rewind to the start of the current block (without a disk read). nRewind = nBlockPos + nSize; blkdat.SkipTo(nRewind); + + std::shared_ptr pblock{}; // needs to remain available after the cs_main lock is released to avoid duplicate reads from disk + { LOCK(cs_main); // detect out of order blocks, and store them for later @@ -4491,7 +4494,7 @@ void Chainstate::LoadExternalBlockFile( if (!pindex || (pindex->nStatus & BLOCK_HAVE_DATA) == 0) { // This block can be processed immediately; rewind to its start, read and deserialize it. blkdat.SetPos(nBlockPos); - std::shared_ptr pblock{std::make_shared()}; + pblock = std::make_shared(); blkdat >> *pblock; nRewind = blkdat.GetPos(); @@ -4515,6 +4518,21 @@ void Chainstate::LoadExternalBlockFile( } } + if (m_blockman.IsPruneMode() && !fReindex && pblock) { + // must update the tip for pruning to work while importing with -loadblock. + // this is a tradeoff to conserve disk space at the expense of time + // spent updating the tip to be able to prune. + // otherwise, ActivateBestChain won't be called by the import process + // until after all of the block files are loaded. ActivateBestChain can be + // called by concurrent network message processing. but, that is not + // reliable for the purpose of pruning while importing. + BlockValidationState state; + if (!ActivateBestChain(state, pblock)) { + LogPrint(BCLog::REINDEX, "failed to activate chain (%s)\n", state.ToString()); + break; + } + } + NotifyHeaderTip(*this); if (!blocks_with_unknown_parent) continue; From 9bf078f66c8f286e1ab5e34b8eeed7d80290a897 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 17:38:58 -0700 Subject: [PATCH 015/272] refactor: update Select_ function Extract the logic that decides whether the new or the tried table is going to be searched to the beginning of the function. Co-authored-by: Martin Zumsande --- src/addrman.cpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index f5ca9a5c3431d..ec5b0213b3c20 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -719,12 +719,21 @@ std::pair AddrManImpl::Select_(bool newOnly) const AssertLockHeld(cs); if (vRandom.empty()) return {}; - if (newOnly && nNew == 0) return {}; + // Decide if we are going to search the new or tried table + bool search_tried; + // Use a 50% chance for choosing between tried and new table entries. if (!newOnly && - (nTried > 0 && (nNew == 0 || insecure_rand.randbool() == 0))) { + (nTried > 0 && + (nNew == 0 || insecure_rand.randbool() == 0))) { + search_tried = true; + } else { + search_tried = false; + } + + if (search_tried) { // use a tried node double fChanceFactor = 1.0; while (1) { From 1284223691127e76135a46d251c52416104f0ff1 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 4 Jan 2023 10:22:53 -0300 Subject: [PATCH 016/272] wallet: refactor coin selection algos to return util::Result so the selection processes can retrieve different errors and not uninformative std::nullopt --- src/wallet/coinselection.cpp | 14 +++++++------- src/wallet/coinselection.h | 7 ++++--- src/wallet/spend.cpp | 19 ++++++++++++++----- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 3fd0280b8b549..5ea9b7fad39fc 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -63,7 +63,7 @@ struct { static const size_t TOTAL_TRIES = 100000; -std::optional SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change) +util::Result SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change) { SelectionResult result(selection_target, SelectionAlgorithm::BNB); CAmount curr_value = 0; @@ -78,7 +78,7 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo curr_available_value += utxo.GetSelectionAmount(); } if (curr_available_value < selection_target) { - return std::nullopt; + return util::Error(); } // Sort the utxo_pool @@ -152,7 +152,7 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo // Check for solution if (best_selection.empty()) { - return std::nullopt; + return util::Error(); } // Set output set @@ -165,7 +165,7 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo return result; } -std::optional SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, FastRandomContext& rng) +util::Result SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, FastRandomContext& rng) { SelectionResult result(target_value, SelectionAlgorithm::SRD); @@ -190,7 +190,7 @@ std::optional SelectCoinsSRD(const std::vector& ut return result; } } - return std::nullopt; + return util::Error(); } /** Find a subset of the OutputGroups that is at least as large as, but as close as possible to, the @@ -252,7 +252,7 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v } } -std::optional KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, +util::Result KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, CAmount change_target, FastRandomContext& rng) { SelectionResult result(nTargetValue, SelectionAlgorithm::KNAPSACK); @@ -286,7 +286,7 @@ std::optional KnapsackSolver(std::vector& groups, } if (nTotalLower < nTargetValue) { - if (!lowest_larger) return std::nullopt; + if (!lowest_larger) return util::Error(); result.AddInput(*lowest_larger); return result; } diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 3abd22c2071a9..95a3bb2bc82cf 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -13,6 +13,7 @@ #include #include #include +#include #include @@ -408,7 +409,7 @@ struct SelectionResult int GetWeight() const { return m_weight; } }; -std::optional SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change); +util::Result SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change); /** Select coins by Single Random Draw. OutputGroups are selected randomly from the eligible * outputs until the target is satisfied @@ -417,10 +418,10 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo * @param[in] target_value The target value to select for * @returns If successful, a SelectionResult, otherwise, std::nullopt */ -std::optional SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, FastRandomContext& rng); +util::Result SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, FastRandomContext& rng); // Original coin selection algorithm as a fallback -std::optional KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, +util::Result KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, CAmount change_target, FastRandomContext& rng); } // namespace wallet diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index a8ecce119aff5..a55e4c45a8fa6 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -554,25 +554,34 @@ util::Result ChooseSelectionResult(const CAmount& nTargetValue, { // Vector of results. We will choose the best one based on waste. std::vector results; + std::vector> errors; + auto append_error = [&] (const util::Result& result) { + // If any specific error message appears here, then something different from a simple "no selection found" happened. + // Let's save it, so it can be retrieved to the user if no other selection algorithm succeeded. + if (HasErrorMsg(result)) { + errors.emplace_back(result); + } + }; if (auto bnb_result{SelectCoinsBnB(groups.positive_group, nTargetValue, coin_selection_params.m_cost_of_change)}) { results.push_back(*bnb_result); - } + } else append_error(bnb_result); // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here. if (auto knapsack_result{KnapsackSolver(groups.mixed_group, nTargetValue, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast)}) { knapsack_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); results.push_back(*knapsack_result); - } + } else append_error(knapsack_result); if (auto srd_result{SelectCoinsSRD(groups.positive_group, nTargetValue, coin_selection_params.rng_fast)}) { srd_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); results.push_back(*srd_result); - } + } else append_error(srd_result); if (results.empty()) { - // No solution found - return util::Error(); + // No solution found, retrieve the first explicit error (if any). + // future: add 'severity level' to errors so the worst one can be retrieved instead of the first one. + return errors.empty() ? util::Error() : errors.front(); } std::vector eligible_results; From 9c18992bbaf649f8c5461d5e4dc39eb1a07ffc77 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Tue, 29 Nov 2022 15:34:59 -0300 Subject: [PATCH 017/272] test: add coverage for `-bantime` --- test/functional/rpc_setban.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/functional/rpc_setban.py b/test/functional/rpc_setban.py index 97354f480c2eb..b4f3d77e5b73a 100755 --- a/test/functional/rpc_setban.py +++ b/test/functional/rpc_setban.py @@ -6,7 +6,8 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( - p2p_port + p2p_port, + assert_equal, ) class SetBanTests(BitcoinTestFramework): @@ -70,6 +71,11 @@ def run_test(self): assert not self.is_banned(node, tor_addr) assert not self.is_banned(node, ip_addr) + self.log.info("Test -bantime") + self.restart_node(1, ["-bantime=1234"]) + self.nodes[1].setban("127.0.0.1", "add") + banned = self.nodes[1].listbanned()[0] + assert_equal(banned['ban_duration'], 1234) if __name__ == '__main__': SetBanTests().main() From 55c4795c5794c5c2f8a69b394b847413c9cfbe36 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Thu, 16 Mar 2023 18:17:48 +0100 Subject: [PATCH 018/272] [net processing] Use TxRelay::m_relay_txs over CNode::m_relays_txs --- src/net_processing.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 25c65c70903f4..41e290bff5c07 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3296,11 +3296,13 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (greatest_common_version >= WTXID_RELAY_VERSION && m_txreconciliation) { // Per BIP-330, we announce txreconciliation support if: // - protocol version per the peer's VERSION message supports WTXID_RELAY; - // - transaction relay is supported per the peer's VERSION message (see m_relays_txs); - // - this is not a block-relay-only connection and not a feeler (see m_relays_txs); + // - transaction relay is supported per the peer's VERSION message + // - this is not a block-relay-only connection and not a feeler // - this is not an addr fetch connection; // - we are not in -blocksonly mode. - if (pfrom.m_relays_txs && !pfrom.IsAddrFetchConn() && !m_ignore_incoming_txs) { + const auto* tx_relay = peer->GetTxRelay(); + if (tx_relay && WITH_LOCK(tx_relay->m_bloom_filter_mutex, return tx_relay->m_relay_txs) && + !pfrom.IsAddrFetchConn() && !m_ignore_incoming_txs) { const uint64_t recon_salt = m_txreconciliation->PreRegisterPeer(pfrom.GetId()); m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDTXRCNCL, TXRECONCILIATION_VERSION, recon_salt)); @@ -3529,7 +3531,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // Peer must not offer us reconciliations if they specified no tx relay support in VERSION. // This flag might also be false in other cases, but the RejectIncomingTxs check above // eliminates them, so that this flag fully represents what we are looking for. - if (!pfrom.m_relays_txs) { + const auto* tx_relay = peer->GetTxRelay(); + if (!tx_relay || !WITH_LOCK(tx_relay->m_bloom_filter_mutex, return tx_relay->m_relay_txs)) { LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "sendtxrcncl received from peer=%d which indicated no tx relay to us; disconnecting\n", pfrom.GetId()); pfrom.fDisconnect = true; return; From 052fbcd5a791855406141e85d32e42e297220fe9 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 17:46:13 -0700 Subject: [PATCH 019/272] addrman: Introduce helper to generalize looking up an addrman entry Unused until later commit. Co-authored-by: Martin Zumsande --- src/addrman.cpp | 15 +++++++++++++++ src/addrman_impl.h | 6 ++++++ 2 files changed, 21 insertions(+) diff --git a/src/addrman.cpp b/src/addrman.cpp index ec5b0213b3c20..966cf6b04323c 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -792,6 +792,21 @@ std::pair AddrManImpl::Select_(bool newOnly) const } } +int AddrManImpl::GetEntry(bool use_tried, size_t bucket, size_t position) const +{ + AssertLockHeld(cs); + + assert(position < ADDRMAN_BUCKET_SIZE); + + if (use_tried) { + assert(bucket < ADDRMAN_TRIED_BUCKET_COUNT); + return vvTried[bucket][position]; + } else { + assert(bucket < ADDRMAN_NEW_BUCKET_COUNT); + return vvNew[bucket][position]; + } +} + std::vector AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional network) const { AssertLockHeld(cs); diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 94fe81aca9bc3..5410c3342c55d 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -253,6 +253,12 @@ class AddrManImpl std::pair Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs); + /** Helper to generalize looking up an addrman entry from either table. + * + * @return int The nid of the entry or -1 if the addrman position is empty. + * */ + int GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::vector GetAddr_(size_t max_addresses, size_t max_pct, std::optional network) const EXCLUSIVE_LOCKS_REQUIRED(cs); void Connected_(const CService& addr, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs); From ca2a9c5f8f14b792a14e81f73b1910a4c8799b93 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 18:01:18 -0700 Subject: [PATCH 020/272] refactor: generalize select logic in preparation for consolidating the logic for searching the new and tried tables, generalize the call paths for both Co-authored-by: Martin Zumsande --- src/addrman.cpp | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 966cf6b04323c..afaa040b0f183 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -723,14 +723,17 @@ std::pair AddrManImpl::Select_(bool newOnly) const // Decide if we are going to search the new or tried table bool search_tried; + int bucket_count; // Use a 50% chance for choosing between tried and new table entries. if (!newOnly && (nTried > 0 && (nNew == 0 || insecure_rand.randbool() == 0))) { search_tried = true; + bucket_count = ADDRMAN_TRIED_BUCKET_COUNT; } else { search_tried = false; + bucket_count = ADDRMAN_NEW_BUCKET_COUNT; } if (search_tried) { @@ -738,18 +741,21 @@ std::pair AddrManImpl::Select_(bool newOnly) const double fChanceFactor = 1.0; while (1) { // Pick a tried bucket, and an initial position in that bucket. - int nKBucket = insecure_rand.randrange(ADDRMAN_TRIED_BUCKET_COUNT); + int nKBucket = insecure_rand.randrange(bucket_count); int nKBucketPos = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE); // Iterate over the positions of that bucket, starting at the initial one, // and looping around. int i; for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) { - if (vvTried[nKBucket][(nKBucketPos + i) % ADDRMAN_BUCKET_SIZE] != -1) break; + int position = (nKBucketPos + i) % ADDRMAN_BUCKET_SIZE; + int node_id = GetEntry(search_tried, nKBucket, position); + if (node_id != -1) break; } // If the bucket is entirely empty, start over with a (likely) different one. if (i == ADDRMAN_BUCKET_SIZE) continue; // Find the entry to return. - int nId = vvTried[nKBucket][(nKBucketPos + i) % ADDRMAN_BUCKET_SIZE]; + int position = (nKBucketPos + i) % ADDRMAN_BUCKET_SIZE; + int nId = GetEntry(search_tried, nKBucket, position); const auto it_found{mapInfo.find(nId)}; assert(it_found != mapInfo.end()); const AddrInfo& info{it_found->second}; @@ -766,18 +772,21 @@ std::pair AddrManImpl::Select_(bool newOnly) const double fChanceFactor = 1.0; while (1) { // Pick a new bucket, and an initial position in that bucket. - int nUBucket = insecure_rand.randrange(ADDRMAN_NEW_BUCKET_COUNT); + int nUBucket = insecure_rand.randrange(bucket_count); int nUBucketPos = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE); // Iterate over the positions of that bucket, starting at the initial one, // and looping around. int i; for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) { - if (vvNew[nUBucket][(nUBucketPos + i) % ADDRMAN_BUCKET_SIZE] != -1) break; + int position = (nUBucketPos + i) % ADDRMAN_BUCKET_SIZE; + int node_id = GetEntry(search_tried, nUBucket, position); + if (node_id != -1) break; } // If the bucket is entirely empty, start over with a (likely) different one. if (i == ADDRMAN_BUCKET_SIZE) continue; // Find the entry to return. - int nId = vvNew[nUBucket][(nUBucketPos + i) % ADDRMAN_BUCKET_SIZE]; + int position = (nUBucketPos + i) % ADDRMAN_BUCKET_SIZE; + int nId = GetEntry(search_tried, nUBucket, position); const auto it_found{mapInfo.find(nId)}; assert(it_found != mapInfo.end()); const AddrInfo& info{it_found->second}; From 48806412e2bcd023b78fc05f6c9ce092360d1db1 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 18:13:00 -0700 Subject: [PATCH 021/272] refactor: consolidate select logic for new and tried tables Co-authored-by: Martin Zumsande --- src/addrman.cpp | 91 +++++++++++++++++-------------------------------- 1 file changed, 31 insertions(+), 60 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index afaa040b0f183..69ad13a912edd 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -736,68 +736,39 @@ std::pair AddrManImpl::Select_(bool newOnly) const bucket_count = ADDRMAN_NEW_BUCKET_COUNT; } - if (search_tried) { - // use a tried node - double fChanceFactor = 1.0; - while (1) { - // Pick a tried bucket, and an initial position in that bucket. - int nKBucket = insecure_rand.randrange(bucket_count); - int nKBucketPos = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE); - // Iterate over the positions of that bucket, starting at the initial one, - // and looping around. - int i; - for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) { - int position = (nKBucketPos + i) % ADDRMAN_BUCKET_SIZE; - int node_id = GetEntry(search_tried, nKBucket, position); - if (node_id != -1) break; - } - // If the bucket is entirely empty, start over with a (likely) different one. - if (i == ADDRMAN_BUCKET_SIZE) continue; - // Find the entry to return. - int position = (nKBucketPos + i) % ADDRMAN_BUCKET_SIZE; - int nId = GetEntry(search_tried, nKBucket, position); - const auto it_found{mapInfo.find(nId)}; - assert(it_found != mapInfo.end()); - const AddrInfo& info{it_found->second}; - // With probability GetChance() * fChanceFactor, return the entry. - if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) { - LogPrint(BCLog::ADDRMAN, "Selected %s from tried\n", info.ToStringAddrPort()); - return {info, info.m_last_try}; - } - // Otherwise start over with a (likely) different bucket, and increased chance factor. - fChanceFactor *= 1.2; + double fChanceFactor = 1.0; + while (1) { + // Pick a bucket, and an initial position in that bucket. + int nBucket = insecure_rand.randrange(bucket_count); + int nBucketPos = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE); + + // Iterate over the positions of that bucket, starting at the initial one, + // and looping around. + int i; + for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) { + int position = (nBucketPos + i) % ADDRMAN_BUCKET_SIZE; + int node_id = GetEntry(search_tried, nBucket, position); + if (node_id != -1) break; } - } else { - // use a new node - double fChanceFactor = 1.0; - while (1) { - // Pick a new bucket, and an initial position in that bucket. - int nUBucket = insecure_rand.randrange(bucket_count); - int nUBucketPos = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE); - // Iterate over the positions of that bucket, starting at the initial one, - // and looping around. - int i; - for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) { - int position = (nUBucketPos + i) % ADDRMAN_BUCKET_SIZE; - int node_id = GetEntry(search_tried, nUBucket, position); - if (node_id != -1) break; - } - // If the bucket is entirely empty, start over with a (likely) different one. - if (i == ADDRMAN_BUCKET_SIZE) continue; - // Find the entry to return. - int position = (nUBucketPos + i) % ADDRMAN_BUCKET_SIZE; - int nId = GetEntry(search_tried, nUBucket, position); - const auto it_found{mapInfo.find(nId)}; - assert(it_found != mapInfo.end()); - const AddrInfo& info{it_found->second}; - // With probability GetChance() * fChanceFactor, return the entry. - if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) { - LogPrint(BCLog::ADDRMAN, "Selected %s from new\n", info.ToStringAddrPort()); - return {info, info.m_last_try}; - } - // Otherwise start over with a (likely) different bucket, and increased chance factor. - fChanceFactor *= 1.2; + + // If the bucket is entirely empty, start over with a (likely) different one. + if (i == ADDRMAN_BUCKET_SIZE) continue; + + // Find the entry to return. + int position = (nBucketPos + i) % ADDRMAN_BUCKET_SIZE; + int nId = GetEntry(search_tried, nBucket, position); + const auto it_found{mapInfo.find(nId)}; + assert(it_found != mapInfo.end()); + const AddrInfo& info{it_found->second}; + + // With probability GetChance() * fChanceFactor, return the entry. + if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) { + LogPrint(BCLog::ADDRMAN, "Selected %s from %s\n", info.ToStringAddrPort(), search_tried ? "tried" : "new"); + return {info, info.m_last_try}; } + + // Otherwise start over with a (likely) different bucket, and increased chance factor. + fChanceFactor *= 1.2; } } From 26c3bf11e2487ed0ac578fb92619c148336003cb Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sun, 19 Feb 2023 10:09:28 -0700 Subject: [PATCH 022/272] scripted-diff: rename local variables to match modern conventions -BEGIN VERIFY SCRIPT- sed -i 's/fChanceFactor/chance_factor/g' src/addrman.cpp sed -i 's/nBucketPos/initial_position/g' src/addrman.cpp sed -i 's/nBucket/bucket/g' src/addrman.cpp src/addrman_impl.h sed -i 's/newOnly/new_only/g' src/addrman.cpp src/addrman_impl.h src/addrman.h src/test/addrman_tests.cpp -END VERIFY SCRIPT- Co-authored-by: Martin Zumsande --- src/addrman.cpp | 38 +++++++++++++++++++------------------- src/addrman.h | 4 ++-- src/addrman_impl.h | 6 +++--- src/test/addrman_tests.cpp | 10 +++++----- 4 files changed, 29 insertions(+), 29 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 69ad13a912edd..fe4a79b2e6639 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -58,9 +58,9 @@ int AddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const NetGr return hash2 % ADDRMAN_NEW_BUCKET_COUNT; } -int AddrInfo::GetBucketPosition(const uint256& nKey, bool fNew, int nBucket) const +int AddrInfo::GetBucketPosition(const uint256& nKey, bool fNew, int bucket) const { - uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << (fNew ? uint8_t{'N'} : uint8_t{'K'}) << nBucket << GetKey()).GetCheapHash(); + uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << (fNew ? uint8_t{'N'} : uint8_t{'K'}) << bucket << GetKey()).GetCheapHash(); return hash1 % ADDRMAN_BUCKET_SIZE; } @@ -714,19 +714,19 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, NodeSeconds } } -std::pair AddrManImpl::Select_(bool newOnly) const +std::pair AddrManImpl::Select_(bool new_only) const { AssertLockHeld(cs); if (vRandom.empty()) return {}; - if (newOnly && nNew == 0) return {}; + if (new_only && nNew == 0) return {}; // Decide if we are going to search the new or tried table bool search_tried; int bucket_count; // Use a 50% chance for choosing between tried and new table entries. - if (!newOnly && + if (!new_only && (nTried > 0 && (nNew == 0 || insecure_rand.randbool() == 0))) { search_tried = true; @@ -736,18 +736,18 @@ std::pair AddrManImpl::Select_(bool newOnly) const bucket_count = ADDRMAN_NEW_BUCKET_COUNT; } - double fChanceFactor = 1.0; + double chance_factor = 1.0; while (1) { // Pick a bucket, and an initial position in that bucket. - int nBucket = insecure_rand.randrange(bucket_count); - int nBucketPos = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE); + int bucket = insecure_rand.randrange(bucket_count); + int initial_position = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE); // Iterate over the positions of that bucket, starting at the initial one, // and looping around. int i; for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) { - int position = (nBucketPos + i) % ADDRMAN_BUCKET_SIZE; - int node_id = GetEntry(search_tried, nBucket, position); + int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE; + int node_id = GetEntry(search_tried, bucket, position); if (node_id != -1) break; } @@ -755,20 +755,20 @@ std::pair AddrManImpl::Select_(bool newOnly) const if (i == ADDRMAN_BUCKET_SIZE) continue; // Find the entry to return. - int position = (nBucketPos + i) % ADDRMAN_BUCKET_SIZE; - int nId = GetEntry(search_tried, nBucket, position); + int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE; + int nId = GetEntry(search_tried, bucket, position); const auto it_found{mapInfo.find(nId)}; assert(it_found != mapInfo.end()); const AddrInfo& info{it_found->second}; - // With probability GetChance() * fChanceFactor, return the entry. - if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) { + // With probability GetChance() * chance_factor, return the entry. + if (insecure_rand.randbits(30) < chance_factor * info.GetChance() * (1 << 30)) { LogPrint(BCLog::ADDRMAN, "Selected %s from %s\n", info.ToStringAddrPort(), search_tried ? "tried" : "new"); return {info, info.m_last_try}; } // Otherwise start over with a (likely) different bucket, and increased chance factor. - fChanceFactor *= 1.2; + chance_factor *= 1.2; } } @@ -1168,11 +1168,11 @@ std::pair AddrManImpl::SelectTriedCollision() return ret; } -std::pair AddrManImpl::Select(bool newOnly) const +std::pair AddrManImpl::Select(bool new_only) const { LOCK(cs); Check(); - auto addrRet = Select_(newOnly); + auto addrRet = Select_(new_only); Check(); return addrRet; } @@ -1266,9 +1266,9 @@ std::pair AddrMan::SelectTriedCollision() return m_impl->SelectTriedCollision(); } -std::pair AddrMan::Select(bool newOnly) const +std::pair AddrMan::Select(bool new_only) const { - return m_impl->Select(newOnly); + return m_impl->Select(new_only); } std::vector AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const diff --git a/src/addrman.h b/src/addrman.h index 4985fc764cf4d..374996e501f7d 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -146,11 +146,11 @@ class AddrMan /** * Choose an address to connect to. * - * @param[in] newOnly Whether to only select addresses from the new table. + * @param[in] new_only Whether to only select addresses from the new table. * @return CAddress The record for the selected peer. * seconds The last time we attempted to connect to that peer. */ - std::pair Select(bool newOnly = false) const; + std::pair Select(bool new_only = false) const; /** * Return all or many randomly selected addresses, optionally by network. diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 5410c3342c55d..3cf3b838d6882 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -90,7 +90,7 @@ class AddrInfo : public CAddress } //! Calculate in which position of a bucket to store this entry. - int GetBucketPosition(const uint256 &nKey, bool fNew, int nBucket) const; + int GetBucketPosition(const uint256 &nKey, bool fNew, int bucket) const; //! Determine whether the statistics about this entry are bad enough so that it can just be deleted bool IsTerrible(NodeSeconds now = Now()) const; @@ -127,7 +127,7 @@ class AddrManImpl std::pair SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs); - std::pair Select(bool newOnly) const + std::pair Select(bool new_only) const EXCLUSIVE_LOCKS_REQUIRED(!cs); std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const @@ -251,7 +251,7 @@ class AddrManImpl void Attempt_(const CService& addr, bool fCountFailure, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs); - std::pair Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::pair Select_(bool new_only) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** Helper to generalize looking up an addrman entry from either table. * diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 758691cfdea01..ad59e123d2a53 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -127,8 +127,8 @@ BOOST_AUTO_TEST_CASE(addrman_ports) // the specified port to tried, but not the other. addrman->Good(CAddress(addr1_port, NODE_NONE)); BOOST_CHECK_EQUAL(addrman->Size(), 2U); - bool newOnly = true; - auto addr_ret3 = addrman->Select(newOnly).first; + bool new_only = true; + auto addr_ret3 = addrman->Select(new_only).first; BOOST_CHECK_EQUAL(addr_ret3.ToStringAddrPort(), "250.1.1.1:8333"); } @@ -144,14 +144,14 @@ BOOST_AUTO_TEST_CASE(addrman_select) BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); BOOST_CHECK_EQUAL(addrman->Size(), 1U); - bool newOnly = true; - auto addr_ret1 = addrman->Select(newOnly).first; + bool new_only = true; + auto addr_ret1 = addrman->Select(new_only).first; BOOST_CHECK_EQUAL(addr_ret1.ToStringAddrPort(), "250.1.1.1:8333"); // Test: move addr to tried, select from new expected nothing returned. BOOST_CHECK(addrman->Good(CAddress(addr1, NODE_NONE))); BOOST_CHECK_EQUAL(addrman->Size(), 1U); - auto addr_ret2 = addrman->Select(newOnly).first; + auto addr_ret2 = addrman->Select(new_only).first; BOOST_CHECK_EQUAL(addr_ret2.ToStringAddrPort(), "[::]:0"); auto addr_ret3 = addrman->Select().first; From 6b229284fd2209938ee8fdffed4d080395b3aa05 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 18:29:45 -0700 Subject: [PATCH 023/272] addrman: add functionality to select by network Add an optional parameter to the addrman Select function that allows callers to specify which network the returned address should be on. Ensure that the proper table is selected with different cases of whether the new or tried table has network addresses that match. Co-authored-by: Martin Zumsande --- src/addrman.cpp | 54 ++++++++++++++++++++++++++++++++-------------- src/addrman.h | 5 +++-- src/addrman_impl.h | 4 ++-- 3 files changed, 43 insertions(+), 20 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index fe4a79b2e6639..cdfd079fcdfb0 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -714,28 +714,41 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, NodeSeconds } } -std::pair AddrManImpl::Select_(bool new_only) const +std::pair AddrManImpl::Select_(bool new_only, std::optional network) const { AssertLockHeld(cs); if (vRandom.empty()) return {}; - if (new_only && nNew == 0) return {}; + + size_t new_count = nNew; + size_t tried_count = nTried; + + if (network.has_value()) { + auto it = m_network_counts.find(*network); + if (it == m_network_counts.end()) return {}; + + auto counts = it->second; + new_count = counts.n_new; + tried_count = counts.n_tried; + } + + if (new_only && new_count == 0) return {}; + if (new_count + tried_count == 0) return {}; // Decide if we are going to search the new or tried table + // If either option is viable, use a 50% chance to choose bool search_tried; - int bucket_count; - - // Use a 50% chance for choosing between tried and new table entries. - if (!new_only && - (nTried > 0 && - (nNew == 0 || insecure_rand.randbool() == 0))) { + if (new_only || tried_count == 0) { + search_tried = false; + } else if (new_count == 0) { search_tried = true; - bucket_count = ADDRMAN_TRIED_BUCKET_COUNT; } else { - search_tried = false; - bucket_count = ADDRMAN_NEW_BUCKET_COUNT; + search_tried = insecure_rand.randbool(); } + const int bucket_count{search_tried ? ADDRMAN_TRIED_BUCKET_COUNT : ADDRMAN_NEW_BUCKET_COUNT}; + + // Loop through the addrman table until we find an appropriate entry double chance_factor = 1.0; while (1) { // Pick a bucket, and an initial position in that bucket. @@ -748,7 +761,16 @@ std::pair AddrManImpl::Select_(bool new_only) const for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) { int position = (initial_position + i) % ADDRMAN_BUCKET_SIZE; int node_id = GetEntry(search_tried, bucket, position); - if (node_id != -1) break; + if (node_id != -1) { + if (network.has_value()) { + const auto it{mapInfo.find(node_id)}; + assert(it != mapInfo.end()); + const auto info{it->second}; + if (info.GetNetwork() == *network) break; + } else { + break; + } + } } // If the bucket is entirely empty, start over with a (likely) different one. @@ -1168,11 +1190,11 @@ std::pair AddrManImpl::SelectTriedCollision() return ret; } -std::pair AddrManImpl::Select(bool new_only) const +std::pair AddrManImpl::Select(bool new_only, std::optional network) const { LOCK(cs); Check(); - auto addrRet = Select_(new_only); + auto addrRet = Select_(new_only, network); Check(); return addrRet; } @@ -1266,9 +1288,9 @@ std::pair AddrMan::SelectTriedCollision() return m_impl->SelectTriedCollision(); } -std::pair AddrMan::Select(bool new_only) const +std::pair AddrMan::Select(bool new_only, std::optional network) const { - return m_impl->Select(new_only); + return m_impl->Select(new_only, network); } std::vector AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const diff --git a/src/addrman.h b/src/addrman.h index 374996e501f7d..c8e31fe28333f 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -146,11 +146,12 @@ class AddrMan /** * Choose an address to connect to. * - * @param[in] new_only Whether to only select addresses from the new table. + * @param[in] new_only Whether to only select addresses from the new table. + * @param[in] network Select only addresses of this network (nullopt = all) * @return CAddress The record for the selected peer. * seconds The last time we attempted to connect to that peer. */ - std::pair Select(bool new_only = false) const; + std::pair Select(bool new_only = false, std::optional network = std::nullopt) const; /** * Return all or many randomly selected addresses, optionally by network. diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 3cf3b838d6882..7aead2812bae8 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -127,7 +127,7 @@ class AddrManImpl std::pair SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs); - std::pair Select(bool new_only) const + std::pair Select(bool new_only, std::optional network) const EXCLUSIVE_LOCKS_REQUIRED(!cs); std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const @@ -251,7 +251,7 @@ class AddrManImpl void Attempt_(const CService& addr, bool fCountFailure, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs); - std::pair Select_(bool new_only) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::pair Select_(bool new_only, std::optional network) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** Helper to generalize looking up an addrman entry from either table. * From 5c8b4baff27e0ccd27fda6e915b956d1e8dd7ce2 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 6 Feb 2023 20:15:50 -0700 Subject: [PATCH 024/272] tests: add addrman_select_by_network test this adds coverage for the 7 different cases of which table should be selected when the network is specified. the different cases are the result of new_only being true or false and whether there are network addresses on both, neither, or one of new vs tried tables. the only case not covered is when new_only is false and the only network addresses are on the new table. Co-authored-by: Martin Zumsande Co-authored-by: Vasil Dimov --- src/test/addrman_tests.cpp | 60 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index ad59e123d2a53..1acdb02c9a7a2 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -192,6 +192,66 @@ BOOST_AUTO_TEST_CASE(addrman_select) BOOST_CHECK_EQUAL(ports.size(), 3U); } +BOOST_AUTO_TEST_CASE(addrman_select_by_network) +{ + auto addrman = std::make_unique(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node)); + BOOST_CHECK(!addrman->Select(/*new_only=*/true, NET_IPV4).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_IPV4).first.IsValid()); + + // add ipv4 address to the new table + CNetAddr source = ResolveIP("252.2.2.2"); + CService addr1 = ResolveService("250.1.1.1", 8333); + BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); + + BOOST_CHECK(addrman->Select(/*new_only=*/true, NET_IPV4).first == addr1); + BOOST_CHECK(addrman->Select(/*new_only=*/false, NET_IPV4).first == addr1); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_IPV6).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_ONION).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_I2P).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_CJDNS).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/true, NET_CJDNS).first.IsValid()); + BOOST_CHECK(addrman->Select(/*new_only=*/false).first == addr1); + + // add I2P address to the new table + CAddress i2p_addr; + i2p_addr.SetSpecial("udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.i2p"); + BOOST_CHECK(addrman->Add({i2p_addr}, source)); + + BOOST_CHECK(addrman->Select(/*new_only=*/true, NET_I2P).first == i2p_addr); + BOOST_CHECK(addrman->Select(/*new_only=*/false, NET_I2P).first == i2p_addr); + BOOST_CHECK(addrman->Select(/*new_only=*/false, NET_IPV4).first == addr1); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_IPV6).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_ONION).first.IsValid()); + BOOST_CHECK(!addrman->Select(/*new_only=*/false, NET_CJDNS).first.IsValid()); + + // bump I2P address to tried table + BOOST_CHECK(addrman->Good(i2p_addr)); + + BOOST_CHECK(!addrman->Select(/*new_only=*/true, NET_I2P).first.IsValid()); + BOOST_CHECK(addrman->Select(/*new_only=*/false, NET_I2P).first == i2p_addr); + + // add another I2P address to the new table + CAddress i2p_addr2; + i2p_addr2.SetSpecial("c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p"); + BOOST_CHECK(addrman->Add({i2p_addr2}, source)); + + BOOST_CHECK(addrman->Select(/*new_only=*/true, NET_I2P).first == i2p_addr2); + + // ensure that both new and tried table are selected from + bool new_selected{false}; + bool tried_selected{false}; + + while (!new_selected || !tried_selected) { + const CAddress selected{addrman->Select(/*new_only=*/false, NET_I2P).first}; + BOOST_REQUIRE(selected == i2p_addr || selected == i2p_addr2); + if (selected == i2p_addr) { + tried_selected = true; + } else { + new_selected = true; + } + } +} + BOOST_AUTO_TEST_CASE(addrman_new_collisions) { auto addrman = std::make_unique(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node)); From a98e542e0c18f7cb2340179631806f14b07430c3 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 18:34:06 -0700 Subject: [PATCH 025/272] test: add addrman test for special case if an addr matching the network requirements is only on the new table and select is invoked with new_only = false, ensure that the code selects the new table. in order to test this case, we use a non deterministic addrman. this means we cannot have more than one address in any addrman table, or risk sporadic failures when the second address happens to conflict. if the code chose a table at random, the test would fail 50% of the time Co-authored-by: Martin Zumsande --- src/test/addrman_tests.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 1acdb02c9a7a2..c97a29eb1e846 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -252,6 +252,28 @@ BOOST_AUTO_TEST_CASE(addrman_select_by_network) } } +BOOST_AUTO_TEST_CASE(addrman_select_special) +{ + // use a non-deterministic addrman to ensure a passing test isn't due to setup + auto addrman = std::make_unique(EMPTY_NETGROUPMAN, /*deterministic=*/false, GetCheckRatio(m_node)); + + // add ipv4 address to the new table + CNetAddr source = ResolveIP("252.2.2.2"); + CService addr1 = ResolveService("250.1.1.3", 8333); + BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); + + // add I2P address to the tried table + CAddress i2p_addr; + i2p_addr.SetSpecial("udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.i2p"); + BOOST_CHECK(addrman->Add({i2p_addr}, source)); + BOOST_CHECK(addrman->Good(i2p_addr)); + + // since the only ipv4 address is on the new table, ensure that the new + // table gets selected even if new_only is false. if the table was being + // selected at random, this test will sporadically fail + BOOST_CHECK(addrman->Select(/*new_only=*/false, NET_IPV4).first == addr1); +} + BOOST_AUTO_TEST_CASE(addrman_new_collisions) { auto addrman = std::make_unique(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node)); From 22a4d1489c0678a90c00318203cfce61672f20b7 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 18:41:25 -0700 Subject: [PATCH 026/272] test: increase coverage of addrman select (without network) Co-authored-by: Martin Zumsande --- src/test/addrman_tests.cpp | 39 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index c97a29eb1e846..b5f9558337adc 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -132,41 +132,40 @@ BOOST_AUTO_TEST_CASE(addrman_ports) BOOST_CHECK_EQUAL(addr_ret3.ToStringAddrPort(), "250.1.1.1:8333"); } - BOOST_AUTO_TEST_CASE(addrman_select) { auto addrman = std::make_unique(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node)); + BOOST_CHECK(!addrman->Select(false).first.IsValid()); + BOOST_CHECK(!addrman->Select(true).first.IsValid()); CNetAddr source = ResolveIP("252.2.2.2"); - // Test: Select from new with 1 addr in new. + // Add 1 address to the new table CService addr1 = ResolveService("250.1.1.1", 8333); BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); BOOST_CHECK_EQUAL(addrman->Size(), 1U); - bool new_only = true; - auto addr_ret1 = addrman->Select(new_only).first; - BOOST_CHECK_EQUAL(addr_ret1.ToStringAddrPort(), "250.1.1.1:8333"); + BOOST_CHECK(addrman->Select(/*new_only=*/true).first == addr1); + BOOST_CHECK(addrman->Select(/*new_only=*/false).first == addr1); - // Test: move addr to tried, select from new expected nothing returned. + // Move address to the tried table BOOST_CHECK(addrman->Good(CAddress(addr1, NODE_NONE))); - BOOST_CHECK_EQUAL(addrman->Size(), 1U); - auto addr_ret2 = addrman->Select(new_only).first; - BOOST_CHECK_EQUAL(addr_ret2.ToStringAddrPort(), "[::]:0"); - - auto addr_ret3 = addrman->Select().first; - BOOST_CHECK_EQUAL(addr_ret3.ToStringAddrPort(), "250.1.1.1:8333"); + BOOST_CHECK_EQUAL(addrman->Size(), 1U); + BOOST_CHECK(!addrman->Select(/*new_only=*/true).first.IsValid()); + BOOST_CHECK(addrman->Select().first == addr1); BOOST_CHECK_EQUAL(addrman->Size(), 1U); - - // Add three addresses to new table. + // Add one address to the new table CService addr2 = ResolveService("250.3.1.1", 8333); + BOOST_CHECK(addrman->Add({CAddress(addr2, NODE_NONE)}, addr2)); + BOOST_CHECK(addrman->Select(/*new_only=*/true).first == addr2); + + // Add two more addresses to the new table CService addr3 = ResolveService("250.3.2.2", 9999); CService addr4 = ResolveService("250.3.3.3", 9999); - BOOST_CHECK(addrman->Add({CAddress(addr2, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); - BOOST_CHECK(addrman->Add({CAddress(addr3, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); + BOOST_CHECK(addrman->Add({CAddress(addr3, NODE_NONE)}, addr2)); BOOST_CHECK(addrman->Add({CAddress(addr4, NODE_NONE)}, ResolveService("250.4.1.1", 8333))); // Add three addresses to tried table. @@ -174,17 +173,17 @@ BOOST_AUTO_TEST_CASE(addrman_select) CService addr6 = ResolveService("250.4.5.5", 7777); CService addr7 = ResolveService("250.4.6.6", 8333); - BOOST_CHECK(addrman->Add({CAddress(addr5, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); + BOOST_CHECK(addrman->Add({CAddress(addr5, NODE_NONE)}, addr3)); BOOST_CHECK(addrman->Good(CAddress(addr5, NODE_NONE))); - BOOST_CHECK(addrman->Add({CAddress(addr6, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); + BOOST_CHECK(addrman->Add({CAddress(addr6, NODE_NONE)}, addr3)); BOOST_CHECK(addrman->Good(CAddress(addr6, NODE_NONE))); BOOST_CHECK(addrman->Add({CAddress(addr7, NODE_NONE)}, ResolveService("250.1.1.3", 8333))); BOOST_CHECK(addrman->Good(CAddress(addr7, NODE_NONE))); - // Test: 6 addrs + 1 addr from last test = 7. + // 6 addrs + 1 addr from last test = 7. BOOST_CHECK_EQUAL(addrman->Size(), 7U); - // Test: Select pulls from new and tried regardless of port number. + // Select pulls from new and tried regardless of port number. std::set ports; for (int i = 0; i < 20; ++i) { ports.insert(addrman->Select().first.GetPort()); From 9b91aae08579c77d2fd5506804c8e2e0cda0d274 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 17:03:56 -0700 Subject: [PATCH 027/272] bench: add coverage for addrman select with network parameter to evaluate the worst case performance with the network parameter passed through, fill the new table with addresses then add a singular I2P address to retrieve Co-authored-by: Martin Zumsande --- src/bench/addrman.cpp | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index d6b52eb587adb..b8c69d0a63793 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -71,6 +72,13 @@ static void FillAddrMan(AddrMan& addrman) AddAddressesToAddrMan(addrman); } +static CNetAddr ResolveIP(const std::string& ip) +{ + CNetAddr addr; + LookupHost(ip, addr, false); + return addr; +} + /* Benchmarks */ static void AddrManAdd(benchmark::Bench& bench) @@ -95,6 +103,25 @@ static void AddrManSelect(benchmark::Bench& bench) }); } +static void AddrManSelectByNetwork(benchmark::Bench& bench) +{ + AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO}; + + // add single I2P address to new table + CService i2p_service; + i2p_service.SetSpecial("udhdrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.i2p"); + CAddress i2p_address(i2p_service, NODE_NONE); + i2p_address.nTime = Now(); + CNetAddr source = ResolveIP("252.2.2.2"); + addrman.Add({i2p_address}, source); + + FillAddrMan(addrman); + + bench.run([&] { + (void)addrman.Select(/*new_only=*/false, NET_I2P); + }); +} + static void AddrManGetAddr(benchmark::Bench& bench) { AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO}; @@ -135,5 +162,6 @@ static void AddrManAddThenGood(benchmark::Bench& bench) BENCHMARK(AddrManAdd, benchmark::PriorityLevel::HIGH); BENCHMARK(AddrManSelect, benchmark::PriorityLevel::HIGH); +BENCHMARK(AddrManSelectByNetwork, benchmark::PriorityLevel::HIGH); BENCHMARK(AddrManGetAddr, benchmark::PriorityLevel::HIGH); BENCHMARK(AddrManAddThenGood, benchmark::PriorityLevel::HIGH); From b0010c83a1b4a3d21719cb68e37faf9b1172522a Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Thu, 23 Feb 2023 13:53:52 -0800 Subject: [PATCH 028/272] bench: test select for a new table with only one address the addrman select function will demonstrate it's worst case performance when it is almost empty, because it might have to linearly search several buckets. add a bench test to cover this case Co-authored-by: Martin Zumsande --- src/bench/addrman.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index b8c69d0a63793..8a5cab443f2cc 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -79,6 +79,13 @@ static CNetAddr ResolveIP(const std::string& ip) return addr; } +static CService ResolveService(const std::string& ip, uint16_t port = 0) +{ + CService serv; + Lookup(ip, serv, port, false); + return serv; +} + /* Benchmarks */ static void AddrManAdd(benchmark::Bench& bench) @@ -103,6 +110,22 @@ static void AddrManSelect(benchmark::Bench& bench) }); } +// The worst case performance of the Select() function is when there is only +// one address on the table, because it linearly searches every position of +// several buckets before identifying the correct bucket +static void AddrManSelectFromAlmostEmpty(benchmark::Bench& bench) +{ + AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO}; + + // Add one address to the new table + CService addr = ResolveService("250.3.1.1", 8333); + addrman.Add({CAddress(addr, NODE_NONE)}, ResolveService("250.3.1.1", 8333)); + + bench.run([&] { + (void)addrman.Select(); + }); +} + static void AddrManSelectByNetwork(benchmark::Bench& bench) { AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO}; @@ -162,6 +185,7 @@ static void AddrManAddThenGood(benchmark::Bench& bench) BENCHMARK(AddrManAdd, benchmark::PriorityLevel::HIGH); BENCHMARK(AddrManSelect, benchmark::PriorityLevel::HIGH); +BENCHMARK(AddrManSelectFromAlmostEmpty, benchmark::PriorityLevel::HIGH); BENCHMARK(AddrManSelectByNetwork, benchmark::PriorityLevel::HIGH); BENCHMARK(AddrManGetAddr, benchmark::PriorityLevel::HIGH); BENCHMARK(AddrManAddThenGood, benchmark::PriorityLevel::HIGH); From 17e705428ddf80c7a7f31fe5430d966cf08a37d6 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Fri, 10 Mar 2023 18:25:59 -0800 Subject: [PATCH 029/272] doc: clarify new_only param for Select function Co-authored-by: Martin Zumsande --- src/addrman.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/addrman.h b/src/addrman.h index c8e31fe28333f..6284b80a52e56 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -146,7 +146,9 @@ class AddrMan /** * Choose an address to connect to. * - * @param[in] new_only Whether to only select addresses from the new table. + * @param[in] new_only Whether to only select addresses from the new table. Passing `true` returns + * an address from the new table or an empty pair. Passing `false` will return an + * address from either the new or tried table (it does not guarantee a tried entry). * @param[in] network Select only addresses of this network (nullopt = all) * @return CAddress The record for the selected peer. * seconds The last time we attempted to connect to that peer. From 6e9f8bb050785dbc754b6bb493aad9139908ef98 Mon Sep 17 00:00:00 2001 From: ishaanam Date: Tue, 23 Aug 2022 15:24:00 -0400 Subject: [PATCH 030/272] rpc, tests: in `utxoupdatepsbt` also look for the transaction in the txindex Previously only the segwit utxos being spent by the psbt were looked for and added to the psbt. Now, the full transaction corresponding to each of these utxos (legacy and segwit) is looked for in the txindex and mempool and added to the psbt. If txindex is disabled and the transaction is not in the mempool, then we fall back to getting just the utxo (if segwit) from the utxo set. --- src/psbt.cpp | 32 ++++++++++++++ src/psbt.h | 3 ++ src/rpc/rawtransaction.cpp | 84 ++++++++++++++++++++++++------------- src/wallet/wallet.cpp | 29 +------------ test/functional/rpc_psbt.py | 12 +++--- 5 files changed, 98 insertions(+), 62 deletions(-) diff --git a/src/psbt.cpp b/src/psbt.cpp index 50ccd9e2c0398..16f4fa857c159 100644 --- a/src/psbt.cpp +++ b/src/psbt.cpp @@ -430,6 +430,38 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& return sig_complete; } +void RemoveUnnecessaryTransactions(PartiallySignedTransaction& psbtx, const int& sighash_type) +{ + // Only drop non_witness_utxos if sighash_type != SIGHASH_ANYONECANPAY + if ((sighash_type & 0x80) != SIGHASH_ANYONECANPAY) { + // Figure out if any non_witness_utxos should be dropped + std::vector to_drop; + for (unsigned int i = 0; i < psbtx.inputs.size(); ++i) { + const auto& input = psbtx.inputs.at(i); + int wit_ver; + std::vector wit_prog; + if (input.witness_utxo.IsNull() || !input.witness_utxo.scriptPubKey.IsWitnessProgram(wit_ver, wit_prog)) { + // There's a non-segwit input or Segwit v0, so we cannot drop any witness_utxos + to_drop.clear(); + break; + } + if (wit_ver == 0) { + // Segwit v0, so we cannot drop any non_witness_utxos + to_drop.clear(); + break; + } + if (input.non_witness_utxo) { + to_drop.push_back(i); + } + } + + // Drop the non_witness_utxos that we can drop + for (unsigned int i : to_drop) { + psbtx.inputs.at(i).non_witness_utxo = nullptr; + } + } +} + bool FinalizePSBT(PartiallySignedTransaction& psbtx) { // Finalize input signatures -- in case we have partial signatures that add up to a complete diff --git a/src/psbt.h b/src/psbt.h index 40d69cd4545c5..0a581933f0e26 100644 --- a/src/psbt.h +++ b/src/psbt.h @@ -1231,6 +1231,9 @@ bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, unsigned **/ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, int sighash = SIGHASH_ALL, SignatureData* out_sigdata = nullptr, bool finalize = true); +/** Reduces the size of the PSBT by dropping unnecessary `non_witness_utxos` (i.e. complete previous transactions) from a psbt when all inputs are segwit v1. */ +void RemoveUnnecessaryTransactions(PartiallySignedTransaction& psbtx, const int& sighash_type); + /** Counts the unsigned inputs of a PSBT. */ size_t CountPSBTUnsignedInputs(const PartiallySignedTransaction& psbt); diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 011983a158c42..879e847acb347 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -169,7 +169,7 @@ static std::vector CreateTxDoc() }; } -// Update PSBT with information from the mempool, the UTXO set, and the provided descriptors +// Update PSBT with information from the mempool, the UTXO set, the txindex, and the provided descriptors PartiallySignedTransaction ProcessPSBT(const std::string& psbt_string, const std::any& context, const HidingSigningProvider& provider) { // Unserialize the transactions @@ -179,50 +179,78 @@ PartiallySignedTransaction ProcessPSBT(const std::string& psbt_string, const std throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed %s", error)); } - // Fetch previous transactions (inputs): - CCoinsView viewDummy; - CCoinsViewCache view(&viewDummy); - { - NodeContext& node = EnsureAnyNodeContext(context); - const CTxMemPool& mempool = EnsureMemPool(node); - ChainstateManager& chainman = EnsureChainman(node); - LOCK2(cs_main, mempool.cs); - CCoinsViewCache &viewChain = chainman.ActiveChainstate().CoinsTip(); - CCoinsViewMemPool viewMempool(&viewChain, mempool); - view.SetBackend(viewMempool); // temporarily switch cache backend to db+mempool view - - for (const CTxIn& txin : psbtx.tx->vin) { - view.AccessCoin(txin.prevout); // Load entries from viewChain into view; can fail. - } + if (g_txindex) g_txindex->BlockUntilSyncedToCurrentChain(); + const NodeContext& node = EnsureAnyNodeContext(context); - view.SetBackend(viewDummy); // switch back to avoid locking mempool for too long - } + // If we can't find the corresponding full transaction for all of our inputs, + // this will be used to find just the utxos for the segwit inputs for which + // the full transaction isn't found + std::map coins; - // Fill the inputs - const PrecomputedTransactionData txdata = PrecomputePSBTData(psbtx); + // Fetch previous transactions: + // First, look in the txindex and the mempool for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) { - PSBTInput& input = psbtx.inputs.at(i); + PSBTInput& psbt_input = psbtx.inputs.at(i); + const CTxIn& tx_in = psbtx.tx->vin.at(i); - if (input.non_witness_utxo || !input.witness_utxo.IsNull()) { - continue; + // The `non_witness_utxo` is the whole previous transaction + if (psbt_input.non_witness_utxo) continue; + + CTransactionRef tx; + + // Look in the txindex + if (g_txindex) { + uint256 block_hash; + g_txindex->FindTx(tx_in.prevout.hash, block_hash, tx); + } + // If we still don't have it look in the mempool + if (!tx) { + tx = node.mempool->get(tx_in.prevout.hash); + } + if (tx) { + psbt_input.non_witness_utxo = tx; + } else { + coins[tx_in.prevout]; // Create empty map entry keyed by prevout + } + } + + // If we still haven't found all of the inputs, look for the missing ones in the utxo set + if (!coins.empty()) { + FindCoins(node, coins); + for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) { + PSBTInput& input = psbtx.inputs.at(i); + + // If there are still missing utxos, add them if they were found in the utxo set + if (!input.non_witness_utxo) { + const CTxIn& tx_in = psbtx.tx->vin.at(i); + const Coin& coin = coins.at(tx_in.prevout); + if (!coin.out.IsNull() && IsSegWitOutput(provider, coin.out.scriptPubKey)) { + input.witness_utxo = coin.out; + } + } } + } - const Coin& coin = view.AccessCoin(psbtx.tx->vin[i].prevout); + const PrecomputedTransactionData& txdata = PrecomputePSBTData(psbtx); - if (IsSegWitOutput(provider, coin.out.scriptPubKey)) { - input.witness_utxo = coin.out; + for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) { + if (PSBTInputSigned(psbtx.inputs.at(i))) { + continue; } // Update script/keypath information using descriptor data. // Note that SignPSBTInput does a lot more than just constructing ECDSA signatures // we don't actually care about those here, in fact. - SignPSBTInput(provider, psbtx, i, &txdata, /*sighash=*/1); + SignPSBTInput(provider, psbtx, /*index=*/i, &txdata, /*sighash=*/1); } // Update script/keypath information using descriptor data. for (unsigned int i = 0; i < psbtx.tx->vout.size(); ++i) { UpdatePSBTOutput(provider, psbtx, i); } + + RemoveUnnecessaryTransactions(psbtx, /*sighash_type=*/1); + return psbtx; } @@ -1632,7 +1660,7 @@ static RPCHelpMan converttopsbt() static RPCHelpMan utxoupdatepsbt() { return RPCHelpMan{"utxoupdatepsbt", - "\nUpdates all segwit inputs and outputs in a PSBT with data from output descriptors, the UTXO set or the mempool.\n", + "\nUpdates all segwit inputs and outputs in a PSBT with data from output descriptors, the UTXO set, txindex, or the mempool.\n", { {"psbt", RPCArg::Type::STR, RPCArg::Optional::NO, "A base64 string of a PSBT"}, {"descriptors", RPCArg::Type::ARR, RPCArg::Optional::OMITTED_NAMED_ARG, "An array of either strings or objects", { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6158ff033cae6..3ead8bee34946 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2156,34 +2156,7 @@ TransactionError CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bool& comp } } - // Only drop non_witness_utxos if sighash_type != SIGHASH_ANYONECANPAY - if ((sighash_type & 0x80) != SIGHASH_ANYONECANPAY) { - // Figure out if any non_witness_utxos should be dropped - std::vector to_drop; - for (unsigned int i = 0; i < psbtx.inputs.size(); ++i) { - const auto& input = psbtx.inputs.at(i); - int wit_ver; - std::vector wit_prog; - if (input.witness_utxo.IsNull() || !input.witness_utxo.scriptPubKey.IsWitnessProgram(wit_ver, wit_prog)) { - // There's a non-segwit input or Segwit v0, so we cannot drop any witness_utxos - to_drop.clear(); - break; - } - if (wit_ver == 0) { - // Segwit v0, so we cannot drop any non_witness_utxos - to_drop.clear(); - break; - } - if (input.non_witness_utxo) { - to_drop.push_back(i); - } - } - - // Drop the non_witness_utxos that we can drop - for (unsigned int i : to_drop) { - psbtx.inputs.at(i).non_witness_utxo = nullptr; - } - } + RemoveUnnecessaryTransactions(psbtx, sighash_type); // Complete if every input is now signed complete = true; diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 58a80e37a200c..441132968826a 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -611,17 +611,17 @@ def test_psbt_input_keys(psbt_input, keys): # Bech32 inputs should be filled with witness UTXO. Other inputs should not be filled because they are non-witness updated = self.nodes[1].utxoupdatepsbt(psbt) decoded = self.nodes[1].decodepsbt(updated) - test_psbt_input_keys(decoded['inputs'][0], ['witness_utxo']) - test_psbt_input_keys(decoded['inputs'][1], []) - test_psbt_input_keys(decoded['inputs'][2], []) + test_psbt_input_keys(decoded['inputs'][0], ['witness_utxo', 'non_witness_utxo']) + test_psbt_input_keys(decoded['inputs'][1], ['non_witness_utxo']) + test_psbt_input_keys(decoded['inputs'][2], ['non_witness_utxo']) # Try again, now while providing descriptors, making P2SH-segwit work, and causing bip32_derivs and redeem_script to be filled in descs = [self.nodes[1].getaddressinfo(addr)['desc'] for addr in [addr1,addr2,addr3]] updated = self.nodes[1].utxoupdatepsbt(psbt=psbt, descriptors=descs) decoded = self.nodes[1].decodepsbt(updated) - test_psbt_input_keys(decoded['inputs'][0], ['witness_utxo', 'bip32_derivs']) - test_psbt_input_keys(decoded['inputs'][1], []) - test_psbt_input_keys(decoded['inputs'][2], ['witness_utxo', 'bip32_derivs', 'redeem_script']) + test_psbt_input_keys(decoded['inputs'][0], ['witness_utxo', 'non_witness_utxo', 'bip32_derivs']) + test_psbt_input_keys(decoded['inputs'][1], ['non_witness_utxo', 'bip32_derivs']) + test_psbt_input_keys(decoded['inputs'][2], ['non_witness_utxo','witness_utxo', 'bip32_derivs', 'redeem_script']) # Two PSBTs with a common input should not be joinable psbt1 = self.nodes[1].createpsbt([{"txid":txid1, "vout":vout1}], {self.nodes[0].getnewaddress():Decimal('10.999')}) From 1869310f3cfa4ab26b5090d8a4002eefdc84870e Mon Sep 17 00:00:00 2001 From: Bushstar Date: Mon, 20 Mar 2023 11:41:52 +0000 Subject: [PATCH 031/272] refactor: remove unused param from legacy pubkey --- src/wallet/scriptpubkeyman.cpp | 8 ++++---- src/wallet/scriptpubkeyman.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 1589e52debccd..db765d9b10627 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1385,10 +1385,10 @@ void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, co WalletLogPrintf("keypool return %d\n", nIndex); } -bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType type, bool internal) +bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType type) { assert(type != OutputType::BECH32M); - if (!CanGetAddresses(internal)) { + if (!CanGetAddresses(/*internal=*/ false)) { return false; } @@ -1396,10 +1396,10 @@ bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType typ { LOCK(cs_KeyStore); int64_t nIndex; - if (!ReserveKeyFromKeyPool(nIndex, keypool, internal) && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + if (!ReserveKeyFromKeyPool(nIndex, keypool, /*fRequestedInternal=*/ false) && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { if (m_storage.IsLocked()) return false; WalletBatch batch(m_storage.GetDatabase()); - result = GenerateNewKey(batch, m_hd_chain, internal); + result = GenerateNewKey(batch, m_hd_chain, /*internal=*/ false); return true; } KeepDestination(nIndex, type); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 4d1432524120b..42407173c3f3f 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -334,7 +334,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv std::map m_index_to_reserved_key; //! Fetches a key from the keypool - bool GetKeyFromPool(CPubKey &key, const OutputType type, bool internal = false); + bool GetKeyFromPool(CPubKey &key, const OutputType type); /** * Reserves a key from the keypool and sets nIndex to its index From 43d8173f9911cb2130e52d5ddac44c0e19edccce Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 21 Mar 2023 14:26:01 +0000 Subject: [PATCH 032/272] guix: import LIEF from upstream (0.12.3) Updates to version 0.12.3. Retain our PPC64 patch. Mention when we can drop our local definition. --- contrib/guix/manifest.scm | 62 +++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/contrib/guix/manifest.scm b/contrib/guix/manifest.scm index 7c1550a8d1a64..353ccde25fc01 100644 --- a/contrib/guix/manifest.scm +++ b/contrib/guix/manifest.scm @@ -208,35 +208,39 @@ chain for " target " development.")) (package-with-extra-patches lief (search-our-patches "lief-fix-ppc64-nx-default.patch"))) -(define-public lief +;; Our python-lief package can be removed once we are using +;; guix 83bfdb409787cb2737e68b093a319b247b7858e6 or later. +(define-public python-lief (package - (name "python-lief") - (version "0.12.1") - (source - (origin - (method git-fetch) - (uri (git-reference - (url "https://github.com/lief-project/LIEF.git") - (commit version))) - (file-name (git-file-name name version)) - (sha256 - (base32 - "1xzbh3bxy4rw1yamnx68da1v5s56ay4g081cyamv67256g0qy2i1")))) - (build-system python-build-system) - (arguments - `(#:phases - (modify-phases %standard-phases - (add-after 'unpack 'parallel-jobs - ;; build with multiple cores - (lambda _ - (substitute* "setup.py" (("self.parallel if self.parallel else 1") (number->string (parallel-job-count))))))))) - (native-inputs - `(("cmake" ,cmake))) - (home-page "https://github.com/lief-project/LIEF") - (synopsis "Library to Instrument Executable Formats") - (description "Python library to to provide a cross platform library which can -parse, modify and abstract ELF, PE and MachO formats.") - (license license:asl2.0))) + (name "python-lief") + (version "0.12.3") + (source (origin + (method git-fetch) + (uri (git-reference + (url "https://github.com/lief-project/LIEF") + (commit version))) + (file-name (git-file-name name version)) + (sha256 + (base32 + "11i6hqmcjh56y554kqhl61698n9v66j2qk1c1g63mv2w07h2z661")))) + (build-system python-build-system) + (native-inputs (list cmake)) + (arguments + (list + #:tests? #f ;needs network + #:phases #~(modify-phases %standard-phases + (replace 'build + (lambda _ + (invoke + "python" "setup.py" "--sdk" "build" + (string-append + "-j" (number->string (parallel-job-count))))))))) + (home-page "https://github.com/lief-project/LIEF") + (synopsis "Library to instrument executable formats") + (description + "@code{python-lief} is a cross platform library which can parse, modify +and abstract ELF, PE and MachO formats.") + (license license:asl2.0))) (define osslsigncode (package @@ -596,7 +600,7 @@ inspecting signatures in Mach-O binaries.") ;; Git git-minimal ;; Tests - (fix-ppc64-nx-default lief)) + (fix-ppc64-nx-default python-lief)) (let ((target (getenv "HOST"))) (cond ((string-suffix? "-mingw32" target) ;; Windows From 24f26e08cc0db4041c51fe8391b1736b47a13af9 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 22 Mar 2023 09:53:13 +0000 Subject: [PATCH 033/272] guix: use cmake-minimal for python-lief This also fixes atleast one --no-substitues build failure I've seen, where cmake dependencies wouldn't build: ```bash The following derivations will be built: /gnu/store/7qqvqq2g7l5ylrjv0gn6zha565a12kar-python-lief-0.12.1.drv /gnu/store/f9zwh1ldy63ga0i5w6cbbqlj6sfq226j-cmake-3.21.4.drv /gnu/store/3wg6ya847id503m5izhzhn1qqs464lfk-python-sphinx-4.2.0.drv building /gnu/store/3wg6ya847id503m5izhzhn1qqs464lfk-python-sphinx-4.2.0.drv... / 'check' phasenote: keeping build directory `/tmp/guix-build-python-sphinx-4.2.0.drv-5' builder for `/gnu/store/3wg6ya847id503m5izhzhn1qqs464lfk-python-sphinx-4.2.0.drv' failed with exit code 1 build of /gnu/store/3wg6ya847id503m5izhzhn1qqs464lfk-python-sphinx-4.2.0.drv failed View build log at '/var/log/guix/drvs/3w/g6ya847id503m5izhzhn1qqs464lfk-python-sphinx-4.2.0.drv.gz'. cannot build derivation `/gnu/store/f9zwh1ldy63ga0i5w6cbbqlj6sfq226j-cmake-3.21.4.drv': 1 dependencies couldn't be built cannot build derivation `/gnu/store/7qqvqq2g7l5ylrjv0gn6zha565a12kar-python-lief-0.12.1.drv': 1 dependencies couldn't be built guix environment: error: build of `/gnu/store/7qqvqq2g7l5ylrjv0gn6zha565a12kar-python-lief-0.12.1.drv' failed ``` --- contrib/guix/manifest.scm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contrib/guix/manifest.scm b/contrib/guix/manifest.scm index 353ccde25fc01..0cfce411174d3 100644 --- a/contrib/guix/manifest.scm +++ b/contrib/guix/manifest.scm @@ -210,6 +210,7 @@ chain for " target " development.")) ;; Our python-lief package can be removed once we are using ;; guix 83bfdb409787cb2737e68b093a319b247b7858e6 or later. +;; Note we currently use cmake-minimal. (define-public python-lief (package (name "python-lief") @@ -224,7 +225,7 @@ chain for " target " development.")) (base32 "11i6hqmcjh56y554kqhl61698n9v66j2qk1c1g63mv2w07h2z661")))) (build-system python-build-system) - (native-inputs (list cmake)) + (native-inputs (list cmake-minimal)) (arguments (list #:tests? #f ;needs network From eb1c3adf38cb71c3e6f298a61871738c4919b4a1 Mon Sep 17 00:00:00 2001 From: fanquake Date: Thu, 23 Mar 2023 10:29:13 +0000 Subject: [PATCH 034/272] depends: qrencode 4.1.1 Upgrade to the latest qrencode, and disable some warnings that cause compile failures with newer compilers (clang-15+). Fixes part of #27299. --- depends/packages/qrencode.mk | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/depends/packages/qrencode.mk b/depends/packages/qrencode.mk index d1687883bcd51..2afd95d7c4fae 100644 --- a/depends/packages/qrencode.mk +++ b/depends/packages/qrencode.mk @@ -1,15 +1,16 @@ package=qrencode -$(package)_version=3.4.4 +$(package)_version=4.1.1 $(package)_download_path=https://fukuchi.org/works/qrencode/ $(package)_file_name=$(package)-$($(package)_version).tar.bz2 -$(package)_sha256_hash=efe5188b1ddbcbf98763b819b146be6a90481aac30cfc8d858ab78a19cde1fa5 +$(package)_sha256_hash=e455d9732f8041cf5b9c388e345a641fd15707860f928e94507b1961256a6923 define $(package)_set_vars -$(package)_config_opts=--disable-shared --without-tools --without-tests --disable-sdltest +$(package)_config_opts=--disable-shared --without-tools --without-tests --without-png $(package)_config_opts += --disable-gprof --disable-gcov --disable-mudflap $(package)_config_opts += --disable-dependency-tracking --enable-option-checking $(package)_config_opts_linux=--with-pic $(package)_config_opts_android=--with-pic +$(package)_cflags += -Wno-int-conversion -Wno-implicit-function-declaration endef define $(package)_preprocess_cmds From aac8793c7a2ee7630641dd74be6ba2ead50c2aee Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Thu, 23 Mar 2023 11:59:29 +0100 Subject: [PATCH 035/272] test: test_bech32_decode in address.py Adds bech32_to_bytes() which can decode a bech32 address and return the version as an `int` and the payload in bytes. bech32_to_bytes() is used by the test_bech32_decode unit test to test decoding of segwit addresses. --- test/functional/test_framework/address.py | 30 ++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/test/functional/test_framework/address.py b/test/functional/test_framework/address.py index 959a2a65bd03b..d1bf186b9d1bc 100644 --- a/test/functional/test_framework/address.py +++ b/test/functional/test_framework/address.py @@ -20,8 +20,11 @@ sha256, taproot_construct, ) -from .segwit_addr import encode_segwit_address from .util import assert_equal +from test_framework.segwit_addr import ( + decode_segwit_address, + encode_segwit_address, +) ADDRESS_BCRT1_UNSPENDABLE = 'bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj' ADDRESS_BCRT1_UNSPENDABLE_DESCRIPTOR = 'addr(bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj)#juyq9d97' @@ -159,6 +162,16 @@ def check_script(script): assert False +def bech32_to_bytes(address): + hrp = address.split('1')[0] + if hrp not in ['bc', 'tb', 'bcrt']: + return (None, None) + version, payload = decode_segwit_address(hrp, address) + if version is None: + return (None, None) + return version, bytearray(payload) + + class TestFrameworkScript(unittest.TestCase): def test_base58encodedecode(self): def check_base58(data, version): @@ -176,3 +189,18 @@ def check_base58(data, version): check_base58(bytes.fromhex('0041c1eaf111802559bad61b60d62b1f897c63928a'), 0) check_base58(bytes.fromhex('000041c1eaf111802559bad61b60d62b1f897c63928a'), 0) check_base58(bytes.fromhex('00000041c1eaf111802559bad61b60d62b1f897c63928a'), 0) + + + def test_bech32_decode(self): + def check_bech32_decode(payload, version): + hrp = "tb" + self.assertEqual(bech32_to_bytes(encode_segwit_address(hrp, version, payload)), (version, payload)) + + check_bech32_decode(bytes.fromhex('36e3e2a33f328de12e4b43c515a75fba2632ecc3'), 0) + check_bech32_decode(bytes.fromhex('823e9790fc1d1782321140d4f4aa61aabd5e045b'), 0) + check_bech32_decode(bytes.fromhex('79be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798'), 1) + check_bech32_decode(bytes.fromhex('39cf8ebd95134f431c39db0220770bd127f5dd3cc103c988b7dcd577ae34e354'), 1) + check_bech32_decode(bytes.fromhex('708244006d27c757f6f1fc6f853b6ec26268b727866f7ce632886e34eb5839a3'), 1) + check_bech32_decode(bytes.fromhex('616211ab00dffe0adcb6ce258d6d3fd8cbd901e2'), 0) + check_bech32_decode(bytes.fromhex('b6a7c98b482d7fb21c9fa8e65692a0890410ff22'), 0) + check_bech32_decode(bytes.fromhex('f0c2109cb1008cfa7b5a09cc56f7267cd8e50929'), 0) From d178082996dc3000f42816f89afcf3fa4d31e159 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Thu, 23 Mar 2023 12:00:54 +0100 Subject: [PATCH 036/272] test: add bech32 decoding support to address_to_scriptpubkey() This permits functional tests to decode bech32 addresses to scriptpubkeys. --- test/functional/test_framework/wallet.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py index f3253630c46ee..eab8fbda470fa 100644 --- a/test/functional/test_framework/wallet.py +++ b/test/functional/test_framework/wallet.py @@ -14,6 +14,7 @@ ) from test_framework.address import ( base58_to_byte, + bech32_to_bytes, create_deterministic_address_bcrt1_p2tr_op_true, key_to_p2pkh, key_to_p2sh_p2wpkh, @@ -49,6 +50,7 @@ key_to_p2sh_p2wpkh_script, key_to_p2wpkh_script, keyhash_to_p2pkh_script, + program_to_witness_script, scripthash_to_p2sh_script, ) from test_framework.util import ( @@ -414,6 +416,9 @@ def getnewdestination(address_type='bech32m'): def address_to_scriptpubkey(address): """Converts a given address to the corresponding output script (scriptPubKey).""" + version, payload = bech32_to_bytes(address) + if version is not None: + return program_to_witness_script(version, payload) # testnet segwit scriptpubkey payload, version = base58_to_byte(address) if version == 111: # testnet pubkey hash return keyhash_to_p2pkh_script(payload) From 18fb36367a28819bd5ab402344802796a1248979 Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Mon, 6 Mar 2023 23:41:46 +0100 Subject: [PATCH 037/272] refactor: Extract util/fs_helpers from util/system This is an extraction of filesystem related functions from util/system into their own utility file. The background of this commit is an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager defined in system.h. Moving these functions out of system.h allows including them from a separate source file without including the ArgsManager definitions from system.h. --- src/Makefile.am | 3 + src/addrdb.cpp | 1 + src/dbwrapper.cpp | 2 +- src/flatfile.cpp | 2 +- src/index/blockfilterindex.cpp | 1 + src/init.cpp | 1 + src/init/common.cpp | 1 + src/kernel/mempool_persist.cpp | 2 +- src/qt/guiutil.cpp | 1 + src/qt/intro.cpp | 1 + src/qt/walletframe.cpp | 2 +- src/rpc/request.cpp | 3 +- src/test/fs_tests.cpp | 2 +- src/test/util_tests.cpp | 5 +- src/util/fs_helpers.cpp | 295 +++++++++++++++++++++++++++++++++ src/util/fs_helpers.h | 63 +++++++ src/util/system.cpp | 262 +---------------------------- src/util/system.h | 46 ----- src/validation.cpp | 1 + src/wallet/bdb.cpp | 1 + src/wallet/sqlite.cpp | 2 +- src/wallet/wallet.cpp | 3 +- 22 files changed, 382 insertions(+), 318 deletions(-) create mode 100644 src/util/fs_helpers.cpp create mode 100644 src/util/fs_helpers.h diff --git a/src/Makefile.am b/src/Makefile.am index 53c809c901688..9ce9a68501849 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -285,6 +285,7 @@ BITCOIN_CORE_H = \ util/exception.h \ util/fastrange.h \ util/fees.h \ + util/fs_helpers.h \ util/getuniquepath.h \ util/golombrice.h \ util/hash_type.h \ @@ -707,6 +708,7 @@ libbitcoin_util_a_SOURCES = \ util/error.cpp \ util/exception.cpp \ util/fees.cpp \ + util/fs_helpers.cpp \ util/getuniquepath.cpp \ util/hasher.cpp \ util/sock.cpp \ @@ -951,6 +953,7 @@ libbitcoinkernel_la_SOURCES = \ uint256.cpp \ util/check.cpp \ util/exception.cpp \ + util/fs_helpers.cpp \ util/getuniquepath.cpp \ util/hasher.cpp \ util/moneystr.cpp \ diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 9ae8244d1c060..71da45ad3d8a3 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp index 0c6debfa801e6..f6faa60e938a0 100644 --- a/src/dbwrapper.cpp +++ b/src/dbwrapper.cpp @@ -8,8 +8,8 @@ #include #include #include +#include #include -#include #include #include diff --git a/src/flatfile.cpp b/src/flatfile.cpp index d6e84d02c12b8..59861a08adfee 100644 --- a/src/flatfile.cpp +++ b/src/flatfile.cpp @@ -8,7 +8,7 @@ #include #include #include -#include +#include FlatFileSeq::FlatFileSeq(fs::path dir, const char* prefix, size_t chunk_size) : m_dir(std::move(dir)), diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp index 59bf6d34cf717..43c22153386fb 100644 --- a/src/index/blockfilterindex.cpp +++ b/src/index/blockfilterindex.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include diff --git a/src/init.cpp b/src/init.cpp index 8a45c38ce3654..167ed06d60d79 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -70,6 +70,7 @@ #include #include #include +#include #include #include #include diff --git a/src/init/common.cpp b/src/init/common.cpp index 791424f5f6d75..216de3aecdae3 100644 --- a/src/init/common.cpp +++ b/src/init/common.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include diff --git a/src/kernel/mempool_persist.cpp b/src/kernel/mempool_persist.cpp index a14b2e616362e..08ca9f8c19d04 100644 --- a/src/kernel/mempool_persist.cpp +++ b/src/kernel/mempool_persist.cpp @@ -15,7 +15,7 @@ #include #include #include -#include +#include #include #include diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp index 83c78d5c18264..e15b8d55094b1 100644 --- a/src/qt/guiutil.cpp +++ b/src/qt/guiutil.cpp @@ -21,6 +21,7 @@ #include