From cca4f82b828669ae23f6ac64fb83e068b81ae189 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Fri, 26 Aug 2022 15:23:21 -0300 Subject: [PATCH 001/867] 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 7dbeccbc09..9f37335850 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/867] 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 713c0826d3..468ad1eafa 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/867] 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 903a569cb9..67d5037fd2 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 1d2d7d2a10..3eb8e2ddcb 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 675c4a759d..cd74f672c1 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/867] 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 cf626bc7c6..634e08e9e2 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 057057a2d7e23c2e29cbfd29a5124b3947a33757 Mon Sep 17 00:00:00 2001 From: Yusuf Sahin HAMZA Date: Wed, 21 Dec 2022 00:18:35 +0300 Subject: [PATCH 005/867] 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 86cfd4a230..2e10c0f0bf 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 006/867] 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 91c2a43932..cbaeb3ce65 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 0e21b56a44d53cec9080edb04410a692717f1ddc Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Thu, 5 Jan 2023 17:35:14 -0500 Subject: [PATCH 007/867] assumeutxo: catch and log fs::remove error instead of two exist checks --- src/validation.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index e24d39170e..e1ba8b96d2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4859,15 +4859,15 @@ static bool DeleteCoinsDBFromDisk(const fs::path db_path, bool is_snapshot) if (is_snapshot) { fs::path base_blockhash_path = db_path / node::SNAPSHOT_BLOCKHASH_FILENAME; - if (fs::exists(base_blockhash_path)) { - bool removed = fs::remove(base_blockhash_path); - if (!removed) { - LogPrintf("[snapshot] failed to remove file %s\n", - fs::PathToString(base_blockhash_path)); + try { + bool existed = fs::remove(base_blockhash_path); + if (!existed) { + LogPrintf("[snapshot] snapshot chainstate dir being removed lacks %s file\n", + fs::PathToString(node::SNAPSHOT_BLOCKHASH_FILENAME)); } - } else { - LogPrintf("[snapshot] snapshot chainstate dir being removed lacks %s file\n", - fs::PathToString(node::SNAPSHOT_BLOCKHASH_FILENAME)); + } catch (const fs::filesystem_error& e) { + LogPrintf("[snapshot] failed to remove file %s: %s\n", + fs::PathToString(base_blockhash_path), fsbridge::get_filesystem_error_message(e)); } } From a1aaa7f51f4205ae4d27fbceb2c3a97bc114e828 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Mon, 16 May 2022 17:58:19 -0300 Subject: [PATCH 008/867] 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 c257af13c4..d2b33a8b70 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 1cb3f434e8..f1e7869d91 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 009/867] 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 0000000000..ce8ab53ddd --- /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 010/867] 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 981dead3b8..011983a158 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 541012e621386cd824eed81295206a34ba3ba497 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 3 Feb 2023 21:42:57 +0100 Subject: [PATCH 011/867] Build: Use AM_V_GEN in Makefiles where appropriate When generating new files as part of the Makefile the recipe is sometimes suppressed with $(AM_V_GEN) and sometimes with `@`. We should prefer $(AM_V_GEN), since this also prints the lines in silent mode. This is arguably more in style with the current recipe echoing. Before: Generated test/data/script_tests.json.h Now: GEN test/data/script_tests.json.h A side effect of this change is that the recipe for generating build.h is now echoed on each make run. Arguably this makes its generation more transparent. --- src/Makefile.am | 5 ++--- src/Makefile.test.include | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 5830090ada..39f5a29aa6 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -346,7 +346,7 @@ BITCOIN_CORE_H = \ obj/build.h: FORCE @$(MKDIR_P) $(builddir)/obj - @$(top_srcdir)/share/genbuild.sh "$(abs_top_builddir)/src/obj/build.h" \ + $(AM_V_GEN) $(top_srcdir)/share/genbuild.sh "$(abs_top_builddir)/src/obj/build.h" \ "$(abs_top_srcdir)" libbitcoin_util_a-clientversion.$(OBJEXT): obj/build.h @@ -1085,12 +1085,11 @@ endif %.raw.h: %.raw @$(MKDIR_P) $(@D) - @{ \ + $(AM_V_GEN) { \ echo "static unsigned const char $(*F)_raw[] = {" && \ $(HEXDUMP) -v -e '8/1 "0x%02x, "' -e '"\n"' $< | $(SED) -e 's/0x ,//g' && \ echo "};"; \ } > "$@.new" && mv -f "$@.new" "$@" - @echo "Generated $@" include Makefile.minisketch.include diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 4d867fdc2f..1c03c6892b 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -414,10 +414,9 @@ endif %.json.h: %.json @$(MKDIR_P) $(@D) - @{ \ + $(AM_V_GEN) { \ echo "namespace json_tests{" && \ echo "static unsigned const char $(*F)[] = {" && \ $(HEXDUMP) -v -e '8/1 "0x%02x, "' -e '"\n"' $< | $(SED) -e 's/0x ,//g' && \ echo "};};"; \ } > "$@.new" && mv -f "$@.new" "$@" - @echo "Generated $@" From 1b1ffbd014b931afb9435ec10911b9a7c130d3e5 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 3 Feb 2023 22:18:54 +0100 Subject: [PATCH 012/867] Build: Log when test -f fails in Makefile Silently emitting an error makes it a bit harder to debug. Instead, print a helpful log message to point the developer in the right direction. Alternatively this could have been implemented by just removing the recipe echo suppression (@), but the subsequent make output became too noisy. --- src/Makefile.am | 2 +- src/Makefile.qt.include | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 39f5a29aa6..7246a61891 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1030,7 +1030,7 @@ clean-local: -rm -rf test/__pycache__ .rc.o: - @test -f $(WINDRES) + @test -f $(WINDRES) || (echo "windres $(WINDRES) not found, but is required to compile windows resource files"; exit 1) ## FIXME: How to get the appropriate modulename_CPPFLAGS in here? $(AM_V_GEN) $(WINDRES) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(CPPFLAGS) -DWINDRES_PREPROC -i $< -o $@ diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include index 602a118259..7852d1a2fa 100644 --- a/src/Makefile.qt.include +++ b/src/Makefile.qt.include @@ -371,13 +371,13 @@ translate: $(srcdir)/qt/bitcoinstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCO @rm -f $(srcdir)/qt/locale/bitcoin_en.xlf.old $(QT_QRC_LOCALE_CPP): $(QT_QRC_LOCALE) $(QT_QM) - @test -f $(RCC) + @test -f $(RCC) || (echo "rcc $(RCC) not found, but is required for generating qrc cpp files"; exit 1) @cp -f $< $(@D)/temp_$( $@ @rm $(@D)/temp_$( $@ CLEAN_QT = $(nodist_qt_libbitcoinqt_a_SOURCES) $(QT_QM) $(QT_FORMS_H) qt/*.gcda qt/*.gcno qt/temp_bitcoin_locale.qrc @@ -404,7 +404,7 @@ bitcoin_qt_apk: FORCE cd qt/android && ./gradlew build ui_%.h: %.ui - @test -f $(UIC) + @test -f $(UIC) || (echo "uic $(UIC) not found, but is required for generating ui headers"; exit 1) @$(MKDIR_P) $(@D) $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(UIC) -o $@ $< || (echo "Error creating $@"; false) @@ -415,6 +415,6 @@ moc_%.cpp: %.h $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(MOC) $(DEFAULT_INCLUDES) $(QT_INCLUDES_UNSUPPRESSED) $(MOC_DEFS) $< > $@ %.qm: %.ts - @test -f $(LRELEASE) + @test -f $(LRELEASE) || (echo "lrelease $(LRELEASE) not found, but is required for generating translations"; exit 1) @$(MKDIR_P) $(@D) $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(LRELEASE) -silent $< -qm $@ 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 013/867] 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 c839647b29..3714335b39 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 014/867] 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 f5ca9a5c34..ec5b0213b3 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 015/867] 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 3fd0280b8b..5ea9b7fad3 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 3abd22c207..95a3bb2bc8 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 a8ecce119a..a55e4c45a8 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 65e3abcbf2b9e818f3b9f1ba35f3cfe7df5e3811 Mon Sep 17 00:00:00 2001 From: willcl-ark Date: Tue, 7 Mar 2023 22:53:23 +0000 Subject: [PATCH 016/867] doc: document json rpc endpoints fixes #20246 Document both JSON-RPC endpoints, when they are active and which types of requests they are able to service. Adds two example curl requests, one for each endpoint. --- doc/JSON-RPC-interface.md | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/doc/JSON-RPC-interface.md b/doc/JSON-RPC-interface.md index ab5db58cdd..6cbb6ebd72 100644 --- a/doc/JSON-RPC-interface.md +++ b/doc/JSON-RPC-interface.md @@ -5,6 +5,41 @@ The headless daemon `bitcoind` has the JSON-RPC API enabled by default, the GUI option. In the GUI it is possible to execute RPC methods in the Debug Console Dialog. +## Endpoints + +There are two JSON-RPC endpoints on the server: + +1. `/` +2. `/wallet//` + +### `/` endpoint + +This endpoint is always active. +It can always service non-wallet requests and can service wallet requests when +exactly one wallet is loaded. + +### `/wallet//` endpoint + +This endpoint is only activated when the wallet component has been compiled in. +It can service both wallet and non-wallet requests. +It MUST be used for wallet requests when two or more wallets are loaded. + +This is the endpoint used by bitcoin-cli when a `-rpcwallet=` parameter is passed in. + +Best practice would dictate using the `/wallet//` endpoint for ALL +requests when multiple wallets are in use. + +### Examples + +```sh +# Get block count from the / endpoint when rpcuser=alice and rpcport=38332 +$ curl --user alice --data-binary '{"jsonrpc": "1.0", "id": "0", "method": "getblockcount", "params": []}' -H 'content-type: text/plain;' localhost:38332/ + +# Get balance from the /wallet/walletname endpoint when rpcuser=alice, rpcport=38332 and rpcwallet=desc-wallet +$ curl --user alice --data-binary '{"jsonrpc": "1.0", "id": "0", "method": "getbalance", "params": []}' -H 'content-type: text/plain;' localhost:38332/wallet/desc-wallet + +``` + ## Parameter passing The JSON-RPC server supports both _by-position_ and _by-name_ [parameter From 9c18992bbaf649f8c5461d5e4dc39eb1a07ffc77 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Tue, 29 Nov 2022 15:34:59 -0300 Subject: [PATCH 017/867] 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 97354f480c..b4f3d77e5b 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 052fbcd5a791855406141e85d32e42e297220fe9 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Sat, 18 Feb 2023 17:46:13 -0700 Subject: [PATCH 018/867] 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 ec5b0213b3..966cf6b043 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 94fe81aca9..5410c3342c 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 019/867] 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 966cf6b043..afaa040b0f 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 020/867] 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 afaa040b0f..69ad13a912 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 021/867] 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 69ad13a912..fe4a79b2e6 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 4985fc764c..374996e501 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 5410c3342c..3cf3b838d6 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 758691cfde..ad59e123d2 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 022/867] 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 fe4a79b2e6..cdfd079fcd 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 374996e501..c8e31fe283 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 3cf3b838d6..7aead2812b 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 023/867] 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 ad59e123d2..1acdb02c9a 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 024/867] 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 1acdb02c9a..c97a29eb1e 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 025/867] 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 c97a29eb1e..b5f9558337 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 026/867] 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 d6b52eb587..b8c69d0a63 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 027/867] 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 b8c69d0a63..8a5cab443f 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 028/867] 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 c8e31fe283..6284b80a52 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 029/867] 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 50ccd9e2c0..16f4fa857c 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 40d69cd454..0a581933f0 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 011983a158..879e847acb 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 6158ff033c..3ead8bee34 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 58a80e37a2..4411329688 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 6c8bde6d54d03224709dce54b8ba32b8c3e37ac7 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 3 Mar 2023 15:07:06 +0000 Subject: [PATCH 030/867] test: move coverage on ParseNonRFCJSONValue() to UniValue::read() Preparation to deprecate ParseNonRFCJSONValue() but keep test coverage on the underlying UniValue::read() unaffected. The test coverage on AmountFromValue is no longer included, since that is already tested in the rpc_parse_monetary_values test case. Fuzzing coverage on ParseNonRFCJSONValue() was duplicated between string.cpp and parse_univalue.cpp, only the one in parse_univalue.cpp is kept. --- src/test/fuzz/parse_univalue.cpp | 9 +++------ src/test/fuzz/string.cpp | 4 ---- src/test/rpc_tests.cpp | 31 +------------------------------ src/univalue/test/object.cpp | 27 +++++++++++++++++++++++++++ 4 files changed, 31 insertions(+), 40 deletions(-) diff --git a/src/test/fuzz/parse_univalue.cpp b/src/test/fuzz/parse_univalue.cpp index 16486f6b96..5b95c20928 100644 --- a/src/test/fuzz/parse_univalue.cpp +++ b/src/test/fuzz/parse_univalue.cpp @@ -21,12 +21,9 @@ FUZZ_TARGET_INIT(parse_univalue, initialize_parse_univalue) const std::string random_string(buffer.begin(), buffer.end()); bool valid = true; const UniValue univalue = [&] { - try { - return ParseNonRFCJSONValue(random_string); - } catch (const std::runtime_error&) { - valid = false; - return UniValue{}; - } + UniValue uv; + if (!uv.read(random_string)) valid = false; + return valid ? uv : UniValue{}; }(); if (!valid) { return; diff --git a/src/test/fuzz/string.cpp b/src/test/fuzz/string.cpp index 9890e4c0e5..5634c02b24 100644 --- a/src/test/fuzz/string.cpp +++ b/src/test/fuzz/string.cpp @@ -159,10 +159,6 @@ FUZZ_TARGET(string) const util::Settings settings; (void)OnlyHasDefaultSectionSetting(settings, random_string_1, random_string_2); (void)ParseNetwork(random_string_1); - try { - (void)ParseNonRFCJSONValue(random_string_1); - } catch (const std::runtime_error&) { - } (void)ParseOutputType(random_string_1); (void)RemovePrefix(random_string_1, random_string_2); (void)ResolveErrMsg(random_string_1, random_string_2); diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index 791c9ddf31..9d380595f1 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -278,6 +278,7 @@ BOOST_AUTO_TEST_CASE(rpc_parse_monetary_values) BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.00000001000000")), 1LL); //should pass, cut trailing 0 BOOST_CHECK_THROW(AmountFromValue(ValueFromString("19e-9")), UniValue); //should fail BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.19e-6")), 19); //should pass, leading 0 is present + BOOST_CHECK_EXCEPTION(AmountFromValue(".19e-6"), UniValue, HasJSON(R"({"code":-3,"message":"Invalid amount"})")); //should fail, no leading 0 BOOST_CHECK_THROW(AmountFromValue(ValueFromString("92233720368.54775808")), UniValue); //overflow error BOOST_CHECK_THROW(AmountFromValue(ValueFromString("1e+11")), UniValue); //overflow error @@ -285,36 +286,6 @@ BOOST_AUTO_TEST_CASE(rpc_parse_monetary_values) BOOST_CHECK_THROW(AmountFromValue(ValueFromString("93e+9")), UniValue); //overflow error } -BOOST_AUTO_TEST_CASE(json_parse_errors) -{ - // Valid - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("1.0").get_real(), 1.0); - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("true").get_bool(), true); - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("[false]")[0].get_bool(), false); - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("{\"a\": true}")["a"].get_bool(), true); - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("{\"1\": \"true\"}")["1"].get_str(), "true"); - // Valid, with leading or trailing whitespace - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue(" 1.0").get_real(), 1.0); - BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("1.0 ").get_real(), 1.0); - - BOOST_CHECK_THROW(AmountFromValue(ParseNonRFCJSONValue(".19e-6")), std::runtime_error); //should fail, missing leading 0, therefore invalid JSON - BOOST_CHECK_EQUAL(AmountFromValue(ParseNonRFCJSONValue("0.00000000000000000000000000000000000001e+30 ")), 1); - // Invalid, initial garbage - BOOST_CHECK_THROW(ParseNonRFCJSONValue("[1.0"), std::runtime_error); - BOOST_CHECK_THROW(ParseNonRFCJSONValue("a1.0"), std::runtime_error); - // Invalid, trailing garbage - BOOST_CHECK_THROW(ParseNonRFCJSONValue("1.0sds"), std::runtime_error); - BOOST_CHECK_THROW(ParseNonRFCJSONValue("1.0]"), std::runtime_error); - // Invalid, keys have to be names - BOOST_CHECK_THROW(ParseNonRFCJSONValue("{1: \"true\"}"), std::runtime_error); - BOOST_CHECK_THROW(ParseNonRFCJSONValue("{true: 1}"), std::runtime_error); - BOOST_CHECK_THROW(ParseNonRFCJSONValue("{[1]: 1}"), std::runtime_error); - BOOST_CHECK_THROW(ParseNonRFCJSONValue("{{\"a\": \"a\"}: 1}"), std::runtime_error); - // BTC addresses should fail parsing - BOOST_CHECK_THROW(ParseNonRFCJSONValue("175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W"), std::runtime_error); - BOOST_CHECK_THROW(ParseNonRFCJSONValue("3J98t1WpEZ73CNmQviecrnyiWrnqRhWNL"), std::runtime_error); -} - BOOST_AUTO_TEST_CASE(rpc_ban) { BOOST_CHECK_NO_THROW(CallRPC(std::string("clearbanned"))); diff --git a/src/univalue/test/object.cpp b/src/univalue/test/object.cpp index 5ddf300393..5fb973c67b 100644 --- a/src/univalue/test/object.cpp +++ b/src/univalue/test/object.cpp @@ -412,6 +412,33 @@ void univalue_readwrite() BOOST_CHECK_EQUAL(strJson1, v.write()); + // Valid + BOOST_CHECK(v.read("1.0") && (v.get_real() == 1.0)); + BOOST_CHECK(v.read("true") && v.get_bool()); + BOOST_CHECK(v.read("[false]") && !v[0].get_bool()); + BOOST_CHECK(v.read("{\"a\": true}") && v["a"].get_bool()); + BOOST_CHECK(v.read("{\"1\": \"true\"}") && (v["1"].get_str() == "true")); + // Valid, with leading or trailing whitespace + BOOST_CHECK(v.read(" 1.0") && (v.get_real() == 1.0)); + BOOST_CHECK(v.read("1.0 ") && (v.get_real() == 1.0)); + BOOST_CHECK(v.read("0.00000000000000000000000000000000000001e+30 ") && v.get_real() == 1e-8); + + BOOST_CHECK(!v.read(".19e-6")); //should fail, missing leading 0, therefore invalid JSON + // Invalid, initial garbage + BOOST_CHECK(!v.read("[1.0")); + BOOST_CHECK(!v.read("a1.0")); + // Invalid, trailing garbage + BOOST_CHECK(!v.read("1.0sds")); + BOOST_CHECK(!v.read("1.0]")); + // Invalid, keys have to be names + BOOST_CHECK(!v.read("{1: \"true\"}")); + BOOST_CHECK(!v.read("{true: 1}")); + BOOST_CHECK(!v.read("{[1]: 1}")); + BOOST_CHECK(!v.read("{{\"a\": \"a\"}: 1}")); + // BTC addresses should fail parsing + BOOST_CHECK(!v.read("175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W")); + BOOST_CHECK(!v.read("3J98t1WpEZ73CNmQviecrnyiWrnqRhWNL")); + /* Check for (correctly reporting) a parsing error if the initial JSON construct is followed by more stuff. Note that whitespace is, of course, exempt. */ From cfbc8a623b5133f1d0b0c0c9be73b2b107e0d687 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 3 Mar 2023 15:07:31 +0000 Subject: [PATCH 031/867] refactor: rpc: hide and rename ParseNonRFCJSONValue() As per https://github.com/bitcoin/bitcoin/pull/26506#pullrequestreview-1211984059, this function is no longer necessary and we can use UniValue::read() directly. To avoid code duplication, we keep the function to throw on invalid input data but rename it to Parse() and remove it from the header. --- src/rpc/client.cpp | 22 ++++++++++------------ src/rpc/client.h | 5 ----- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 9449b9d197..2b517e77c8 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -222,6 +222,14 @@ static const CRPCConvertParam vRPCConvertParams[] = }; // clang-format on +/** Parse string to UniValue or throw runtime_error if string contains invalid JSON */ +static UniValue Parse(std::string_view raw) +{ + UniValue parsed; + if (!parsed.read(raw)) throw std::runtime_error(tfm::format("Error parsing JSON: %s", raw)); + return parsed; +} + class CRPCConvertTable { private: @@ -234,13 +242,13 @@ class CRPCConvertTable /** Return arg_value as UniValue, and first parse it if it is a non-string parameter */ UniValue ArgToUniValue(std::string_view arg_value, const std::string& method, int param_idx) { - return members.count({method, param_idx}) > 0 ? ParseNonRFCJSONValue(arg_value) : arg_value; + return members.count({method, param_idx}) > 0 ? Parse(arg_value) : arg_value; } /** Return arg_value as UniValue, and first parse it if it is a non-string parameter */ UniValue ArgToUniValue(std::string_view arg_value, const std::string& method, const std::string& param_name) { - return membersByName.count({method, param_name}) > 0 ? ParseNonRFCJSONValue(arg_value) : arg_value; + return membersByName.count({method, param_name}) > 0 ? Parse(arg_value) : arg_value; } }; @@ -254,16 +262,6 @@ CRPCConvertTable::CRPCConvertTable() static CRPCConvertTable rpcCvtTable; -/** Non-RFC4627 JSON parser, accepts internal values (such as numbers, true, false, null) - * as well as objects and arrays. - */ -UniValue ParseNonRFCJSONValue(std::string_view raw) -{ - UniValue parsed; - if (!parsed.read(raw)) throw std::runtime_error(tfm::format("Error parsing JSON: %s", raw)); - return parsed; -} - UniValue RPCConvertValues(const std::string &strMethod, const std::vector &strParams) { UniValue params(UniValue::VARR); diff --git a/src/rpc/client.h b/src/rpc/client.h index 3c5c4fc4d6..b67cd27fdf 100644 --- a/src/rpc/client.h +++ b/src/rpc/client.h @@ -17,9 +17,4 @@ UniValue RPCConvertValues(const std::string& strMethod, const std::vector& strParams); -/** Non-RFC4627 JSON parser, accepts internal values (such as numbers, true, false, null) - * as well as objects and arrays. - */ -UniValue ParseNonRFCJSONValue(std::string_view raw); - #endif // BITCOIN_RPC_CLIENT_H From b8401c3281978beed6198b2f9782b6a8dd35cbd7 Mon Sep 17 00:00:00 2001 From: Martin Leitner-Ankerl Date: Sat, 11 Jun 2022 09:23:51 +0200 Subject: [PATCH 032/867] Add pool based memory resource & allocator A memory resource similar to std::pmr::unsynchronized_pool_resource, but optimized for node-based containers. Co-Authored-By: Pieter Wuille --- src/Makefile.am | 1 + src/Makefile.bench.include | 1 + src/Makefile.test.include | 1 + src/Makefile.test_util.include | 1 + src/bench/pool.cpp | 50 +++++ src/support/allocators/pool.h | 349 +++++++++++++++++++++++++++++ src/test/pool_tests.cpp | 156 +++++++++++++ src/test/util/poolresourcetester.h | 129 +++++++++++ 8 files changed, 688 insertions(+) create mode 100644 src/bench/pool.cpp create mode 100644 src/support/allocators/pool.h create mode 100644 src/test/pool_tests.cpp create mode 100644 src/test/util/poolresourcetester.h diff --git a/src/Makefile.am b/src/Makefile.am index 53c809c901..985a8e8915 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -261,6 +261,7 @@ BITCOIN_CORE_H = \ shutdown.h \ signet.h \ streams.h \ + support/allocators/pool.h \ support/allocators/secure.h \ support/allocators/zeroafterfree.h \ support/cleanse.h \ diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index f1e4e706a1..c230728a1c 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -42,6 +42,7 @@ bench_bench_bitcoin_SOURCES = \ bench/nanobench.h \ bench/peer_eviction.cpp \ bench/poly1305.cpp \ + bench/pool.cpp \ bench/prevector.cpp \ bench/rollingbloom.cpp \ bench/rpc_blockchain.cpp \ diff --git a/src/Makefile.test.include b/src/Makefile.test.include index a39b0abd9d..98ed3ba4ad 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -116,6 +116,7 @@ BITCOIN_TESTS =\ test/pmt_tests.cpp \ test/policy_fee_tests.cpp \ test/policyestimator_tests.cpp \ + test/pool_tests.cpp \ test/pow_tests.cpp \ test/prevector_tests.cpp \ test/raii_event_tests.cpp \ diff --git a/src/Makefile.test_util.include b/src/Makefile.test_util.include index aefefe789a..11b93ad13e 100644 --- a/src/Makefile.test_util.include +++ b/src/Makefile.test_util.include @@ -15,6 +15,7 @@ TEST_UTIL_H = \ test/util/logging.h \ test/util/mining.h \ test/util/net.h \ + test/util/poolresourcetester.h \ test/util/random.h \ test/util/script.h \ test/util/setup_common.h \ diff --git a/src/bench/pool.cpp b/src/bench/pool.cpp new file mode 100644 index 0000000000..b3e54d85a2 --- /dev/null +++ b/src/bench/pool.cpp @@ -0,0 +1,50 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include + +#include + +template +void BenchFillClearMap(benchmark::Bench& bench, Map& map) +{ + size_t batch_size = 5000; + + // make sure each iteration of the benchmark contains exactly 5000 inserts and one clear. + // do this at least 10 times so we get reasonable accurate results + + bench.batch(batch_size).minEpochIterations(10).run([&] { + auto rng = ankerl::nanobench::Rng(1234); + for (size_t i = 0; i < batch_size; ++i) { + map[rng()]; + } + map.clear(); + }); +} + +static void PoolAllocator_StdUnorderedMap(benchmark::Bench& bench) +{ + auto map = std::unordered_map(); + BenchFillClearMap(bench, map); +} + +static void PoolAllocator_StdUnorderedMapWithPoolResource(benchmark::Bench& bench) +{ + using Map = std::unordered_map, + std::equal_to, + PoolAllocator, + sizeof(std::pair) + 4 * sizeof(void*), + alignof(void*)>>; + + // make sure the resource supports large enough pools to hold the node. We do this by adding the size of a few pointers to it. + auto pool_resource = Map::allocator_type::ResourceType(); + auto map = Map{0, std::hash{}, std::equal_to{}, &pool_resource}; + BenchFillClearMap(bench, map); +} + +BENCHMARK(PoolAllocator_StdUnorderedMap, benchmark::PriorityLevel::HIGH); +BENCHMARK(PoolAllocator_StdUnorderedMapWithPoolResource, benchmark::PriorityLevel::HIGH); diff --git a/src/support/allocators/pool.h b/src/support/allocators/pool.h new file mode 100644 index 0000000000..c8e70ebacf --- /dev/null +++ b/src/support/allocators/pool.h @@ -0,0 +1,349 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_SUPPORT_ALLOCATORS_POOL_H +#define BITCOIN_SUPPORT_ALLOCATORS_POOL_H + +#include +#include +#include +#include +#include +#include +#include +#include + +/** + * A memory resource similar to std::pmr::unsynchronized_pool_resource, but + * optimized for node-based containers. It has the following properties: + * + * * Owns the allocated memory and frees it on destruction, even when deallocate + * has not been called on the allocated blocks. + * + * * Consists of a number of pools, each one for a different block size. + * Each pool holds blocks of uniform size in a freelist. + * + * * Exhausting memory in a freelist causes a new allocation of a fixed size chunk. + * This chunk is used to carve out blocks. + * + * * Block sizes or alignments that can not be served by the pools are allocated + * and deallocated by operator new(). + * + * PoolResource is not thread-safe. It is intended to be used by PoolAllocator. + * + * @tparam MAX_BLOCK_SIZE_BYTES Maximum size to allocate with the pool. If larger + * sizes are requested, allocation falls back to new(). + * + * @tparam ALIGN_BYTES Required alignment for the allocations. + * + * An example: If you create a PoolResource<128, 8>(262144) and perform a bunch of + * allocations and deallocate 2 blocks with size 8 bytes, and 3 blocks with size 16, + * the members will look like this: + * + * m_free_lists m_allocated_chunks + * ┌───┐ ┌───┐ ┌────────────-------──────┐ + * │ │ blocks │ ├─►│ 262144 B │ + * │ │ ┌─────┐ ┌─────┐ └─┬─┘ └────────────-------──────┘ + * │ 1 ├─►│ 8 B ├─►│ 8 B │ │ + * │ │ └─────┘ └─────┘ : + * │ │ │ + * │ │ ┌─────┐ ┌─────┐ ┌─────┐ ▼ + * │ 2 ├─►│16 B ├─►│16 B ├─►│16 B │ ┌───┐ ┌─────────────────────────┐ + * │ │ └─────┘ └─────┘ └─────┘ │ ├─►│ ▲ │ ▲ + * │ │ └───┘ └──────────┬──────────────┘ │ + * │ . │ │ m_available_memory_end + * │ . │ m_available_memory_it + * │ . │ + * │ │ + * │ │ + * │16 │ + * └───┘ + * + * Here m_free_lists[1] holds the 2 blocks of size 8 bytes, and m_free_lists[2] + * holds the 3 blocks of size 16. The blocks came from the data stored in the + * m_allocated_chunks list. Each chunk has bytes 262144. The last chunk has still + * some memory available for the blocks, and when m_available_memory_it is at the + * end, a new chunk will be allocated and added to the list. + */ +template +class PoolResource final +{ + static_assert(ALIGN_BYTES > 0, "ALIGN_BYTES must be nonzero"); + static_assert((ALIGN_BYTES & (ALIGN_BYTES - 1)) == 0, "ALIGN_BYTES must be a power of two"); + + /** + * In-place linked list of the allocations, used for the freelist. + */ + struct ListNode { + ListNode* m_next; + + explicit ListNode(ListNode* next) : m_next(next) {} + }; + static_assert(std::is_trivially_destructible_v, "Make sure we don't need to manually call a destructor"); + + /** + * Internal alignment value. The larger of the requested ALIGN_BYTES and alignof(FreeList). + */ + static constexpr std::size_t ELEM_ALIGN_BYTES = std::max(alignof(ListNode), ALIGN_BYTES); + static_assert((ELEM_ALIGN_BYTES & (ELEM_ALIGN_BYTES - 1)) == 0, "ELEM_ALIGN_BYTES must be a power of two"); + static_assert(sizeof(ListNode) <= ELEM_ALIGN_BYTES, "Units of size ELEM_SIZE_ALIGN need to be able to store a ListNode"); + static_assert((MAX_BLOCK_SIZE_BYTES & (ELEM_ALIGN_BYTES - 1)) == 0, "MAX_BLOCK_SIZE_BYTES needs to be a multiple of the alignment."); + + /** + * Size in bytes to allocate per chunk + */ + const size_t m_chunk_size_bytes; + + /** + * Contains all allocated pools of memory, used to free the data in the destructor. + */ + std::list m_allocated_chunks{}; + + /** + * Single linked lists of all data that came from deallocating. + * m_free_lists[n] will serve blocks of size n*ELEM_ALIGN_BYTES. + */ + std::array m_free_lists{}; + + /** + * Points to the beginning of available memory for carving out allocations. + */ + std::byte* m_available_memory_it = nullptr; + + /** + * Points to the end of available memory for carving out allocations. + * + * That member variable is redundant, and is always equal to `m_allocated_chunks.back() + m_chunk_size_bytes` + * whenever it is accessed, but `m_available_memory_end` caches this for clarity and efficiency. + */ + std::byte* m_available_memory_end = nullptr; + + /** + * How many multiple of ELEM_ALIGN_BYTES are necessary to fit bytes. We use that result directly as an index + * into m_free_lists. Round up for the special case when bytes==0. + */ + [[nodiscard]] static constexpr std::size_t NumElemAlignBytes(std::size_t bytes) + { + return (bytes + ELEM_ALIGN_BYTES - 1) / ELEM_ALIGN_BYTES + (bytes == 0); + } + + /** + * True when it is possible to make use of the freelist + */ + [[nodiscard]] static constexpr bool IsFreeListUsable(std::size_t bytes, std::size_t alignment) + { + return alignment <= ELEM_ALIGN_BYTES && bytes <= MAX_BLOCK_SIZE_BYTES; + } + + /** + * Replaces node with placement constructed ListNode that points to the previous node + */ + void PlacementAddToList(void* p, ListNode*& node) + { + node = new (p) ListNode{node}; + } + + /** + * Allocate one full memory chunk which will be used to carve out allocations. + * Also puts any leftover bytes into the freelist. + * + * Precondition: leftover bytes are either 0 or few enough to fit into a place in the freelist + */ + void AllocateChunk() + { + // if there is still any available memory left, put it into the freelist. + size_t remaining_available_bytes = std::distance(m_available_memory_it, m_available_memory_end); + if (0 != remaining_available_bytes) { + PlacementAddToList(m_available_memory_it, m_free_lists[remaining_available_bytes / ELEM_ALIGN_BYTES]); + } + + void* storage = ::operator new (m_chunk_size_bytes, std::align_val_t{ELEM_ALIGN_BYTES}); + m_available_memory_it = new (storage) std::byte[m_chunk_size_bytes]; + m_available_memory_end = m_available_memory_it + m_chunk_size_bytes; + m_allocated_chunks.emplace_back(m_available_memory_it); + } + + /** + * Access to internals for testing purpose only + */ + friend class PoolResourceTester; + +public: + /** + * Construct a new PoolResource object which allocates the first chunk. + * chunk_size_bytes will be rounded up to next multiple of ELEM_ALIGN_BYTES. + */ + explicit PoolResource(std::size_t chunk_size_bytes) + : m_chunk_size_bytes(NumElemAlignBytes(chunk_size_bytes) * ELEM_ALIGN_BYTES) + { + assert(m_chunk_size_bytes >= MAX_BLOCK_SIZE_BYTES); + AllocateChunk(); + } + + /** + * Construct a new Pool Resource object, defaults to 2^18=262144 chunk size. + */ + PoolResource() : PoolResource(262144) {} + + /** + * Disable copy & move semantics, these are not supported for the resource. + */ + PoolResource(const PoolResource&) = delete; + PoolResource& operator=(const PoolResource&) = delete; + PoolResource(PoolResource&&) = delete; + PoolResource& operator=(PoolResource&&) = delete; + + /** + * Deallocates all memory allocated associated with the memory resource. + */ + ~PoolResource() + { + for (std::byte* chunk : m_allocated_chunks) { + std::destroy(chunk, chunk + m_chunk_size_bytes); + ::operator delete ((void*)chunk, std::align_val_t{ELEM_ALIGN_BYTES}); + } + } + + /** + * Allocates a block of bytes. If possible the freelist is used, otherwise allocation + * is forwarded to ::operator new(). + */ + void* Allocate(std::size_t bytes, std::size_t alignment) + { + if (IsFreeListUsable(bytes, alignment)) { + const std::size_t num_alignments = NumElemAlignBytes(bytes); + if (nullptr != m_free_lists[num_alignments]) { + // we've already got data in the pool's freelist, unlink one element and return the pointer + // to the unlinked memory. Since FreeList is trivially destructible we can just treat it as + // uninitialized memory. + return std::exchange(m_free_lists[num_alignments], m_free_lists[num_alignments]->m_next); + } + + // freelist is empty: get one allocation from allocated chunk memory. + const std::ptrdiff_t round_bytes = static_cast(num_alignments * ELEM_ALIGN_BYTES); + if (round_bytes > m_available_memory_end - m_available_memory_it) { + // slow path, only happens when a new chunk needs to be allocated + AllocateChunk(); + } + + // Make sure we use the right amount of bytes for that freelist (might be rounded up), + return std::exchange(m_available_memory_it, m_available_memory_it + round_bytes); + } + + // Can't use the pool => use operator new() + return ::operator new (bytes, std::align_val_t{alignment}); + } + + /** + * Returns a block to the freelists, or deletes the block when it did not come from the chunks. + */ + void Deallocate(void* p, std::size_t bytes, std::size_t alignment) noexcept + { + if (IsFreeListUsable(bytes, alignment)) { + const std::size_t num_alignments = NumElemAlignBytes(bytes); + // put the memory block into the linked list. We can placement construct the FreeList + // into the memory since we can be sure the alignment is correct. + PlacementAddToList(p, m_free_lists[num_alignments]); + } else { + // Can't use the pool => forward deallocation to ::operator delete(). + ::operator delete (p, std::align_val_t{alignment}); + } + } + + /** + * Number of allocated chunks + */ + [[nodiscard]] std::size_t NumAllocatedChunks() const + { + return m_allocated_chunks.size(); + } + + /** + * Size in bytes to allocate per chunk, currently hardcoded to a fixed size. + */ + [[nodiscard]] size_t ChunkSizeBytes() const + { + return m_chunk_size_bytes; + } +}; + + +/** + * Forwards all allocations/deallocations to the PoolResource. + */ +template +class PoolAllocator +{ + PoolResource* m_resource; + + template + friend class PoolAllocator; + +public: + using value_type = T; + using ResourceType = PoolResource; + + /** + * Not explicit so we can easily construct it with the correct resource + */ + PoolAllocator(ResourceType* resource) noexcept + : m_resource(resource) + { + } + + PoolAllocator(const PoolAllocator& other) noexcept = default; + PoolAllocator& operator=(const PoolAllocator& other) noexcept = default; + + template + PoolAllocator(const PoolAllocator& other) noexcept + : m_resource(other.resource()) + { + } + + /** + * The rebind struct here is mandatory because we use non type template arguments for + * PoolAllocator. See https://en.cppreference.com/w/cpp/named_req/Allocator#cite_note-2 + */ + template + struct rebind { + using other = PoolAllocator; + }; + + /** + * Forwards each call to the resource. + */ + T* allocate(size_t n) + { + return static_cast(m_resource->Allocate(n * sizeof(T), alignof(T))); + } + + /** + * Forwards each call to the resource. + */ + void deallocate(T* p, size_t n) noexcept + { + m_resource->Deallocate(p, n * sizeof(T), alignof(T)); + } + + ResourceType* resource() const noexcept + { + return m_resource; + } +}; + +template +bool operator==(const PoolAllocator& a, + const PoolAllocator& b) noexcept +{ + return a.resource() == b.resource(); +} + +template +bool operator!=(const PoolAllocator& a, + const PoolAllocator& b) noexcept +{ + return !(a == b); +} + +#endif // BITCOIN_SUPPORT_ALLOCATORS_POOL_H diff --git a/src/test/pool_tests.cpp b/src/test/pool_tests.cpp new file mode 100644 index 0000000000..dfe857d05b --- /dev/null +++ b/src/test/pool_tests.cpp @@ -0,0 +1,156 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include + +#include + +#include +#include +#include +#include + +BOOST_FIXTURE_TEST_SUITE(pool_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(basic_allocating) +{ + auto resource = PoolResource<8, 8>(); + PoolResourceTester::CheckAllDataAccountedFor(resource); + + // first chunk is already allocated + size_t expected_bytes_available = resource.ChunkSizeBytes(); + BOOST_TEST(expected_bytes_available == PoolResourceTester::AvailableMemoryFromChunk(resource)); + + // chunk is used, no more allocation + void* block = resource.Allocate(8, 8); + expected_bytes_available -= 8; + BOOST_TEST(expected_bytes_available == PoolResourceTester::AvailableMemoryFromChunk(resource)); + + BOOST_TEST(0 == PoolResourceTester::FreeListSizes(resource)[1]); + resource.Deallocate(block, 8, 8); + PoolResourceTester::CheckAllDataAccountedFor(resource); + BOOST_TEST(1 == PoolResourceTester::FreeListSizes(resource)[1]); + + // alignment is too small, but the best fitting freelist is used. Nothing is allocated. + void* b = resource.Allocate(8, 1); + BOOST_TEST(b == block); // we got the same block of memory as before + BOOST_TEST(0 == PoolResourceTester::FreeListSizes(resource)[1]); + BOOST_TEST(expected_bytes_available == PoolResourceTester::AvailableMemoryFromChunk(resource)); + + resource.Deallocate(block, 8, 1); + PoolResourceTester::CheckAllDataAccountedFor(resource); + BOOST_TEST(1 == PoolResourceTester::FreeListSizes(resource)[1]); + BOOST_TEST(expected_bytes_available == PoolResourceTester::AvailableMemoryFromChunk(resource)); + + // can't use resource because alignment is too big, allocate system memory + b = resource.Allocate(8, 16); + BOOST_TEST(b != block); + block = b; + PoolResourceTester::CheckAllDataAccountedFor(resource); + BOOST_TEST(1 == PoolResourceTester::FreeListSizes(resource)[1]); + BOOST_TEST(expected_bytes_available == PoolResourceTester::AvailableMemoryFromChunk(resource)); + + resource.Deallocate(block, 8, 16); + PoolResourceTester::CheckAllDataAccountedFor(resource); + BOOST_TEST(1 == PoolResourceTester::FreeListSizes(resource)[1]); + BOOST_TEST(expected_bytes_available == PoolResourceTester::AvailableMemoryFromChunk(resource)); + + // can't use chunk because size is too big + block = resource.Allocate(16, 8); + PoolResourceTester::CheckAllDataAccountedFor(resource); + BOOST_TEST(1 == PoolResourceTester::FreeListSizes(resource)[1]); + BOOST_TEST(expected_bytes_available == PoolResourceTester::AvailableMemoryFromChunk(resource)); + + resource.Deallocate(block, 16, 8); + PoolResourceTester::CheckAllDataAccountedFor(resource); + BOOST_TEST(1 == PoolResourceTester::FreeListSizes(resource)[1]); + BOOST_TEST(expected_bytes_available == PoolResourceTester::AvailableMemoryFromChunk(resource)); + + // it's possible that 0 bytes are allocated, make sure this works. In that case the call is forwarded to operator new + // 0 bytes takes one entry from the first freelist + void* p = resource.Allocate(0, 1); + BOOST_TEST(0 == PoolResourceTester::FreeListSizes(resource)[1]); + BOOST_TEST(expected_bytes_available == PoolResourceTester::AvailableMemoryFromChunk(resource)); + + resource.Deallocate(p, 0, 1); + PoolResourceTester::CheckAllDataAccountedFor(resource); + BOOST_TEST(1 == PoolResourceTester::FreeListSizes(resource)[1]); + BOOST_TEST(expected_bytes_available == PoolResourceTester::AvailableMemoryFromChunk(resource)); +} + +// Allocates from 0 to n bytes were n > the PoolResource's data, and each should work +BOOST_AUTO_TEST_CASE(allocate_any_byte) +{ + auto resource = PoolResource<128, 8>(1024); + + uint8_t num_allocs = 200; + + auto data = std::vector>(); + + // allocate an increasing number of bytes + for (uint8_t num_bytes = 0; num_bytes < num_allocs; ++num_bytes) { + uint8_t* bytes = new (resource.Allocate(num_bytes, 1)) uint8_t[num_bytes]; + BOOST_TEST(bytes != nullptr); + data.emplace_back(bytes, num_bytes); + + // set each byte to num_bytes + std::fill(bytes, bytes + num_bytes, num_bytes); + } + + // now that we got all allocated, test if all still have the correct values, and give everything back to the allocator + uint8_t val = 0; + for (auto const& span : data) { + for (auto x : span) { + BOOST_TEST(val == x); + } + std::destroy(span.data(), span.data() + span.size()); + resource.Deallocate(span.data(), span.size(), 1); + ++val; + } + + PoolResourceTester::CheckAllDataAccountedFor(resource); +} + +BOOST_AUTO_TEST_CASE(random_allocations) +{ + struct PtrSizeAlignment { + void* ptr; + size_t bytes; + size_t alignment; + }; + + // makes a bunch of random allocations and gives all of them back in random order. + auto resource = PoolResource<128, 8>(65536); + std::vector ptr_size_alignment{}; + for (size_t i = 0; i < 1000; ++i) { + // make it a bit more likely to allocate than deallocate + if (ptr_size_alignment.empty() || 0 != InsecureRandRange(4)) { + // allocate a random item + std::size_t alignment = std::size_t{1} << InsecureRandRange(8); // 1, 2, ..., 128 + std::size_t size = (InsecureRandRange(200) / alignment + 1) * alignment; // multiple of alignment + void* ptr = resource.Allocate(size, alignment); + BOOST_TEST(ptr != nullptr); + BOOST_TEST((reinterpret_cast(ptr) & (alignment - 1)) == 0); + ptr_size_alignment.push_back({ptr, size, alignment}); + } else { + // deallocate a random item + auto& x = ptr_size_alignment[InsecureRandRange(ptr_size_alignment.size())]; + resource.Deallocate(x.ptr, x.bytes, x.alignment); + x = ptr_size_alignment.back(); + ptr_size_alignment.pop_back(); + } + } + + // deallocate all the rest + for (auto const& x : ptr_size_alignment) { + resource.Deallocate(x.ptr, x.bytes, x.alignment); + } + + PoolResourceTester::CheckAllDataAccountedFor(resource); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/util/poolresourcetester.h b/src/test/util/poolresourcetester.h new file mode 100644 index 0000000000..93f62eb2a9 --- /dev/null +++ b/src/test/util/poolresourcetester.h @@ -0,0 +1,129 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_TEST_UTIL_POOLRESOURCETESTER_H +#define BITCOIN_TEST_UTIL_POOLRESOURCETESTER_H + +#include + +#include +#include +#include +#include +#include + +/** + * Helper to get access to private parts of PoolResource. Used in unit tests and in the fuzzer + */ +class PoolResourceTester +{ + struct PtrAndBytes { + uintptr_t ptr; + std::size_t size; + + PtrAndBytes(const void* p, std::size_t s) + : ptr(reinterpret_cast(p)), size(s) + { + } + + /** + * defines a sort ordering by the pointer value + */ + friend bool operator<(PtrAndBytes const& a, PtrAndBytes const& b) + { + return a.ptr < b.ptr; + } + }; + +public: + /** + * Extracts the number of elements per freelist + */ + template + static std::vector FreeListSizes(const PoolResource& resource) + { + auto sizes = std::vector(); + for (const auto* ptr : resource.m_free_lists) { + size_t size = 0; + while (ptr != nullptr) { + ++size; + ptr = ptr->m_next; + } + sizes.push_back(size); + } + return sizes; + } + + /** + * How many bytes are still available from the last allocated chunk + */ + template + static std::size_t AvailableMemoryFromChunk(const PoolResource& resource) + { + return resource.m_available_memory_end - resource.m_available_memory_it; + } + + /** + * Once all blocks are given back to the resource, tests that the freelists are consistent: + * + * * All data in the freelists must come from the chunks + * * Memory doesn't overlap + * * Each byte in the chunks can be accounted for in either the freelist or as available bytes. + */ + template + static void CheckAllDataAccountedFor(const PoolResource& resource) + { + // collect all free blocks by iterating all freelists + std::vector free_blocks; + for (std::size_t freelist_idx = 0; freelist_idx < resource.m_free_lists.size(); ++freelist_idx) { + std::size_t bytes = freelist_idx * resource.ELEM_ALIGN_BYTES; + auto* ptr = resource.m_free_lists[freelist_idx]; + while (ptr != nullptr) { + free_blocks.emplace_back(ptr, bytes); + ptr = ptr->m_next; + } + } + // also add whatever has not yet been used for blocks + auto num_available_bytes = resource.m_available_memory_end - resource.m_available_memory_it; + if (num_available_bytes > 0) { + free_blocks.emplace_back(resource.m_available_memory_it, num_available_bytes); + } + + // collect all chunks + std::vector chunks; + for (const std::byte* ptr : resource.m_allocated_chunks) { + chunks.emplace_back(ptr, resource.ChunkSizeBytes()); + } + + // now we have all the data from all freelists on the one hand side, and all chunks on the other hand side. + // To check if all of them match, sort by address and iterate. + std::sort(free_blocks.begin(), free_blocks.end()); + std::sort(chunks.begin(), chunks.end()); + + auto chunk_it = chunks.begin(); + auto chunk_ptr_remaining = chunk_it->ptr; + auto chunk_size_remaining = chunk_it->size; + for (const auto& free_block : free_blocks) { + if (chunk_size_remaining == 0) { + assert(chunk_it != chunks.end()); + ++chunk_it; + assert(chunk_it != chunks.end()); + chunk_ptr_remaining = chunk_it->ptr; + chunk_size_remaining = chunk_it->size; + } + assert(free_block.ptr == chunk_ptr_remaining); // ensure addresses match + assert(free_block.size <= chunk_size_remaining); // ensure no overflow + assert((free_block.ptr & (resource.ELEM_ALIGN_BYTES - 1)) == 0); // ensure correct alignment + chunk_ptr_remaining += free_block.size; + chunk_size_remaining -= free_block.size; + } + // ensure we are at the end of the chunks + assert(chunk_ptr_remaining == chunk_it->ptr + chunk_it->size); + ++chunk_it; + assert(chunk_it == chunks.end()); + assert(chunk_size_remaining == 0); + } +}; + +#endif // BITCOIN_TEST_UTIL_POOLRESOURCETESTER_H From e19943f049ed8aa4f32a1d8440a9fbf160367f0f Mon Sep 17 00:00:00 2001 From: Martin Leitner-Ankerl Date: Sat, 11 Jun 2022 09:28:13 +0200 Subject: [PATCH 033/867] Calculate memory usage correctly for unordered_maps that use PoolAllocator Extracts the resource from a PoolAllocator and uses it for calculation of the node's memory usage. --- src/memusage.h | 20 ++++++++++++++++++++ src/test/pool_tests.cpp | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/src/memusage.h b/src/memusage.h index 9755be0ff5..bb39066a7d 100644 --- a/src/memusage.h +++ b/src/memusage.h @@ -7,6 +7,7 @@ #include #include +#include #include #include @@ -166,6 +167,25 @@ static inline size_t DynamicUsage(const std::unordered_map& m) return MallocUsage(sizeof(unordered_node >)) * m.size() + MallocUsage(sizeof(void*) * m.bucket_count()); } +template +static inline size_t DynamicUsage(const std::unordered_map, + MAX_BLOCK_SIZE_BYTES, + ALIGN_BYTES>>& m) +{ + auto* pool_resource = m.get_allocator().resource(); + + // The allocated chunks are stored in a std::list. Size per node should + // therefore be 3 pointers: next, previous, and a pointer to the chunk. + size_t estimated_list_node_size = MallocUsage(sizeof(void*) * 3); + size_t usage_resource = estimated_list_node_size * pool_resource->NumAllocatedChunks(); + size_t usage_chunks = MallocUsage(pool_resource->ChunkSizeBytes()) * pool_resource->NumAllocatedChunks(); + return usage_resource + usage_chunks + MallocUsage(sizeof(void*) * m.bucket_count()); } +} // namespace memusage + #endif // BITCOIN_MEMUSAGE_H diff --git a/src/test/pool_tests.cpp b/src/test/pool_tests.cpp index dfe857d05b..8a07e09a44 100644 --- a/src/test/pool_tests.cpp +++ b/src/test/pool_tests.cpp @@ -2,6 +2,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include #include #include @@ -153,4 +154,37 @@ BOOST_AUTO_TEST_CASE(random_allocations) PoolResourceTester::CheckAllDataAccountedFor(resource); } +BOOST_AUTO_TEST_CASE(memusage_test) +{ + auto std_map = std::unordered_map{}; + + using Map = std::unordered_map, + std::equal_to, + PoolAllocator, + sizeof(std::pair) + sizeof(void*) * 4, + alignof(void*)>>; + auto resource = Map::allocator_type::ResourceType(1024); + + PoolResourceTester::CheckAllDataAccountedFor(resource); + + { + auto resource_map = Map{0, std::hash{}, std::equal_to{}, &resource}; + + // can't have the same resource usage + BOOST_TEST(memusage::DynamicUsage(std_map) != memusage::DynamicUsage(resource_map)); + + for (size_t i = 0; i < 10000; ++i) { + std_map[i]; + resource_map[i]; + } + + // Eventually the resource_map should have a much lower memory usage because it has less malloc overhead + BOOST_TEST(memusage::DynamicUsage(resource_map) <= memusage::DynamicUsage(std_map) * 90 / 100); + } + + PoolResourceTester::CheckAllDataAccountedFor(resource); +} + BOOST_AUTO_TEST_SUITE_END() From 1afca6b663bb54022afff193fd9d83856606b189 Mon Sep 17 00:00:00 2001 From: Martin Leitner-Ankerl Date: Sat, 11 Jun 2022 10:48:35 +0200 Subject: [PATCH 034/867] Add PoolResource fuzzer Fuzzes PoolResource with random allocations/deallocations, and multiple asserts. Co-Authored-By: Pieter Wuille --- src/Makefile.test.include | 1 + src/test/fuzz/poolresource.cpp | 174 +++++++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+) create mode 100644 src/test/fuzz/poolresource.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 98ed3ba4ad..fc49c1fd40 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -301,6 +301,7 @@ test_fuzz_fuzz_SOURCES = \ test/fuzz/partially_downloaded_block.cpp \ test/fuzz/policy_estimator.cpp \ test/fuzz/policy_estimator_io.cpp \ + test/fuzz/poolresource.cpp \ test/fuzz/pow.cpp \ test/fuzz/prevector.cpp \ test/fuzz/primitives_transaction.cpp \ diff --git a/src/test/fuzz/poolresource.cpp b/src/test/fuzz/poolresource.cpp new file mode 100644 index 0000000000..ce64ef6472 --- /dev/null +++ b/src/test/fuzz/poolresource.cpp @@ -0,0 +1,174 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +namespace { + +template +class PoolResourceFuzzer +{ + FuzzedDataProvider& m_provider; + PoolResource m_test_resource; + uint64_t m_sequence{0}; + size_t m_total_allocated{}; + + struct Entry { + Span span; + size_t alignment; + uint64_t seed; + + Entry(Span s, size_t a, uint64_t se) : span(s), alignment(a), seed(se) {} + }; + + std::vector m_entries; + +public: + PoolResourceFuzzer(FuzzedDataProvider& provider) + : m_provider{provider}, + m_test_resource{provider.ConsumeIntegralInRange(MAX_BLOCK_SIZE_BYTES, 262144)} + { + } + + void Allocate(size_t size, size_t alignment) + { + assert(size > 0); // Must allocate at least 1 byte. + assert(alignment > 0); // Alignment must be at least 1. + assert((alignment & (alignment - 1)) == 0); // Alignment must be power of 2. + assert((size & (alignment - 1)) == 0); // Size must be a multiple of alignment. + + auto span = Span(static_cast(m_test_resource.Allocate(size, alignment)), size); + m_total_allocated += size; + + auto ptr_val = reinterpret_cast(span.data()); + assert((ptr_val & (alignment - 1)) == 0); + + uint64_t seed = m_sequence++; + RandomContentFill(m_entries.emplace_back(span, alignment, seed)); + } + + void + Allocate() + { + if (m_total_allocated > 0x1000000) return; + size_t alignment_bits = m_provider.ConsumeIntegralInRange(0, 7); + size_t alignment = 1 << alignment_bits; + size_t size_bits = m_provider.ConsumeIntegralInRange(0, 16 - alignment_bits); + size_t size = m_provider.ConsumeIntegralInRange(1U << size_bits, (1U << (size_bits + 1)) - 1U) << alignment_bits; + Allocate(size, alignment); + } + + void RandomContentFill(Entry& entry) + { + XoRoShiRo128PlusPlus rng(entry.seed); + auto ptr = entry.span.data(); + auto size = entry.span.size(); + + while (size >= 8) { + auto r = rng(); + std::memcpy(ptr, &r, 8); + size -= 8; + ptr += 8; + } + if (size > 0) { + auto r = rng(); + std::memcpy(ptr, &r, size); + } + } + + void RandomContentCheck(const Entry& entry) + { + XoRoShiRo128PlusPlus rng(entry.seed); + auto ptr = entry.span.data(); + auto size = entry.span.size(); + + std::byte buf[8]; + while (size >= 8) { + auto r = rng(); + std::memcpy(buf, &r, 8); + assert(std::memcmp(buf, ptr, 8) == 0); + size -= 8; + ptr += 8; + } + if (size > 0) { + auto r = rng(); + std::memcpy(buf, &r, size); + assert(std::memcmp(buf, ptr, size) == 0); + } + } + + void Deallocate(const Entry& entry) + { + auto ptr_val = reinterpret_cast(entry.span.data()); + assert((ptr_val & (entry.alignment - 1)) == 0); + RandomContentCheck(entry); + m_total_allocated -= entry.span.size(); + m_test_resource.Deallocate(entry.span.data(), entry.span.size(), entry.alignment); + } + + void Deallocate() + { + if (m_entries.empty()) { + return; + } + + size_t idx = m_provider.ConsumeIntegralInRange(0, m_entries.size() - 1); + Deallocate(m_entries[idx]); + if (idx != m_entries.size() - 1) { + m_entries[idx] = std::move(m_entries.back()); + } + m_entries.pop_back(); + } + + void Clear() + { + while (!m_entries.empty()) { + Deallocate(); + } + + PoolResourceTester::CheckAllDataAccountedFor(m_test_resource); + } + + void Fuzz() + { + LIMITED_WHILE(m_provider.ConsumeBool(), 10000) + { + CallOneOf( + m_provider, + [&] { Allocate(); }, + [&] { Deallocate(); }); + } + Clear(); + } +}; + + +} // namespace + +FUZZ_TARGET(pool_resource) +{ + FuzzedDataProvider provider(buffer.data(), buffer.size()); + CallOneOf( + provider, + [&] { PoolResourceFuzzer<128, 1>{provider}.Fuzz(); }, + [&] { PoolResourceFuzzer<128, 2>{provider}.Fuzz(); }, + [&] { PoolResourceFuzzer<128, 4>{provider}.Fuzz(); }, + [&] { PoolResourceFuzzer<128, 8>{provider}.Fuzz(); }, + + [&] { PoolResourceFuzzer<8, 8>{provider}.Fuzz(); }, + [&] { PoolResourceFuzzer<16, 16>{provider}.Fuzz(); }, + + [&] { PoolResourceFuzzer<256, alignof(max_align_t)>{provider}.Fuzz(); }, + [&] { PoolResourceFuzzer<256, 64>{provider}.Fuzz(); }); +} From 5e4ac5abf54f8e6d6330df0c73119aa0cca4c103 Mon Sep 17 00:00:00 2001 From: Martin Leitner-Ankerl Date: Sat, 11 Jun 2022 11:00:53 +0200 Subject: [PATCH 035/867] Call ReallocateCache() on each Flush() This frees up all associated memory with the map, not only the nodes. This is necessary in preparation for using the PoolAllocator for CCoinsMap, which does not actually free any memory on clear(). --- src/coins.cpp | 9 ++++++--- src/test/validation_flush_tests.cpp | 5 ++--- src/validation.cpp | 1 - 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 5a6ae525a7..f55932f302 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -253,9 +253,12 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn bool CCoinsViewCache::Flush() { bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/true); - if (fOk && !cacheCoins.empty()) { - /* BatchWrite must erase all cacheCoins elements when erase=true. */ - throw std::logic_error("Not all cached coins were erased"); + if (fOk) { + if (!cacheCoins.empty()) { + /* BatchWrite must erase all cacheCoins elements when erase=true. */ + throw std::logic_error("Not all cached coins were erased"); + } + ReallocateCache(); } cachedCoinsUsage = 0; return fOk; diff --git a/src/test/validation_flush_tests.cpp b/src/test/validation_flush_tests.cpp index 26c48eb0e0..205164b94c 100644 --- a/src/test/validation_flush_tests.cpp +++ b/src/test/validation_flush_tests.cpp @@ -131,8 +131,7 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate) CoinsCacheSizeState::OK); } - // Flushing the view doesn't take us back to OK because cacheCoins has - // preallocated memory that doesn't get reclaimed even after flush. + // Flushing the view does take us back to OK because ReallocateCache() is called BOOST_CHECK_EQUAL( chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, 0), @@ -144,7 +143,7 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate) BOOST_CHECK_EQUAL( chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, 0), - CoinsCacheSizeState::CRITICAL); + CoinsCacheSizeState::OK); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validation.cpp b/src/validation.cpp index e82fead89e..cf7688ea9f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4930,7 +4930,6 @@ bool Chainstate::ResizeCoinsCaches(size_t coinstip_size, size_t coinsdb_size) } else { // Otherwise, flush state to disk and deallocate the in-memory coins map. ret = FlushStateToDisk(state, FlushStateMode::ALWAYS); - CoinsTip().ReallocateCache(); } return ret; } From 9f947fc3d4b779f017332135323b34e8f216f613 Mon Sep 17 00:00:00 2001 From: Martin Leitner-Ankerl Date: Sat, 11 Jun 2022 11:27:38 +0200 Subject: [PATCH 036/867] Use PoolAllocator for CCoinsMap In my benchmarks, using this pool allocator for CCoinsMap gives about 20% faster `-reindex-chainstate` with -dbcache=5000 with practically the same memory usage. The change in max RSS changed was 0.3%. The `validation_flush_tests` tests need to be updated because memory allocation is now done in large pools instead of one node at a time, so the limits need to be updated accordingly. --- src/coins.cpp | 6 +++-- src/coins.h | 20 ++++++++++++++- src/test/coins_tests.cpp | 39 ++++++++++++++++++++++++++--- src/test/fuzz/coins_view.cpp | 3 ++- src/test/validation_flush_tests.cpp | 28 +++++++++++++-------- 5 files changed, 79 insertions(+), 17 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index f55932f302..0fe642e46b 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -34,7 +34,7 @@ size_t CCoinsViewBacked::EstimateSize() const { return base->EstimateSize(); } CCoinsViewCache::CCoinsViewCache(CCoinsView* baseIn, bool deterministic) : CCoinsViewBacked(baseIn), m_deterministic(deterministic), - cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/deterministic)) + cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/deterministic), CCoinsMap::key_equal{}, &m_cache_coins_memory_resource) {} size_t CCoinsViewCache::DynamicMemoryUsage() const { @@ -317,7 +317,9 @@ void CCoinsViewCache::ReallocateCache() // Cache should be empty when we're calling this. assert(cacheCoins.size() == 0); cacheCoins.~CCoinsMap(); - ::new (&cacheCoins) CCoinsMap(0, SaltedOutpointHasher(/*deterministic=*/m_deterministic)); + m_cache_coins_memory_resource.~CCoinsMapMemoryResource(); + ::new (&m_cache_coins_memory_resource) CCoinsMapMemoryResource{}; + ::new (&cacheCoins) CCoinsMap{0, SaltedOutpointHasher{/*deterministic=*/m_deterministic}, CCoinsMap::key_equal{}, &m_cache_coins_memory_resource}; } void CCoinsViewCache::SanityCheck() const diff --git a/src/coins.h b/src/coins.h index dd336b210a..039a07054d 100644 --- a/src/coins.h +++ b/src/coins.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -131,7 +132,23 @@ struct CCoinsCacheEntry CCoinsCacheEntry(Coin&& coin_, unsigned char flag) : coin(std::move(coin_)), flags(flag) {} }; -typedef std::unordered_map CCoinsMap; +/** + * PoolAllocator's MAX_BLOCK_SIZE_BYTES parameter here uses sizeof the data, and adds the size + * of 4 pointers. We do not know the exact node size used in the std::unordered_node implementation + * because it is implementation defined. Most implementations have an overhead of 1 or 2 pointers, + * so nodes can be connected in a linked list, and in some cases the hash value is stored as well. + * Using an additional sizeof(void*)*4 for MAX_BLOCK_SIZE_BYTES should thus be sufficient so that + * all implementations can allocate the nodes from the PoolAllocator. + */ +using CCoinsMap = std::unordered_map, + PoolAllocator, + sizeof(std::pair) + sizeof(void*) * 4, + alignof(void*)>>; + +using CCoinsMapMemoryResource = CCoinsMap::allocator_type::ResourceType; /** Cursor for iterating over CoinsView state */ class CCoinsViewCursor @@ -220,6 +237,7 @@ class CCoinsViewCache : public CCoinsViewBacked * declared as "const". */ mutable uint256 hashBlock; + mutable CCoinsMapMemoryResource m_cache_coins_memory_resource{}; mutable CCoinsMap cacheCoins; /* Cached dynamic memory usage for the inner Coin objects. */ diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index e082800fc3..853dc6dc1e 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -6,6 +6,7 @@ #include #include