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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33116.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/47204972085</sub>
<sub>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.</sub>
<details><summary>Hints</summary>
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.
</details>
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 ?
auto txid{Txid::FromUint256(ParseHashV(request.params[0], "parameter 1"))};
fixed
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 ?
auto txid{Txid::FromHex(hashStr)};
Left this one as is because 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 ?
for (const Txid& txid : replaced_txids) replaced_list.push_back(txid.ToString());
fixed, along with the other ones in the rpc code
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:
Code review ACK a20724d926d5844168c6a13fa8293df8c8927efe
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:
<details> <summary>git diff on a20724d926</summary>
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 318b50f7fe..30074bff71 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2352,9 +2352,8 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
// they must either disconnect and retry or request the full block.
// Thus, the protocol spec specified allows for us to provide duplicate txn here,
// however we MUST always provide at least what the remote peer needs
- typedef std::pair<unsigned int, Txid> PairType;
- for (PairType& pair : merkleBlock.vMatchedTxn)
- MakeAndPushMessage(pfrom, NetMsgType::TX, TX_NO_WITNESS(*pblock->vtx[pair.first]));
+ for (auto& [tx_idx, _] : merkleBlock.vMatchedTxn)
+ MakeAndPushMessage(pfrom, NetMsgType::TX, TX_NO_WITNESS(*pblock->vtx[tx_idx]));
}
// else
// no response
</details>
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"))};
orthogonal nit: not sure why we're passing "parameter 1" instead of "txid" (here, + in 2 more locations), less clear and suffers from 0-indexing ambiguity.
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)
<details> <summary>git diff on a20724d926</summary>
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
index 18363ddebc..a37e4cf4b8 100644
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -483,10 +483,9 @@ static RPCHelpMan getmempoolancestors()
UniValue o(UniValue::VOBJ);
for (CTxMemPool::txiter ancestorIt : ancestors) {
const CTxMemPoolEntry &e = *ancestorIt;
- const Txid& hash = e.GetTx().GetHash();
UniValue info(UniValue::VOBJ);
entryToJSON(mempool, info, e);
- o.pushKV(hash.ToString(), std::move(info));
+ o.pushKV(e.GetTx().GetHash().ToString(), std::move(info));
}
return o;
}
@@ -549,10 +548,9 @@ static RPCHelpMan getmempooldescendants()
UniValue o(UniValue::VOBJ);
for (CTxMemPool::txiter descendantIt : setDescendants) {
const CTxMemPoolEntry &e = *descendantIt;
- const Txid& hash = e.GetTx().GetHash();
UniValue info(UniValue::VOBJ);
entryToJSON(mempool, info, e);
- o.pushKV(hash.ToString(), std::move(info));
+ o.pushKV(e.GetTx().GetHash().ToString(), std::move(info));
}
return o;
}
</details>
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:
<details> <summary>git diff on a20724d926</summary>
diff --git a/src/test/merkleblock_tests.cpp b/src/test/merkleblock_tests.cpp
index 243214d329..48a61a3fc0 100644
--- a/src/test/merkleblock_tests.cpp
+++ b/src/test/merkleblock_tests.cpp
@@ -46,10 +46,10 @@ BOOST_AUTO_TEST_CASE(merkleblock_construct_from_txids_found)
BOOST_CHECK_EQUAL(vMatched.size(), 2U);
// Ordered by occurrence in depth-first tree traversal.
- BOOST_CHECK_EQUAL(vMatched[0].ToString(), txhash2.ToString());
+ BOOST_CHECK_EQUAL(vMatched[0], txhash2);
BOOST_CHECK_EQUAL(vIndex[0], 1U);
- BOOST_CHECK_EQUAL(vMatched[1].ToString(), txhash1.ToString());
+ BOOST_CHECK_EQUAL(vMatched[1], txhash1);
BOOST_CHECK_EQUAL(vIndex[1], 8U);
}
</details>
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
<details> <summary>git diff on a20724d926</summary>
diff --git a/src/node/mini_miner.cpp b/src/node/mini_miner.cpp
index c26355d832..a1c2edcb7b 100644
--- a/src/node/mini_miner.cpp
+++ b/src/node/mini_miner.cpp
@@ -16,6 +16,7 @@
#include <algorithm>
#include <numeric>
+#include <ranges>
#include <utility>
namespace node {
@@ -61,12 +62,8 @@ MiniMiner::MiniMiner(const CTxMemPool& mempool, const std::vector<COutPoint>& ou
if (m_requested_outpoints_by_txid.empty()) return;
// Calculate the cluster and construct the entry map.
- std::vector<Txid> txids_needed;
- txids_needed.reserve(m_requested_outpoints_by_txid.size());
- for (const auto& [txid, _]: m_requested_outpoints_by_txid) {
- txids_needed.push_back(txid);
- }
- const auto cluster = mempool.GatherClusters(txids_needed);
+ auto txids_needed{m_requested_outpoints_by_txid | std::views::keys};
+ const auto cluster = mempool.GatherClusters({txids_needed.begin(), txids_needed.end()});
if (cluster.empty()) {
// An empty cluster means that at least one of the transactions is missing from the mempool
// (should not be possible given processing above) or DoS limit was hit.
</details>
review ACK a20724d926d5844168c6a13fa8293df8c8927efe 🎰
<details><summary>Show signature</summary>
Signature:
untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK a20724d926d5844168c6a13fa8293df8c8927efe 🎰
owjWK+M1xuldjVXGuTv8u/HzGJbx30wzLvhYH1vVwALP65R1cq9YqC+QVHEwpFHaQTLY9RmEbhRByKiC25jNBQ==
</details>
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>
nit in 5528e0f6e01438f15ed997269bdb4db55873c0ba: Seems a bit odd why this is required. Shouldn't call sites only either put txid or wtxid in here, known at compile time? Also, it seems a bit confusing to call it txid-hasher, when it hashes the wtxid.
You're saying have both a txid and a wtxid hasher, yes? I think that would make sense.
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;
doc-nit: you'll also have to adjust the cpp file
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());
nit: You can use FromUint256 to retain the compile-time check here.
(nits are non-blocking)
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:
<details> <summary>git diff on a20724d926</summary>
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index d50c39593d..bbdcedd28f 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -780,12 +780,11 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
for (const auto& input: tx.vin) mempoolDuplicate.SpendCoin(input.prevout);
AddCoins(mempoolDuplicate, tx, std::numeric_limits<int>::max());
}
- for (auto it = mapNextTx.cbegin(); it != mapNextTx.cend(); it++) {
- Txid hash = it->second->GetHash();
- indexed_transaction_set::const_iterator it2 = mapTx.find(hash);
+ for (const auto& [_, next_tx] : mapNextTx) {
+ auto it2 = mapTx.find(next_tx->GetHash());
const CTransaction& tx = it2->GetTx();
assert(it2 != mapTx.end());
- assert(&tx == it->second);
+ assert(&tx == next_tx);
}
assert(totalTxSize == checkTotal);
</details>
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)
<details> <summary>git diff on a20724d926</summary>
diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp
index 3e16ac15e1..bbdd815fe4 100644
--- a/src/test/policyestimator_tests.cpp
+++ b/src/test/policyestimator_tests.cpp
@@ -77,8 +77,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
/*has_no_mempool_parents=*/true)};
m_node.validation_signals->TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence());
}
- Txid hash = tx.GetHash();
- txHashes[j].push_back(hash);
+ txHashes[j].push_back(tx.GetHash());
}
}
//Create blocks where higher fee txs are included more often
@@ -178,8 +177,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
/*has_no_mempool_parents=*/true)};
m_node.validation_signals->TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence());
}
- Txid hash = tx.GetHash();
- txHashes[j].push_back(hash);
+ txHashes[j].push_back(tx.GetHash());
}
}
{
@@ -242,8 +240,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
/*has_no_mempool_parents=*/true)};
m_node.validation_signals->TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence());
}
- Txid hash = tx.GetHash();
- CTransactionRef ptx = mpool.get(hash);
+ CTransactionRef ptx = mpool.get(tx.GetHash());
if (ptx)
block.push_back(ptx);
</details>
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
<details> <summary>git diff on a20724d926</summary>
diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp
index 40b82f5a5c..210b94915c 100644
--- a/src/policy/rbf.cpp
+++ b/src/policy/rbf.cpp
@@ -62,7 +62,6 @@ std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
CTxMemPool::setEntries& all_conflicts)
{
AssertLockHeld(pool.cs);
- const Txid txid = tx.GetHash();
uint64_t nConflictingCount = 0;
for (const auto& mi : iters_conflicting) {
nConflictingCount += mi->GetCountWithDescendants();
@@ -72,7 +71,7 @@ std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
// times), but we just want to be conservative to avoid doing too much work.
if (nConflictingCount > MAX_REPLACEMENT_CANDIDATES) {
return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)",
- txid.ToString(),
+ tx.GetHash().ToString(),
nConflictingCount,
MAX_REPLACEMENT_CANDIDATES);
}
</details>
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:
<details> <summary>git diff on a20724d926</summary>
diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp
index a9bb90a697..11dd856e1b 100644
--- a/src/index/txindex.cpp
+++ b/src/index/txindex.cpp
@@ -43,8 +43,8 @@ bool TxIndex::DB::ReadTxPos(const Txid& txid, CDiskTxPos& pos) const
bool TxIndex::DB::WriteTxs(const std::vector<std::pair<Txid, CDiskTxPos>>& v_pos)
{
CDBBatch batch(*this);
- for (const auto& tuple : v_pos) {
- batch.Write(std::make_pair(DB_TXINDEX, tuple.first.ToUint256()), tuple.second);
+ for (const auto& [txid, pos] : v_pos) {
+ batch.Write(std::make_pair(DB_TXINDEX, txid.ToUint256()), pos);
}
return WriteBatch(batch);
}
</details>
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?
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
src/rest.cpp|820 col 21| auto hash{Txid::FromHex(hashStr)};
src/rest.cpp|909 col 29| auto txid{Txid::FromHex(txid_out.at(0))};
src/rpc/mining.cpp|356 col 29| if (auto txid{Txid::FromHex(str)}) {
I appreciate the reviews. Let me address the nits before this is merged, even if non-blocking. No rush, so might as well clean it up.
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, no changes since a20724d926d5844168c6a13fa8293df8c8927efe except address review nits.
re ACK de0675f9de5feae1f070840ad7218b1378fb880b
changes since last ACK:
re-ACK de0675f9de5feae1f070840ad7218b1378fb880b
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"))};
nit, if you have to retouch: could rename this one to "txid" as well (and with s/parameter 3/blockhash/ below, we got them all)
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) {
out-of-scope nit: the fact that the name 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
Code-review ACK de0675f9de5feae1f070840ad7218b1378fb880b