wallet, rpc: add v3 transaction creation and wallet support

ishaanam commented at 8:13 pm on July 7, 2025: contributor

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:

Test a v3 sibling conflict kicking out one of our transactions from the mempool
Implement separate size limit for TRUC children
Test that we can’t fund a v2 transaction when everything is v3 unconfirmed
Test a v3 sibling conflict being removed from the mempool
Test limiting v3 transaction weight in coin selection
Simplify tests
Add documentation
Test that user-input max weight is not overwritten by truc max weight
Test v3 in RPCs other than createrawtransaction

DrahtBot commented at 8:14 pm on July 7, 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/32896.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	glozow, rkrux, murchandamus

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:

#32983 (rpc: refactor: use string_view in Arg/MaybeArg by stickies-v)
#32857 (wallet: allow skipping script paths by Sjors)
#32523 (wallet: Remove isminetypes by achow101)
#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.

ishaanam force-pushed on Jul 7, 2025

DrahtBot added the label CI failed on Jul 7, 2025

DrahtBot commented at 8:19 pm on July 7, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/45505893140 LLM reason (✨ experimental): The CI failure is caused by a lint error due to a file permission issue with a Python script that has a shebang line but incorrect executable permissions.

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.

ishaanam force-pushed on Jul 8, 2025

in test/functional/wallet_v3_txs.py:123 in e7b4084738 outdated

65+
66+        self.v3_tx_spends_unconfirmed_v2_tx()
67+        self.v3_utxos_appear_in_listunspent()
68+
69+    @cleanup
70+    def v3_tx_spends_unconfirmed_v2_tx(self):

glozow commented at 5:16 pm on July 8, 2025:

We should also check that we can’t fund a v2 transaction when everything is v3 unconfirmed

ishaanam commented at 7:22 pm on July 8, 2025:

Good point, I will add a test for this.

in src/wallet/rpc/spend.cpp:720 in 75d606c564 outdated

715@@ -715,6 +716,12 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact
716         coinControl.m_max_tx_weight = options["max_tx_weight"].getInt<int>();
717     }
718 
719+    if (tx.version == TRUC_VERSION) {
720+        if (!coinControl.m_max_tx_weight.has_value() || coinControl.m_max_tx_weight.value() > 40000) {

glozow commented at 5:17 pm on July 8, 2025:

Instead of magic numbers, try using TRUC_MAX_VSIZE * WITNESS_SCALE_FACTOR

glozow commented at 5:33 pm on July 8, 2025:

Also, there is a separate size limit for TRUC children - I don’t think that’s been implemented yet?

ishaanam commented at 7:23 pm on July 8, 2025:

Yes, I will look into implementing the separate size limit.

in src/wallet/transaction.h:261 in 75d606c564 outdated

257@@ -258,6 +258,9 @@ class CWalletTx
258     // BlockConflicted.
259     std::set<Txid> mempool_conflicts;
260 
261+    // Set of v3 transactions that spend from this tx

glozow commented at 5:25 pm on July 8, 2025:

Comment seems inaccurate: it mentions a set but this only allows for 1 transaction. Should also mention (1) that this is used to stop us from creating another unconfirmed child and (2) this is specifically the in mempool-sibling, as there can be multiple siblings but only 1 in mempool (unless there was a reorg).

ishaanam commented at 7:23 pm on July 8, 2025:

Done

in src/wallet/spend.h:87 in 75d606c564 outdated

82@@ -83,6 +83,8 @@ struct CoinFilterParams {
83     bool include_immature_coinbase{false};
84     // By default, skip locked UTXOs
85     bool skip_locked{true};
86+    // Whether or not to care about the tx version
87+    bool track_version{true}; // only used by AvailableCoinsListUnspent

glozow commented at 5:37 pm on July 8, 2025:

A more descriptive name may be exclude_version3.

ishaanam commented at 7:26 pm on July 8, 2025:

I’m not sure if that name would be accurate, because this boolean is set to true even when we are trying to create a v3 transaction, in which case we are technically not excluding version 3.

glozow commented at 2:12 pm on July 24, 2025:

Ah right! Resolving

glozow commented at 6:49 pm on August 1, 2025:

1910d9b61f23f65335367f2f8dd021ac1ccd907a

Could be more specific. Also, this isn’t only used by AvailableCoinsListUnspent. It’s always checked in AvailableCoins, but AvailableCoinsListUnspent is the only caller that disables it because it’s not building a transaction.

0    // When true, filter unconfirmed coins by whether their version's TRUCness matches what is set by CCoinControl.
1    bool check_version_trucness{true};

glozow commented at 5:41 pm on July 8, 2025: member

Nice, concept ACK! Saw that these tests fail on #31936, which is helpful for showing its issues. Ultimately, I think the wallet commits should be introduced before the RPC ones.

ishaanam force-pushed on Jul 8, 2025

in test/functional/test_framework/messages.py:84 in 7fdff3ff7c outdated

79@@ -80,6 +80,9 @@
80 
81 DEFAULT_MEMPOOL_EXPIRY_HOURS = 336  # hours
82 
83+TX_MIN_STANDARD_VERSION = 1
84+TX_MAX_STANDARD_VERSION = 3

maflcko commented at 6:25 am on July 9, 2025:

there’s also test/functional/feature_taproot.py:TX_MAX_STANDARD_VERSION = 3

ishaanam commented at 8:28 pm on July 9, 2025:

I’ve changed this test to import TX_MAX_STANDARD_VERSION instead.

in src/rpc/rawtransaction_util.cpp:161 in 7fdff3ff7c outdated

154@@ -154,6 +155,13 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
155         rawTx.nLockTime = nLockTime;
156     }
157 
158+    if (!version.isNull()) {
159+        uint32_t nVersion = version.getInt<uint32_t>();
160+        if (nVersion < TX_MIN_STANDARD_VERSION || nVersion > TX_MAX_STANDARD_VERSION)
161+            throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, version out of range(") + util::ToString(TX_MIN_STANDARD_VERSION) + "~" + util::ToString(TX_MAX_STANDARD_VERSION) + ")");

maflcko commented at 6:26 am on July 9, 2025:

could use strprintf for shorter code? Also missing {} around body of the if?

ishaanam commented at 8:29 pm on July 9, 2025:

Done

ishaanam force-pushed on Jul 9, 2025

ishaanam force-pushed on Jul 10, 2025

ishaanam force-pushed on Jul 14, 2025

ishaanam marked this as ready for review on Jul 14, 2025

ishaanam commented at 2:46 pm on July 14, 2025: contributor

This PR is now ready for review.

glozow added the label Wallet on Jul 14, 2025

glozow added the label RPC/REST/ZMQ on Jul 14, 2025

DrahtBot removed the label CI failed on Jul 14, 2025

in src/rpc/rawtransaction_util.cpp:161 in b3817a8220 outdated

154@@ -154,6 +155,15 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
155         rawTx.nLockTime = nLockTime;
156     }
157 
158+    if (!version.isNull()) {
159+        uint32_t nVersion = version.getInt<uint32_t>();
160+        if (nVersion < TX_MIN_STANDARD_VERSION || nVersion > TX_MAX_STANDARD_VERSION) {
161+            // throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, version out of range(") + util::ToString(TX_MIN_STANDARD_VERSION) + "~" + util::ToString(TX_MAX_STANDARD_VERSION) + ")");

rkrux commented at 3:14 pm on July 15, 2025:

This commented error should be removed now.

ishaanam commented at 8:28 pm on July 15, 2025:

Done

rkrux commented at 3:17 pm on July 15, 2025: contributor

Concept ACK b3817a822054fb2ea2964535b5a0ee55dd5d55ac Thanks for picking this up, I will try to review it soon.

The TODOs seem to be done and can be removed from the PR description.

in src/rpc/rawtransaction.cpp:441 in b3817a8220 outdated

437@@ -437,7 +438,7 @@ static RPCHelpMan createrawtransaction()
438     if (!request.params[3].isNull()) {
439         rbf = request.params[3].get_bool();
440     }
441-    CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf);
442+    CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf, request.params[4]);

maflcko commented at 4:47 pm on July 15, 2025:

0    CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf, self.Arg<uint32_t>("version"));

nit: This should allow to drop the parsing in ConstructTransaction and the manual indexing here.

ishaanam commented at 8:28 pm on July 15, 2025:

Done

ishaanam force-pushed on Jul 15, 2025

DrahtBot added the label CI failed on Jul 15, 2025

DrahtBot commented at 11:39 pm on July 15, 2025: contributor

🚧 At least one of the CI tasks failed. Task macOS-cross, gui, no tests: https://github.com/bitcoin/bitcoin/runs/46044194546 LLM reason (✨ experimental): The CI failure is caused by a linker error due to an undefined symbol in RPCHelpMan.

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.

in src/rpc/rawtransaction.cpp:441 in 657d153c26 outdated

437@@ -437,7 +438,7 @@ static RPCHelpMan createrawtransaction()
438     if (!request.params[3].isNull()) {
439         rbf = request.params[3].get_bool();
440     }
441-    CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf);
442+    CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf, self.Arg<unsigned long>("version"));

maflcko commented at 6:41 am on July 16, 2025:

nit: Using uint32_t and something like this should fix the linker error:

 0diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
 1index 5da02b4df4..0604dec2dc 100644
 2--- a/src/rpc/util.cpp
 3+++ b/src/rpc/util.cpp
 4@@ -731,6 +731,7 @@ TMPL_INST(CheckRequiredOrDefault, const UniValue&, *CHECK_NONFATAL(maybe_arg););
 5 TMPL_INST(CheckRequiredOrDefault, bool, CHECK_NONFATAL(maybe_arg)->get_bool(););
 6 TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>(););
 7 TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>(););
 8+TMPL_INST(CheckRequiredOrDefault, uint32_t, CHECK_NONFATAL(maybe_arg)->getInt<uint32_t>(););
 9 TMPL_INST(CheckRequiredOrDefault, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str(););
10 
11 bool RPCHelpMan::IsValidNumArgs(size_t num_args) const

ishaanam commented at 3:51 pm on July 16, 2025:

this fixed the error, thanks!

ishaanam force-pushed on Jul 16, 2025

DrahtBot removed the label CI failed on Jul 16, 2025

in test/functional/wallet_v3_txs.py:29 in b7f8be8c4d outdated

24+    assert_greater_than,
25+    assert_raises_rpc_error,
26+)
27+
28+TRUC_MAX_VSIZE = 10000
29+TRUC_CHILD_MAX_VSIZE = 1000

glozow commented at 7:22 pm on July 16, 2025:

maybe these should live in mempool_util?

ishaanam commented at 5:23 pm on July 17, 2025:

Done

in test/functional/wallet_v3_txs.py:82 in b7f8be8c4d outdated

74+        raw_tx = from_node.signrawtransactionwithwallet(raw_tx)["hex"]
75+        txid = from_node.sendrawtransaction(raw_tx)
76+        self.sync_mempools()
77+        return txid
78+
79+    def bulk_tx(self, tx, amount, target_vsize):

glozow commented at 7:23 pm on July 16, 2025:

This looks like it was copy-pasted from the test framework - can we just use the existing method, perhaps with a wrapper if needed?

ishaanam commented at 5:24 pm on July 17, 2025:

I’ve added a bulk_vout function to script_util.py which is used by MiniWallet and these tests.

in test/functional/wallet_v3_txs.py:129 in b7f8be8c4d outdated

124+        self.log.info("Test unavailable funds when v3 tx spends unconfirmed v2 tx")
125+
126+        self.generate(self.nodes[2], 1)
127+
128+        # by default, sendall uses tx version 2
129+        self.charlie.sendall([self.bob.getnewaddress()])

glozow commented at 7:27 pm on July 16, 2025:

Might want to assert that it’s v2

ishaanam commented at 5:30 pm on July 17, 2025:

I’ve added a version 2 assertion everywhere that sendall is used in the tests.

in test/functional/wallet_v3_txs.py:195 in b7f8be8c4d outdated

195+        alice_unspent = self.alice.listunspent(minconf=0)[0]
196+        inputs=[{'txid' : parent_txid, 'vout' : alice_unspent['vout']},]
197+        outputs = {self.alice.getnewaddress() : alice_unspent['amount'] - Decimal(0.00000120)} # two outputs
198+        self.send_tx(self.alice, inputs, outputs, 3)
199+
200+        # bob tries to spend money

glozow commented at 7:32 pm on July 16, 2025:

Should there also be a test where Alice tries to spend her change after Bob has spent from the parent? That wouldn’t require include_unsafe IIUC

ishaanam commented at 5:31 pm on July 17, 2025:

I’ve added a test for alice spending change

in test/functional/wallet_v3_txs.py:172 in b7f8be8c4d outdated

167+        assert_raises_rpc_error(
168+            -4,
169+            "Insufficient funds",
170+            self.bob.fundrawtransaction,
171+            raw_tx_v2, {'include_unsafe': True}
172+        )

glozow commented at 7:33 pm on July 16, 2025:

Could you also add v2_tx_spends_confirmed_v3_tx and v3_tx_spends_confirmed_v2_tx to check that version mismatches are fine there?

ishaanam commented at 5:31 pm on July 17, 2025:

Done

in test/functional/wallet_v3_txs.py:293 in b7f8be8c4d outdated

264+        parent_txid = self.send_tx(self.charlie, inputs, outputs, 3)
265+
266+        # alice spends her output with a v3 transaction
267+        alice_unspent = self.alice.listunspent(minconf=0)[0]
268+        inputs=[{'txid' : parent_txid, 'vout' : alice_unspent['vout']},]
269+        outputs = {self.alice.getnewaddress() : alice_unspent['amount'] - Decimal(0.00000120)} # two outputs

glozow commented at 7:36 pm on July 16, 2025:

What does “two outputs” mean here? This has 1 output, no?

ishaanam commented at 5:31 pm on July 17, 2025:

Yes, this was an outdated comment which I’ve now removed

in test/functional/wallet_v3_txs.py:302 in b7f8be8c4d outdated

273+        bob_unspent = self.bob.listunspent(minconf=0)[0]
274+        inputs=[{'txid' : parent_txid, 'vout' : bob_unspent['vout']},]
275+        outputs = {self.bob.getnewaddress() : bob_unspent['amount'] - Decimal(0.00010120)} # two outputs
276+        bob_txid = self.send_tx(self.bob, inputs, outputs, 3)
277+
278+        assert_equal(self.alice.gettransaction(alice_txid)['mempoolconflicts'], [bob_txid])

glozow commented at 7:37 pm on July 16, 2025:

Will Alice’s wallet attempt to rebroadcast her transaction if/when the parent + Bob’s confirm? Can we do an RBF of Alice’s transaction to evict Bob’s?

ishaanam commented at 5:36 pm on July 17, 2025:

Yes, because when Bob confirms, it is removed from mempool_conflicts and Alice’s transaction is considered “Inactive”. I’ve added a part to this test case where Alice evicts Bob’s transaction.

in test/functional/wallet_v3_txs.py:284 in b7f8be8c4d outdated

279+
280+    @cleanup
281+    def v3_conflict_removed_from_mempool(self):
282+        self.log.info("Test a v3 conflict being removed")
283+        self.generate(self.nodes[2], 1)
284+        # send a v2 output to alice

glozow commented at 7:40 pm on July 16, 2025:

0        # send a v2 output to alice and confirm it

ishaanam commented at 5:31 pm on July 17, 2025:

Done

in test/functional/wallet_v3_txs.py:292 in b7f8be8c4d outdated

287+        # create a v3 tx to alice and bob
288+        inputs=[]
289+        outputs = {self.alice.getnewaddress() : 2.0, self.bob.getnewaddress() : 2.0}
290+        self.send_tx(self.charlie, inputs, outputs, 3)
291+
292+        alice_v2_unspent = [unspent for unspent in self.alice.listunspent(minconf=0) if unspent["confirmations"] != 0][0]

glozow commented at 7:42 pm on July 16, 2025:

Isn’t this just…?

0        alice_v2_unspent = self.alice.listunspent(minconf=1)[0]

ishaanam commented at 5:32 pm on July 17, 2025:

Yes, I’ve updated both of the places where I was doing this.

in test/functional/wallet_v3_txs.py:351 in b7f8be8c4d outdated

315+        inputs=[]
316+        outputs = {self.bob.getnewaddress() : 1.999}
317+        self.send_tx(self.bob, inputs, outputs, 3)
318+
319+    @cleanup
320+    def mempool_conflicts_removed_when_v3_conflict_removed(self):

glozow commented at 7:47 pm on July 16, 2025:

Can this be consolidated with v3_conflict_removed_from_mempool since they are mostly the same?

ishaanam commented at 5:38 pm on July 17, 2025:

It would be tricky to consolidate the two because one of them is testing preventing the wallet from creating a transaction and the other is testing marking conflicts correctly.

in src/wallet/spend.cpp:934 in 827fdc4904 outdated

930             auto it = filtered_groups.find(select_filter.filter);
931             if (it == filtered_groups.end()) continue;
932+            if (temp_selection_params.m_version == TRUC_VERSION && (select_filter.filter.conf_mine == 0 || select_filter.filter.conf_theirs == 0)) {
933+                if (temp_selection_params.m_max_tx_weight > (TRUC_CHILD_MAX_VSIZE * WITNESS_SCALE_FACTOR)) {
934+                    temp_selection_params.m_max_tx_weight = TRUC_CHILD_MAX_VSIZE * WITNESS_SCALE_FACTOR;
935+                }

glozow commented at 7:55 pm on July 16, 2025:

The approach of modifying the CoinSelectionParams.m_max_tx_weight seems fine to me. The only issue I notice is the error message being “The inputs size exceeds the maximum weight. Please try sending a smaller amount or manually consolidating your wallet’s UTXOs”, which could be slightly confusing, but I think out of scope for this PR. A separate PR could maybe tell the user what the limit was, since it could be MAX_STANDARD_TX_WEIGHT, max TRUC weight, max child TRUC weight, or ancestor/cluster size limits (oh, just found out that isn’t handled - also a potential followup).

ishaanam force-pushed on Jul 17, 2025

DrahtBot added the label CI failed on Jul 17, 2025

DrahtBot commented at 5:30 pm on July 17, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/46200015717 LLM reason (✨ experimental): The failure is caused by lint errors detected by ruff due to unused imports in Python files.

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.

ishaanam force-pushed on Jul 17, 2025

ishaanam commented at 6:08 pm on July 17, 2025: contributor

Thanks for the review @glozow! I’ve addressed your comments and also refactored a few of the functional tests.

DrahtBot removed the label CI failed on Jul 17, 2025

DrahtBot added the label Needs rebase on Jul 18, 2025

wallet: unconfirmed ancestors and descendants are always truc 1910d9b61f

ishaanam force-pushed on Jul 21, 2025

DrahtBot removed the label Needs rebase on Jul 21, 2025

in src/wallet/spend.cpp:287 in cf712ab56e outdated

282@@ -283,6 +283,16 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
283             if (input_bytes == -1) {
284                 input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control);
285             }
286+            auto it = wallet.mapWallet.find(outpoint.hash);
287+            if (coin_control.m_version.has_value() && it != wallet.mapWallet.end()) {

murchandamus commented at 10:44 pm on July 21, 2025:

In “wallet: throw error at conflicting tx versions in pre-selected inputs” (cf712ab56e3256db5dfcf422860bdb3cf87cd03e): Is m_version always set? Otherwise, what would happen if coin_control.m_version is not set, and one preselected input is TRUC, but another one is not?

ishaanam commented at 3:43 pm on July 23, 2025:

This is a good point, if coin_control.m_version is not set and the preselected inputs have different versions, then an error would not be thrown. I have changed m_version so that it has a default value of CTransaction::CURRENT_VERSION instead of being optional.

in src/wallet/wallet.cpp:1402 in bff006b7ae outdated

1393@@ -1393,6 +1394,21 @@ void CWallet::transactionAddedToMempool(const CTransactionRef& tx) {
1394                 return wtx.mempool_conflicts.insert(txid).second ? TxUpdate::CHANGED : TxUpdate::UNCHANGED;
1395             });
1396         }
1397+
1398+    }
1399+
1400+    if (tx->version == TRUC_VERSION) {
1401+        // this makes an unconfirmed v3 output unspendable
1402+        // for all of the utxos that this spends...

murchandamus commented at 10:56 pm on July 21, 2025:

In “wallet: don’t include unconfirmed v3 txs with children in available coins” (bff006b7ae7fa120fac4f2118e891d2c88d38f9a):

I find this comment hard to parse. Do you mean something along the lines of:

0        // this marks all outputs of any unconfirmed v3 parent transactions as unspendable
1        // if any of its outputs are spent by this transaction

ishaanam commented at 3:43 pm on July 23, 2025:

Thanks, I’ve updated the comment

in test/functional/wallet_v3_txs.py:237 in 46f4f372b8 outdated

225+            self.alice.fundrawtransaction,
226+            alice_tx
227+        )
228+
229+    @cleanup
230+    def spend_inputs_with_different_versions(self, version_a, version_b):

murchandamus commented at 11:15 pm on July 21, 2025:

Following the prior comment, I think it would be good to have a test where you are trying to build a transaction for which the version is not set, that has both a pre-selected version 1|2 and a pre-selected version 3 input.

ishaanam commented at 3:44 pm on July 23, 2025:

I’ve added a test for this.

murchandamus commented at 11:16 pm on July 21, 2025: contributor

Concept ACK

ishaanam force-pushed on Jul 23, 2025

DrahtBot added the label CI failed on Jul 23, 2025

DrahtBot commented at 3:47 pm on July 23, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/46575305874 LLM reason (✨ experimental): Lint errors caused by f-string syntax issues caused the CI failure.

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.

ishaanam force-pushed on Jul 23, 2025

in src/wallet/spend.cpp:287 in 40c6d135f3 outdated

282@@ -283,6 +283,16 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
283             if (input_bytes == -1) {
284                 input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control);
285             }
286+            auto it = wallet.mapWallet.find(outpoint.hash);
287+            if (it != wallet.mapWallet.end()) {

achow101 commented at 6:28 pm on July 23, 2025:

In 40c6d135f37a1d8dd64cbea3ca30229c2e012fa2 “wallet: throw error at conflicting tx versions in pre-selected inputs”

We want to avoid querying mapWallet as much as possible for performance. txo.GetWalletTx returns a reference to the CWalletTx that can be used here instead of a mapWallet lookup.

ishaanam commented at 6:12 pm on July 28, 2025:

Done

in src/rpc/rawtransaction.cpp:1683 in 2bcdf53a58 outdated

1679@@ -1679,7 +1680,7 @@ static RPCHelpMan createpsbt()
1680     if (!request.params[3].isNull()) {
1681         rbf = request.params[3].get_bool();
1682     }
1683-    CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf);
1684+    CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf, DEFAULT_RAWTX_VERSION);

achow101 commented at 6:35 pm on July 23, 2025:

In 2bcdf53a58ee6cd7c70f2afd97a16fce5dcc21c6 “rpc: Support v3 raw transactions creation”

If createrawtransaction is going to support setting the version, then createpsbt, walletcreatefundedpsbt, send, and sendall need to as well.

ishaanam commented at 6:12 pm on July 28, 2025:

Done, will add additional tests for these RPCs as well.

DrahtBot removed the label CI failed on Jul 23, 2025

in src/wallet/spend.cpp:395 in 1910d9b61f outdated

386@@ -386,6 +387,14 @@ CoinsResult AvailableCoins(const CWallet& wallet,
387                 safeTx = false;
388             }
389 
390+            if (nDepth == 0 && params.track_version) {
391+                if (coinControl->m_version == TRUC_VERSION) {
392+                    if (wtx.tx->version != TRUC_VERSION) continue;
393+                } else {
394+                    if (wtx.tx->version == TRUC_VERSION) continue;
395+                }

glozow commented at 2:12 pm on July 24, 2025:

This might be clearer: EDIT: ah nvm, I see you use the 2 clauses further in later commits. Feel free to ignore.

0
1                // an unconfirmed TRUC must spend only TRUC, and non-TRUC must only spend non-TRUC
2                if ((wtx.tx->version == TRUC_VERSION) != (coinControl->m_version == TRUC_VERSION)) continue;

in src/wallet/wallet.cpp:1424 in 279b45e9ba outdated

1404+            auto wallet_it = mapWallet.find(tx_in.prevout.hash);
1405+            if (wallet_it != mapWallet.end()) {
1406+                CWalletTx& wtx = wallet_it->second;
1407+                if (wtx.isUnconfirmed()) {
1408+                    wtx.v3_spend = tx->GetHash();
1409+                }

glozow commented at 2:35 pm on July 24, 2025:

nit: Naming these parent_it and parent_wtx would make it easier to understand

ishaanam commented at 6:18 pm on July 28, 2025:

Done

in src/wallet/wallet.cpp:1401 in 279b45e9ba outdated

1393@@ -1393,6 +1394,21 @@ void CWallet::transactionAddedToMempool(const CTransactionRef& tx) {
1394                 return wtx.mempool_conflicts.insert(txid).second ? TxUpdate::CHANGED : TxUpdate::UNCHANGED;
1395             });
1396         }
1397+
1398+    }
1399+
1400+    if (tx->version == TRUC_VERSION) {
1401+        // this marks all outputs of any unconfirmed v3 parent transactions as unspendable

glozow commented at 2:38 pm on July 24, 2025:

nit: I think “marks as unspendable” seems a little strong? Perhaps also explain why you are doing this

0        // Unconfirmed TRUC transactions are only allowed a 1-parent-1-child topology.
1        // For any unconfirmed v3 parents (there should be a maximum of 1 except in reorgs),
2        // record this child so the wallet doesn't try to spend any other outputs

ishaanam commented at 6:13 pm on July 28, 2025:

Done

in src/wallet/wallet.cpp:1466 in 279b45e9ba outdated

1461@@ -1446,6 +1462,19 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRe
1462             });
1463         }
1464     }
1465+
1466+    // reverse what happens in transactionAddedToMempool

glozow commented at 2:44 pm on July 24, 2025:

Maybe be more specific? Unset v3_spend to make it possible to spend from this transaction again. If this transaction was replaced by another child of the same parent, their transactionAddedToMempool will update v3_spend.

ishaanam commented at 6:13 pm on July 28, 2025:

Done

in src/wallet/spend.cpp:403 in 279b45e9ba outdated

389@@ -390,6 +390,7 @@ CoinsResult AvailableCoins(const CWallet& wallet,
390             if (nDepth == 0 && params.track_version) {
391                 if (coinControl->m_version == TRUC_VERSION) {
392                     if (wtx.tx->version != TRUC_VERSION) continue;
393+                    if (wtx.v3_spend.has_value()) continue; // this unconfirmed v3 transaction already has a child
394                 } else {
395                     if (wtx.tx->version == TRUC_VERSION) continue;

glozow commented at 2:45 pm on July 24, 2025:

I think you could Assume(!wtx.v3_spend.has_value()) here

ishaanam commented at 7:11 pm on July 25, 2025:

Why is it safe to assume that the transaction doesn’t already have a child here?

glozow commented at 8:49 pm on July 25, 2025:

non-TRUC transactions shouldn’t have TRUC children. I think it would be a bug if we set this variable for a transaction that isn’t TRUC

ishaanam commented at 2:19 pm on July 28, 2025:

Oh, that makes sense, I thought that you were talking about changing the if-statement on line 393.

in src/wallet/transaction.h:263 in 279b45e9ba outdated

257@@ -258,6 +258,10 @@ class CWalletTx
258     // BlockConflicted.
259     std::set<Txid> mempool_conflicts;
260 
261+    // Track v3 mempool tx that spends from this tx
262+    // so that we don't try to create another unconfirmed child
263+    std::optional<Txid> v3_spend;

glozow commented at 2:46 pm on July 24, 2025:

Naming everywhere should replace “v3” with “truc.” We have so many versions and “legacy” types everywhere, so it’s best to try to reduce ambiguity when possible. Also, “child” is more clear than “spend,” and “in_mempool” would also help with clarity.

0    std::optional<Txid> truc_child_in_mempool;

ishaanam commented at 6:13 pm on July 28, 2025:

Done

in src/wallet/coincontrol.h:115 in eecff95b6f outdated

108@@ -109,10 +109,10 @@ class CCoinControl
109     int m_max_depth = DEFAULT_MAX_DEPTH;
110     //! SigningProvider that has pubkeys and scripts to do spend size estimation for external inputs
111     FlatSigningProvider m_external_provider;
112+    //! Version
113+    uint32_t m_version = CTransaction::CURRENT_VERSION;
114     //! Locktime
115     std::optional<uint32_t> m_locktime;
116-    //! Version
117-    std::optional<uint32_t> m_version;

glozow commented at 2:49 pm on July 24, 2025:

Question: why is eecff95b6fe611c8ed40895108aacd6b44543234 necessary? It would be helpful to put the motivation in the commit message.

ishaanam commented at 6:14 pm on July 28, 2025:

Added description to commit message.

in src/wallet/wallet.cpp:1409 in 2f62dec368 outdated

1405@@ -1406,6 +1406,17 @@ void CWallet::transactionAddedToMempool(const CTransactionRef& tx) {
1406                 CWalletTx& wtx = wallet_it->second;
1407                 if (wtx.isUnconfirmed()) {
1408                     wtx.v3_spend = tx->GetHash();
1409+                    // Find all wallet transactions that spend utxos from this tx

glozow commented at 2:52 pm on July 24, 2025:

Again would be helpful to add one or two sentences to explain why we need to do this and why marking as mempool conflict is the appropriate treatment for a sibling (I don’t think it’s immediately obvious to everyone).

ishaanam commented at 6:15 pm on July 28, 2025:

Done

in src/wallet/wallet.cpp:1412 in 2f62dec368 outdated

1405@@ -1406,6 +1406,17 @@ void CWallet::transactionAddedToMempool(const CTransactionRef& tx) {
1406                 CWalletTx& wtx = wallet_it->second;
1407                 if (wtx.isUnconfirmed()) {
1408                     wtx.v3_spend = tx->GetHash();
1409+                    // Find all wallet transactions that spend utxos from this tx
1410+                    for (long unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
1411+                        for (auto range = mapTxSpends.equal_range(COutPoint(wtx.tx->GetHash(), i)); range.first != range.second; range.first++) {
1412+                            const Txid& spent_id = range.first->second;

glozow commented at 2:53 pm on July 24, 2025:

Naming this sibling_txid would be clearer

ishaanam commented at 6:16 pm on July 28, 2025:

Done

in src/wallet/rpc/spend.cpp:1322 in e384f69d94 outdated

1309@@ -1303,6 +1310,13 @@ RPCHelpMan send()
1310             if (options.exists("max_tx_weight")) {
1311                 coin_control.m_max_tx_weight = options["max_tx_weight"].getInt<int>();
1312             }
1313+
1314+            if (rawTx.version == TRUC_VERSION) {
1315+                if (!coin_control.m_max_tx_weight.has_value() || coin_control.m_max_tx_weight.value() > TRUC_MAX_VSIZE * WITNESS_SCALE_FACTOR) {
1316+                    coin_control.m_max_tx_weight = TRUC_MAX_VSIZE * WITNESS_SCALE_FACTOR;
1317+                }
1318+            }

glozow commented at 2:56 pm on July 24, 2025:

m_max_tx_weight should be corrected to the minimum of the original value and the TRUC maximum, otherwise you might be overriding what the user passed in above.

glozow commented at 2:57 pm on July 24, 2025:

(perhaps add a test case for this?)

ishaanam commented at 6:16 pm on July 28, 2025:

I’ve corrected the code and will add a test case for this

in src/policy/policy.h:150 in f9ed2875ab outdated

146@@ -147,6 +147,7 @@ std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate);
147 // allowing the new transaction version in the wallet/RPC.
148 static constexpr decltype(CTransaction::version) TX_MIN_STANDARD_VERSION{1};
149 static constexpr decltype(CTransaction::version) TX_MAX_STANDARD_VERSION{3};
150+static constexpr decltype(CTransaction::version) DEFAULT_RAWTX_VERSION{CTransaction::CURRENT_VERSION};

glozow commented at 2:58 pm on July 24, 2025:

f9ed2875ab6fbe073bc8ca6192b771bda0db7af2: this is unused in the commit and should be squashed into the next one. Also, I think this should live rawtransaction.cpp and wallet should have its own default version constexpr

ishaanam commented at 6:16 pm on July 28, 2025:

Done

in src/wallet/rpc/spend.cpp:1305 in 2bcdf53a58 outdated

1301@@ -1302,7 +1302,7 @@ RPCHelpMan send()
1302                     ParseOutputs(outputs),
1303                     InterpretSubtractFeeFromOutputInstructions(options["subtract_fee_from_outputs"], outputs.getKeys())
1304             );
1305-            CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf);
1306+            CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf, DEFAULT_RAWTX_VERSION);

glozow commented at 3:00 pm on July 24, 2025:

I think perhaps the wallet’s default tx version should be specified in a separate constexpr that lives in the wallet code?

ishaanam commented at 6:16 pm on July 28, 2025:

Done

in src/policy/policy.cpp:101 in 2bcdf53a58 outdated

 97@@ -98,7 +98,7 @@ bool IsStandard(const CScript& scriptPubKey, TxoutType& whichType)
 98 
 99 bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_datacarrier_bytes, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& reason)
100 {
101-    if (tx.version > TX_MAX_STANDARD_VERSION || tx.version < 1) {
102+    if (tx.version > TX_MAX_STANDARD_VERSION || tx.version < TX_MIN_STANDARD_VERSION) {

glozow commented at 3:07 pm on July 24, 2025:

This change is in 2bcdf53a58ee6cd7c70f2afd97a16fce5dcc21c6 but seems to belong in b07d9b92821 instead

ishaanam commented at 6:17 pm on July 28, 2025:

I’ve moved the change to the correct commit

in test/functional/test_framework/mempool_util.py:33 in 98e865fb2c outdated

29@@ -30,6 +30,11 @@
30     MiniWallet,
31 )
32 
33+ORPHAN_TX_EXPIRE_TIME = 1200

glozow commented at 3:10 pm on July 24, 2025:

Orphans don’t expire anymore and this is unused (is it from a rebase maybe?)

ishaanam commented at 6:17 pm on July 28, 2025:

I’ve removed it, pretty sure it was from a rebase

in test/functional/test_framework/wallet.py:134 in 98e865fb2c outdated

128-        dummy_vbytes = target_vsize - tx.get_vsize()
129-        # compensate for the increase of the compact-size encoded script length
130-        # (note that the length encoding of the unpadded output script needs one byte)
131-        dummy_vbytes -= len(ser_compact_size(dummy_vbytes)) - 1
132-        tx.vout[-1].scriptPubKey = CScript([OP_RETURN] + [OP_1] * dummy_vbytes)
133-        assert_equal(tx.get_vsize(), target_vsize)

glozow commented at 3:10 pm on July 24, 2025:

Maybe put the bulk_vout refactor into a separate commit

ishaanam commented at 6:17 pm on July 28, 2025:

Done

in test/functional/wallet_v3_txs.py:5 in 98e865fb2c outdated

0@@ -0,0 +1,422 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2025 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test how the wallet deals with v3 transactions"""

glozow commented at 3:10 pm on July 24, 2025:

s/v3/TRUC

ishaanam commented at 6:17 pm on July 28, 2025:

Done

glozow commented at 3:14 pm on July 24, 2025: member

Looks pretty good! Generally, I prefer more detailed comments when the code is not 100% self-explanatory, particularly for the confusing areas like mempool conflicts

wallet: don't include unconfirmed v3 txs with children in available coins 7f43b5559c

wallet: set m_version in coin control to default value

In future commits we assume that coin_control.m_version has a
value when making sure that we follow truc rules, so we should
give it a default value of CTransaction::CURRENT_VERSION.

5e3a7f6ee6

wallet: throw error at conflicting tx versions in pre-selected inputs fd436480f4

wallet: mark unconfirmed v3 siblings as mempool conflicts ac910c5091

wallet: limit v3 tx weight in coin selection 84c3c1a117

rpc: Add version parameter for transaction a9d2480791

ishaanam force-pushed on Jul 28, 2025

ishaanam commented at 6:23 pm on July 28, 2025: contributor

Thanks for the feedback @glozow and @achow101! I’m converting this to a draft while I work on some additional test cases, but otherwise the code is ready for review.

ishaanam marked this as a draft on Jul 28, 2025

ishaanam force-pushed on Jul 30, 2025

rpc: Support version 3 transaction creation

Adds v3 support to the following RPCs:
- createrawtransaction
- createpsbt
- send
- sendall
- walletcreatefundedpsbt

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>
Co-authored-by: ishaanam <ishaana.misra@gmail.com>

9b478f1f68

test: extract `bulk_vout` from `bulk_tx` so it can be used by wallet tests 0ce717c3b7

test: add truc wallet tests 5e5a254103

doc: add release notes for version 3 transactions a2c43c3b94

ishaanam force-pushed on Jul 30, 2025

DrahtBot added the label CI failed on Jul 30, 2025

ishaanam marked this as ready for review on Jul 30, 2025

ishaanam commented at 4:11 pm on July 30, 2025: contributor

I have added more tests and this PR is ready for review.

glozow added this to the milestone 30.0 on Jul 30, 2025

DrahtBot removed the label CI failed on Jul 30, 2025

in src/wallet/spend.cpp:393 in 7f43b5559c outdated

389@@ -390,8 +390,10 @@ CoinsResult AvailableCoins(const CWallet& wallet,
390             if (nDepth == 0 && params.track_version) {
391                 if (coinControl->m_version == TRUC_VERSION) {
392                     if (wtx.tx->version != TRUC_VERSION) continue;
393+                    if (wtx.truc_child_in_mempool.has_value()) continue; // this unconfirmed v3 transaction already has a child

glozow commented at 6:50 pm on August 1, 2025:

7f43b5559c5f508ac210350847bca6f0f0e091be

nit: generally prefer comments to live on separate lines

0                     // this unconfirmed v3 transaction already has a child
1                    if (wtx.truc_child_in_mempool.has_value()) continue;

in src/wallet/wallet.cpp:1473 in 7f43b5559c outdated

1468+        // unset truc_child_in_mempool to make it possible to spend from
1469+        // this again. If this tx was replaced by another
1470+        // child of the same parent, transactionAddedToMempool
1471+        // will update truc_child_in_mempool
1472+        for (const CTxIn& tx_in : tx->vin) {
1473+            auto wallet_it = mapWallet.find(tx_in.prevout.hash);

glozow commented at 6:52 pm on August 1, 2025:

nit 7f43b5559c5f508ac210350847bca6f0f0e091be

call this parent_it as well?

in src/wallet/wallet.cpp:1469 in 7f43b5559c outdated

1462@@ -1446,6 +1463,22 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRe
1463             });
1464         }
1465     }
1466+
1467+    if (tx->version == TRUC_VERSION) {
1468+        // unset truc_child_in_mempool to make it possible to spend from
1469+        // this again. If this tx was replaced by another

glozow commented at 6:53 pm on August 1, 2025:

0        // If this tx has a parent, unset its truc_child_in_mempool to make it possible
1        // to spend from the parent again. If this tx was replaced by another

in src/wallet/wallet.cpp:1492 in ac910c5091 outdated

1488@@ -1475,6 +1489,15 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRe
1489                 CWalletTx& wtx = wallet_it->second;
1490                 if (wtx.truc_child_in_mempool == tx->GetHash()) {
1491                     wtx.truc_child_in_mempool = std::nullopt;
1492+                    // Find all wallet transactions that spend utxos from this tx

glozow commented at 7:05 pm on August 1, 2025:

This comment can be confusing - we’re not actually looking for transactions spending from this tx. We’re looking for siblings of this tx, which spend from utxos of wtx (which is actually this transaction’s parent). Might be helpful to rename wallet_it to parent_it and update this comment.

in src/rpc/rawtransaction_util.h:56 in 9b478f1f68 outdated

52@@ -53,6 +53,6 @@ std::vector<std::pair<CTxDestination, CAmount>> ParseOutputs(const UniValue& out
53 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);
57+CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional<bool> rbf, const uint32_t& version);

glozow commented at 7:10 pm on August 1, 2025:

9b478f1f68c00b5c514a30a53d19cd58fe813795

integers can just be passed by value

in doc/release-notes-32896.md:13 in a2c43c3b94

 8+- `sendall`
 9+- `walletcreatefundedpsbt`
10+
11+Wallet
12+------
13+Support has been added for spending version 3 transactions received by

glozow commented at 7:26 pm on August 1, 2025:

Now that there is send and sendall support, is it just transactions received by the wallet?

in doc/release-notes-32896.md:14 in a2c43c3b94

 9+- `walletcreatefundedpsbt`
10+
11+Wallet
12+------
13+Support has been added for spending version 3 transactions received by
14+the wallet. The wallet ensures that version 3 policy rules are being

glozow commented at 7:26 pm on August 1, 2025:

Maybe mention TRUC and/or BIP 431?

in src/wallet/spend.cpp:933 in 84c3c1a117 outdated

928         for (const auto& select_filter : ordered_filters) {
929             auto it = filtered_groups.find(select_filter.filter);
930             if (it == filtered_groups.end()) continue;
931+            if (temp_selection_params.m_version == TRUC_VERSION && (select_filter.filter.conf_mine == 0 || select_filter.filter.conf_theirs == 0)) {
932+                if (temp_selection_params.m_max_tx_weight > (TRUC_CHILD_MAX_VSIZE * WITNESS_SCALE_FACTOR)) {
933+                    temp_selection_params.m_max_tx_weight = TRUC_CHILD_MAX_VSIZE * WITNESS_SCALE_FACTOR;

glozow commented at 7:31 pm on August 1, 2025:

Missing test coverage for this condition I think? Need user_input_weight_not_overwritten test for something spending unconfirmed v3

glozow commented at 7:38 pm on August 1, 2025: member

a2c43c3b9447377d2f2635e23c9043e40b769dd4 looks pretty good to me! Minor comments mostly about docs

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

Code Coverage & Benchmarks

Reviews

Conflicts