Part of the type safety refactor.
This PR changes the GenTxid class to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256.
<!--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/32631.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | w0xlt, dergoegge, maflcko, theStack |
| Concept ACK | instagibbs, hodlinator |
| 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.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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-->
In txrequest the ByPeer and ByTxHash indexes still sort announcements by the underlying uint256 txhash only, as opposed to using the GenTxid comparators which sort by type and then hash. The patch below can be used to make sure that there are no accidental switches from uint256 comparisons to GenTxid comparisons.
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 011e3cd928..1a47a39dfd 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -206,6 +206,12 @@ struct QueuedBlock {
std::unique_ptr<PartiallyDownloadedBlock> partialBlock;
};
+struct CompareGenTxid {
+ bool operator()(const GenTxid& a, const GenTxid& b) const {
+ return std::tuple(a.index(), a.ToUint256()) < std::tuple(b.index(), b.ToUint256());
+ }
+};
+
/**
* Data structure for an individual peer. This struct is not protected by
* cs_main since it does not contain validation-critical data.
@@ -302,7 +308,7 @@ struct Peer {
* non-wtxid-relay peers, wtxid for wtxid-relay peers). We use the
* mempool to sort transactions in dependency order before relay, so
* this does not have to be sorted. */
- std::set<GenTxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);
+ std::set<GenTxid, CompareGenTxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);
/** Whether the peer has requested us to send our complete mempool. Only
* permitted if the peer has NetPermissionFlags::Mempool or we advertise
* NODE_BLOOM. See BIP35. */
@@ -856,7 +862,7 @@ private:
std::shared_ptr<const CBlock> m_most_recent_block GUARDED_BY(m_most_recent_block_mutex);
std::shared_ptr<const CBlockHeaderAndShortTxIDs> m_most_recent_compact_block GUARDED_BY(m_most_recent_block_mutex);
uint256 m_most_recent_block_hash GUARDED_BY(m_most_recent_block_mutex);
- std::unique_ptr<const std::map<GenTxid, CTransactionRef>> m_most_recent_block_txs GUARDED_BY(m_most_recent_block_mutex);
+ std::unique_ptr<const std::map<GenTxid, CTransactionRef, CompareGenTxid>> m_most_recent_block_txs GUARDED_BY(m_most_recent_block_mutex);
// Data about the low-work headers synchronization, aggregated from all peers' HeadersSyncStates.
/** Mutex guarding the other m_headers_presync_* variables. */
@@ -2027,7 +2033,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha
std::async(std::launch::deferred, [&] { return NetMsg::Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })};
{
- auto most_recent_block_txs = std::make_unique<std::map<GenTxid, CTransactionRef>>();
+ auto most_recent_block_txs = std::make_unique<std::map<GenTxid, CTransactionRef, CompareGenTxid>>();
for (const auto& tx : pblock->vtx) {
most_recent_block_txs->emplace(tx->GetHash(), tx);
most_recent_block_txs->emplace(tx->GetWitnessHash(), tx);
diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
index 3cdeb664d4..6ab5d98263 100644
--- a/src/util/transaction_identifier.h
+++ b/src/util/transaction_identifier.h
@@ -88,13 +88,12 @@ public:
return std::visit([](const auto& id) -> const uint256& { return id.ToUint256(); }, *this);
}
- friend bool operator==(const GenTxid& a, const GenTxid& b) {
- return a.index() == b.index() && a.ToUint256() == b.ToUint256();
- }
-
- friend bool operator<(const GenTxid& a, const GenTxid& b) {
- return std::tuple(a.index(), a.ToUint256()) < std::tuple(b.index(), b.ToUint256());
- }
+ friend bool operator==(const GenTxid& a, const GenTxid& b) = delete;
+ friend bool operator!=(const GenTxid& a, const GenTxid& b) = delete;
+ friend bool operator<(const GenTxid& a, const GenTxid& b) = delete;
+ friend bool operator<=(const GenTxid& a, const GenTxid& b) = delete;
+ friend bool operator>(const GenTxid& a, const GenTxid& b) = delete;
+ friend bool operator>=(const GenTxid& a, const GenTxid& b) = delete;
};
#endif // BITCOIN_UTIL_TRANSACTION_IDENTIFIER_H
Then run cmake --build build --target bitcoind. To check all tests as well, comment out the unit and fuzz tests for txrequest, as these tests directly compare GenTxid objects.
382 | + GenTxid gtxid; 383 | + if (fuzzed_data_provider.ConsumeBool()) { 384 | + rand_tx->GetHash(); 385 | + } else { 386 | + rand_tx->GetWitnessHash(); 387 | + }
The return values for GetHash and GetWitnessHash need to be assigned to gtxid
5818 | + std::set<GenTxid>::iterator it = vInvTx.back(); 5819 | vInvTx.pop_back(); 5820 | - uint256 hash = *it; 5821 | - CInv inv(peer->m_wtxid_relay ? MSG_WTX : MSG_TX, hash); 5822 | + GenTxid hash = *it; 5823 | + CInv inv(peer->m_wtxid_relay ? MSG_WTX : MSG_TX, hash.ToUint256());
Assume(peer->m_wtxid_relay == std::holds_alternative<Wtxid>(hash));
CInv inv(peer->m_wtxid_relay ? MSG_WTX : MSG_TX, hash.ToUint256());
done
377 | @@ -378,27 +378,27 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction
378 | // We can tolerate having up to 1 parent in m_lazy_recent_rejects_reconsiderable since we
379 | // submit 1p1c packages. However, fail immediately if any are in m_lazy_recent_rejects.
380 | std::optional<uint256> rejected_parent_reconsiderable;
not that it really matters but could do
std::optional<Txid> rejected_parent_reconsiderable;
completed
92 | + return a.index() == b.index() && a.ToUint256() == b.ToUint256(); 93 | + } 94 | + 95 | + friend bool operator<(const GenTxid& a, const GenTxid& b) { 96 | + return std::tuple(a.index(), a.ToUint256()) < std::tuple(b.index(), b.ToUint256()); 97 | + }
nit: can also be implemented as a single three-way operator:
friend auto operator<=>(const GenTxid& a, const GenTxid& b) {
return std::tuple(a.index(), a.ToUint256()) <=> std::tuple(b.index(), b.ToUint256());
}
(needs to include <compare> and <tuple>)
done
655 | - return (mapTx.count(gtxid.GetHash()) != 0); 656 | + return std::visit(util::Overloaded{ 657 | + [this](const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return (mapTx.get<index_by_wtxid>().count(wtxid) != 0); }, 658 | + [this](const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return (mapTx.count(txid) != 0); } 659 | + }, gtxid); 660 | }
In 811d2f34a0309a62a69897a15db47214bbfec776:
Isn't it more straightforward to just to have two bool exists(const Txid& txid) and bool exists(const Wtxid& wtxid) overloads?
I gotchu
Concept ACK
I'm not sure if the intermediate commits make things better here. Looking at the final diff seems simple enough.
900 | @@ -893,7 +901,11 @@ CTransactionRef CTxMemPool::get(const uint256& hash) const 901 | TxMempoolInfo CTxMemPool::info(const GenTxid& gtxid) const 902 | { 903 | LOCK(cs); 904 | - indexed_transaction_set::const_iterator i = (gtxid.IsWtxid() ? get_iter_from_wtxid(gtxid.GetHash()) : mapTx.find(gtxid.GetHash())); 905 | + indexed_transaction_set::const_iterator i = std::visit(util::Overloaded{ 906 | + [this](const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return get_iter_from_wtxid(wtxid); }, 907 | + [this](const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return mapTx.find(txid); } 908 | + }, gtxid);
Similarly to my comment about TxMempool::exists(), I think it would be better here to just overload info:
<details> <summary>git diff on ad6d02720c</summary>
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index c0f6ac3f03..c34511d27d 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5821,7 +5821,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
continue;
}
// Not in the mempool anymore? don't bother sending it.
- auto txinfo = m_mempool.info(ToGenTxid(inv).ToVariant());
+ auto txinfo{std::visit([&](const auto& tx) { return m_mempool.info(tx); }, ToGenTxid(inv).ToVariant())};
if (!txinfo.tx) {
continue;
}
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 4153794218..a60c692e84 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -890,17 +890,18 @@ CTransactionRef CTxMemPool::get(const uint256& hash) const
return i->GetSharedTx();
}
-TxMempoolInfo CTxMemPool::info(const GenTxidVariant& gtxid) const
+TxMempoolInfo CTxMemPool::info(const Txid& txid) const
{
LOCK(cs);
- indexed_transaction_set::const_iterator i = std::visit(util::Overloaded{
- [this](const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return get_iter_from_wtxid(wtxid); },
- [this](const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return mapTx.find(txid); }
- }, gtxid);
+ indexed_transaction_set::const_iterator i{mapTx.find(txid)};
+ return i == mapTx.end() ? TxMempoolInfo{} : GetInfo(i);
+}
- if (i == mapTx.end())
- return TxMempoolInfo();
- return GetInfo(i);
+TxMempoolInfo CTxMemPool::info(const Wtxid& wtxid) const
+{
+ LOCK(cs);
+ indexed_transaction_set::const_iterator i{get_iter_from_wtxid(wtxid)};
+ return i == mapTx.end() ? TxMempoolInfo{} : GetInfo(i);
}
TxMempoolInfo CTxMemPool::info_for_relay(const GenTxidVariant& gtxid, uint64_t last_sequence) const
diff --git a/src/txmempool.h b/src/txmempool.h
index f96434a344..5421f029bb 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -663,7 +663,8 @@ public:
AssertLockHeld(cs);
return mapTx.project<0>(mapTx.get<index_by_wtxid>().find(wtxid));
}
- TxMempoolInfo info(const GenTxidVariant& gtxid) const;
+ TxMempoolInfo info(const Txid& txid) const;
+ TxMempoolInfo info(const Wtxid& wtxid) const;
/** Returns info for a transaction if its entry_sequence < last_sequence */
TxMempoolInfo info_for_relay(const GenTxidVariant& gtxid, uint64_t last_sequence) const;
</details>
Thank you for the patch, it helped.
Concept ACK, the new GenTxid allows us to better benefit from the type safety of Txid and Wtxid and this PR takes a clear and structured approach that seems straightforward to review.
Approach-wise, I think I have a slightly different view. I find code generally easier to understand when we use the underlying types {Txid, Wtxid} directly and as such would prefer functions to be overloaded with those types, pushing the handling of variants to the edge as much as possible. This also has the side benefit of gently nudging users of those functions to not using variants if they don't have to. I have left 2 such comments on the first commits, but I suspect it applies to later commits too (did not investigate that in detail yet). This is largely a style preference, and I don't want to block the PRs progress over it if other reviewers disagree.
concept ACK
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task TSan, depends, gui: https://github.com/bitcoin/bitcoin/runs/43405323838</sub>
<sub>LLM reason (✨ experimental): The CI failure is caused by compilation errors due to missing 'util::Overloaded' and 'gtxid.ToVariant' members, indicating code incompatibility or missing definitions.</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>
Concept ACK
Thanks for reviewing @stickies-v! I took your suggestion for mempool's exists, info, and info_for_relay. These are all fairly simple functions where I agree that the separate txid/wtxid overloads make the code clearer.
I looked at txrequest and txdownloadman_impl to see where txid and wtxid could be used internally in place of the variant, but it seems to be a less straightfoward refactor. Maybe AddTxAnnouncement could be a good candidate? If you see something that I'm missing (or isn't as complicated as I might think it is), let me know and I'll give it a go, if not in this PR then in a follow-up.
5773 | @@ -5770,7 +5774,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) 5774 | txinfo.tx->GetWitnessHash().ToUint256() : 5775 | txinfo.tx->GetHash().ToUint256(), 5776 | }; 5777 | - tx_relay->m_tx_inventory_to_send.erase(inv.hash); 5778 | + tx_relay->m_tx_inventory_to_send.erase(Txid::FromUint256(inv.hash));
ea58f45b43fcabef02019c5f8d2b3c79ba3340ca: The hash could be referring to a wtxid here, and erasing would not work for peers supporting wtxid, making the code buggier at this commit (only to be fixed in later commits)? Suggestion:
tx_relay->m_tx_inventory_to_send.erase(peer->m_wtxid_relay ? GenTxidVariant{Wtxid::FromUint256(inv.hash)} : GenTxidVariant{Txid::FromUint256(inv.hash)});
Good catch, fixed. It's eventually changed to ToGenTxid(inv) so I use ToVariant() here and remove that later on.
2173 | + GenTxid gtxid; 2174 | + if (peer.m_wtxid_relay) { 2175 | + gtxid = wtxid; 2176 | + } else { 2177 | + gtxid = txid; 2178 | }
nit: Avoid default-initialized GenTxid:
auto gtxid{peer.m_wtxid_relay ? GenTxid{wtxid} : GenTxid{txid}};
Fixed. I thought there was some reason I chose not to do this from the start, but I forgot.. so yeah I agree this is better.
806 | LOCK(cs); 807 | - indexed_transaction_set::const_iterator j = wtxid ? get_iter_from_wtxid(hashb) : mapTx.find(hashb); 808 | + indexed_transaction_set::const_iterator j = std::visit(util::Overloaded{ 809 | + [this](const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return get_iter_from_wtxid(wtxid); }, 810 | + [this](const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return mapTx.find(txid); } 811 | + }, hashb);
nit in ea58f45b43fcabef02019c5f8d2b3c79ba3340ca:
At first it seems more optimal to keep taking bool wtxid as a function parameter, and assume our GenTxidVariants are of the expected type. The current way gives the impression that we may be mixing Txid with Wtxid for the same peer.
indexed_transaction_set::const_iterator j = wtxid ? get_iter_from_wtxid(std::get<Wtxid>(hashb)) : mapTx.find(std::get<Txid>(hashb));
A strong argument against reverting to that is that std::get still checks the index matches internally, and std::variant has no zero-overhead alternative. So we might as well use std::visit and drop the function parameter as you have done.
Might be worth adding something to the commit message such as:
"Now that we are storing CTxMemPool::CompareDepthAndScore parameters using std::variant we have no portable zero-overhead way of accessing them, so use std::visit and drop bool wtxid in-parameter."
Thanks for reviewing. I don't think I can improve upon your suggested commit message, so hope you don't mind that I took this directly.
1583 | @@ -1583,7 +1584,7 @@ void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler) 1584 | CTransactionRef tx = m_mempool.get(txid); 1585 | 1586 | if (tx != nullptr) { 1587 | - RelayTransaction(txid, tx->GetWitnessHash()); 1588 | + RelayTransaction(Txid::FromUint256(txid), tx->GetWitnessHash());
nit: It's tempting to avoid this change in the final diff through making unbroadcast_txids more strongly typed:
<details><summary>Diff</summary>
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 1246628b75..06b8a5e39f 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1578,13 +1578,13 @@ void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_service
void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler)
{
- std::set<uint256> unbroadcast_txids = m_mempool.GetUnbroadcastTxs();
+ std::set<Txid> unbroadcast_txids = m_mempool.GetUnbroadcastTxs();
for (const auto& txid : unbroadcast_txids) {
CTransactionRef tx = m_mempool.get(txid);
if (tx != nullptr) {
- RelayTransaction(Txid::FromUint256(txid), tx->GetWitnessHash());
+ RelayTransaction(txid, tx->GetWitnessHash());
} else {
m_mempool.RemoveUnbroadcastTx(txid, true);
}
diff --git a/src/node/mempool_persist.cpp b/src/node/mempool_persist.cpp
index 437645f139..488f3ad195 100644
--- a/src/node/mempool_persist.cpp
+++ b/src/node/mempool_persist.cpp
@@ -127,7 +127,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active
}
}
- std::set<uint256> unbroadcast_txids;
+ std::set<Txid> unbroadcast_txids;
file >> unbroadcast_txids;
if (opts.apply_unbroadcast_set) {
unbroadcast = unbroadcast_txids.size();
@@ -152,7 +152,7 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock
std::map<uint256, CAmount> mapDeltas;
std::vector<TxMempoolInfo> vinfo;
- std::set<uint256> unbroadcast_txids;
+ std::set<Txid> unbroadcast_txids;
static Mutex dump_mutex;
LOCK(dump_mutex);
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 625d88d807..f311df2581 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -1083,7 +1083,7 @@ size_t CTxMemPool::DynamicMemoryUsage() const {
return memusage::MallocUsage(sizeof(CTxMemPoolEntry) + 15 * sizeof(void*)) * mapTx.size() + memusage::DynamicUsage(mapNextTx) + memusage::DynamicUsage(mapDeltas) + memusage::DynamicUsage(txns_randomized) + cachedInnerUsage;
}
-void CTxMemPool::RemoveUnbroadcastTx(const uint256& txid, const bool unchecked) {
+void CTxMemPool::RemoveUnbroadcastTx(const Txid& txid, const bool unchecked) {
LOCK(cs);
if (m_unbroadcast_txids.erase(txid))
diff --git a/src/txmempool.h b/src/txmempool.h
index e61ba9d4ca..5175b8bea8 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -411,7 +411,7 @@ private:
/**
* Track locally submitted transactions to periodically retry initial broadcast.
*/
- std::set<uint256> m_unbroadcast_txids GUARDED_BY(cs);
+ std::set<Txid> m_unbroadcast_txids GUARDED_BY(cs);
/**
@@ -678,26 +678,26 @@ public:
size_t DynamicMemoryUsage() const;
/** Adds a transaction to the unbroadcast set */
- void AddUnbroadcastTx(const uint256& txid)
+ void AddUnbroadcastTx(const Txid& txid)
{
LOCK(cs);
// Sanity check the transaction is in the mempool & insert into
// unbroadcast set.
- if (exists(Txid::FromUint256(txid))) m_unbroadcast_txids.insert(txid);
+ if (exists(txid)) m_unbroadcast_txids.insert(txid);
};
/** Removes a transaction from the unbroadcast set */
- void RemoveUnbroadcastTx(const uint256& txid, const bool unchecked = false);
+ void RemoveUnbroadcastTx(const Txid& txid, const bool unchecked = false);
/** Returns transactions in unbroadcast set */
- std::set<uint256> GetUnbroadcastTxs() const
+ std::set<Txid> GetUnbroadcastTxs() const
{
LOCK(cs);
return m_unbroadcast_txids;
}
/** Returns whether a txid is in the unbroadcast set */
- bool IsUnbroadcastTx(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs)
+ bool IsUnbroadcastTx(const Txid& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs)
{
AssertLockHeld(cs);
return m_unbroadcast_txids.count(txid) != 0;
</details>
Edit: Added missing part of diff.
I'm leaving the rest of the mempool for the next (and final) type safety PR. My goal for this PR is to only touch GenTxid instances. I think if I start changing things here, it cascades and I would have to address a lot more of the mempool.
Yeah, makes sense to draw the line somewhere. (Noticed I had accidentally omitted the last 30 lines of the diff, fixed FWIW).
Concept ACK b740b6f9479a2351cd3b66a0f6e53848452b6bf8
5807 | @@ -5810,14 +5808,15 @@ bool PeerManagerImpl::SendMessages(CNode* pto) 5808 | while (!vInvTx.empty() && nRelayedTransactions < broadcast_max) { 5809 | // Fetch the top element from the heap 5810 | std::pop_heap(vInvTx.begin(), vInvTx.end(), compareInvMempoolOrder); 5811 | - std::set<uint256>::iterator it = vInvTx.back(); 5812 | + std::set<GenTxidVariant>::iterator it = vInvTx.back(); 5813 | vInvTx.pop_back(); 5814 | - uint256 hash = *it; 5815 | - CInv inv(peer->m_wtxid_relay ? MSG_WTX : MSG_TX, hash); 5816 | + GenTxidVariant hash = *it;
nit in a105a10d71c266cf7971fc66109c9ac8558e6d60:
Could avoid copy as it will keep the instance alive.
const GenTxidVariant& hash = *it;
We erase the iterator soon after but still use the hash, so making a copy here is correct.
2438 | @@ -2437,7 +2439,9 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic 2439 | continue; 2440 | } 2441 | 2442 | - CTransactionRef tx = FindTxForGetData(*tx_relay, ToGenTxid(inv)); 2443 | + CTransactionRef tx{inv.IsMsgWtx() ? 2444 | + FindTxForGetData(*tx_relay, Wtxid::FromUint256(inv.hash)) : 2445 | + FindTxForGetData(*tx_relay, Txid::FromUint256(inv.hash))};
(nit in ab954a169f07c86f85e292d3f488df0cc066b722: Happy to see Freemasons have embraced Bitcoin. Might be neater to have 32 spaces for a multiple of 4 instead of current 33).
I'll include this with my next push.
425 | @@ -435,7 +426,7 @@ class TxRequestTracker::Impl { 426 | auto it_prev = std::prev(it); 427 | // The next best CANDIDATE_READY, if any, immediately precedes the REQUESTED or CANDIDATE_BEST 428 | // announcement in the ByTxHash index. 429 | - if (it_prev->m_txhash == it->m_txhash && it_prev->GetState() == State::CANDIDATE_READY) { 430 | + if (it_prev->m_txhash.ToUint256() == it->m_txhash.ToUint256() && it_prev->GetState() == State::CANDIDATE_READY) {
So for segwit peers we use Wtxid and for non-segwit peers we use Txid?
Scary that the variant fails comparison although the hash is the same, so we have to manually find places where it shouldn't matter and do this kind of thing. :\
Might be easier to ACK this PR if you peel off txrequest.cpp/h into it's own PR.
Yeah the inv should match the peer's wtxid relay setting. The announcements are converted to GenTxids in ProcessMessage and that's what's passed into AddTxAnnouncement and used throughout txdownloadman_impl and txrequest.
I would prefer to keep it all in this PR, as I don't want both the old GenTxid class and the new variant in master. But I agree it's a bit iffy to have to manually ensure that we didn't miss a place where a previous uint256 comparison is now suddenly a GenTxid comparison. The patch I provide at the top should catch anywhere that I might've missed.
Yes, with this PR's change, we would have be intentional about when we're comparing GenTxid objects (type first, then hash) and when we're comparing only the underlying hash. Another option would be to make the patch an actual part of the code, deleting the GenTxid comparators and having custom comparators wherever GenTxids are involved. This felt like overkill to me though, and I think one of the goals of this type safety refactor is to, from now on, differentiate between the transaction identifiers and the underlying transaction hash. In that case, it makes sense to me to have a standard way to compare GenTxids and then make the conversion to uint256 explicit if comparison of the hash is needed.
Had a closer look at TxRequestTracker.
I support replacing uint256 Announcement::m_txhash+bool Announcement::m_is_wtxid with a GenTxId, but would prefer renaming the field just to force out any issues.
On master the interface seems to deliberately use either GenTxId or uint256 depending on what we want to match on:
https://github.com/bitcoin/bitcoin/blob/1be688f575151109816fa8956d54a5b5220e3b00/src/txrequest.h#L135-L178
All of the below are changed in the current version of this PR to use GenTxId instead of uint256, but then only end up using the uint256-part internally:
ForgetTxHash()RequestedTx()ReceivedResponse()GetCandidatePeers()ComputePriority()It feels safer to at least drop those kind of changes on principle, even if they currently do not introduce bugs. We don't want to accidentally introduce cases where a Wtxid of a transaction without any witness data does not match a Txid with the same exact hash.
Took this suggestion. After staring at txrequest some more, I agree that it makes more sense conceptually to keep functions that are only used to look up announcements as uint256. You're right that there is already a distinction made on master between a GenTxid and a txhash so I don't see a reason to change that in this PR (other than making the actual Announcement type safe).
Thanks for taking this suggestion, including renaming the field!
Code review d5a6e7a0a40bb78c2fcadd2662f2d64b86c374b7
Thanks for incorporating most of my feedback!
Mostly focused on reviewing non-test code.
444 | + GenTxidVariant{Txid::FromUint256(m_hash)}; 445 | + } 446 | + 447 | + static GenTxid FromVariant(const GenTxidVariant& variant) { 448 | + return GenTxid{ 449 | + variant.index() == 1, // true if Wtxid, false if Txid
in commit 715b81ef23c9944386d7186fe4590cf53e3df455: nit: avoids the magic number and is more self-explanatory
std::holds_alternative<::Wtxid>(variant),
(but no big deal, especially since this code is deleted in the last commit anyways)
Done.
220 | @@ -220,11 +221,11 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, 221 | // MAX_PEER_TX_REQUEST_IN_FLIGHT requests in flight (and don't have NetPermissionFlags::Relay). 222 | auto delay{0us}; 223 | if (!info.m_preferred) delay += NONPREF_PEER_TX_DELAY; 224 | - if (!gtxid.IsWtxid() && m_num_wtxid_peers > 0) delay += TXID_RELAY_DELAY; 225 | + if (!std::holds_alternative<Wtxid>(gtxid) && m_num_wtxid_peers > 0) delay += TXID_RELAY_DELAY;
I wonder if it's worth it to retain the .IsWtxid() interface in the variant-based GenTxid class for better readability and shorter code at the call-sites? Should be as simple as e.g.
class GenTxid : public std::variant<Txid, Wtxid>
{
public:
.....
bool IsWtxid() const { return std::holds_alternative<Wtxid>(*this); }
.....
}
Done, agreed that that's better. I also changed the comparator in the new class to use IsWtxid() over index().
86 | +public: 87 | + using variant::variant; 88 | + 89 | + bool IsWtxid() const { return std::holds_alternative<Wtxid>(*this); } 90 | + 91 | + const uint256& ToUint256() const LIFETIMEBOUND {
nit: Braces should be on newline for functions as per developer-notes.md, here and for the next one:
const uint256& ToUint256() const LIFETIMEBOUND
{
79 | @@ -76,4 +80,21 @@ using Txid = transaction_identifier<false>; 80 | /** Wtxid commits to all transaction fields including the witness. */ 81 | using Wtxid = transaction_identifier<true>; 82 | 83 | +/** A generic txid reference (txid or wtxid). */
nit: this docstring imo doesn't add anything useful. Would either remove or beef up (e.g. explaining when (not) to use this).
5446 | @@ -5443,19 +5447,17 @@ namespace { 5447 | class CompareInvMempoolOrder 5448 | { 5449 | CTxMemPool* mp; 5450 | - bool m_wtxid_relay; 5451 | public: 5452 | - explicit CompareInvMempoolOrder(CTxMemPool *_mempool, bool use_wtxid) 5453 | + explicit CompareInvMempoolOrder(CTxMemPool *_mempool)
clang-tidy nit:
explicit CompareInvMempoolOrder(CTxMemPool* _mempool)
436 | @@ -437,6 +437,19 @@ class GenTxid 437 | const uint256& GetHash() const LIFETIMEBOUND { return m_hash; } 438 | friend bool operator==(const GenTxid& a, const GenTxid& b) { return a.m_is_wtxid == b.m_is_wtxid && a.m_hash == b.m_hash; } 439 | friend bool operator<(const GenTxid& a, const GenTxid& b) { return std::tie(a.m_is_wtxid, a.m_hash) < std::tie(b.m_is_wtxid, b.m_hash); } 440 | + 441 | + GenTxidVariant ToVariant() const {
clang-tidy nit for 5b5abf88de7975babe2264456076f85f1c9f11a0:
<details> <summary>git diff on a6f1af5f55</summary>
diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
index 54c2fb578f..adadb2a3e7 100644
--- a/src/primitives/transaction.h
+++ b/src/primitives/transaction.h
@@ -438,17 +438,18 @@ public:
friend bool operator==(const GenTxid& a, const GenTxid& b) { return a.m_is_wtxid == b.m_is_wtxid && a.m_hash == b.m_hash; }
friend bool operator<(const GenTxid& a, const GenTxid& b) { return std::tie(a.m_is_wtxid, a.m_hash) < std::tie(b.m_is_wtxid, b.m_hash); }
- GenTxidVariant ToVariant() const {
+ GenTxidVariant ToVariant() const
+ {
return m_is_wtxid ?
- GenTxidVariant{Wtxid::FromUint256(m_hash)} :
- GenTxidVariant{Txid::FromUint256(m_hash)};
+ GenTxidVariant{Wtxid::FromUint256(m_hash)} :
+ GenTxidVariant{Txid::FromUint256(m_hash)};
}
- static GenTxid FromVariant(const GenTxidVariant& variant) {
+ static GenTxid FromVariant(const GenTxidVariant& variant)
+ {
return GenTxid{
std::holds_alternative<::Wtxid>(variant),
- variant.ToUint256()
- };
+ variant.ToUint256()};
}
};
diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
index 4137dd7040..39104a1a5c 100644
--- a/src/util/transaction_identifier.h
+++ b/src/util/transaction_identifier.h
@@ -88,11 +88,13 @@ public:
bool IsWtxid() const { return std::holds_alternative<Wtxid>(*this); }
- const uint256& ToUint256() const LIFETIMEBOUND {
+ const uint256& ToUint256() const LIFETIMEBOUND
+ {
return std::visit([](const auto& id) -> const uint256& { return id.ToUint256(); }, *this);
}
- friend auto operator<=>(const GenTxidVariant& a, const GenTxidVariant& b) {
+ friend auto operator<=>(const GenTxidVariant& a, const GenTxidVariant& b)
+ {
return std::tuple(a.IsWtxid(), a.ToUint256()) <=> std::tuple(b.IsWtxid(), b.ToUint256());
}
};
</details>
2438 | @@ -2437,7 +2439,9 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic 2439 | continue; 2440 | } 2441 | 2442 | - CTransactionRef tx = FindTxForGetData(*tx_relay, ToGenTxid(inv)); 2443 | + CTransactionRef tx{inv.IsMsgWtx() ? 2444 | + FindTxForGetData(*tx_relay, Wtxid::FromUint256(inv.hash)) :
clang-tidy nit for da9927139b0f4583c9245aa3b29185612f94cdc7:
<details> <summary>git diff on 07822f5386</summary>
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index df66d4c93d..a0ed1e2620 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2440,8 +2440,8 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
}
CTransactionRef tx{inv.IsMsgWtx() ?
- FindTxForGetData(*tx_relay, Wtxid::FromUint256(inv.hash)) :
- FindTxForGetData(*tx_relay, Txid::FromUint256(inv.hash))};
+ FindTxForGetData(*tx_relay, Wtxid::FromUint256(inv.hash)) :
+ FindTxForGetData(*tx_relay, Txid::FromUint256(inv.hash))};
if (tx) {
// WTX and WITNESS_TX imply we serialize with witness
const auto maybe_with_witness = (inv.IsMsgTx() ? TX_NO_WITNESS : TX_WITH_WITNESS);
diff --git a/src/validation.cpp b/src/validation.cpp
index 9d788b50f6..c0bc5f28fc 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1135,8 +1135,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
AssertLockHeld(m_pool.cs);
// CheckPackageLimits expects the package transactions to not already be in the mempool.
- assert(std::all_of(txns.cbegin(), txns.cend(), [this](const auto& tx)
- { return !m_pool.exists(tx->GetHash());}));
+ assert(std::all_of(txns.cbegin(), txns.cend(), [this](const auto& tx) { return !m_pool.exists(tx->GetHash()); }));
assert(txns.size() == workspaces.size());
@@ -1346,8 +1345,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
AssertLockHeld(m_pool.cs);
// Sanity check: none of the transactions should be in the mempool, and none of the transactions
// should have a same-txid-different-witness equivalent in the mempool.
- assert(std::all_of(workspaces.cbegin(), workspaces.cend(), [this](const auto& ws){
- return !m_pool.exists(ws.m_ptx->GetHash()); }));
+ assert(std::all_of(workspaces.cbegin(), workspaces.cend(), [this](const auto& ws) { return !m_pool.exists(ws.m_ptx->GetHash()); }));
bool all_submitted = true;
FinalizeSubpackage(args);
</details>
127 | +bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxidVariant& gtxid, bool include_reconsiderable) 128 | { 129 | - const uint256& hash = gtxid.GetHash(); 130 | - 131 | - if (gtxid.IsWtxid()) { 132 | + bool in_orphanage = std::visit(util::Overloaded{
clang-tidy nit in 151ab0e50bf37dae4631ba0b7e6bdccf788cbc11:
<details> <summary>git diff on 44c6c811ba</summary>
diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
index c169c3bf41..054c8fb7dd 100644
--- a/src/node/txdownloadman_impl.cpp
+++ b/src/node/txdownloadman_impl.cpp
@@ -126,21 +126,21 @@ void TxDownloadManagerImpl::BlockDisconnected()
bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxidVariant& gtxid, bool include_reconsiderable)
{
bool in_orphanage = std::visit(util::Overloaded{
- // Normal query by wtxid.
- [this](const Wtxid& wtxid) { return m_orphanage.HaveTx(wtxid); },
- // Never query by txid: it is possible that the transaction in the orphanage has the same
- // txid but a different witness, which would give us a false positive result. If we decided
- // not to request the transaction based on this result, an attacker could prevent us from
- // downloading a transaction by intentionally creating a malleated version of it. While
- // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
- // discerning which one that is, so the orphanage can store multiple transactions with the
- // same txid.
- //
- // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
- // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
- // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
- [this](const Txid& txid) { return m_orphanage.HaveTx(Wtxid::FromUint256(txid.ToUint256())); }
- }, gtxid);
+ // Normal query by wtxid.
+ [this](const Wtxid& wtxid) { return m_orphanage.HaveTx(wtxid); },
+ // Never query by txid: it is possible that the transaction in the orphanage has the same
+ // txid but a different witness, which would give us a false positive result. If we decided
+ // not to request the transaction based on this result, an attacker could prevent us from
+ // downloading a transaction by intentionally creating a malleated version of it. While
+ // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
+ // discerning which one that is, so the orphanage can store multiple transactions with the
+ // same txid.
+ //
+ // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
+ // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
+ // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
+ [this](const Txid& txid) { return m_orphanage.HaveTx(Wtxid::FromUint256(txid.ToUint256())); }},
+ gtxid);
if (in_orphanage) return true;
@@ -183,7 +183,7 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxidVariant&
if (const auto* wtxid = std::get_if<Wtxid>(>xid)) {
if (auto orphan_tx{m_orphanage.GetTx(*wtxid)}) {
auto unique_parents{GetUniqueParents(*orphan_tx)};
- std::erase_if(unique_parents, [&](const auto& txid){
+ std::erase_if(unique_parents, [&](const auto& txid) {
return AlreadyHaveTx(txid, /*include_reconsiderable=*/false);
});
@@ -398,7 +398,7 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction
// Filter parents that we already have.
// Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been
// previously rejected for being too low feerate. This orphan might CPFP it.
- std::erase_if(unique_parents, [&](const auto& txid){
+ std::erase_if(unique_parents, [&](const auto& txid) {
return AlreadyHaveTx(txid, /*include_reconsiderable=*/false);
});
const auto now{GetTime<std::chrono::microseconds>()};
diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp
index bdb38242f6..f2fb186b8e 100644
--- a/src/test/fuzz/txdownloadman.cpp
+++ b/src/test/fuzz/txdownloadman.cpp
@@ -263,8 +263,7 @@ FUZZ_TARGET(txdownloadman, .init = initialize)
// returned true.
Assert(expect_work);
}
- }
- );
+ });
// Jump forwards or backwards
auto time_skip = fuzzed_data_provider.PickValueInArray(TIME_SKIPS);
if (fuzzed_data_provider.ConsumeBool()) time_skip *= -1;
@@ -438,8 +437,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize)
state_missing_inputs.Invalid(TxValidationResult::TX_MISSING_INPUTS, "");
txdownload_impl.MempoolRejectedTx(ptx, state_missing_inputs, rand_peer, fuzzed_data_provider.ConsumeBool());
}
- }
- );
+ });
auto time_skip = fuzzed_data_provider.PickValueInArray(TIME_SKIPS);
if (fuzzed_data_provider.ConsumeBool()) time_skip *= -1;
</details>
278 | + std::vector<std::pair<NodeId, GenTxidVariant>> expired; 279 | auto requestable = m_txrequest.GetRequestable(nodeid, current_time, &expired); 280 | for (const auto& entry : expired) { 281 | LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "wtx" : "tx", 282 | - entry.second.GetHash().ToString(), entry.first); 283 | + entry.second.ToUint256().ToString(), entry.first);
clang-tidy nit in 6c2ba583d5dde911b4cf32edf5baf5acdb9824fb:
<details> <summary>git diff on 50c04bce87</summary>
diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
index 8db30d21d1..a514ac8143 100644
--- a/src/node/txdownloadman_impl.cpp
+++ b/src/node/txdownloadman_impl.cpp
@@ -275,12 +275,12 @@ std::vector<GenTxidVariant> TxDownloadManagerImpl::GetRequestsToSend(NodeId node
auto requestable = m_txrequest.GetRequestable(nodeid, current_time, &expired);
for (const auto& entry : expired) {
LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "wtx" : "tx",
- entry.second.ToUint256().ToString(), entry.first);
+ entry.second.ToUint256().ToString(), entry.first);
}
for (const GenTxidVariant& gtxid : requestable) {
if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) {
LogDebug(BCLog::NET, "Requesting %s %s peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx",
- gtxid.ToUint256().ToString(), nodeid);
+ gtxid.ToUint256().ToString(), nodeid);
requests.emplace_back(gtxid);
m_txrequest.RequestedTx(nodeid, gtxid, current_time + GETDATA_TX_INTERVAL);
} else {
diff --git a/src/test/txrequest_tests.cpp b/src/test/txrequest_tests.cpp
index 298586db37..e6b3e8bc15 100644
--- a/src/test/txrequest_tests.cpp
+++ b/src/test/txrequest_tests.cpp
@@ -161,17 +161,18 @@ public:
* backwards (but note that the ordering of this event only follows the scenario's m_now.
*/
void Check(NodeId peer, const std::vector<GenTxidVariant>& expected, size_t candidates, size_t inflight,
- size_t completed, const std::string& checkname,
- std::chrono::microseconds offset = std::chrono::microseconds{0})
+ size_t completed, const std::string& checkname,
+ std::chrono::microseconds offset = std::chrono::microseconds{0})
{
const auto comment = m_testname + " " + checkname;
auto& runner = m_runner;
const auto now = m_now;
assert(offset.count() <= 0);
- runner.actions.emplace_back(m_now, [=,&runner]() {
+ runner.actions.emplace_back(m_now, [=, &runner]() {
std::vector<std::pair<NodeId, GenTxidVariant>> expired_now;
auto ret = runner.txrequest.GetRequestable(peer, now + offset, &expired_now);
- for (const auto& entry : expired_now) runner.expired.insert(entry);
+ for (const auto& entry : expired_now)
+ runner.expired.insert(entry);
runner.txrequest.SanityCheck();
runner.txrequest.PostGetRequestableSanityCheck(now + offset);
size_t total = candidates + inflight + completed;
@@ -193,7 +194,7 @@ public:
{
const auto& testname = m_testname;
auto& runner = m_runner;
- runner.actions.emplace_back(m_now, [=,&runner]() {
+ runner.actions.emplace_back(m_now, [=, &runner]() {
auto it = runner.expired.find(std::pair<NodeId, GenTxidVariant>{peer, gtxid});
BOOST_CHECK_MESSAGE(it != runner.expired.end(), "[" + testname + "] missing expiration");
if (it != runner.expired.end()) runner.expired.erase(it);
diff --git a/src/txrequest.cpp b/src/txrequest.cpp
index ec2ebe978b..0d0e7543d1 100644
--- a/src/txrequest.cpp
+++ b/src/txrequest.cpp
@@ -575,7 +575,7 @@ public:
}
void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred,
- std::chrono::microseconds reqtime)
+ std::chrono::microseconds reqtime)
{
// Bail out if we already have a CANDIDATE_BEST announcement for this (txhash, peer) combination. The case
// where there is a non-CANDIDATE_BEST announcement already will be caught by the uniqueness property of the
@@ -595,7 +595,7 @@ public:
//! Find the GenTxids to request now from peer.
std::vector<GenTxidVariant> GetRequestable(NodeId peer, std::chrono::microseconds now,
- std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
+ std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
{
// Move time.
SetTimePoint(now, expired);
@@ -730,7 +730,7 @@ void TxRequestTracker::PostGetRequestableSanityCheck(std::chrono::microseconds n
}
void TxRequestTracker::ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred,
- std::chrono::microseconds reqtime)
+ std::chrono::microseconds reqtime)
{
m_impl->ReceivedInv(peer, gtxid, preferred, reqtime);
}
@@ -746,7 +746,7 @@ void TxRequestTracker::ReceivedResponse(NodeId peer, const GenTxidVariant& txhas
}
std::vector<GenTxidVariant> TxRequestTracker::GetRequestable(NodeId peer, std::chrono::microseconds now,
- std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
+ std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
{
return m_impl->GetRequestable(peer, now, expired);
}
diff --git a/src/txrequest.h b/src/txrequest.h
index f32fb1f2b9..3f2acef3a3 100644
--- a/src/txrequest.h
+++ b/src/txrequest.h
@@ -133,7 +133,7 @@ public:
* from the specified gtxid.
*/
void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred,
- std::chrono::microseconds reqtime);
+ std::chrono::microseconds reqtime);
/** Deletes all announcements for a given peer.
*
@@ -164,7 +164,7 @@ public:
* simultaneously by one peer, and end up being requested from them, the requests will happen in announcement order.
*/
std::vector<GenTxidVariant> GetRequestable(NodeId peer, std::chrono::microseconds now,
- std::vector<std::pair<NodeId, GenTxidVariant>>* expired = nullptr);
+ std::vector<std::pair<NodeId, GenTxidVariant>>* expired = nullptr);
/** Marks a transaction as requested, with a specified expiry.
*
</details>
806 | LOCK(cs); 807 | - indexed_transaction_set::const_iterator j = wtxid ? get_iter_from_wtxid(hashb) : mapTx.find(hashb); 808 | + indexed_transaction_set::const_iterator j = std::visit(util::Overloaded{ 809 | + [this](const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return get_iter_from_wtxid(wtxid); }, 810 | + [this](const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return mapTx.find(txid); } 811 | + }, hashb);
In 3e378fd6594d1811773153e14b9e34cb538ac66e:
These visitors can be cleaned up quite a bit by first replacing get_iter_from_wtxid with a new GetIter(const Wtxid&) overload (see https://github.com/stickies-v/bitcoin/commit/2585f12836bd5ef599c75e338066e2a13d6e58f7 and https://github.com/stickies-v/bitcoin/commit/589357b193b98d15b6db8db48f521ab193fc0136). This code block then becomes quite a bit more concise. It increases the diff a bit in other places, but part of that is from increased type safety. Overall, I think it's sensible to make this change in this PR?
auto j{std::visit([&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(cs) { return GetIter(id); }, hashb)};
if (!j.has_value()) return false;
auto i{std::visit([&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(cs) { return GetIter(id); }, hasha)};
if (!i.has_value()) return true;
uint64_t counta = i.value()->GetCountWithAncestors();
uint64_t countb = j.value()->GetCountWithAncestors();
See the diff here (from branch https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2025-06/overload-getiter)
Used your commits, thanks. I agree it's a lot cleaner and a minimal change so it makes sense as part of this refactor. Also, making GetIter type safe was going to happen in the next PR anyway.
910 | + indexed_transaction_set::const_iterator i{mapTx.find(txid)}; 911 | + return i == mapTx.end() ? TxMempoolInfo{} : GetInfo(i); 912 | } 913 | 914 | -TxMempoolInfo CTxMemPool::info_for_relay(const GenTxid& gtxid, uint64_t last_sequence) const 915 | +TxMempoolInfo CTxMemPool::info(const Wtxid& wtxid) const
In da9927139b0f4583c9245aa3b29185612f94cdc7:
With my other suggestion to implement GetIter(const Wtxid&), the info and info_for_relay duplication can be avoided, because they can now be trivially templated (moving them into txmempool.h). It does also require moving GetInfo to the header (see https://github.com/stickies-v/bitcoin/commit/8d62f6eb0c279cda82dcb5fa867ba8ae5d39fa97), but I think that's okay (ideally that function would just be replaced with a TxMempoolInfo(txiter) ctor but that's a much bigger diff).
These two functions can then be implemented as
template <typename T>
requires std::is_same_v<T, Txid> || std::is_same_v<T, Wtxid>
TxMempoolInfo info(const T& txid) const
{
LOCK(cs);
auto i{GetIter(txid)};
return i.has_value() ? GetInfo(*i) : TxMempoolInfo{};
}
/** Returns info for a transaction if its entry_sequence < last_sequence */
template <typename T>
requires std::is_same_v<T, Txid> || std::is_same_v<T, Wtxid>
TxMempoolInfo info_for_relay(const T& txid, uint64_t last_sequence) const
{
LOCK(cs);
auto i{GetIter(txid)};
return (i.has_value() && i.value()->GetSequence() < last_sequence) ? GetInfo(*i) : TxMempoolInfo{};
}
See the diff here as part of branch https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2025-06/overload-getiter
Took this suggestion too and used concepts for the templates. Definitely better to not have to repeat these two two functions.
I think the changes here can be simplified quite a bit by first replacing get_iter_from_wtxid with a GetIter(const wtxid&) overload, see my suggestions here and here (or full branch https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2025-06/overload-getiter).
There are also quite a few clang-tidy violations that would be helpful to address here, left per-commit comments.
Reimplements the GenTxid class as a variant for better type safety.
Also adds two temporary functions to the old GenTxid class that
convert to and from the new variant.
This allows adding a GetIter(const Wtxid&) overload in a next
commit, making it easier to visit this function from a variant.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Overloading GetIter makes it easier to use std::visit patterns
from a GenTxid.
Thanks for reviewing @hodlinator and @stickies-v! Pretty sure I addressed all comments, but let me know if I missed anything in the most recent push (very possible I did). After taking your suggestions, it's looking better and cleaner now for sure. The txrequest changes should be quite a bit easier to review now as well.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/runs/45122218183</sub>
<sub>LLM reason (✨ experimental): The CI failure is caused by a lint error in the script 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>
5446 | @@ -5443,19 +5447,17 @@ namespace { 5447 | class CompareInvMempoolOrder 5448 | { 5449 | CTxMemPool* mp; 5450 | - bool m_wtxid_relay; 5451 | public: 5452 | - explicit CompareInvMempoolOrder(CTxMemPool *_mempool, bool use_wtxid) 5453 | + explicit CompareInvMempoolOrder(CTxMemPool* _mempool)
nit in e5a1ccd5d6e83d1027e3d2516ffd36d0e88291c4: this ctor can be simplified with member initialization lists. And while these lines are changed anyway, it might be good to add some constness guarantees and modernize naming.
Complete diff would be:
<details> <summary>git diff on e5a1ccd5d6</summary>
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 7a97071052..f390b0f944 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5442,18 +5442,15 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi
namespace {
class CompareInvMempoolOrder
{
- CTxMemPool* mp;
+ const CTxMemPool* m_mempool;
public:
- explicit CompareInvMempoolOrder(CTxMemPool* _mempool)
- {
- mp = _mempool;
- }
+ explicit CompareInvMempoolOrder(CTxMemPool* mempool) : m_mempool{mempool} {}
bool operator()(std::set<GenTxidVariant>::iterator a, std::set<GenTxidVariant>::iterator b)
{
/* As std::make_heap produces a max-heap, we want the entries with the
* fewest ancestors/highest fee to sort later. */
- return mp->CompareDepthAndScore(*b, *a);
+ return m_mempool->CompareDepthAndScore(*b, *a);
}
};
} // namespace
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 819e7e0550..e3660d9aea 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -794,7 +794,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
assert(innerUsage == cachedInnerUsage);
}
-bool CTxMemPool::CompareDepthAndScore(const GenTxidVariant& hasha, const GenTxidVariant& hashb)
+bool CTxMemPool::CompareDepthAndScore(const GenTxidVariant& hasha, const GenTxidVariant& hashb) const
{
/* Return `true` if hasha should be considered sooner than hashb. Namely when:
* a is not in the mempool, but b is
diff --git a/src/txmempool.h b/src/txmempool.h
index 205cfa346b..ca1d1e870e 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -467,7 +467,7 @@ public:
void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs);
- bool CompareDepthAndScore(const GenTxidVariant& hasha, const GenTxidVariant& hashb);
+ bool CompareDepthAndScore(const GenTxidVariant& hasha, const GenTxidVariant& hashb) const;
bool isSpent(const COutPoint& outpoint) const;
unsigned int GetTransactionsUpdated() const;
void AddTransactionsUpdated(unsigned int n);
</details>
843 | @@ -844,10 +844,6 @@ std::vector<CTxMemPool::indexed_transaction_set::const_iterator> CTxMemPool::Get 844 | return iters; 845 | } 846 | 847 | -static TxMempoolInfo GetInfo(CTxMemPool::indexed_transaction_set::const_iterator it) {
nit in 472849c747334f52cb87bf13b9ee6d23e24d3494:
I think keeping this (see https://github.com/stickies-v/bitcoin/commit/8d62f6eb0c279cda82dcb5fa867ba8ae5d39fa97) in a separate move-only commit is a bit easier for review.
80 | @@ -76,4 +81,25 @@ using Txid = transaction_identifier<false>; 81 | /** Wtxid commits to all transaction fields including the witness. */ 82 | using Wtxid = transaction_identifier<true>; 83 | 84 | +template <typename T> 85 | +concept TransactionIdentifier = std::is_same_v<T, Txid> || std::is_same_v<T, Wtxid>;
nit: TransactionIdentifier could be interpreted as the unabbreviated Txid. Perhaps TxidOrWtxid is both more clear and shorter? I realize all naming approaches here are probably going to kinda suck in some way, especially as we already have GenTxid as a variant too. Don't want to bikeshed, disregard if you don't think it's an improvement.
Done. I didn't really know the best name for this initially. I like TxidOrWtxid a bit better.
2438 | @@ -2437,7 +2439,9 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic 2439 | continue; 2440 | } 2441 | 2442 | - CTransactionRef tx = FindTxForGetData(*tx_relay, ToGenTxid(inv)); 2443 | + CTransactionRef tx{inv.IsMsgWtx() ? 2444 | + FindTxForGetData(*tx_relay, Wtxid::FromUint256(inv.hash)) : 2445 | + FindTxForGetData(*tx_relay, Txid::FromUint256(inv.hash))};
in 472849c747334f52cb87bf13b9ee6d23e24d3494: this looks like it still just should be using ToGenTxid? Using the visitor pattern here makes things quite verbose wrt thread safety annotations, but then that made me realize - perhaps FindTxForGetData should just take an inv? It's a net_processing function, it seems like an intuitive fit and avoids duplication.
<details> <summary>git diff on 472849c747</summary>
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 785f6d799a..128a188d00 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -947,8 +947,7 @@ private:
std::atomic<std::chrono::seconds> m_last_tip_update{0s};
/** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */
- template <TransactionIdentifier T>
- CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const T& txid)
+ CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const CInv& inv)
EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, NetEventsInterface::g_msgproc_mutex);
void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc)
@@ -2392,11 +2391,15 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
}
}
-template <TransactionIdentifier T>
-CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const T& txid)
+CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const CInv& inv)
{
+ auto gtxid{ToGenTxid(inv).ToVariant()};
// If a tx was in the mempool prior to the last INV for this peer, permit the request.
- auto txinfo = m_mempool.info_for_relay(txid, tx_relay.m_last_inv_sequence);
+ auto txinfo{std::visit(
+ [&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) {
+ return m_mempool.info_for_relay(id, tx_relay.m_last_inv_sequence);
+ },
+ gtxid)};
if (txinfo.tx) {
return std::move(txinfo.tx);
}
@@ -2405,7 +2408,7 @@ CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay,
{
LOCK(m_most_recent_block_mutex);
if (m_most_recent_block_txs != nullptr) {
- auto it = m_most_recent_block_txs->find(txid.ToUint256());
+ auto it = m_most_recent_block_txs->find(gtxid.ToUint256());
if (it != m_most_recent_block_txs->end()) return it->second;
}
}
@@ -2439,10 +2442,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
continue;
}
- CTransactionRef tx{inv.IsMsgWtx() ?
- FindTxForGetData(*tx_relay, Wtxid::FromUint256(inv.hash)) :
- FindTxForGetData(*tx_relay, Txid::FromUint256(inv.hash))};
- if (tx) {
+ if (auto tx{FindTxForGetData(*tx_relay, inv)}) {
// WTX and WITNESS_TX imply we serialize with witness
const auto maybe_with_witness = (inv.IsMsgTx() ? TX_NO_WITNESS : TX_WITH_WITNESS);
MakeAndPushMessage(pfrom, NetMsgType::TX, maybe_with_witness(*tx));
</details>
Took this suggestion 🤝
672 | @@ -673,7 +673,7 @@ class ChainImpl : public Chain 673 | { 674 | if (!m_node.mempool) return false; 675 | LOCK(m_node.mempool->cs); 676 | - return m_node.mempool->exists(GenTxid::Txid(txid)); 677 | + return m_node.mempool->exists(Txid::FromUint256(txid));
nit on 472849c747: isInMempool can be changed to take a Txid without any further code changes and avoids the conversion and having to touch it again in the future.
<details> <summary>git diff on 472849c747</summary>
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index f6e831624d..511ba41568 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -208,7 +208,7 @@ public:
virtual RBFTransactionState isRBFOptIn(const CTransaction& tx) = 0;
//! Check if transaction is in mempool.
- virtual bool isInMempool(const uint256& txid) = 0;
+ virtual bool isInMempool(const Txid& txid) = 0;
//! Check if transaction has descendants in mempool.
virtual bool hasDescendantsInMempool(const uint256& txid) = 0;
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 6bead3ea1e..0b7cea347c 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -669,11 +669,11 @@ public:
LOCK(m_node.mempool->cs);
return IsRBFOptIn(tx, *m_node.mempool);
}
- bool isInMempool(const uint256& txid) override
+ bool isInMempool(const Txid& txid) override
{
if (!m_node.mempool) return false;
LOCK(m_node.mempool->cs);
- return m_node.mempool->exists(Txid::FromUint256(txid));
+ return m_node.mempool->exists(txid);
}
bool hasDescendantsInMempool(const uint256& txid) override
{
</details>
Good looks
143 | - // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will 144 | - // help us find non-segwit transactions, saving bandwidth, and should have no false positives. 145 | - if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; 146 | +bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxidVariant& gtxid, bool include_reconsiderable) 147 | +{ 148 | + if (std::visit(util::Overloaded{
In 7c4eb58fb0:
I don't really see the point of using util::Overloaded here when we're always doing a Wtxid query?
This looks more straightforward to me:
<details> <summary>git diff on 7c4eb58fb0</summary>
diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
index a16490ceb9..87a47b30ee 100644
--- a/src/node/txdownloadman_impl.cpp
+++ b/src/node/txdownloadman_impl.cpp
@@ -9,7 +9,6 @@
#include <consensus/validation.h>
#include <logging.h>
#include <txmempool.h>
-#include <util/overloaded.h>
#include <validation.h>
#include <validationinterface.h>
@@ -125,26 +124,19 @@ void TxDownloadManagerImpl::BlockDisconnected()
bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxidVariant& gtxid, bool include_reconsiderable)
{
- if (std::visit(util::Overloaded{
- // Normal query by wtxid.
- [this](const Wtxid& wtxid) { return m_orphanage.HaveTx(wtxid); },
- // Never query by txid: it is possible that the transaction in the orphanage has the same
- // txid but a different witness, which would give us a false positive result. If we decided
- // not to request the transaction based on this result, an attacker could prevent us from
- // downloading a transaction by intentionally creating a malleated version of it. While
- // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
- // discerning which one that is, so the orphanage can store multiple transactions with the
- // same txid.
- //
- // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
- // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
- // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
- [this](const Txid& txid) { return m_orphanage.HaveTx(Wtxid::FromUint256(txid.ToUint256())); }},
- gtxid)) {
- return true;
- }
-
const uint256& hash = gtxid.ToUint256();
+ // Never query by txid: it is possible that the transaction in the orphanage has the same
+ // txid but a different witness, which would give us a false positive result. If we decided
+ // not to request the transaction based on this result, an attacker could prevent us from
+ // downloading a transaction by intentionally creating a malleated version of it. While
+ // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
+ // discerning which one that is, so the orphanage can store multiple transactions with the
+ // same txid.
+ //
+ // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
+ // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
+ // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
+ if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true;
if (include_reconsiderable && RecentRejectsReconsiderableFilter().contains(hash)) return true;
</details>
I agree that there isn't a need for branching on wtxid/txid and that having the comment there is enough. Just to be sure, @glozow if you prefer to keep the branching, let me know and I can revert.
edit: I'm also open to making the comment clearer if we opt for no branch.
174 | + runner.actions.emplace_back(m_now, [=, &runner]() { 175 | std::vector<std::pair<NodeId, GenTxid>> expired_now; 176 | auto ret = runner.txrequest.GetRequestable(peer, now + offset, &expired_now); 177 | - for (const auto& entry : expired_now) runner.expired.insert(entry); 178 | + for (const auto& entry : expired_now) 179 | + runner.expired.insert(entry);
nit in db125fed333a74999a63ac0e8d8b038cab96a3f4: multiline statements require braces as per developer notes, so would revert this change (or add braces)
274 | @@ -274,29 +275,29 @@ std::vector<GenTxid> TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std 275 | auto requestable = m_txrequest.GetRequestable(nodeid, current_time, &expired); 276 | for (const auto& entry : expired) { 277 | LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "wtx" : "tx", 278 | - entry.second.GetHash().ToString(), entry.first); 279 | + entry.second.ToUint256().ToString(), entry.first);
nit in db125fed333a74999a63ac0e8d8b038cab96a3f4:
could consider modernizing with structured bindings while touching this:
<details> <summary>git diff on db125fed33</summary>
diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
index 7116c433be..32f3ae7931 100644
--- a/src/node/txdownloadman_impl.cpp
+++ b/src/node/txdownloadman_impl.cpp
@@ -273,9 +273,9 @@ std::vector<GenTxidVariant> TxDownloadManagerImpl::GetRequestsToSend(NodeId node
std::vector<GenTxidVariant> requests;
std::vector<std::pair<NodeId, GenTxidVariant>> expired;
auto requestable = m_txrequest.GetRequestable(nodeid, current_time, &expired);
- for (const auto& entry : expired) {
- LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "wtx" : "tx",
- entry.second.ToUint256().ToString(), entry.first);
+ for (const auto& [node_id, gtxid] : expired) {
+ LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx",
+ gtxid.ToUint256().ToString(), node_id);
}
for (const GenTxidVariant& gtxid : requestable) {
if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) {
</details>
412 | @@ -413,8 +413,8 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction 413 | // 414 | // Search by txid and, if the tx has a witness, wtxid 415 | std::vector<NodeId> orphan_resolution_candidates{nodeid}; 416 | - m_txrequest.GetCandidatePeers(ptx->GetHash(), orphan_resolution_candidates); 417 | - if (ptx->HasWitness()) m_txrequest.GetCandidatePeers(ptx->GetWitnessHash(), orphan_resolution_candidates); 418 | + m_txrequest.GetCandidatePeers(ptx->GetHash().ToUint256(), orphan_resolution_candidates); 419 | + if (ptx->HasWitness()) m_txrequest.GetCandidatePeers(ptx->GetWitnessHash().ToUint256(), orphan_resolution_candidates);
These changes seem unnecessary and unrelated to any code changes made in this PR?
Good catch, same for ForgetTxHash.
519 | @@ -520,8 +520,8 @@ std::pair<bool, std::optional<PackageToValidate>> TxDownloadManagerImpl::Receive 520 | const Wtxid& wtxid = ptx->GetWitnessHash(); 521 | 522 | // Mark that we have received a response 523 | - m_txrequest.ReceivedResponse(nodeid, txid); 524 | - if (ptx->HasWitness()) m_txrequest.ReceivedResponse(nodeid, wtxid); 525 | + m_txrequest.ReceivedResponse(nodeid, txid.ToUint256()); 526 | + if (ptx->HasWitness()) m_txrequest.ReceivedResponse(nodeid, wtxid.ToUint256());
These changes seem unnecessary and unrelated to any code changes made in this PR?
Approach ACK 700819f87b019f854340886dc8d35e66e813b53f
I did a full code review and while the changes are numerous, type-safety or pre-existing usage of GenTxid::{Txid,Wtxid} makes many of them trivially obvious to be a refactor, which in my opinion makes this PR reviewable even without in-depth knowledge of the p2p code this PR touches.
Now that we are storing `CTxMemPool::CompareDepthAndScore` parameters using
`std::variant` we have no portable zero-overhead way of accessing them,
so use `std::visit` and drop `bool wtxid` in-parameter.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
This allows it to be used by templated functions in a future commit.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Convert all of `txdownloadman_impl` to the new variant except for
`GetRequestsToSend`, which will be easier to switch at the same
time as `txrequest`.
Switch all instances of GenTxid to the new variant
in `txrequest` and complete `txdownloadman_impl` by
converting `GetRequestsToSend`.
-BEGIN VERIFY SCRIPT-
sed -i 's/GenTxidVariant/GenTxid/g' $(git grep -l 'GenTxidVariant')
-END VERIFY SCRIPT-
92 | + return std::visit([](const auto& id) -> const uint256& { return id.ToUint256(); }, *this); 93 | + } 94 | + 95 | + friend auto operator<=>(const GenTxidVariant& a, const GenTxidVariant& b) 96 | + { 97 | + return std::tuple(a.IsWtxid(), a.ToUint256()) <=> std::tuple(b.IsWtxid(), b.ToUint256());
nit 11d28f21bb8f0c3094934b3fef45871f73bb216a: This creates a copy, when read-only access is needed. (clang -O2 seems to strip the copy, but gcc may not). You can avoid it via:
return std::tuple<bool,const uint256&>(a.IsWtxid(), a.ToUint256()) <=> std::tuple<bool,const uint256&>(b.IsWtxid(), b.ToUint256());
449 | + { 450 | + return GenTxid{ 451 | + std::holds_alternative<::Wtxid>(variant), 452 | + variant.ToUint256()}; 453 | + } 454 | };
nit in 11d28f21bb8f0c3094934b3fef45871f73bb216a (first commit): Could add a comment to this class, documenting that it will be removed in the last commit?
2390 | @@ -2391,10 +2391,15 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& 2391 | } 2392 | } 2393 | 2394 | -CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const GenTxid& gtxid) 2395 | +CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const CInv& inv)
nit: Imo, function parameters should reflect what a function needs to do its job, as opposed to passing it what you have available at the call sites.
CInv can be more than a tx announcement, and it's only used here to be passed to ToGenTxid (which crashes if its not a tx inv). Distinguishing tx from other announcements is not the job of FindTxForGetData. It only needs a wtxid/txid to do its job and should take a GenTxid (or be templated).
Code review ACK a60f863d3e276534444571282f432b913d3967db
left two nits that can be ignored or followed-up later on.
review ACK a60f863d3e276534444571282f432b913d3967db 🎽
<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 a60f863d3e276534444571282f432b913d3967db 🎽
Y4M+5Nl4FOcgYfClv1qRnd5797LuKUVoDluobop5CMpD3562zy81q/G5Wgo0lri3tMAlaUzCOjdjwFe/zausCw==
</details>
Code-review ACK a60f863d3e276534444571282f432b913d3967db
i get that this is an overall improvement to the type safety, but I think that it's gone about sort of the wrong way since it leaves a lot of data-structures that are supposed to be made homogeneous as hetergeneous still. E.g.,
std::set<GenTxid> m_tx_inventory_to_send;
is supposed to be std::variant<std::set<Txid>, std::set<WTxid>>.
You can create a templated wrapper to encapsulate a generic container interface for this.
168 | + * from dependent transactions being requested out of order: if multiple dependent transactions are announced 169 | + * simultaneously by one peer, and end up being requested from them, the requests will happen in announcement order. 170 | */ 171 | std::vector<GenTxid> GetRequestable(NodeId peer, std::chrono::microseconds now, 172 | - std::vector<std::pair<NodeId, GenTxid>>* expired = nullptr); 173 | + std::vector<std::pair<NodeId, GenTxid>>* expired = nullptr);
nit: There's 7 extra spaces in function prototypes for GetRequestable (this is 1 of 3 occurrences). This is due to changes to the code before the scripted-diff not accounting for a loss of Variant from the previous lines return value.
969 | return std::nullopt; 970 | } 971 | 972 | +std::optional<CTxMemPool::txiter> CTxMemPool::GetIter(const Wtxid& wtxid) const 973 | +{ 974 | + AssertLockHeld(cs);
nit: Missing AssertLockHeld in pre-existing TxId overload?
Light post-merge ACK
Follow ups to these last few comments can be found here: #33005
edit: after I fix CI...