From 8d056b82eedcc4766f9b120dde5e151ab8679030 Mon Sep 17 00:00:00 2001 From: MacroFake Date: Thu, 30 Jun 2022 18:38:11 +0200 Subject: [PATCH 1/5] Merge bitcoin/bitcoin#25383: wallet: don't read db every time that a new 'WalletBatch' is created c318211ddd48d44dd81dded553afeee3bc41c89e walletdb: fix last client version update (furszy) bda8ebe608e6572eaaf40cd28dab6954241c9b0d wallet: don't read db every time that a new WalletBatch is created (furszy) Pull request description: Found it while was working on #25297. We are performing a db read operation every time that a new `WalletBatch` is created, inside the constructor, just to check if the client version field is inside the db or not. As the client version field does not change in the entire db lifecycle, this operation can be done only once: The first time that the db is accessed/opened and the client version value can be cached. ACKs for top commit: achow101: ACK c318211ddd48d44dd81dded553afeee3bc41c89e w0xlt: reACK https://github.com/bitcoin/bitcoin/pull/25383/commits/c318211ddd48d44dd81dded553afeee3bc41c89e Tree-SHA512: 7fb780c656e169e8eb21e7212242494a647f6506d6da2cca828703713d440d29c82bec9e7d2c410f37b49361226ccd80846d3eeb8168383d0c2a11d85d73bee2 --- src/wallet/bdb.cpp | 6 ------ src/wallet/walletdb.cpp | 10 ++++------ 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index ef6947a25f949..98e309e0b9cbe 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -320,12 +320,6 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool read_only, b env = database.env.get(); pdb = database.m_db.get(); strFile = database.strFile; - if (!Exists(std::string("version"))) { - bool fTmp = fReadOnly; - fReadOnly = false; - Write(std::string("version"), CLIENT_VERSION); - fReadOnly = fTmp; - } } void BerkeleyDatabase::Open() diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 5992579d857f0..a518f1d642f82 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -862,12 +862,10 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) if (result != DBErrors::LOAD_OK) return result; - // Last client version to open this wallet, was previously the file version number + // Last client version to open this wallet int last_client = CLIENT_VERSION; - m_batch->Read(DBKeys::VERSION, last_client); - - int wallet_version = pwallet->GetVersion(); - pwallet->WalletLogPrintf("Wallet File Version = %d\n", wallet_version > 0 ? wallet_version : last_client); + bool has_last_client = m_batch->Read(DBKeys::VERSION, last_client); + pwallet->WalletLogPrintf("Wallet file version = %d, last client version = %d\n", pwallet->GetVersion(), last_client); pwallet->WalletLogPrintf("Keys: %u plaintext, %u encrypted, %u total; Watch scripts: %u; HD PubKeys: %u; Metadata: %u; Unknown wallet records: %u\n", wss.nKeys, wss.nCKeys, wss.nKeys + wss.nCKeys, @@ -889,7 +887,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) if (wss.fIsEncrypted && (last_client == 40000 || last_client == 50000)) return DBErrors::NEED_REWRITE; - if (last_client < CLIENT_VERSION) // Update + if (!has_last_client || last_client != CLIENT_VERSION) // Update m_batch->Write(DBKeys::VERSION, CLIENT_VERSION); if (wss.fAnyUnordered) From df58edf273b9dc9ea0ce208b5395103afb178e62 Mon Sep 17 00:00:00 2001 From: MacroFake Date: Fri, 1 Jul 2022 14:56:19 +0200 Subject: [PATCH 2/5] Merge bitcoin/bitcoin#25471: rpc: Disallow gettxoutsetinfo queries for a specific block with `use_index=false` 27c8056885b05bd25f540dbf6ede89230d43c7b9 rpc: Disallow gettxoutsetinfo queries for a specific block with use_index=false (Martin Zumsande) Pull request description: In the `gettxoutsetinfo` RPC, if we set `use_index` to false but specify `hash_or_height`, we currently hit a nonfatal error, e.g. `gettxoutsetinfo "muhash" "1" "false"` results in: ``` Internal bug detected: "!pindex || pindex->GetBlockHash() == view->GetBestBlock()" rpc/blockchain.cpp:836 (GetUTXOStats) ``` The failing check was added in [#24410](https://github.com/bitcoin/bitcoin/pull/24410/commits/664a14ba7ccb40aa82d35a59831acd35db1897a6), but the previous behavior, returning the specified height together with data corresponding to the tip's height, was very confusing too in my opinion. Fix this by disallowing the interaction of `use_index=false` and `hash_or_height` and add a RPC help example with `-named` because users might ask themselves how to use the `use_index` flag witout hitting an error. An alternative way would be to allow the interaction if the specified `hash_or_height` happens to correspond to the tip (which should then also be applied to the `HASH_SERIALIZED` check before). If reviewers would prefer that, please say so. ACKs for top commit: fjahr: utACK 27c8056885b05bd25f540dbf6ede89230d43c7b9 shaavan: ACK 27c8056885b05bd25f540dbf6ede89230d43c7b9 Tree-SHA512: 1d81c34eaa48c86134a2cf7193246d5de6bfd819d413c3b3fae9cb9290e0297a336111eeaecede2f0f020b0f9a181d240de0da4493e1b387fe63b8189154442b --- src/rpc/blockchain.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 871ef727b15ab..b05b3f5358b16 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1041,7 +1041,7 @@ static RPCHelpMan gettxoutsetinfo() "Note this call may take some time if you are not using coinstatsindex.\n", { {"hash_type", RPCArg::Type::STR, RPCArg::Default{"hash_serialized_2"}, "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'."}, - {"hash_or_height", RPCArg::Type::NUM, RPCArg::Optional::OMITTED_NAMED_ARG, "The block hash or height of the target height (only available with coinstatsindex).", "", {"", "string or numeric"}}, + {"hash_or_height", RPCArg::Type::NUM, RPCArg::DefaultHint{"the current best block"}, "The block hash or height of the target height (only available with coinstatsindex).", "", {"", "string or numeric"}}, {"use_index", RPCArg::Type::BOOL, RPCArg::Default{true}, "Use coinstatsindex, if available."}, }, RPCResult{ @@ -1077,6 +1077,7 @@ static RPCHelpMan gettxoutsetinfo() HelpExampleCli("gettxoutsetinfo", R"("none")") + HelpExampleCli("gettxoutsetinfo", R"("none" 1000)") + HelpExampleCli("gettxoutsetinfo", R"("none" '"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09"')") + + HelpExampleCli("-named gettxoutsetinfo", R"(hash_type='muhash' use_index='false')") + HelpExampleRpc("gettxoutsetinfo", "") + HelpExampleRpc("gettxoutsetinfo", R"("none")") + HelpExampleRpc("gettxoutsetinfo", R"("none", 1000)") + @@ -1114,6 +1115,9 @@ static RPCHelpMan gettxoutsetinfo() throw JSONRPCError(RPC_INVALID_PARAMETER, "hash_serialized_2 hash type cannot be queried for a specific block"); } + if (!index_requested) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot set use_index to false when querying for a specific block"); + } pindex = ParseHashOrHeight(request.params[1], chainman); } From 76f7be72706b8faa17d05c12af44b503f13220fc Mon Sep 17 00:00:00 2001 From: MacroFake Date: Mon, 4 Jul 2022 08:29:02 +0200 Subject: [PATCH 3/5] Merge bitcoin/bitcoin#25535: test: pass `dustrelayfee=0` option for tests using dust (instead of `acceptnonstdtxn=1`) 1770be72d5eb47cae565d4ac86de5bc16f94fdd6 test: pass `dustrelayfee=0` option for tests using dust (instead of `acceptnonstdtxn=1`) (Sebastian Falbesoner) Pull request description: By specifying the `dustrelayfee=0` option instead of the more generic `acceptnonstdtxn=1`, we can be more specific about what part of the transaction is non-standard and can be sure that all other aspects follow the standard policy. Note that for the test `feature_dbcrash.py`, the UTXO creation at the start of the test has to be split up to several txs to not exceed the tx standard size limit of 100k vbytes https://github.com/bitcoin/bitcoin/blob/4129c1375430dbfe8dd414868c43fceb3d091fc3/src/policy/policy.h#L26-L27 ACKs for top commit: MarcoFalke: review ACK 1770be72d5eb47cae565d4ac86de5bc16f94fdd6 Tree-SHA512: 5cb852a92883a7443ab7dc15b48efa76b5d1424b6b0da1fa6b075fbe9a83522e3ff60382d36c08d4b07143ed898c115614582474e37837332caaee73b0db0e47 --- test/functional/feature_dbcrash.py | 10 +++++++--- test/functional/wallet_basic.py | 4 ++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/test/functional/feature_dbcrash.py b/test/functional/feature_dbcrash.py index 96e97c4da8307..030a8b112a0ee 100755 --- a/test/functional/feature_dbcrash.py +++ b/test/functional/feature_dbcrash.py @@ -62,8 +62,8 @@ def set_test_params(self): self.node2_args = ["-dbcrashratio=24", "-dbcache=16"] + self.base_args # Node3 is a normal node with default args, will mine full blocks - # and non-standard txs (e.g. txs with "dust" outputs) - self.node3_args = ["-acceptnonstdtxn"] + # and txs (with "dust" outputs + self.node3_args = ["-dustrelayfee=0"] self.extra_args = [self.node0_args, self.node1_args, self.node2_args, self.node3_args] def skip_test_if_missing_module(self): @@ -217,7 +217,11 @@ def run_test(self): # Start by creating a lot of utxos on node3 initial_height = self.nodes[3].getblockcount() - utxo_list = create_confirmed_utxos(self, self.nodes[3].getnetworkinfo()['relayfee'], self.nodes[3], 5000, sync_fun=self.no_op) + utxo_list = [] + for _ in range(5): + utxo_list.extend(create_confirmed_utxos(self, self.nodes[3].getnetworkinfo()['relayfee'], self.nodes[3], 1000, sync_fun=self.no_op)) + self.generate(self.nodes[3], 1, sync_fun=self.no_op) + assert_equal(len(self.nodes[3].getrawmempool()), 0) self.log.info(f"Prepped {len(utxo_list)} utxo entries") # Sync these blocks with the other nodes diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 603e0d50060eb..ff748259d5b41 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -27,11 +27,11 @@ def set_test_params(self): self.num_nodes = 4 if self.options.descriptors: self.extra_args = [[ - "-acceptnonstdtxn=1" + "-dustrelayfee=0" ] for i in range(self.num_nodes)] else: self.extra_args = [[ - "-acceptnonstdtxn=1", + "-dustrelayfee=0", '-usehd={:d}'.format(i%2==0) ] for i in range(self.num_nodes)] self.setup_clean_chain = True From bdd0237b27ecdac986d9ecbef1c70d26d650d481 Mon Sep 17 00:00:00 2001 From: MacroFake Date: Tue, 5 Jul 2022 18:55:52 +0200 Subject: [PATCH 4/5] Merge bitcoin/bitcoin#19393: test: Add more tests for orphan tx handling c0a5fceee9858afd24fe0bf655b7b30728e96e78 test: Add test for erase orphan tx conflicted by block (Hennadii Stepanov) fa45bb21193ae0c220cfc224d5e3ea0e7f3ec988 test: Add test for erase orphan tx included by block (Hennadii Stepanov) 5c049780c8b310428cf72fb304bf0c1071742785 test: Add test for erase orphan tx from peer (Hennadii Stepanov) Pull request description: This PR adds test coverage for the following cases: - erase orphan transactions when a peer is disconnected - erase an orphan transaction when it is included in a new tip block - erase an orphan transaction when it is conflicted with other transactions included in a new tip block Found useful while working on #19374. ACKs for top commit: aureleoules: tACK c0a5fceee9858afd24fe0bf655b7b30728e96e78 (`make check` and `test/functional/test_runner.py`). kouloumos: ACK c0a5fceee9858afd24fe0bf655b7b30728e96e78 with a nit per https://github.com/bitcoin/bitcoin/pull/19393#discussion_r899156623. pg156: Reviewed to https://github.com/bitcoin/bitcoin/pull/19393/commits/c0a5fceee9858afd24fe0bf655b7b30728e96e78. Concept ACK. Agree due to the lack of RPC calls to inspect orphan pool, using `assert_debug_log` to match strings in log is a reasonable way to test. Tree-SHA512: 98f8deeee2d1c588c7e28a82e513d4a18655084198369db33fe2710458251eeaffed030626940072d7576f57fcbf7d856d761990129e2ca9e372d2ccbd86d07d --- test/functional/p2p_invalid_tx.py | 68 ++++++++++++++++++++++++++++--- 1 file changed, 63 insertions(+), 5 deletions(-) diff --git a/test/functional/p2p_invalid_tx.py b/test/functional/p2p_invalid_tx.py index f7a6b644c7e05..361f45b0924ef 100755 --- a/test/functional/p2p_invalid_tx.py +++ b/test/functional/p2p_invalid_tx.py @@ -72,7 +72,6 @@ def run_test(self): block.solve() # Save the coinbase for later block2 = block - tip = block.sha256 node.p2ps[0].send_blocks_and_test([block1, block2], node, success=True) self.log.info("Mature the block.") @@ -116,24 +115,24 @@ def test_orphan_tx_handling(self, base_tx, resolve_via_block): SCRIPT_PUB_KEY_OP_TRUE = b'\x51\x75' * 15 + b'\x51' tx_withhold = CTransaction() tx_withhold.vin.append(CTxIn(outpoint=COutPoint(base_tx, 0))) - tx_withhold.vout.append(CTxOut(nValue=50 * COIN - 12000, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_withhold.vout = [(CTxOut(nValue=25 * COIN - 12000, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)] * 2 tx_withhold.calc_sha256() # Our first orphan tx with some outputs to create further orphan txs tx_orphan_1 = CTransaction() tx_orphan_1.vin.append(CTxIn(outpoint=COutPoint(tx_withhold.sha256, 0))) - tx_orphan_1.vout = [CTxOut(nValue=10 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)] * 3 + tx_orphan_1.vout = [CTxOut(nValue=8 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)] * 3 tx_orphan_1.calc_sha256() # A valid transaction with low fee tx_orphan_2_no_fee = CTransaction() tx_orphan_2_no_fee.vin.append(CTxIn(outpoint=COutPoint(tx_orphan_1.sha256, 0))) - tx_orphan_2_no_fee.vout.append(CTxOut(nValue=10 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_orphan_2_no_fee.vout.append(CTxOut(nValue=8 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) # A valid transaction with sufficient fee tx_orphan_2_valid = CTransaction() tx_orphan_2_valid.vin.append(CTxIn(outpoint=COutPoint(tx_orphan_1.sha256, 1))) - tx_orphan_2_valid.vout.append(CTxOut(nValue=10 * COIN - 12000, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_orphan_2_valid.vout.append(CTxOut(nValue=8 * COIN - 12000, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) tx_orphan_2_valid.calc_sha256() # An invalid transaction with negative fee @@ -197,6 +196,7 @@ def test_orphan_tx_handling(self, base_tx, resolve_via_block): with node.assert_debug_log(['orphanage overflow, removed 1 tx']): node.p2ps[0].send_txs_and_test(orphan_tx_pool, node, success=False) + self.log.info('Test orphan with rejected parents') rejected_parent = CTransaction() rejected_parent.vin.append(CTxIn(outpoint=COutPoint(tx_orphan_2_invalid.sha256, 0))) rejected_parent.vout.append(CTxOut(nValue=11 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) @@ -205,6 +205,64 @@ def test_orphan_tx_handling(self, base_tx, resolve_via_block): #with node.assert_debug_log(['not keeping orphan with rejected parents {}'.format(rejected_parent.hash)]): node.p2ps[0].send_txs_and_test([rejected_parent], node, success=False) + self.log.info('Test that a peer disconnection causes erase its transactions from the orphan pool') + with node.assert_debug_log(['Erased 100 orphan tx from peer=25']): + self.reconnect_p2p(num_connections=1) + + self.log.info('Test that a transaction in the orphan pool is included in a new tip block causes erase this transaction from the orphan pool') + tx_withhold_until_block_A = CTransaction() + tx_withhold_until_block_A.vin.append(CTxIn(outpoint=COutPoint(tx_withhold.sha256, 1))) + tx_withhold_until_block_A.vout = [CTxOut(nValue=12 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)] * 2 + tx_withhold_until_block_A.calc_sha256() + + tx_orphan_include_by_block_A = CTransaction() + tx_orphan_include_by_block_A.vin.append(CTxIn(outpoint=COutPoint(tx_withhold_until_block_A.sha256, 0))) + tx_orphan_include_by_block_A.vout.append(CTxOut(nValue=12 * COIN - 12000, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_orphan_include_by_block_A.calc_sha256() + + self.log.info('Send the orphan ... ') + node.p2ps[0].send_txs_and_test([tx_orphan_include_by_block_A], node, success=False) + + tip = int(node.getbestblockhash(), 16) + height = node.getblockcount() + 1 + block_A = create_block(tip, create_coinbase(height)) + block_A.vtx.extend([tx_withhold, tx_withhold_until_block_A, tx_orphan_include_by_block_A]) + block_A.hashMerkleRoot = block_A.calc_merkle_root() + block_A.solve() + + self.log.info('Send the block that includes the previous orphan ... ') + with node.assert_debug_log(["Erased 1 orphan tx included or conflicted by block"]): + node.p2ps[0].send_blocks_and_test([block_A], node, success=True) + + self.log.info('Test that a transaction in the orphan pool conflicts with a new tip block causes erase this transaction from the orphan pool') + tx_withhold_until_block_B = CTransaction() + tx_withhold_until_block_B.vin.append(CTxIn(outpoint=COutPoint(tx_withhold_until_block_A.sha256, 1))) + tx_withhold_until_block_B.vout.append(CTxOut(nValue=11 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_withhold_until_block_B.calc_sha256() + + tx_orphan_include_by_block_B = CTransaction() + tx_orphan_include_by_block_B.vin.append(CTxIn(outpoint=COutPoint(tx_withhold_until_block_B.sha256, 0))) + tx_orphan_include_by_block_B.vout.append(CTxOut(nValue=10 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_orphan_include_by_block_B.calc_sha256() + + tx_orphan_conflict_by_block_B = CTransaction() + tx_orphan_conflict_by_block_B.vin.append(CTxIn(outpoint=COutPoint(tx_withhold_until_block_B.sha256, 0))) + tx_orphan_conflict_by_block_B.vout.append(CTxOut(nValue=9 * COIN, scriptPubKey=SCRIPT_PUB_KEY_OP_TRUE)) + tx_orphan_conflict_by_block_B.calc_sha256() + self.log.info('Send the orphan ... ') + node.p2ps[0].send_txs_and_test([tx_orphan_conflict_by_block_B], node, success=False) + + tip = int(node.getbestblockhash(), 16) + height = node.getblockcount() + 1 + block_B = create_block(tip, create_coinbase(height)) + block_B.vtx.extend([tx_withhold_until_block_B, tx_orphan_include_by_block_B]) + block_B.hashMerkleRoot = block_B.calc_merkle_root() + block_B.solve() + + self.log.info('Send the block that includes a transaction which conflicts with the previous orphan ... ') + with node.assert_debug_log(["Erased 1 orphan tx included or conflicted by block"]): + node.p2ps[0].send_blocks_and_test([block_B], node, success=True) + if __name__ == '__main__': InvalidTxRequestTest().main() From d82b8b5bcc24af543c222c7238a5808fa6a5ebb2 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 8 Jul 2022 10:09:25 -0400 Subject: [PATCH 5/5] Merge bitcoin/bitcoin#25337: refactor: encapsulate wallet's address book access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit d69045e291e32e02d105d1b5ff1c8b86db0ae69e test: add coverage for 'listreceivedbyaddress' no change addrs return (furszy) 324f00a6420bbd64c67c264e50632e6fa36ae732 refactor: 'ListReceived' use optional for filtered address (furszy) b459fc122feace9e9a738c48aab21961cf15dddc refactor: RPC 'ListReceived', encapsulate m_address_book access (furszy) fa9f2ab8fd53075d2a3ec93ddac4908e73525c46 refactor: RPC 'listlabels', encapsulate 'CWallet::ListAddrBookLabels' functionality (furszy) 83e42c4b94e376a19d3eb0a2379769b8b8ac5fc8 refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel' (furszy) 2b48642499016cb357e4bcec32481cd50361194e refactor: use ForEachAddrBookEntry in interfaces::getAddresses (furszy) 032842ae4196aaed5ea3567ea01a61ed75ab2edd wallet: implement ForEachAddrBookEntry method (furszy) 09649bc95d5f2855a54a8cf02e65215a3b333c92 refactor: implement general 'ListAddrBookAddresses' for addressbook destinations lookup (furszy) 192eb1e61c3c43baec7f32c498ab0ce0656a58f7 refactor: getAddress don't access m_address_book, use FindAddressEntry function (furszy) Pull request description: ### Context The wallet's `m_address_book` field is being accessed directly from several places across the sources. ### Problem Code structure wise, we shouldn't be accessing it directly. It could end up being modified by mistake (from a place that has nothing to do with the wallet like an RPC command or the GUI) and cause a bigger issue: like an address book entry 'purpose' string change, which if done badly (from 'send' to 'receive'), could end up in a user sharing a "receive" address that he/she doesn't own. ### Solution Encapsulate `m_address_book` access inside the wallet. ------------------------------------------------------- Extra Note: This is the initial step towards decoupling the address book functionality from the wallet's sources. In other words, the creation of the `AddressBookManager` (which will be coming in a follow-up PR). ACKs for top commit: achow101: ACK d69045e291e32e02d105d1b5ff1c8b86db0ae69e theStack: ACK d69045e291e32e02d105d1b5ff1c8b86db0ae69e ✅ w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25337/commits/d69045e291e32e02d105d1b5ff1c8b86db0ae69e Tree-SHA512: dba17acd86f171b4e9af0223bbbcad380048570f6a2f6a92732a51f01abe8806debaf65c9e9e5569fa76a541903cbb50adcb5f56ef77858151c698ae6b218e2a --- src/interfaces/wallet.h | 2 +- src/wallet/interfaces.cpp | 20 ++++----- src/wallet/wallet.cpp | 56 ++++++++++++++++++++---- src/wallet/wallet.h | 25 ++++++++++- test/functional/wallet_listreceivedby.py | 5 +++ 5 files changed, 86 insertions(+), 22 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 9e9ba4ec27366..c031e1ac7efc5 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -127,7 +127,7 @@ class Wallet std::string* purpose) = 0; //! Get wallet address list. - virtual std::vector getAddresses() = 0; + virtual std::vector getAddresses() const = 0; //! Get receive requests. virtual std::vector getAddressReceiveRequests() = 0; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index d1996ecc1932a..2c2ad4f41b6f0 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -204,29 +204,27 @@ class WalletImpl : public Wallet std::string* purpose) override { LOCK(m_wallet->cs_wallet); - auto it = m_wallet->m_address_book.find(dest); - if (it == m_wallet->m_address_book.end() || it->second.IsChange()) { - return false; - } + const auto& entry = m_wallet->FindAddressBookEntry(dest, /*allow_change=*/false); + if (!entry) return false; // addr not found if (name) { - *name = it->second.GetLabel(); + *name = entry->GetLabel(); } if (is_mine) { *is_mine = m_wallet->IsMine(dest); } if (purpose) { - *purpose = it->second.purpose; + *purpose = entry->purpose; } return true; } - std::vector getAddresses() override + std::vector getAddresses() const override { LOCK(m_wallet->cs_wallet); std::vector result; - for (const auto& item : m_wallet->m_address_book) { - if (item.second.IsChange()) continue; - result.emplace_back(item.first, m_wallet->IsMine(item.first), item.second.GetLabel(), item.second.purpose); - } + m_wallet->ForEachAddrBookEntry([&](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change) EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet) { + if (is_change) return; + result.emplace_back(dest, m_wallet->IsMine(dest), label, purpose); + }); return result; } std::vector getAddressReceiveRequests() override { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 869af8d63b53a..95b1818e4610f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4388,21 +4388,59 @@ std::set< std::set > CWallet::GetAddressGroupings() const return ret; } -std::set CWallet::GetLabelAddresses(const std::string& label) const +void CWallet::ForEachAddrBookEntry(const ListAddrBookFunc& func) const { LOCK(cs_wallet); - std::set result; - for (const std::pair& item : m_address_book) - { - if (item.second.IsChange()) continue; - const CTxDestination& address = item.first; - const std::string& strName = item.second.GetLabel(); - if (strName == label) - result.insert(address); + for (const std::pair& item : m_address_book) { + const auto& entry = item.second; + func(item.first, entry.GetLabel(), entry.purpose, entry.IsChange()); } +} +std::vector CWallet::ListAddrBookAddresses(const std::optional& _filter) const +{ + AssertLockHeld(cs_wallet); + std::vector result; + AddrBookFilter filter = _filter ? *_filter : AddrBookFilter(); + ForEachAddrBookEntry([&result, &filter](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change) { + // Filter by change + if (filter.ignore_change && is_change) return; + // Filter by label + if (filter.m_op_label && *filter.m_op_label != label) return; + // All good + result.emplace_back(dest); + }); +} + +std::vector CWallet::ListAddrBookAddresses(const std::optional& _filter) const +{ + AssertLockHeld(cs_wallet); + std::vector result; + AddrBookFilter filter = _filter ? *_filter : AddrBookFilter(); + ForEachAddrBookEntry([&result, &filter](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change) { + // Filter by change + if (filter.ignore_change && is_change) return; + // Filter by label + if (filter.m_op_label && *filter.m_op_label != label) return; + // All good + result.emplace_back(dest); + }); return result; } +std::set CWallet::ListAddrBookLabels(const std::string& purpose) const +{ + AssertLockHeld(cs_wallet); + std::set label_set; + ForEachAddrBookEntry([&](const CTxDestination& _dest, const std::string& _label, + const std::string& _purpose, bool _is_change) { + if (_is_change) return; + if (purpose.empty() || _purpose == purpose) { + label_set.insert(_label); + } + }); + return label_set; +} + bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool fInternalIn) { m_spk_man = pwallet->GetScriptPubKeyMan(fInternalIn); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index e3164577e5727..8c27d4db66458 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1233,7 +1233,30 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati std::set> GetAddressGroupings() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); std::map GetAddressBalances() const; - std::set GetLabelAddresses(const std::string& label) const; + // Filter struct for 'ListAddrBookAddresses' + struct AddrBookFilter { + // Fetch addresses with the provided label + std::optional m_op_label{std::nullopt}; + // Don't include change addresses by default + bool ignore_change{true}; + }; + + /** + * Filter and retrieve destinations stored in the addressbook + */ + std::vector ListAddrBookAddresses(const std::optional& filter) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + /** + * Retrieve all the known labels in the address book + */ + std::set ListAddrBookLabels(const std::string& purpose) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + /** + * Walk-through the address book entries. + * Stops when the provided 'ListAddrBookFunc' returns false. + */ + using ListAddrBookFunc = std::function; + void ForEachAddrBookEntry(const ListAddrBookFunc& func) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Marks all outputs in each one of the destinations dirty, so their cache is diff --git a/test/functional/wallet_listreceivedby.py b/test/functional/wallet_listreceivedby.py index 040bc5e402518..4129be9f628d4 100755 --- a/test/functional/wallet_listreceivedby.py +++ b/test/functional/wallet_listreceivedby.py @@ -60,6 +60,11 @@ def run_test(self): {"address": empty_addr}, {"address": empty_addr, "label": "", "amount": 0, "confirmations": 0, "txids": []}) + # No returned addy should be a change addr + for node in self.nodes: + for addr_obj in node.listreceivedbyaddress(): + assert_equal(node.getaddressinfo(addr_obj["address"])["ischange"], False) + # Test Address filtering # Only on addr expected = {"address": addr, "label": "", "amount": Decimal("0.1"), "confirmations": 10, "txids": [txid, ]}