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
ACK	glozow, achow101, rkrux
Concept ACK	murchandamus
Stale ACK	w0xlt

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)
#28201 (Silent Payments: sending by josibake)
#27865 (wallet: Track no-longer-spendable TXOs separately 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};

ishaanam commented at 3:25 pm on August 4, 2025:

Done

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:79 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:181 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:294 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

achow101 commented at 10:11 pm on August 4, 2025:

The comment still seems to be there.

ishaanam commented at 7:41 pm on August 5, 2025:

The comment was in multiple places, I’ve removed it now.

in test/functional/wallet_v3_txs.py:278 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:321 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:962 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

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:222 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:409 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:1425 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:407 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

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

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;

ishaanam commented at 3:26 pm on August 4, 2025:

Done

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?

ishaanam commented at 3:26 pm on August 4, 2025:

Done

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

ishaanam commented at 3:26 pm on August 4, 2025:

Done

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.

ishaanam commented at 3:27 pm on August 4, 2025:

Done

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

ishaanam commented at 3:27 pm on August 4, 2025:

Done

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

 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?

ishaanam commented at 3:27 pm on August 4, 2025:

Done

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

 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?

ishaanam commented at 3:27 pm on August 4, 2025:

Done

in src/wallet/spend.cpp:962 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

ishaanam commented at 3:27 pm on August 4, 2025:

I’ve added an additional test case.

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

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

ishaanam force-pushed on Aug 4, 2025

DrahtBot added the label CI failed on Aug 4, 2025

in src/wallet/wallet.cpp:1409 in 0256da5083 outdated

1403@@ -1404,9 +1404,23 @@ void CWallet::transactionAddedToMempool(const CTransactionRef& tx) {
1404         for (const CTxIn& tx_in : tx->vin) {
1405             auto parent_it = mapWallet.find(tx_in.prevout.hash);
1406             if (parent_it != mapWallet.end()) {
1407-                CWalletTx& parent_wtx = parent_it->second;
1408-                if (parent_wtx.isUnconfirmed()) {
1409-                    parent_wtx.truc_child_in_mempool = tx->GetHash();
1410+                CWalletTx& parent_tx = parent_it->second;
1411+                if (parent_tx.isUnconfirmed()) {
1412+                    parent_tx.truc_child_in_mempool = tx->GetHash();

achow101 commented at 9:21 pm on August 4, 2025:

In 0256da5083b6a430993594b227fbae4e005e6024 “wallet: mark unconfirmed v3 siblings as mempool conflicts”

nit: This code is unchanged, renaming is not necessary here.

ishaanam commented at 7:34 pm on August 5, 2025:

I’ve fixed how these variables are named.

in src/wallet/rpc/spend.cpp:1320 in 8d085d4c2c outdated

1315+                if (coin_control.m_max_tx_weight.has_value()) {
1316+                    coin_control.m_max_tx_weight = std::min<long int>(coin_control.m_max_tx_weight.value(), TRUC_MAX_VSIZE * WITNESS_SCALE_FACTOR);
1317+                } else {
1318+                    coin_control.m_max_tx_weight = TRUC_MAX_VSIZE * WITNESS_SCALE_FACTOR;
1319+                }
1320+            }

achow101 commented at 9:28 pm on August 4, 2025:

In 8d085d4c2ca451af75a9adfcaebb8cc0ccb3d86c “wallet: limit v3 tx weight in coin selection”

This check is redundant since FundTransaction is being called and it already is setting the weight appropriately.

ishaanam commented at 7:34 pm on August 5, 2025:

I’ve removed it.

in src/rpc/rawtransaction_util.cpp:159 in 894a05477e outdated

154@@ -154,6 +155,11 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
155         rawTx.nLockTime = nLockTime;
156     }
157 
158+    if (version < TX_MIN_STANDARD_VERSION || version > TX_MAX_STANDARD_VERSION) {
159+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, version out of range(%s~%s)", util::ToString(TX_MIN_STANDARD_VERSION), util::ToString(TX_MAX_STANDARD_VERSION)));

achow101 commented at 9:38 pm on August 4, 2025:

In 894a05477e23bddb7385467d0dc07739d8557b6c “rpc: Support version 3 transaction creation”

nit: util::ToString is unnecessary if %d is used as the format specifier.

0        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, version out of range(%d~%d)", TX_MIN_STANDARD_VERSION, TX_MAX_STANDARD_VERSION));

ishaanam commented at 7:34 pm on August 5, 2025:

Done

in test/functional/wallet_v3_txs.py:92 in 09c9f82825 outdated

87+        test_func(2, 3)
88+        test_func(3, 2)
89+
90+    def run_test(self):
91+        self.connect_nodes(0, 1)
92+        self.connect_nodes(0, 2)

achow101 commented at 9:52 pm on August 4, 2025:

In 09c9f828250e3840d7557374ff568204305e2679 “test: add truc wallet tests”

This seems unnecessary as setup_network will already connect our nodes, and it doesn’t look like this test does any disconnections or requires a particular node topography.

ishaanam commented at 7:34 pm on August 5, 2025:

I’ve removed it

in test/functional/wallet_v3_txs.py:68 in 09c9f82825 outdated

63+    def skip_test_if_missing_module(self):
64+        self.skip_if_no_wallet()
65+
66+    def set_test_params(self):
67+        getcontext().prec=10
68+        self.num_nodes = 3

achow101 commented at 9:54 pm on August 4, 2025:

In 09c9f828250e3840d7557374ff568204305e2679 “test: add truc wallet tests”

It looks like we end up having just one wallet per node, and the tests only really care about the wallet separation, not that they are on different nodes. So this should be just self.num_nodes = 1 with multiple wallets on that single node.

ishaanam commented at 7:35 pm on August 5, 2025:

Done

in test/functional/wallet_v3_txs.py:152 in 09c9f82825 outdated

147+            raw_tx_v2, {'include_unsafe': True}
148+        )
149+
150+    @cleanup
151+    def v2_tx_spends_confirmed_v3_tx(self, version_a, version_b):
152+        self.log.info(f"Test unavailable funds when v{version_a} tx spends confirmed v{version_b} tx")

achow101 commented at 9:59 pm on August 4, 2025:

In 09c9f828250e3840d7557374ff568204305e2679 “test: add truc wallet tests”

s/unavailable/available

ishaanam commented at 7:35 pm on August 5, 2025:

Done

in test/functional/wallet_v3_txs.py:188 in 09c9f82825 outdated

183+        self.generate(self.nodes[2], 1)
184+
185+        inputs=[]
186+        outputs = {self.alice.getnewaddress() : 2.0, self.bob.getnewaddress() : 2.0}
187+        self.send_tx(self.charlie, inputs, outputs, 3)
188+        parent_txid = self.charlie.getrawmempool()[0]

achow101 commented at 10:00 pm on August 4, 2025:

In 09c9f828250e3840d7557374ff568204305e2679 “test: add truc wallet tests”

The getrawmempool can be avoided:

0        parent_txid = self.send_tx(self.charlie, inputs, outputs, 3)

ishaanam commented at 7:36 pm on August 5, 2025:

Done in both places where this was applicable.

in test/functional/wallet_v3_txs.py:39 in 09c9f82825 outdated

32+)
33+
34+def cleanup(func):
35+    def wrapper(self, *args):
36+        try:
37+            func(self, *args)

achow101 commented at 10:02 pm on August 4, 2025:

In 09c9f828250e3840d7557374ff568204305e2679 “test: add truc wallet tests”

Since all of the test cases do a self.generate, that could be added here as well before func.

ishaanam commented at 7:37 pm on August 5, 2025:

Done

in test/functional/wallet_v3_txs.py:193 in 09c9f82825 outdated

204+            self.bob.fundrawtransaction,
205+            bob_tx, {'include_unsafe': True}
206+        )
207+
208+    @cleanup
209+    def truc_tx_with_conflicting_sibling_change(self):

achow101 commented at 10:07 pm on August 4, 2025:

In 09c9f828250e3840d7557374ff568204305e2679 “test: add truc wallet tests”

I don’t see how this test is materially different from truc_tx_with_conflicting_sibling. Based on the naming, I assume this was added in response to https://github.com/bitcoin/bitcoin/pull/32896/commits/09c9f828250e3840d7557374ff568204305e2679#r2211372152? However, this test isn’t testing anything with a change output since the parent tx is from charlie.

ishaanam commented at 7:38 pm on August 5, 2025:

I’ve updated this test so it actually tests spending change now.

in test/functional/wallet_v3_txs.py:328 in 09c9f82825 outdated

323+
324+        alice_v2_unspent = self.alice.listunspent(minconf=1)[0]
325+        alice_unspent = [unspent for unspent in self.alice.listunspent(minconf=0) if unspent["confirmations"] == 0][0]
326+
327+        # alice spends both of her outputs
328+        inputs = [{'txid' : alice_v2_unspent['txid'], 'vout' : 0}, {'txid' : alice_unspent['txid'], 'vout' : alice_unspent['vout']}]

achow101 commented at 10:15 pm on August 4, 2025:

In 09c9f828250e3840d7557374ff568204305e2679 “test: add truc wallet tests”

For correctness, use alive_v2_unspent["vout"] rather than assume the output is at 0.

ishaanam commented at 7:41 pm on August 5, 2025:

Done

in test/functional/wallet_v3_txs.py:192 in 09c9f82825 outdated

187+        self.send_tx(self.charlie, inputs, outputs, 3)
188+        parent_txid = self.charlie.getrawmempool()[0]
189+
190+        # alice spends her output with a v3 transaction
191+        alice_unspent = self.alice.listunspent(minconf=0)[0]
192+        inputs=[{'txid' : parent_txid, 'vout' : alice_unspent['vout']},]

achow101 commented at 10:17 pm on August 4, 2025:

In 09c9f828250e3840d7557374ff568204305e2679 “test: add truc wallet tests”

Since we already have the UTXO from listunspent, we don’t need to specify the txid and vout individually.

0        inputs=[alice_unspent]

Since this is short, it could also be inlined. This comment applies throughout this file.

ishaanam commented at 7:41 pm on August 5, 2025:

Done here and throughout the file

in test/functional/wallet_v3_txs.py:325 in 09c9f82825 outdated

320+        inputs=[]
321+        outputs = {self.alice.getnewaddress() : 2.0, self.bob.getnewaddress() : 2.0}
322+        self.send_tx(self.charlie, inputs, outputs, 3)
323+
324+        alice_v2_unspent = self.alice.listunspent(minconf=1)[0]
325+        alice_unspent = [unspent for unspent in self.alice.listunspent(minconf=0) if unspent["confirmations"] == 0][0]

achow101 commented at 10:21 pm on August 4, 2025:

In 09c9f828250e3840d7557374ff568204305e2679 “test: add truc wallet tests”

listunspent also has a maxconf, so this could be self.alice.listunspent(minconf=0, maxconf=0)[0]

ishaanam commented at 7:42 pm on August 5, 2025:

Done

in test/functional/wallet_v3_txs.py:551 in 09c9f82825 outdated

546+        assert unconfirmed_v3 not in decoded_vin_txids
547+
548+    @cleanup
549+    def walletcreatefundedpsbt_v3(self):
550+        self.log.info("Test setting version to 3 with walletcreatefundedpsbt")
551+        self.log.info("Test setting version to 3 with createpsbt")

achow101 commented at 10:27 pm on August 4, 2025:

In 09c9f828250e3840d7557374ff568204305e2679 “test: add truc wallet tests”

This log is not supposed to be here.

ishaanam commented at 7:42 pm on August 5, 2025:

I’ve removed it.

achow101 commented at 10:40 pm on August 4, 2025: member

I think sendall is missing the weight check if the transaction is truc.

in test/functional/wallet_v3_txs.py:493 in 0563fc1263 outdated

525+        decoded_tx = self.charlie.decoderawtransaction(tx_hex)
526+        decoded_vin_txids = [txin["txid"] for txin in decoded_tx["vin"]]
527+
528+        assert_equal(decoded_tx["version"], 3)
529+
530+        assert confirmed_v3 in decoded_vin_txids

maflcko commented at 7:34 am on August 5, 2025:

https://cirrus-ci.com/task/4521033710960640?logs=ci#L2066:

 0[11:45:25.309]  test  2025-08-04T15:45:24.445408Z TestFramework (ERROR): Unexpected exception 
 1[11:45:25.309]                                    Traceback (most recent call last):
 2[11:45:25.309]                                      File "/ci_container_base/test/functional/test_framework/test_framework.py", line 195, in main
 3[11:45:25.309]                                        self.run_test()
 4[11:45:25.309]                                      File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_v3_txs.py", line 122, in run_test
 5[11:45:25.309]                                        self.sendall_with_unconfirmed_v3()
 6[11:45:25.309]                                      File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_v3_txs.py", line 37, in wrapper
 7[11:45:25.309]                                        func(self, *args)
 8[11:45:25.309]                                      File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_v3_txs.py", line 530, in sendall_with_unconfirmed_v3
 9[11:45:25.309]                                        assert confirmed_v3 in decoded_vin_txids
10[11:45:25.309]                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11[11:45:25.309]                                    AssertionError

achow101 commented at 9:22 pm on August 6, 2025:

The CI failure is caused by charlie spending the confirmed_v2 UTXO when it is creating one of the unconfirmed UTXOs.

In 2d37a5113ff0b0ab682788e4ef13c4cc223c17fe, we should still expect to see this failure, and in fact, more likely to see it (~50% of the time) because the UTXO pool of alice is much smaller at this point in time. The only reason we do not see it is because the test framework sets fallbackfee to 20 sat/vb which is in the high feerate regime which changes the behavior of coin selection to prefer smaller input sets. However this is not behavior that we should rely on.

This failure can be forced by setting a lower feerate in send_tx.

There are a few possible solutions:

explicitly choose the utxos to spend when creating the unconfirmed utxos
use lockunspent on the confirmed utxos before creating the unconfirmed utxos

ishaanam commented at 3:05 pm on August 8, 2025:

Thanks for finding this! I’ve fixed it by explicitly choosing the utxos to spend. I’ve also tested that this works by setting a lower feerate in send_tx and ensuring that the test still passes.

ishaanam force-pushed on Aug 5, 2025

ishaanam commented at 7:46 pm on August 5, 2025: contributor

@achow101 thanks for the review!

I think sendall is missing the weight check if the transaction is truc.

I’ve added the weight check in sendall for truc txs (including those spending unconfirmed truc children) and I have added two additional test cases for this.

ishaanam force-pushed on Aug 5, 2025

in src/wallet/coinselection.h:178 in 43a479ca68 outdated

173@@ -174,6 +174,8 @@ struct CoinSelectionParams {
174      * 1) Received from other wallets, 2) replacing other txs, 3) that have been replaced.
175      */
176     bool m_include_unsafe_inputs = false;
177+    /** The version of the transaction we are trying to create. */
178+    uint32_t m_version = CTransaction::CURRENT_VERSION;

w0xlt commented at 9:55 pm on August 5, 2025:

nit

0    uint32_t m_version{CTransaction::CURRENT_VERSION};

ishaanam commented at 3:06 pm on August 8, 2025:

Done

DrahtBot removed the label CI failed on Aug 5, 2025

in src/wallet/rpc/spend.cpp:1508 in 43a479ca68 outdated

1503@@ -1483,6 +1504,11 @@ RPCHelpMan sendall()
1504                     if (send_max && fee_rate.GetFee(output.input_bytes) > output.txout.nValue) {
1505                         continue;
1506                     }
1507+                    // we are spending a TRUC child, so we have a
1508+                    // lower max weight

glozow commented at 6:57 pm on August 6, 2025:

Is this supposed to say “we are spending an unconfirmed TRUC transaction” ? Also, can be one line?

ishaanam commented at 3:06 pm on August 8, 2025:

Done

in test/functional/wallet_v3_txs.py:521 in 43a479ca68 outdated

516+        psbt = self.charlie.walletcreatefundedpsbt(inputs=[], outputs=outputs, version=3)["psbt"]
517+        assert_equal(self.charlie.decodepsbt(psbt)["tx"]["version"], 3)
518+
519+    @cleanup
520+    def sendall_truc_weight_limit(self):
521+        self.charlie.sendall([self.alice.getnewaddress() for _ in range(300)], add_to_wallet=False)

glozow commented at 7:42 pm on August 6, 2025:

Could make this version 2 to be explicit

Also, could make this more efficient by reusing the outputs vector

ishaanam commented at 3:07 pm on August 8, 2025:

I’ve made version 2 explicit. I didn’t reuse the output because sendall doesn’t allow for duplicate outputs.

in test/functional/wallet_v3_txs.py:147 in 43a479ca68 outdated

142+            self.bob.fundrawtransaction,
143+            raw_tx_v2, {'include_unsafe': True}
144+        )
145+
146+    @cleanup
147+    def v2_tx_spends_confirmed_v3_tx(self, version_a, version_b):

glozow commented at 7:43 pm on August 6, 2025:

0    def va_tx_spends_confirmed_vb_tx(self, version_a, version_b):

ishaanam commented at 3:07 pm on August 8, 2025:

Done

in test/functional/wallet_v3_txs.py:441 in 43a479ca68 outdated

436+        psbt = self.charlie.createpsbt(inputs=[], outputs=outputs, version=3)
437+        assert_equal(self.charlie.decodepsbt(psbt)["tx"]["version"], 3)
438+
439+    @cleanup
440+    def send_v3(self):
441+        self.log.info("Test setting version to 3 with `send`")

glozow commented at 7:48 pm on August 6, 2025:

nit: this format seems to be an outlier

0        self.log.info("Test setting version to 3 with send")

ishaanam commented at 3:07 pm on August 8, 2025:

Done

in test/functional/wallet_v3_txs.py:520 in 43a479ca68 outdated

515+        outputs = {self.alice.getnewaddress() : 10}
516+        psbt = self.charlie.walletcreatefundedpsbt(inputs=[], outputs=outputs, version=3)["psbt"]
517+        assert_equal(self.charlie.decodepsbt(psbt)["tx"]["version"], 3)
518+
519+    @cleanup
520+    def sendall_truc_weight_limit(self):

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

nit: These last 2 subtests don’t have a log associated with them

ishaanam commented at 3:07 pm on August 8, 2025:

I’ve added logs.

glozow commented at 7:50 pm on August 6, 2025: member

ACK 43a479ca688, only minor nits

DrahtBot requested review from murchandamus on Aug 6, 2025

DrahtBot requested review from rkrux on Aug 6, 2025

w0xlt commented at 9:24 pm on August 6, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32896/commits/43a479ca6885b4222fb5e0f673354e54144d6835

ishaanam force-pushed on Aug 8, 2025

glozow commented at 6:30 pm on August 8, 2025: member

reACK 013cbf7eda2

Changes were braced initialization, docs/rename nits, and changing sendall_with_unconfirmed_v3 to choose UTXOs explicitly so we can assert what’s spent at the end.

DrahtBot requested review from w0xlt on Aug 8, 2025

in test/functional/wallet_v3_txs.py:468 in f4d209c0e9 outdated

463+        self.generate(self.nodes[0], 1)
464+
465+        unspent1 = self.alice.listunspent()[0]
466+        unspent2 = self.alice.listunspent()[1]
467+        unspent3 = self.alice.listunspent()[2]
468+        unspent4 = self.alice.listunspent()[3]

achow101 commented at 11:18 pm on August 8, 2025:

In f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

Could store the result of listunspent and access the specific utxos by index later:

0        utxos = self.alice.listunspent()

ishaanam commented at 7:41 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:460 in f4d209c0e9 outdated

455+    def sendall_with_unconfirmed_v3(self):
456+        self.log.info("Test setting version to 3 with sendall + unconfirmed inputs")
457+
458+        outputs = {}
459+        for _ in range(4):
460+            outputs[self.alice.getnewaddress()] = 2.00001

achow101 commented at 11:40 pm on August 8, 2025:

In f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

This could be condensed into a one-liner

0        outputs = {self.alice.getnewaddress(): 2.00001 for _ in range(4)}

ishaanam commented at 7:41 pm on August 12, 2025:

Done

achow101 commented at 11:40 pm on August 8, 2025: member

ACK 013cbf7eda2a3c643668b467cdbeb37027044b3a

w0xlt commented at 0:45 am on August 12, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32896/commits/013cbf7eda2a3c643668b467cdbeb37027044b3a

TRUC rules implemented:  

Rules	Commits
1. A TRUC tx must only have TRUC unconfirmed ancestors	afbf6c1, f3aba33
2. A non‑TRUC tx must only have non‑TRUC unconfirmed ancestors	afbf6c1; f3aba33
3. A TRUC’s ancestor set (incl. itself) must be within `TRUC_ANCESTOR_LIMIT`	Enforced by mempool/TRUC policy (outside this PR)
4. A TRUC’s descendant set (incl. itself) must be within TRUC_DESCENDANT_LIMIT	1ce3c06, 9921f22
5. If a TRUC tx has any unconfirmed ancestors, the tx’s sigop‑adjusted vsize must be ≤ TRUC_CHILD_MAX_VSIZE	eca8f1f
6. A TRUC tx must be ≤ TRUC_MAX_VSIZE	eca8f1f

All of these rules (except the third one) are included in the added test file.

in src/wallet/spend.cpp:293 in f3aba33732 outdated

288+                if (parent_tx.tx->version == TRUC_VERSION && coin_control.m_version != TRUC_VERSION) {
289+                    return util::Error{strprintf(_("Can't spend unconfirmed version 3 pre-selected input with a version %d tx"), coin_control.m_version)};
290+                } else if (coin_control.m_version == TRUC_VERSION && parent_tx.tx->version != TRUC_VERSION) {
291+                    return util::Error{strprintf(_("Can't spend unconfirmed version %d pre-selected input with a version 3 tx"), parent_tx.tx->version)};
292+                }
293+            }

rkrux commented at 10:18 am on August 12, 2025:

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

This passes tests.

 0diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
 1index 63ac992183..0f74c5f55b 100644
 2--- a/src/wallet/spend.cpp
 3+++ b/src/wallet/spend.cpp
 4@@ -284,12 +284,8 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
 5                 input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control);
 6             }
 7             const CWalletTx& parent_tx = txo->GetWalletTx();
 8-            if (wallet.GetTxDepthInMainChain(parent_tx) == 0) {
 9-                if (parent_tx.tx->version == TRUC_VERSION && coin_control.m_version != TRUC_VERSION) {
10-                    return util::Error{strprintf(_("Can't spend unconfirmed version 3 pre-selected input with a version %d tx"), coin_control.m_version)};
11-                } else if (coin_control.m_version == TRUC_VERSION && parent_tx.tx->version != TRUC_VERSION) {
12-                    return util::Error{strprintf(_("Can't spend unconfirmed version %d pre-selected input with a version 3 tx"), parent_tx.tx->version)};
13-                }
14+            if (wallet.GetTxDepthInMainChain(parent_tx) == 0 && parent_tx.tx->version != coin_control.m_version) {
15+                return util::Error{strprintf(_("Can't spend unconfirmed version %d pre-selected input with a version %d tx"), parent_tx.tx->version, coin_control.m_version)};
16             }
17         } else {
18             // The input is external. We did not find the tx in mapWallet.

rkrux commented at 1:04 pm on August 12, 2025:

While the suggestions is shorter, I don’t think this is as thorough as the original implementation. I’m not sure if transaction version 1 could cause an issue here - this suggestion can be ignored.

ishaanam commented at 7:42 pm on August 12, 2025:

transaction version 1 would cause an issue here, I have added a test case for it so this should fail tests now.

in src/wallet/rpc/spend.cpp:709 in eca8f1f5ef outdated

704@@ -704,6 +705,12 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact
705         coinControl.m_max_tx_weight = options["max_tx_weight"].getInt<int>();
706     }
707 
708+    if (tx.version == TRUC_VERSION) {
709+        if (!coinControl.m_max_tx_weight.has_value() || coinControl.m_max_tx_weight.value() > TRUC_MAX_VSIZE * WITNESS_SCALE_FACTOR) {

rkrux commented at 10:26 am on August 12, 2025:

In eca8f1f5ef89259edd61b92b5cf8aa9761af486f “wallet: limit v3 tx weight in coin selection”

Nit: can consider using TRUC_MAX_WEIGHT now that TRUC_MAX_VSIZE * WITNESS_SCALE_FACTOR has been used thrice in the file.

ishaanam commented at 8:23 pm on August 12, 2025:

Done

in src/wallet/spend.cpp:962 in eca8f1f5ef outdated

929         for (const auto& select_filter : ordered_filters) {
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;

rkrux commented at 10:27 am on August 12, 2025:

In eca8f1f5ef89259edd61b92b5cf8aa9761af486f “wallet: limit v3 tx weight in coin selection”

Same nit for TRUC_CHILD_MAX_WEIGHT.

rkrux commented at 3:25 pm on August 12, 2025:

In https://github.com/bitcoin/bitcoin/commit/eca8f1f5ef89259edd61b92b5cf8aa9761af486f “wallet: limit v3 tx weight in coin selection”

Nit: The temp_selection_params object can be called updated_coin_selection_params - better to avoid the temp prefix imho.

A separate approach could be to update the property in the argument reference coin_selection_params which might even help in achieving the points in this comment, but it need not be done in this PR.

ishaanam commented at 8:23 pm on August 12, 2025:

Done

ishaanam commented at 8:25 pm on August 12, 2025:

I’ve updated the name to updated_selection_params. I agree that updating coin_selection_params directly would be outside the scope of this pr.

in src/policy/policy.cpp:98 in 9bc49a8d10 outdated

97@@ -98,7 +98,7 @@ bool IsStandard(const CScript& scriptPubKey, TxoutType& whichType)
98

rkrux commented at 10:28 am on August 12, 2025:

In 9bc49a8d10c7f840de73709bef913dfbaedd1907 “rpc: Add version parameter for transaction”

In commit message:

0- rpc: Add version parameter for transaction
1+ rpc: Add transaction min standard version parameter

ishaanam commented at 7:43 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:74 in f4d209c0e9 outdated

69+        self.num_nodes = 1
70+        self.setup_clean_chain = True
71+        # whitelist peers to speed up tx relay / mempool sync
72+        self.noban_tx_relay = True
73+
74+    def send_tx(self, from_node, inputs, outputs, version):

rkrux commented at 10:52 am on August 12, 2025:

In f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

IIUC, different from wallets are passed to this function while the node remains same.

0- def send_tx(self, from_node, inputs, outputs, version):
1+ def send_tx(self, from_wallet, inputs, outputs, version):

ishaanam commented at 7:43 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:173 in f4d209c0e9 outdated

170+    @cleanup
171+    def truc_tx_with_conflicting_sibling(self):
172+        # unconfirmed v3 tx to alice & bob
173+        self.log.info("Test v3 transaction with conflicting sibling")
174+
175+        outputs = {self.alice.getnewaddress() : 2.0, self.bob.getnewaddress() : 2.0}

rkrux commented at 10:53 am on August 12, 2025:

In f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

0     def truc_tx_with_conflicting_sibling(self):
1-        # unconfirmed v3 tx to alice & bob
2         self.log.info("Test v3 transaction with conflicting sibling")
3 
4+        # unconfirmed v3 tx to alice & bob
5         outputs = {self.alice.getnewaddress() : 2.0, self.bob.getnewaddress() : 2.0}

ishaanam commented at 7:43 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:244 in f4d209c0e9 outdated

239+            alice_tx
240+        )
241+
242+    @cleanup
243+    def spend_inputs_with_different_versions_default_version(self):
244+        self.log.info("Test spending a pre-selected v3 input with a v2 transaction")

rkrux commented at 10:59 am on August 12, 2025:

In f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

0@@ -241,7 +241,7 @@ class WalletV3Test(BitcoinTestFramework): [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)
1     def spend_inputs_with_different_versions_default_version(self):
2-        self.log.info("Test spending a pre-selected v3 input with a v2 transaction")
3+        self.log.info("Test spending a pre-selected v3 input with the default version of transaction")

Otherwise, we end up with duplicated subtest logs.

ishaanam commented at 7:43 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:148 in f4d209c0e9 outdated

143+            raw_tx_v2, {'include_unsafe': True}
144+        )
145+
146+    @cleanup
147+    def va_tx_spends_confirmed_vb_tx(self, version_a, version_b):
148+        self.log.info(f"Test available funds when v{version_a} tx spends confirmed v{version_b} tx")

rkrux commented at 11:10 am on August 12, 2025:

In f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

IIUC, v_b txs seem to be spending v_a txs?

0     def tx_spends_unconfirmed_tx_with_wrong_version(self, version_a, version_b):
1-        self.log.info(f"Test unavailable funds when v{version_a} tx spends unconfirmed v{version_b} tx")
2+        self.log.info(f"Test unavailable funds when v{version_b} tx spends unconfirmed v{version_a} tx")
3 
4         outputs = {self.bob.getnewaddress() : 2.0}
5         self.send_tx(self.charlie, [], outputs, version_a)
6@@ -145,7 +145,7 @@ class WalletV3Test(BitcoinTestFramework): [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)
7     def va_tx_spends_confirmed_vb_tx(self, version_a, version_b):
8-        self.log.info(f"Test available funds when v{version_a} tx spends confirmed v{version_b} tx")
9+        self.log.info(f"Test available funds when v{version_b} tx spends confirmed v{version_a} tx")

ishaanam commented at 7:43 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:80 in f4d209c0e9 outdated

75+        raw_tx = from_node.createrawtransaction(inputs=inputs, outputs=outputs, version=version)
76+        if inputs == []:
77+            raw_tx = from_node.fundrawtransaction(raw_tx, {'include_unsafe' : True})["hex"]
78+        raw_tx = from_node.signrawtransactionwithwallet(raw_tx)["hex"]
79+        txid = from_node.sendrawtransaction(raw_tx)
80+        self.sync_mempools()

rkrux commented at 11:26 am on August 12, 2025:

In https://github.com/bitcoin/bitcoin/commit/f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

Because there is only one node in the test, I don’t think these mempool sync calls are needed.

 0diff --git a/test/functional/wallet_v3_txs.py b/test/functional/wallet_v3_txs.py
 1index a357022c42..db13960537 100755
 2--- a/test/functional/wallet_v3_txs.py
 3+++ b/test/functional/wallet_v3_txs.py
 4@@ -37,7 +37,6 @@ def cleanup(func):
 5             self.generate(self.nodes[0], 1)
 6             func(self, *args)
 7         finally:
 8-            self.sync_mempools()
 9             self.generate(self.nodes[0], 1)
10             try:
11                 self.alice.sendall([self.charlie.getnewaddress()])
12@@ -47,7 +46,6 @@ def cleanup(func):
13                 self.bob.sendall([self.charlie.getnewaddress()])
14             except JSONRPCException as e:
15                 assert "Total value of UTXO pool too low to pay for transaction" in e.error['message']
16-            self.sync_mempools()
17             self.generate(self.nodes[0], 1)
18             assert_equal(0, self.alice.getbalances()["mine"]["untrusted_pending"])
19             assert_equal(0, self.bob.getbalances()["mine"]["untrusted_pending"])
20@@ -77,7 +75,6 @@ class WalletV3Test(BitcoinTestFramework):
21             raw_tx = from_node.fundrawtransaction(raw_tx, {'include_unsafe' : True})["hex"]
22         raw_tx = from_node.signrawtransactionwithwallet(raw_tx)["hex"]
23         txid = from_node.sendrawtransaction(raw_tx)
24-        self.sync_mempools()
25         return txid
26 
27         outputs = {self.alice.getnewaddress() : 2.0}
28         self.send_tx(self.charlie, [], outputs, 3)
29@@ -336,7 +333,6 @@ class WalletV3Test(BitcoinTestFramework):
30         # alice spends both of her utxos, replacing bob's tx
31         outputs = {self.charlie.getnewaddress() : alice_v2_unspent['amount'] + alice_unspent['amount'] - Decimal(0.00005120)}
32         alice_txid = self.send_tx(self.alice, [alice_v2_unspent, alice_unspent], outputs, 3)
33-        self.sync_mempools()
34         # bob's tx now has a mempool conflict
35         assert_equal(self.bob.gettransaction(bob_txid)['mempoolconflicts'], [alice_txid])
36         # alice fee-bumps her tx so it only spends the v2 utxo

ishaanam commented at 7:43 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:196 in f4d209c0e9 outdated

191+            bob_tx, {'include_unsafe': True}
192+        )
193+
194+    @cleanup
195+    def truc_tx_with_conflicting_sibling_change(self):
196+        # unconfirmed v3 tx to alice & bob

rkrux commented at 11:33 am on August 12, 2025:

In https://github.com/bitcoin/bitcoin/commit/f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

 0     def truc_tx_with_conflicting_sibling_change(self):
 1-        # unconfirmed v3 tx to alice & bob
 2         self.log.info("Test v3 transaction with conflicting sibling")
 3 
 4         outputs = {self.alice.getnewaddress() : 8.0}
 5@@ -201,6 +197,7 @@ class WalletV3Test(BitcoinTestFramework):
 6 
 7         self.generate(self.nodes[0], 1)
 8 
 9+        # unconfirmed v3 tx from alice to bob with change
10         outputs = {self.alice.getnewaddress() : 2.0, self.bob.getnewaddress() : 2.0}
11         self.send_tx(self.alice, [], outputs, 3)

ishaanam commented at 7:46 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:265 in f4d209c0e9 outdated

262+    def v3_tx_evicted_from_mempool_by_sibling(self):
263+        # unconfirmed v3 tx to alice & bob
264+        self.log.info("Test v3 transaction with conflicting sibling")
265+
266+        outputs = {self.alice.getnewaddress() : 2.0, self.bob.getnewaddress() : 2.0}
267+        self.send_tx(self.charlie, [], outputs, 3)

rkrux commented at 11:36 am on August 12, 2025:

0@@ -260,9 +257,9 @@ class WalletV3Test(BitcoinTestFramework): [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)
1     def v3_tx_evicted_from_mempool_by_sibling(self):
2-        # unconfirmed v3 tx to alice & bob
3         self.log.info("Test v3 transaction with conflicting sibling")
4 
5+        # unconfirmed v3 tx to alice & bob
6         outputs = {self.alice.getnewaddress() : 2.0, self.bob.getnewaddress() : 2.0}
7         self.send_tx(self.charlie, [], outputs, 3)

ishaanam commented at 7:44 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:282 in f4d209c0e9 outdated

277+        bob_txid = self.send_tx(self.bob, [bob_unspent], outputs, 3)
278+
279+        assert_equal(self.alice.gettransaction(alice_txid)['mempoolconflicts'], [bob_txid])
280+
281+        self.log.info("Test that re-submitting Alice's transaction with a higher fee removes bob's tx as a mempool conflict")
282+        outputs = {self.alice.getnewaddress() : alice_unspent['amount'] - Decimal(0.00030120)}

rkrux commented at 11:41 am on August 12, 2025:

For clarity purpose:

 0         # alice spends her output with a v3 transaction
 1         alice_unspent = self.alice.listunspent(minconf=0)[0]
 2-        outputs = {self.alice.getnewaddress() : alice_unspent['amount'] - Decimal(0.00000120)}
 3+        alice_fee = Decimal(0.00000120)
 4+        outputs = {self.alice.getnewaddress() : alice_unspent['amount'] - alice_fee}
 5         alice_txid = self.send_tx(self.alice, [alice_unspent], outputs, 3)
 6 
 7         # bob tries to spend money
 8@@ -279,7 +277,8 @@ class WalletV3Test(BitcoinTestFramework):
 9         assert_equal(self.alice.gettransaction(alice_txid)['mempoolconflicts'], [bob_txid])
10 
11         self.log.info("Test that re-submitting Alice's transaction with a higher fee removes bob's tx as a mempool conflict")
12-        outputs = {self.alice.getnewaddress() : alice_unspent['amount'] - Decimal(0.00030120)}
13+        fee_delta = Decimal(0.00030000)
14+        outputs = {self.alice.getnewaddress() : alice_unspent['amount'] - (alice_fee + fee_delta)}
15         alice_txid = self.send_tx(self.alice, [alice_unspent], outputs, 3)
16         assert_equal(self.alice.gettransaction(alice_txid)['mempoolconflicts'], [])

ishaanam commented at 7:44 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:345 in f4d209c0e9 outdated

340+        # bob's tx now has a mempool conflict
341+        assert_equal(self.bob.gettransaction(bob_txid)['mempoolconflicts'], [alice_txid])
342+        # alice fee-bumps her tx so it only spends the v2 utxo
343+        outputs = {self.charlie.getnewaddress() : alice_v2_unspent['amount'] - Decimal(0.00015120)}
344+        self.send_tx(self.alice, [alice_v2_unspent], outputs, 2)
345+        # bob's tx now has non conflicts and can be rebroadcast

rkrux commented at 11:52 am on August 12, 2025:

In f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

To test the “can be rebroadcast” assumption.

0         self.send_tx(self.alice, [alice_v2_unspent], outputs, 2)
1         # bob's tx now has non conflicts and can be rebroadcast
2-        assert_equal(self.bob.gettransaction(bob_txid)['mempoolconflicts'], [])
3+        bob_tx_details = self.bob.gettransaction(bob_txid)
4+        assert_equal(bob_tx_details['mempoolconflicts'], [])
5+        self.bob.sendrawtransaction(bob_tx_details['hex'])

ishaanam commented at 7:44 pm on August 12, 2025:

Done

in test/functional/test_framework/script_util.py:142 in 633aa947d6 outdated

137+    # determine number of needed padding bytes
138+    dummy_vbytes = target_vsize - tx.get_vsize()
139+    # compensate for the increase of the compact-size encoded script length
140+    # (note that the length encoding of the unpadded output script needs one byte)
141+    dummy_vbytes -= len(ser_compact_size(dummy_vbytes)) - 1
142+    tx.vout[-1].scriptPubKey = CScript([OP_RETURN] + [OP_1] * dummy_vbytes)

rkrux commented at 12:00 pm on August 12, 2025:

In 633aa947d684a0fdd3c13353ddd8418cb0195aa5 “test: extract bulk_vout from bulk_tx so it can be used by wallet tests”

Fine to extract it but since it particularly bulks the last vout, we can call the function bulk_last_vout?

 0diff --git a/test/functional/test_framework/script_util.py b/test/functional/test_framework/script_util.py
 1index 5736eb6bc4..caf9deb346 100755
 2--- a/test/functional/test_framework/script_util.py
 3+++ b/test/functional/test_framework/script_util.py
 4@@ -131,7 +131,7 @@ def script_to_p2sh_p2wsh_script(script):
 5     p2shscript = CScript([OP_0, sha256(script)])
 6     return script_to_p2sh_script(p2shscript)
 7 
 8-def bulk_vout(tx, target_vsize):
 9+def bulk_last_vout(tx, target_vsize):
10     if target_vsize < tx.get_vsize():
11         raise RuntimeError(f"target_vsize {target_vsize} is less than transaction virtual size {tx.get_vsize()}")
12     # determine number of needed padding bytes
13diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py
14index a47ccab01c..f6ae1f70ef 100644
15--- a/test/functional/test_framework/wallet.py
16+++ b/test/functional/test_framework/wallet.py
17@@ -43,7 +43,7 @@ from test_framework.script import (
18     taproot_construct,
19 )
20 from test_framework.script_util import (
21-    bulk_vout,
22+    bulk_last_vout,
23     key_to_p2pk_script,
24     key_to_p2pkh_script,
25     key_to_p2sh_p2wpkh_script,
26@@ -121,7 +121,7 @@ class MiniWallet:
27         returns the tx
28         """
29         tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN])))
30-        bulk_vout(tx, target_vsize)
31+        bulk_last_vout(tx, target_vsize)
32 
33 
34     def get_balance(self):
35diff --git a/test/functional/wallet_v3_txs.py b/test/functional/wallet_v3_txs.py
36index db9f1483ab..53ad9c068c 100755
37--- a/test/functional/wallet_v3_txs.py
38+++ b/test/functional/wallet_v3_txs.py
39@@ -17,7 +17,7 @@ from test_framework.script import (
40     OP_RETURN
41 )
42 
43-from test_framework.script_util import bulk_vout
44+from test_framework.script_util import bulk_last_vout
45 
46 from test_framework.test_framework import BitcoinTestFramework
47 from test_framework.util import (
48@@ -78,7 +78,7 @@ class WalletV3Test(BitcoinTestFramework):
49 
50     def bulk_tx(self, tx, amount, target_vsize):
51         tx.vout.append(CTxOut(nValue=(amount * COIN), scriptPubKey=CScript([OP_RETURN])))
52-        bulk_vout(tx, target_vsize)
53+        bulk_last_vout(tx, target_vsize)
54 
55     def run_test_with_swapped_versions(self, test_func):
56         test_func(2, 3)

An alternative could be that this function can also append the op_return vout in the transaction instead of assuming the caller does it, in which case this function would become bulk_tx - but this could be a larger change and can be avoided in this PR.

in test/functional/wallet_v3_txs.py:407 in f4d209c0e9 outdated

400+        assert_raises_rpc_error(
401+            -4,
402+            "Maximum transaction weight is less than transaction weight without inputs",
403+            self.charlie.fundrawtransaction,
404+            tx.serialize_with_witness().hex(),
405+            {'include_unsafe' : True, 'max_tx_weight' : int(TRUC_MAX_VSIZE/2)}

rkrux commented at 12:02 pm on August 12, 2025:

In f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

The user-input tx weight is the same as the previously bulk-ed tx weight?

ishaanam commented at 7:45 pm on August 12, 2025:

Yes, because this is an easy way to check that we are accounting for the right maximum tx weight.

in test/functional/wallet_v3_txs.py:72 in f4d209c0e9 outdated

67+    def set_test_params(self):
68+        getcontext().prec=10
69+        self.num_nodes = 1
70+        self.setup_clean_chain = True
71+        # whitelist peers to speed up tx relay / mempool sync
72+        self.noban_tx_relay = True

rkrux commented at 12:08 pm on August 12, 2025:

In https://github.com/bitcoin/bitcoin/commit/f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

I don’t think this is required because there is only one node in the test.

ishaanam commented at 7:45 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:197 in f4d209c0e9 outdated

192+        )
193+
194+    @cleanup
195+    def truc_tx_with_conflicting_sibling_change(self):
196+        # unconfirmed v3 tx to alice & bob
197+        self.log.info("Test v3 transaction with conflicting sibling")

rkrux commented at 12:10 pm on August 12, 2025:

To deduplicate subtest logs.

0- self.log.info("Test v3 transaction with conflicting sibling")
1+ self.log.info("Test v3 transaction with conflicting sibling change")

ishaanam commented at 7:44 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:264 in f4d209c0e9 outdated

259+        )
260+
261+    @cleanup
262+    def v3_tx_evicted_from_mempool_by_sibling(self):
263+        # unconfirmed v3 tx to alice & bob
264+        self.log.info("Test v3 transaction with conflicting sibling")

rkrux commented at 12:10 pm on August 12, 2025:

To deduplicate subtest logs.

0- self.log.info("Test v3 transaction with conflicting sibling")
1+ self.log.info("Test v3 transaction evicted because of conflicting sibling")

ishaanam commented at 7:44 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:36 in f4d209c0e9 outdated

30+    TRUC_MAX_VSIZE,
31+    TRUC_CHILD_MAX_VSIZE,
32+)
33+
34+def cleanup(func):
35+    def wrapper(self, *args):

rkrux commented at 12:17 pm on August 12, 2025:

In f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

Can add a comment before this wrapper to mention the expectation - from what I understand, the intent is to drain the alice and bob wallets by sending their funds to charlie, along with clearing the node mempool by generating blocks (of pending transactions).

ishaanam commented at 7:44 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:57 in f4d209c0e9 outdated

52+            assert_equal(0, self.alice.getbalances()["mine"]["untrusted_pending"])
53+            assert_equal(0, self.bob.getbalances()["mine"]["untrusted_pending"])
54+            assert_equal(0, self.alice.getbalances()["mine"]["trusted"])
55+            assert_equal(0, self.bob.getbalances()["mine"]["trusted"])
56+            assert_equal(0, self.alice.getbalances()["mine"]["immature"])
57+            assert_equal(0, self.bob.getbalances()["mine"]["immature"])

rkrux commented at 12:18 pm on August 12, 2025:

In https://github.com/bitcoin/bitcoin/commit/f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

To avoid the redundant RPCs.

 0             self.generate(self.nodes[0], 1)
 1-            assert_equal(0, self.alice.getbalances()["mine"]["untrusted_pending"])
 2-            assert_equal(0, self.bob.getbalances()["mine"]["untrusted_pending"])
 3-            assert_equal(0, self.alice.getbalances()["mine"]["trusted"])
 4-            assert_equal(0, self.bob.getbalances()["mine"]["trusted"])
 5-            assert_equal(0, self.alice.getbalances()["mine"]["immature"])
 6-            assert_equal(0, self.bob.getbalances()["mine"]["immature"])
 7+
 8+            for balance in [self.alice.getbalances()["mine"], self.bob.getbalances()["mine"]]:
 9+                for balance_type in ["untrusted_pending", "trusted", "immature"]:
10+                    assert_equal(0, balance[balance_type])

ishaanam commented at 7:44 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:321 in f4d209c0e9 outdated

316+        # bob can now create a transaction
317+        outputs = {self.bob.getnewaddress() : 1.999}
318+        self.send_tx(self.bob, [], outputs, 3)
319+
320+    @cleanup
321+    def mempool_conflicts_removed_when_v3_conflict_removed(self):

rkrux commented at 12:28 pm on August 12, 2025:

In f4d209c0e94fad8435215abba3e61d79585d0d12 “test: add truc wallet tests”

There doesn’t seem to be a test log for this one.

ishaanam commented at 7:44 pm on August 12, 2025:

Done

in test/functional/wallet_v3_txs.py:339 in f4d209c0e9 outdated

333+        inputs=[]
334+        outputs = {self.bob.getnewaddress() : 1.999}
335+        bob_txid = self.send_tx(self.bob, inputs, outputs, 3)
336+        # alice spends both of her utxos, replacing bob's tx
337+        outputs = {self.charlie.getnewaddress() : alice_v2_unspent['amount'] + alice_unspent['amount'] - Decimal(0.00005120)}
338+        alice_txid = self.send_tx(self.alice, [alice_v2_unspent, alice_unspent], outputs, 3)

rkrux commented at 12:34 pm on August 12, 2025:

Contrary to what’s seen in the previous test - by spending a confirmed unspent with a TRUC unspent, the alice transaction is allowed to be created even if bob’s transaction spending a TRUC unspent was already there. Which TRUC rule does it correspond to?

rkrux commented at 2:07 pm on August 12, 2025:

Ok, I debugged these transactions.

In the mempool_conflicts_removed_when_v3_conflict_removed test, I can understand that the latter Alice tx has higher fees than the former Bob tx due to which Alice’s tx sibling-evicts Bob’s tx.

What’s not clear to me is that why in the v3_conflict_removed_from_mempool test the latter Bob tx can’t sibling-evict the former Alice’s tx even though it has higher fees (and instead the “insufficient funds” is thrown)?

glozow commented at 2:12 pm on August 12, 2025:

Sibling eviction isn’t implemented here - the wallet doesn’t try to fund a transaction that would “conflict” with that transaction. This is consistent with the wallet never trying to RBF something that has descendants, even though it is possible.

rkrux commented at 12:34 pm on August 12, 2025: contributor

Looking good, code review 013cbf7eda2a3c643668b467cdbeb37027044b3a

I have shared few suggestions, all are minor I believe.

DrahtBot requested review from rkrux on Aug 12, 2025

glozow commented at 1:32 pm on August 12, 2025: member

A TRUC’s ancestor set (incl. itself) must be within TRUC_ANCESTOR_LIMIT

All of these rules (except the third one) are included in the added test file.

Ah true, the wallet should never select multiple unconfirmed TRUC outputs. We can test this by having 2 unconfirmed TRUC outputs to spend, neither able to fund the transaction fully, and checking you get “insufficient funds.”

DrahtBot requested review from rkrux on Aug 12, 2025

w0xlt commented at 6:05 pm on August 12, 2025: contributor

There’s repeated logic for iterating through parent outputs and updating sibling transaction states in both transactionAddedToMempool and transactionRemovedFromMempool.

The below patch can improve this, using the same patter in both functions. If the logic for handling TRUC sibling conflicts needs to be changed in the future, it only needs to be updated in one place.

 0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 1index 28a646ef4b..c8e5e3a8e8 100644
 2--- a/src/wallet/wallet.cpp
 3+++ b/src/wallet/wallet.cpp
 4@@ -1374,6 +1374,23 @@ bool CWallet::TransactionCanBeAbandoned(const Txid& hashTx) const
 5     return wtx && !wtx->isAbandoned() && GetTxDepthInMainChain(*wtx) == 0 && !wtx->InMempool();
 6 }
 7 
 8+void CWallet::UpdateTrucSiblingConflicts(const CWalletTx& parent_wtx, const Txid& child_txid, bool add_conflict)
 9+{
10+    AssertLockHeld(cs_wallet);
11+    // Find all other txs in our wallet that spend utxos from this parent
12+    // so that we can mark/unmark them as mempool-conflicted by this tx.
13+    for (size_t i = 0; i < parent_wtx.tx->vout.size(); i++) {
14+        for (auto range = mapTxSpends.equal_range(COutPoint(parent_wtx.tx->GetHash(), i)); range.first != range.second; range.first++) {
15+            const Txid& sibling_txid = range.first->second;
16+            // Skip the child tx itself
17+            if (sibling_txid == child_txid) continue;
18+            RecursiveUpdateTxState(/*batch=*/nullptr, sibling_txid, [&child_txid, add_conflict](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
19+                return add_conflict ? (wtx.mempool_conflicts.insert(child_txid).second ? TxUpdate::CHANGED : TxUpdate::UNCHANGED)
20+                                    : (wtx.mempool_conflicts.erase(child_txid) ? TxUpdate::CHANGED : TxUpdate::UNCHANGED);
21+            });
22+        }
23+    }
24+}
25+
26 void CWallet::MarkInputsDirty(const CTransactionRef& tx)
27 {
28     for (const CTxIn& txin : tx->vin) {
29@@ -1407,20 +1424,10 @@ void CWallet::transactionAddedToMempool(const CTransactionRef& tx) {
30                 CWalletTx& parent_wtx = parent_it->second;
31                 if (parent_wtx.isUnconfirmed()) {
32                     parent_wtx.truc_child_in_mempool = tx->GetHash();
33-                    // Find all other txs in our wallet that spend utxos from this parent
34-                    // so that we can mark them as mempool-conflicted by this new tx.
35                     // Even though these siblings do not spend the same utxos, they can't
36                     // be present in the mempool at the same time because of TRUC policy rules
37-                    for (long unsigned int i = 0; i < parent_wtx.tx->vout.size(); i++) {
38-                        for (auto range = mapTxSpends.equal_range(COutPoint(parent_wtx.tx->GetHash(), i)); range.first != range.second; range.first++) {
39-                            const Txid& sibling_txid = range.first->second;
40-                            // Skip the recently added tx
41-                            if (sibling_txid == txid) continue;
42-                            RecursiveUpdateTxState(/*batch=*/nullptr, sibling_txid, [&txid](CWalletTx& parent_wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
43-                                return parent_wtx.mempool_conflicts.insert(txid).second ? TxUpdate::CHANGED : TxUpdate::UNCHANGED;
44-                            });
45-                        }
46-                    }
47+                    UpdateTrucSiblingConflicts(parent_wtx, txid, /*add_conflict=*/true);
48                 }
49             }
50         }
51@@ -1489,15 +1496,7 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRe
52                 CWalletTx& parent_wtx = parent_it->second;
53                 if (parent_wtx.truc_child_in_mempool == tx->GetHash()) {
54                     parent_wtx.truc_child_in_mempool = std::nullopt;
55-                    // Find all wallet transactions that spend utxos from the same parent_wtx
56-                    for (long unsigned int i = 0; i < parent_wtx.tx->vout.size(); i++) {
57-                        for (auto range = mapTxSpends.equal_range(COutPoint(parent_wtx.tx->GetHash(), i)); range.first != range.second; range.first++) {
58-                            const Txid& spent_id = range.first->second;
59-                            RecursiveUpdateTxState(/*batch=*/nullptr, spent_id, [&txid](CWalletTx& parent_wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
60-                                return parent_wtx.mempool_conflicts.erase(txid) ? TxUpdate::CHANGED : TxUpdate::UNCHANGED;
61-                            });
62-                        }
63-                    }
64+                    UpdateTrucSiblingConflicts(parent_wtx, txid, /*add_conflict=*/false);
65                 }
66             }
67         }
68diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
69index e056b68182..1e3a1e0e48 100644
70--- a/src/wallet/wallet.h
71+++ b/src/wallet/wallet.h
72@@ -598,6 +598,9 @@ private:
73     //! Cache of descriptor ScriptPubKeys used for IsMine. Maps ScriptPubKey to set of spkms
74     std::unordered_map<CScript, std::vector<ScriptPubKeyMan*>, SaltedSipHasher> m_cached_spks;
75 
76+    //! Update mempool conflicts for TRUC sibling transactions
77+    void UpdateTrucSiblingConflicts(const CWalletTx& parent_wtx, const Txid& child_txid, bool add_conflict) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
78+
79 public:
80     /*
81      * Main wallet lock.

ishaanam force-pushed on Aug 12, 2025

in src/wallet/spend.cpp:330 in 025692c4cf outdated

324@@ -316,6 +325,9 @@ CoinsResult AvailableCoins(const CWallet& wallet,
325     AssertLockHeld(wallet.cs_wallet);
326 
327     CoinsResult result;
328+    // track unconfirmed truc outputs separately if we are tracking trucness
329+    CoinsResult unconfirmed_truc_coins;
330+    std::unordered_map<uint256, CAmount, SaltedTxidHasher> truc_txid_by_value;

glozow commented at 8:03 pm on August 12, 2025:

should use Txid when possible

ishaanam commented at 8:25 pm on August 12, 2025:

Updated here and later in the function

in src/wallet/spend.cpp:515 in 025692c4cf outdated

511+            for (const COutput& output : outputs) {
512+                if (output.outpoint.hash == truc_txid) {
513+                        result.Add(type, output);
514+                }
515+            }
516+        }

glozow commented at 8:05 pm on August 12, 2025:

Do you need to track all of them or can you just keep a pair<Txid, CAmount>?

ishaanam commented at 8:26 pm on August 12, 2025:

I figured it would be simplest to just track all of the unconfirmed truc outputs and just select the ones that we need at the end of the function.

ishaanam force-pushed on Aug 12, 2025

DrahtBot added the label CI failed on Aug 12, 2025

DrahtBot commented at 8:23 pm on August 12, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/47940439625 LLM reason (✨ experimental): Linting errors 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 Aug 12, 2025

in src/wallet/wallet.cpp:1237 in 28113b9913 outdated

1237@@ -1238,6 +1238,22 @@ bool CWallet::TransactionCanBeAbandoned(const Txid& hashTx) const
1238     const CWalletTx* wtx = GetWalletTx(hashTx);
1239     return wtx && !wtx->isAbandoned() && GetTxDepthInMainChain(*wtx) == 0 && !wtx->InMempool();
1240 }
1241+void CWallet::UpdateTrucSiblingConflicts(const CWalletTx& parent_wtx, const Txid& child_txid, bool add_conflict) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)

achow101 commented at 9:38 pm on August 12, 2025:

In 28113b9913c9b1d70907a28c553d5223ba63be42 “wallet: mark unconfirmed v3 siblings as mempool conflicts”

nit: needs an empty line between this function and the previous.

ishaanam commented at 2:40 pm on August 13, 2025:

Done

in src/wallet/spend.cpp:478 in e7f4eb9e3e outdated

472@@ -470,8 +473,18 @@ CoinsResult AvailableCoins(const CWallet& wallet,
473             is_from_p2sh = true;
474         }
475 
476-        result.Add(GetOutputType(type, is_from_p2sh),
477-                   COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, tx_safe, wtx.GetTxTime(), tx_from_me, feerate));
478+        if (wtx.tx->version == TRUC_VERSION && nDepth == 0 && params.check_version_trucness) {
479+            unconfirmed_truc_coins.Add(GetOutputType(type, is_from_p2sh),
480+                       COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, tx_safe, wtx.GetTxTime(), tx_from_me, feerate));

achow101 commented at 9:53 pm on August 12, 2025:

In e7f4eb9e3e95eeedab0b4c3dd643c7614de02880 “wallet: don’t return utxos from multiple truc txs in AvailableCoins”

nit: Could deduplicate the two Add calls by storing the output type and the COutput in a variable.

ishaanam commented at 2:41 pm on August 13, 2025:

I have replaced CoinsResult with a vector

in src/wallet/spend.cpp:483 in e7f4eb9e3e outdated

480+                       COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, tx_safe, wtx.GetTxTime(), tx_from_me, feerate));
481+            if (truc_txid_by_value.find(wtx.tx->GetHash()) == truc_txid_by_value.end()) {
482+                truc_txid_by_value[wtx.tx->GetHash()] = output.nValue;
483+            } else {
484+                truc_txid_by_value[wtx.tx->GetHash()] += output.nValue;
485+            }

achow101 commented at 9:57 pm on August 12, 2025:

In e7f4eb9e3e95eeedab0b4c3dd643c7614de02880 “wallet: don’t return utxos from multiple truc txs in AvailableCoins”

nit: Can reduce map lookups with a try_emplace

0            auto& [_, it] = truc_txid_by_value.try_emplace(wtx.tx->GetHash(), 0);
1            it += output.nValue;

rkrux commented at 12:08 pm on August 13, 2025:

 0diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
 1index 3f04b83905..f4a4dea7ad 100644
 2--- a/src/wallet/spend.cpp
 3+++ b/src/wallet/spend.cpp
 4@@ -476,11 +476,8 @@ CoinsResult AvailableCoins(const CWallet& wallet,
 5         if (wtx.tx->version == TRUC_VERSION && nDepth == 0 && params.check_version_trucness) {
 6             unconfirmed_truc_coins.Add(GetOutputType(type, is_from_p2sh),
 7                        COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, tx_safe, wtx.GetTxTime(), tx_from_me, feerate));
 8-            if (truc_txid_by_value.find(wtx.tx->GetHash()) == truc_txid_by_value.end()) {
 9-                truc_txid_by_value[wtx.tx->GetHash()] = output.nValue;
10-            } else {
11-                truc_txid_by_value[wtx.tx->GetHash()] += output.nValue;
12-            }
13+            auto [it, _] = truc_txid_by_value.try_emplace(wtx.tx->GetHash(), 0);
14+            it->second += output.nValue;
15         } else {
16             result.Add(GetOutputType(type, is_from_p2sh),
17                        COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, tx_safe, wtx.GetTxTime(), tx_from_me, feerate));

Otherwise there is a compilation error.

ishaanam commented at 2:41 pm on August 13, 2025:

Done

DrahtBot removed the label CI failed on Aug 12, 2025

ishaanam force-pushed on Aug 13, 2025

in src/wallet/spend.cpp:506 in 45dd785072 outdated

497@@ -468,6 +498,19 @@ CoinsResult AvailableCoins(const CWallet& wallet,
498         }
499     }
500 
501+    if (params.check_version_trucness && unconfirmed_truc_coins.size() > 0) {
502+        auto highest_value_truc_tx = std::max_element(truc_txid_by_value.begin(), truc_txid_by_value.end(), [](const auto& tx1, const auto& tx2){

glozow commented at 3:07 pm on August 13, 2025:

Do I understand correctly that the approach here is to just return 1 unconfirmed TRUC coin from AvailableCoins, i.e. the one with the highest value? That seems like it could weaken coin selection quality a little bit - is there a way to encode these kinds of restrictions (once coin X is selected, ABC are off the table) into the coin selection process instead?

This is probably rare in practice (I think people would only use unconfirmed TRUC when CPFPing or really desperate), but it would be helpful to provide justification here.

ishaanam commented at 3:21 pm on August 13, 2025:

It returns either 1 unconfirmed TRUC coin, or multiple coins that are outputs of the same transaction (if the combined value is higher than the alternatives ). I think that adding these more complex restrictions to coin selection would be a longer project. I will add a comment here to explain the reasoning.

ishaanam force-pushed on Aug 13, 2025

DrahtBot added the label CI failed on Aug 13, 2025

DrahtBot commented at 4:35 pm on August 13, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/48008323258 LLM reason (✨ experimental): Failure due to clang-tidy reported an error in spend.cpp: unnecessary temporary object created while calling emplace_back.

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 removed the label CI failed on Aug 13, 2025

in src/wallet/spend.cpp:507 in 41e4564965 outdated

503+    // be improved in the future by encoding these restrictions in
504+    // the coin selection itself so that we don't have to filter out
505+    // other unconfirmed TRUC coins beforehand.
506+    if (params.check_version_trucness && unconfirmed_truc_coins.size() > 0) {
507+        auto highest_value_truc_tx = std::max_element(truc_txid_by_value.begin(), truc_txid_by_value.end(), [](const auto& tx1, const auto& tx2){
508+                return tx1.second < tx2.second;

glozow commented at 7:03 pm on August 13, 2025:

There isn’t coverage for this comparator being correct, though lgtm

glozow commented at 7:04 pm on August 13, 2025: member

reACK 5c087f58245cfba6b61691ece5387eccf8a1a5f9

DrahtBot requested review from w0xlt on Aug 13, 2025

DrahtBot requested review from achow101 on Aug 13, 2025

w0xlt commented at 10:13 pm on August 13, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32896/commits/5c087f58245cfba6b61691ece5387eccf8a1a5f9

in src/wallet/spend.cpp:484 in 41e4564965 outdated

481+            auto [it, _] = truc_txid_by_value.try_emplace(wtx.tx->GetHash(), 0);
482+            it->second += output.nValue;
483+        } else {
484+            result.Add(GetOutputType(type, is_from_p2sh),
485+                       COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, tx_safe, wtx.GetTxTime(), tx_from_me, feerate));
486+        }

rkrux commented at 1:19 pm on August 14, 2025:

In 41e456496591ca31e7cedb66752a1cd01d09ae7d “wallet: don’t return utxos from multiple truc txs in AvailableCoins”

I think this nit #32896 (review) was more around to avoid the duplication in the map key-value objects.

 0diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
 1index 9049c0ba32..149320b2e0 100644
 2--- a/src/wallet/spend.cpp
 3+++ b/src/wallet/spend.cpp
 4@@ -473,14 +473,14 @@ CoinsResult AvailableCoins(const CWallet& wallet,
 5             is_from_p2sh = true;
 6         }
 7 
 8+        auto available_output_type = GetOutputType(type, is_from_p2sh);
 9+        auto available_output = COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, tx_safe, wtx.GetTxTime(), tx_from_me, feerate);
10         if (wtx.tx->version == TRUC_VERSION && nDepth == 0 && params.check_version_trucness) {
11-            unconfirmed_truc_coins.emplace_back(GetOutputType(type, is_from_p2sh),
12-                       COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, tx_safe, wtx.GetTxTime(), tx_from_me, feerate));
13+            unconfirmed_truc_coins.emplace_back(available_output_type, available_output);
14             auto [it, _] = truc_txid_by_value.try_emplace(wtx.tx->GetHash(), 0);
15             it->second += output.nValue;
16         } else {
17-            result.Add(GetOutputType(type, is_from_p2sh),
18-                       COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, tx_safe, wtx.GetTxTime(), tx_from_me, feerate));
19+            result.Add(available_output_type, available_output);
20         }
21 
22         outpoints.push_back(outpoint);

achow101 commented at 8:04 pm on August 14, 2025:

Yeah, this is what I meant with that comment.

ishaanam commented at 4:53 pm on August 15, 2025:

Done

in src/wallet/spend.cpp:502 in 41e4564965 outdated

497@@ -488,6 +498,24 @@ CoinsResult AvailableCoins(const CWallet& wallet,
498         }
499     }
500 
501+    // Return the unconfirmed TRUC coin, or multiple TRUC coins from
502+    // the same transaction, that have the highest value. This could

rkrux commented at 1:23 pm on August 14, 2025:

In https://github.com/bitcoin/bitcoin/commit/41e456496591ca31e7cedb66752a1cd01d09ae7d “wallet: don’t return utxos from multiple truc txs in AvailableCoins”

nit if retouched: don’t think it’s important to mention if a single coin is returned, it should be covered by the second phrase.

 0@@ -498,9 +498,8 @@ CoinsResult AvailableCoins(const CWallet& wallet,
 1         }
 2     }
 3 
 4-    // Return the unconfirmed TRUC coin, or multiple TRUC coins from
 5-    // the same transaction, that have the highest value. This could
 6-    // be improved in the future by encoding these restrictions in
 7+    // Return all the coins from one TRUC transaction, that have
 8+    // the highest value. This could be improved in the future by encoding these restrictions in
 9     // the coin selection itself so that we don't have to filter out
10     // other unconfirmed TRUC coins beforehand.

ishaanam commented at 4:54 pm on August 15, 2025:

Done

in src/wallet/rpc/spend.cpp:1388 in 796f63640c outdated

1370@@ -1369,6 +1371,7 @@ RPCHelpMan sendall()
1371                         {"send_max", RPCArg::Type::BOOL, RPCArg::Default{false}, "When true, only use UTXOs that can pay for their own fees to maximize the output amount. When 'false' (default), no UTXO is left behind. send_max is incompatible with providing specific inputs."},
1372                         {"minconf", RPCArg::Type::NUM, RPCArg::Default{0}, "Require inputs with at least this many confirmations."},
1373                         {"maxconf", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "Require inputs with at most this many confirmations."},
1374+                        {"version", RPCArg::Type::NUM, RPCArg::Default{DEFAULT_WALLET_TX_VERSION}, "Transaction version"},

rkrux commented at 1:59 pm on August 14, 2025:

In 796f63640ca799e2e98b9dafd1fa2cc9ded6883a “rpc: Support version 3 transaction creation”

There’s an inputs argument* in the sendall RPC few lines above that accepts a set of inputs given by the user and a corresponding if block few lines below. I think the TRUC changes (ancestor/child version match) of FetchSelectedInputs need to be added here too? The AvailableCoins function used in this RPC as well has the TRUC changes already.

https://github.com/bitcoin/bitcoin/blob/9b1a7c3e8dd78d97fbf47c2d056d043b05969176/src/wallet/rpc/spend.cpp#L1481-L1491

If makes sense, it can be done in a follow-up PR as well.

ishaanam commented at 4:54 pm on August 15, 2025:

Done

rkrux approved

rkrux commented at 2:02 pm on August 14, 2025: contributor

lgtm ACK 5c087f58245cfba6b61691ece5387eccf8a1a5f9

Thanks for taking this over the finish line. To implement the TRUC conditions in the wallet, I’ve observed that the related changes are done in the following control sites of selecting coins while building the transaction via different corresponding RPCs:

FetchSelectedInputs: to ensure the parent-child TRUC versions are in agreement.
AvailableCoins: to ensure the parent-child TRUC versions are in agreement, to ensure the TRUC ancestor/descendant limits are adhered to.
AutomaticCoinSelection: to ensure the TRUC transaction weight criterion are met.
Notifications - transactionAddedToMempool, transactionRemovedFromMempool: to ensure transaction mempool conflicts are updated and unconfirmed TRUC child is tracked.

The several functional tests also give more confidence.

The couple nits below can be done if there is any other need to retouch the PR.

DrahtBot added the label Needs rebase on Aug 15, 2025

wallet: unconfirmed ancestors and descendants are always truc ec2676becd

wallet: don't include unconfirmed v3 txs with children in available coins 2e9617664e

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.

cc155226fe

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

wallet: mark unconfirmed v3 siblings as mempool conflicts 85c5410615

wallet: limit v3 tx weight in coin selection da8748ad62

wallet: don't return utxos from multiple truc txs in AvailableCoins c5a2d08011

rpc: Add transaction min standard version parameter 4c20343b4d

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>

2cb473d9f2

test: extract `bulk_vout` from `bulk_tx` so it can be used by wallet tests 5d932e14db

test: add truc wallet tests 4ef8065a5e

doc: add release notes for version 3 transactions 5c8bf7b39e

ishaanam force-pushed on Aug 15, 2025

glozow commented at 4:59 pm on August 15, 2025: member

reACK 5c8bf7b39e9

DrahtBot requested review from rkrux on Aug 15, 2025

DrahtBot requested review from w0xlt on Aug 15, 2025

DrahtBot removed the label Needs rebase on Aug 15, 2025

in src/wallet/spend.cpp:513 in c5a2d08011 outdated

508+                });
509+
510+        const Txid& truc_txid = highest_value_truc_tx->first;
511+        for (const auto& [type, output] : unconfirmed_truc_coins) {
512+            if (output.outpoint.hash == truc_txid) {
513+                    result.Add(type, output);

achow101 commented at 8:25 pm on August 15, 2025:

In c5a2d080116270ecd0414c14eb412fa30eaaedaf “wallet: don’t return utxos from multiple truc txs in AvailableCoins”

nit: extra indent

achow101 commented at 8:26 pm on August 15, 2025: member

ACK 5c8bf7b39e9bffba7c4d5778b56b1ebe72f5ea1b

I’m not fully sold on the approach of choosing the highest value TRUC UTXO, but don’t have any ideas on how to change that for this PR. Maybe @murchandamus has some thoughts on that.

rkrux approved

rkrux commented at 6:50 am on August 16, 2025: contributor

ACK 5c8bf7b39e9bffba7c4d5778b56b1ebe72f5ea1b

0git range-diff 5c087f5...5c8bf7b

murchandamus commented at 0:51 am on August 19, 2025: contributor

I’m not fully sold on the approach of choosing the highest value TRUC UTXO, but don’t have any ideas on how to change that for this PR. Maybe @murchandamus has some thoughts on that.

Been pondering this a bit today.

In the first pass, we will always try to fund transactions only with confirmed UTXOs. If we are just requesting a new TRUC transaction, it will always get funded by confirmed UTXOs if we have sufficient confirmed funds.
If we are deliberately creating a child transaction, it’s probably because we want to CPFP an existing TRUC transaction and that already restricts the unconfirmed UTXOs to be created by the specific parent transaction. In that case, only other outputs from the same transactions are other eligible unconfirmed inputs.
This leaves only the case in which we are creating a TRUC transaction, but we do not have sufficient confirmed funds to pay for it. In that case, we can use one or several unconfirmed UTXOs from a single transaction in addition to any number of confirmed UTXOs.

We do have the nDepth member on COutput, so if we have several unconfirmed single outputs from transactions, we could keep track of whether we have selected an unconfirmed UTXO. Whenever we were to add a second unconfirmed UTXO, we could keep it or replace it with new one, depending on which has the greater effective value instead of adding it. Perhaps, just in case of creating TRUC transactions, we would create a new type of OutputGroups based on common parent transaction instead of common output script in that context. Because we can only use one OutputGroup that contains unconfirmed UTXOs, we could generate all subsets of a transaction’s outputs as separate OutputGroups, and thereby have those unconfirmed UTXOs appear multiple times in different configurations. We could only use that approach if there is a small number (<10?) of outputs to ourselves on the transaction as power sets grow exponentially. Either way, it might be easier to have a dedicated algorithm for TRUC child transactions, that tracks the two pots separately, but all of that seems very complicated.

Given that it should be fairly uncommon to have all of your funds in flight and the complexity of alternatives, using the largest UTXO or group of UTXOs feels viable to me, although it could be improved in the future.

glozow merged this on Aug 19, 2025

glozow closed this on Aug 19, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts