diff options
| -rw-r--r-- | src/primitives/transaction.h | 7 | ||||
| -rw-r--r-- | src/wallet/spend.cpp | 41 |
2 files changed, 34 insertions, 14 deletions
diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index f496ea022e..6b4a6335a1 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -17,6 +17,7 @@ #include <ios> #include <limits> #include <memory> +#include <numeric> #include <string> #include <tuple> #include <utility> @@ -280,6 +281,12 @@ inline void SerializeTransaction(const TxType& tx, Stream& s) { s << tx.nLockTime; } +template<typename TxType> +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 3ced3ebeb5..dd3f0e99d1 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -8,6 +8,7 @@ #include <interfaces/chain.h> #include <numeric> #include <policy/policy.h> +#include <primitives/transaction.h> #include <script/signingprovider.h> #include <util/check.h> #include <util/fees.h> @@ -778,7 +779,6 @@ static util::Result<CreatedTransactionResult> 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 +960,28 @@ static util::Result<CreatedTransactionResult> 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; + const CAmount output_value = CalculateOutputValue(txNew); + Assume(recipients_sum + change_amount == output_value); + CAmount current_fee = result->GetSelectedValue() - output_value; - // 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) { - return util::Error{Untranslated(STR_INTERNAL_BUG("Fee needed > fee paid"))}; + // Sanity check that the fee cannot be negative as that means we have more output value than input value + if (current_fee < 0) { + return util::Error{Untranslated(STR_INTERNAL_BUG("Fee paid < 0"))}; } // 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 = result->GetSelectedValue() - CalculateOutputValue(txNew); + if (fee_needed != current_fee) { + return util::Error{Untranslated(STR_INTERNAL_BUG("Change adjustment: Fee needed != fee paid"))}; + } } // 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 +1012,16 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( } ++i; } - nFeeRet = fee_needed; + current_fee = result->GetSelectedValue() - CalculateOutputValue(txNew); + if (fee_needed != current_fee) { + return util::Error{Untranslated(STR_INTERNAL_BUG("SFFO: Fee needed != fee paid"))}; + } + } + + // fee_needed should now always be less than or equal to the current fees that we pay. + // If it is not, it is a bug. + if (fee_needed > current_fee) { + return util::Error{Untranslated(STR_INTERNAL_BUG("Fee needed > fee paid"))}; } // Give up if change keypool ran out and change is required @@ -1030,7 +1043,7 @@ static util::Result<CreatedTransactionResult> 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 +1059,14 @@ static util::Result<CreatedTransactionResult> 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<CreatedTransactionResult> CreateTransaction( |