Merge 27216 via rpc_getaddressinfo_isactive

src/script/descriptor.cpp

@@ -821,8 +821,11 @@ class PKDescriptor final : public DescriptorImpl
 private:
     const bool m_xonly;
 protected:
-    std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider&) const override
+    std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider& out) const override
     {
+        CKeyID id = keys[0].GetID();
+        out.pubkeys.emplace(id, keys[0]);
+
         if (m_xonly) {
             CScript script = CScript() << ToByteVector(XOnlyPubKey(keys[0])) << OP_CHECKSIG;
             return Vector(std::move(script));

src/wallet/rpc/addresses.cpp

@@ -531,6 +531,7 @@ RPCHelpMan getaddressinfo()
                         {RPCResult::Type::STR, "address", "The bitcoin address validated."},
                         {RPCResult::Type::STR_HEX, "scriptPubKey", "The hex-encoded output script generated by the address."},
                         {RPCResult::Type::BOOL, "ismine", "If the address is yours."},
+                        {RPCResult::Type::BOOL, "isactive", "If the key is in the active keypool (always equal to \"ismine\" in descriptor wallets)."},
                         {RPCResult::Type::BOOL, "iswatchonly", "If the address is watchonly."},
                         {RPCResult::Type::BOOL, "solvable", "If we know how to spend coins sent to this address, ignoring the possible lack of private keys."},
                         {RPCResult::Type::STR, "desc", /*optional=*/true, "A descriptor for spending coins sent to this address (only when solvable)."},
@@ -605,6 +606,7 @@ RPCHelpMan getaddressinfo()
 
     isminetype mine = pwallet->IsMine(dest);
     ret.pushKV("ismine", bool(mine & ISMINE_SPENDABLE));
+    ret.pushKV("isactive", pwallet->IsDestinationActive(dest));
 
     if (provider) {
         auto inferred = InferDescriptor(scriptPubKey, *provider);

src/wallet/scriptpubkeyman.cpp

@@ -443,6 +443,23 @@ std::vector<WalletDestination> LegacyScriptPubKeyMan::MarkUnusedAddresses(const
     return result;
 }
 
+bool LegacyDataSPKM::IsKeyActive(const CScript& script) const
+{
+    LOCK(cs_KeyStore);
+
+    if (!IsMine(script)) return false; // Not in the keystore at all
+
+    for (const auto& key_id : GetAffectedKeys(script, *this)) {
+        const auto it = mapKeyMetadata.find(key_id);
+        if (it == mapKeyMetadata.end()) return false; // This key must be really old
+
+        if (!it->second.hd_seed_id.IsNull() && it->second.hd_seed_id == m_hd_chain.seed_id) return true;
+    }
+
+    // Imported or dumped for a new keypool
+    return false;
+}
+
 void LegacyScriptPubKeyMan::UpgradeKeyMetadata()
 {
     LOCK(cs_KeyStore);

src/wallet/scriptpubkeyman.h

@@ -207,6 +207,9 @@ public:
      */
     virtual std::vector<WalletDestination> MarkUnusedAddresses(const CScript& script) { return {}; }
 
+    /* Determines if address is derived from active key manager */
+    virtual bool IsKeyActive(const CScript& script) const = 0;
+
     /** Sets up the key generation stuff, i.e. generates new HD seeds and sets them as active.
       * Returns false if already setup or setup fails, true if setup is successful
       * Set force=true to make it re-setup if already setup, used for upgrades
@@ -315,6 +318,7 @@ public:
 
     // ScriptPubKeyMan overrides
     bool CheckDecryptionKey(const CKeyingMaterial& master_key) override;
+    [[nodiscard]] bool IsKeyActive(const CScript& script) const override;
     std::unordered_set<CScript, SaltedSipHasher> GetScriptPubKeys() const override;
     std::unique_ptr<SigningProvider> GetSolvingProvider(const CScript& script) const override;
     uint256 GetID() const override { return uint256::ONE; }
@@ -652,6 +656,8 @@ public:
 
     std::vector<WalletDestination> MarkUnusedAddresses(const CScript& script) override;
 
+    [[nodiscard]] bool IsKeyActive(const CScript& script) const override { return IsMine(script); }
+
     bool IsHDEnabled() const override;
 
     //! Setup descriptors based on the given CExtkey

src/wallet/test/scriptpubkeyman_tests.cpp

@@ -40,5 +40,159 @@ BOOST_AUTO_TEST_CASE(CanProvide)
     BOOST_CHECK(keyman.CanProvide(p2sh_script, data));
 }
 
+BOOST_AUTO_TEST_CASE(Legacy_IsKeyActive)
+{
+    CWallet wallet(m_node.chain.get(), "", CreateMockableWalletDatabase());
+    {
+        LOCK(wallet.cs_wallet);
+        wallet.SetMinVersion(FEATURE_LATEST);
+        wallet.m_keypool_size = 10;
+    }
+    LegacyScriptPubKeyMan& spkm = *wallet.GetOrCreateLegacyScriptPubKeyMan();
+
+    // Start off empty
+    BOOST_CHECK(spkm.GetScriptPubKeys().empty());
+
+    // Generate 20 keypool keys (10 internal, 10 external)
+    {
+        LOCK(wallet.cs_wallet);
+        spkm.SetupGeneration();
+    }
+
+    // 4 scripts per keypool key (P2PK, P2PKH, P2WPKH, P2SH-P2WPKH)
+    // Plus 4 scripts for the seed key
+    auto scripts1 = spkm.GetScriptPubKeys();
+    BOOST_CHECK_EQUAL(scripts1.size(), 84);
+
+    // All keys are active
+    for (const CScript& script : scripts1) {
+        BOOST_CHECK(spkm.IsKeyActive(script));
+    }
+
+    // Requesting single from spkm should not deactivate key
+    CTxDestination dest1;
+    {
+        LOCK(wallet.cs_wallet);
+        auto result = spkm.GetNewDestination(OutputType::BECH32);
+        dest1 = result.value();
+    }
+    CScript script = GetScriptForDestination(dest1);
+    BOOST_CHECK(spkm.IsKeyActive(script));
+
+    // Key pool size did not change
+    auto scripts2 = spkm.GetScriptPubKeys();
+    BOOST_CHECK_EQUAL(scripts2.size(), 84);
+
+    // Use key that is not the next key
+    // (i.e. address gap in wallet recovery)
+    {
+        LOCK(wallet.cs_wallet);
+        LOCK(spkm.cs_KeyStore);
+        auto keys = spkm.MarkReserveKeysAsUsed(5);
+        BOOST_CHECK_EQUAL(keys.size(), 4); // Because we already used one with GetNewDestination
+    }
+
+    // Key pool size did not change
+    auto scripts3 = spkm.GetScriptPubKeys();
+    BOOST_CHECK_EQUAL(scripts3.size(), 84);
+
+    // All keys are still active
+    for (const CScript& script : scripts3) {
+        BOOST_CHECK(spkm.IsKeyActive(script));
+    }
+
+    // When user encrypts wallet for the first time,
+    // all existing keys are removed from active keypool
+    {
+        LOCK(wallet.cs_wallet);
+        // called by EncryptWallet()
+        spkm.SetupGeneration(true);
+    }
+
+    // 20 new keys were added
+    auto scripts4 = spkm.GetScriptPubKeys();
+    BOOST_CHECK_EQUAL(scripts4.size(), 84 * 2);
+
+    // All 10 original keys are now inactive
+    for (const CScript& script : scripts3) {
+        BOOST_CHECK(!spkm.IsKeyActive(script));
+    }
+}
+
+BOOST_AUTO_TEST_CASE(Descriptor_IsKeyActive)
+{
+    CWallet wallet(m_node.chain.get(), "", CreateMockableWalletDatabase());
+    {
+        LOCK(wallet.cs_wallet);
+        wallet.LoadMinVersion(FEATURE_LATEST);
+        wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
+        wallet.m_keypool_size = 10;
+        wallet.SetupDescriptorScriptPubKeyMans();
+    }
+    DescriptorScriptPubKeyMan* spkm = dynamic_cast<DescriptorScriptPubKeyMan*>(wallet.GetScriptPubKeyMan(OutputType::BECH32, /*internal=*/false));
+
+    // Start off with 10 pre-generated keys, 1 script each
+    auto scripts1 = spkm->GetScriptPubKeys();
+    BOOST_CHECK_EQUAL(scripts1.size(), 10);
+
+    // All keys are active
+    for (const CScript& script : scripts1) {
+        BOOST_CHECK(spkm->IsKeyActive(script));
+    }
+
+    // Requesting single key from spkm should not deactivate key
+    auto dest1 = spkm->GetNewDestination(OutputType::BECH32);
+    CScript script = GetScriptForDestination(dest1.value());
+    BOOST_CHECK(spkm->IsKeyActive(script));
+
+    // Key pool size did not change
+    auto scripts2 = spkm->GetScriptPubKeys();
+    BOOST_CHECK_EQUAL(scripts2.size(), 10);
+
+    // Use key that is not the next key
+    // (i.e. address gap in wallet recovery)
+    {
+        LOCK(spkm->cs_desc_man);
+        WalletDescriptor descriptor = spkm->GetWalletDescriptor();
+        FlatSigningProvider provider;
+        std::vector<CScript> scripts3;
+        descriptor.descriptor->ExpandFromCache(/*pos=*/5, descriptor.cache, scripts3, provider);
+
+        BOOST_CHECK_EQUAL(scripts3.size(), 1);
+        spkm->MarkUnusedAddresses(scripts3.front());
+    }
+
+    // Key pool size increased to replace used keys
+    auto scripts4 = spkm->GetScriptPubKeys();
+    BOOST_CHECK_EQUAL(scripts4.size(), 16);
+
+    // All keys are still active
+    for (const CScript& script : scripts4) {
+        BOOST_CHECK(spkm->IsKeyActive(script));
+    }
+
+    // When user encrypts wallet for the first time,
+    // all existing keys are removed from active keypool
+    {
+        LOCK(wallet.cs_wallet);
+        // called by EncryptWallet()
+        wallet.SetupDescriptorScriptPubKeyMans();
+    }
+
+    // This SPKM is not affected
+    for (const CScript& script : scripts4) {
+        BOOST_CHECK(spkm->IsKeyActive(script));
+    }
+
+    // ...but at the wallet level all the keys from that SPKM are deactivated
+    int num_script_keys_not_found = 0;
+    for (const CScript& script : scripts4) {
+        if (!wallet.IsDestinationActive(WitnessV0ScriptHash(script))) {
+            ++num_script_keys_not_found;
+        }
+    }
+    BOOST_CHECK_EQUAL(num_script_keys_not_found, 16);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 } // namespace wallet

src/wallet/wallet.cpp

@@ -2701,6 +2701,13 @@ void CWallet::ForEachAddrBookEntry(const ListAddrBookFunc& func) const
     }
 }
 
+bool CWallet::IsDestinationActive(const CTxDestination& dest) const
+{
+    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); });
+}
+
 std::vector<CTxDestination> CWallet::ListAddrBookAddresses(const std::optional<AddrBookFilter>& _filter) const
 {
     AssertLockHeld(cs_wallet);

src/wallet/wallet.h

@@ -786,6 +786,11 @@ public:
     using ListAddrBookFunc = std::function<void(const CTxDestination& dest, const std::string& label, bool is_change, const std::optional<AddressPurpose> purpose)>;
     void ForEachAddrBookEntry(const ListAddrBookFunc& func) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
+    /**
+     * Determines if a destination is in the active spkm (not imported and not dumped for a new keypool)
+     */
+    [[nodiscard]] bool IsDestinationActive(const CTxDestination& dest) const;
+
     /**
      * Marks all outputs in each one of the destinations dirty, so their cache is
      * reset and does not return outdated information.

test/functional/wallet_keypool.py

@@ -4,33 +4,113 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test the wallet keypool and interaction with wallet encryption/locking."""
 
+import re
 import time
 from decimal import Decimal
 
+from test_framework.descriptors import descsum_create
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal, assert_raises_rpc_error
 from test_framework.wallet_util import WalletUnlock
 
+TEST_KEYPOOL_SIZE = 10
+TEST_NEW_KEYPOOL_SIZE = TEST_KEYPOOL_SIZE + 2
+
 class KeyPoolTest(BitcoinTestFramework):
     def add_options(self, parser):
         self.add_wallet_options(parser)
 
     def set_test_params(self):
         self.num_nodes = 1
+        self.extra_args = [[f"-keypool={TEST_KEYPOOL_SIZE}"]]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
     def run_test(self):
         nodes = self.nodes
+
+        # Derive addresses from the wallet without removing them from keypool
+        addrs = []
+        if not self.options.descriptors:
+            path = str(self.nodes[0].datadir_path / 'wallet.dump')
+            nodes[0].dumpwallet(path)
+            file = open(path, "r", encoding="utf8")
+            m = re.search(r"masterkey: (\w+)", file.read())
+            file.close()
+            xpriv = m.group(1)
+            desc = descsum_create(f"wpkh({xpriv}/0h/0h/*h)")
+            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(descriptor=desc["desc"], range=[0, 9])
+
+        addr0 = addrs[0]
+        addr9 = addrs[9] # arbitrary future address index
+
+        # Address is mine and active before it is removed from keypool by getnewaddress
+        addr0_before_getting_data = nodes[0].getaddressinfo(addr0)
+        assert addr0_before_getting_data['ismine']
+        assert addr0_before_getting_data['isactive']
+
         addr_before_encrypting = nodes[0].getnewaddress()
         addr_before_encrypting_data = nodes[0].getaddressinfo(addr_before_encrypting)
+        assert addr0 == addr_before_encrypting
+        # Address is still mine and active even after being removed from keypool
+        assert addr_before_encrypting_data['ismine']
+        assert addr_before_encrypting_data['isactive']
+
         wallet_info_old = nodes[0].getwalletinfo()
         if not self.options.descriptors:
             assert addr_before_encrypting_data['hdseedid'] == wallet_info_old['hdseedid']
 
+        # Address is mine and active before wallet is encrypted (resetting keypool)
+        addr9_before_encrypting_data = nodes[0].getaddressinfo(addr9)
+        assert addr9_before_encrypting_data['ismine']
+        assert addr9_before_encrypting_data['isactive']
+
+        # Imported things are never considered active, no need to rescan
+        # Imported public keys / addresses can't be mine because they are not spendable
+        if self.options.descriptors:
+            nodes[0].importdescriptors([{
+                "desc": "addr(bcrt1q95gp4zeaah3qcerh35yhw02qeptlzasdtst55v)",
+                "timestamp": "now"
+            }])
+        else:
+            nodes[0].importaddress("bcrt1q95gp4zeaah3qcerh35yhw02qeptlzasdtst55v", "label", rescan=False)
+        import_addr_data = nodes[0].getaddressinfo("bcrt1q95gp4zeaah3qcerh35yhw02qeptlzasdtst55v")
+        assert import_addr_data["iswatchonly"] is not self.options.descriptors
+        assert not import_addr_data["ismine"]
+        assert not import_addr_data["isactive"]
+
+        if self.options.descriptors:
+            nodes[0].importdescriptors([{
+                "desc": "pk(02f893ca95b0d55b4ce4e72ae94982eb679158cb2ebc120ff62c17fedfd1f0700e)",
+                "timestamp": "now"
+            }])
+        else:
+            nodes[0].importpubkey("02f893ca95b0d55b4ce4e72ae94982eb679158cb2ebc120ff62c17fedfd1f0700e", "label", rescan=False)
+        import_pub_data = nodes[0].getaddressinfo("bcrt1q4v7a8wn5vqd6fk4026s5gzzxyu7cfzz23n576h")
+        assert import_pub_data["iswatchonly"] is not self.options.descriptors
+        assert not import_pub_data["ismine"]
+        assert not import_pub_data["isactive"]
+
+        nodes[0].importprivkey("cPMX7v5CNV1zCphFSq2hnR5rCjzAhA1GsBfD1qrJGdj4QEfu38Qx", "label", rescan=False)
+        import_priv_data = nodes[0].getaddressinfo("bcrt1qa985v5d53qqtrfujmzq2zrw3r40j6zz4ns02kj")
+        assert not import_priv_data["iswatchonly"]
+        assert import_priv_data["ismine"]
+        assert not import_priv_data["isactive"]
+
         # Encrypt wallet and wait to terminate
         nodes[0].encryptwallet('test')
+        addr9_after_encrypting_data = nodes[0].getaddressinfo(addr9)
+        # Key is from unencrypted seed, no longer considered active
+        assert not addr9_after_encrypting_data['isactive']
+        # ...however it *IS* still mine since we can spend with this key
+        assert addr9_after_encrypting_data['ismine']
+
         if self.options.descriptors:
             # Import hardened derivation only descriptors
             nodes[0].walletpassphrase('test', 10)
@@ -76,7 +156,9 @@ class KeyPoolTest(BitcoinTestFramework):
                 }
             ])
             nodes[0].walletlock()
-        # Keep creating keys
+        # Keep creating keys until we run out
+        for _ in range(TEST_KEYPOOL_SIZE - 1):
+            nodes[0].getnewaddress()
         addr = nodes[0].getnewaddress()
         addr_data = nodes[0].getaddressinfo(addr)
         wallet_info = nodes[0].getwalletinfo()
@@ -85,24 +167,23 @@ class KeyPoolTest(BitcoinTestFramework):
             assert addr_data['hdseedid'] == wallet_info['hdseedid']
         assert_raises_rpc_error(-12, "Error: Keypool ran out, please call keypoolrefill first", nodes[0].getnewaddress)
 
-        # put six (plus 2) new keys in the keypool (100% external-, +100% internal-keys, 1 in min)
+        # put two new keys in the keypool
         with WalletUnlock(nodes[0], 'test'):
-            nodes[0].keypoolrefill(6)
+            nodes[0].keypoolrefill(TEST_NEW_KEYPOOL_SIZE)
         wi = nodes[0].getwalletinfo()
         if self.options.descriptors:
-            assert_equal(wi['keypoolsize_hd_internal'], 24)
+            # Descriptors wallet: keypool size applies to both internal and external
-            assert_equal(wi['keypoolsize'], 24)
+            # chains and there are four of each (legacy, nested, segwit, and taproot)
+            assert_equal(wi['keypoolsize_hd_internal'], TEST_NEW_KEYPOOL_SIZE * 4)
+            assert_equal(wi['keypoolsize'], TEST_NEW_KEYPOOL_SIZE * 4)
         else:
-            assert_equal(wi['keypoolsize_hd_internal'], 6)
+            # Legacy wallet: keypool size applies to both internal and external HD chains
-            assert_equal(wi['keypoolsize'], 6)
+            assert_equal(wi['keypoolsize_hd_internal'], TEST_NEW_KEYPOOL_SIZE)
+            assert_equal(wi['keypoolsize'], TEST_NEW_KEYPOOL_SIZE)
 
         # drain the internal keys
-        nodes[0].getrawchangeaddress()
+        for _ in range(TEST_NEW_KEYPOOL_SIZE):
-        nodes[0].getrawchangeaddress()
+            nodes[0].getrawchangeaddress()
-        nodes[0].getrawchangeaddress()
-        nodes[0].getrawchangeaddress()
-        nodes[0].getrawchangeaddress()
-        nodes[0].getrawchangeaddress()
         # remember keypool sizes
         wi = nodes[0].getwalletinfo()
         kp_size_before = [wi['keypoolsize_hd_internal'], wi['keypoolsize']]
@@ -115,13 +196,8 @@ class KeyPoolTest(BitcoinTestFramework):
 
         # drain the external keys
         addr = set()
-        addr.add(nodes[0].getnewaddress(address_type="bech32"))
+        for _ in range(TEST_NEW_KEYPOOL_SIZE):
-        addr.add(nodes[0].getnewaddress(address_type="bech32"))
+            addr.add(nodes[0].getnewaddress(address_type="bech32"))
-        addr.add(nodes[0].getnewaddress(address_type="bech32"))
-        addr.add(nodes[0].getnewaddress(address_type="bech32"))
-        addr.add(nodes[0].getnewaddress(address_type="bech32"))
-        addr.add(nodes[0].getnewaddress(address_type="bech32"))
-        assert len(addr) == 6
         # remember keypool sizes
         wi = nodes[0].getwalletinfo()
         kp_size_before = [wi['keypoolsize_hd_internal'], wi['keypoolsize']]
@@ -132,16 +208,18 @@ class KeyPoolTest(BitcoinTestFramework):
         kp_size_after = [wi['keypoolsize_hd_internal'], wi['keypoolsize']]
         assert_equal(kp_size_before, kp_size_after)
 
-        # refill keypool with three new addresses
+        # refill keypool
         nodes[0].walletpassphrase('test', 1)
-        nodes[0].keypoolrefill(3)
+        # At this point the keypool has >45 keys in it
+        # calling keypoolrefill with anything smaller than that is a noop
+        nodes[0].keypoolrefill(50)
 
         # test walletpassphrase timeout
         time.sleep(1.1)
         assert_equal(nodes[0].getwalletinfo()["unlocked_until"], 0)
 
         # drain the keypool
-        for _ in range(3):
+        for _ in range(50):
             nodes[0].getnewaddress()
         assert_raises_rpc_error(-12, "Keypool ran out", nodes[0].getnewaddress)