Added support for creating v3 raw transaction:
- Overloaded to include additional parameter
this resolves #31348
Added support for creating v3 raw transaction:
this resolves #31348
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31936.
See the guideline for information on the review process.
Type | Reviewers |
---|---|
ACK | adamandrews1 |
Concept ACK | glozow, 1440000bytes |
Approach NACK | achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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.
153@@ -154,6 +154,13 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
154 rawTx.nLockTime = nLockTime;
155 }
156
157+ if (!version.isNull()) {
158+ int64_t nVersion = version.getInt<int64_t>();
159+ if (nVersion < 0 || nVersion > TRANSACTION_VERSION_MAX)
160+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, version out of range");
62@@ -63,6 +63,9 @@ static constexpr int64_t VALIDATION_WEIGHT_PER_SIGOP_PASSED{50};
63 // How much weight budget is added to the witness size (Tapscript only, see BIP 342).
64 static constexpr int64_t VALIDATION_WEIGHT_OFFSET{50};
65
66+// Maximum transaction version
67+static const int32_t TRANSACTION_VERSION_MAX = 3;
TX_MAX_STANDARD_VERSION
everywhere?
TX_MAX_STANDARD_VERSION
. It also appears to be unused, did you maybe forget to delete it?
153@@ -154,6 +154,13 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
154 rawTx.nLockTime = nLockTime;
155 }
156
157+ if (!version.isNull()) {
158+ int64_t nVersion = version.getInt<int64_t>();
int64_t
? version
has type uint32_t
.
int64_t
was my bad. However version
field of raw transaction seems to be int32_t
type according to bitcoint developer doc.. The doc explicitly states that the field is signed. Should I used unsigned instead?
Let’s use uint32_t
because that is what is used in both CTransaction
and CMutableTransaction
.
https://github.com/bitcoin/bitcoin/blob/72c150dfe7619c2b6e2fd06a1fa431b545a3a225/src/primitives/transaction.h#L308
Those docs might be outdated because this change was done recently: https://github.com/bitcoin/bitcoin/pull/29325
121@@ -122,6 +122,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
122 { "createrawtransaction", 1, "outputs" },
123 { "createrawtransaction", 2, "locktime" },
124 { "createrawtransaction", 3, "replaceable" },
125+ { "createrawtransaction", 4, "version" },
options
object here (used in many RPCs) would suffice for this use case.
@glozow WDYT?
258@@ -259,7 +259,11 @@ def createrawtransaction_tests(self):
259 assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [])
260
261 # Test `createrawtransaction` invalid extra parameters
262- assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [], {}, 0, False, 'foo')
263+ assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [], {}, 0, False, 2, 'foo')
264+
265+ # Test `createrawtransaction` invalid version parameters
266+ assert_raises_rpc_error(-8, "Invalid parameter, version out of range(0~3)", self.nodes[0].createrawtransaction, [], {}, 0, False, -1)
267+ assert_raises_rpc_error(-8, "Invalid parameter, version out of range(0~3)", self.nodes[0].createrawtransaction, [], {}, 0, False, 4)
raw_multisig_transaction_legacy_tests
but with version 3 raw transaction. This seems to cover all the domain you specified. Could you pls take a look?
https://github.com/bitcoin/bitcoin/blob/1121cb7a91e0dfa4f247273bf89e2e1b47c82628/test/functional/rpc_rawtransaction.py#L96
While updating the code, I noticed a couple of potential improvements.
No range validation tests for locktime
argument in createrawtransaction
RPC.
I added range validation tests for new version
argument. Should we extend this to locktime
as well?
createpsbt
RPC still creates only v2 raw transactions even though it shares same RPCHelpMan
with createrawtransaction
.
Should it support v3 raw transactions too?
Re: #31936 (comment)
There seems to be couple functional tests related to locktime out of range here: https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/test/functional/rpc_rawtransaction.py#L312-L313
Good point, I think it should but in a separate PR because along with it, we’d need to ensure that other PSBT related RPCs such as walletcreatefundedpsbt
among others work with v3 transactions as well. There’s a PSBT_GLOBAL_TX_VERSION
field as well in the PSBT that might need to be set to 3
, so I believe best to be in a separate PR.
This above point makes me realise that related to my previous comment #31936 (review), we can add a more comprehensive functional test for raw transaction related RPCs in this PR that creates a v3 raw transaction createrawtransaction
, funds a raw transaction fundrawtransaction
, decodes it decoderawtransaction
, signs it signrawtransactionwithwallet
, and then sends it sendrawtransaction
. This would ensure that the integration of v3 with all the related transaction RPCs gel together.
There’s a PSBT_GLOBAL_TX_VERSION field as well in the PSBT that might need to be set to 3
An update to my previous comment: #31936 (comment)
This can only done when PSBT V2 is implemented #21283, so no need for this at the moment.
153@@ -154,6 +154,13 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
154 rawTx.nLockTime = nLockTime;
155 }
156
157+ if (!version.isNull()) {
158+ int64_t nVersion = version.getInt<int64_t>();
159+ if (nVersion < 0 || nVersion > TX_MAX_STANDARD_VERSION)
53@@ -54,5 +54,6 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in);
54
55 /** Create a transaction from univalue parameters */
56 CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional<bool> rbf);
Is this comment outdated? I don’t understand where are the default args here that are referred to in the above comment.
In C++, function overloading can be used as a tool to set default values for function parameters. This is done here, by setting the default value in the function implementation, as opposed to in the function signature.
version
param, the function signature defaults can suffice.
I was trying to say in #31936 (review) that a default doesn’t make sense at all here (in any way), because:
Finally, default values in C++ function signatures or function overloads are problematic, when more defaults are added in the future, because there is no way to skip over one default value and only specify the other one. Modern languages, such as Rust don’t support this at all, or Python allows for named args.
Interesting, I like these points quite a lot. Summarising below, please correct me if I misunderstand:
We can avoid using defaults here completely by letting the corresponding version
default trickle down from the RPC help into these functions here.
As I checked in the call sites of CreateTransaction
, this^ requires other transaction creation RPCs to accept a version param as well such as createpsbt
, walletcreatefundedpsbt
, send
, sendall
, which seems consistent with an earlier suggestion here #31936 (review).
This increases the scope of the PR a bit but I feel it’s worth doing these changes as these RPCs are quite related to each other in terms of functionality. I can correspondingly update the issue description as well.
This also helps to make a case for using the options
argument in a previously open ended question here #31936 (review) because most of the above mentioned RPCs accept an option
param wherein the version
option can be added.
495@@ -491,7 +496,7 @@ def transaction_version_number_tests(self):
496 decrawtx = self.nodes[0].decoderawtransaction(rawtx)
497 assert_equal(decrawtx['version'], 0xffffffff)
498
499- def raw_multisig_transaction_legacy_tests(self):
500+ def raw_multisig_transaction_legacy_tests(self, version=2):
Although this works but there seems to be a note here regarding legacy wallets, which are being deprecated. I think a more suitable place could be in the getrawtransaction_verbosity_tests
that use create_self_transfer()
, which already accepts a version argument in the tests - just need to pass down 3.
Also, testing the value of the transaction version after being mined would complete the all-round testing of this functionality for which only adding a new field
for version in the getrawtransaction_verbosity_tests
subtest should suffice here: https://github.com/bitcoin/bitcoin/blob/199d47d9629b603d5a23b96c5260c68add00ccb3/test/functional/rpc_rawtransaction.py#L187
180@@ -180,6 +181,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
181 { "createpsbt", 1, "outputs" },
182 { "createpsbt", 2, "locktime" },
183 { "createpsbt", 3, "replaceable" },
184+ { "createpsbt", 4, "version" },
If accepting a new argument can’t be avoided for this PSBT RPC because of the same CreateTxDoc()
being reused underneath, then I think we can explore updating functional tests for PSBT in this PR itself because createpsbt
RPC uses the same ConstructTransaction()
as well.
rpc_psbt.py
is the file that would need to be updated. Using a similar approach like done in the rpc_rawtransaction
would be preferred that is reusing bunch of the existing testing methods.
Are you still working on this?
sure!
143@@ -144,6 +144,7 @@ std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate);
144 // Changing the default transaction version requires a two step process: first
145 // adapting relay policy by bumping TX_MAX_STANDARD_VERSION, and then later
146 // allowing the new transaction version in the wallet/RPC.
147+static constexpr decltype(CTransaction::version) TX_MIN_STANDARD_VERSION{1};
157@@ -158,6 +158,7 @@ static std::vector<RPCArg> CreateTxDoc()
158 {"locktime", RPCArg::Type::NUM, RPCArg::Default{0}, "Raw locktime. Non-0 value also locktime-activates inputs"},
159 {"replaceable", RPCArg::Type::BOOL, RPCArg::Default{true}, "Marks this transaction as BIP125-replaceable.\n"
160 "Allows this transaction to be replaced by a transaction with higher fees. If provided, it is an error if explicit sequence numbers are incompatible."},
161+ {"version", RPCArg::Type::NUM, RPCArg::Default{2}, "Transaction version"},
CURRENT_VERSION
instead of magic number
Resolves code review. The following tests are failing, but I will fix them soon.
0rpc_psbt.py --descriptors | ✖ Failed | 12 s
1tool_wallet.py --descriptors | ✖ Failed | 6 s
2wallet_bumpfee.py --descriptors | ✖ Failed | 11 s
3wallet_create_tx.py --descriptors | ✖ Failed | 2 s
4wallet_fundrawtransaction.py --descriptors | ✖ Failed | 5 s
5wallet_miniscript.py --descriptors | ✖ Failed | 1 s
6wallet_resendwallettransactions.py --descriptors | ✖ Failed | 13 s
7wallet_send.py --descriptors | ✖ Failed | 14 s
8wallet_spend_unconfirmed.py | ✖ Failed | 1 s
9wallet_taproot.py --descriptors | ✖ Failed | 8 s
10wallet_txn_clone.py | ✖ Failed | 1 s
11wallet_txn_clone.py --mineblock | ✖ Failed | 1 s
12wallet_txn_clone.py --segwit | ✖ Failed | 1 s
13wallet_txn_doublespend.py --descriptors | ✖ Failed | 1 s
14wallet_txn_doublespend.py --mineblock | ✖ Failed | 1 s
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40031572119
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
1294@@ -1295,7 +1295,7 @@ RPCHelpMan send()
1295 ParseOutputs(outputs),
1296 InterpretSubtractFeeFromOutputInstructions(options["subtract_fee_from_outputs"], outputs.getKeys())
1297 );
1298- CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf);
1299+ CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf, 3);
CI is still failing @glozow Resolves Code Review! PTAL!! I have created a new commit to add the TX_MIN_VERSION parameter. I am concerned that this change violates the rule of creating only one commit per PR. The tests were failing because spend.cpp was using version 3, while rawtransaction.cpp was using version 2. I fixed the test failures by modifying both to use version 2. If you want to use version 3 of rawtransaction, it should be possible since the method accepts the version as a parameter. There is no constant value to replace version 2, so the use of magic numbers has been retained.
Re: #31936 (comment)
I am concerned that this change violates the rule of creating only one commit per PR.
I don’t think this is true because this rule seems quite limiting, and there are many PRs with more than one commit. Is this mentioned somewhere in the repo? We can get it updated separately.
Re: #31936 (comment)
I am concerned that this change violates the rule of creating only one commit per PR.
I don’t think this is true because this rule seems quite limiting, and there are many PRs with more than one commit. Is this mentioned somewhere in the repo? We can get it updated separately.
I misunderstood. I thought the concept of “atomic commits” meant having one commit per PR. I didn’t realize it meant “you shouldn’t modify the same line across multiple commits. link
@glozow To avoid using magic numbers, I used the CTransaction::CURRENT_VERSION constant, but this change caused the CI to fail. I think we might need to add a commit to introduce a new constant. What do you think?
[20:44:30.954] /ci_container_base/ci/scratch/build-i686-pc-linux-gnu/src/./rpc/rawtransaction.cpp:161:(.text+0x29162): undefined reference to
CTransaction::CURRENT_VERSION'`
@glozow To avoid using magic numbers, I used the CTransaction::CURRENT_VERSION constant, but this change caused the CI to fail.
You can declare a DEFAULT_RAWTX_VERSION{CTransaction::CURRENT_VERSION}
Also, I think you should swap the commits: first assign the minimum standard version to a named constant, and then use it. Currently, you are introducing a magic number and then renaming it to MIN_VERSION
- that seems backwards. This is a nit, but this order defeats the purpose of having a separate commit imo.
Changes made to rpc_rawtransaction.py
in https://github.com/bitcoin/bitcoin/pull/31936/commits/d0453049f5d6678096f3575f6d595467506a05f1 in the functions raw_multisig_transaction_legacy_tests
and raw_multisig_transaction_legacy_tests
does not have any effect.
A better approach might be to validate a v3 transaction in createrawtransaction_tests
, for example, as shown below.
0 # Multiple mixed outputs
1 tx = tx_from_hex(self.nodes[2].createrawtransaction(inputs=[{'txid': TXID, 'vout': 9}], outputs=multidict([(address, 99), (address2, 99), ('data', '99')])))
2+ assert_equal(tx.version, 2)
3 assert_equal(len(tx.vout), 3)
4 assert_equal(
5 tx.serialize().hex(),
6 self.nodes[2].createrawtransaction(inputs=[{'txid': TXID, 'vout': 9}], outputs=[{address: 99}, {address2: 99}, {'data': '99'}]),
7 )
8
9+ rawtx_v3 = self.nodes[2].createrawtransaction(inputs=[{'txid': TXID, 'vout': 9}], outputs=OrderedDict([(address, 99), (address2, 99)]), version=3)
10+ tx = tx_from_hex(rawtx_v3)
11+ assert_equal(tx.version, 3)
Added support for creating v3 raw transaction:
- Overloaded to include additional parameter
Co-authored-by: chungeun-choi <cucuridas@gmail.com>
Co-authored-by: dongwook-chan <dongwook.chan@gmail.com>
Co-authored-by: sean-k1 <uhs2000@naver.com>
153@@ -154,6 +154,13 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
154 rawTx.nLockTime = nLockTime;
155 }
156
157+ if (!version.isNull()) {
158+ uint32_t nVersion = version.getInt<uint32_t>();
159+ if (nVersion < TX_MIN_STANDARD_VERSION || nVersion > TX_MAX_STANDARD_VERSION)
160+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, version out of range(1~3)");
TX_MIN_STANDARD_VERSION
and TX_MAX_STANDARD_VERSION
get updated.
254@@ -255,7 +255,11 @@ def createrawtransaction_tests(self):
255 assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [])
256
257 # Test `createrawtransaction` invalid extra parameters
258- assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [], {}, 0, False, 'foo')
259+ assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [], {}, 0, False, 2, 3, 'foo')
260+
261+ # Test `createrawtransaction` invalid version parameters
262+ assert_raises_rpc_error(-8, "Invalid parameter, version out of range(1~3)", self.nodes[0].createrawtransaction, [], {}, 0, False, 0)
263+ assert_raises_rpc_error(-8, "Invalid parameter, version out of range(1~3)", self.nodes[0].createrawtransaction, [], {}, 0, False, 4)
TX_MAX_STANDARD_VERSION + 1
or at least to a number with headroom like 999
.
338@@ -335,6 +339,10 @@ def createrawtransaction_tests(self):
339 self.nodes[2].createrawtransaction(inputs=[{'txid': TXID, 'vout': 9}], outputs=[{address: 99}, {address2: 99}, {'data': '99'}]),
340 )
341
342+ rawtx_v3 = self.nodes[2].createrawtransaction(inputs=[{'txid': TXID, 'vout': 9}], outputs=OrderedDict([(address, 99), (address2, 99)]), version=3)
343+ tx = tx_from_hex(rawtx_v3)
344+ assert_equal(tx.version, 3)
tx.version
in the range [TX_MIN_STANDARD_VERSION, TX_MAX_STANDARD_VERSION]
?
utACK 63d9b9e3b8e225eccd55209e7a4213604ec88425
Suggested some improvements, but not blocking ones IMO
Approach NACK
While I agree that we should support v3 transactions, I think this implementation is incomplete without any v3 support in the wallet’s coin selection. As it is now, this appears to enable a massive footgun - users can use createrawtransaction
to create a v3 transaction, fund it with fundrawtransaction
, and then wind up with a v3 transaction that they cannot broadcast. This is because the topological restrictions of v3 transactions are not being taken into account during coin selection, it is possible that the wallet will select inputs which are incompatible with a v3 transaction, and therefore produce a transaction that won’t be broadcast.
I expect that supporting sending v3 transactions from Bitcoin Core will actual be a significantly more involved project. While I appreciate your enthusiasm, I think it would be better to leave the implementation of this feature to experienced contributors.