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

luke-jr · 2021-08-13T00:19:27Z

(Non-GUI part of bitcoin-core/gui#562, but without the bloom stuff)

ghost · 2021-08-13T00:27:45Z

Concept ACK

src/wallet/wallet.cpp

kristapsk · 2021-08-27T11:58:53Z

Concept ACK

DrahtBot · 2021-09-09T17:33:06Z

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)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)

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.

rajarshimaitra

Strong concept + tACK 9fe7926

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 · 2021-12-18T23:50:11Z

Rebased

theStack · 2021-12-29T01:44:22Z

Concept ACK

ghost

tACK bfb162d

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:

bitcoin-cli -rpcwallet="" getnewaddress
tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln

Check result for getaddressinfo:

bitcoin-cli -rpcwallet="" getaddressinfo tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln
{
  "address": "tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln",
  "scriptPubKey": "0014857b28643beb848865ec3b4f187f2bc63aab4eb3",
  "ismine": true,
  "solvable": true,
  "desc": "wpkh([6ba7ec01/0'/0'/40']02f1b431e7cacb981fb24cb28365c6f45b8b7a4f891d3b4dfc1981dd7705b95249)#jypvcx7u",
  "iswatchonly": false,
  "isscript": false,
  "iswitness": true,
  "witness_version": 0,
  "witness_program": "857b28643beb848865ec3b4f187f2bc63aab4eb3",
  "pubkey": "02f1b431e7cacb981fb24cb28365c6f45b8b7a4f891d3b4dfc1981dd7705b95249",
  "ischange": false,
  "timestamp": 1639765317,
  "hdkeypath": "m/0'/0'/40'",
  "hdseedid": "37bb332a78fc64d32f269a40710ed0d34d0a0ba5",
  "hdmasterfingerprint": "6ba7ec01",
  "labels": [
    ""
  ],
  "use_txids": [
  ]
}

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

bitcoin-cli -rpcwallet="" getaddressinfo tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln
{
  "address": "tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln",
  "scriptPubKey": "0014857b28643beb848865ec3b4f187f2bc63aab4eb3",
  "ismine": true,
  "solvable": true,
  "desc": "wpkh([6ba7ec01/0'/0'/40']02f1b431e7cacb981fb24cb28365c6f45b8b7a4f891d3b4dfc1981dd7705b95249)#jypvcx7u",
  "iswatchonly": false,
  "isscript": false,
  "iswitness": true,
  "witness_version": 0,
  "witness_program": "857b28643beb848865ec3b4f187f2bc63aab4eb3",
  "pubkey": "02f1b431e7cacb981fb24cb28365c6f45b8b7a4f891d3b4dfc1981dd7705b95249",
  "ischange": false,
  "timestamp": 1639765317,
  "hdkeypath": "m/0'/0'/40'",
  "hdseedid": "37bb332a78fc64d32f269a40710ed0d34d0a0ba5",
  "hdmasterfingerprint": "6ba7ec01",
  "labels": [
    ""
  ],
  "use_txids": [
    "feaf134bc878824c5cfb7fc1d7b2a6c802f2482dd7becf8f5cc46a473e3bd4e0"
  ]
}

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

bitcoin-cli -rpcwallet="" getaddressinfo tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln
{
  "address": "tb1qs4ajsepmawzgse0v8d83sletcca2kn4nzr9hln",
  "scriptPubKey": "0014857b28643beb848865ec3b4f187f2bc63aab4eb3",
  "ismine": true,
  "solvable": true,
  "desc": "wpkh([6ba7ec01/0'/0'/40']02f1b431e7cacb981fb24cb28365c6f45b8b7a4f891d3b4dfc1981dd7705b95249)#jypvcx7u",
  "iswatchonly": false,
  "isscript": false,
  "iswitness": true,
  "witness_version": 0,
  "witness_program": "857b28643beb848865ec3b4f187f2bc63aab4eb3",
  "pubkey": "02f1b431e7cacb981fb24cb28365c6f45b8b7a4f891d3b4dfc1981dd7705b95249",
  "ischange": false,
  "timestamp": 1639765317,
  "hdkeypath": "m/0'/0'/40'",
  "hdseedid": "37bb332a78fc64d32f269a40710ed0d34d0a0ba5",
  "hdmasterfingerprint": "6ba7ec01",
  "labels": [
    ""
  ],
  "use_txids": [
    "b8d96d7683b1ed482c53c2b0b191e91d5db671ee21009fac4d72546bb13fde47",
    "feaf134bc878824c5cfb7fc1d7b2a6c802f2482dd7becf8f5cc46a473e3bd4e0"
  ]
}

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

bitcoin-cli -rpcwallet="" getaddressinfo tb1qsp9g4v04yakgulv7ylz9t8g7m6cj46qff6k023
{
  "address": "tb1qsp9g4v04yakgulv7ylz9t8g7m6cj46qff6k023",
  "scriptPubKey": "0014804a8ab1f5276c8e7d9e27c4559d1edeb12ae809",
  "ismine": false,
  "solvable": false,
  "iswatchonly": false,
  "isscript": false,
  "iswitness": true,
  "witness_version": 0,
  "witness_program": "804a8ab1f5276c8e7d9e27c4559d1edeb12ae809",
  "ischange": false,
  "labels": [
  ],
  "use_txids": [
  ]
}

Send some bitcoin to this address from Core:

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

Check result for getaddressinfo:

bitcoin-cli -rpcwallet="" getaddressinfo tb1qsp9g4v04yakgulv7ylz9t8g7m6cj46qff6k023
{
  "address": "tb1qsp9g4v04yakgulv7ylz9t8g7m6cj46qff6k023",
  "scriptPubKey": "0014804a8ab1f5276c8e7d9e27c4559d1edeb12ae809",
  "ismine": false,
  "solvable": false,
  "iswatchonly": false,
  "isscript": false,
  "iswitness": true,
  "witness_version": 0,
  "witness_program": "804a8ab1f5276c8e7d9e27c4559d1edeb12ae809",
  "ischange": false,
  "labels": [
  ],
  "use_txids": [
    "f9d09be836ca90951a5ce734598a5eb1fc56d2ed36a7d0c0c47a11dbd4a1e624"
  ]
}

luke-jr · 2022-02-09T07:52:23Z

Rebased again

…(memory only)

…known to be used

luke-jr · 2024-02-21T15:39:59Z

Rebased again

ryanofsky

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

ryanofsky · 2024-02-23T18:09:21Z

src/wallet/wallet.h

+     * Unlike other fields in address data struct, the used value is determined
+     * at runtime and not serialized as part of address data.
+     */
+    bool m_used{false};


In commit "Wallet: Keep track of what addresses are used in wallet transactions (memory only)" (fc7954a)

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.

ryanofsky · 2024-02-23T18:36:34Z

src/wallet/wallettool.cpp

+        // NOTE: We need to skip initialisation of the m_used flag, or else the address book count might be wrong
+        const std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, options, CWallet::do_init_used_flag::Skip);


In commit "Wallet: Keep track of what addresses are used in wallet transactions (memory only)" (fc7954a)

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

bitcoin/src/wallet/wallet.cpp

Line 3171 in 1ac627c

walletInstance->WalletLogPrintf("m_address_book.size() = %u\n", walletInstance->m_address_book.size());

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

bool 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 · 2024-02-23T18:51:26Z

src/wallet/wallet.cpp

re: #22693 (comment)

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:

std::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).

src/wallet/wallet.cpp

…(memory only) Github-Pull: bitcoin#22693 Rebased-From: fc7954a

…known to be used Github-Pull: bitcoin#22693 Rebased-From: 022887d

Github-Pull: bitcoin#22693 Rebased-From: a00bc6f

S3RK · 2024-04-11T12:08:54Z

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 · 2024-04-15T21:11:44Z

Approach NACK

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

achow101 · 2024-10-15T14:46:31Z

This PR does not seem to have attracted much attention from reviewers. As such, it does not seem important enough right now to keep it sitting idle in the list of open PRs.

Closing due to lack of interest.

DrahtBot added RPC/REST/ZMQ Wallet labels Aug 13, 2021

luke-jr force-pushed the getaddressinfo_txids branch from bcec6dc to 160d61c Compare August 13, 2021 02:46

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

luke-jr force-pushed the getaddressinfo_txids branch from 160d61c to 9fe7926 Compare August 13, 2021 02:49

bre876 approved these changes Aug 16, 2021

View reviewed changes

kristapsk reviewed Aug 27, 2021

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Sep 9, 2021

wallet: Automatically add receiving destinations to the address book #22929

Merged

DrahtBot mentioned this pull request Sep 25, 2021

Allow UTXO locks to be written to wallet DB #23065

Merged

DrahtBot added the Needs rebase label Sep 26, 2021

rajarshimaitra mentioned this pull request Dec 18, 2021

Use the new script_sync logic for RPC backend bitcoindevkit/bdk#466

Closed

3 tasks

rajarshimaitra reviewed Dec 18, 2021

View reviewed changes

luke-jr force-pushed the getaddressinfo_txids branch from f954a0d to c6e5a3b Compare December 18, 2021 21:34

DrahtBot removed the Needs rebase label Dec 18, 2021

DrahtBot mentioned this pull request Dec 19, 2021

refactor: Remove gArgs from bdb.h and sqlite.h #23732

Merged

ghost approved these changes Dec 29, 2021

View reviewed changes

This was referenced Jan 11, 2022

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

Revert "Add to spends only transcations from me" #24083

Merged

DrahtBot added the Needs rebase label Feb 1, 2022

luke-jr force-pushed the getaddressinfo_txids branch from c6e5a3b to 8719b08 Compare February 9, 2022 07:51

luke-jr mentioned this pull request Feb 9, 2022

Wallet, GUI: Warn when sending to already-used Bitcoin addresses #15987

Closed

DrahtBot removed the Needs rebase label Feb 9, 2022

luke-jr force-pushed the getaddressinfo_txids branch 2 times, most recently from 03e8a66 to cd4e5dd Compare December 18, 2023 15:19

DrahtBot requested review from rajarshimaitra and removed request for pk-b2 and rajarshimaitra December 18, 2023 15:19

luke-jr force-pushed the getaddressinfo_txids branch from cd4e5dd to 03e8a66 Compare December 18, 2023 15:19

DrahtBot requested a review from pk-b2 December 18, 2023 15:19

DrahtBot added the CI failed label Jan 14, 2024

DrahtBot added the Needs rebase label Feb 12, 2024

luke-jr added 3 commits February 21, 2024 15:38

Wallet: Keep track of what addresses are used in wallet transactions …

fc7954a

…(memory only)

Wallet: Add fairly-efficient [negative] check that an address is not …

022887d

…known to be used

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

a00bc6f

luke-jr force-pushed the getaddressinfo_txids branch from 03e8a66 to a00bc6f Compare February 21, 2024 15:38

DrahtBot removed Needs rebase CI failed labels Feb 21, 2024

ryanofsky reviewed Feb 23, 2024

View reviewed changes

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024

Wallet: Keep track of what addresses are used in wallet transactions …

b58acfc

…(memory only) Github-Pull: bitcoin#22693 Rebased-From: fc7954a

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024

Wallet: Add fairly-efficient [negative] check that an address is not …

559a93d

…known to be used Github-Pull: bitcoin#22693 Rebased-From: 022887d

Retropex pushed a commit to Retropex/bitcoin that referenced this pull request Mar 28, 2024

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

06aafaa

Github-Pull: bitcoin#22693 Rebased-From: a00bc6f

achow101 removed request for achow101, pk-b2 and rajarshimaitra April 15, 2024 21:11

DrahtBot added CI failed and removed CI failed labels Sep 7, 2024

achow101 closed this Oct 15, 2024

bitcoin locked and limited conversation to collaborators Oct 15, 2025

		// NOTE: We need to skip initialisation of the m_used flag, or else the address book count might be wrong
		const std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, options, CWallet::do_init_used_flag::Skip);

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

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

Uh oh!

Conversation

luke-jr commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 13, 2021

Uh oh!

Uh oh!

kristapsk commented Aug 27, 2021

Uh oh!

DrahtBot commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Dec 18, 2021

Uh oh!

theStack commented Dec 29, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Feb 9, 2022

Uh oh!

luke-jr commented Feb 21, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

S3RK commented Apr 11, 2024

Uh oh!

achow101 commented Apr 15, 2024

Uh oh!

achow101 commented Oct 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

luke-jr commented Aug 13, 2021 •

edited

Loading

DrahtBot commented Sep 9, 2021 •

edited

Loading

ryanofsky Feb 23, 2024 •

edited

Loading

ryanofsky Feb 23, 2024 •

edited

Loading