0.18: [wallet] abort when attempting to fund a transaction above maxtxfee

Sjors · promag · commit 8f354ced6e34 · 2019-08-25T02:48:30.000+01:00
FundTransaction calls GetMinimumFee which, when the fee rate is absurdly high, quietly reduced the fee to -maxtxfee. Becaue an absurdly high fee rate is usually the result of a fat finger, aborting seems safer behavior. Github-Pull: #16257 Rebased-From: 806b005
diff --git a/doc/release-notes-0.18.1-16257.md b/doc/release-notes-0.18.1-16257.md
@@ -0,0 +1,6 @@
+Wallet changes
+--------------
+When creating a transaction with a fee above `-maxtxfee` (default 0.1 BTC),
+the RPC commands `walletcreatefundedpsbt` and  `fundrawtransaction` will now fail
+instead of rounding down the fee. Beware that the `feeRate` argument is specified
+in BTC per kilobyte, not satoshi per byte.
diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp
@@ -38,7 +38,6 @@ std::string StringForFeeReason(FeeReason reason) {
         {FeeReason::PAYTXFEE, "PayTxFee set"},
         {FeeReason::FALLBACK, "Fallback fee"},
         {FeeReason::REQUIRED, "Minimum Required Fee"},
-        {FeeReason::MAXTXFEE, "MaxTxFee limit"}
     };
     auto reason_string = fee_reason_strings.find(reason);
 
diff --git a/src/policy/fees.h b/src/policy/fees.h
@@ -43,7 +43,6 @@ enum class FeeReason {
     PAYTXFEE,
     FALLBACK,
     REQUIRED,
-    MAXTXFEE,
 };
 
 std::string StringForFeeReason(FeeReason reason);
diff --git a/src/wallet/fees.cpp b/src/wallet/fees.cpp
@@ -21,13 +21,7 @@ CAmount GetRequiredFee(const CWallet& wallet, unsigned int nTxBytes)
 
 CAmount GetMinimumFee(const CWallet& wallet, unsigned int nTxBytes, const CCoinControl& coin_control, const CTxMemPool& pool, const CBlockPolicyEstimator& estimator, FeeCalculation* feeCalc)
 {
-    CAmount fee_needed = GetMinimumFeeRate(wallet, coin_control, pool, estimator, feeCalc).GetFee(nTxBytes);
-    // Always obey the maximum
-    if (fee_needed > maxTxFee) {
-        fee_needed = maxTxFee;
-        if (feeCalc) feeCalc->reason = FeeReason::MAXTXFEE;
-    }
-    return fee_needed;
+    return GetMinimumFeeRate(wallet, coin_control, pool, estimator, feeCalc).GetFee(nTxBytes);
 }
 
 CFeeRate GetRequiredFeeRate(const CWallet& wallet)
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -2690,6 +2690,11 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
         }
     }
 
+    if (nFeeRet > maxTxFee) {
+        strFailReason = _("Fee exceeds maximum configured by -maxtxfee");
+        return false;
+    }
+
     return true;
 }
 
diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py
@@ -662,6 +662,7 @@ def run_test(self):
         result = self.nodes[3].fundrawtransaction(rawtx) # uses min_relay_tx_fee (set by settxfee)
         result2 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2*min_relay_tx_fee})
         result3 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 10*min_relay_tx_fee})
+        assert_raises_rpc_error(-4, "Fee exceeds maximum configured by -maxtxfee", self.nodes[3].fundrawtransaction, rawtx, {"feeRate": 1})
         result_fee_rate = result['fee'] * 1000 / count_bytes(result['hex'])
         assert_fee_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate)
         assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate)
diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
@@ -128,6 +128,15 @@ def run_test(self):
         assert_equal(walletprocesspsbt_out['complete'], True)
         self.nodes[1].sendrawtransaction(self.nodes[1].finalizepsbt(walletprocesspsbt_out['psbt'])['hex'])
 
+        # feeRate of 0.1 BTC / KB produces a total fee slightly below -maxtxfee (~0.05280000):
+        res = 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": 0.1})
+        assert_greater_than(res["fee"], 0.05)
+        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
+        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
         psbtx = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2wsh_pos},{"txid":txid,"vout":p2sh_pos},{"txid":txid,"vout":p2sh_p2wsh_pos}], {self.nodes[1].getnewaddress():29.99})['psbt']
         walletprocesspsbt_out = self.nodes[1].walletprocesspsbt(psbtx)

Original file line number	Diff line number	Diff line change
`@@ -21,13 +21,7 @@ CAmount GetRequiredFee(const CWallet& wallet, unsigned int nTxBytes)`
`21`	`21`
`22`	`22`	`CAmount GetMinimumFee(const CWallet& wallet, unsigned int nTxBytes, const CCoinControl& coin_control, const CTxMemPool& pool, const CBlockPolicyEstimator& estimator, FeeCalculation* feeCalc)`
`23`	`23`	`{`
`24`		`- CAmount fee_needed = GetMinimumFeeRate(wallet, coin_control, pool, estimator, feeCalc).GetFee(nTxBytes);`
`25`		`- // Always obey the maximum`
`26`		`- if (fee_needed > maxTxFee) {`
`27`		`- fee_needed = maxTxFee;`
`28`		`- if (feeCalc) feeCalc->reason = FeeReason::MAXTXFEE;`
`29`		`- }`
`30`		`- return fee_needed;`
	`24`	`+ return GetMinimumFeeRate(wallet, coin_control, pool, estimator, feeCalc).GetFee(nTxBytes);`
`31`	`25`	`}`
`32`	`26`
`33`	`27`	`CFeeRate GetRequiredFeeRate(const CWallet& wallet)`
Original file line number	Diff line number	Diff line change
`@@ -2690,6 +2690,11 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC`
`2690`	`2690`	`}`
`2691`	`2691`	`}`
`2692`	`2692`
	`2693`	`+ if (nFeeRet > maxTxFee) {`
	`2694`	`+ strFailReason = _("Fee exceeds maximum configured by -maxtxfee");`
	`2695`	`+ return false;`
	`2696`	`+ }`
	`2697`	`+`
`2693`	`2698`	`return true;`
`2694`	`2699`	`}`
`2695`	`2700`