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

pinheadmz commented at 9:36 PM on March 6, 2023: member

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: https://github.com/bitcoin-core/gui/pull/723

DrahtBot commented at 9:37 PM on March 6, 2023: 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	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.

in src/wallet/test/scriptpubkeyman_tests.cpp:142 in 63f1d1e56a outdated

 137 | +    {
 138 | +        LOCK(spkm->cs_desc_man);
 139 | +        WalletDescriptor descriptor = spkm->GetWalletDescriptor();
 140 | +        FlatSigningProvider provider;
 141 | +        std::vector<CScript> scripts3;
 142 | +        descriptor.descriptor->ExpandFromCache(/*index=*/10, descriptor.cache, scripts3, provider);

fanquake commented at 2:45 PM on March 7, 2023:

https://cirrus-ci.com/task/6502838134636544:

wallet/test/scriptpubkeyman_tests.cpp:142:48: error: argument name 'index' in comment does not match parameter name 'pos' [bugprone-argument-comment,-warnings-as-errors]
        descriptor.descriptor->ExpandFromCache(/*index=*/10, descriptor.cache, scripts3, provider);
                                               ^
./script/descriptor.h:137:38: note: 'pos' declared here
    virtual bool ExpandFromCache(int pos, const DescriptorCache& read_cache, std::vector<CScript>& output_scripts, FlatSigningProvider& out) const = 0;

pinheadmz commented at 3:01 PM on March 7, 2023:

oooh thanks, didn't realize those comments were parsed by anything!

fanquake commented at 2:46 PM on March 7, 2023: member

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 force-pushed on Mar 8, 2023

pinheadmz force-pushed on Mar 9, 2023

pinheadmz force-pushed on Mar 11, 2023

pinheadmz force-pushed on Mar 13, 2023

pinheadmz renamed this:
~~Add wallet method to detect if a key is "fresh"~~
Add wallet method to detect if a key is "active"
on Mar 13, 2023

pinheadmz force-pushed on Mar 13, 2023

pinheadmz marked this as ready for review on Mar 13, 2023

pinheadmz force-pushed on Apr 5, 2023

pinheadmz commented at 4:01 PM on April 5, 2023: member

rebased on master and addressed some nits from follow-up PR https://github.com/bitcoin-core/gui/pull/723

in src/script/descriptor.cpp:746 in 81f29f03a6 outdated

 729 | @@ -730,8 +730,11 @@ class PKDescriptor final : public DescriptorImpl
 730 |  private:
 731 |      const bool m_xonly;
 732 |  protected:
 733 | -    std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider&) const override
 734 | +    std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider& out) const override
 735 |      {
 736 | +        CKeyID id = keys[0].GetID();
 737 | +        out.pubkeys.emplace(id, keys[0]);

jonatack commented at 10:02 PM on April 5, 2023:

45d565073cab12b6107aae9d08048 nit, remove unnecessary local object while still keeping code readability.

(Also if we're not sure the element exists or its absence would be a logic error, maybe s/keys[0]/keys.at(0) to have bounds checks, as accessing a nonexistent element with vector#operator[] is UB).

-        CKeyID id = keys[0].GetID();
-        out.pubkeys.emplace(id, keys[0]);
+        out.pubkeys.emplace(/*CKeyID=*/keys[0].GetID(), /*CPubKey=*/keys[0]);

pinheadmz commented at 2:48 PM on April 8, 2023:

The style here matches the neighboring descriptors but I see your point about using .at(0) -- do you think all the descriptor::MakeScripts() should be cleaned up like this?

Also this chunk is the patch alluded to in the comment

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

When we expand a bare pubkey descriptor we weren't populating out.pubkeys and so GetAffectedKeys() wouldn't return anything

jonatack commented at 10:56 PM on April 13, 2023:

As to which one to use here in this new code, rather than following other code, I guess if we're not sure the element exists, use vector#find and check presence with !end(). If its absence is a program logic error (and to avoid UB), then vector#at. If we're sure of presence and optimizing for speed (say, in a hotspot), then vector#operator[].

in src/wallet/scriptpubkeyman.h:202 in 81f29f03a6 outdated

 197 | @@ -198,6 +198,9 @@ class ScriptPubKeyMan
 198 |       */
 199 |      virtual std::vector<WalletDestination> MarkUnusedAddresses(const CScript& script) { return {}; }
 200 |  
 201 | +    /* Determines if address is derived from active key manager */
 202 | +    virtual bool IsKeyActive(const CScript& script) { return false; };

jonatack commented at 9:04 PM on April 6, 2023:

https://github.com/bitcoin/bitcoin/commit/45d565073cab12b6107aae9d08048d5310d752d4 If they are thread-safe, should the IsKeyActive member functions be const?

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 9afd2538b0c..87ab7cc034b 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
 
-bool LegacyScriptPubKeyMan::IsKeyActive(const CScript& script)
+bool LegacyScriptPubKeyMan::IsKeyActive(const CScript& script) const
@@ -2228,7 +2226,7 @@ std::vector<WalletDestination> DescriptorScriptPubKeyMan::MarkUnusedAddresses(co
 
-bool DescriptorScriptPubKeyMan::IsKeyActive(const CScript& script)
+bool DescriptorScriptPubKeyMan::IsKeyActive(const CScript& script) const

diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index 4b2a6c5b177..aeef722d31f 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
     /* Determines if address is derived from active key manager */
-    virtual bool IsKeyActive(const CScript& script) { return false; };
+    virtual bool IsKeyActive(const CScript& script) const { return false; };
 
@@ -385,7 +385,7 @@ public:
 
-    bool IsKeyActive(const CScript& script) override;
+    bool IsKeyActive(const CScript& script) const override;
 
@@ -612,7 +612,7 @@ public:
 
-    bool IsKeyActive(const CScript& script) override;
+    bool IsKeyActive(const CScript& script) const override;

</p></details>

pinheadmz commented at 4:23 PM on April 8, 2023:

thanks will add this -- also I think these could be [[nodiscard]] as well? But I don't see that for any of the other bool Is...() members in SPKM...

in src/wallet/scriptpubkeyman.cpp:406 in 81f29f03a6 outdated

 406 | +            return false;
 407 | +        }
 408 | +        const CKeyMetadata& meta = it->second;
 409 | +        if (m_hd_chain.seed_id == meta.hd_seed_id)
 410 | +            return true;
 411 | +    }

jonatack commented at 9:10 PM on April 6, 2023:

https://github.com/bitcoin/bitcoin/commit/45d565073cab12b6107aae9d08048d5310d752d4

add missing conditional brackets (see developer notes)
avoid unneeded const CKeyMetadata& meta overhead in the loop, or...

     LOCK(cs_KeyStore);
-
-    // Not in the keystore at all
-    if (!IsMine(script)) return false;
+    if (!IsMine(script)) return false; // not in the keystore at all
 
     for (const auto& key_id : GetAffectedKeys(script, *this)) {
-        auto it = mapKeyMetadata.find(key_id);
+        const auto it{mapKeyMetadata.find(key_id)};
         if (it == mapKeyMetadata.end()) {
             // This key must be really old
             return false;
         }
-        const CKeyMetadata& meta = it->second;
-        if (m_hd_chain.seed_id == meta.hd_seed_id)
+        if (m_hd_chain.seed_id == it->second.hd_seed_id) {
             return true;
+        }
     }

...or alternately, do we need a null check here?

         const CKeyMetadata& meta = it->second;
-        if (m_hd_chain.seed_id == meta.hd_seed_id)
+        if (!meta.hd_seed_id.IsNull() && meta.hd_seed_id == m_hd_chain.seed_id) {

pinheadmz commented at 4:23 PM on April 8, 2023:

Thanks, updating.

in src/wallet/rpc/addresses.cpp:606 in 81f29f03a6 outdated

 602 | @@ -602,6 +603,7 @@ RPCHelpMan getaddressinfo()
 603 |      ret.pushKVs(detail);
 604 |  
 605 |      ret.pushKV("ischange", ScriptIsChange(*pwallet, scriptPubKey));
 606 | +    ret.pushKV("isactive", pwallet->IsDestinationActive(dest));

jonatack commented at 9:27 PM on April 6, 2023:

81f29f03a607cbfb7162705 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? (When I'm double-checking an address with getaddressinfo before using it to receive, ismine is a field I'm already checking.)

Otherwise, it might be handy for cli users to order isactive just after ismine, and in general group the "is" type of fields together in the RPC help and output, i.e. an order along the lines of ismine, isactive, iswatchonly, iswitness, isscript.

pinheadmz commented at 4:23 PM on April 8, 2023:

ok, moving isactive next to ismine. You have a good point about legacy wallet deprecation. I suppose when that happens most of this new code will be deprecated as well since its mainly implemented only in LegacyScriptPubKeyMan. Would it make sense to only add the field to RPC response in legacy wallets?

pinheadmz commented at 3:56 PM on July 10, 2023:

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? (When I'm double-checking an address with getaddressinfo before using it to receive, ismine is a field I'm already checking.) @jonatack after diving back in to this issue I realized that ismine != isactive for descriptor wallets all the time. I added tests in wallet_keypool to confirm this. The difference is whether or not the SPKM is ACTIVE (A descriptor wallet can have both active and inactive SPKMs and ismine at the RPC level merely indicates if the wallet can spend from the address).

So to summarize the test I just added:

create descriptor wallet
getnewaddress, getaddressinfo: ismine and isactive are both true
encrypt the wallet
getaddressinfo again: ismine is true but isactive is false

in src/wallet/test/scriptpubkeyman_tests.cpp:65 in 81f29f03a6 outdated

  60 | +    }
  61 | +
  62 | +    // 4 scripts per keypool key (P2PK, P2PKH, P2WPKH, P2SH-P2WPKH)
  63 | +    // Plus 4 scripts for the seed key
  64 | +    auto scripts1 = spkm.GetScriptPubKeys();
  65 | +    BOOST_CHECK(scripts1.size() == 84);

jonatack commented at 10:01 PM on April 6, 2023:

in general can use BOOST_CHECK_EQUAL for equality checks, if you prefer

    BOOST_CHECK_EQUAL(scripts1.size(), 84);

pinheadmz commented at 4:23 PM on April 8, 2023:

thanks, I do prefer!

in src/wallet/test/scriptpubkeyman_tests.cpp:193 in 81f29f03a6 outdated

 188 | +    int num_script_keys_not_found = 0;
 189 | +    for (const CScript& script : scripts4) {
 190 | +        WitnessV0ScriptHash scripthash(script);
 191 | +        if (!wallet.IsDestinationActive(scripthash))
 192 | +            num_script_keys_not_found++;
 193 | +    }

jonatack commented at 10:13 PM on April 6, 2023:

81f29f03a607cbfb7162705 conditional brackets and prefix increment operator per developer notes, drop unneeded localvar

-        WitnessV0ScriptHash scripthash(script);
-        if (!wallet.IsDestinationActive(scripthash))
-            num_script_keys_not_found++;
+        if (!wallet.IsDestinationActive(WitnessV0ScriptHash(script))) {
+            ++num_script_keys_not_found;
+        }

pinheadmz commented at 4:23 PM on April 8, 2023:

thanks got it

in src/wallet/wallet.cpp:2562 in 81f29f03a6 outdated

2556 | @@ -2557,6 +2557,16 @@ void CWallet::ForEachAddrBookEntry(const ListAddrBookFunc& func) const
2557 |      }
2558 |  }
2559 |  
2560 | +bool CWallet::IsDestinationActive(const CTxDestination& dest) const
2561 | +{
2562 | +    CScript script = GetScriptForDestination(dest);

jonatack commented at 10:41 PM on April 6, 2023:

https://github.com/bitcoin/bitcoin/commit/81f29f03a607cbfb7162705c1c1618ca7b59640e

avoid copy

    const CScript& script{GetScriptForDestination(dest)};

remove extra newline, and another implementation for the fun of it

 bool CWallet::IsDestinationActive(const CTxDestination& dest) const
 {
-    CScript script = GetScriptForDestination(dest);
-    for (const auto& spk_man : GetActiveScriptPubKeyMans()) {
-        if (spk_man->IsKeyActive(script)) return true;
-    }
-    return false;
+    const CScript& script{GetScriptForDestination(dest)};
+    const std::set<ScriptPubKeyMan*>& spkms{GetActiveScriptPubKeyMans()};
+    return std::any_of(spkms.cbegin(), spkms.cend(), [&script](const auto& spkm) { return spkm->IsKeyActive(script); });
 }
-

pinheadmz commented at 4:23 PM on April 8, 2023:

ah really cool with any_of thanks

in test/functional/wallet_keypool.py:106 in 81f29f03a6 outdated

 101 | +
 102 |          # Encrypt wallet and wait to terminate
 103 |          nodes[0].encryptwallet('test')
 104 | +        addr9_after_encrypting_data = nodes[0].getaddressinfo(addr9)
 105 | +        # Key is from unencrypted seed, no longer considered active
 106 | +        assert addr9_after_encrypting_data['isactive'] == False

jonatack commented at 11:16 PM on April 6, 2023:

https://github.com/bitcoin/bitcoin/commit/81f29f03a607cbfb7162705c1c1618ca7b59640e style nit, either omit the == False and the 3 == True above, or use assert_equal

pinheadmz commented at 4:23 PM on April 8, 2023:

okay yes

in src/wallet/rpc/addresses.cpp:509 in 81f29f03a6 outdated

 505 | @@ -506,7 +506,8 @@ RPCHelpMan getaddressinfo()
 506 |                          {RPCResult::Type::STR, "desc", /*optional=*/true, "A descriptor for spending coins sent to this address (only when solvable)."},
 507 |                          {RPCResult::Type::STR, "parent_desc", /*optional=*/true, "The descriptor used to derive this address if this is a descriptor wallet"},
 508 |                          {RPCResult::Type::BOOL, "isscript", "If the key is a script."},
 509 | -                        {RPCResult::Type::BOOL, "ischange", "If the address was used for change output."},
 510 | +                        {RPCResult::Type::BOOL, "ischange", "If the address was or will be used for change output."},

jonatack commented at 11:20 PM on April 6, 2023:

Might be good to explain this "or will be" change.

pinheadmz commented at 4:23 PM on April 8, 2023:

ok?

"If the address is reserved for use as a change output. It may or may not have already been used as change."

jonatack commented at 11:23 PM on April 6, 2023: member

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?

pinheadmz force-pushed on Apr 8, 2023

pinheadmz commented at 4:30 PM on April 8, 2023: member

@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 force-pushed on Apr 13, 2023

pinheadmz commented at 3:14 PM on April 13, 2023: member

rebase to: 58fba35e25c2b9dcdd5b3dd0aa8f412801a98146

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

in doc/release-notes/release-notes-27216.md:1 in b95e2ee982 outdated

   0 | @@ -0,0 +1,5 @@
   1 | +RPC

jonatack commented at 6:42 PM on April 13, 2023:

Per doc/release-notes-empty-template.md would suggest

Updated RPCs
------------

in doc/release-notes/release-notes-27216.md:5 in b95e2ee982 outdated

   0 | @@ -0,0 +1,5 @@
   1 | +RPC
   2 | +---
   3 | +
   4 | +- `getaddressinfo` returns new boolean field `"isactive"` which is `true` if the
   5 | +address is derived from an active descriptor or HD seed.

jonatack commented at 6:43 PM on April 13, 2023:

address is derived from an active descriptor or HD seed. (#27216)

in doc/release-notes/release-notes-27216.md:4 in b95e2ee982 outdated

   0 | @@ -0,0 +1,5 @@
   1 | +RPC
   2 | +---
   3 | +
   4 | +- `getaddressinfo` returns new boolean field `"isactive"` which is `true` if the

jonatack commented at 6:49 PM on April 13, 2023:

Suggestion

- RPC `getaddressinfo` now returns a new boolean field `"isactive"`, which is `true` if the

For the release note (and/or in the RPC help), it may be good to briefly explain why this field is added, i.e. maybe something roughly like: "Keys become non-active if derived before the wallet was encrypted or imported using RPCs importprivkey, importaddress, or importpubkey. The new field is intended to help users avoid using such keys to receive new transactions, as they can be exposed or compromised."
A release note can take time to do well and is often a separate commit (up to you).

in src/wallet/rpc/addresses.cpp:510 in b95e2ee982 outdated

 506 |                          {RPCResult::Type::BOOL, "solvable", "If we know how to spend coins sent to this address, ignoring the possible lack of private keys."},
 507 |                          {RPCResult::Type::STR, "desc", /*optional=*/true, "A descriptor for spending coins sent to this address (only when solvable)."},
 508 |                          {RPCResult::Type::STR, "parent_desc", /*optional=*/true, "The descriptor used to derive this address if this is a descriptor wallet"},
 509 |                          {RPCResult::Type::BOOL, "isscript", "If the key is a script."},
 510 | -                        {RPCResult::Type::BOOL, "ischange", "If the address was used for change output."},
 511 | +                        {RPCResult::Type::BOOL, "ischange", "If the address is reserved for use as a change output. It may or may not have already been used as change."},

jonatack commented at 6:56 PM on April 13, 2023:

Unsure, but seems like this doc change could be a separate commit if it is an unrelated fix/improvement.

in test/functional/wallet_keypool.py:48 in b95e2ee982 outdated

  43 | +            addrs = nodes[0].deriveaddresses(desc, [0, 9])
  44 | +        else:
  45 | +            list_descriptors = nodes[0].listdescriptors()
  46 | +            for desc in list_descriptors["descriptors"]:
  47 | +                if desc['active'] and not desc["internal"] and desc["desc"][:4] == "wpkh":
  48 | +                    addrs = nodes[0].deriveaddresses(desc["desc"], [0, 9])

jonatack commented at 10:31 PM on April 13, 2023:

nit, prefer named args when multiple

-            addrs = nodes[0].deriveaddresses(desc, [0, 9])
+            addrs = nodes[0].deriveaddresses(descriptor=desc, range=[0, 9])
         else:
             list_descriptors = nodes[0].listdescriptors()
             for desc in list_descriptors["descriptors"]:
                 if desc['active'] and not desc["internal"] and desc["desc"][:4] == "wpkh":
-                    addrs = nodes[0].deriveaddresses(desc["desc"], [0, 9])
+                    addrs = nodes[0].deriveaddresses(descriptor=desc["desc"], range=[0, 9])

in test/functional/wallet_keypool.py:78 in b95e2ee982 outdated

  73 | +            nodes[0].importdescriptors([{
  74 | +                "desc": "addr(bcrt1q95gp4zeaah3qcerh35yhw02qeptlzasdtst55v)",
  75 | +                "timestamp": "now"
  76 | +            }])
  77 | +        else:
  78 | +            nodes[0].importaddress("bcrt1q95gp4zeaah3qcerh35yhw02qeptlzasdtst55v", "label", False)

jonatack commented at 10:34 PM on April 13, 2023:

            nodes[0].importaddress("bcrt1q95gp4zeaah3qcerh35yhw02qeptlzasdtst55v", "label", rescan=False)

in test/functional/wallet_keypool.py:90 in b95e2ee982 outdated

  85 | +            nodes[0].importdescriptors([{
  86 | +                "desc": "pk(02f893ca95b0d55b4ce4e72ae94982eb679158cb2ebc120ff62c17fedfd1f0700e)",
  87 | +                "timestamp": "now"
  88 | +            }])
  89 | +        else:
  90 | +            nodes[0].importpubkey("02f893ca95b0d55b4ce4e72ae94982eb679158cb2ebc120ff62c17fedfd1f0700e", "label", False)

jonatack commented at 10:34 PM on April 13, 2023:

            nodes[0].importpubkey("02f893ca95b0d55b4ce4e72ae94982eb679158cb2ebc120ff62c17fedfd1f0700e", "label", rescan=False)

in test/functional/wallet_keypool.py:96 in b95e2ee982 outdated

  91 | +        import_pub_data = nodes[0].getaddressinfo("bcrt1q4v7a8wn5vqd6fk4026s5gzzxyu7cfzz23n576h")
  92 | +        assert not import_pub_data["ismine"]
  93 | +        assert import_pub_data["iswatchonly"] is not self.options.descriptors
  94 | +        assert not import_pub_data["isactive"]
  95 | +
  96 | +        nodes[0].importprivkey("cPMX7v5CNV1zCphFSq2hnR5rCjzAhA1GsBfD1qrJGdj4QEfu38Qx", "label", False)

jonatack commented at 10:34 PM on April 13, 2023:

        nodes[0].importprivkey("cPMX7v5CNV1zCphFSq2hnR5rCjzAhA1GsBfD1qrJGdj4QEfu38Qx", "label", rescan=False)

in test/functional/wallet_keypool.py:202 in b95e2ee982 outdated

 211 |  
 212 | -        # refill keypool with three new addresses
 213 | +        # refill keypool
 214 |          nodes[0].walletpassphrase('test', 1)
 215 | -        nodes[0].keypoolrefill(3)
 216 | +        nodes[0].keypoolrefill(50)

jonatack commented at 10:40 PM on April 13, 2023:

It may be nice to add a comment on why the choice of 50 here.

in src/wallet/scriptpubkeyman.cpp:395 in b95e2ee982 outdated

 391 | @@ -392,6 +392,23 @@ std::vector<WalletDestination> LegacyScriptPubKeyMan::MarkUnusedAddresses(const
 392 |      return result;
 393 |  }
 394 |  
 395 | +[[nodiscard]] bool LegacyScriptPubKeyMan::IsKeyActive(const CScript& script) const

jonatack commented at 10:48 PM on April 13, 2023:

My understanding is that [[nodiscard]] only needs to be declared in the declaration (i.e. in the .h file here), not the definition as well (idem for the other functions where it has been added to the definitions in addition to the declarations).

pinheadmz commented at 5:25 PM on April 14, 2023:

ah yeah "...Appears in a function declaration, enumeration declaration, or class declaration. "

but theres a few leaks in bitcoin...

--> git grep nodiscard *.cpp | wc -l
       8

jonatack commented at 9:57 PM on April 18, 2023:

Interesting, though only 3 of the 8 seem to be extraneous annotations (two in src/base58.cpp and one in src/node/eviction.cpp, and the one in src/script/descriptor.cpp should just have static added).

in src/wallet/scriptpubkeyman.cpp:2228 in b95e2ee982 outdated

2222 | @@ -2206,6 +2223,11 @@ std::vector<WalletDestination> DescriptorScriptPubKeyMan::MarkUnusedAddresses(co
2223 |      return result;
2224 |  }
2225 |  
2226 | +[[nodiscard]] bool DescriptorScriptPubKeyMan::IsKeyActive(const CScript& script) const
2227 | +{
2228 | +    return IsMine(script);

jonatack commented at 10:49 PM on April 13, 2023:

nit, a one-liner like this can just be inlined in the declaration, and omit the definition.

in src/wallet/wallet.h:679 in b95e2ee982 outdated

 672 | @@ -673,6 +673,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
 673 |      using ListAddrBookFunc = std::function<void(const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change)>;
 674 |      void ForEachAddrBookEntry(const ListAddrBookFunc& func) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 675 |  
 676 | +    /**
 677 | +     * Determines if a destination is in the active spkm (not imported and not dumped for a new keypool)
 678 | +     */
 679 | +    bool IsDestinationActive(const CTxDestination& dest) const;

jonatack commented at 10:50 PM on April 13, 2023:

nit, can be nodiscard

jonatack commented at 11:06 PM on April 13, 2023: member

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.

in src/wallet/test/scriptpubkeyman_tests.cpp:194 in b95e2ee982 outdated

 188 | +    for (const CScript& script : scripts4) {
 189 | +        if (!wallet.IsDestinationActive(WitnessV0ScriptHash(script))) {
 190 | +            ++num_script_keys_not_found;
 191 | +        }
 192 | +    }
 193 | +    BOOST_CHECK_EQUAL(num_script_keys_not_found, 16);

jonatack commented at 11:16 PM on April 13, 2023:

Another implementation for fun!

-    int num_script_keys_not_found = 0;
-    for (const CScript& script : scripts4) {
-        if (!wallet.IsDestinationActive(WitnessV0ScriptHash(script))) {
-            ++num_script_keys_not_found;
-        }
-    }
+    int num_script_keys_not_found = std::accumulate(scripts4.cbegin(), scripts4.cend(), 0, [&wallet] (int n, const CScript& s) { return wallet.IsDestinationActive(WitnessV0ScriptHash(s)) ? n : ++n; });

(maybe improveable with std::reduce instead)

pinheadmz referenced this in commit 8139075244 on Apr 14, 2023

pinheadmz force-pushed on Apr 14, 2023

pinheadmz commented at 7:26 PM on April 14, 2023: member

thanks again @jonatack rebase to 8139075244f1f361a14f80920c45d8c4f41d553e

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

jonatack commented at 10:52 PM on April 18, 2023: member

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

Commit 2a5774f45c07f0d000f0cbdf0 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.

in src/wallet/scriptpubkeyman.h:202 in 8139075244 outdated

 197 | @@ -198,6 +198,9 @@ class ScriptPubKeyMan
 198 |       */
 199 |      virtual std::vector<WalletDestination> MarkUnusedAddresses(const CScript& script) { return {}; }
 200 |  
 201 | +    /* Determines if address is derived from active key manager */
 202 | +    [[nodiscard]] virtual bool IsKeyActive(const CScript& script) const { return false; };

jonatack commented at 10:57 PM on April 18, 2023:

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

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

pinheadmz commented at 2:06 PM on April 19, 2023:

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

pinheadmz commented at 3:59 PM on June 2, 2023:

Thanks, both comments addressed with rebase

jonatack commented at 4:00 PM on April 28, 2023: member

https://github.com/bitcoin/bitcoin/commit/81f29f03a607cbfb7162705c1c1618ca7b59640e 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 (review) above, according to #20160 legacy wallet support may be dropped in the next couple releases.

DrahtBot added the label CI failed on May 18, 2023

pinheadmz referenced this in commit fa9b7d9545 on May 18, 2023

pinheadmz force-pushed on May 18, 2023

pinheadmz commented at 6:25 PM on May 18, 2023: member

Needs rebase

done thanks

DrahtBot renamed this:
~~Add wallet method to detect if a key is "active"~~
wallet: Add wallet method to detect if a key is "active"
on May 18, 2023

DrahtBot removed the label CI failed on May 18, 2023

DrahtBot added the label Wallet on May 18, 2023

wallet: implement IsKeyActive() in scriptpubkeyman

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()

f39957f5de

wallet: implement IsDestinationActive() and add to rpc getaddressinfo

This connects SPKM.IsKeyActive() up to the wallet level.

d33f3bdb84

test: cover ScriptPubKeyMan::IsKeyActive() and Wallet::IsDestinationActive() ba65d93a4c

pinheadmz referenced this in commit f7cd46280f on Jun 2, 2023

pinheadmz force-pushed on Jun 2, 2023

luke-jr commented at 8:54 PM on June 22, 2023: member

I don't mind the new field, but it's not correct to say users should be discouraged from using any key they import...

jonatack commented at 9:57 PM on June 22, 2023: member

re-ACK f7cd46280fefa06f9487a8a2927de093af1d421d only changes since my last review are rebase, making the IsKeyActive virtual function declaration pure, and improving a commit message

modulo #27216 (comment)
happy to re-review if any changes are needed for #27216 (comment)

jarolrod commented at 7:25 AM on July 5, 2023: member

Concept ACK

pinheadmz commented at 7:19 PM on July 5, 2023: member

I don't mind the new field, but it's not correct to say users should be discouraged from using any key they import... @luke-jr Fair point, I updated the OP (no code changes needed)

legacy wallet support may be dropped in the next couple releases. @jonatack It may take about that long for this PR to get merged anyway 🤣. So, I can remove the legacy stuff from this PR right now and only descriptor wallet users can enjoy the feature, OR, we can keep isactive in there for now and then remove it along with the rest of the legacy wallet stuff when that is appropriate. Thoughts? Preference?

jonatack commented at 8:59 PM on July 5, 2023: member

@pinheadmz I'd do the simplest thing, or what @achow101 thinks on this and regarding progress on the #20160 timeline.

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

doc: release notes for #27216 85f83339dd

pinheadmz force-pushed on Jul 10, 2023

luke-jr referenced this in commit 9c1936ac89 on Aug 16, 2023

luke-jr referenced this in commit a7333dc919 on Aug 16, 2023

luke-jr referenced this in commit 964ed4b9da on Aug 16, 2023

luke-jr referenced this in commit 3f78578fe7 on Aug 16, 2023

achow101 requested review from S3RK on Sep 20, 2023

achow101 requested review from Sjors on Sep 20, 2023

S3RK commented at 7:59 AM on September 25, 2023: contributor

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 commented at 2:07 PM on September 26, 2023: member

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 commented at 10:21 PM on September 26, 2023: contributor

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 added the label Needs rebase on Oct 5, 2023

DrahtBot commented at 4:52 PM on October 5, 2023: contributor

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

pablomartin4btc commented at 8:25 PM on October 7, 2023: member

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 commented at 2:50 PM on October 12, 2023: member

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".

pinheadmz closed this on Oct 12, 2023

luke-jr referenced this in commit 6009e964dd on Mar 5, 2024

luke-jr referenced this in commit b752585194 on Mar 5, 2024

luke-jr referenced this in commit e2be2f7470 on Mar 5, 2024

luke-jr referenced this in commit 738023dad6 on Mar 5, 2024

luke-jr referenced this in commit a6953c5a8a on Mar 13, 2024

luke-jr referenced this in commit f7d289ec03 on Mar 13, 2024

luke-jr referenced this in commit 7464dc5bd6 on Mar 13, 2024

luke-jr referenced this in commit 621c9903fa on Mar 13, 2024

luke-jr referenced this in commit 0b5e99a074 on Jun 13, 2024

luke-jr referenced this in commit ec8d084b5f on Jun 13, 2024

luke-jr referenced this in commit 3ca4af2a6a on Jun 13, 2024

luke-jr referenced this in commit bb8a78bece on Jun 13, 2024

bitcoin locked this on Oct 11, 2024