From e5daf976d5b064b585029d4bb38d68a8153ea13b Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 5 Dec 2022 15:31:30 -0500 Subject: [PATCH 1/3] wallet: Rename nFeeRet in CreateTransactionInternal to current_fee nFeeRet represents the fee that the transaction currently pays. Update it's name to reflect that. --- src/wallet/spend.cpp | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 3ced3ebeb54..d92fd52ead1 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -778,7 +778,6 @@ static util::Result CreateTransactionInternal( AssertLockHeld(wallet.cs_wallet); // out variables, to be packed into returned result structure - CAmount nFeeRet; int nChangePosInOut = change_pos; FastRandomContext rng_fast; @@ -960,24 +959,24 @@ static util::Result CreateTransactionInternal( return util::Error{_("Missing solving data for estimating transaction size")}; } CAmount fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); - nFeeRet = result->GetSelectedValue() - recipients_sum - change_amount; + CAmount current_fee = result->GetSelectedValue() - recipients_sum - change_amount; // The only time that fee_needed should be less than the amount available for fees is when // we are subtracting the fee from the outputs. If this occurs at any other time, it is a bug. - if (!coin_selection_params.m_subtract_fee_outputs && fee_needed > nFeeRet) { + if (!coin_selection_params.m_subtract_fee_outputs && fee_needed > current_fee) { return util::Error{Untranslated(STR_INTERNAL_BUG("Fee needed > fee paid"))}; } // If there is a change output and we overpay the fees then increase the change to match the fee needed - if (nChangePosInOut != -1 && fee_needed < nFeeRet) { + if (nChangePosInOut != -1 && fee_needed < current_fee) { auto& change = txNew.vout.at(nChangePosInOut); - change.nValue += nFeeRet - fee_needed; - nFeeRet = fee_needed; + change.nValue += current_fee - fee_needed; + current_fee = fee_needed; } // Reduce output values for subtractFeeFromAmount if (coin_selection_params.m_subtract_fee_outputs) { - CAmount to_reduce = fee_needed - nFeeRet; + CAmount to_reduce = fee_needed - current_fee; int i = 0; bool fFirst = true; for (const auto& recipient : vecSend) @@ -1008,7 +1007,7 @@ static util::Result CreateTransactionInternal( } ++i; } - nFeeRet = fee_needed; + current_fee = fee_needed; } // Give up if change keypool ran out and change is required @@ -1030,7 +1029,7 @@ static util::Result CreateTransactionInternal( return util::Error{_("Transaction too large")}; } - if (nFeeRet > wallet.m_default_max_tx_fee) { + if (current_fee > wallet.m_default_max_tx_fee) { return util::Error{TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED)}; } @@ -1046,14 +1045,14 @@ static util::Result CreateTransactionInternal( reservedest.KeepDestination(); wallet.WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n", - nFeeRet, nBytes, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay, + current_fee, nBytes, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay, feeCalc.est.pass.start, feeCalc.est.pass.end, (feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool) > 0.0 ? 100 * feeCalc.est.pass.withinTarget / (feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool) : 0.0, feeCalc.est.pass.withinTarget, feeCalc.est.pass.totalConfirmed, feeCalc.est.pass.inMempool, feeCalc.est.pass.leftMempool, 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 CreatedTransactionResult(tx, nFeeRet, nChangePosInOut, feeCalc); + return CreatedTransactionResult(tx, current_fee, nChangePosInOut, feeCalc); } util::Result CreateTransaction( From c1a84f108e320bd44c172a4dd3bb486ab777ff69 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 5 Dec 2022 14:33:35 -0500 Subject: [PATCH 2/3] wallet: Move fee underpayment check to after fee setting It doesn't make sense to be checking whether the fee paid is underpaying before we've finished setting the fees. So do that after we have done the reduction for SFFO and change adjustment for fee overpayment. --- src/primitives/transaction.h | 7 +++++++ src/wallet/spend.cpp | 27 ++++++++++++++++++--------- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index f496ea022e6..6b4a6335a1a 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -280,6 +281,12 @@ inline void SerializeTransaction(const TxType& tx, Stream& s) { s << tx.nLockTime; } +template +inline CAmount CalculateOutputValue(const TxType& tx) +{ + return std::accumulate(tx.vout.cbegin(), tx.vout.cend(), CAmount{0}, [](CAmount sum, const auto& txout) { return sum + txout.nValue; }); +} + /** The basic transaction that is broadcasted on the network and contained in * blocks. A transaction can contain multiple inputs and outputs. diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index d92fd52ead1..a1a98381e5a 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include