send: move wallet CreateTransaction* flow to use OperationResult/CallResult.

furszy · furszy · commit 3318ee2cb9e2 · 2022-04-16T18:14:30.000-03:00
diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
@@ -11,6 +11,7 @@
 #include <pubkey.h>                    // For CKeyID and CScriptID (definitions needed in CTxDestination instantiation)
 #include <script/standard.h>           // For CTxDestination
 #include <support/allocators/secure.h> // For SecureString
+#include <operationresult.h>
 #include <util/message.h>
 #include <util/ui_change_type.h>
 
@@ -138,12 +139,11 @@ class Wallet
     virtual void listLockedCoins(std::vector<COutPoint>& outputs) = 0;
 
     //! Create transaction.
-    virtual CTransactionRef createTransaction(const std::vector<wallet::CRecipient>& recipients,
+    virtual CallResult<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,
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
@@ -204,10 +204,10 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
     {
         CAmount nFeeRequired = 0;
         int nChangePosRet = -1;
-        bilingual_str error;
 
         auto& newTx = transaction.getWtx();
-        newTx = m_wallet->createTransaction(vecSend, coinControl, !wallet().privateKeysDisabled() /* sign */, nChangePosRet, nFeeRequired, error);
+        auto res = m_wallet->createTransaction(vecSend, coinControl, !wallet().privateKeysDisabled() /* sign */, nChangePosRet, nFeeRequired);
+        newTx = *res.GetObjResult();
         transaction.setTransactionFee(nFeeRequired);
         if (fSubtractFeeFromAmount && newTx)
             transaction.reassignAmounts(nChangePosRet);
@@ -218,7 +218,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(res.GetError().translated),
                 CClientUIInterface::MSG_ERROR);
             return TransactionCreationFailed;
         }
diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp
@@ -221,10 +221,10 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
     CTransactionRef tx_new;
     CAmount fee_ret;
     int change_pos_in_out = -1; // No requested location for change
-    bilingual_str fail_reason;
     FeeCalculation fee_calc_out;
-    if (!CreateTransaction(wallet, recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, fee_calc_out, false)) {
-        errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + fail_reason);
+    auto res = CreateTransaction(wallet, recipients, tx_new, fee_ret, change_pos_in_out, new_coin_control, fee_calc_out, false);
+    if (!res) {
+        errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + res.GetError());
         return Result::WALLET_ERROR;
     }
 
diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp
@@ -249,21 +249,18 @@ class WalletImpl : public Wallet
         LOCK(m_wallet->cs_wallet);
         return m_wallet->ListLockedCoins(outputs);
     }
-    CTransactionRef createTransaction(const std::vector<CRecipient>& recipients,
+    CallResult<CTransactionRef> createTransaction(const std::vector<CRecipient>& recipients,
         const CCoinControl& coin_control,
         bool sign,
         int& change_pos,
-        CAmount& fee,
-        bilingual_str& fail_reason) override
+        CAmount& fee) override
     {
         LOCK(m_wallet->cs_wallet);
         CTransactionRef tx;
         FeeCalculation fee_calc_out;
-        if (!CreateTransaction(*m_wallet, recipients, tx, fee, change_pos,
-                fail_reason, coin_control, fee_calc_out, sign)) {
-            return {};
-        }
-        return tx;
+        auto res = CreateTransaction(*m_wallet, recipients, tx, fee, change_pos,
+                                     coin_control, fee_calc_out, sign);
+        return res ? CallResult<CTransactionRef>(tx) : CallResult<CTransactionRef>(res.GetError());
     }
     void commitTransaction(CTransactionRef tx,
         WalletValueMap value_map,
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
@@ -157,12 +157,11 @@ UniValue SendMoney(CWallet& wallet, const CCoinControl &coin_control, std::vecto
     // Send
     CAmount nFeeRequired = 0;
     int nChangePosRet = -1;
-    bilingual_str error;
     CTransactionRef tx;
     FeeCalculation fee_calc_out;
-    const bool fCreated = CreateTransaction(wallet, recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, fee_calc_out, true);
-    if (!fCreated) {
-        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original);
+    auto res = CreateTransaction(wallet, recipients, tx, nFeeRequired, nChangePosRet, coin_control, fee_calc_out, true);
+    if (!res) {
+        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, res.GetError().original);
     }
     wallet.CommitTransaction(tx, std::move(map_value), {} /* orderForm */);
     if (verbose) {
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
@@ -651,13 +651,12 @@ static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng
     }
 }
 
-static bool CreateTransactionInternal(
+static OperationResult CreateTransactionInternal(
         CWallet& wallet,
         const std::vector<CRecipient>& vecSend,
         CTransactionRef& tx,
         CAmount& nFeeRet,
         int& nChangePosInOut,
-        bilingual_str& error,
         const CCoinControl& coin_control,
         FeeCalculation& fee_calc_out,
         bool sign) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
@@ -666,6 +665,7 @@ static bool CreateTransactionInternal(
 
     FastRandomContext rng_fast;
     CMutableTransaction txNew; // The resulting transaction that we make
+    bilingual_str error; // possible error str
 
     CoinSelectionParams coin_selection_params{rng_fast}; // Parameters for coin selection, init with dummy
     coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
@@ -736,13 +736,11 @@ static bool CreateTransactionInternal(
     // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly
     // provided one
     if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) {
-        error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB));
-        return false;
+        return ErrorOut(strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB)));
     }
     if (feeCalc.reason == FeeReason::FALLBACK && !wallet.m_allow_fallback_fee) {
         // eventually allow a fallback fee
-        error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee.");
-        return false;
+        return ErrorOut(_("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."));
     }
 
     // Calculate the cost of change
@@ -766,10 +764,8 @@ static bool CreateTransactionInternal(
             coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION);
         }
 
-        if (IsDust(txout, wallet.chain().relayDustFee()))
-        {
-            error = _("Transaction amount too small");
-            return false;
+        if (IsDust(txout, wallet.chain().relayDustFee())) {
+            return ErrorOut(_("Transaction amount too small"));
         }
         txNew.vout.push_back(txout);
     }
@@ -785,8 +781,7 @@ static bool CreateTransactionInternal(
     // Choose coins to use
     std::optional<SelectionResult> result = SelectCoins(wallet, vAvailableCoins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params);
     if (!result) {
-        error = _("Insufficient funds");
-        return false;
+        return ErrorOut(_("Insufficient funds"));
     }
 
     // Always make a change output
@@ -799,10 +794,8 @@ static bool CreateTransactionInternal(
         // Insert change txn at random position:
         nChangePosInOut = rng_fast.randrange(txNew.vout.size() + 1);
     }
-    else if ((unsigned int)nChangePosInOut > txNew.vout.size())
-    {
-        error = _("Transaction change output index out of range");
-        return false;
+    else if ((unsigned int)nChangePosInOut > txNew.vout.size()) {
+        return ErrorOut(_("Transaction change output index out of range"));
     }
 
     assert(nChangePosInOut != -1);
@@ -829,8 +822,7 @@ static bool CreateTransactionInternal(
     TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, &coin_control);
     int nBytes = tx_sizes.vsize;
     if (nBytes == -1) {
-        error = _("Missing solving data for estimating transaction size");
-        return false;
+        return ErrorOut(_("Missing solving data for estimating transaction size"));
     }
     nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes);
 
@@ -894,7 +886,7 @@ static bool CreateTransactionInternal(
                     } else {
                         error = _("The transaction amount is too small to send after the fee has been deducted");
                     }
-                    return false;
+                    return ErrorOut(error);
                 }
             }
             ++i;
@@ -904,12 +896,11 @@ static bool CreateTransactionInternal(
 
     // Give up if change keypool ran out and change is required
     if (scriptChange.empty() && nChangePosInOut != -1) {
-        return false;
+        return ErrorOut(error);
     }
 
     if (sign && !wallet.SignTransaction(txNew)) {
-        error = _("Signing transaction failed");
-        return false;
+        return ErrorOut(_("Signing transaction failed"));
     }
 
     // Return the constructed transaction data.
@@ -919,20 +910,17 @@ static bool CreateTransactionInternal(
     if ((sign && GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT) ||
         (!sign && tx_sizes.weight > MAX_STANDARD_TX_WEIGHT))
     {
-        error = _("Transaction too large");
-        return false;
+        return ErrorOut(_("Transaction too large"));
     }
 
     if (nFeeRet > wallet.m_default_max_tx_fee) {
-        error = TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED);
-        return false;
+        return ErrorOut(TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED));
     }
 
     if (gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) {
         // Lastly, ensure this tx will pass the mempool's chain limits
         if (!wallet.chain().checkChainLimits(tx)) {
-            error = _("Transaction has too long of a mempool chain");
-            return false;
+            return ErrorOut(_("Transaction has too long of a mempool chain"));
         }
     }
 
@@ -949,44 +937,41 @@ static bool CreateTransactionInternal(
               feeCalc.est.fail.start, feeCalc.est.fail.end,
               (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) > 0.0 ? 100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) : 0.0,
               feeCalc.est.fail.withinTarget, feeCalc.est.fail.totalConfirmed, feeCalc.est.fail.inMempool, feeCalc.est.fail.leftMempool);
-    return true;
+    return {true};
 }
 
-bool CreateTransaction(
-        CWallet& wallet,
-        const std::vector<CRecipient>& vecSend,
-        CTransactionRef& tx,
-        CAmount& nFeeRet,
-        int& nChangePosInOut,
-        bilingual_str& error,
-        const CCoinControl& coin_control,
-        FeeCalculation& fee_calc_out,
-        bool sign)
+OperationResult CreateTransaction(
+                CWallet& wallet,
+                const std::vector<CRecipient>& vecSend,
+                CTransactionRef& tx,
+                CAmount& nFeeRet,
+                int& nChangePosInOut,
+                const CCoinControl& coin_control,
+                FeeCalculation& fee_calc_out,
+                bool sign)
 {
     if (vecSend.empty()) {
-        error = _("Transaction must have at least one recipient");
-        return false;
+        return ErrorOut(_("Transaction must have at least one recipient"));
     }
 
     if (std::any_of(vecSend.cbegin(), vecSend.cend(), [](const auto& recipient){ return recipient.nAmount < 0; })) {
-        error = _("Transaction amounts must not be negative");
-        return false;
+        return ErrorOut(_("Transaction amounts must not be negative"));
     }
 
     LOCK(wallet.cs_wallet);
 
     int nChangePosIn = nChangePosInOut;
     Assert(!tx); // tx is an out-param. TODO change the return type from bool to tx (or nullptr)
-    bool res = CreateTransactionInternal(wallet, vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, fee_calc_out, sign);
+    auto res = CreateTransactionInternal(wallet, vecSend, tx, nFeeRet, nChangePosInOut, coin_control, fee_calc_out, sign);
     // try with avoidpartialspends unless it's enabled already
     if (res && nFeeRet > 0 /* 0 means non-functional fee rate estimation */ && wallet.m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) {
         CCoinControl tmp_cc = coin_control;
         tmp_cc.m_avoid_partial_spends = true;
         CAmount nFeeRet2;
         CTransactionRef tx2;
         int nChangePosInOut2 = nChangePosIn;
-        bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results
-        if (CreateTransactionInternal(wallet, vecSend, tx2, nFeeRet2, nChangePosInOut2, error2, tmp_cc, fee_calc_out, sign)) {
+        // if an error occurs, we discard the results
+        if (CreateTransactionInternal(wallet, vecSend, tx2, nFeeRet2, nChangePosInOut2, tmp_cc, fee_calc_out, sign)) {
             // if fee of this alternative one is within the range of the max fee, we use this one
             const bool use_aps = nFeeRet2 <= nFeeRet + wallet.m_max_aps_fee;
             wallet.WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", nFeeRet, nFeeRet2, use_aps ? "grouped" : "non-grouped");
@@ -1023,7 +1008,9 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
 
     CTransactionRef tx_new;
     FeeCalculation fee_calc_out;
-    if (!CreateTransaction(wallet, vecSend, tx_new, nFeeRet, nChangePosInOut, error, coinControl, fee_calc_out, false)) {
+    auto res = CreateTransaction(wallet, vecSend, tx_new, nFeeRet, nChangePosInOut, coinControl, fee_calc_out, false);
+    if (!res) {
+        error = res.GetError();
         return false;
     }
 
diff --git a/src/wallet/spend.h b/src/wallet/spend.h
@@ -6,6 +6,7 @@
 #define BITCOIN_WALLET_SPEND_H
 
 #include <consensus/amount.h>
+#include <operationresult.h>
 #include <wallet/coinselection.h>
 #include <wallet/transaction.h>
 #include <wallet/wallet.h>
@@ -87,7 +88,7 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vec
  * selected by SelectCoins(); Also create the change output, when needed
  * @note passing nChangePosInOut as -1 will result in setting a random position
  */
-bool CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true);
+OperationResult CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true);
 
 /**
  * Insert additional inputs into the transaction by
diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
@@ -30,14 +30,13 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
         CTransactionRef tx;
         CAmount fee;
         int change_pos = -1;
-        bilingual_str error;
         CCoinControl coin_control;
         coin_control.m_feerate.emplace(10000);
         coin_control.fOverrideFeeRate = true;
         // We need to use a change type with high cost of change so that the leftover amount will be dropped to fee instead of added as a change output
         coin_control.m_change_type = OutputType::LEGACY;
         FeeCalculation fee_calc;
-        BOOST_CHECK(CreateTransaction(*wallet, {recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
+        BOOST_CHECK(CreateTransaction(*wallet, {recipient}, tx, fee, change_pos, coin_control, fee_calc));
         BOOST_CHECK_EQUAL(tx->vout.size(), 1);
         BOOST_CHECK_EQUAL(tx->vout[0].nValue, recipient.nAmount + leftover_input_amount - fee);
         BOOST_CHECK_GT(fee, 0);
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
@@ -523,11 +523,10 @@ class ListCoinsTestingSetup : public TestChain100Setup
         CTransactionRef tx;
         CAmount fee;
         int changePos = -1;
-        bilingual_str error;
         CCoinControl dummy;
         FeeCalculation fee_calc_out;
         {
-            BOOST_CHECK(CreateTransaction(*wallet, {recipient}, tx, fee, changePos, error, dummy, fee_calc_out));
+            BOOST_CHECK(CreateTransaction(*wallet, {recipient}, tx, fee, changePos, dummy, fee_calc_out));
         }
         wallet->CommitTransaction(tx, {}, {});
         CMutableTransaction blocktx;

Original file line number	Diff line number	Diff line change
`@@ -204,10 +204,10 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact`
`204`	`204`	`{`
`205`	`205`	`CAmount nFeeRequired = 0;`
`206`	`206`	`int nChangePosRet = -1;`
`207`		`- bilingual_str error;`
`208`	`207`
`209`	`208`	`auto& newTx = transaction.getWtx();`
`210`		`- newTx = m_wallet->createTransaction(vecSend, coinControl, !wallet().privateKeysDisabled() /* sign */, nChangePosRet, nFeeRequired, error);`
	`209`	`+ auto res = m_wallet->createTransaction(vecSend, coinControl, !wallet().privateKeysDisabled() /* sign */, nChangePosRet, nFeeRequired);`
	`210`	`+ newTx = *res.GetObjResult();`
`211`	`211`	`transaction.setTransactionFee(nFeeRequired);`
`212`	`212`	`if (fSubtractFeeFromAmount && newTx)`
`213`	`213`	`transaction.reassignAmounts(nChangePosRet);`
`@@ -218,7 +218,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact`
`218`	`218`	`{`
`219`	`219`	`return SendCoinsReturn(AmountWithFeeExceedsBalance);`
`220`	`220`	`}`
`221`		`- Q_EMIT message(tr("Send Coins"), QString::fromStdString(error.translated),`
	`221`	`+ Q_EMIT message(tr("Send Coins"), QString::fromStdString(res.GetError().translated),`
`222`	`222`	`CClientUIInterface::MSG_ERROR);`
`223`	`223`	`return TransactionCreationFailed;`
`224`	`224`	`}`
Original file line number	Diff line number	Diff line change
`@@ -523,11 +523,10 @@ class ListCoinsTestingSetup : public TestChain100Setup`
`523`	`523`	`CTransactionRef tx;`
`524`	`524`	`CAmount fee;`
`525`	`525`	`int changePos = -1;`
`526`		`- bilingual_str error;`
`527`	`526`	`CCoinControl dummy;`
`528`	`527`	`FeeCalculation fee_calc_out;`
`529`	`528`	`{`
`530`		`- BOOST_CHECK(CreateTransaction(*wallet, {recipient}, tx, fee, changePos, error, dummy, fee_calc_out));`
	`529`	`+ BOOST_CHECK(CreateTransaction(*wallet, {recipient}, tx, fee, changePos, dummy, fee_calc_out));`
`531`	`530`	`}`
`532`	`531`	`wallet->CommitTransaction(tx, {}, {});`
`533`	`532`	`CMutableTransaction blocktx;`