The walletcreatefundedpsbt RPC call currently ignores -walletrbf and defaults to not use RBF. This PR fixes that.
This PR also replaces UniValue in ConstructTransaction with a bool in preparation of moving this helper method out of the RPC codebase entirely. This may be a bit overkill, but does slightly simplify it.
Fixes #15878
(my initial attempt ddfce96d79809112d8253a8a82cc9bf11c3a1beb used Optional<Bool>) Given that wallet->m_signal_rbf is a boolean I think we should just stop treating replaceable as an optional parameter. In that case ConstructTransaction can take a boolean parameter and be simplified a bit.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
3978 | @@ -3979,7 +3979,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request) 3979 | {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."}, 3980 | }, 3981 | }, 3982 | - {"replaceable", RPCArg::Type::BOOL, /* default */ "false", "Marks this transaction as BIP125 replaceable.\n" 3983 | + {"replaceable", RPCArg::Type::BOOL, /* default */ "fallback to wallet's default", "Marks this transaction as BIP125 replaceable.\n"
"fallback to" seems unnecessary here.
Would need a scripted diff to change that?
$ git grep --count "fallback to wallet's default"
src/wallet/rpcwallet.cpp:7
I used the same wording as elsewhere, so could indeed do a replace-all.
Done, but not scripted.
4013 | @@ -4014,7 +4014,11 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request) 4014 | 4015 | CAmount fee; 4016 | int change_position; 4017 | - CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], request.params[3]["replaceable"]); 4018 | + bool rbf = pwallet->m_signal_rbf; 4019 | + if (!request.params[3]["replaceable"].isNull()) { 4020 | + rbf = request.params[3]["replaceable"].isTrue();
Probably ought to throw an error if it's not a boolean here.
Added a check.
utACK 9b1b9cf5632be18f3ad1bf2e10ebd004fceb2368
@Sjors Please squash the first and third commit, as they don't compile:
CXX wallet/libbitcoin_wallet_a-rpcwallet.o
wallet/rpcwallet.cpp: In function ‘UniValue walletcreatefundedpsbt(const JSONRPCRequest&)’:
wallet/rpcwallet.cpp:4137:143: error: cannot convert ‘const UniValue’ to ‘bool’
4137 | CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], request.params[3]["replaceable"]);
| ^
| |
| const UniValue
In file included from wallet/rpcwallet.cpp:20:
./rpc/rawtransaction_util.h:30:128: note: initializing argument 4 of ‘CMutableTransaction ConstructTransaction(const UniValue&, const UniValue&, const UniValue&, bool)’
30 | CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, bool rbf);
| ~~~~~^~~
@Sjors ^ :eyes:
Soon (TM), sorry for the delay.
Rebased and squashed first and last commit. @MarcoFalke hopefully good to go.
211 | assert "bip32_derivs" not in psbt_in 212 | assert_equal(decoded_psbt["tx"]["locktime"], block_height+2) 213 | 214 | - # Same construction with only locktime set 215 | - psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], block_height, {}, True) 216 | + # Same construction with only locktime set and RBF explictly enabled
nit: typo in explictly
Code review ACK f847fa961c29cf95416ba489d46d2a61368c7c61
Fixed the typo. You can see the diff with:
PREV=f847fa9 N=2 && git range-diff `git merge-base --all HEAD $PREV`...$PREV HEAD~$N...HEAD
17 | @@ -18,7 +18,7 @@ 18 | #include <util/rbf.h> 19 | #include <util/strencodings.h> 20 | 21 | -CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, const UniValue& rbf) 22 | +CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, bool rbf)
Will ConstructTransaction be moved out of the RPC module?
Without this change the issue could probably have been addressed with a small update in walletcreatefundedpsbt.
If ConstructTransaction stays in the RPC codebase forever then this change has also introduced duplicate code at two call sites for no reason.
I expect more stuff to be moved out of the RPC over time, but for now this was the simplest fix, also easiest to backport.
224 |
225 | # Same construction without optional arguments
226 | psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}])
227 | decoded_psbt = self.nodes[0].decodepsbt(psbtx_info["psbt"])
228 | for tx_in in decoded_psbt["tx"]["vin"]:
229 | - assert tx_in["sequence"] > MAX_BIP125_RBF_SEQUENCE
Perhaps test the walletrbf=0 case assert tx_in["sequence"] > MAX_BIP125_RBF_SEQUENCE on nodes[1]?
Added a commit with that test (it shouldn't break existing ACKs)
ACK 4348388
+1 for finding this issue and fixing it. Feel free to ignore my review comments.
232 | 233 | + # Same construction without optional arguments, for a node with -walletrbf=0 234 | + unspent1 = self.nodes[1].listunspent()[0] 235 | + psbtx_info = self.nodes[1].walletcreatefundedpsbt([{"txid":unspent1["txid"], "vout":unspent1["vout"]}], [{self.nodes[2].getnewaddress():unspent1["amount"]+1}], block_height) 236 | + decoded_psbt = self.nodes[1].decodepsbt(psbtx_info["psbt"]) 237 | + for tx_in, psbt_in in zip(decoded_psbt["tx"]["vin"], decoded_psbt["inputs"]):
No need to zip here.
for tx_in in decoded_psbt["tx"]["vin"]:
assert tx_in["sequence"] > MAX_BIP125_RBF_SEQUENCE
17f0cb0f792883c791954472a3173bf46356ce09
Agree, same as L223 is enough.
223 | @@ -224,6 +224,13 @@ def run_test(self): 224 | assert_equal(tx_in["sequence"], MAX_BIP125_RBF_SEQUENCE) 225 | assert_equal(decoded_psbt["tx"]["locktime"], 0) 226 | 227 | + # Same construction without optional arguments, for a node with -walletrbf=0 228 | + unspent1 = self.nodes[1].listunspent()[0] 229 | + psbtx_info = self.nodes[1].walletcreatefundedpsbt([{"txid":unspent1["txid"], "vout":unspent1["vout"]}], [{self.nodes[2].getnewaddress():unspent1["amount"]+1}], block_height) 230 | + decoded_psbt = self.nodes[1].decodepsbt(psbtx_info["psbt"]) 231 | + for tx_in, psbt_in in zip(decoded_psbt["tx"]["vin"], decoded_psbt["inputs"]): 232 | + assert tx_in["sequence"] > MAX_BIP125_RBF_SEQUENCE
17f0cb0f792883c791954472a3173bf46356ce09
Use assert_greater_than.
201 | @@ -197,26 +202,26 @@ def run_test(self): 202 | # replaceable arg 203 | block_height = self.nodes[0].getblockcount() 204 | unspent = self.nodes[0].listunspent()[0] 205 | - psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], block_height+2, {"replaceable":True}, False) 206 | + psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], block_height+2, {"replaceable": False}, False) 207 | decoded_psbt = self.nodes[0].decodepsbt(psbtx_info["psbt"]) 208 | for tx_in, psbt_in in zip(decoded_psbt["tx"]["vin"], decoded_psbt["inputs"]): 209 | - assert_equal(tx_in["sequence"], MAX_BIP125_RBF_SEQUENCE) 210 | + assert tx_in["sequence"] > MAX_BIP125_RBF_SEQUENCE
43483889cfa7cb0aa95ba6bb298c9d644e4f21d0
Use assert_greater_than.
Concept ACK.
Moved to 0.18.2
Rebased and addressed nits.
Code Review ACK d6b3640ac732f6f66a8cb6761084d1beecc8a876
@MarcoFalke @l2a5b1 @meshcollider can you re-ACK?
re-ACK d6b3640
ACK d6b3640ac732f6f66a8cb6761084d1beecc8a876
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK d6b3640ac732f6f66a8cb6761084d1beecc8a876
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjtHQwAlvgQph2uP/hqvyh4EGclSdMka8jVYUY56nEWpMuESGsZdo+OecyPiahc
HP2P5wvPIwUS6LF+HXzCLw9yxZY6KKZIqYOO2nG2mcP9KycMvcncopG+QyqOUxp/
hFVueMAoNFo8xfpodLq7/LjEIWt32F5FHyaLr4jBQZcxJo5nFBGtwBpzRVcK00LE
+YdXVacxCu9Q74ET99AlbBbQk5kVBuIuK1TYFsPQzQFsgp+L0ysOx697VSDFOHEW
4RcTw/wrIbmg0ItVWq6Eq2ufJmHogY5mzurRPxli3ybm4+bSHVQbyiC+0sh++qx/
E1p03kuygA+lhImxwB3SpRBqUCyf+uR9vc7yMPjQ0p5izkOCNS5XHl31wwkGBl80
bJgF8KgqkkHQrHn/o/PzxLiixkIbZw0K+KU7+4Cl7r3Qio3b6SCCBF94tW7jg8hx
EsjpGUS7m+z4qZt91zPndcbCSxF5brfN/14LsVcYEoTF9YiU9Y6VbMSHEDrTG+vO
u+gEf0y3
=31Xg
-----END PGP SIGNATURE-----
Timestamp of file with hash 388665302574fab8386985975f35ab1e4733960dbf1495ad6009328b30e160eb -
</details>
