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

0d101a3 test: Add test for maxtxfee option (MarcoFalke)
1775501 wallet: Remove unreachable code in CreateTransaction (MarcoFalke)
5c1b971 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 0d101a3, only change is small test fixup

Tree-SHA512: 3464b24ae7cd4e72ed41438c6661828ba1304af020f05da62720b23668ae734e16cf47c6d97e150cc84ef631ee099b16fc786c858f3d089905845437338fd512

Author: laanwj

src/qt/walletmodel.cpp

@@ -221,9 +221,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

@@ -2696,11 +2696,6 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
         }
     }
 
-    if (nFeeRet > this->m_default_max_tx_fee) {
-        strFailReason = TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED);
-        return false;
-    }
-
     return true;
 }
 
@@ -3010,14 +3005,6 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
                     return false;
                 }
 
-                // 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 (nFeeNeeded < chain().relayMinFee().GetFee(nBytes))
-                {
-                    strFailReason = _("Transaction too large for fee policy");
-                    return false;
-                }
-
                 if (nFeeRet >= nFeeNeeded) {
                     // Reduce fee to only the needed amount if possible. This
                     // prevents potential overpayment in fees if the coins
@@ -3134,6 +3121,11 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
         }
     }
 
+    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/rpc_psbt.py

@@ -136,7 +136,7 @@ def run_test(self):
         assert_greater_than(0.06, res["fee"])
 
         # feeRate of 10 BTC / KB produces a total fee well above -maxtxfee
-        # previously this was silenty capped at -maxtxfee
+        # previously this was silently capped at -maxtxfee
         assert_raises_rpc_error(-4, "Fee exceeds maximum configured by -maxtxfee", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 10})
 
         # partially sign multisig things with node 1

test/functional/wallet_bumpfee.py

@@ -80,6 +80,7 @@ def run_test(self):
         test_bumpfee_metadata(rbf_node, dest_address)
         test_locked_wallet_fails(rbf_node, dest_address)
         test_change_script_match(rbf_node, dest_address)
+        test_maxtxfee_fails(self, rbf_node, dest_address)
         # These tests wipe out a number of utxos that are expected in other tests
         test_small_output_with_feerate_succeeds(rbf_node, dest_address)
         test_no_more_inputs_fails(rbf_node, dest_address)
@@ -248,6 +249,15 @@ def test_settxfee(rbf_node, dest_address):
     rbf_node.settxfee(Decimal("0.00000000"))  # unset paytxfee
 
 
+def test_maxtxfee_fails(test, rbf_node, dest_address):
+    test.restart_node(1, ['-maxtxfee=0.00003'] + test.extra_args[1])
+    rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT)
+    rbfid = spend_one_input(rbf_node, dest_address)
+    assert_raises_rpc_error(-4, "Unable to create transaction: Fee exceeds maximum configured by -maxtxfee", rbf_node.bumpfee, rbfid)
+    test.restart_node(1, test.extra_args[1])
+    rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT)
+
+
 def test_rebumping(rbf_node, dest_address):
     # check that re-bumping the original tx fails, but bumping the bumper succeeds
     rbfid = spend_one_input(rbf_node, dest_address)

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,
 )
 from test_framework.blocktools import (
     TIME_GENESIS_BLOCK,
@@ -26,6 +27,10 @@ def run_test(self):
         self.nodes[0].generate(200)
         self.nodes[0].setmocktime(0)
 
+        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')
         assert_equal(self.nodes[0].getblockchaininfo()['blocks'], 200)
         txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)
@@ -38,6 +43,40 @@ def run_test(self):
         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(address_type='bech32'): 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()