wallet: Add wallet method to detect if a key is "active" #27216

pinheadmz · 2023-03-06T21:36:57Z

This is a first step towards closing #3314 by implementing functions in Wallet and ScriptPubKeyMan that determine if a key is "active" meaning "derived from a currently active scriptpubkey manager."

A key is NOT "active" if:

It was imported using an import{privkey, address, pubkey} rpc
It was derived before the wallet was encrypted (which replaces the active SPKM(s))

Users should be informed that these keys may be compromised:

Some other wallet generated the private key and may still have a copy
The private key was stored on disk unencrypted

This PR adds a new field isactive to rpc getaddressinfo, follow-up work will be adding this to the GUI address book as well, something like this. The original issue also recommends sweeping all value from keys in this category, I think that feature can be discussed in the future as a follow-up.

This PR also patches PKDescriptor from #22051 where matching public keys were not returned.

Follow-up PR for the GUI: bitcoin-core/gui#723

DrahtBot · 2023-03-06T21:37:00Z

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	jarolrod, pablomartin4btc
Stale ACK	jonatack

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:

#28528 (test: Use test framework utils in functional tests by osagie98)
#28454 (wallet: allowing walletpassphrase timeout to be MAX_SLEEP_TIME if set to -1 by kevkevinpal)
#28403 (test: Bump walletpassphrase timeouts to avoid intermittent issues by MarcoFalke)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
#24142 (Deprecate SubtractFeeFromOutputs by achow101)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation 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.

src/wallet/test/scriptpubkeyman_tests.cpp

fanquake · 2023-03-07T14:46:32Z

https://github.com/bitcoin/bitcoin/pull/27216/checks?check_run_id=11804958852

Assertion failed: lock cs_wallet not held in ./wallet/wallet.h:444; locks held:
'cs_KeyStore' in wallet/scriptpubkeyman.cpp:1185 (in thread 'test')

pinheadmz · 2023-04-05T16:01:20Z

rebased on master and addressed some nits from follow-up PR bitcoin-core/gui#723

jonatack

Concept ACK, tested and reviewed each commit. Some thoughts and questions follow.

This PR also patches PKDescriptor from #22051 where matching public keys were not returned.

I didn't see where/how; can you help or explain?

src/script/descriptor.cpp

src/wallet/scriptpubkeyman.h

src/wallet/scriptpubkeyman.cpp

src/wallet/rpc/addresses.cpp

src/wallet/test/scriptpubkeyman_tests.cpp

src/wallet/wallet.cpp

test/functional/wallet_keypool.py

src/wallet/rpc/addresses.cpp

pinheadmz · 2023-04-08T16:30:50Z

@jonatack thanks so much for the great review, addressed comments (1 or 2 questions along the way) rebase on master and also rebased the GUI follow up

pinheadmz · 2023-04-13T15:14:49Z

rebase to: 58fba35e25c2b9dcdd5b3dd0aa8f412801a98146

added nodiscard
cleaned up help message in getaddressinfo
added release note file

jonatack

ACK with some suggestions below.

The release note and added test coverage could optionally all be in separate commits, provided each commit is hygienic, i.e. builds and all tests pass. It's a bit easier for reviewers if updates needed for tests to pass with a change are in the same commit, while additional coverage is in separate commits.

doc/release-notes/release-notes-27216.md

src/wallet/rpc/addresses.cpp

test/functional/wallet_keypool.py

src/wallet/scriptpubkeyman.cpp

src/wallet/wallet.h

src/script/descriptor.cpp

src/wallet/test/scriptpubkeyman_tests.cpp

pinheadmz · 2023-04-14T19:26:58Z

thanks again @jonatack
rebase to 8139075

refactor commits to separate tests in hygenic order!
address review nits

jonatack

ACK 8139075 modulo eyes from someone more familiar with the wallet to validate the approach.

Commit 2a5774f should probably be renamed to something like test: cover ScriptPubKeyMan::IsKeyActive and IsDestinationActive

Re-reviewing this shuffled version was a little trickier due to the unneeded rebase. Please rebase only when you have to. Aside from that, the reorganized commits seem easier to review now.

jonatack · 2023-04-18T22:57:39Z

src/wallet/scriptpubkeyman.h

    virtual std::vector<WalletDestination> MarkUnusedAddresses(const CScript& script) { return {}; }

+    /* Determines if address is derived from active key manager */
+    [[nodiscard]] virtual bool IsKeyActive(const CScript& script) const { return false; };


fb3d39a It may make sense to declare this base class member as a pure virtual function. The implementation here seems unused/unnecessary.

Suggested change

[[nodiscard]] virtual bool IsKeyActive(const CScript& script) const { return false; };

virtual bool IsKeyActive(const CScript& script) const = 0;

Ah thanks for the ack, I'll update this and the commit message you mentioned if I need to revise again.

Thanks, both comments addressed with rebase

jonatack · 2023-04-28T16:00:55Z

81f29f0 I'm not sure if or when legacy wallet support is expected to be phased out, but if isactive is only different from ismine in legacy wallets, is it worth adding a field (and all the new methods), versus just updating the ismine field documentation in the help for descriptor wallets

Following up on #27216 (comment) above, according to #20160 legacy wallet support may be dropped in the next couple releases.

Github-Pull: bitcoin#27216 Rebased-From: 6765021

S3RK

Unfortunately, we can't use "activeness" of a descriptor to determine if it was generated before encryption.

Descriptor wallets allow as many descriptors as you want, but only one descriptor per OutputType could be active at a time. The activeness only specifies which descriptor to use when generating new addresses for that type. Therefore, there are more scenarios, besides the two you've listed, in which a key will be NOT "active". The simplest one would be — I just imported two p2wpkh descriptors for two BIP-44 accounts that I have. And only one of these descriptors could be active at a time.

In general, I believe that the proper solution to "exposed" keys is to get a new wallet with new set of keys, rather than rotating keys in existing wallet. We don't have a good separation between UTXOs/addresses within a wallet. On the other hand we already support multiple wallets, which are completely isolated so there are no chances of leaking/mixing UTXOs and addresses.

pinheadmz · 2023-09-26T14:07:12Z

I just imported two p2wpkh descriptors for two BIP-44 accounts that I have. And only one of these descriptors could be active at a time.

You would have to set "active":"true" when executing RPC importdescriptors and the default is false. But I see your point, considering this feature is for safety its not all that difficult for an advanced user to bypass that check.

Even if you did do this (import with active:true), I still think it is useful for getaddrinfo to notify you if an address is derived from an inactive descriptor. The issue I am targeting with this PR is more about protecting less advanced users (particularly with the GUI) from giving out "risky" receive addresses.

S3RK · 2023-09-26T22:21:00Z

I have two concerns with the approach:

I don't like changing semantic of "active" field to mean it's somehow unsafe.
Even for less advances users, I don't think it fully addresses the describe use-case, because it doesn't allow easy sweeping of compromised keys.

1/ The activeness indicates from which descriptor to derive new addresses. Based on the semantics of that field, It is incorrect to imply that addresses from inactive descriptors are less safe that addresses from active descriptors. Moreover, activeness of the descriptor is a user configurable parameter and we can't just reuse it to store information about "safety" of the descriptor.

2/ IIUC here's the use cases this PR tries to address: a user believes existing private keys are compromised and rotates the keys in the wallet. Since the keys are compromised it's unsafe to receive new and keep old funds. So after rotating the keys the user would have to sweep all the compromised keys. If I'm not mistaken, we currently don't support sweeping coins from a descriptor. However, we support the whole wallet sweeping with sendall command.

Separate wallet solution is also less error prone because it doesn't rely on a user to pay attention to the addresses to maintain safety. This approach (with optional sweeping) also works for privacy leaks, because it also prevents mixing UTXO histories.

DrahtBot · 2023-10-05T16:52:24Z

🐙 This pull request conflicts with the target branch and needs rebase.

pablomartin4btc

Concept ACK
Interesting points of view from both @pinheadmz and @S3RK, regarding "activeness" concept for descriptor wallets, the use case scenario this PR tries to solve and safety related with rotating keys vs wallet sweeping.
I'll review this PR further.

pinheadmz · 2023-10-12T14:50:34Z

I'm closing this to rethink the approach. Since 'active' descriptors can be imported it defeats the value of that field for this purpose. Really it seems the only property the wallet can really know for sure is if it generated keys into an encrypted wallet. I'll be back with a UI indicator for keys that we know have always been "safe".

This new method returns true if the given CScript key is derived from the SPKM. For Legacy that means checking the hd_seed_id in the key's metadata. Also patches PKDescriptor to return associated public keys in MakeScripts() Github-Pull: bitcoin#27216 Rebased-From: f39957f

This connects SPKM.IsKeyActive() up to the wallet level. Github-Pull: bitcoin#27216 Rebased-From: d33f3bd

…ctive() Github-Pull: bitcoin#27216 Rebased-From: ba65d93

Github-Pull: bitcoin#27216 Rebased-From: 6765021

This new method returns true if the given CScript key is derived from the SPKM. For Legacy that means checking the hd_seed_id in the key's metadata. Also patches PKDescriptor to return associated public keys in MakeScripts() Github-Pull: bitcoin#27216 Rebased-From: f39957f

This connects SPKM.IsKeyActive() up to the wallet level. Github-Pull: bitcoin#27216 Rebased-From: d33f3bd

…ctive() Github-Pull: bitcoin#27216 Rebased-From: ba65d93

Github-Pull: bitcoin#27216 Rebased-From: 6765021

This new method returns true if the given CScript key is derived from the SPKM. For Legacy that means checking the hd_seed_id in the key's metadata. Also patches PKDescriptor to return associated public keys in MakeScripts() Github-Pull: bitcoin#27216 Rebased-From: f39957f

This connects SPKM.IsKeyActive() up to the wallet level. Github-Pull: bitcoin#27216 Rebased-From: d33f3bd

…ctive() Github-Pull: bitcoin#27216 Rebased-From: ba65d93

Github-Pull: bitcoin#27216 Rebased-From: 6765021

fanquake reviewed Mar 7, 2023

View reviewed changes

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

pinheadmz force-pushed the used-addr-ui branch 4 times, most recently from 87da692 to 1e4b0e8 Compare March 9, 2023 21:07

DrahtBot mentioned this pull request Mar 10, 2023

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

pinheadmz force-pushed the used-addr-ui branch 2 times, most recently from c1bca08 to 4fec5fd Compare March 13, 2023 14:25

pinheadmz changed the title ~~Add wallet method to detect if a key is "fresh"~~ Add wallet method to detect if a key is "active" Mar 13, 2023

pinheadmz force-pushed the used-addr-ui branch from 4fec5fd to ad03bb1 Compare March 13, 2023 17:15

pinheadmz marked this pull request as ready for review March 13, 2023 18:27

pinheadmz mentioned this pull request Mar 24, 2023

Add warnings for non-active addresses in receive tab and address book bitcoin-core/gui#723

Closed

pinheadmz force-pushed the used-addr-ui branch from ad03bb1 to 81f29f0 Compare April 5, 2023 14:43

jonatack reviewed Apr 6, 2023

View reviewed changes

pinheadmz force-pushed the used-addr-ui branch from 81f29f0 to 05959e5 Compare April 8, 2023 16:23

pinheadmz force-pushed the used-addr-ui branch from 05959e5 to b95e2ee Compare April 13, 2023 15:12

jonatack reviewed Apr 13, 2023

View reviewed changes

src/wallet/test/scriptpubkeyman_tests.cpp Show resolved Hide resolved

pinheadmz added a commit to pinheadmz/bitcoin that referenced this pull request Apr 14, 2023

doc: release notes for bitcoin#27216

8139075

pinheadmz force-pushed the used-addr-ui branch from b95e2ee to 8139075 Compare April 14, 2023 19:25

jonatack reviewed Apr 18, 2023

View reviewed changes

DrahtBot added the CI failed label May 18, 2023

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023

test: cover "ismine" and "isactive" field in rpc getaddressinfo

3f78578

Github-Pull: bitcoin#27216 Rebased-From: 6765021

DrahtBot mentioned this pull request Aug 24, 2023

wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively #28333

Open

DrahtBot mentioned this pull request Sep 4, 2023

test: Bump walletpassphrase timeouts to avoid intermittent issues #28403

Merged

DrahtBot mentioned this pull request Sep 13, 2023

wallet: allowing walletpassphrase timeout to be MAX_SLEEP_TIME if set to -1 #28454

Closed

achow101 requested review from S3RK and Sjors September 20, 2023 17:05

DrahtBot mentioned this pull request Sep 25, 2023

test: Use test framework utils in functional tests #28528

Closed

S3RK reviewed Sep 25, 2023

View reviewed changes

S3RK mentioned this pull request Sep 26, 2023

Exposed / Compromised (must sweep) / Used / Imported (?) flags on addresses #3314

Open

DrahtBot mentioned this pull request Sep 28, 2023

Deprecate SubtractFeeFromOutputs #24142

Closed

DrahtBot added the Needs rebase label Oct 5, 2023

pablomartin4btc reviewed Oct 7, 2023

View reviewed changes

pinheadmz closed this Oct 12, 2023

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2024

wallet: implement IsDestinationActive() and add to rpc getaddressinfo

b752585

This connects SPKM.IsKeyActive() up to the wallet level. Github-Pull: bitcoin#27216 Rebased-From: d33f3bd

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2024

test: cover ScriptPubKeyMan::IsKeyActive() and Wallet::IsDestinationA…

e2be2f7

…ctive() Github-Pull: bitcoin#27216 Rebased-From: ba65d93

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2024

test: cover "ismine" and "isactive" field in rpc getaddressinfo

738023d

Github-Pull: bitcoin#27216 Rebased-From: 6765021

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 13, 2024

wallet: implement IsDestinationActive() and add to rpc getaddressinfo

f7d289e

This connects SPKM.IsKeyActive() up to the wallet level. Github-Pull: bitcoin#27216 Rebased-From: d33f3bd

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 13, 2024

test: cover ScriptPubKeyMan::IsKeyActive() and Wallet::IsDestinationA…

7464dc5

…ctive() Github-Pull: bitcoin#27216 Rebased-From: ba65d93

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 13, 2024

test: cover "ismine" and "isactive" field in rpc getaddressinfo

621c990

Github-Pull: bitcoin#27216 Rebased-From: 6765021

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2024

wallet: implement IsDestinationActive() and add to rpc getaddressinfo

ec8d084

This connects SPKM.IsKeyActive() up to the wallet level. Github-Pull: bitcoin#27216 Rebased-From: d33f3bd

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2024

test: cover ScriptPubKeyMan::IsKeyActive() and Wallet::IsDestinationA…

3ca4af2

…ctive() Github-Pull: bitcoin#27216 Rebased-From: ba65d93

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2024

test: cover "ismine" and "isactive" field in rpc getaddressinfo

bb8a78b

Github-Pull: bitcoin#27216 Rebased-From: 6765021

bitcoin locked and limited conversation to collaborators Oct 11, 2024

	[[nodiscard]] virtual bool IsKeyActive(const CScript& script) const { return false; };
	virtual bool IsKeyActive(const CScript& script) const = 0;

wallet: Add wallet method to detect if a key is "active" #27216

wallet: Add wallet method to detect if a key is "active" #27216

Uh oh!

Conversation

pinheadmz commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

fanquake commented Mar 7, 2023

Uh oh!

pinheadmz commented Apr 5, 2023

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pinheadmz commented Apr 8, 2023

Uh oh!

pinheadmz commented Apr 13, 2023

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pinheadmz commented Apr 14, 2023

Uh oh!

jonatack left a comment • 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!

jonatack Apr 18, 2023 • 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!

pinheadmz Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

pinheadmz Jun 2, 2023

Choose a reason for hiding this comment

Uh oh!

jonatack commented Apr 28, 2023

Uh oh!

S3RK left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Sep 26, 2023

Uh oh!

S3RK commented Sep 26, 2023

Uh oh!

DrahtBot commented Oct 5, 2023

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Oct 12, 2023

Uh oh!

pinheadmz commented Mar 6, 2023 •

edited

Loading

DrahtBot commented Mar 6, 2023 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack Apr 18, 2023 •

edited

Loading