merge bitcoin#25656: return util::Result from GetReservedDestination methods

Author: kwvg

src/coinjoin/client.cpp

@@ -1575,12 +1575,10 @@ bool CCoinJoinClientSession::CreateCollateralTransaction(CMutableTransaction& tx
     if (txout.nValue >= CoinJoin::GetCollateralAmount() * 2) {
         // make our change address
         CScript scriptChange;
-        CTxDestination dest;
-        bilingual_str error;
         ReserveDestination reserveDest(m_wallet.get());
-        bool success = reserveDest.GetReservedDestination(dest, true, error);
-        assert(success); // should never fail, as we just unlocked
-        scriptChange = GetScriptForDestination(dest);
+        auto dest_opt = reserveDest.GetReservedDestination(true);
+        assert(dest_opt); // should never fail, as we just unlocked
+        scriptChange = GetScriptForDestination(*dest_opt);
         reserveDest.KeepDestination();
         // return change
         txCollateral.vout.emplace_back(txout.nValue - CoinJoin::GetCollateralAmount(), scriptChange);

src/coinjoin/util.cpp

@@ -29,8 +29,9 @@ inline unsigned int GetSizeOfCompactSizeDiff(uint64_t nSizePrev, uint64_t nSizeN
 CKeyHolder::CKeyHolder(CWallet* pwallet) :
     reserveDestination(pwallet)
 {
-    bilingual_str error;
-    reserveDestination.GetReservedDestination(dest, false, error);
+    auto dest_opt = reserveDestination.GetReservedDestination(false);
+    assert(dest_opt);
+    dest = *dest_opt;
 }
 
 void CKeyHolder::KeepKey()
@@ -100,11 +101,10 @@ CTransactionBuilderOutput::CTransactionBuilderOutput(CTransactionBuilder* pTxBui
     nAmount(nAmountIn)
 {
     assert(pTxBuilder);
-    CTxDestination txdest;
-    bilingual_str error;
     LOCK(wallet.cs_wallet);
-    dest.GetReservedDestination(txdest, false, error);
-    script = ::GetScriptForDestination(txdest);
+    auto dest_opt = dest.GetReservedDestination(false);
+    assert(dest_opt);
+    script = ::GetScriptForDestination(*dest_opt);
 }
 
 bool CTransactionBuilderOutput::UpdateAmount(const CAmount nNewAmount)

src/wallet/scriptpubkeyman.cpp

@@ -288,21 +288,18 @@ bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBat
     return true;
 }
 
-bool LegacyScriptPubKeyMan::GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, bilingual_str& error)
+util::Result<CTxDestination> LegacyScriptPubKeyMan::GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool)
 {
     LOCK(cs_KeyStore);
     if (!CanGetAddresses(internal)) {
-        error = _("Error: Keypool ran out, please call keypoolrefill first");
-        return false;
+        return util::Error{_("Error: Keypool ran out, please call keypoolrefill first")};
     }
 
     if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
-        error = _("Error: Keypool ran out, please call keypoolrefill first");
-        return false;
+        return util::Error{_("Error: Keypool ran out, please call keypoolrefill first")};
     }
     // TODO: unify with bitcoin and use here GetDestinationForKey even if we have no type
-    address = PKHash(keypool.vchPubKey);
-    return true;
+    return CTxDestination(PKHash(keypool.vchPubKey));
 }
 
 void LegacyScriptPubKeyMan::MarkUnusedAddresses(WalletBatch &batch, const CScript& script, const std::optional<int64_t>& block_time)
@@ -1905,17 +1902,12 @@ bool DescriptorScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, Walle
     return true;
 }
 
-bool DescriptorScriptPubKeyMan::GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, bilingual_str& error)
+util::Result<CTxDestination> DescriptorScriptPubKeyMan::GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool)
 {
     LOCK(cs_desc_man);
     auto op_dest = GetNewDestination();
     index = m_wallet_descriptor.next_index - 1;
-    if (op_dest) {
-        address = *op_dest;
-    } else {
-        error = util::ErrorString(op_dest);
-    }
-    return bool(op_dest);
+    return op_dest;
 }
 
 void DescriptorScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CTxDestination& addr)

src/wallet/scriptpubkeyman.h

@@ -171,7 +171,7 @@ class ScriptPubKeyMan
     virtual bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) { return false; }
     virtual bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) { return false; }
 
-    virtual bool GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, bilingual_str& error) { return false; }
+    virtual util::Result<CTxDestination> GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) { return util::Error{Untranslated("Not supported")}; }
     virtual void KeepDestination(int64_t index) {}
     virtual void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) {}
 
@@ -325,7 +325,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
     bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override;
     bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override;
 
-    bool GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, bilingual_str& error) override;
+    util::Result<CTxDestination> GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) override;
     void KeepDestination(int64_t index) override;
     void ReturnDestination(int64_t index, bool internal, const CTxDestination&) override;
 
@@ -561,7 +561,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
     bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override;
     bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override;
 
-    bool GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, bilingual_str& error) override;
+    util::Result<CTxDestination> GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) override;
     void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) override;
 
     // Tops up the descriptor cache and m_map_script_pub_keys. The cache is stored in the wallet file

src/wallet/spend.cpp

@@ -811,11 +811,13 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
         // Reserve a new key pair from key pool. If it fails, provide a dummy
         // destination in case we don't need change.
         CTxDestination dest;
-        bilingual_str dest_err;
-        if (!reservedest.GetReservedDestination(dest, true, dest_err)) {
-            error = _("Transaction needs a change address, but we can't generate it.") + Untranslated(" ") + dest_err;
+        auto op_dest = reservedest.GetReservedDestination(true);
+        if (!op_dest) {
+            error = _("Transaction needs a change address, but we can't generate it.") + Untranslated(" ") + util::ErrorString(op_dest);
+        } else {
+            dest = *op_dest;
+            scriptChange = GetScriptForDestination(dest);
         }
-        scriptChange = GetScriptForDestination(dest);
         // A valid destination implies a change script (and
         // vice-versa). An empty change script will abort later, if the
         // change keypool ran out, but change is required.

src/wallet/test/coinjoin_tests.cpp

@@ -183,12 +183,13 @@ class CTransactionBuilderTestSetup : public TestChain100Setup
         CompactTallyItem tallyItem;
         ReserveDestination reserveDest(wallet.get());
         int nChangePosRet{RANDOM_CHANGE_POSITION};
-        bilingual_str strError;
         CCoinControl coinControl;
         coinControl.m_feerate = CFeeRate(1000);
         {
             LOCK(wallet->cs_wallet);
-            BOOST_CHECK(reserveDest.GetReservedDestination(tallyItem.txdest, false, strError));
+            auto dest_opt = reserveDest.GetReservedDestination(false);
+            BOOST_CHECK(dest_opt);
+            tallyItem.txdest = *dest_opt;
         }
         for (CAmount nAmount : vecAmounts) {
             CTransactionRef tx;

src/wallet/wallet.cpp

@@ -2366,15 +2366,11 @@ util::Result<CTxDestination> CWallet::GetNewChangeDestination()
 {
     LOCK(cs_wallet);
 
-    CTxDestination dest;
-    bilingual_str error;
     ReserveDestination reservedest(this);
-    if (!reservedest.GetReservedDestination(dest, true, error)) {
-        return util::Error{error};
-    }
+    auto op_dest = reservedest.GetReservedDestination(true);
+    if (op_dest) reservedest.KeepDestination();
 
-    reservedest.KeepDestination();
-    return dest;
+    return op_dest;
 }
 
 std::optional<int64_t> CWallet::GetOldestKeyPoolTime() const
@@ -2443,12 +2439,11 @@ std::set<std::string> CWallet::ListAddrBookLabels(const std::string& purpose) co
     return label_set;
 }
 
-bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool fInternalIn, bilingual_str& error)
+util::Result<CTxDestination> ReserveDestination::GetReservedDestination(bool fInternalIn)
 {
     m_spk_man = pwallet->GetScriptPubKeyMan(fInternalIn);
     if (!m_spk_man) {
-        error = _("Error: No addresses available.");
-        return false;
+        return util::Error{_("Error: No addresses available.")};
     }
 
     if (nIndex == -1)
@@ -2457,14 +2452,13 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool fInte
 
         CKeyPool keypool;
         int64_t index;
-        if (!m_spk_man->GetReservedDestination(fInternalIn, address, index, keypool, error)) {
-            return false;
-        }
+        auto op_address = m_spk_man->GetReservedDestination(fInternalIn, index, keypool);
+        if (!op_address) return op_address;
+        address = *op_address;
         nIndex = index;
         fInternal = keypool.fInternal;
     }
-    dest = address;
-    return true;
+    return address;
 }
 
 void ReserveDestination::KeepDestination()

src/wallet/wallet.h

@@ -203,7 +203,7 @@ class ReserveDestination
     }
 
     //! Reserve an address
-    bool GetReservedDestination(CTxDestination& pubkey, bool internal, bilingual_str& error);
+    util::Result<CTxDestination> GetReservedDestination(bool internal);
     //! Return reserved address
     void ReturnDestination();
     //! Keep the address. Do not return its key to the keypool when this object goes out of scope