RPC/Wallet: Add "use_txids" to output of getaddressinfo

luke-jr commented at 0:19 am on August 13, 2021: member

(Non-GUI part of https://github.com/bitcoin-core/gui/pull/562, but without the bloom stuff)

ghost commented at 0:27 am on August 13, 2021: none

Concept ACK

DrahtBot added the label RPC/REST/ZMQ on Aug 13, 2021

DrahtBot added the label Wallet on Aug 13, 2021

luke-jr force-pushed on Aug 13, 2021

luke-jr renamed this:
~~RPC/Wallet: Add "txids" Array to getaddressinfo result for used addresses~~
RPC/Wallet: Add "use_txids" to output of getaddressinfo
on Aug 13, 2021

luke-jr force-pushed on Aug 13, 2021

bre876 approved

in src/wallet/wallet.cpp:733 in 9fe79260cb outdated

641+    if (std::holds_alternative<std::monostate>(callback)) return true;
642+
643+    found_any = false;
644+    for (const auto& entry : mapWallet) {
645+        const CWalletTx& wtx = entry.second;
646+        for (uint32_t i = 0; i < wtx.tx->vout.size(); ++i) {

kristapsk commented at 11:55 am on August 27, 2021:

nit - return type of vector::size() is vector::size_type which is size_t not uint32_t.

jonatack commented at 2:32 pm on July 24, 2022:

Yes, I think using uint32_t instead of size_t here is adding a type conversion to the lookup of const auto& output = wtx.tx->vout[i]; invoked on every loop iteration. Not sure if any compilers mind a size_t being passed into std::get<std::function<void(const CWalletTx&, uint32_t)>>(callback)(wtx, i); at the end of the loop but in the worst case could cast it to uint32 in that call if needed.

0        for (size_t i = 0; i < wtx.tx->vout.size(); ++i) {

luke-jr commented at 6:15 pm on November 10, 2022:

There is no type conversion from uint32_t to size_t to begin with?

kristapsk commented at 11:58 am on August 27, 2021: contributor

Concept ACK

DrahtBot commented at 5:33 pm on September 9, 2021: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	ghost, theStack, pk-b2, jonatack, fjahr, ajtowns, brunoerg
Approach ACK	ryanofsky
Approach NACK	S3RK, achow101
Stale ACK	rajarshimaitra, aureleoules, kristapsk

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Sep 26, 2021

rajarshimaitra commented at 3:15 pm on December 18, 2021: contributor

Strong concept + tACK https://github.com/bitcoin/bitcoin/pull/22693/commits/9fe79260cb7cdde75f4fce90f7615973a9233904

This is very useful for downstream wallets trying to sync using bitcoin core rpc (mostly in home servers). This makes transaction history lookup of addresses easy.

Thank you for working on this.

Hope this gets in soon.

luke-jr force-pushed on Dec 18, 2021

DrahtBot removed the label Needs rebase on Dec 18, 2021

luke-jr commented at 11:50 pm on December 18, 2021: member

Rebased

theStack commented at 1:44 am on December 29, 2021: contributor

Concept ACK

unknown approved

unknown commented at 5:12 am on December 29, 2021: none

tACK https://github.com/bitcoin/bitcoin/pull/22693/commits/bfb162d4aaf8c0b17e19219d70d5d8630360a2ab

This can be helpful information for privacy. Since it returns two transactions if RBF is used to replace earlier transaction, this adds another interesting information in the results. It is documented in the comments, would be better if present in RPC help as well.

Steps that I performed to test use_txids in getaddressinfo:

Create new address in Core:

0bitcoin-cli -rpcwallet="" getnewaddress
1tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln

Check result for getaddressinfo:

 0bitcoin-cli -rpcwallet="" getaddressinfo tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln
 1{
 2  "address": "tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln",
 3  "scriptPubKey": "0014857b28643beb848865ec3b4f187f2bc63aab4eb3",
 4  "ismine": true,
 5  "solvable": true,
 6  "desc": "wpkh([6ba7ec01/0'/0'/40']02f1b431e7cacb981fb24cb28365c6f45b8b7a4f891d3b4dfc1981dd7705b95249)#jypvcx7u",
 7  "iswatchonly": false,
 8  "isscript": false,
 9  "iswitness": true,
10  "witness_version": 0,
11  "witness_program": "857b28643beb848865ec3b4f187f2bc63aab4eb3",
12  "pubkey": "02f1b431e7cacb981fb24cb28365c6f45b8b7a4f891d3b4dfc1981dd7705b95249",
13  "ischange": false,
14  "timestamp": 1639765317,
15  "hdkeypath": "m/0'/0'/40'",
16  "hdseedid": "37bb332a78fc64d32f269a40710ed0d34d0a0ba5",
17  "hdmasterfingerprint": "6ba7ec01",
18  "labels": [
19    ""
20  ],
21  "use_txids": [
22  ]
23}

Send some bitcoin to this address and check result for getaddressinfo:

 0bitcoin-cli -rpcwallet="" getaddressinfo tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln
 1{
 2  "address": "tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln",
 3  "scriptPubKey": "0014857b28643beb848865ec3b4f187f2bc63aab4eb3",
 4  "ismine": true,
 5  "solvable": true,
 6  "desc": "wpkh([6ba7ec01/0'/0'/40']02f1b431e7cacb981fb24cb28365c6f45b8b7a4f891d3b4dfc1981dd7705b95249)#jypvcx7u",
 7  "iswatchonly": false,
 8  "isscript": false,
 9  "iswitness": true,
10  "witness_version": 0,
11  "witness_program": "857b28643beb848865ec3b4f187f2bc63aab4eb3",
12  "pubkey": "02f1b431e7cacb981fb24cb28365c6f45b8b7a4f891d3b4dfc1981dd7705b95249",
13  "ischange": false,
14  "timestamp": 1639765317,
15  "hdkeypath": "m/0'/0'/40'",
16  "hdseedid": "37bb332a78fc64d32f269a40710ed0d34d0a0ba5",
17  "hdmasterfingerprint": "6ba7ec01",
18  "labels": [
19    ""
20  ],
21  "use_txids": [
22    "feaf134bc878824c5cfb7fc1d7b2a6c802f2482dd7becf8f5cc46a473e3bd4e0"
23  ]
24}

Replace the transaction using RBF and confirm if there are two transaction ids in result for getaddressinfo:

 0bitcoin-cli -rpcwallet="" getaddressinfo tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln
 1{
 2  "address": "tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln",
 3  "scriptPubKey": "0014857b28643beb848865ec3b4f187f2bc63aab4eb3",
 4  "ismine": true,
 5  "solvable": true,
 6  "desc": "wpkh([6ba7ec01/0'/0'/40']02f1b431e7cacb981fb24cb28365c6f45b8b7a4f891d3b4dfc1981dd7705b95249)#jypvcx7u",
 7  "iswatchonly": false,
 8  "isscript": false,
 9  "iswitness": true,
10  "witness_version": 0,
11  "witness_program": "857b28643beb848865ec3b4f187f2bc63aab4eb3",
12  "pubkey": "02f1b431e7cacb981fb24cb28365c6f45b8b7a4f891d3b4dfc1981dd7705b95249",
13  "ischange": false,
14  "timestamp": 1639765317,
15  "hdkeypath": "m/0'/0'/40'",
16  "hdseedid": "37bb332a78fc64d32f269a40710ed0d34d0a0ba5",
17  "hdmasterfingerprint": "6ba7ec01",
18  "labels": [
19    ""
20  ],
21  "use_txids": [
22    "b8d96d7683b1ed482c53c2b0b191e91d5db671ee21009fac4d72546bb13fde47",
23    "feaf134bc878824c5cfb7fc1d7b2a6c802f2482dd7becf8f5cc46a473e3bd4e0"
24  ]
25}

Create an address in Electrum, check result for getaddressinfo in Core:

 0bitcoin-cli -rpcwallet="" getaddressinfo tb1qsp9g4v04yakgulv7ylz9t8g7m6cj46qff6k023
 1{
 2  "address": "tb1qsp9g4v04yakgulv7ylz9t8g7m6cj46qff6k023",
 3  "scriptPubKey": "0014804a8ab1f5276c8e7d9e27c4559d1edeb12ae809",
 4  "ismine": false,
 5  "solvable": false,
 6  "iswatchonly": false,
 7  "isscript": false,
 8  "iswitness": true,
 9  "witness_version": 0,
10  "witness_program": "804a8ab1f5276c8e7d9e27c4559d1edeb12ae809",
11  "ischange": false,
12  "labels": [
13  ],
14  "use_txids": [
15  ]
16}

Send some bitcoin to this address from Core:

0bitcoin-cli -rpcwallet="" sendtoaddress "tb1qsp9g4v04yakgulv7ylz9t8g7m6cj46qff6k023" 0.001
1f9d09be836ca90951a5ce734598a5eb1fc56d2ed36a7d0c0c47a11dbd4a1e624

Check result for getaddressinfo:

 0bitcoin-cli -rpcwallet="" getaddressinfo tb1qsp9g4v04yakgulv7ylz9t8g7m6cj46qff6k023
 1{
 2  "address": "tb1qsp9g4v04yakgulv7ylz9t8g7m6cj46qff6k023",
 3  "scriptPubKey": "0014804a8ab1f5276c8e7d9e27c4559d1edeb12ae809",
 4  "ismine": false,
 5  "solvable": false,
 6  "iswatchonly": false,
 7  "isscript": false,
 8  "iswitness": true,
 9  "witness_version": 0,
10  "witness_program": "804a8ab1f5276c8e7d9e27c4559d1edeb12ae809",
11  "ischange": false,
12  "labels": [
13  ],
14  "use_txids": [
15    "f9d09be836ca90951a5ce734598a5eb1fc56d2ed36a7d0c0c47a11dbd4a1e624"
16  ]
17}

DrahtBot added the label Needs rebase on Feb 1, 2022

luke-jr force-pushed on Feb 9, 2022

luke-jr commented at 7:52 am on February 9, 2022: member

Rebased again

DrahtBot removed the label Needs rebase on Feb 9, 2022

unknown approved

unknown commented at 8:30 am on February 9, 2022: none

reACK https://github.com/bitcoin/bitcoin/pull/22693/commits/8719b084754b8346e1cb551843afd3afcd439b71

pk-b2 commented at 5:38 am on April 30, 2022: none

Concept ACK

tACK 8719b084754b8346e1cb551843afd3afcd439b71

Generated a new address and verified there is no existing tx associated with it

 0bitcoin-cli getaddressinfo bcrt1qmdr8t0qu4xps0zu47n8206jsxzcpurh53f7de9
 1{
 2  "address": "bcrt1qmdr8t0qu4xps0zu47n8206jsxzcpurh53f7de9",
 3  "scriptPubKey": "0014db4675bc1ca983078b95f4cea7ea5030b01e0ef4",
 4  "ismine": false,
 5  "solvable": false,
 6  "iswatchonly": false,
 7  "isscript": false,
 8  "iswitness": true,
 9  "witness_version": 0,
10  "witness_program": "db4675bc1ca983078b95f4cea7ea5030b01e0ef4",
11  "ischange": false,
12  "labels": [
13  ],
14  "use_txids": [
15  ]
16}

Generated a new address and send to address

 0bitcoin-cli getaddressinfo bcrt1q23zuu74fshheztes967d26vgz6kutkv340st8a
 1{
 2  "address": "bcrt1q23zuu74fshheztes967d26vgz6kutkv340st8a",
 3  "scriptPubKey": "00145445ce7aa985ef912f302ebcd5698816adc5d991",
 4  "ismine": false,
 5  "solvable": false,
 6  "iswatchonly": false,
 7  "isscript": false,
 8  "iswitness": true,
 9  "witness_version": 0,
10  "witness_program": "5445ce7aa985ef912f302ebcd5698816adc5d991",
11  "ischange": false,
12  "labels": [
13  ],
14  "use_txids": [
15    "bf446b264f267ab6c0fd9fb8663945b80778cd3967de91acac80e60c7d040328"
16  ]
17}

Reused address to verify that both txids show up

 0bitcoin-cli getaddressinfo bcrt1q23zuu74fshheztes967d26vgz6kutkv340st8a
 1{
 2  "address": "bcrt1q23zuu74fshheztes967d26vgz6kutkv340st8a",
 3  "scriptPubKey": "00145445ce7aa985ef912f302ebcd5698816adc5d991",
 4  "ismine": false,
 5  "solvable": false,
 6  "iswatchonly": false,
 7  "isscript": false,
 8  "iswitness": true,
 9  "witness_version": 0,
10  "witness_program": "5445ce7aa985ef912f302ebcd5698816adc5d991",
11  "ischange": false,
12  "labels": [
13  ],
14  "use_txids": [
15    "cb41ac571ddde6622b0b00d1a39b95bfcf04b8d1f9513371d2918497691d611b",
16    "bf446b264f267ab6c0fd9fb8663945b80778cd3967de91acac80e60c7d040328"
17  ]
18}

in src/wallet/rpc/addresses.cpp:545 in 8719b08475 outdated

540@@ -541,6 +541,10 @@ RPCHelpMan getaddressinfo()
541                         {
542                             {RPCResult::Type::STR, "label name", "Label name (defaults to \"\")."},
543                         }},
544+                        {RPCResult::Type::ARR, "use_txids", "",

pk-b2 commented at 5:39 am on April 30, 2022:

Are there any guidelines when a response attribute should be optional vs always present?

pk-b2 commented at 5:53 am on April 30, 2022:

Looking at other RPCs I tried to find a similar naming convention for txids.

I found listreceivedbyaddress which uses an array called txids and getblock that uses tx to return transaction ids in their responses.

Not really consistent, but I was wondering if it would make sense to follow the listreceivedbyaddress response here and call the attribute txids as well instead of use_txids?

luke-jr commented at 6:18 pm on November 10, 2022:

I think use_txids is clearer.

in src/wallet/wallet.cpp:685 in 8719b08475 outdated

664+        if (!ExtractDestination(output.scriptPubKey, dest)) continue;
665+        m_address_book[dest].m_used = true;
666+    }
667+}
668+
669+void CWallet::InitialiseAddressBookUsed()

pk-b2 commented at 5:40 am on April 30, 2022:

nit: Seems Initialize* with a Z is used across the code base

in src/wallet/wallet.h:807 in 8719b08475 outdated

672@@ -666,7 +673,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
673     CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const;
674     void chainStateFlushed(const CBlockLocator& loc) override;
675 
676-    DBErrors LoadWallet();
677+    enum class do_init_used_flag { Init, Skip };

pk-b2 commented at 5:45 am on April 30, 2022:

Does it make sense to keep the naming of this enum more generic to cover the initialization of the entire address book than just the used attribute?

e.g. enum class AddressBook { Initialize, Skip_Initialization }

jonatack commented at 5:13 pm on July 27, 2022:

3ee478b9cba305dc87de1ad093ca14a89e58b516 it looks like this codebase uses PascalCase for enum classes, and doing so would also make the code more readable, for example easier to distinguish the PascalCase type from the snake_cased param name in function definitions

luke-jr commented at 6:17 pm on November 10, 2022:

It’s a weird hack made necessary only by an oddity in the wallet tool. I’m not sure it makes sense to guess about what other hacks might or might not be needed in the future.

in src/wallet/wallet.h:808 in 8719b08475 outdated

672@@ -666,7 +673,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
673     CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const;
674     void chainStateFlushed(const CBlockLocator& loc) override;
675 
676-    DBErrors LoadWallet();
677+    enum class do_init_used_flag { Init, Skip };
678+    DBErrors LoadWallet(const do_init_used_flag do_init_used_flag_val = do_init_used_flag::Init);

pk-b2 commented at 6:09 am on April 30, 2022:

I haven’t done any measurements with a large historical wallet, but is it a performance concern when we immediately initialize the address book on wallet creation?

Is it worth considering to lazy-initialize the address book on first use of FindScriptPubKeyUsed?

luke-jr commented at 6:18 pm on November 10, 2022:

I doubt it

in src/wallet/wallet.h:563 in 8719b08475 outdated

467@@ -463,6 +468,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
468     bool UnlockAllCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
469     void ListLockedCoins(std::vector<COutPoint>& vOutpts) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
470 
471+    bool FindScriptPubKeyUsed(const std::set<CScript>& keys, const std::variant<std::monostate, std::function<void(const CWalletTx&)>, std::function<void(const CWalletTx&, uint32_t)>>& callback = std::monostate()) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

pk-b2 commented at 6:26 am on April 30, 2022:

Would something like FindTransactions fit better here?

furszy commented at 4:26 pm on June 1, 2022: member

About 698658e3:

Not sure if you have seen it but we are already keeping track of the used destinations in a slightly different way, inside the address book entry destdata field, under key called “used”. (check CWallet::IsAddressUsed and CWallet::SetAddressUsed).

I think that would make more sense to expand the flow that we currently have to support this new functionality.

It has the nice property of, as the “used” field is already stored in db, not needing to introduce the walk through every tx and tx output in the wallet during startup to mark the destinations as used that is inside 698658e3.

Note:

Have implemented the mentioned approach here: https://github.com/furszy/bitcoin/commit/8dd0f2821c3f8e2693503b56968242ed951a0a2e and https://github.com/furszy/bitcoin/commit/8b963a0dd9477ba9a002a639f23eac544f801794

Made them as part of the review process, can add unit and functional test coverage if you like them.

luke-jr commented at 4:33 pm on June 1, 2022: member

Not sure if you have seen it but we are already keeping track of the used destinations in a slightly different way, inside the address book entry destdata field, under key called “used”. (check CWallet::IsAddressUsed and CWallet::SetAddressUsed).

That’s not the same thing, despite its confusing name.

unknown approved

unknown commented at 5:27 pm on June 1, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/22693/commits/8719b084754b8346e1cb551843afd3afcd439b71

Address reuse is bad for privacy. Every address should be treated as invoice valid only for a payment. Hopefully this pull request will decrease address reuse: https://blog.bitmex.com/bitcoin-address-re-use-statistics/

furszy commented at 7:33 pm on June 1, 2022: member

Not sure if you have seen it but we are already keeping track of the used destinations in a slightly different way, inside the address book entry destdata field, under key called “used”. (check CWallet::IsAddressUsed and CWallet::SetAddressUsed).

That’s not the same thing, despite its confusing name.

Hmm, the storage part of the flow seems to be what you have in 698658e3 with a single difference:

-> Current Flow: When the wallet receives a transaction, AddToWallet loops over the tx inputs and calls SetSpentKeyState, which extracts the destinations and mark the ones owned by the wallet as “used” inside the address book entry destdata map.

Then IsAddressUsed merely checks if the “used” key is on the address book entry destdata map. Which denotes that the wallet received funds to that specific destination (so, it’s “used”).

-> The difference with 698658e3: It does not mark the tx outputs destinations as “used” (which is what my commit implements).

Am I missing something?

Aside from that, outside of the scope of this PR, I do agree that the “how this is currently used” part of the flow is misleading. Mostly because IsSpentKey(which receives an outpoint to check if there is a known output that uses the same destination inside the wallet) is called from AvailableCoins without much context. But.. for that, I have a PR that cleans up the IsSpentKey function a bit #25005.

luke-jr commented at 4:38 am on June 2, 2022: member

Am I missing something?

The destdata key deals with and is marked by inputs. But when an address is used, it’s by an output, not an input.

furszy commented at 7:46 pm on June 3, 2022: member

Am I missing something?

The destdata key deals with and is marked by inputs. But when an address is used, it’s by an output, not an input.

Actually, that is what I tried to express in the “The difference with 698658e3” paragraph of my previous comment:

It does not mark the tx outputs destinations as “used” (which is what my commit implements).

So I think that, up until there, we are sync then.

The “marked by inputs” can be described as: “get the prev-output scriptpubkey, extract the destination and mark it as used”. Which is what currently have. Now for this new feature, we need to add the “used” mark for the outputs of the transactions created by the wallet as well.

So, my point is to generalize the “used” concept that we already have: Surely we can agree that the “used” flag (implemented in one way or another) is merely a boolean inside each address book entry that tells us, in an accessible manner, if the destination already appeared on a transaction or not. Don’t care if the destination got marked as “used” because it appeared on an input (prev-tx-output) or an output of a tx that we created. In other words, shouldn’t care whether the address book entry purpose is “receive” or “send”, at the end we are just looking for a flag that can be used to signal something like “hey, this destination was already used”.

On top of this single place storage solution, we can know whether the destination is from the wallet or not based on the “purpose” field or the IsMine result to distinguish where to use the information etc.

DrahtBot added the label Needs rebase on Jun 22, 2022

luke-jr force-pushed on Jun 30, 2022

DrahtBot removed the label Needs rebase on Jul 1, 2022

unknown approved

unknown commented at 2:05 pm on July 1, 2022: none

reACK https://github.com/bitcoin/bitcoin/pull/22693/commits/3ee478b9cba305dc87de1ad093ca14a89e58b516

This pull request looks easier to review and test still pending since couple of years. Maybe tweeting about it, explaining the positives and requesting to review might help @luke-jr

aureleoules commented at 1:04 pm on July 19, 2022: member

ACK 3ee478b9cba305dc87de1ad093ca14a89e58b516.

I understand that FindScriptPubKeyUsed takes a callback function that can either have one or two arguments (CWalletTX and the vout index) but I would suggest adding some documentation about this as the C++ syntax is a bit obscure in my opinion.

I tested the code by generating an address and sending coins to it, the txid correctly appeared in the getaddressinfo’s use_txids field.

I also tested loading a wallet with ~14k transactions, it took 6ms on my machine to load the address book.

in src/wallet/wallet.cpp:689 in 3ee478b9cb outdated

684+
685+void CWallet::InitialiseAddressBookUsed()
686+{
687+    for (const auto& entry : mapWallet) {
688+        const CWalletTx& wtx = entry.second;
689+        UpdateAddressBookUsed(wtx);

jonatack commented at 11:30 am on July 23, 2022:

2d3b9e24add7a9eb9824b60fc5590d3a1e4b8c6d can avoid a temporary in this loop and document with named arg instead

0 void CWallet::InitialiseAddressBookUsed()
1 {
2     for (const auto& entry : mapWallet) {
3-        const CWalletTx& wtx = entry.second;
4-        UpdateAddressBookUsed(wtx);
5+        UpdateAddressBookUsed(/*wtx=*/entry.second);
6     }
7 }

luke-jr commented at 5:56 pm on November 10, 2022:

I don’t see any benefit to this. It just makes the code less readable?

in src/wallet/wallet.cpp:2387 in 3ee478b9cb outdated

2234@@ -2180,7 +2235,12 @@ DBErrors CWallet::LoadWallet()
2235         assert(m_internal_spk_managers.empty());
2236     }
2237 
2238-    return nLoadWalletRet;
2239+    if (nLoadWalletRet != DBErrors::LOAD_OK)
2240+        return nLoadWalletRet;

jonatack commented at 11:35 am on July 23, 2022:

2d3b9e24add7a9eb9824b60fc5590d3a1e4b8c6d If an if only has a single-statement then-clause, it can appear on the same line as the if, without braces. In every other case, per our developer notes braces are required (due to CVE-2014-1266, the Apple “goto fail” vulnerability).

0-    if (nLoadWalletRet != DBErrors::LOAD_OK)
1+    if (nLoadWalletRet != DBErrors::LOAD_OK) {
2         return nLoadWalletRet;
3+    }

luke-jr commented at 6:25 pm on November 10, 2022:

Fixed

in src/wallet/wallet.h:347 in 3ee478b9cb outdated

265@@ -264,6 +266,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
266     void AddToSpends(const COutPoint& outpoint, const uint256& wtxid, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
267     void AddToSpends(const CWalletTx& wtx, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
268 
269+    void UpdateAddressBookUsed(const CWalletTx&) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
270+    void InitialiseAddressBookUsed() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

jonatack commented at 11:36 am on July 23, 2022:

https://github.com/bitcoin/bitcoin/commit/2d3b9e24add7a9eb9824b60fc5590d3a1e4b8c6d suggest ordering these two methods here and in the cpp file with initialize first, then update.

luke-jr commented at 6:25 pm on November 10, 2022:

Done

jonatack commented at 11:37 am on July 23, 2022: contributor

Reviewed and debug built the first commit https://github.com/bitcoin/bitcoin/commit/2d3b9e24add7a9eb9824b60fc5590d3a1e4b8c6d

in src/wallet/wallet.cpp:807 in 3ee478b9cb outdated

689+        UpdateAddressBookUsed(wtx);
690+    }
691+}
692+
693+bool CWallet::FindScriptPubKeyUsed(const std::set<CScript>& keys, const std::variant<std::monostate, std::function<void(const CWalletTx&)>, std::function<void(const CWalletTx&, uint32_t)>>& callback) const
694+{

jonatack commented at 2:20 pm on July 24, 2022:

16ad0586 Missing run-time lock assertion here.

0 bool CWallet::FindScriptPubKeyUsed(const std::set<CScript>& keys, const std::variant<std::monostate, std::function<void(const CWalletTx&)>, std::function<void(const CWalletTx&, uint32_t)>>& callback) const
1 {
2+    AssertLockHeld(cs_wallet);
3     bool found_any = false;

luke-jr commented at 6:25 pm on November 10, 2022:

Added, though I’m not sure why we need runtime checks when we have compiletime…

in src/wallet/wallet.cpp:820 in 3ee478b9cb outdated

701+        if (address_book_it->second.m_used) {
702+            found_any = true;
703+            break;
704+        }
705+    }
706+    if (!found_any) return false;

jonatack commented at 2:26 pm on July 24, 2022:

16ad0586 Maybe replace the found_any localvar and the loops with lambda closures / std::any_of.

luke-jr commented at 6:05 pm on November 10, 2022:

Not sure what you have in mind here, but this seems fine as-is.

in src/wallet/wallet.cpp:831 in 3ee478b9cb outdated

712+        for (uint32_t i = 0; i < wtx.tx->vout.size(); ++i) {
713+            const auto& output = wtx.tx->vout[i];
714+            if (keys.count(output.scriptPubKey)) {
715+                found_any = true;
716+                const auto callback_type = callback.index();
717+                if (callback_type == 1) {

jonatack commented at 2:56 pm on July 24, 2022:

16ad0586fbe6d15673a03f8b91db54fe0419bc08 Two unnecessary temporaries/moves can be avoided in this loop.

0-            const auto& output = wtx.tx->vout[i];
1-            if (keys.count(output.scriptPubKey)) {
2+            if (keys.count(wtx.tx->vout[i].scriptPubKey)) {
3                 found_any = true;
4-                const auto callback_type = callback.index();
5-                if (callback_type == 1) {
6+                if (callback.index() == 1) {

achow101 commented at 9:48 pm on January 12, 2023:

This was marked resolved but not changed.

in src/wallet/rpc/addresses.cpp:646 in 3ee478b9cb outdated

635@@ -632,6 +636,15 @@ RPCHelpMan getaddressinfo()
636     }
637     ret.pushKV("labels", std::move(labels));
638 
639+    // NOTE: Intentionally not special-casing a single txid: while addresses
640+    // should never be reused, it's not unexpected to have RBF result in
641+    // multiple txids for a single use.
642+    UniValue use_txids(UniValue::VARR);
643+    pwallet->FindScriptPubKeyUsed(std::set<CScript>{scriptPubKey}, [&use_txids](const CWalletTx&wtx) {

jonatack commented at 3:18 pm on July 24, 2022:

94b20a9

0    pwallet->FindScriptPubKeyUsed(std::set<CScript>{scriptPubKey}, [&use_txids](const CWalletTx& wtx) {

in src/wallet/rpc/addresses.cpp:547 in 3ee478b9cb outdated

540@@ -541,6 +541,10 @@ RPCHelpMan getaddressinfo()
541                         {
542                             {RPCResult::Type::STR, "label name", "Label name (defaults to \"\")."},
543                         }},
544+                        {RPCResult::Type::ARR, "use_txids", "",
545+                        {
546+                            {RPCResult::Type::STR_HEX, "txid", "The ids of transactions involving this wallet which received with the address"},

jonatack commented at 3:37 pm on July 24, 2022:

94b20a9f9e81e5b0ef45a0259b81be83b602c325

s/which/that/ (“which” would require a comma-space preceding it)
maybe add “have already” and s/the/this/

0                            {RPCResult::Type::STR_HEX, "txid", "The ids of transactions involving this wallet that have already received with this address"},

luke-jr commented at 6:08 pm on November 10, 2022:

I think the current string is better. Not sure why it would require a comma.

in test/functional/wallet_basic.py:647 in 3ee478b9cb outdated

641@@ -642,6 +642,12 @@ def run_test(self):
642         assert not address_info["iswatchonly"]
643         assert not address_info["isscript"]
644         assert not address_info["ischange"]
645+        assert_equal(address_info['use_txids'], [])
646+
647+        # Test getaddressinfo 'txids' field

jonatack commented at 3:55 pm on July 24, 2022:

94b20a9f9e81e5b0ef45a0259b81be83b602c325

0        # Test getaddressinfo 'use_txids' field

Could test with multiple txids

 0-        # Test getaddressinfo 'txids' field
 1-        txid = self.nodes[0].sendtoaddress("mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ", 1)
 2-        address_info = self.nodes[0].getaddressinfo("mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ")
 3-        assert_equal(address_info['use_txids'], [txid])
 4+        # Test getaddressinfo 'use_txids' field
 5+        addr = "mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ"
 6+        txid_1 = self.nodes[0].sendtoaddress(addr, 1)
 7+        address_info = self.nodes[0].getaddressinfo(addr)
 8+        assert_equal(address_info['use_txids'], [txid_1])
 9+        txid_2 = self.nodes[0].sendtoaddress(addr, 1)
10+        address_info = self.nodes[0].getaddressinfo(addr)
11+        assert_equal(address_info['use_txids'], [txid_1, txid_2])

luke-jr commented at 6:26 pm on November 10, 2022:

Done

jonatack commented at 3:56 pm on July 24, 2022: contributor

Reviewed/tested 16ad0586fbe6d15673a03f8b91db54fe0419bc08 and https://github.com/bitcoin/bitcoin/commit/94b20a9f9e81e5b0ef45a0259b81be83b602c325

Would naming the RPC field used_txids be clearer than use_txids? (if I’m not confused)

in src/wallet/wallettool.cpp:170 in 3ee478b9cb outdated

167@@ -168,7 +168,8 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command)
168         DatabaseOptions options;
169         ReadDatabaseArgs(args, options);
170         options.require_existing = true;
171-        const std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, args, options);
172+        // NOTE: We need to skip initialisation of the m_used flag, or else the address book count might be wrong

jonatack commented at 5:23 pm on July 27, 2022:

3ee478b9cba305dc87de1ad093ca14a89e58b516 a test demonstrating the bug would be nice

luke-jr commented at 6:27 pm on November 10, 2022:

This is already caught by an existing test (test/functional/tool_wallet.py IIRC)

in src/wallet/wallettool.cpp:50 in 3ee478b9cb outdated

46@@ -47,7 +47,7 @@ static void WalletCreate(CWallet* wallet_instance, uint64_t wallet_creation_flag
47     wallet_instance->TopUpKeyPool();
48 }
49 
50-static const std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::path& path, const ArgsManager& args, DatabaseOptions options)
51+static const std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::path& path, const ArgsManager& args, DatabaseOptions options, CWallet::do_init_used_flag do_init_used_flag_val = CWallet::do_init_used_flag::Init)

jonatack commented at 5:26 pm on July 27, 2022:

3ee478b9cba305dc87de1ad093ca14a89e58b516 it may be more maintainable and explicit to pass the value rather than use a default (there aren’t too many callers)

luke-jr commented at 6:11 pm on November 10, 2022:

Generally, this should never be used. It’s weird that the wallet tool wants to analyze the on-disk addressbook specifically.

jonatack commented at 5:28 pm on July 27, 2022: contributor

Light review of 3ee478b9cba305dc87de1ad093ca14a89e58b516

General overall approach ACK modulo review feedback

DrahtBot added the label Needs rebase on Aug 5, 2022

fjahr commented at 6:04 pm on August 7, 2022: contributor

Concept ACK, will wait for rebase and addressing of previous comments before going into a deeper review

jonatack commented at 7:09 pm on September 22, 2022: contributor

Needs rebase/update.

in src/wallet/wallet.h:245 in 2d3b9e24ad outdated

203@@ -204,6 +204,7 @@ class CAddressBookData
204     std::string m_label;
205 public:
206     std::string purpose;
207+    bool m_used{false};

ryanofsky commented at 7:22 pm on September 27, 2022:

In commit “Wallet: Keep track of what addresses are used in wallet transactions (memory only)” (2d3b9e24add7a9eb9824b60fc5590d3a1e4b8c6d)

The other class members aren’t documented very well, but it would be good to at least document this new member. Maybe //! Whether address is the destination of any wallet transation. Unlike other fields in address data struct, the used value is determined at runtime and not serialized as part of address data.?

It might also be good to rename this field since there is already an existing "used" field associated with wallet addresses, stored in the StringMap destdata "used" key. (At one point I actually had a PR that converted the destdata “used” key into a bool m_used field which I could reopen if it helps make the data representation clearer.)

luke-jr commented at 6:13 pm on November 10, 2022:

It’s indeed confusing that there is an existing “used” that doesn’t actually indicate usedness.

But this field is actually the correct value, so I’m not sure what else it might be named?

luke-jr commented at 6:27 pm on November 10, 2022:

Added the doc

S3RK commented at 10:02 pm on November 10, 2022:

Can we just reuse existing StringMap destdata “used” key to store the correct “used” state?

luke-jr commented at 10:44 pm on November 10, 2022:

I don’t know if there’s usage of the incorrect “used” flag.

Also, I don’t think it makes sense to store this one.

S3RK commented at 7:31 am on November 14, 2022:

Based on my research the existing flag is used for “avoid reuse” feature in the wallet. Sounds like a good idea to reuse it and a) improve other parts of the wallet b) make features behave consistently

in src/wallet/wallet.cpp:806 in 16ad0586fb outdated

689@@ -689,6 +690,42 @@ void CWallet::InitialiseAddressBookUsed()
690     }
691 }
692 
693+bool CWallet::FindScriptPubKeyUsed(const std::set<CScript>& keys, const std::variant<std::monostate, std::function<void(const CWalletTx&)>, std::function<void(const CWalletTx&, uint32_t)>>& callback) const

ryanofsky commented at 7:47 pm on September 27, 2022:

In commit “Wallet: Add fairly-efficient [negative] check that an address is not known to be used” (16ad0586fbe6d15673a03f8b91db54fe0419bc08)

I understand this code may need to get more complicated in a future PR, but in this PR I think it would be better to replace

0std::variant<std::monostate, std::function<void(const CWalletTx&)>, std::function<void(const CWalletTx&, uint32_t)>>

with a simpler:

0std::function<void(const CWalletTx&)>

It would make this PR easier to review and understand, and make test coverage better by not adding untested code. And std::function is nullable, so the monostate case could just correspond to the null case.

luke-jr commented at 6:14 pm on November 10, 2022:

Don’t think it’s worth it to change this now just to revert it later.

ryanofsky commented at 6:51 pm on February 23, 2024:

re: #22693 (review)

Don’t think it’s worth it to change this now just to revert it later.

Sure, I can see how knowing the output index would be useful. But in that case replacing the std::variant with just:

0std::function<void(const CWalletTx&, uint32_t)>

would simplify this function a lot and not complicate the rpc much at all (would just add an unused parameter).

in src/wallet/wallet.cpp:802 in 2d3b9e24ad outdated

671@@ -672,6 +672,23 @@ void CWallet::AddToSpends(const CWalletTx& wtx, WalletBatch* batch)
672         AddToSpends(txin.prevout, wtx.GetHash(), batch);
673 }
674 
675+void CWallet::UpdateAddressBookUsed(const CWalletTx& wtx)
676+{
677+    for (const auto& output : wtx.tx->vout) {
678+        CTxDestination dest;
679+        if (!ExtractDestination(output.scriptPubKey, dest)) continue;
680+        m_address_book[dest].m_used = true;

ryanofsky commented at 5:54 pm on September 28, 2022:

In commit “Bugfix: Wallet: Don’t initialise “used” flag for wallet tool “info” command” (3ee478b9cba305dc87de1ad093ca14a89e58b516)

IIUC this line causes a problem which is then “fixed” by the last commit in this PR “Bugfix: Wallet: Don’t initialise “used” flag for wallet tool “info” command” (3ee478b9cba305dc87de1ad093ca14a89e58b516). The problem is that it inserts new addresses into the address book which did not previously exist.

But I wonder if it would work to replace this with:

0auto found = m_address_book.find(dest);
1if (found != m_address_book.end()) found->m_used = true;

luke-jr commented at 5:53 pm on November 10, 2022:

No? It needs to set m_used regardless of whether it’s already in the addressbook or not…

S3RK commented at 9:54 pm on November 10, 2022:

It seems to me this is called whenever new item added to mapWallet including spends from the wallet. If so, then this will add all outputs of spending txs to the address book.

luke-jr commented at 10:43 pm on November 10, 2022:

Yes, that’s what it’s supposed to do.

S3RK commented at 7:32 am on November 14, 2022:

I don’t think we should automatically add to the address book all addresses we’re sending to

ryanofsky commented at 7:07 pm on February 23, 2024:

re: #22693 (review)

I don’t think we should automatically add to the address book all addresses we’re sending to

I think i understand why this is being done after seeing the original PR #15987

The goal of the PR is to warn the user if they are sending to an address they have previously sent to. The address book is a convenient place to store this information.

Originally looking at this PR, I thought the goal just to make it easy to look up whether a receiving address had been used, in which case dest should already be in m_address_book here. But even for receiving addresses it can make sense in some cases to set m_address_book[dest].m_used = true even if dest is is not in m_address_book , for example if this is a backup wallet that is being resynced and the address book is incomplete. So I think the approach of adding to m_address_book unconditionally makes sense for IsMine receiving addresses, and can make sense for addresses being sent to as well.

S3RK commented at 12:06 pm on April 11, 2024:

This would conflate user entered data with programmatically generated data. Address book today is managed by the user. If we want to store a list of all addresses we sent to we should pick a new storage for that.

ryanofsky commented at 6:22 pm on September 28, 2022: contributor

Reviewed 3ee478b9cba305dc87de1ad093ca14a89e58b516 and I think this seems useful and could be a good approach with a little bit of cleanup. I think the only bad part of this approach is that it adds new addresses to the address book requiring the last commit “Bugfix: Wallet: Don’t initialise “used” flag for wallet tool “info” command” (3ee478b9cba305dc87de1ad093ca14a89e58b516) which seems fragile and messy. It would be better to not change what addresses are in the address book if possible, or maybe do something like modify ForEachAddrBook / ListAddrBook functions so existing code isn’t affected and doesn’t need a do_init_used_flag switch.

Would also be good to update the PR description to motivate this, maybe taking relevant parts from #15987 discussion.

luke-jr force-pushed on Nov 10, 2022

luke-jr commented at 6:28 pm on November 10, 2022: member

Rebased & addressed review

Would naming the RPC field used_txids be clearer than use_txids? (if I’m not confused)

No, the txids aren’t used. The address is used, in those txid(s).

luke-jr force-pushed on Nov 10, 2022

DrahtBot removed the label Needs rebase on Nov 10, 2022

in src/wallet/rpc/addresses.cpp:547 in c4f88f4145 outdated

538@@ -539,6 +539,10 @@ RPCHelpMan getaddressinfo()
539                         {
540                             {RPCResult::Type::STR, "label name", "Label name (defaults to \"\")."},
541                         }},
542+                        {RPCResult::Type::ARR, "use_txids", "",
543+                        {
544+                            {RPCResult::Type::STR_HEX, "txid", "The ids of transactions involving this wallet which received with the address"},

ajtowns commented at 7:42 am on January 3, 2023:

Perhaps better to call the field receiving_txids or similar, to avoid the ambiguity between “I’ve received funds via this address” and “I’ve spent funds that I received via this address”.

brunoerg commented at 10:18 am on January 27, 2023:

I agree with @ajtowns, I confess it took me a while to understand what use_txids means, at a first look I thought it would return all txids that involves that address but reading the doc could understand that’s only about the receiving ones. receiving_txids sounds better.

ajtowns commented at 7:56 am on January 3, 2023: contributor

Concept ACK – I’m not familiar enough with the wallet to do more than that.

I’m not really clear on what the use for this is. It seems much more limited than #15987 – if I understand correctly, this PR only makes it easier to tell if you’re reusing one of your own wallet addresses for receiving, it doesn’t tell you if you’re reusing an address you’ve already used to send to someone else. That seems like it might be useful if you’ve got multiple applications sharing your wallet (doing both lightning and coinjoin maybe?), but otherwise not very interesting?

If you setup a watchonly wallet for someone else – if they gave you their xpub eg – it could also help you avoid reusing addresses when paying them, however.

in src/wallet/wallet.cpp:795 in 60decf2579 outdated

696+{
697+    for (const auto& entry : mapWallet) {
698+        const CWalletTx& wtx = entry.second;
699+        UpdateAddressBookUsed(wtx);
700+    }
701+}

achow101 commented at 9:32 pm on January 12, 2023:

In 60decf25796558085cf151cd982e2cb8bff1d32b “Wallet: Keep track of what addresses are used in wallet transactions (memory only)”

Since UpdateAddressBookUsed does not require any other information that is loaded into the wallet, this function could be removed and have UpdateAddressBookUsed called when each transaction is loaded in LoadToWallet.

luke-jr commented at 11:15 pm on July 5, 2023:

That would break the wallettool again

in src/wallet/wallet.cpp:806 in 803946f3e8 outdated

709@@ -709,6 +710,43 @@ void CWallet::UpdateAddressBookUsed(const CWalletTx& wtx)
710     }
711 }
712 
713+bool CWallet::FindScriptPubKeyUsed(const std::set<CScript>& keys, const std::variant<std::monostate, std::function<void(const CWalletTx&)>, std::function<void(const CWalletTx&, uint32_t)>>& callback) const

achow101 commented at 9:45 pm on January 12, 2023:

In 803946f3e8ac66d2e2e23a336e2a36f8e44881c7 “Wallet: Add fairly-efficient [negative] check that an address is not known to be used”

nit: s/keys/scripts/. I would prefer that variable names are accurate.

in src/wallet/rpc/addresses.cpp:545 in c961db6773 outdated

538@@ -539,6 +539,10 @@ RPCHelpMan getaddressinfo()
539                         {
540                             {RPCResult::Type::STR, "label name", "Label name (defaults to \"\")."},
541                         }},
542+                        {RPCResult::Type::ARR, "use_txids", "",

achow101 commented at 9:52 pm on January 12, 2023:

In c961db6773a6c87c3f9ade97983d29caa354f745 “RPC/Wallet: Add “use_txids” to output of getaddressinfo”

Without having read the help text, I’m having a hard time figuring out the meaning of this field. My initial reading had “use” as a verb, but that doesn’t make sense. I think it might be clearer to rename this to something like used_in_txids or received_with_txids.

achow101 commented at 9:06 pm on April 15, 2024:

This comment was marked as resolved but nothing was changed.

in test/functional/wallet_basic.py:659 in c961db6773 outdated

636@@ -637,6 +637,16 @@ def run_test(self):
637         assert not address_info["iswatchonly"]
638         assert not address_info["isscript"]
639         assert not address_info["ischange"]
640+        assert_equal(address_info['use_txids'], [])
641+
642+        # Test getaddressinfo 'use_txids' field
643+        addr = "mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ"

achow101 commented at 9:54 pm on January 12, 2023:

In c961db6773a6c87c3f9ade97983d29caa354f745 “RPC/Wallet: Add “use_txids” to output of getaddressinfo”

nit: Does this address have to be hardcoded? I think we want to avoid hardcoding things like this in tests.

brunoerg commented at 10:08 am on January 27, 2023:

I think we should avoid hardcoded stuff, but I noticed this address is hardcoded right before when testing getaddressinfo, probably to test scriptPubKey. Anyway, I think we could move it to a variable and use it whatever we need.

 0diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py
 1index 0372ebf15..54730533e 100755
 2--- a/test/functional/wallet_basic.py
 3+++ b/test/functional/wallet_basic.py
 4@@ -630,8 +630,9 @@ class WalletTest(BitcoinTestFramework):
 5 
 6         # Test getaddressinfo on external address. Note that these addresses are taken from disablewallet.py
 7         assert_raises_rpc_error(-5, "Invalid prefix for Base58-encoded address", self.nodes[0].getaddressinfo, "3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy")
 8-        address_info = self.nodes[0].getaddressinfo("mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ")
 9-        assert_equal(address_info['address'], "mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ")
10+        addr = "mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ"
11+        address_info = self.nodes[0].getaddressinfo(addr)
12+        assert_equal(address_info['address'], addr)
13         assert_equal(address_info["scriptPubKey"], "76a9144e3854046c7bd1594ac904e4793b6a45b36dea0988ac")
14         assert not address_info["ismine"]
15         assert not address_info["iswatchonly"]
16@@ -640,7 +641,6 @@ class WalletTest(BitcoinTestFramework):
17         assert_equal(address_info['use_txids'], [])
18 
19         # Test getaddressinfo 'use_txids' field
20-        addr = "mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ"
21         txid_1 = self.nodes[0].sendtoaddress(addr, 1)
22         address_info = self.nodes[0].getaddressinfo(addr)
23         assert_equal(address_info['use_txids'], [txid_1])

achow101 commented at 10:10 pm on January 12, 2023: member

I’m not a fan of the hack in the last commit (c4f88f4145386fb8014433164a68b272b6255270), it seems really fragile and ugly to have this parameter have to be passed all the way through the LoadWallet call stack. Related to that, I’m not sure that adding an (in memory) address book entry for every single output address in all of our transactions is a good idea. It will result in users seeing a ton of address book entries that weren’t there before that are all marked “unknown”. This could be extremely confusing.

Although it would increase the memory footprint a bit, I think we could resolve both of these issues by having a separate field in the wallet that tracks which addresses have received coins the past. This way we won’t pollute the address book. Additionally, it could store CScripts rather than CTxDestinations which allows us to avoid reusing things that don’t have an address, such as P2PK outputs, data carrier outputs, and non-standard scripts.

brunoerg commented at 10:18 am on January 27, 2023: contributor

Concept ACK

S3RK commented at 8:09 pm on January 27, 2023: contributor

@luke-jr do you want to have the “use_txids” for all addresses throughout the whole blockchain or only for a subset of them?

DrahtBot added the label Needs rebase on Feb 1, 2023

fanquake marked this as a draft on Feb 28, 2023

fanquake commented at 10:35 am on February 28, 2023: member

Drafted for now. Feedback/comments needs addressing, needs rebase, PR description needs updating to mention the new related PR (#15987 is closed).

fanquake commented at 9:55 am on May 29, 2023: member

Can be reopened if/when work is continued.

fanquake closed this on May 29, 2023

achow101 reopened this on Jul 5, 2023

luke-jr commented at 11:18 pm on July 5, 2023: member

Rebased.

if I understand correctly, this PR only makes it easier to tell if you’re reusing one of your own wallet addresses for receiving, it doesn’t tell you if you’re reusing an address you’ve already used to send to someone else.

do you want to have the “use_txids” for all addresses throughout the whole blockchain or only for a subset of them?

As documented, it only keeps track of addresses used in wallet transactions (whether we are the sender or receiver).

luke-jr force-pushed on Jul 5, 2023

luke-jr marked this as ready for review on Jul 5, 2023

DrahtBot added the label CI failed on Jul 6, 2023

DrahtBot removed the label Needs rebase on Jul 6, 2023

DrahtBot removed the label CI failed on Jul 6, 2023

DrahtBot added the label Needs rebase on Aug 22, 2023

cacrowley approved

luke-jr force-pushed on Nov 24, 2023

DrahtBot removed the label Needs rebase on Nov 25, 2023

DrahtBot added the label Needs rebase on Dec 14, 2023

luke-jr force-pushed on Dec 18, 2023

luke-jr requested review from aureleoules on Dec 18, 2023

luke-jr requested review from kristapsk on Dec 18, 2023

luke-jr requested review from rajarshimaitra on Dec 18, 2023

luke-jr requested review from ajtowns on Dec 18, 2023

luke-jr requested review from S3RK on Dec 18, 2023

luke-jr requested review from jonatack on Dec 18, 2023

luke-jr requested review from achow101 on Dec 18, 2023

luke-jr requested review from furszy on Dec 18, 2023

luke-jr requested review from ryanofsky on Dec 18, 2023

luke-jr requested review from brunoerg on Dec 18, 2023

luke-jr requested review from pk-b2 on Dec 18, 2023

luke-jr requested review from cacrowley on Dec 18, 2023

luke-jr requested review from bre876 on Dec 18, 2023

DrahtBot removed the label Needs rebase on Dec 18, 2023

kristapsk approved

kristapsk commented at 3:13 pm on December 18, 2023: contributor

ACK cd4e5ddaf7f9bd50690952eb82228c3b328c9221

DrahtBot removed review request from rajarshimaitra on Dec 18, 2023

DrahtBot removed review request from cacrowley on Dec 18, 2023

DrahtBot removed review request from bre876 on Dec 18, 2023

DrahtBot removed review request from pk-b2 on Dec 18, 2023

DrahtBot requested review from pk-b2 on Dec 18, 2023

DrahtBot requested review from fjahr on Dec 18, 2023

DrahtBot requested review from rajarshimaitra on Dec 18, 2023

DrahtBot requested review from theStack on Dec 18, 2023

luke-jr force-pushed on Dec 18, 2023

DrahtBot removed review request from pk-b2 on Dec 18, 2023

DrahtBot removed review request from rajarshimaitra on Dec 18, 2023

DrahtBot requested review from rajarshimaitra on Dec 18, 2023

luke-jr force-pushed on Dec 18, 2023

DrahtBot requested review from pk-b2 on Dec 18, 2023

DrahtBot added the label CI failed on Jan 14, 2024

DrahtBot added the label Needs rebase on Feb 12, 2024

Wallet: Keep track of what addresses are used in wallet transactions (memory only) fc7954a148

Wallet: Add fairly-efficient [negative] check that an address is not known to be used 022887d933

RPC/Wallet: Add "use_txids" to output of getaddressinfo a00bc6f395

luke-jr force-pushed on Feb 21, 2024

luke-jr commented at 3:39 pm on February 21, 2024: member

Rebased again

DrahtBot removed the label Needs rebase on Feb 21, 2024

DrahtBot removed the label CI failed on Feb 21, 2024

in src/wallet/wallet.h:244 in fc7954a148 outdated

236@@ -237,6 +237,12 @@ struct CAddressBookData
237      */
238     std::optional<std::string> label;
239 
240+    /** Whether address is the destination of any wallet transation.
241+     * Unlike other fields in address data struct, the used value is determined
242+     * at runtime and not serialized as part of address data.
243+     */
244+    bool m_used{false};

ryanofsky commented at 6:09 pm on February 23, 2024:

In commit “Wallet: Keep track of what addresses are used in wallet transactions (memory only)” (fc7954a1488b815dac4311b5a461fe5bf2a20b19)

Would suggest moving this next to the previously_spent member below and naming it previously_received instead of m_used. Calling it “used” is ambiguous and confusing because at the walletdb level, “used” indicates whether funds sent to the address have previously been spent, not whether funds were sent to the address.

Other names for “previously_received” / “previously_spent” could work too of course, but “used” seems too ambiguous.

in src/wallet/wallettool.cpp:171 in fc7954a148 outdated

166@@ -167,7 +167,8 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command)
167         DatabaseOptions options;
168         ReadDatabaseArgs(args, options);
169         options.require_existing = true;
170-        const std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, options);
171+        // NOTE: We need to skip initialisation of the m_used flag, or else the address book count might be wrong
172+        const std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, options, CWallet::do_init_used_flag::Skip);

ryanofsky commented at 6:36 pm on February 23, 2024:

In commit “Wallet: Keep track of what addresses are used in wallet transactions (memory only)” (fc7954a1488b815dac4311b5a461fe5bf2a20b19)

This workaround is not really sufficient because it fixes the address book count for the wallet tool without fixing it for the bitcoind wallet:

https://github.com/bitcoin/bitcoin/blob/1ac627c485a43e50a9a49baddce186ee3ad4daad/src/wallet/wallet.cpp#L3171

Ideally no workaround might be necessary if we could avoid adding extra entries to m_address_book (see #22693 (review)). But assuming a workaround is necessary, it could be implemented more simply in both places with a new CAddressBookData method:

0bool InAddressBook() const { return label || purpose || !receive_requests.empty(); }

used with std::count_if instead of m_address_book.size() to return number of address book entries.

ryanofsky commented at 7:27 pm on February 23, 2024: contributor

Approach ACK a00bc6f395ecd2f9657b9edd4c9c77883a0dc718. There are a few small things I think need to be cleaned up but this seems like a good approach and a nice feature.

Retropex referenced this in commit b58acfcd11 on Mar 28, 2024

Retropex referenced this in commit 559a93d167 on Mar 28, 2024

Retropex referenced this in commit 06aafaaa85 on Mar 28, 2024

S3RK commented at 12:08 pm on April 11, 2024: contributor

I’m still not convinced about the approach of storing programmatically generated data in the address book, which is supposed to store user entered data.

For that reason I’m leaning Approach NACK.

achow101 commented at 9:11 pm on April 15, 2024: member

Approach NACK

The review I left last year outlines my concerns with this approach, and they have not been addressed in any way.

achow101 removed review request from achow101 on Apr 15, 2024

achow101 removed review request from rajarshimaitra on Apr 15, 2024

achow101 removed review request from pk-b2 on Apr 15, 2024

RPC/Wallet: Add “use_txids” to output of getaddressinfo #22693

Code Coverage

Reviews

Conflicts