qt, wallet: Convert uint256 to Txid

marcofleon commented at 3:17 pm on April 8, 2025: contributor

This is part of #32189.

Converts all instances of transactions from uint256 to Txid in the wallet, GUI, and related interfaces.

DrahtBot commented at 3:17 pm on April 8, 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/32238.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	laanwj, hebasto, BrandonOdiwuor, yancyribbens
Approach ACK	w0xlt
Stale ACK	stickies-v, achow101

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:

#28201 (Silent Payments: sending by josibake)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory 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.

marcofleon marked this as a draft on Apr 8, 2025

laanwj added the label Wallet on Apr 9, 2025

laanwj commented at 9:46 am on April 9, 2025: member

Concept ACK

hebasto commented at 10:10 am on April 9, 2025: member

Concept ACK.

DrahtBot added the label Needs rebase on Apr 10, 2025

marcofleon force-pushed on Apr 10, 2025

DrahtBot removed the label Needs rebase on Apr 10, 2025

marcofleon commented at 5:02 pm on April 10, 2025: contributor

Rebased after #32237. Ready to go!

marcofleon marked this as ready for review on Apr 10, 2025

BrandonOdiwuor commented at 5:00 pm on April 11, 2025: contributor

Concept ACK

in src/util/transaction_identifier.h:70 in 8205a30a0b outdated

64@@ -65,6 +65,10 @@ class transaction_identifier
65      * to using the Txid and Wtxid types. Until then it makes for a smoother
66      * transition to allow this conversion. */
67     operator const uint256&() const LIFETIMEBOUND { return m_wrapped; }
68+
69+    // TODO: This should never go into master. It's only used for intermediate
70+    // commits so that each conceptual chunk can be built.

maflcko commented at 5:16 pm on April 11, 2025:

seems a bit odd. I’d expect each chunk to be self-contained, complete and atomic, so there shouldn’t be need for this?

Now that you have the final set of changes, maybe they can be re-structured into chunks without need for this? Possibly with a few manual conversions, if really needed in some places temporarily.

marcofleon commented at 2:48 pm on April 14, 2025:

Agreed, it was awkward. I thought that temporary line would end up being used in more places, but it was only necessary for commitBumpTransaction, so I addressed that particular occurrence. Should be better now.

marcofleon force-pushed on Apr 14, 2025

in src/wallet/rpc/backup.cpp:340 in afbb1a1bfd outdated

336@@ -337,7 +337,7 @@ RPCHelpMan importprunedfunds()
337     if (!DecodeHexTx(tx, request.params[0].get_str())) {
338         throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input.");
339     }
340-    uint256 hashTx = tx.GetHash();
341+    Txid hashTx = tx.GetHash();

stickies-v commented at 2:43 pm on April 15, 2025:

nit: This hashTx is actually never used as a Txid, so might as well simplify the code a bit to the below (which doesn’t require updating if/when vMatch becomes a Txid container too):

 0diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
 1index 89176c01f5..c7f8cc7e1e 100644
 2--- a/src/wallet/rpc/backup.cpp
 3+++ b/src/wallet/rpc/backup.cpp
 4@@ -337,7 +337,6 @@ RPCHelpMan importprunedfunds()
 5     if (!DecodeHexTx(tx, request.params[0].get_str())) {
 6         throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input.");
 7     }
 8-    Txid hashTx = tx.GetHash();
 9 
10     DataStream ssMB{ParseHexV(request.params[1], "proof")};
11     CMerkleBlock merkleBlock;
12@@ -357,7 +356,7 @@ RPCHelpMan importprunedfunds()
13     }
14 
15     std::vector<uint256>::const_iterator it;
16-    if ((it = std::find(vMatch.begin(), vMatch.end(), hashTx.ToUint256())) == vMatch.end()) {
17+    if ((it = std::find(vMatch.begin(), vMatch.end(), tx.GetHash())) == vMatch.end()) {
18         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction given doesn't exist in proof");
19     }
20

marcofleon commented at 3:57 pm on April 17, 2025:

Good call, done. Made me realize I should update vMatch in the parent PR, so thanks.

in src/wallet/rpc/coins.cpp:59 in afbb1a1bfd outdated

55@@ -56,7 +56,7 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b
56 
57     // Tally
58     CAmount amount = 0;
59-    for (const std::pair<const uint256, CWalletTx>& wtx_pair : wallet.mapWallet) {
60+    for (const std::pair<const Txid, CWalletTx>& wtx_pair : wallet.mapWallet) {

stickies-v commented at 2:49 pm on April 15, 2025:

nit: could use structured bindings here:

 0diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
 1index 6b3645210d..f04384c86d 100644
 2--- a/src/wallet/rpc/coins.cpp
 3+++ b/src/wallet/rpc/coins.cpp
 4@@ -56,8 +56,7 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b
 5 
 6     // Tally
 7     CAmount amount = 0;
 8-    for (const std::pair<const Txid, CWalletTx>& wtx_pair : wallet.mapWallet) {
 9-        const CWalletTx& wtx = wtx_pair.second;
10+    for (const auto& [_, wtx] : wallet.mapWallet) {
11         int depth{wallet.GetTxDepthInMainChain(wtx)};
12         if (depth < min_depth
13             // Coinbase with less than 1 confirmation is no longer in the main chain

marcofleon commented at 3:58 pm on April 17, 2025:

Done.

in src/wallet/rpc/spend.cpp:1070 in afbb1a1bfd outdated

1066@@ -1067,7 +1067,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
1067         throw JSONRPCError(RPC_WALLET_ERROR, "bumpfee is not available with wallets that have private keys disabled. Use psbtbumpfee instead.");
1068     }
1069 
1070-    uint256 hash(ParseHashV(request.params[0], "txid"));
1071+    Txid hash = Txid::FromUint256((ParseHashV(request.params[0], "txid")));

stickies-v commented at 3:13 pm on April 15, 2025:

nit: too many parentheses (and might as well use curly braces)

0    Txid hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};

(edit: the same comment for removeprunedfunds, gettransaction and abandontransaction)

marcofleon commented at 3:58 pm on April 17, 2025:

Done.

in src/wallet/rpc/transactions.cpp:39 in afbb1a1bfd outdated

34@@ -34,11 +35,11 @@ static void WalletTxToJSON(const CWallet& wallet, const CWalletTx& wtx, UniValue
35     } else {
36         entry.pushKV("trusted", CachedTxIsTrusted(wallet, wtx));
37     }
38-    uint256 hash = wtx.GetHash();
39+    Txid hash = wtx.GetHash();
40     entry.pushKV("txid", hash.GetHex());

stickies-v commented at 3:15 pm on April 15, 2025:

nit: would just inline and make it consistent

0    entry.pushKV("txid", wtx.GetHash().GetHex());

marcofleon commented at 3:58 pm on April 17, 2025:

Done.

in src/wallet/rpc/transactions.cpp:104 in afbb1a1bfd outdated

100@@ -100,7 +101,7 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons
101 
102     // Tally
103     std::map<CTxDestination, tallyitem> mapTally;
104-    for (const std::pair<const uint256, CWalletTx>& pairWtx : wallet.mapWallet) {
105+    for (const std::pair<const Txid, CWalletTx>& pairWtx : wallet.mapWallet) {
106         const CWalletTx& wtx = pairWtx.second;
107

stickies-v commented at 3:20 pm on April 15, 2025:

nit: could use structured bindings

0    for (const auto& [_, wtx] : wallet.mapWallet) {

marcofleon commented at 3:59 pm on April 17, 2025:

Done.

in src/wallet/wallet.cpp:2298 in afbb1a1bfd outdated

2404@@ -2405,7 +2405,7 @@ DBErrors CWallet::LoadWallet()
2405     return nLoadWalletRet;
2406 }
2407 
2408-util::Result<void> CWallet::RemoveTxs(std::vector<uint256>& txs_to_remove)
2409+util::Result<void> CWallet::RemoveTxs(std::vector<Txid>& txs_to_remove)

stickies-v commented at 3:58 pm on April 15, 2025:

nit: could be a good time to update this to std::span?

 0diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
 1index 89176c01f5..6b246f9cbb 100644
 2--- a/src/wallet/rpc/backup.cpp
 3+++ b/src/wallet/rpc/backup.cpp
 4@@ -395,9 +395,7 @@ RPCHelpMan removeprunedfunds()
 5     LOCK(pwallet->cs_wallet);
 6 
 7     Txid hash = Txid::FromUint256((ParseHashV(request.params[0], "txid")));
 8-    std::vector<Txid> vHash;
 9-    vHash.push_back(hash);
10-    if (auto res = pwallet->RemoveTxs(vHash); !res) {
11+    if (auto res = pwallet->RemoveTxs(std::span{&hash, 1}); !res) {
12         throw JSONRPCError(RPC_WALLET_ERROR, util::ErrorString(res).original);
13     }
14 
15diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
16index 2b8d3149e6..0b9c1da0be 100644
17--- a/src/wallet/test/wallet_tests.cpp
18+++ b/src/wallet/test/wallet_tests.cpp
19@@ -951,8 +951,7 @@ BOOST_FIXTURE_TEST_CASE(RemoveTxs, TestChain100Setup)
20         BOOST_CHECK(wallet->HasWalletSpend(prev_tx));
21         BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 1u);
22 
23-        std::vector<Txid> vHashIn{ block_hash };
24-        BOOST_CHECK(wallet->RemoveTxs(vHashIn));
25+        BOOST_CHECK(wallet->RemoveTxs(std::span{&block_hash, 1}));
26 
27         BOOST_CHECK(!wallet->HasWalletSpend(prev_tx));
28         BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 0u);
29diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
30index c401909003..fff100ddbe 100644
31--- a/src/wallet/wallet.cpp
32+++ b/src/wallet/wallet.cpp
33@@ -75,6 +75,7 @@
34 #include <condition_variable>
35 #include <exception>
36 #include <optional>
37+#include <span>
38 #include <stdexcept>
39 #include <thread>
40 #include <tuple>
41@@ -2405,7 +2406,7 @@ DBErrors CWallet::LoadWallet()
42     return nLoadWalletRet;
43 }
44 
45-util::Result<void> CWallet::RemoveTxs(std::vector<Txid>& txs_to_remove)
46+util::Result<void> CWallet::RemoveTxs(std::span<Txid> txs_to_remove)
47 {
48     AssertLockHeld(cs_wallet);
49     bilingual_str str_err;  // future: make RunWithinTxn return a util::Result
50@@ -2419,7 +2420,7 @@ util::Result<void> CWallet::RemoveTxs(std::vector<Txid>& txs_to_remove)
51     return {}; // all good
52 }
53 
54-util::Result<void> CWallet::RemoveTxs(WalletBatch& batch, std::vector<Txid>& txs_to_remove)
55+util::Result<void> CWallet::RemoveTxs(WalletBatch& batch, std::span<Txid> txs_to_remove)
56 {
57     AssertLockHeld(cs_wallet);
58     if (!batch.HasActiveTxn()) return util::Error{strprintf(_("The transactions removal process can only be executed within a db txn"))};
59diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
60index 17ab3ec7c4..f5dabea51d 100644
61--- a/src/wallet/wallet.h
62+++ b/src/wallet/wallet.h
63@@ -44,6 +44,7 @@
64 #include <memory>
65 #include <optional>
66 #include <set>
67+#include <span>
68 #include <string>
69 #include <unordered_map>
70 #include <utility>
71@@ -793,8 +794,8 @@ public:
72     DBErrors LoadWallet();
73 
74     /** Erases the provided transactions from the wallet. */
75-    util::Result<void> RemoveTxs(std::vector<Txid>& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
76-    util::Result<void> RemoveTxs(WalletBatch& batch, std::vector<Txid>& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
77+    util::Result<void> RemoveTxs(std::span<Txid> txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
78+    util::Result<void> RemoveTxs(WalletBatch& batch, std::span<Txid> txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
79 
80     bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& purpose);
81

marcofleon commented at 4:00 pm on April 17, 2025:

I’ll leave this as is for now. A bit out of scope for this PR.

in src/wallet/wallet.cpp:773 in afbb1a1bfd outdated

769@@ -770,7 +770,7 @@ bool CWallet::IsSpent(const COutPoint& outpoint) const
770     range = mapTxSpends.equal_range(outpoint);
771 
772     for (TxSpends::const_iterator it = range.first; it != range.second; ++it) {
773-        const uint256& wtxid = it->second;
774+        const Txid& wtxid = it->second;

stickies-v commented at 4:14 pm on April 15, 2025:

nit: would be a nice opportunity to fix the confusing naming and just call this txid

marcofleon commented at 4:00 pm on April 17, 2025:

Agreed, done.

in src/wallet/wallet.cpp:784 in afbb1a1bfd outdated

780@@ -781,7 +781,7 @@ bool CWallet::IsSpent(const COutPoint& outpoint) const
781     return false;
782 }
783 
784-void CWallet::AddToSpends(const COutPoint& outpoint, const uint256& wtxid, WalletBatch* batch)
785+void CWallet::AddToSpends(const COutPoint& outpoint, const Txid& wtxid, WalletBatch* batch)

stickies-v commented at 4:14 pm on April 15, 2025:

nit: would be a nice opportunity to fix the confusing naming and just call this txid

in src/wallet/wallet.cpp:987 in afbb1a1bfd outdated

982@@ -983,12 +983,12 @@ void CWallet::MarkDirty()
983 {
984     {
985         LOCK(cs_wallet);
986-        for (std::pair<const uint256, CWalletTx>& item : mapWallet)
987+        for (std::pair<const Txid, CWalletTx>& item : mapWallet)
988             item.second.MarkDirty();

stickies-v commented at 4:16 pm on April 15, 2025:

structured bindings nit

0        for (auto& [_, wallet_tx] : mapWallet)
1            wallet_tx.MarkDirty();

marcofleon commented at 4:01 pm on April 17, 2025:

Done. I love structured bindings now.

in src/wallet/wallet.cpp:2429 in afbb1a1bfd outdated

2426     if (!batch.HasActiveTxn()) return util::Error{strprintf(_("The transactions removal process can only be executed within a db txn"))};
2427 
2428     // Check for transaction existence and remove entries from disk
2429-    using TxIterator = std::unordered_map<uint256, CWalletTx, SaltedTxidHasher>::const_iterator;
2430+    using TxIterator = std::unordered_map<Txid, CWalletTx, SaltedTxidHasher>::const_iterator;
2431     std::vector<TxIterator> erased_txs;

stickies-v commented at 4:29 pm on April 15, 2025:

nit: avoid redefining the iterator type

0    std::vector<decltype(mapWallet)::const_iterator> erased_txs;

marcofleon commented at 4:01 pm on April 17, 2025:

Done.

stickies-v commented at 4:38 pm on April 15, 2025: contributor

Approach ACK, nice to see increased type safety.

Can’t see any behaviour change, but I’m not confident enough yet that I didn’t miss any, so gonna give this another go. Left some style nits in the meanwhile.

If you have any rationale/assumptions that help verify that these changes are indeed refactors, could be good to put them in the PR description? E.g. relying on Txid’s type safety is obviously a big one.

w0xlt commented at 1:05 am on April 17, 2025: contributor

Approach ACK

stickies-v approved

stickies-v commented at 3:35 pm on April 17, 2025: contributor

ACK afbb1a1bfd729dc5e92cc5a83be681999a2d3b43

I checked all the callsites of functions with changed signatures and verified that indeed Txid is the correct type.

DrahtBot requested review from laanwj on Apr 17, 2025

DrahtBot requested review from hebasto on Apr 17, 2025

DrahtBot requested review from BrandonOdiwuor on Apr 17, 2025

marcofleon force-pushed on Apr 17, 2025

DrahtBot added the label CI failed on Apr 17, 2025

DrahtBot commented at 4:05 pm on April 17, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40737509570

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.

marcofleon force-pushed on Apr 17, 2025

DrahtBot removed the label CI failed on Apr 17, 2025

stickies-v commented at 6:15 pm on April 18, 2025: contributor

re-ACK 65fcfbb2b38bef20a58daa6c828c51890180611d - no changes except addressing style nits and adding missing includes.

nit: some of the new util/transaction_identifier.h includes, such as in interfaces/wallet.h could have been a forward declaration instead to minize compile time

yancyribbens commented at 12:40 pm on April 19, 2025: contributor

Concept ACK

DrahtBot added the label CI failed on Apr 29, 2025

DrahtBot removed the label CI failed on May 2, 2025

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

ACK 65fcfbb2b38bef20a58daa6c828c51890180611d

furszy commented at 11:40 pm on May 3, 2025: member

Would be good to register this new type into the Qt’s meta-object system. Look at the RegisterMetaTypes() function on src/qt/bitcoin.cpp. If we don’t do it, the framework will/should throw an error “not registered object type” when this new type is used as an argument in a signal or slot, or wrapped into a QVariant response (if I recall correctly, this was due to framework’s internal dynamic casting).

laanwj commented at 9:13 pm on May 5, 2025: member

Would be good to register this new type into the Qt’s meta-object system. Look at the RegisterMetaTypes() function on src/qt/bitcoin.cpp.

Good point. It’s used in at least the TransactionView::bumpedFee signal, which might be broken now.

It’s likely that adding only Q_DECLARE_METATYPE(Txid) at the top is enough as this was enough for uint256.

marcofleon force-pushed on May 6, 2025

marcofleon commented at 1:25 pm on May 6, 2025: contributor

Would be good to register this new type into the Qt’s meta-object system.

Good catch, thank you. Added it in.

It’s likely that adding only Q_DECLARE_METATYPE(Txid) at the top is enough

Thanks, makes sense. I checked the Qt docs too and found this: “Note that if you intend to use the type in queued signal and slot connections or in QObject’s property system, you also have to call qRegisterMetaType() since the names are resolved at runtime.”

I didn’t see anywhere where a Txid is used in invokeMethod or QueuedConnection (assuming that’s where to look), so I think just declaring it should be fine.

hebasto commented at 1:33 pm on May 6, 2025: member

Would be good to register this new type into the Qt’s meta-object system. Look at the RegisterMetaTypes() function on src/qt/bitcoin.cpp.

Good point. It’s used in at least the TransactionView::bumpedFee signal, which might be broken now.

It’s likely that adding only Q_DECLARE_METATYPE(Txid) at the top is enough as this was enough for uint256.

I don’t think it is necessary in Qt 6.

See: https://www.qt.io/blog/whats-new-in-qmetatype-qvariant

marcofleon force-pushed on May 6, 2025

marcofleon commented at 3:44 pm on May 6, 2025: contributor

Reverted the added Q_DECLARE_METATYPE following #32238 (comment). I’m assuming a lot of the metatype code will be dropped eventually now that we use Qt 6.

DrahtBot added the label Needs rebase on May 7, 2025

qt, refactor: Convert uint256 to Txid in the GUI

Switch all instances of a transaction from a uint256 to the Txid type.

b3214cefe6

wallet, refactor: Convert uint256 to Txid in wallet interfaces

In most cases throughout the wallet, the implicit conversion from `Txid` to
`const uint256&` works. However, `commitBumpTransaction` requires a `uint256&`
out parameter, so `bumped_txid` in `feebumper::CommitTransaction` is also
updated here to use `Txid`.

c8ed51e62b

wallet, refactor: Convert uint256 to Txid in wallet

Switch all instances of transactions from uint256 to Txid in the
wallet and relevant tests.

0671d66a8e

marcofleon force-pushed on May 7, 2025

marcofleon commented at 3:24 pm on May 7, 2025: contributor

Rebased after https://github.com/bitcoin/bitcoin/pull/28710

DrahtBot removed the label Needs rebase on May 7, 2025

qt, wallet: Convert uint256 to Txid #32238

Code Coverage & Benchmarks

Reviews

Conflicts