Merge #16322: wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction

0d101a340c44841cbbc5982d55354b1787bc39e2 test: Add test for maxtxfee option (MarcoFalke)
177550101b600ccb32886695326eb72cd9752c8b wallet: Remove unreachable code in CreateTransaction (MarcoFalke)
5c1b9714cb0a13be28324f91f4ec9ca66a1de8c7 wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction (João Barbosa)

Pull request description:

  Follow up to #16257, this PR makes `bumpfee` aware of `-maxtxfee`.

  It also prevents dangling locked unspents when calling `fundrawtransaction` - because the previous check was after `LockCoin`.

ACKs for top commit:
  MarcoFalke:
    re-ACK 0d101a340c44841cbbc5982d55354b1787bc39e2, only change is small test fixup

Tree-SHA512: 3464b24ae7cd4e72ed41438c6661828ba1304af020f05da62720b23668ae734e16cf47c6d97e150cc84ef631ee099b16fc786c858f3d089905845437338fd512

src/qt/walletmodel.cpp

@@ -269,9 +269,12 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
         return TransactionCreationFailed;
     }
 
-    // Reject absurdly high fee
-    if (nFeeRequired > m_wallet->getDefaultMaxTxFee())
+    // Reject absurdly high fee. (This can never happen because the
+    // wallet never creates transactions with fee greater than
+    // m_default_max_tx_fee. This merely a belt-and-suspenders check).
+    if (nFeeRequired > m_wallet->getDefaultMaxTxFee()) {
         return AbsurdFee;
+    }
 
     return SendCoinsReturn(OK);
 }

src/wallet/wallet.cpp

@@ -3236,11 +3236,6 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
         }
     }
 
-    if (nFeeRet > m_default_max_tx_fee) {
-        strFailReason = TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED);
-        return false;
-    }
-
     return true;
 }
 
@@ -3724,13 +3719,6 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
 
                     nFee = GetMinimumFee(*this, nBytes, coin_control, &feeCalc);
 
-                    // If we made it here and we aren't even able to meet the relay fee on the next pass, give up
-                    // because we must be at the maximum allowed fee.
-                    if (nFee < ::minRelayTxFee.GetFee(nBytes)) {
-                        strFailReason = _("Transaction too large for fee policy");
-                        return false;
-                    }
-
                     return true;
                 };
 
@@ -3902,6 +3890,11 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
         tx = MakeTransactionRef(std::move(txNew));
     }
 
+    if (nFeeRet > m_default_max_tx_fee) {
+        strFailReason = TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED);
+        return false;
+    }
+
     if (gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) {
         // Lastly, ensure this tx will pass the mempool's chain limits
         if (!chain().checkChainLimits(tx)) {

test/functional/wallet_create_tx.py

@@ -6,6 +6,7 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
+    assert_raises_rpc_error,
 )
 
 
@@ -18,6 +19,10 @@ class CreateTxWalletTest(BitcoinTestFramework):
         self.skip_if_no_wallet()
 
     def run_test(self):
+        self.test_anti_fee_sniping()
+        self.test_tx_size_too_large()
+
+    def test_anti_fee_sniping(self):
         self.log.info('Check that we have some (old) blocks and that anti-fee-sniping is disabled')
         self.bump_mocktime(8 * 60 * 60 + 1)
         assert_equal(self.nodes[0].getblockchaininfo()['blocks'], 200)
@@ -31,6 +36,40 @@ class CreateTxWalletTest(BitcoinTestFramework):
         tx = self.nodes[0].decoderawtransaction(self.nodes[0].gettransaction(txid)['hex'])
         assert 0 < tx['locktime'] <= 201
 
+    def test_tx_size_too_large(self):
+        # More than 10kB of outputs, so that we hit -maxtxfee with a high feerate
+        outputs = {self.nodes[0].getnewaddress(): 0.000025 for i in range(400)}
+        raw_tx = self.nodes[0].createrawtransaction(inputs=[], outputs=outputs)
+
+        for fee_setting in ['-minrelaytxfee=0.01', '-mintxfee=0.01', '-paytxfee=0.01']:
+            self.log.info('Check maxtxfee in combination with {}'.format(fee_setting))
+            self.restart_node(0, extra_args=[fee_setting])
+            assert_raises_rpc_error(
+                -6,
+                "Fee exceeds maximum configured by -maxtxfee",
+                lambda: self.nodes[0].sendmany(dummy="", amounts=outputs),
+            )
+            assert_raises_rpc_error(
+                -4,
+                "Fee exceeds maximum configured by -maxtxfee",
+                lambda: self.nodes[0].fundrawtransaction(hexstring=raw_tx),
+            )
+
+        self.log.info('Check maxtxfee in combination with settxfee')
+        self.restart_node(0)
+        self.nodes[0].settxfee(0.01)
+        assert_raises_rpc_error(
+            -6,
+            "Fee exceeds maximum configured by -maxtxfee",
+            lambda: self.nodes[0].sendmany(dummy="", amounts=outputs),
+        )
+        assert_raises_rpc_error(
+            -4,
+            "Fee exceeds maximum configured by -maxtxfee",
+            lambda: self.nodes[0].fundrawtransaction(hexstring=raw_tx),
+        )
+        self.nodes[0].settxfee(0)
+
 
 if __name__ == '__main__':
     CreateTxWalletTest().main()

test/functional/wallet_fallbackfee.py

@@ -21,7 +21,6 @@ class WalletRBFTest(BitcoinTestFramework):
         self.restart_node(0, extra_args=["-fallbackfee=0"])
         assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1))
         assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].fundrawtransaction(self.nodes[0].createrawtransaction([], {self.nodes[0].getnewaddress(): 1})))
-        assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].sendfrom("", self.nodes[0].getnewaddress(), 1))
         assert_raises_rpc_error(-6, "Fee estimation failed", lambda: self.nodes[0].sendmany("", {self.nodes[0].getnewaddress(): 1}))
 
 if __name__ == '__main__':