This is the final leg of the type safety refactor.
All of these changes are straightforward uint256 –> Txid along with any necessary explicit conversions. Also, transaction_identifier.h is moved to primitives in the last commit, as Txid and Wtxid become fundamental types after this PR.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33116.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, janb84, dergoegge, theStack |
| Stale ACK | maflcko, kevkevinpal |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
dbwrapper write methods by l0rinc)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.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/47204972085
LLM reason (✨ experimental): The CI failure is caused by a missing include guard in the file src/primitives/transaction_identifier.h, detected during a lint check.
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.
460@@ -461,12 +461,12 @@ static RPCHelpMan getmempoolancestors()
461 if (!request.params[1].isNull())
462 fVerbose = request.params[1].get_bool();
463
464- uint256 hash = ParseHashV(request.params[0], "parameter 1");
465+ auto hash{Txid::FromUint256(ParseHashV(request.params[0], "parameter 1"))};
NIT; is the variable name still descriptive ? would something like txId not be better suited ?
0 auto txid{Txid::FromUint256(ParseHashV(request.params[0], "parameter 1"))};
816@@ -817,7 +817,7 @@ static bool rest_tx(const std::any& context, HTTPRequest* req, const std::string
817 std::string hashStr;
818 const RESTResponseFormat rf = ParseDataFormat(hashStr, uri_part);
819
820- auto hash{uint256::FromHex(hashStr)};
821+ auto hash{Txid::FromHex(hashStr)};
NIT; is the variable name still descriptive ? would something like txId not be better suited ?
0 auto txid{Txid::FromHex(hashStr)};
hash seems to be used quite a bit throughout the gui and I don’t want to change more code than I do currently.
1123@@ -1124,7 +1124,7 @@ static RPCHelpMan submitpackage()
1124 }
1125 rpc_result.pushKV("tx-results", std::move(tx_result_map));
1126 UniValue replaced_list(UniValue::VARR);
1127- for (const uint256& hash : replaced_txids) replaced_list.push_back(hash.ToString());
1128+ for (const Txid& hash : replaced_txids) replaced_list.push_back(hash.ToString());
NIT; is the variable name still descriptive ?
0 for (const Txid& txid : replaced_txids) replaced_list.push_back(txid.ToString());
Concept ACK 92c5e147d9445e126f86a08538200d1f6a390efd
This PR is the last pr of the Txid type safety refactor initiative. In this PR transaction_identifier.h is moved to primitives to finish the refactor and uint256 to Txid (where relevant) along with any necessary explicit conversions. Which seems all correct.
During my code review, I have found some optional NITS regarding the use of variable name “hash”. Is this still a descriptive name after this refactor?
Steps undertaken:
Adds back a comment that was unintentionally deleted in
the GenTxid refactor.
re ACK a20724d926d5844168c6a13fa8293df8c8927efe
changes since last ACK:
2351@@ -2352,7 +2352,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
2352 // they must either disconnect and retry or request the full block.
2353 // Thus, the protocol spec specified allows for us to provide duplicate txn here,
2354 // however we MUST always provide at least what the remote peer needs
2355- typedef std::pair<unsigned int, uint256> PairType;
2356+ typedef std::pair<unsigned int, Txid> PairType;
nit: we don’t actually use the Txid element here, so we can avoid the typedef altogether:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index 318b50f7fe..30074bff71 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -2352,9 +2352,8 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
5 // they must either disconnect and retry or request the full block.
6 // Thus, the protocol spec specified allows for us to provide duplicate txn here,
7 // however we MUST always provide at least what the remote peer needs
8- typedef std::pair<unsigned int, Txid> PairType;
9- for (PairType& pair : merkleBlock.vMatchedTxn)
10- MakeAndPushMessage(pfrom, NetMsgType::TX, TX_NO_WITNESS(*pblock->vtx[pair.first]));
11+ for (auto& [tx_idx, _] : merkleBlock.vMatchedTxn)
12+ MakeAndPushMessage(pfrom, NetMsgType::TX, TX_NO_WITNESS(*pblock->vtx[tx_idx]));
13 }
14 // else
15 // no response
460@@ -461,12 +461,12 @@ static RPCHelpMan getmempoolancestors()
461 if (!request.params[1].isNull())
462 fVerbose = request.params[1].get_bool();
463
464- uint256 hash = ParseHashV(request.params[0], "parameter 1");
465+ auto txid{Txid::FromUint256(ParseHashV(request.params[0], "parameter 1"))};
482@@ -483,10 +483,10 @@ static RPCHelpMan getmempoolancestors()
483 UniValue o(UniValue::VOBJ);
484 for (CTxMemPool::txiter ancestorIt : ancestors) {
485 const CTxMemPoolEntry &e = *ancestorIt;
486- const uint256& _hash = e.GetTx().GetHash();
487+ const Txid& hash = e.GetTx().GetHash();
488 UniValue info(UniValue::VOBJ);
489 entryToJSON(mempool, info, e);
490- o.pushKV(_hash.ToString(), std::move(info));
491+ o.pushKV(hash.ToString(), std::move(info));
nit: inlining it is more clear imo (+below)
0diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
1index 18363ddebc..a37e4cf4b8 100644
2--- a/src/rpc/mempool.cpp
3+++ b/src/rpc/mempool.cpp
4@@ -483,10 +483,9 @@ static RPCHelpMan getmempoolancestors()
5 UniValue o(UniValue::VOBJ);
6 for (CTxMemPool::txiter ancestorIt : ancestors) {
7 const CTxMemPoolEntry &e = *ancestorIt;
8- const Txid& hash = e.GetTx().GetHash();
9 UniValue info(UniValue::VOBJ);
10 entryToJSON(mempool, info, e);
11- o.pushKV(hash.ToString(), std::move(info));
12+ o.pushKV(e.GetTx().GetHash().ToString(), std::move(info));
13 }
14 return o;
15 }
16@@ -549,10 +548,9 @@ static RPCHelpMan getmempooldescendants()
17 UniValue o(UniValue::VOBJ);
18 for (CTxMemPool::txiter descendantIt : setDescendants) {
19 const CTxMemPoolEntry &e = *descendantIt;
20- const Txid& hash = e.GetTx().GetHash();
21 UniValue info(UniValue::VOBJ);
22 entryToJSON(mempool, info, e);
23- o.pushKV(hash.ToString(), std::move(info));
24+ o.pushKV(e.GetTx().GetHash().ToString(), std::move(info));
25 }
26 return o;
27 }
38@@ -39,7 +39,7 @@ BOOST_AUTO_TEST_CASE(merkleblock_construct_from_txids_found)
39 // vMatchedTxn is only used when bloom filter is specified.
40 BOOST_CHECK_EQUAL(merkleBlock.vMatchedTxn.size(), 0U);
41
42- std::vector<uint256> vMatched;
43+ std::vector<Txid> vMatched;
nit: now that both types are identical, we can do direct comparison:
0diff --git a/src/test/merkleblock_tests.cpp b/src/test/merkleblock_tests.cpp
1index 243214d329..48a61a3fc0 100644
2--- a/src/test/merkleblock_tests.cpp
3+++ b/src/test/merkleblock_tests.cpp
4@@ -46,10 +46,10 @@ BOOST_AUTO_TEST_CASE(merkleblock_construct_from_txids_found)
5 BOOST_CHECK_EQUAL(vMatched.size(), 2U);
6
7 // Ordered by occurrence in depth-first tree traversal.
8- BOOST_CHECK_EQUAL(vMatched[0].ToString(), txhash2.ToString());
9+ BOOST_CHECK_EQUAL(vMatched[0], txhash2);
10 BOOST_CHECK_EQUAL(vIndex[0], 1U);
11
12- BOOST_CHECK_EQUAL(vMatched[1].ToString(), txhash1.ToString());
13+ BOOST_CHECK_EQUAL(vMatched[1], txhash1);
14 BOOST_CHECK_EQUAL(vIndex[1], 8U);
15 }
16
60@@ -61,7 +61,7 @@ MiniMiner::MiniMiner(const CTxMemPool& mempool, const std::vector<COutPoint>& ou
61 if (m_requested_outpoints_by_txid.empty()) return;
62
63 // Calculate the cluster and construct the entry map.
64- std::vector<uint256> txids_needed;
65+ std::vector<Txid> txids_needed;
nit: could modernize
0diff --git a/src/node/mini_miner.cpp b/src/node/mini_miner.cpp
1index c26355d832..a1c2edcb7b 100644
2--- a/src/node/mini_miner.cpp
3+++ b/src/node/mini_miner.cpp
4@@ -16,6 +16,7 @@
5
6 #include <algorithm>
7 #include <numeric>
8+#include <ranges>
9 #include <utility>
10
11 namespace node {
12@@ -61,12 +62,8 @@ MiniMiner::MiniMiner(const CTxMemPool& mempool, const std::vector<COutPoint>& ou
13 if (m_requested_outpoints_by_txid.empty()) return;
14
15 // Calculate the cluster and construct the entry map.
16- std::vector<Txid> txids_needed;
17- txids_needed.reserve(m_requested_outpoints_by_txid.size());
18- for (const auto& [txid, _]: m_requested_outpoints_by_txid) {
19- txids_needed.push_back(txid);
20- }
21- const auto cluster = mempool.GatherClusters(txids_needed);
22+ auto txids_needed{m_requested_outpoints_by_txid | std::views::keys};
23+ const auto cluster = mempool.GatherClusters({txids_needed.begin(), txids_needed.end()});
24 if (cluster.empty()) {
25 // An empty cluster means that at least one of the transactions is missing from the mempool
26 // (should not be possible given processing above) or DoS limit was hit.
review ACK a20724d926d5844168c6a13fa8293df8c8927efe 🎰
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK a20724d926d5844168c6a13fa8293df8c8927efe 🎰
3owjWK+M1xuldjVXGuTv8u/HzGJbx30wzLvhYH1vVwALP65R1cq9YqC+QVHEwpFHaQTLY9RmEbhRByKiC25jNBQ==
37@@ -23,8 +38,9 @@ class SaltedTxidHasher
38 public:
39 SaltedTxidHasher();
40
41- size_t operator()(const uint256& txid) const {
42- return SipHashUint256(k0, k1, txid);
43+ template <TxidOrWtxid T>
223@@ -224,7 +224,7 @@ class HeadersSyncState {
224 arith_uint256 m_current_chain_work;
225
226 /** m_hasher is a salted hasher for making our 1-bit commitments to headers we've seen. */
227- const SaltedTxidHasher m_hasher;
228+ const SaltedUint256Hasher m_hasher;
180@@ -181,12 +181,12 @@ BOOST_AUTO_TEST_CASE(merkle_block_1)
181 BOOST_CHECK_EQUAL(merkleBlock.header.GetHash().GetHex(), block.GetHash().GetHex());
182
183 BOOST_CHECK_EQUAL(merkleBlock.vMatchedTxn.size(), 1U);
184- std::pair<unsigned int, uint256> pair = merkleBlock.vMatchedTxn[0];
185+ std::pair<unsigned int, Txid> pair = merkleBlock.vMatchedTxn[0];
186
187- BOOST_CHECK(merkleBlock.vMatchedTxn[0].second == uint256{"74d681e0e03bafa802c8aa084379aa98d9fcd632ddc2ed9782b586ec87451f20"});
188+ BOOST_CHECK(merkleBlock.vMatchedTxn[0].second == Txid::FromHex("74d681e0e03bafa802c8aa084379aa98d9fcd632ddc2ed9782b586ec87451f20").value());
FromUint256 to retain the compile-time check here.
780@@ -781,7 +781,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
781 AddCoins(mempoolDuplicate, tx, std::numeric_limits<int>::max());
782 }
783 for (auto it = mapNextTx.cbegin(); it != mapNextTx.cend(); it++) {
784- uint256 hash = it->second->GetHash();
785+ Txid hash = it->second->GetHash();
nit: we don’t need the iterator, so could modernize to:
0diff --git a/src/txmempool.cpp b/src/txmempool.cpp
1index d50c39593d..bbdcedd28f 100644
2--- a/src/txmempool.cpp
3+++ b/src/txmempool.cpp
4@@ -780,12 +780,11 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
5 for (const auto& input: tx.vin) mempoolDuplicate.SpendCoin(input.prevout);
6 AddCoins(mempoolDuplicate, tx, std::numeric_limits<int>::max());
7 }
8- for (auto it = mapNextTx.cbegin(); it != mapNextTx.cend(); it++) {
9- Txid hash = it->second->GetHash();
10- indexed_transaction_set::const_iterator it2 = mapTx.find(hash);
11+ for (const auto& [_, next_tx] : mapNextTx) {
12+ auto it2 = mapTx.find(next_tx->GetHash());
13 const CTransaction& tx = it2->GetTx();
14 assert(it2 != mapTx.end());
15- assert(&tx == it->second);
16+ assert(&tx == next_tx);
17 }
18
19 assert(totalTxSize == checkTotal);
76@@ -77,7 +77,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
77 /*has_no_mempool_parents=*/true)};
78 m_node.validation_signals->TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence());
79 }
80- uint256 hash = tx.GetHash();
81+ Txid hash = tx.GetHash();
nit: just inline (+ 2 times below)
0diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp
1index 3e16ac15e1..bbdd815fe4 100644
2--- a/src/test/policyestimator_tests.cpp
3+++ b/src/test/policyestimator_tests.cpp
4@@ -77,8 +77,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
5 /*has_no_mempool_parents=*/true)};
6 m_node.validation_signals->TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence());
7 }
8- Txid hash = tx.GetHash();
9- txHashes[j].push_back(hash);
10+ txHashes[j].push_back(tx.GetHash());
11 }
12 }
13 //Create blocks where higher fee txs are included more often
14@@ -178,8 +177,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
15 /*has_no_mempool_parents=*/true)};
16 m_node.validation_signals->TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence());
17 }
18- Txid hash = tx.GetHash();
19- txHashes[j].push_back(hash);
20+ txHashes[j].push_back(tx.GetHash());
21 }
22 }
23 {
24@@ -242,8 +240,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
25 /*has_no_mempool_parents=*/true)};
26 m_node.validation_signals->TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence());
27 }
28- Txid hash = tx.GetHash();
29- CTransactionRef ptx = mpool.get(hash);
30+ CTransactionRef ptx = mpool.get(tx.GetHash());
31 if (ptx)
32 block.push_back(ptx);
33
61@@ -62,7 +62,7 @@ std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
62 CTxMemPool::setEntries& all_conflicts)
63 {
64 AssertLockHeld(pool.cs);
65- const uint256 txid = tx.GetHash();
66+ const Txid txid = tx.GetHash();
unnecessary allocation nit
0diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp
1index 40b82f5a5c..210b94915c 100644
2--- a/src/policy/rbf.cpp
3+++ b/src/policy/rbf.cpp
4@@ -62,7 +62,6 @@ std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
5 CTxMemPool::setEntries& all_conflicts)
6 {
7 AssertLockHeld(pool.cs);
8- const Txid txid = tx.GetHash();
9 uint64_t nConflictingCount = 0;
10 for (const auto& mi : iters_conflicting) {
11 nConflictingCount += mi->GetCountWithDescendants();
12@@ -72,7 +71,7 @@ std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
13 // times), but we just want to be conservative to avoid doing too much work.
14 if (nConflictingCount > MAX_REPLACEMENT_CANDIDATES) {
15 return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)",
16- txid.ToString(),
17+ tx.GetHash().ToString(),
18 nConflictingCount,
19 MAX_REPLACEMENT_CANDIDATES);
20 }
48+bool TxIndex::DB::WriteTxs(const std::vector<std::pair<Txid, CDiskTxPos>>& v_pos)
49 {
50 CDBBatch batch(*this);
51 for (const auto& tuple : v_pos) {
52- batch.Write(std::make_pair(DB_TXINDEX, tuple.first), tuple.second);
53+ batch.Write(std::make_pair(DB_TXINDEX, tuple.first.ToUint256()), tuple.second);
structured bindings nit:
0diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp
1index a9bb90a697..11dd856e1b 100644
2--- a/src/index/txindex.cpp
3+++ b/src/index/txindex.cpp
4@@ -43,8 +43,8 @@ bool TxIndex::DB::ReadTxPos(const Txid& txid, CDiskTxPos& pos) const
5 bool TxIndex::DB::WriteTxs(const std::vector<std::pair<Txid, CDiskTxPos>>& v_pos)
6 {
7 CDBBatch batch(*this);
8- for (const auto& tuple : v_pos) {
9- batch.Write(std::make_pair(DB_TXINDEX, tuple.first.ToUint256()), tuple.second);
10+ for (const auto& [txid, pos] : v_pos) {
11+ batch.Write(std::make_pair(DB_TXINDEX, txid.ToUint256()), pos);
12 }
13 return WriteBatch(batch);
14 }
21@@ -22,7 +22,7 @@ BOOST_AUTO_TEST_CASE(disconnectpool_memory_limits)
22 // is within an expected range.
23
24 // Overhead for the hashmap depends on number of buckets
25- std::unordered_map<uint256, CTransaction*, SaltedTxidHasher> temp_map;
26+ std::unordered_map<uint256, CTransaction*, SaltedUint256Hasher> temp_map;
Since DisconnectedBlockTransactions is Txid based, seems unusual to regress to uint256 here?
0 std::unordered_map<Txid, CTransaction*, SaltedTxidHasher> temp_map;
ACK a20724d926d5844168c6a13fa8293df8c8927efe
Left nits mostly around making incremental improvements around code touched in this PR, but nothing blocking.
ACK a20724d
I tested by doing the following
To check for any usage of .uint256 instead of .ToUint256
grep -nri "\.uint256" ./src
To check for any imports of util/transaction_identifier
grep -nri "util/transaction_identifier" ./src
One thing I did notice is that Txid::FromHex (introduced in #30482) is only used in the following locations, does it make sense to swap it out with uint256::FromHex? Not needed in this PR though
0src/rest.cpp|820 col 21| auto hash{Txid::FromHex(hashStr)};
1src/rest.cpp|909 col 29| auto txid{Txid::FromHex(txid_out.at(0))};
2src/rpc/mining.cpp|356 col 29| if (auto txid{Txid::FromHex(str)}) {
Bit late to the party, but I would like to ask a question:
Do we expect (present or future) any unordered_ containers keyed by GenTxid? From what I see, we already have salted hashers for Txid and Wtxid and use them in hashed containers. GenTxid (either Txid or Wtxid) uses and ordered map, so no hasher is needed. But if anyone later wants an unordered_map<GenTxid, ...> (e.g. for speed), they might convert back to uint256 or rely on std::hash (which weakens type-safety and DoS safety?). Would a proper SaltedGenTxidHasher (which mixes tag + bytes) make sense? Asking also for clarification whether I got the understanding right.
These remaining miscellaneous changes were identified by commenting out
the `operator const uint256&` conversion and the `Compare(const uint256&)`
method from `transaction_identifier.h`.
Moves the file from `src/util` to `src/primitives`. Now that the
refactor is complete, Txid and Wtxid are fundamental types, so it
makes sense for them to reside in `src/primitives`.
Recent push addresses all comments from @maflcko and @stickies-v. Thanks!
does it make sense to swap it out with
uint256::FromHex@kevkevinpal It seems to be used in various tests as well, so it might be worth keeping for the time being.
Do we expect (present or future) any unordered_ containers keyed by GenTxid? @cedwies Not that I know of, but if at some point we introduce a
GenTxidunordered map, then yeah a hasher for GenTxids could make sense. I’m not too sure about the DoS attack potential if it only hashed by txhash and didn’t include type? But either way, I think we implement this if needed later on.
re ACK de0675f9de5feae1f070840ad7218b1378fb880b
changes since last ACK:
321@@ -322,10 +322,10 @@ static RPCHelpMan getrawtransaction()
322 const NodeContext& node = EnsureAnyNodeContext(request.context);
323 ChainstateManager& chainman = EnsureChainman(node);
324
325- uint256 hash = ParseHashV(request.params[0], "parameter 1");
326+ auto txid{Txid::FromUint256(ParseHashV(request.params[0], "parameter 1"))};
132@@ -133,7 +133,7 @@ uint256 CPartialMerkleTree::TraverseAndExtract(int height, unsigned int pos, uns
133 }
134 }
135
136-CPartialMerkleTree::CPartialMerkleTree(const std::vector<uint256> &vTxid, const std::vector<bool> &vMatch) : nTransactions(vTxid.size()), fBad(false) {
137+CPartialMerkleTree::CPartialMerkleTree(const std::vector<Txid> &vTxid, const std::vector<bool> &vMatch) : nTransactions(vTxid.size()), fBad(false) {
vMatch is used for different types (vector of bool, vector of txids) in different methods is a bit confusing, probably worth it to rename if the file is touched again in the future