This is a possible fix for #10122
If the fee is capped on -maxtxfee, it will check if the fee is still above -mintxfee and -paytxfee for the given transaction. However, it seems this is a very edge case. In order to run into this, I had to pass -maxtxfee=0.01 -mintxfee=0.005 and then send a transaction that was about 89kB.
It seems that under the defaults no one would run into this; but maybe there’s more scenario’s under which this would be a problem.
@MarcoFalke the help text already contains ...setting this too low may abort large transactions.. It feels redundant to add something like If this maximum makes the fee per kB for a transaction lower than -mintxfee, it will be aborted.?
How do I add release notes?
To add release notes run vim doc/release-notes-16192.md and type
0Wallet
1------
2
3- blabla
-paytxfee and -mintxfee settings. When capping the fee on -maxtxfee, it should still be above both, imo?
2958@@ -2959,6 +2959,16 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
2959 return false;
2960 }
2961
2962+ // If the fee was capped to MAXTXFEE and this results in a too low fee rate given our current
2963+ // configuration, abort.
2964+ if (feeCalc.reason == FeeReason::MAXTXFEE && nFeeNeeded < m_pay_tx_fee.GetFee(nBytes)) {
2965+ strFailReason = _("Transaction is too large to fit in the maximum fee at the configured fixed fee rate");
2966+ return false;
2967+ } else if (feeCalc.reason == FeeReason::MAXTXFEE && nFeeNeeded < m_min_fee.GetFee(nBytes)) {
else.
521+ BOOST_CHECK(!result);
522+ BOOST_CHECK(error.compare("Transaction is too large to fit in the maximum fee at the minimum fee rate") == 0);
523+ }
524+}
525+
526+
https://github.com/bitcoin/bitcoin/pull/16192/commits/58ac729278b71a0fc3d4795777593ffd266dfc2a is unnecessary as the condition will short-circuit. I.e. GetFee already won’t be called if the reason is different.
For the built-in logical AND operator, the result is true if both operands are true. Otherwise, the result is false. This operator is short-circuiting: if the first operand is false, the second operand is not evaluated https://en.cppreference.com/w/cpp/language/operator_logical
Wouldn’t it still run the comparison twice in situations where feeCalc.reason != FeeReason::MAXTXFEE
Would expect it to be optimized out by essentially any compiler.
2963+ // configuration, abort.
2964+ if (feeCalc.reason == FeeReason::MAXTXFEE) {
2965+ if (nFeeNeeded < m_pay_tx_fee.GetFee(nBytes)) {
2966+ strFailReason = _("Transaction is too large to fit in the maximum fee at the configured fixed fee rate");
2967+ return false;
2968+ } else if (nFeeNeeded < m_min_fee.GetFee(nBytes)) {
else?
2964+ if (feeCalc.reason == FeeReason::MAXTXFEE) {
2965+ if (nFeeNeeded < m_pay_tx_fee.GetFee(nBytes)) {
2966+ strFailReason = _("Transaction is too large to fit in the maximum fee at the configured fixed fee rate");
2967+ return false;
2968+ } else if (nFeeNeeded < m_min_fee.GetFee(nBytes)) {
2969+ strFailReason = _("Transaction is too large to fit in the maximum fee at the minimum fee rate");
fit in the maximum fee?
fee rate too low?
Fee rate too low after limiting to -maxtxfee. Just specifying fee rate too low could get people thinking they configured it too low, which is not necessarily the case. Increasing the configured fee would make the problem worse.
0@@ -0,0 +1,12 @@
1+Wallet: Transaction fees
2+------------------------
3+
4+Bitcoin Core caps fees at `-maxtxfee=<x>` (default:
Currently the wallet caps fees at ...?
OK, I can revert it if that’s desirable. Any pointers as to why example_test.py is failing?
EDIT: Never mind, it succeeded on the next check-in - must have been some transient issue
507+ wallet->m_pay_tx_fee = CFeeRate(CAmount(5000000));
508+ {
509+ auto locked_chain = chain->lock();
510+ bool result = wallet->CreateTransaction(*locked_chain, recips, tx, reservekey, fee, changePos, error, dummy);
511+ BOOST_CHECK(!result);
512+ BOOST_CHECK(error.compare("Transaction is too large to fit in the maximum fee at the configured fixed fee rate") == 0);
BOOST_CHECK_EQUAL(error, ...).
485+ CWallet::ScanResult result = wallet->ScanForWalletTransactions(::ChainActive().Genesis()->GetBlockHash(), {} /* stop_block */, reserver, false /* update */);
486+
487+ // Create a transaction with a lot of outputs so it's big enough to trigger the error at
488+ // the configured paytxfee and mintxfee levels.
489+ std::vector<CRecipient> recips = {};
490+ recips.resize(8000,CRecipient{GetScriptForRawPubKey({}), 5000, false /* subtract fee */});
, in a couple of places.
clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script to preempt whitespace nitpicking.
0wallet/test/wallet_tests.cpp:49:25: warning: unused variable 'result' [-Wunused-variable]
1 CWallet::ScanResult result = wallet->ScanForWalletTransactions(::ChainActive().Genesis()->GetBlockHash(), {} /* stop_block */, reserver, false /* update */);
Could use:
0BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
(line numbers are off because I saw this when running the new test case against the current master)
gcc version 7.4)
FTR I’m running with:
0$ gcc --version
1Apple LLVM version 10.0.1 (clang-1001.0.46.4)
2Target: x86_64-apple-darwin18.6.0
16+
17+ def run_test(self):
18+ self.nodes[0].generate(105)
19+
20+ outputs = {}
21+
