Merge dashpay#6823: backport: merge bitcoin#25218, bitcoin#25721, bitcoin#25656, bitcoin#25616, bitcoin#26005, bitcoin#24855, bitcoin#18554, bitcoin#17204, bitcoin#20562, bitcoin#21166, bitcoin#25044, bitcoin#25364, bitcoin#25525, bitcoin#25512, bitcoin#24678, bitcoin#26747, partial bitcoin#22154, bitcoin#24584 (wallet backports: part 8)

c0f9225 merge bitcoin#26747: fix confusing error / GUI crash on cross-chain legacy wallet restore (Kittywhiskers Van Gogh)
d63ca8a merge bitcoin#24678: Prevent wallet unload on GetWalletForJSONRPCRequest (Kittywhiskers Van Gogh)
2a880d8 merge bitcoin#25512: remove wallet dependency and refactor rpc_signrawtransaction.py (Kittywhiskers Van Gogh)
f405790 merge bitcoin#25525: remove wallet dependency from mempool_updatefromblock.py (Kittywhiskers Van Gogh)
c24c9ea merge bitcoin#25364: remove wallet dependency from feature_nulldummy.py (Kittywhiskers Van Gogh)
a118fe1 test: reduce `num_nodes` in `feature_nulldummy.py` to 1 (Kittywhiskers Van Gogh)
c6eabc1 merge bitcoin#25044: Use MiniWallet in rpc_rawtransaction.py (Kittywhiskers Van Gogh)
4e0725e merge bitcoin#21166: Introduce DeferredSignatureChecker and have SignatureExtractorClass subclass it (Kittywhiskers Van Gogh)
f883122 merge bitcoin#20562: Test that a fully signed tx given to signrawtx is unchanged (Kittywhiskers Van Gogh)
c349dad merge bitcoin#17204: Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code (Kittywhiskers Van Gogh)
c5e8bd6 merge bitcoin#18554: ensure wallet files are not reused across chains (Kittywhiskers Van Gogh)
0fca914 merge bitcoin#24855: Fix `setwalletflag` disabling of flags (Kittywhiskers Van Gogh)
f15a1b9 merge bitcoin#26005: Fix error handling (copy_file failure in RestoreWallet, and in general via interfaces) (Kittywhiskers Van Gogh)
240765e merge bitcoin#25616: Return `util::Result` from WalletLoader methods (Kittywhiskers Van Gogh)
bd897fa merge bitcoin#25656: return util::Result from `GetReservedDestination` methods (Kittywhiskers Van Gogh)
56accfe merge bitcoin#25721: Replace BResult with util::Result (Kittywhiskers Van Gogh)
03939f2 partial bitcoin#24584: avoid mixing different `OutputTypes` during coin selection (Kittywhiskers Van Gogh)
50ca8cf refactor: remove `CKey` overload for `Create{,AndProcess}Block()` (Kittywhiskers Van Gogh)
9e23b11 merge bitcoin#25218: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination (Kittywhiskers Van Gogh)
ec764fd partial bitcoin#22154: Add OutputType::BECH32M and related wallet support for fetching bech32m addresses (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * The `CKey` overload for `Create{,AndProcess}Block()` was introduced as a convenience function in [dash#2264](dashpay#2264). To allow for the easier backport of [bitcoin#24584](bitcoin#24584) (by resolving an ambiguous overload error) and because `GetScriptForRawPubKey` makes the same construction, it was opted to realign with upstream and drop the overload.

  * Even though we don't actively support any address types that utilize Bech32m, [bitcoin#25656](bitcoin#25656) assumes that `GetReservedDestination` modifies a `bilingual_str` for its error case in order to encapsulate it in a `util::Result`. In order to complete that backport, [bitcoin#22154](bitcoin#22154) has been partially backported.

  * `CKeyHolder` has always assumed that `GetReservedDestination` will succeed without validation of this assumption ([source](https://github.com/dashpay/dash/blob/develop/src/coinjoin/util.cpp#L29-L33)). As [bitcoin#25656](bitcoin#25656) forces us to unwrap the returned value, this assumption has been documented with an `assert` ([source](https://github.com/dashpay/dash/blob/bd897fa7097d3d68d19b8a9af14b23f4007645ca/src/coinjoin/util.cpp#L32-L34))

  * When backporting [bitcoin#21166](bitcoin#21166), we don't have the luxury of a `scriptWitness` to include the redemption script, so `rpc_signrawtransaction.py` must import the script in order to spend it. This creates an additional complication as descriptor wallets do not permit wallets to mix descriptors with and without private keys ([source](https://github.com/dashpay/dash/blob/cc9da5d9e28bf9a7b411dc390523ac6d2dd09de6/src/wallet/rpc/backup.cpp#L1758)) and as P2SH descriptors don't involve a private key, those specific subtests need to be skipped for descriptor wallets.

    * `OP_TRUE` has been appended to the script, this mirrors the `OP_TRUE` in the `scriptWitness` ([source](https://github.com/bitcoin/bitcoin/blob/a97a9298cea085858e1a65a5e9b20d7a9e0f7303/test/functional/rpc_signrawtransaction.py#L270))

  * The subtest `test_signing_with_missing_prevtx_info()` introduced in [bitcoin#25044](bitcoin#25044) requires an extra `walletpassphrase` as the preceding tests that unlock the wallet are not called for descriptor wallets, making the call necessary. It is redundant but harmless for legacy wallets.

  * With the backport of [bitcoin#19937](bitcoin#19937) in [dash#6726](dashpay#6726) (as ae7e4cb), we can remove a workaround in `feature_nulldummy.py` where we used two nodes to satisfy `getblocktemplate`'s requirement of a connected node as that requirement was lifted on test chains.

  ## Breaking Changes

  None expected.

  ## How Has This Been Tested?

  A full CoinJoin session run on c0f9225

  ![CoinJoin session run on build c0f9225](https://github.com/user-attachments/assets/1dd31814-8d4a-47ab-95e5-5684818f3971)

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK c0f9225
  UdjinM6:
    utACK c0f9225

Tree-SHA512: 2217281ea75afe3eb6061e1acbd42afb66ff2ab28c15a3ad03bdb528ef4e40ea30bf1adad8d0ba93310a3d561d6c3f2ad95780b0b972af238523c6f2950a39c7

Author: PastaPastaPasta

src/Makefile.test.include

@@ -162,6 +162,7 @@ BITCOIN_TESTS =\
   test/random_tests.cpp \
   test/ratecheck_tests.cpp \
   test/rest_tests.cpp \
+  test/result_tests.cpp \
   test/reverselock_tests.cpp \
   test/rpc_tests.cpp \
   test/sanity_tests.cpp \
@@ -214,6 +215,7 @@ BITCOIN_TESTS += \
   wallet/test/wallet_crypto_tests.cpp \
   wallet/test/wallet_transaction_tests.cpp \
   wallet/test/coinselector_tests.cpp \
+  wallet/test/availablecoins_tests.cpp \
   wallet/test/init_tests.cpp \
   wallet/test/ismine_tests.cpp \
   wallet/test/scriptpubkeyman_tests.cpp

src/bench/coin_selection.cpp

@@ -54,10 +54,10 @@ static void CoinSelection(benchmark::Bench& bench)
     addCoin(3 * COIN, wallet, wtxs);
 
     // Create coins
-    std::vector<COutput> coins;
+    wallet::CoinsResult available_coins;
     for (const auto& wtx : wtxs) {
         const auto txout = wtx->tx->vout.at(0);
-        coins.emplace_back(COutPoint(wtx->GetHash(), 0), txout, /*depth=*/6 * 24, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/ 0);
+        available_coins.legacy.emplace_back(COutPoint(wtx->GetHash(), 0), txout, /*depth=*/6 * 24, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/ 0);
     }
     const CoinEligibilityFilter filter_standard(1, 6, 0);
     FastRandomContext rand{};
@@ -73,7 +73,7 @@ static void CoinSelection(benchmark::Bench& bench)
         /*avoid_partial=*/ false,
     };
     bench.run([&] {
-        auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, coins, coin_selection_params);
+        auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, available_coins, coin_selection_params, /*allow_mixed_output_types=*/true);
         assert(result);
         assert(result->GetSelectedValue() == 1003 * COIN);
         assert(result->GetInputSet().size() == 2);

src/bench/wallet_loading.cpp

@@ -47,12 +47,8 @@ static void BenchUnloadWallet(std::shared_ptr<CWallet>&& wallet)
 
 static void AddTx(CWallet& wallet)
 {
-    bilingual_str error;
-    CTxDestination dest;
-    wallet.GetNewDestination("", dest, error);
-
     CMutableTransaction mtx;
-    mtx.vout.push_back({COIN, GetScriptForDestination(dest)});
+    mtx.vout.push_back({COIN, GetScriptForDestination(*Assert(wallet.GetNewDestination("")))});
     mtx.vin.push_back(CTxIn());
 
     wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInactive{});

src/coinjoin/client.cpp

@@ -1552,7 +1552,7 @@ bool CCoinJoinClientSession::CreateCollateralTransaction(CMutableTransaction& tx
     AssertLockHeld(m_wallet->cs_wallet);
 
     CCoinControl coin_control(CoinType::ONLY_COINJOIN_COLLATERAL);
-    std::vector<COutput> vCoins{AvailableCoinsListUnspent(*m_wallet, &coin_control).coins};
+    std::vector<COutput> vCoins{AvailableCoinsListUnspent(*m_wallet, &coin_control).all()};
     if (vCoins.empty()) {
         strReason = strprintf("%s requires a collateral transaction and could not locate an acceptable input!", gCoinJoinName);
         return false;
@@ -1571,11 +1571,10 @@ bool CCoinJoinClientSession::CreateCollateralTransaction(CMutableTransaction& tx
     if (txout.nValue >= CoinJoin::GetCollateralAmount() * 2) {
         // make our change address
         CScript scriptChange;
-        CTxDestination dest;
         ReserveDestination reserveDest(m_wallet.get());
-        bool success = reserveDest.GetReservedDestination(dest, true);
-        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,7 +29,9 @@ inline unsigned int GetSizeOfCompactSizeDiff(uint64_t nSizePrev, uint64_t nSizeN
 CKeyHolder::CKeyHolder(CWallet* pwallet) :
     reserveDestination(pwallet)
 {
-    reserveDestination.GetReservedDestination(dest, false);
+    auto dest_opt = reserveDestination.GetReservedDestination(false);
+    assert(dest_opt);
+    dest = *dest_opt;
 }
 
 void CKeyHolder::KeepKey()
@@ -99,10 +101,10 @@ CTransactionBuilderOutput::CTransactionBuilderOutput(CTransactionBuilder* pTxBui
     nAmount(nAmountIn)
 {
     assert(pTxBuilder);
-    CTxDestination txdest;
     LOCK(wallet.cs_wallet);
-    dest.GetReservedDestination(txdest, false);
-    script = ::GetScriptForDestination(txdest);
+    auto dest_opt = dest.GetReservedDestination(false);
+    assert(dest_opt);
+    script = ::GetScriptForDestination(*dest_opt);
 }
 
 bool CTransactionBuilderOutput::UpdateAmount(const CAmount nNewAmount)
@@ -280,12 +282,13 @@ bool CTransactionBuilder::Commit(bilingual_str& strResult)
     CTransactionRef tx;
     {
         LOCK2(m_wallet.cs_wallet, ::cs_main);
-        FeeCalculation fee_calc_out;
-        if (auto txr = wallet::CreateTransaction(m_wallet, vecSend, nChangePosRet, strResult, coinControl, fee_calc_out)) {
-            tx = txr->tx;
-            nFeeRet = txr->fee;
-            nChangePosRet = txr->change_pos;
+        auto ret = wallet::CreateTransaction(m_wallet, vecSend, nChangePosRet, coinControl);
+        if (ret) {
+            tx = ret->tx;
+            nFeeRet = ret->fee;
+            nChangePosRet = ret->change_pos;
         } else {
+            strResult = util::ErrorString(ret);
             return false;
         }
     }

src/dummywallet.cpp

@@ -78,7 +78,8 @@ void DummyWalletInit::AddWalletOptions(ArgsManager& argsman) const
         "-flushwallet",
         "-privdb",
         "-walletrejectlongchains",
-        "-unsafesqlitesync"
+        "-walletcrosschain",
+        "-unsafesqlitesync",
     });
 }
 

src/interfaces/wallet.h

@@ -112,7 +112,7 @@ class Wallet
     virtual std::string getWalletName() = 0;
 
     // Get a new address.
-    virtual bool getNewDestination(const std::string label, CTxDestination& dest) = 0;
+    virtual util::Result<CTxDestination> getNewDestination(const std::string& label) = 0;
 
     //! Get public key.
     virtual bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) = 0;
@@ -167,12 +167,11 @@ class Wallet
     virtual std::vector<COutPoint> listProTxCoins() = 0;
 
     //! Create transaction.
-    virtual CTransactionRef createTransaction(const std::vector<wallet::CRecipient>& recipients,
+    virtual util::Result<CTransactionRef> createTransaction(const std::vector<wallet::CRecipient>& recipients,
         const wallet::CCoinControl& coin_control,
         bool sign,
         int& change_pos,
-        CAmount& fee,
-        bilingual_str& fail_reason) = 0;
+        CAmount& fee) = 0;
 
     //! Commit transaction.
     virtual void commitTransaction(CTransactionRef tx,
@@ -348,35 +347,35 @@ class Wallet
 class WalletLoader : public ChainClient
 {
 public:
-   //! Register non-core wallet RPCs
-   virtual void registerOtherRpcs(const Span<const CRPCCommand>& commands) = 0;
+    //! Register non-core wallet RPCs
+    virtual void registerOtherRpcs(const Span<const CRPCCommand>& commands) = 0;
 
-   //! Create new wallet.
-   virtual std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
+    //! Create new wallet.
+    virtual util::Result<std::unique_ptr<Wallet>> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector<bilingual_str>& warnings) = 0;
 
-   //! Load existing wallet.
-   virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
+    //! Load existing wallet.
+    virtual util::Result<std::unique_ptr<Wallet>> loadWallet(const std::string& name, std::vector<bilingual_str>& warnings) = 0;
 
-   //! Return default wallet directory.
-   virtual std::string getWalletDir() = 0;
+    //! Return default wallet directory.
+    virtual std::string getWalletDir() = 0;
 
-   //! Restore backup wallet
-   virtual BResult<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0;
+    //! Restore backup wallet
+    virtual util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0;
 
-   //! Return available wallets in wallet directory.
-   virtual std::vector<std::string> listWalletDir() = 0;
+    //! Return available wallets in wallet directory.
+    virtual std::vector<std::string> listWalletDir() = 0;
 
-   //! Return interfaces for accessing wallets (if any).
-   virtual std::vector<std::unique_ptr<Wallet>> getWallets() = 0;
+    //! Return interfaces for accessing wallets (if any).
+    virtual std::vector<std::unique_ptr<Wallet>> getWallets() = 0;
 
-   //! Register handler for load wallet messages. This callback is triggered by
-   //! createWallet and loadWallet above, and also triggered when wallets are
-   //! loaded at startup or by RPC.
-   using LoadWalletFn = std::function<void(std::unique_ptr<Wallet> wallet)>;
-   virtual std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) = 0;
+    //! Register handler for load wallet messages. This callback is triggered by
+    //! createWallet and loadWallet above, and also triggered when wallets are
+    //! loaded at startup or by RPC.
+    using LoadWalletFn = std::function<void(std::unique_ptr<Wallet> wallet)>;
+    virtual std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) = 0;
 
-   //! Return pointer to internal context, useful for testing.
-   virtual wallet::WalletContext* context() { return nullptr; }
+    //! Return pointer to internal context, useful for testing.
+    virtual wallet::WalletContext* context() { return nullptr; }
 };
 
 //! Information about one wallet address.

src/qt/addresstablemodel.cpp

@@ -366,23 +366,21 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con
     else if(type == Receive)
     {
         // Generate a new address to associate with given label
-        CTxDestination dest;
-        if(!walletModel->wallet().getNewDestination(strLabel, dest))
-        {
+        auto op_dest = walletModel->wallet().getNewDestination(strLabel);
+        if (!op_dest) {
             WalletModel::UnlockContext ctx(walletModel->requestUnlock());
-            if(!ctx.isValid())
-            {
+            if (!ctx.isValid()) {
                 // Unlock wallet failed or was cancelled
                 editStatus = WALLET_UNLOCK_FAILURE;
                 return QString();
             }
-            if(!walletModel->wallet().getNewDestination(strLabel, dest))
-            {
+            op_dest = walletModel->wallet().getNewDestination(strLabel);
+            if (!op_dest) {
                 editStatus = KEY_GENERATION_FAILURE;
                 return QString();
             }
         }
-        strAddress = EncodeDestination(dest);
+        strAddress = EncodeDestination(*op_dest);
     }
     else
     {

src/qt/walletcontroller.cpp

@@ -258,9 +258,13 @@ void CreateWalletActivity::createWallet()
     }
 
     QTimer::singleShot(500ms, worker(), [this, name, flags] {
-        std::unique_ptr<interfaces::Wallet> wallet = node().walletLoader().createWallet(name, m_passphrase, flags, m_error_message, m_warning_message);
+        auto wallet{node().walletLoader().createWallet(name, m_passphrase, flags, m_warning_message)};
 
-        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
+        if (wallet) {
+            m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
+        } else {
+            m_error_message = util::ErrorString(wallet);
+        }
 
         QTimer::singleShot(500ms, this, &CreateWalletActivity::finish);
     });
@@ -330,9 +334,13 @@ void OpenWalletActivity::open(const std::string& path)
         tr("Opening Wallet <b>%1</b>…").arg(name.toHtmlEscaped()));
 
     QTimer::singleShot(0, worker(), [this, path] {
-        std::unique_ptr<interfaces::Wallet> wallet = node().walletLoader().loadWallet(path, m_error_message, m_warning_message);
+        auto wallet{node().walletLoader().loadWallet(path, m_warning_message)};
 
-        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
+        if (wallet) {
+            m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
+        } else {
+            m_error_message = util::ErrorString(wallet);
+        }
 
         QTimer::singleShot(0, this, &OpenWalletActivity::finish);
     });
@@ -375,8 +383,11 @@ void RestoreWalletActivity::restore(const fs::path& backup_file, const std::stri
     QTimer::singleShot(0, worker(), [this, backup_file, wallet_name] {
         auto wallet{node().walletLoader().restoreWallet(backup_file, wallet_name, m_warning_message)};
 
-        m_error_message = wallet ? bilingual_str{} : wallet.GetError();
-        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(wallet.ReleaseObj());
+        if (wallet) {
+            m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
+        } else {
+            m_error_message = util::ErrorString(wallet);
+        }
 
         QTimer::singleShot(0, this, &RestoreWalletActivity::finish);
     });

src/qt/walletmodel.cpp

@@ -251,11 +251,11 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
     }
 
     CAmount nFeeRequired = 0;
-    bilingual_str error;
     int nChangePosRet = -1;
 
     auto& newTx = transaction.getWtx();
-    newTx = m_wallet->createTransaction(vecSend, coinControl, !wallet().privateKeysDisabled() /* sign */, nChangePosRet, nFeeRequired, error);
+    const auto& res = m_wallet->createTransaction(vecSend, coinControl, !wallet().privateKeysDisabled() /* sign */, nChangePosRet, nFeeRequired);
+    newTx = res ? *res : nullptr;
     transaction.setTransactionFee(nFeeRequired);
     if (fSubtractFeeFromAmount && newTx)
         transaction.reassignAmounts(nChangePosRet);
@@ -266,7 +266,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
         {
             return SendCoinsReturn(AmountWithFeeExceedsBalance);
         }
-        Q_EMIT message(tr("Send Coins"), QString::fromStdString(error.translated),
+        Q_EMIT message(tr("Send Coins"), QString::fromStdString(util::ErrorString(res).translated),
                      CClientUIInterface::MSG_ERROR);
         return TransactionCreationFailed;
     }