rpc: Support v3 raw transactions creation #31936

Bue-von-hon · 2025-02-22T07:30:15Z

Added support for creating v3 raw transaction:

Overloaded to include additional parameter

this resolves #31348

DrahtBot · 2025-02-22T07:30:18Z

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:

#32966 (Silent Payments: Receiving by Eunovo)
#32896 (wallet, rpc: add v3 transaction creation and wallet support by ishaanam)
#28201 (Silent Payments: sending by josibake)
#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.

Bue-von-hon · 2025-02-22T07:33:31Z

@rkrux @theStack @ytrezq PTAL

maflcko · 2025-02-22T09:45:36Z

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

glozow

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.

glozow · 2025-02-24T16:02:00Z

src/rpc/rawtransaction_util.cpp

+    if (!version.isNull()) {
+        int64_t nVersion = version.getInt<int64_t>();
+        if (nVersion < 0 || nVersion > TRANSACTION_VERSION_MAX)
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, version out of range");


Perhaps include what the min and max are.

Added variable to denote min, max value of the version.

bitcoin/src/rpc/rawtransaction_util.cpp

Lines 159 to 160 in 1121cb7

if (nVersion < TX_MIN_STANDARD_VERSION || nVersion > TX_MAX_STANDARD_VERSION)

throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, version out of range(1~3)");

glozow · 2025-02-24T16:02:55Z

src/script/script.h

@@ -63,6 +63,9 @@ static constexpr int64_t VALIDATION_WEIGHT_PER_SIGOP_PASSED{50};
 // How much weight budget is added to the witness size (Tapscript only, see BIP 342).
 static constexpr int64_t VALIDATION_WEIGHT_OFFSET{50};

+// Maximum transaction version
+static const int32_t TRANSACTION_VERSION_MAX = 3;


Why aren't we using TX_MAX_STANDARD_VERSION everywhere?

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

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

removed it.

glozow · 2025-02-24T16:07:17Z

src/rpc/rawtransaction_util.cpp

@@ -154,6 +154,13 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
        rawTx.nLockTime = nLockTime;
    }

+    if (!version.isNull()) {
+        int64_t nVersion = version.getInt<int64_t>();


Why int64_t? version has type uint32_t.

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

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

Let's use uint32_t because that is what is used in both CTransaction and CMutableTransaction.

bitcoin/src/primitives/transaction.h

Line 308 in 72c150d

const uint32_t version;

bitcoin/src/primitives/transaction.h

Line 381 in 72c150d

uint32_t version;

Those docs might be outdated because this change was done recently: #29325

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.

replaced it.

luke-jr · 2025-02-26T00:28:38Z

src/rpc/client.cpp

@@ -122,6 +122,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
    { "createrawtransaction", 1, "outputs" },
    { "createrawtransaction", 2, "locktime" },
    { "createrawtransaction", 3, "replaceable" },
+    { "createrawtransaction", 4, "version" },


Probably best not to have this be a positional parameter.

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

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

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

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

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?

Related, see #31936 (comment)

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?

chungeun-choi · 2025-03-01T07:23:05Z

@glozow @luke-jr PTAL

rkrux

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

rkrux · 2025-03-01T07:32:44Z

test/functional/rpc_rawtransaction.py

+        assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [], {}, 0, False, 2, 'foo')
+
+        # Test `createrawtransaction` invalid version parameters
+        assert_raises_rpc_error(-8, "Invalid parameter, version out of range(0~3)", self.nodes[0].createrawtransaction, [], {}, 0, False, -1)
+        assert_raises_rpc_error(-8, "Invalid parameter, version out of range(0~3)", self.nodes[0].createrawtransaction, [], {}, 0, False, 4)


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.

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

bitcoin/test/functional/rpc_rawtransaction.py

Line 96 in 1121cb7

self.raw_multisig_transaction_legacy_tests(3)

See: #31936 (comment)

dongwook-chan · 2025-03-01T07:37:29Z

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

rkrux · 2025-03-01T08:21:51Z

Re: #31936 (comment)

There seems to be couple functional tests related to locktime out of range here:

bitcoin/test/functional/rpc_rawtransaction.py

Lines 312 to 313 in 3c1f72a

    
           assert_raises_rpc_error(-8, "Invalid parameter, locktime out of range", self.nodes[0].createrawtransaction, [], {}, -1) 
        
           assert_raises_rpc_error(-8, "Invalid parameter, locktime out of range", self.nodes[0].createrawtransaction, [], {}, 4294967296)

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 (comment), 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 · 2025-03-01T09:26:01Z

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.

src/rpc/rawtransaction_util.cpp

adamandrews1 · 2025-03-06T22:41:49Z

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

maflcko · 2025-03-09T11:06:42Z

src/rpc/rawtransaction_util.h

@@ -54,5 +54,6 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in);

 /** Create a transaction from univalue parameters */
 CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional<bool> rbf);


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

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

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.

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.

I was trying to say in #31936 (comment) 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.

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 (comment).

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 (comment) because most of the above mentioned RPCs accept an option param wherein the version option can be added.

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

rkrux · 2025-03-13T11:30:25Z

src/rpc/client.cpp

@@ -122,6 +122,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
    { "createrawtransaction", 1, "outputs" },
    { "createrawtransaction", 2, "locktime" },
    { "createrawtransaction", 3, "replaceable" },
+    { "createrawtransaction", 4, "version" },


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 · 2025-03-13T12:16:35Z

src/rpc/rawtransaction_util.cpp

@@ -154,6 +154,13 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
        rawTx.nLockTime = nLockTime;
    }

+    if (!version.isNull()) {
+        int64_t nVersion = version.getInt<int64_t>();


Let's use uint32_t because that is what is used in both CTransaction and CMutableTransaction.

bitcoin/src/primitives/transaction.h

Line 308 in 72c150d

const uint32_t version;

bitcoin/src/primitives/transaction.h

Line 381 in 72c150d

uint32_t version;

Those docs might be outdated because this change was done recently: #29325

rkrux · 2025-03-13T12:28:04Z

test/functional/rpc_rawtransaction.py

@@ -491,7 +496,7 @@ def transaction_version_number_tests(self):
        decrawtx = self.nodes[0].decoderawtransaction(rawtx)
        assert_equal(decrawtx['version'], 0xffffffff)

-    def raw_multisig_transaction_legacy_tests(self):
+    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:

bitcoin/test/functional/rpc_rawtransaction.py

Line 187 in 199d47d

fields = [

rkrux · 2025-03-13T12:36:57Z

src/rpc/client.cpp

@@ -180,6 +181,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
    { "createpsbt", 1, "outputs" },
    { "createpsbt", 2, "locktime" },
    { "createpsbt", 3, "replaceable" },
+    { "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.

rkrux · 2025-03-13T12:38:21Z

src/rpc/rawtransaction_util.h

@@ -54,5 +54,6 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in);

 /** Create a transaction from univalue parameters */
 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.

rkrux

Answering open comments.

rkrux · 2025-03-24T14:19:19Z

src/rpc/rawtransaction_util.h

@@ -54,5 +54,6 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in);

 /** Create a transaction from univalue parameters */
 CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional<bool> rbf);


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 (comment).

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 (comment) because most of the above mentioned RPCs accept an option param wherein the version option can be added.

maflcko · 2025-03-31T06:27:14Z

Are you still working on this?

Bue-von-hon · 2025-03-31T07:05:58Z

Are you still working on this?

sure!

glozow · 2025-03-31T14:48:24Z

src/script/script.h

@@ -63,6 +63,9 @@ static constexpr int64_t VALIDATION_WEIGHT_PER_SIGOP_PASSED{50};
 // How much weight budget is added to the witness size (Tapscript only, see BIP 342).
 static constexpr int64_t VALIDATION_WEIGHT_OFFSET{50};

+// Maximum transaction version
+static const int32_t TRANSACTION_VERSION_MAX = 3;


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

glozow · 2025-03-31T14:57:08Z

src/policy/policy.h

@@ -144,6 +144,7 @@ std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate);
 // Changing the default transaction version requires a two step process: first
 // adapting relay policy by bumping TX_MAX_STANDARD_VERSION, and then later
 // allowing the new transaction version in the wallet/RPC.
+static constexpr decltype(CTransaction::version) TX_MIN_STANDARD_VERSION{1};


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

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.

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

glozow · 2025-03-31T14:58:08Z

src/rpc/rawtransaction.cpp

@@ -158,6 +158,7 @@ static std::vector<RPCArg> CreateTxDoc()
        {"locktime", RPCArg::Type::NUM, RPCArg::Default{0}, "Raw locktime. Non-0 value also locktime-activates inputs"},
        {"replaceable", RPCArg::Type::BOOL, RPCArg::Default{true}, "Marks this transaction as BIP125-replaceable.\n"
                "Allows this transaction to be replaced by a transaction with higher fees. If provided, it is an error if explicit sequence numbers are incompatible."},
+        {"version", RPCArg::Type::NUM, RPCArg::Default{2}, "Transaction version"},


Should use CURRENT_VERSION instead of magic number

glozow · 2025-03-31T16:25:35Z

src/rpc/client.cpp

@@ -122,6 +122,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
    { "createrawtransaction", 1, "outputs" },
    { "createrawtransaction", 2, "locktime" },
    { "createrawtransaction", 3, "replaceable" },
+    { "createrawtransaction", 4, "version" },


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?

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>

Bue-von-hon · 2025-04-26T12:30:57Z

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

Bue-von-hon · 2025-04-30T05:13:57Z

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

adamandrews1

utACK 63d9b9e

Suggested some improvements, but not blocking ones IMO

adamandrews1 · 2025-05-01T16:51:28Z

src/rpc/rawtransaction_util.cpp

+    if (!version.isNull()) {
+        uint32_t nVersion = version.getInt<uint32_t>();
+        if (nVersion < TX_MIN_STANDARD_VERSION || nVersion > TX_MAX_STANDARD_VERSION)
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, version out of range(1~3)");


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.

adamandrews1 · 2025-05-01T16:53:21Z

test/functional/rpc_rawtransaction.py

+
+        # Test `createrawtransaction` invalid version parameters
+        assert_raises_rpc_error(-8, "Invalid parameter, version out of range(1~3)", self.nodes[0].createrawtransaction, [], {}, 0, False, 0)
+        assert_raises_rpc_error(-8, "Invalid parameter, version out of range(1~3)", self.nodes[0].createrawtransaction, [], {}, 0, False, 4)


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.

adamandrews1 · 2025-05-01T16:56:09Z

test/functional/rpc_rawtransaction.py

+        rawtx_v3 = self.nodes[2].createrawtransaction(inputs=[{'txid': TXID, 'vout': 9}], outputs=OrderedDict([(address, 99), (address2, 99)]), version=3)
+        tx = tx_from_hex(rawtx_v3)
+        assert_equal(tx.version, 3)


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

achow101 · 2025-05-03T00:42:25Z

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.

Bue-von-hon · 2025-05-10T02:19:39Z

@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 · 2025-05-10T03:19:45Z

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

Hints

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 · 2025-05-20T20:34:01Z

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 · 2025-05-20T21:12:11Z

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 · 2025-05-21T08:46:40Z

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:

createrawtransaction '[]' '[{"data":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}]'

fundrawtransaction 02000000000100000000000000007c6a4c79aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa00000000 '{"fee_rate":"1.2"}'

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

Co-authored-by: chungeun-choi <cucuridas@gmail.com> Co-authored-by: sean-k1 <uhs2000@naver.com>

sean-k1 · 2025-05-31T12:47:58Z

@achow101 @maflcko
we fixed the lint error.
There seems to be a lot of discussion, let me know when it's clear.

sean-k1 · 2025-07-08T05:48:30Z

@achow101 @maflcko
PTAL

maflcko · 2025-07-08T09:04:40Z

ref #32896

maflcko · 2025-07-08T09:07:39Z

PTAL

Not sure what you are asking for. Feedback has been provided, and the burden to address it and reply to it is on the pull request author. I can't make it go away by looking at it more.

Apart from the prior feedback, you'd also have to squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits into atomic units (https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches)

maflcko · 2025-07-14T14:51:21Z

Closing for now, due to inactivity. The given feedback hasn't bee addressed over a long time and at this point it may be best to focus review on #32896.

…port 5c8bf7b doc: add release notes for version 3 transactions (ishaanam) 4ef8065 test: add truc wallet tests (ishaanam) 5d932e1 test: extract `bulk_vout` from `bulk_tx` so it can be used by wallet tests (ishaanam) 2cb473d rpc: Support version 3 transaction creation (Bue-von-hon) 4c20343 rpc: Add transaction min standard version parameter (Bue-von-hon) c5a2d08 wallet: don't return utxos from multiple truc txs in AvailableCoins (ishaanam) da8748a wallet: limit v3 tx weight in coin selection (ishaanam) 85c5410 wallet: mark unconfirmed v3 siblings as mempool conflicts (ishaanam) 0804fc3 wallet: throw error at conflicting tx versions in pre-selected inputs (ishaanam) cc15522 wallet: set m_version in coin control to default value (ishaanam) 2e96176 wallet: don't include unconfirmed v3 txs with children in available coins (ishaanam) ec2676b wallet: unconfirmed ancestors and descendants are always truc (ishaanam) Pull request description: This PR Implements the following: - If creating a v3 transaction, `AvailableCoins` doesn't return unconfirmed v2 utxos (and vice versa) - `AvailableCoins` doesn't return an unconfirmed v3 utxo if its transaction already has a child - If a v3 transaction is kicked out of the mempool by a sibling, mark the sibling as a mempool conflict - Throw an error if pre-selected inputs are of the wrong transaction version - Allow setting version to 3 manually in `createrawtransaction` (uses commits from #31936) - Limits a v3 transaction weight in coin selection Closes #31348 To-Do: - [x] Test a v3 sibling conflict kicking out one of our transactions from the mempool - [x] Implement separate size limit for TRUC children - [x] Test that we can't fund a v2 transaction when everything is v3 unconfirmed - [x] Test a v3 sibling conflict being removed from the mempool - [x] Test limiting v3 transaction weight in coin selection - [x] Simplify tests - [x] Add documentation - [x] Test that user-input max weight is not overwritten by truc max weight - [x] Test v3 in RPCs other than `createrawtransaction` ACKs for top commit: glozow: reACK 5c8bf7b achow101: ACK 5c8bf7b rkrux: ACK 5c8bf7b Tree-SHA512: da8aea51c113e193dd0b442eff765bd6b8dc0e5066272d3e52190a223c903f48788795f32c554f268af0d2607b5b8c3985c648879cb176c65540837c05d0abb5

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

kakao-will-k mentioned this pull request Feb 22, 2025

Add support for creating v3 raw transactions in createrawtransaction RPC #31348

Closed

DrahtBot added the CI failed label Feb 22, 2025

glozow reviewed Feb 24, 2025

View reviewed changes

luke-jr reviewed Feb 26, 2025

View reviewed changes

Bue-von-hon force-pushed the create-v3-rawtransaction branch from 8a4348d to db71e16 Compare March 1, 2025 07:07

rkrux reviewed Mar 1, 2025

View reviewed changes

DrahtBot removed the CI failed label Mar 1, 2025

DrahtBot mentioned this pull request Mar 1, 2025

Implement BIP 370 PSBTv2 #21283

Open

This comment was marked as abuse.

Sign in to view

instagibbs reviewed Mar 6, 2025

View reviewed changes

src/rpc/rawtransaction_util.cpp Outdated Show resolved Hide resolved

Bue-von-hon force-pushed the create-v3-rawtransaction branch from db71e16 to 1121cb7 Compare March 9, 2025 01:21

DrahtBot mentioned this pull request Mar 9, 2025

wallet: Disable creating and loading legacy wallets #31250

Merged

maflcko reviewed Mar 9, 2025

View reviewed changes

DrahtBot mentioned this pull request Mar 9, 2025

Remove the legacy wallet and BDB dependency #28710

Merged

rkrux reviewed Mar 13, 2025

View reviewed changes

rkrux reviewed Mar 24, 2025

View reviewed changes

glozow reviewed Mar 31, 2025

View reviewed changes

fanquake marked this pull request as draft April 1, 2025 11:21

Bue-von-hon force-pushed the create-v3-rawtransaction branch from 1121cb7 to 3722e10 Compare April 5, 2025 11:34

Bue-von-hon and others added 2 commits April 26, 2025 20:43

rpc: Add current version parameter for transaction

cb15291

rpc: Support v3 raw transactions creation

a43237d

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>

Bue-von-hon force-pushed the create-v3-rawtransaction branch from d3e3ea7 to a43237d Compare April 26, 2025 12:06

Fix test

63d9b9e

Bue-von-hon marked this pull request as ready for review April 26, 2025 12:29

DrahtBot removed the Needs rebase label Apr 26, 2025

adamandrews1 reviewed May 1, 2025

View reviewed changes

DrahtBot requested a review from glozow May 1, 2025 16:57

Resolves code review

6a69ad5

DrahtBot added the CI failed label May 10, 2025

Fix lint

e9a1354

Co-authored-by: chungeun-choi <cucuridas@gmail.com> Co-authored-by: sean-k1 <uhs2000@naver.com>

DrahtBot removed the CI failed label May 31, 2025

ishaanam mentioned this pull request Jul 7, 2025

wallet, rpc: add v3 transaction creation and wallet support #32896

Merged

9 tasks

DrahtBot mentioned this pull request Jul 7, 2025

rpc: use anti-fee-sniping in send, sendall and walletcreatefundedpsbt #32892

Closed

DrahtBot mentioned this pull request Jul 14, 2025

Silent Payments: Receiving #32966

Draft

maflcko closed this Jul 14, 2025

	if (nVersion < TX_MIN_STANDARD_VERSION \|\| nVersion > TX_MAX_STANDARD_VERSION)
	throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, version out of range(1~3)");

		@@ -54,5 +54,6 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in);

		/** Create a transaction from univalue parameters */
		CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional<bool> rbf);

rpc: Support v3 raw transactions creation #31936

rpc: Support v3 raw transactions creation #31936

Uh oh!

Conversation

Bue-von-hon commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Bue-von-hon commented Feb 22, 2025

Uh oh!

maflcko commented Feb 22, 2025

Uh oh!

glozow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chungeun-choi commented Mar 1, 2025

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongwook-chan commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkrux commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkrux commented Mar 1, 2025

Uh oh!

Bue-von-hon commented Feb 22, 2025 •

edited

Loading

DrahtBot commented Feb 22, 2025 •

edited

Loading

glozow left a comment •

edited

Loading

dongwook-chan commented Mar 1, 2025 •

edited

Loading

rkrux commented Mar 1, 2025 •

edited

Loading

adamandrews1 commented Mar 6, 2025 •

edited

Loading