Merge 4ee8a58ce7 into merged_master (Bitcoin PR #17373)

This PR associates OutputType::BECH32 to PAK online keys, where before we
were able to directly access keys from the keypool. In a future refactoring
we should give PAK keys their own output type (and own scriptpubkey manager)
so that the wallet won't accept payments "to the PAK key".

Also changes `ReserveDestination::SetBlindingPubKey` to use a visitor pattern
to apply a blinding pubkey to a destination directly, rather than using the
old hacky method of regenerating the destination by pulling its key (which
is no longer contained in the class) out and giving it to a new constructor.
This was a long-overdue refactoring and the minimal-diff way to get the code
compiling (and it's not bad, maybe 10LOC to add a new visitor class) but
nonetheless I apologize for sticking this into a merge commit.

src/outputtype.cpp

@@ -64,27 +64,6 @@ CTxDestination GetDestinationForKey(const CPubKey& key, OutputType type)
     }
 }
 
-// Elements
-CTxDestination GetDestinationForKey(const CPubKey& key, OutputType type, const CPubKey& blinding_pubkey)
-{
-    switch (type) {
-    case OutputType::LEGACY: return PKHash(key, blinding_pubkey);
-    case OutputType::P2SH_SEGWIT:
-    case OutputType::BECH32: {
-        if (!key.IsCompressed()) return PKHash(key, blinding_pubkey);
-        CTxDestination witdest = WitnessV0KeyHash(PKHash(key), blinding_pubkey);
-        CScript witprog = GetScriptForDestination(witdest);
-        if (type == OutputType::P2SH_SEGWIT) {
-            return ScriptHash(witprog, blinding_pubkey);
-        } else {
-            return witdest;
-        }
-    }
-    default: assert(false);
-    }
-}
-//
-
 std::vector<CTxDestination> GetAllDestinationsForKey(const CPubKey& key)
 {
     PKHash keyid(key);

src/outputtype.h

@@ -35,7 +35,6 @@ const std::string& FormatOutputType(OutputType type);
  * The caller must make sure LearnRelatedScripts has been called beforehand.
  */
 CTxDestination GetDestinationForKey(const CPubKey& key, OutputType);
-CTxDestination GetDestinationForKey(const CPubKey& key, OutputType, const CPubKey& blinding_pubkey);
 
 /** Get all destinations (potentially) supported by the wallet for the given key. */
 std::vector<CTxDestination> GetAllDestinationsForKey(const CPubKey& key);

src/script/standard.h

@@ -156,6 +156,22 @@ public:
     }
 };
 
+// ELEMENTS
+class SetBlindingPubKeyVisitor : public boost::static_visitor<>
+{
+public:
+    const CPubKey& blinding_pubkey;
+
+    explicit SetBlindingPubKeyVisitor(const CPubKey& blinding_pubkey_) : blinding_pubkey(blinding_pubkey_) {}
+
+    void operator()(CNoDestination &dest) const { }
+    void operator()(PKHash &dest) const { dest.blinding_pubkey = blinding_pubkey; }
+    void operator()(ScriptHash &dest) const { dest.blinding_pubkey = blinding_pubkey; }
+    void operator()(WitnessV0KeyHash &dest) const { dest.blinding_pubkey = blinding_pubkey; }
+    void operator()(WitnessV0ScriptHash &dest) const { dest.blinding_pubkey = blinding_pubkey; }
+    void operator()(WitnessUnknown &dest) const { dest.blinding_pubkey = blinding_pubkey; }
+    void operator()(NullData &dest) const { }
+};
 /**
  * A txout script template with a specific destination. It is either:
  *  * CNoDestination: no destination set

src/wallet/scriptpubkeyman.cpp

@@ -18,7 +18,7 @@ bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestinat
 
     // Generate a new key that is added to wallet
     CPubKey new_key;
-    if (!GetKeyFromPool(new_key)) {
+    if (!GetKeyFromPool(new_key, type)) {
         error = "Error: Keypool ran out, please call keypoolrefill first";
         return false;
     }
@@ -26,7 +26,7 @@ bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestinat
     dest = GetDestinationForKey(new_key, type);
     if (add_blinding_key) {
         CPubKey blinding_pubkey = GetBlindingPubKey(GetScriptForDestination(dest));
-        dest = GetDestinationForKey(new_key, type, blinding_pubkey);
+        boost::apply_visitor(SetBlindingPubKeyVisitor(blinding_pubkey), dest);
     }
 
     return true;
@@ -273,24 +273,19 @@ bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn)
     return true;
 }
 
-bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool)
+bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool)
 {
+    if (!CanGetAddresses(internal)) {
+        return false;
+    }
+
     if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
         return false;
     }
+    address = GetDestinationForKey(keypool.vchPubKey, type);
     return true;
 }
 
-void LegacyScriptPubKeyMan::KeepDestination(int64_t index)
-{
-    KeepKey(index);
-}
-
-void LegacyScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey)
-{
-    ReturnKey(index, internal, pubkey);
-}
-
 void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
 {
     AssertLockHeld(cs_wallet);
@@ -471,7 +466,7 @@ size_t LegacyScriptPubKeyMan::KeypoolCountExternalKeys()
 unsigned int LegacyScriptPubKeyMan::GetKeyPoolSize() const
 {
     AssertLockHeld(cs_wallet);
-    return setInternalKeyPool.size() + setExternalKeyPool.size();
+    return setInternalKeyPool.size() + setExternalKeyPool.size() + set_pre_split_keypool.size();
 }
 
 int64_t LegacyScriptPubKeyMan::GetTimeFirstKey() const
@@ -1103,15 +1098,20 @@ void LegacyScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const
     m_pool_key_to_index[pubkey.GetID()] = index;
 }
 
-void LegacyScriptPubKeyMan::KeepKey(int64_t nIndex)
+void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex, const OutputType& type)
 {
     // Remove from key pool
     WalletBatch batch(m_storage.GetDatabase());
     batch.ErasePool(nIndex);
+    CPubKey pubkey;
+    bool have_pk = GetPubKey(m_index_to_reserved_key.at(nIndex), pubkey);
+    assert(have_pk);
+    LearnRelatedScripts(pubkey, type);
+    m_index_to_reserved_key.erase(nIndex);
     WalletLogPrintf("keypool keep %d\n", nIndex);
 }
 
-void LegacyScriptPubKeyMan::ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey)
+void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, const CTxDestination&)
 {
     // Return to key pool
     {
@@ -1123,13 +1123,15 @@ void LegacyScriptPubKeyMan::ReturnKey(int64_t nIndex, bool fInternal, const CPub
         } else {
             setExternalKeyPool.insert(nIndex);
         }
-        m_pool_key_to_index[pubkey.GetID()] = nIndex;
+        CKeyID& pubkey_id = m_index_to_reserved_key.at(nIndex);
+        m_pool_key_to_index[pubkey_id] = nIndex;
+        m_index_to_reserved_key.erase(nIndex);
         NotifyCanGetAddressesChanged();
     }
     WalletLogPrintf("keypool return %d\n", nIndex);
 }
 
-bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, bool internal)
+bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType type, bool internal)
 {
     if (!CanGetAddresses(internal)) {
         return false;
@@ -1145,7 +1147,7 @@ bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, bool internal)
             result = GenerateNewKey(batch, internal);
             return true;
         }
-        KeepKey(nIndex);
+        KeepDestination(nIndex, type);
         result = keypool.vchPubKey;
     }
     return true;
@@ -1190,6 +1192,8 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key
             throw std::runtime_error(std::string(__func__) + ": keypool entry invalid");
         }
 
+        assert(m_index_to_reserved_key.count(nIndex) == 0);
+        m_index_to_reserved_key[nIndex] = keypool.vchPubKey.GetID();
         m_pool_key_to_index.erase(keypool.vchPubKey.GetID());
         WalletLogPrintf("keypool reserve %d\n", nIndex);
     }

src/wallet/scriptpubkeyman.h

@@ -150,9 +150,9 @@ public:
     virtual bool GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error, bool add_blinding_key) { return false; }
     virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; }
 
-    virtual bool GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) { return false; }
-    virtual void KeepDestination(int64_t index) {}
-    virtual void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) {}
+    virtual bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) { return false; }
+    virtual void KeepDestination(int64_t index, const OutputType& type) {}
+    virtual void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) {}
 
     virtual bool TopUp(unsigned int size = 0) { return false; }
 
@@ -246,9 +246,11 @@ private:
     std::set<int64_t> set_pre_split_keypool GUARDED_BY(cs_wallet);
     int64_t m_max_keypool_index GUARDED_BY(cs_wallet) = 0;
     std::map<CKeyID, int64_t> m_pool_key_to_index;
+    // Tracks keypool indexes to CKeyIDs of keys that have been taken out of the keypool but may be returned to it
+    std::map<int64_t, CKeyID> m_index_to_reserved_key;
 
     //! Fetches a key from the keypool
-    bool GetKeyFromPool(CPubKey &key, bool internal = false);
+    bool GetKeyFromPool(CPubKey &key, const OutputType type, bool internal = false);
 
     /**
      * Reserves a key from the keypool and sets nIndex to its index
@@ -266,9 +268,6 @@ private:
      */
     bool ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal);
 
-    void KeepKey(int64_t nIndex);
-    void ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey);
-
 public:
     bool GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error, bool add_blinding_key) override;
     isminetype IsMine(const CScript& script) const override;
@@ -276,9 +275,9 @@ public:
     //! will encrypt previously unencrypted keys
     bool EncryptKeys(CKeyingMaterial& vMasterKeyIn);
 
-    bool GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) override;
-    void KeepDestination(int64_t index) override;
-    void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) override;
+    bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) override;
+    void KeepDestination(int64_t index, const OutputType& type) override;
+    void ReturnDestination(int64_t index, bool internal, const CTxDestination&) override;
 
     bool TopUp(unsigned int size = 0) override;
 

src/wallet/wallet.cpp

@@ -3802,7 +3802,7 @@ bool CWallet::GetOnlinePakKey(CPubKey& online_pubkey, std::string& error)
     error.clear();
     auto spk_man = m_spk_man.get();
     if (spk_man) {
-        if (!spk_man->GetKeyFromPool(online_pubkey)) {
+        if (!spk_man->GetKeyFromPool(online_pubkey, OutputType::BECH32)) {
             error = "Error: Keypool ran out, please call keypoolrefill first";
             return false;
         }
@@ -4016,49 +4016,40 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool inter
         return false;
     }
 
-    if (!pwallet->CanGetAddresses(internal)) {
-        return false;
-    }
 
     if (nIndex == -1)
     {
         CKeyPool keypool;
-        if (!m_spk_man->GetReservedDestination(type, internal, nIndex, keypool)) {
+        if (!m_spk_man->GetReservedDestination(type, internal, address, nIndex, keypool)) {
             return false;
         }
-        vchPubKey = keypool.vchPubKey;
         fInternal = keypool.fInternal;
     }
-    assert(vchPubKey.IsValid());
-    address = GetDestinationForKey(vchPubKey, type);
     dest = address;
     return true;
 }
 
 void ReserveDestination::SetBlindingPubKey(const CPubKey& blinding_pubkey, CTxDestination& dest)
 {
-    address = GetDestinationForKey(vchPubKey, type, blinding_pubkey);
+    boost::apply_visitor(SetBlindingPubKeyVisitor(blinding_pubkey), address);
     dest = address;
 }
 
 void ReserveDestination::KeepDestination()
 {
     if (nIndex != -1) {
-        m_spk_man->KeepDestination(nIndex);
-        m_spk_man->LearnRelatedScripts(vchPubKey, type);
+        m_spk_man->KeepDestination(nIndex, type);
     }
     nIndex = -1;
-    vchPubKey = CPubKey();
     address = CNoDestination();
 }
 
 void ReserveDestination::ReturnDestination()
 {
     if (nIndex != -1) {
-        m_spk_man->ReturnDestination(nIndex, fInternal, vchPubKey);
+        m_spk_man->ReturnDestination(nIndex, fInternal, address);
     }
     nIndex = -1;
-    vchPubKey = CPubKey();
     address = CNoDestination();
 }
 

src/wallet/wallet.h

@@ -140,12 +140,11 @@ class ReserveDestination
 protected:
     //! The wallet to reserve from
     CWallet* const pwallet;
-    LegacyScriptPubKeyMan* m_spk_man{nullptr};
+    //! The ScriptPubKeyMan to reserve from. Based on type when GetReservedDestination is called
+    ScriptPubKeyMan* m_spk_man{nullptr};
     OutputType const type;
     //! The index of the address's key in the keypool
     int64_t nIndex{-1};
-    //! The public key for the address
-    CPubKey vchPubKey;
     //! The destination
     CTxDestination address;
     //! Whether this is from the internal (change output) keypool