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
Approach NACK	achow101
Stale ACK	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?

glozow commented at 2:48 pm on March 31, 2025:

Yes, this seems to be redundant with TX_MAX_STANDARD_VERSION. It also appears to be unused, did you maybe forget to delete it?

Bue-von-hon commented at 8:42 am on April 5, 2025:

removed it.

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.

Bue-von-hon commented at 8:42 am on April 5, 2025:

replaced it.

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)

glozow commented at 4:25 pm on March 31, 2025:

This is needed as the arg is non-string type. My understanding is that @luke-jr is concept nacking the PR. I don’t really understand the comments about the positionalness of this arg?

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

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

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

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.

Bue-von-hon commented at 8:33 am on April 5, 2025:

This is truly amazing content. I wrote a blog post on this topic.

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

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 outdated

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!

in src/policy/policy.h:147 in 1121cb7a91 outdated

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

glozow commented at 2:57 pm on March 31, 2025:

concept ack introducing this constant. I think it would be best to have it in a separate commit and replace the magic number in policy.cpp with this

Bue-von-hon commented at 7:40 am on April 5, 2025:

I’m not sure about the meaning of ‘separate commit,’ but I replaced the magic number in the policy.cpp file with a constant value.

glozow commented at 7:25 pm on April 7, 2025:

I mean putting the changes assigning 1 to this constant in one commit, and then the version parameter addition in a second commit.

in src/rpc/rawtransaction.cpp:161 in 1121cb7a91 outdated

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

glozow commented at 2:58 pm on March 31, 2025:

Should use CURRENT_VERSION instead of magic number

Bue-von-hon commented at 8:42 am on April 5, 2025:

Fix it.

fanquake marked this as a draft on Apr 1, 2025

Bue-von-hon force-pushed on Apr 5, 2025

Bue-von-hon commented at 11:36 am on April 5, 2025: none

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

Bue-von-hon marked this as ready for review on Apr 5, 2025

DrahtBot commented at 12:33 pm on April 5, 2025: contributor

🚧 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.

DrahtBot added the label CI failed on Apr 5, 2025

in src/wallet/rpc/spend.cpp:1298 in 3722e108ae outdated

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

glozow commented at 7:33 pm on April 7, 2025:

You are changing the default version without motivation here. Also, as mentioned in previous review comments, please try to avoid magic numbers - here and in 3 other places.

glozow commented at 7:35 pm on April 7, 2025: member

CI is still failing

Bue-von-hon force-pushed on Apr 11, 2025

Bue-von-hon commented at 0:14 am on April 11, 2025: none

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.

Bue-von-hon marked this as a draft on Apr 11, 2025

rkrux commented at 7:49 am on April 11, 2025: contributor

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.

Bue-von-hon commented at 10:52 am on April 11, 2025: none

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

Bue-von-hon force-pushed on Apr 11, 2025

Bue-von-hon marked this as ready for review on Apr 11, 2025

Bue-von-hon marked this as a draft on Apr 11, 2025

Bue-von-hon marked this as ready for review on Apr 11, 2025

Bue-von-hon commented at 2:14 pm on April 11, 2025: none

@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'`

DrahtBot removed the label CI failed on Apr 11, 2025

Bue-von-hon requested review from luke-jr on Apr 11, 2025

Bue-von-hon requested review from instagibbs on Apr 11, 2025

Bue-von-hon requested review from maflcko on Apr 11, 2025

Bue-von-hon requested review from rkrux on Apr 11, 2025

Bue-von-hon requested review from glozow on Apr 11, 2025

Bue-von-hon requested review from adamandrews1 on Apr 11, 2025

Bue-von-hon commented at 10:59 am on April 15, 2025: none

@luke-jr @instagibbs @maflcko @rkrux @glozow @adamandrews1 PTAL 🙏

glozow commented at 6:14 pm on April 15, 2025: member

@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.

w0xlt commented at 8:31 pm on April 17, 2025: contributor

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)

DrahtBot added the label Needs rebase on Apr 25, 2025

Bue-von-hon force-pushed on Apr 26, 2025

rpc: Add version parameter for transaction 51c7b4c9e0

Bue-von-hon marked this as a draft on Apr 26, 2025

rpc: Add current version parameter for transaction cb15291ea2

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>

a43237d11e

Bue-von-hon force-pushed on Apr 26, 2025

Fix test 63d9b9e3b8

Bue-von-hon marked this as ready for review on Apr 26, 2025

Bue-von-hon commented at 12:30 pm on April 26, 2025: none

@luke-jr @instagibbs @maflcko @rkrux @glozow @adamandrews1 @w0xlt @1440000bytes PTAL 🙏

DrahtBot removed the label Needs rebase on Apr 26, 2025

Bue-von-hon commented at 5:13 am on April 30, 2025: none

@luke-jr @instagibbs @maflcko @rkrux @glozow @adamandrews1 @w0xlt @1440000bytes gentle ping.

in src/rpc/rawtransaction_util.cpp:160 in 63d9b9e3b8 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+        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)");

adamandrews1 commented at 4:51 pm on May 1, 2025:

This RPC error should be a formatted string so that it remains accurate as TX_MIN_STANDARD_VERSION and TX_MAX_STANDARD_VERSION get updated.

in test/functional/rpc_rawtransaction.py:262 in 63d9b9e3b8 outdated

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)

adamandrews1 commented at 4:53 pm on May 1, 2025:

The final parameter (tx version) for this test should be set to TX_MAX_STANDARD_VERSION + 1 or at least to a number with headroom like 999.

in test/functional/rpc_rawtransaction.py:344 in 63d9b9e3b8 outdated

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)

adamandrews1 commented at 4:56 pm on May 1, 2025:

Shouldn’t this test be repeated for each valid tx.version in the range [TX_MIN_STANDARD_VERSION, TX_MAX_STANDARD_VERSION]?

adamandrews1 commented at 4:57 pm on May 1, 2025: none

utACK 63d9b9e3b8e225eccd55209e7a4213604ec88425

Suggested some improvements, but not blocking ones IMO

DrahtBot requested review from glozow on May 1, 2025

achow101 commented at 0:42 am on May 3, 2025: member

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.

Resolves code review 6a69ad597d

Bue-von-hon commented at 2:19 am on May 10, 2025: none

@adamandrews1 @achow101 As per the comments, we’ll hand over the task to a more suitable developer. Instead, we plan to proceed with issue-26756 (we’re a developer group based in South-Korea). This issue involves aligning the behavior of the AvailableCoins method with the getbalance method, which seems well-suited to our expertise. If you believe this issue isn’t appropriate for us, we’d appreciate it if you could suggest alternative issues.

P.S. We’ve incorporated all code review feedback. Please review when convenient.

DrahtBot added the label CI failed on May 10, 2025

DrahtBot commented at 3:19 am on May 10, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/41977639266 LLM reason (✨ experimental): The CI failure is due to the lint-locale-dependence.py check detecting the use of std::to_string.

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.

maflcko commented at 8:34 pm on May 20, 2025: member

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.

createrawtransaction doesn’t do other policy checks either, such as OP_RETURN checks, so one could argue this is fine as well.

Though, CI is failing on this pull.

achow101 commented at 9:12 pm on May 20, 2025: member

createrawtransaction doesn’t do other policy checks either, such as OP_RETURN checks, so one could argue this is fine as well.

fundrawtransaction does though, and that was my point. This PR doesn’t touch fundrawtransaction (or rather the wallet’s coin selection code) and hence this is incomplete.

maflcko commented at 8:46 am on May 21, 2025: member

createrawtransaction doesn’t do other policy checks either, such as OP_RETURN checks, so one could argue this is fine as well.

fundrawtransaction does though, and that was my point.

I don’t think fundrawtransaction checks for OP_RETURN. At least locally, the following passes for me:

0createrawtransaction '[]' '[{"data":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}]'
1
2fundrawtransaction 02000000000100000000000000007c6a4c79aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa00000000 '{"fee_rate":"1.2"}'

The following sendrawtransaction fails with scriptpubkey (code -26).

rpc: Support v3 raw transactions creation #31936

Code Coverage & Benchmarks

Reviews

Conflicts