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
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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"
Would need a scripted diff to change that?
0$ git grep --count "fallback to wallet's default"
1src/wallet/rpcwallet.cpp:7
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();
@Sjors Please squash the first and third commit, as they don’t compile:
0 CXX wallet/libbitcoin_wallet_a-rpcwallet.o
1wallet/rpcwallet.cpp: In function ‘UniValue walletcreatefundedpsbt(const JSONRPCRequest&)’:
2wallet/rpcwallet.cpp:4137:143: error: cannot convert ‘const UniValue’ to ‘bool’
3 4137 | CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], request.params[3]["replaceable"]);
4 | ^
5 | |
6 | const UniValue
7In file included from wallet/rpcwallet.cpp:20:
8./rpc/rawtransaction_util.h:30:128: note: initializing argument 4 of ‘CMutableTransaction ConstructTransaction(const UniValue&, const UniValue&, const UniValue&, bool)’
9 30 | CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, bool rbf);
10 | ~~~~~^~~
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
explictly
Fixed the typo. You can see the diff with:
0PREV=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.
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
walletrbf=0 case assert tx_in["sequence"] > MAX_BIP125_RBF_SEQUENCE on nodes[1]?
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.
0for tx_in in decoded_psbt["tx"]["vin"]:
1 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.
ACK d6b3640ac732f6f66a8cb6761084d1beecc8a876
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK d6b3640ac732f6f66a8cb6761084d1beecc8a876
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUjtHQwAlvgQph2uP/hqvyh4EGclSdMka8jVYUY56nEWpMuESGsZdo+OecyPiahc
8HP2P5wvPIwUS6LF+HXzCLw9yxZY6KKZIqYOO2nG2mcP9KycMvcncopG+QyqOUxp/
9hFVueMAoNFo8xfpodLq7/LjEIWt32F5FHyaLr4jBQZcxJo5nFBGtwBpzRVcK00LE
10+YdXVacxCu9Q74ET99AlbBbQk5kVBuIuK1TYFsPQzQFsgp+L0ysOx697VSDFOHEW
114RcTw/wrIbmg0ItVWq6Eq2ufJmHogY5mzurRPxli3ybm4+bSHVQbyiC+0sh++qx/
12E1p03kuygA+lhImxwB3SpRBqUCyf+uR9vc7yMPjQ0p5izkOCNS5XHl31wwkGBl80
13bJgF8KgqkkHQrHn/o/PzxLiixkIbZw0K+KU7+4Cl7r3Qio3b6SCCBF94tW7jg8hx
14EsjpGUS7m+z4qZt91zPndcbCSxF5brfN/14LsVcYEoTF9YiU9Y6VbMSHEDrTG+vO
15u+gEf0y3
16=31Xg
17-----END PGP SIGNATURE-----
Timestamp of file with hash 388665302574fab8386985975f35ab1e4733960dbf1495ad6009328b30e160eb -
Labels
Wallet
RPC/REST/ZMQ
Milestone
0.18.2