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
Approach ACK	stickies-v

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:

#32189 (refactor: Txid type safety (parent PR) by marcofleon)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#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

qt, refactor: Convert uint256 to Txid in the GUI

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

fe8a7f809a

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

50bee953f0

wallet, refactor: Convert uint256 to Txid in wallet

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

afbb1a1bfd

marcofleon force-pushed on Apr 14, 2025

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

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

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

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

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

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

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

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

in src/wallet/rpc/transactions.cpp:106 in afbb1a1bfd

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

in src/wallet/wallet.cpp:2408 in afbb1a1bfd

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

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

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

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

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

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

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

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;

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.

qt, wallet: Convert uint256 to Txid #32238

Code Coverage & Benchmarks

Reviews

Conflicts