wallet: allow zero-fee fundrawtransaction/walletcreatefundedpsbt and other fixes

jonatack commented at 6:21 pm on November 19, 2020: member

Fixes https://github.com/bitcoin/bitcoin/pull/20305/files#r525406176. A check to raise an error on zero-fee txns was mistakenly extended in a0d4957 from the bumpfee and send{toaddress, many} RPCs to also include fundrawtransaction and walletcreatefundedpsbt. This commit re-overrides zero fee rate checking for these two RPCs, not only for the feeRate (BTC/kvB) arg to return to previous behavior, but also for the new fee_rate (sat/vB) arg. Negative fee rates will still raise “amount out of range” by the MoneyRange check in src/bitcoin-tx.cpp::AmountFromValue.
Updates a wallet bumpfee test from feeRate (BTC/kvB) to fee_rate (sat/vB)
Fixes https://github.com/bitcoin/bitcoin/pull/20305/files#r525405363 to use the correct incremental fee rate constant in the bumpfee help (thanks Marco Falke for the catch) and rectifies “1.000 sat/vB sat/vB” in the help to “1.000 sat/vB”

jonatack force-pushed on Nov 19, 2020

in src/wallet/rpcwallet.cpp:222 in 3856f0fdc4 outdated

226-            }
227-            cc.fOverrideFeeRate = true;
228-        }
229-        cc.m_feerate = fee_rate_in_sat_vb;
230+        cc.m_feerate = CFeeRate(AmountFromValue(fee_rate), COIN);
231+        if (override_min_fee) cc.fOverrideFeeRate = true;

Xekyo commented at 6:25 pm on November 19, 2020:

I see that we’re no longer checking for smaller than zero. Are we just relying on that being invalid for a transaction in the first place?

jonatack commented at 6:29 pm on November 19, 2020:

Thanks for the quick review! AmountFromValue does raise on negative values in its MoneyRange check and we should already have tests for that. I’ll double check. Edit: found test coverage improvements we can make.

Xekyo approved

Xekyo commented at 6:27 pm on November 19, 2020: member

utACK

MarcoFalke commented at 6:27 pm on November 19, 2020: member

in commit 3856f0fdc41fa72e8123f5ba64077d62e8163b4f: Could add test for

0-fee for send* rpcs, and
negative fee for all rpcs?

jonatack commented at 6:30 pm on November 19, 2020: member

I think we already have these tests, added in #20305. Will re-verify.

DrahtBot added the label RPC/REST/ZMQ on Nov 19, 2020

DrahtBot added the label Wallet on Nov 19, 2020

jonatack commented at 6:49 pm on November 19, 2020: member

Found some test coverage improvements to make, thanks @Xekyo and @MarcoFalke. Updated.

jonatack force-pushed on Nov 19, 2020

MarcoFalke commented at 7:00 pm on November 19, 2020: member

Concept ACK and thanks. Will review when ci is green

laanwj added this to the milestone 0.19.2 on Nov 19, 2020

laanwj removed this from the milestone 0.19.2 on Nov 19, 2020

MarcoFalke added this to the milestone 0.21.0 on Nov 19, 2020

MarcoFalke added the label Needs backport (0.21) on Nov 19, 2020

MarcoFalke commented at 7:47 pm on November 19, 2020: member

ci:

0�[0m�[0;31mwallet_bumpfee.py                                | ✖ Failed  | 30 s

jonatack commented at 7:49 pm on November 19, 2020: member

0substring: 'Insufficient total fee 0.00000141, must be at least 0.00001704 (oldFee 0.00000999 + incrementalFee 0.00000705)'
1error message: 'Insufficient total fee 0.00000141, must be at least 0.00001712 (oldFee 0.00001007 + incrementalFee 0.00000705)'.

jonatack commented at 9:01 pm on November 19, 2020: member

Restarted the tasks, they pass, and the failure looks unrelated to this changeset. I can push a patch to loosen the test, though, if wanted.

MarcoFalke commented at 8:24 am on November 20, 2020: member

review ACK a02349dc8250fc22e0cd754b45f379210fbc6e37 did not test 🐟

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK a02349dc8250fc22e0cd754b45f379210fbc6e37 did not test 🐟
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjBDQv+Mogn85V5yxQ2/9YheV57DThEoDMEMllRJi/uc/OQBdQbtq6Kyv2j/6U+
 8RKE2phLjWZJOlZJk6ip40srFiJ/wuwMKSoGm8/0eWGowncSAVpMRGqKp1ffH27dO
 99OMoqyI7dAUbciko/liaQoA+Q1xvLpdg24APW5W97ZVhtVfgh3G0nvzkP7w65sUF
10E/WrRvuNbSnG4K2ZNefTGlKLBPUAlH85QUXbQKxcg8GLVn4xNRnWdENaxndGGBXs
11m8LNOmshBgT+86X8ODdcArx1BdDQ8wiC9ldrIzaxiGd5CrrBmiNVmBnJ7zbOSQqw
12RUq3PtBUymw42l2tgIjRG4bIrMdZRr9to4TiiTKcIkQ1dhDbBSWth/beaiHgvX96
13Q8HKPbg/ZWXC6NammYIihG9A5FjlYpzpYpScEGrxBgs7XByaCcRplNrvftSwnlMQ
14zL8DjLDAOyGvbgktnnGqH+zg7BiqvGJ1t6qQmeMjt8kMeg7V37vb8STb49O0hKkj
15HRIX8M36
16=j1gS
17-----END PGP SIGNATURE-----

python: error while loading shared libraries: libpython3.8.so.1.0: cannot open shared object file: No such file or directory

in test/functional/rpc_psbt.py:204 in a02349dc82 outdated

199         res4 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.00000999, "add_inputs": True})
200         assert_approx(res4["fee"], 0.00000381, 0.0000001)
201 
202+        self.log.info("Test min fee rate checks with walletcreatefundedpsbt are bypassed and that funding non-standard 'zero-fee' transactions is valid")
203+        for param in ["fee_rate", "feeRate"]:
204+            assert_equal(self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"fee_rate": 0, "add_inputs": True})["fee"], 0)

jonatack commented at 8:35 am on November 20, 2020:

0            assert_equal(self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {param: 0, "add_inputs": True})["fee"], 0)

jonatack commented at 8:48 am on November 20, 2020:

Updated.

Allow zero-fee fundrawtxn and walletcreatefundedpsbt calls

A check to raise an error on zero-fee txns was mistakenly extended in commit
a0d4957 from the bumpfee and send{toaddress, many} RPCs to also include
fundrawtransaction and walletcreatefundedpsbt.

This commit overrides zero fee rate checking for these two RPCs, not only for
the feeRate (BTC/kvB) arg to return to previous behavior, but also for the new
fee_rate (sat/vB) arg.

1b3d700928

Update feeRate (BTC/kvB) to fee_rate (sat/vB) in wallet_bumpfee

as the feeRate argument should soon be deprecated.

Also loosen one test (and a similar one) that caused a one-off CI failure with:
expected message
'Insufficient total fee 0.00000141, must be at least 0.00001704 (oldFee 0.00000999 + incrementalFee 0.00000705)'
actual message
'Insufficient total fee 0.00000141, must be at least 0.00001712 (oldFee 0.00001007 + incrementalFee 0.00000705)'

3f1e10b2b1

Use the correct incremental fee constant in bumpfee help

and remove redundant units ("Must be at least 1.000 sat/vB sat/vB" -> "1.00 sat vB")

9f08780dd7

jonatack force-pushed on Nov 20, 2020

jonatack commented at 8:54 am on November 20, 2020: member

Latest push:

addition to first commit: fix a test in rpc_psbt.py
addition to second commit: loosen two tests in wallet_bumpfee.py that may cause spurious CI failures
addition to third commit: fix “sat vB sat vB” in the bumpfee help

diff git diff a02349d 9f08780:

 0diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
 1index a39e8a4e30..dd75e876bc 100644
 2--- a/src/wallet/rpcwallet.cpp
 3+++ b/src/wallet/rpcwallet.cpp
 4@@ -3405,7 +3405,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
 5                     {"fee_rate", RPCArg::Type::AMOUNT, /* default */ "not set, fall back to wallet fee estimation",
 6                              "\nSpecify a fee rate in " + CURRENCY_ATOM + "/vB instead of relying on the built-in fee estimator.\n"
 7-                             "Must be at least " + incremental_fee + " " + CURRENCY_ATOM + "/vB higher than the current transaction fee rate.\n"
 8+                             "Must be at least " + incremental_fee + " higher than the current transaction fee rate.\n"
 9                              "WARNING: before version 0.21, fee_rate was in " + CURRENCY_UNIT + "/kvB. As of 0.21, fee_rate is in " + CURRENCY_ATOM + "/vB.\n"},
10diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
11index 0afa9d7835..5840801b00 100755
12--- a/test/functional/rpc_psbt.py
13+++ b/test/functional/rpc_psbt.py
14@@ -201,7 +201,7 @@ class PSBTTest(BitcoinTestFramework):
15 
16         self.log.info("Test min fee rate checks with walletcreatefundedpsbt are bypassed and that funding non-standard 'zero-fee' transactions is valid")
17         for param in ["fee_rate", "feeRate"]:
18-            assert_equal(self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"fee_rate": 0, "add_inputs": True})["fee"], 0)
19+            assert_equal(self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {param: 0, "add_inputs": True})["fee"], 0) 
20diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py
21index fab3ab1e27..99c9737258 100755
22--- a/test/functional/wallet_bumpfee.py
23+++ b/test/functional/wallet_bumpfee.py
24@@ -107,12 +107,10 @@ class BumpFeeTest(BitcoinTestFramework):
25         # Bumping to just above minrelay should fail to increase the total fee enough.
26-        assert_raises_rpc_error(-8, "Insufficient total fee 0.00000141, must be at least 0.00001704 (oldFee 0.00000999 + incrementalFee 0.00000705)",
27-            rbf_node.bumpfee, rbfid, {"fee_rate": INSUFFICIENT})
28+        assert_raises_rpc_error(-8, "Insufficient total fee 0.00000141", rbf_node.bumpfee, rbfid, {"fee_rate": INSUFFICIENT})
29 
30         self.log.info("Test invalid fee rate settings")
31-        assert_raises_rpc_error(-8, "Insufficient total fee 0.00, must be at least 0.00001704 (oldFee 0.00000999 + incrementalFee 0.00000705)",
32-            rbf_node.bumpfee, rbfid, {"fee_rate": 0})
33+        assert_raises_rpc_error(-8, "Insufficient total fee 0.00", rbf_node.bumpfee, rbfid, {"fee_rate": 0})
34         assert_raises_rpc_error(-4, "Specified or calculated fee 0.141 is too high (cannot be higher than -maxtxfee 0.10",
35             rbf_node.bumpfee, rbfid, {"fee_rate": TOO_HIGH})

MarcoFalke commented at 10:07 am on November 20, 2020: member

review ACK 9f08780dd7946b63476e9736745131db8e7f4e93 🐾

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 9f08780dd7946b63476e9736745131db8e7f4e93  🐾
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYEACgkQzit1aX5p
 7pUizQQwAgefxPHhloYKwmNmmM14WEXjPc2tk28ufFSUqXP2V8lgfpOqZ0bGt/ivi
 8A39fyIC+jgJeACH4WlCC7EPFURgVTDpdUQtHhkCS3Ybl6jEAJ4d5S63Il8nhWLix
 9LsFFIgZHLRZyGEflQszOWiYUJhtHZrk8aTIX4GfeZbc4dDdIfbx1/LV34yuROYA9
10A7ts9zcKuYk8F9SYSNNFTRXU0idA4NJJ3aLE8KBPUf3C/tev8W+cVqccBk6sp0xG
11eNeOGdPi1ROjrnIW6QdsiAPfyX7p8pUMeLbz0DT+/jvOMLc7BvX7H0QP1hLzA7HL
12hWvVWHzhxfnnwMftIF0aXpJ87M+kX8Jq+pKZXZ2TOKWsrYYhI4eMwQvtnZnoHhIT
13XBkUwqZNuesaTADUoRimEhAHrD5lfb30HFOaCY3KziUfmGSLh5/C7WgQmULgN3sa
14AvrbWnMYfHe/4MtT67K9w2jAa1HWwgAuYCOLGddR3F3jCCGZ7lu9AYfzaDwUSvze
156IKK1sRg
16=NG4i
17-----END PGP SIGNATURE-----

python: error while loading shared libraries: libpython3.8.so.1.0: cannot open shared object file: No such file or directory

promag commented at 2:59 pm on November 20, 2020: member

Code review ACK 9f08780dd7946b63476e9736745131db8e7f4e93.

Maybe makes sense to eventually add ParseFeeRate(const UniValue&) .

Xekyo commented at 9:59 pm on November 20, 2020: member

Code review reACK 9f08780dd7946b63476e9736745131db8e7f4e93.

MarcoFalke merged this on Nov 21, 2020

MarcoFalke closed this on Nov 21, 2020

jonatack deleted the branch on Nov 21, 2020

sidhujag referenced this in commit f950ab5dc3 on Nov 21, 2020

jonatack commented at 4:51 pm on November 26, 2020: member

Backported to 0.21 by #20510.

Needs backport (0.21) tag can be removed.

jonatack referenced this in commit 54e1edcc2b on Nov 26, 2020

jonatack referenced this in commit 6e4969f76f on Nov 26, 2020

jonatack referenced this in commit 6313362553 on Nov 26, 2020

MarcoFalke removed the label Needs backport (0.21) on Dec 4, 2020

MarcoFalke commented at 5:16 pm on December 4, 2020: member

Backported in #20510

MarcoFalke referenced this in commit aa4b8ebfec on Dec 4, 2020

DrahtBot locked this on Feb 15, 2022

wallet: allow zero-fee fundrawtransaction/walletcreatefundedpsbt and other fixes #20426