rpc: Support v3 raw transactions creation

Bue-von-hon commented at 7:30 am on February 22, 2025: none

Added support for creating v3 raw transaction:

Overloaded to include additional parameter

this resolves #31348

DrahtBot commented at 7:30 am on February 22, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31936.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	glozow, 1440000bytes, adamandrews1

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21283 (Implement BIP 370 PSBTv2 by achow101)

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.

DrahtBot added the label RPC/REST/ZMQ on Feb 22, 2025

Bue-von-hon commented at 7:33 am on February 22, 2025: none

@rkrux @theStack @ytrezq PTAL

DrahtBot added the label CI failed on Feb 22, 2025

maflcko commented at 9:45 am on February 22, 2025: member

You’ll have to add tests for any new feature and make sure the tests pass

in src/rpc/rawtransaction_util.cpp:160 in 8a4348d2f1 outdated

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");

glozow commented at 4:02 pm on February 24, 2025:

Perhaps include what the min and max are.

dongwook-chan commented at 1:34 pm on March 9, 2025:

Added variable to denote min, max value of the version. https://github.com/bitcoin/bitcoin/blob/1121cb7a91e0dfa4f247273bf89e2e1b47c82628/src/rpc/rawtransaction_util.cpp#L159-L160

in src/script/script.h:67 in 8a4348d2f1 outdated

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;

glozow commented at 4:02 pm on February 24, 2025:

Why aren’t we using TX_MAX_STANDARD_VERSION everywhere?

dongwook-chan commented at 1:36 pm on March 9, 2025:

@glozow Are you implying that we should use the variable instead of constant values in the test code?

in src/rpc/rawtransaction_util.cpp:158 in 8a4348d2f1 outdated

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>();

glozow commented at 4:07 pm on February 24, 2025:

Why int64_t? version has type uint32_t.

dongwook-chan commented at 5:39 am on March 1, 2025:

@glozow Thanks for the feedback. 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?

adamandrews1 commented at 10:41 pm on March 6, 2025:

it’s weird that it’s signed, but I think we should use the same type as the docs

rkrux commented at 12:16 pm on March 13, 2025:

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

https://github.com/bitcoin/bitcoin/blob/72c150dfe7619c2b6e2fd06a1fa431b545a3a225/src/primitives/transaction.h#L381

Those docs might be outdated because this change was done recently: https://github.com/bitcoin/bitcoin/pull/29325

rkrux commented at 11:18 am on March 14, 2025:

These docs are indeed outdated, last update was 4 years back: https://github.com/bitcoin-dot-org/developer.bitcoin.org. Only the code in this repo should be treated as source of truth.

glozow commented at 4:11 pm on February 24, 2025: member

Concept ACK. Thanks for your PR, but it needs to pass CI and new features need tests to be considered (please read CONTRIBUTING.md). Perhaps a functional test in rpc_rawtransaction.py.

in src/rpc/client.cpp:125 in 8a4348d2f1 outdated

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" },

luke-jr commented at 0:28 am on February 26, 2025:

Probably best not to have this be a positional parameter.

dongwook-chan commented at 7:15 am on March 1, 2025:

@luke-jr We had to add it because without it, RPC method vaildation does not pass.

luke-jr commented at 3:01 pm on March 1, 2025:

No, the issue isn’t this one line of code, but the overall addition of the parameter

adamandrews1 commented at 10:40 pm on March 6, 2025:

I agree think it’s a bit too important for a 4th positional arg

dongwook-chan commented at 1:38 pm on March 9, 2025:

@luke-jr @adamandrews1 So should I remove the line? If I should, could you advise which code should be updated to avoid failure of the aforementioned validation?

rkrux commented at 11:30 am on March 13, 2025:

I am inclining towards not adding a positional argument for this. Mostly because this is not something the user would use frequently and instead would let the defaults take over. I think using an optional options object here (used in many RPCs) would suffice for this use case. @glozow WDYT?

rkrux commented at 2:57 pm on March 24, 2025:

Related, see #31936 (review)

Bue-von-hon force-pushed on Mar 1, 2025

chungeun-choi commented at 7:23 am on March 1, 2025: none

@glozow @luke-jr PTAL

in test/functional/rpc_rawtransaction.py:266 in db71e16164 outdated

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)

rkrux commented at 7:32 am on March 1, 2025:

IMO we should add a test wherein a v3 transaction is successfully created, signed, and broadcast to the network. ATM, only the erroneous cases are covered.

dongwook-chan commented at 1:31 pm on March 9, 2025:

@rkrux Utilized existing 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

rkrux commented at 1:00 pm on March 13, 2025:

See: #31936 (review)

rkrux commented at 7:34 am on March 1, 2025: contributor

Thank you for working on this, please mention in the PR description that it closes the issue #31348.

dongwook-chan commented at 7:37 am on March 1, 2025: none

@glozow @luke-jr @rkrux

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?

DrahtBot removed the label CI failed on Mar 1, 2025

rkrux commented at 8:21 am on March 1, 2025: contributor

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.

rkrux commented at 9:26 am on March 1, 2025: contributor

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.

1440000bytes commented at 4:30 pm on March 3, 2025: none

Concept ACK

in src/rpc/rawtransaction_util.cpp:159 in db71e16164 outdated

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)

instagibbs commented at 7:49 pm on March 6, 2025:

You should add a test for this, but I’m pretty sure version 0 was never standard for relay.

adamandrews1 commented at 10:41 pm on March 6, 2025: none

Concept ACK Note: May need to revisit #31298 after this merges

rpc: Support v3 raw transactions creation

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>

1121cb7a91

Bue-von-hon force-pushed on Mar 9, 2025

in src/rpc/rawtransaction_util.h:56 in 1121cb7a91

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);

maflcko commented at 11:06 am on March 9, 2025:

not sure about default args. It would be better to explicitly enumerate the call sites and just put the default there, if needed.

rkrux commented at 12:38 pm on March 13, 2025:

Is this comment outdated? I don’t understand where are the default args here that are referred to in the above comment.

maflcko commented at 11:29 am on March 14, 2025:

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.

rkrux commented at 12:01 pm on March 14, 2025:

Oh I see this comment referred to the new implementation of this older function caused by the introduction of function overloading here. Yeah, I am not in favour of this as well, seem unnecessary for just a version param, the function signature defaults can suffice.

maflcko commented at 12:20 pm on March 14, 2025:

I was trying to say in #31936 (review) that a default doesn’t make sense at all here (in any way), because:

Every call site that omits the default has the same fallback, so changing the default in the future will make review harder.
Every call site needs to document the default in the RPC help anyway, so simply using the default from the docs is self-consistent and self-documenting and avoids the need for a default here

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.

rkrux commented at 2:19 pm on March 24, 2025:

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.

in test/functional/rpc_rawtransaction.py:499 in 1121cb7a91

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):

rkrux commented at 12:28 pm on March 13, 2025:

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

in src/rpc/client.cpp:184 in 1121cb7a91

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" },

rkrux commented at 12:36 pm on March 13, 2025:

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.

rkrux commented at 2:20 pm on March 24, 2025: contributor

Answering open comments.

maflcko commented at 6:27 am on March 31, 2025: member

Are you still working on this?

Bue-von-hon commented at 7:05 am on March 31, 2025: none

Are you still working on this?

sure!

rpc: Support v3 raw transactions creation #31936

Code Coverage & Benchmarks

Reviews

Conflicts