std::variant
#32631
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32631.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | dergoegge, stickies-v, instagibbs, theStack, hodlinator |
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:
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.
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.
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index 011e3cd928..1a47a39dfd 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -206,6 +206,12 @@ struct QueuedBlock {
5 std::unique_ptr<PartiallyDownloadedBlock> partialBlock;
6 };
7
8+struct CompareGenTxid {
9+ bool operator()(const GenTxid& a, const GenTxid& b) const {
10+ return std::tuple(a.index(), a.ToUint256()) < std::tuple(b.index(), b.ToUint256());
11+ }
12+};
13+
14 /**
15 * Data structure for an individual peer. This struct is not protected by
16 * cs_main since it does not contain validation-critical data.
17@@ -302,7 +308,7 @@ struct Peer {
18 * non-wtxid-relay peers, wtxid for wtxid-relay peers). We use the
19 * mempool to sort transactions in dependency order before relay, so
20 * this does not have to be sorted. */
21- std::set<GenTxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);
22+ std::set<GenTxid, CompareGenTxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);
23 /** Whether the peer has requested us to send our complete mempool. Only
24 * permitted if the peer has NetPermissionFlags::Mempool or we advertise
25 * NODE_BLOOM. See BIP35. */
26@@ -856,7 +862,7 @@ private:
27 std::shared_ptr<const CBlock> m_most_recent_block GUARDED_BY(m_most_recent_block_mutex);
28 std::shared_ptr<const CBlockHeaderAndShortTxIDs> m_most_recent_compact_block GUARDED_BY(m_most_recent_block_mutex);
29 uint256 m_most_recent_block_hash GUARDED_BY(m_most_recent_block_mutex);
30- std::unique_ptr<const std::map<GenTxid, CTransactionRef>> m_most_recent_block_txs GUARDED_BY(m_most_recent_block_mutex);
31+ std::unique_ptr<const std::map<GenTxid, CTransactionRef, CompareGenTxid>> m_most_recent_block_txs GUARDED_BY(m_most_recent_block_mutex);
32
33 // Data about the low-work headers synchronization, aggregated from all peers' HeadersSyncStates.
34 /** Mutex guarding the other m_headers_presync_* variables. */
35@@ -2027,7 +2033,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha
36 std::async(std::launch::deferred, [&] { return NetMsg::Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })};
37
38 {
39- auto most_recent_block_txs = std::make_unique<std::map<GenTxid, CTransactionRef>>();
40+ auto most_recent_block_txs = std::make_unique<std::map<GenTxid, CTransactionRef, CompareGenTxid>>();
41 for (const auto& tx : pblock->vtx) {
42 most_recent_block_txs->emplace(tx->GetHash(), tx);
43 most_recent_block_txs->emplace(tx->GetWitnessHash(), tx);
44diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
45index 3cdeb664d4..6ab5d98263 100644
46--- a/src/util/transaction_identifier.h
47+++ b/src/util/transaction_identifier.h
48@@ -88,13 +88,12 @@ public:
49 return std::visit([](const auto& id) -> const uint256& { return id.ToUint256(); }, *this);
50 }
51
52- friend bool operator==(const GenTxid& a, const GenTxid& b) {
53- return a.index() == b.index() && a.ToUint256() == b.ToUint256();
54- }
55-
56- friend bool operator<(const GenTxid& a, const GenTxid& b) {
57- return std::tuple(a.index(), a.ToUint256()) < std::tuple(b.index(), b.ToUint256());
58- }
59+ friend bool operator==(const GenTxid& a, const GenTxid& b) = delete;
60+ friend bool operator!=(const GenTxid& a, const GenTxid& b) = delete;
61+ friend bool operator<(const GenTxid& a, const GenTxid& b) = delete;
62+ friend bool operator<=(const GenTxid& a, const GenTxid& b) = delete;
63+ friend bool operator>(const GenTxid& a, const GenTxid& b) = delete;
64+ friend bool operator>=(const GenTxid& a, const GenTxid& b) = delete;
65 };
66
67 #endif // BITCOIN_UTIL_TRANSACTION_IDENTIFIER_H
68
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+ }
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());
0 Assume(peer->m_wtxid_relay == std::holds_alternative<Wtxid>(hash));
1 CInv inv(peer->m_wtxid_relay ? MSG_WTX : MSG_TX, hash.ToUint256());
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
0 std::optional<Txid> rejected_parent_reconsiderable;
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:
0 friend auto operator<=>(const GenTxid& a, const GenTxid& b) {
1 return std::tuple(a.index(), a.ToUint256()) <=> std::tuple(b.index(), b.ToUint256());
2 }
(needs to include <compare> and <tuple>)
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?
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:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index c0f6ac3f03..c34511d27d 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -5821,7 +5821,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
5 continue;
6 }
7 // Not in the mempool anymore? don't bother sending it.
8- auto txinfo = m_mempool.info(ToGenTxid(inv).ToVariant());
9+ auto txinfo{std::visit([&](const auto& tx) { return m_mempool.info(tx); }, ToGenTxid(inv).ToVariant())};
10 if (!txinfo.tx) {
11 continue;
12 }
13diff --git a/src/txmempool.cpp b/src/txmempool.cpp
14index 4153794218..a60c692e84 100644
15--- a/src/txmempool.cpp
16+++ b/src/txmempool.cpp
17@@ -890,17 +890,18 @@ CTransactionRef CTxMemPool::get(const uint256& hash) const
18 return i->GetSharedTx();
19 }
20
21-TxMempoolInfo CTxMemPool::info(const GenTxidVariant& gtxid) const
22+TxMempoolInfo CTxMemPool::info(const Txid& txid) const
23 {
24 LOCK(cs);
25- indexed_transaction_set::const_iterator i = std::visit(util::Overloaded{
26- [this](const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return get_iter_from_wtxid(wtxid); },
27- [this](const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(cs) { return mapTx.find(txid); }
28- }, gtxid);
29+ indexed_transaction_set::const_iterator i{mapTx.find(txid)};
30+ return i == mapTx.end() ? TxMempoolInfo{} : GetInfo(i);
31+}
32
33- if (i == mapTx.end())
34- return TxMempoolInfo();
35- return GetInfo(i);
36+TxMempoolInfo CTxMemPool::info(const Wtxid& wtxid) const
37+{
38+ LOCK(cs);
39+ indexed_transaction_set::const_iterator i{get_iter_from_wtxid(wtxid)};
40+ return i == mapTx.end() ? TxMempoolInfo{} : GetInfo(i);
41 }
42
43 TxMempoolInfo CTxMemPool::info_for_relay(const GenTxidVariant& gtxid, uint64_t last_sequence) const
44diff --git a/src/txmempool.h b/src/txmempool.h
45index f96434a344..5421f029bb 100644
46--- a/src/txmempool.h
47+++ b/src/txmempool.h
48@@ -663,7 +663,8 @@ public:
49 AssertLockHeld(cs);
50 return mapTx.project<0>(mapTx.get<index_by_wtxid>().find(wtxid));
51 }
52- TxMempoolInfo info(const GenTxidVariant& gtxid) const;
53+ TxMempoolInfo info(const Txid& txid) const;
54+ TxMempoolInfo info(const Wtxid& wtxid) const;
55
56 /** Returns info for a transaction if its entry_sequence < last_sequence */
57 TxMempoolInfo info_for_relay(const GenTxidVariant& gtxid, uint64_t last_sequence) const;
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.
🚧 At least one of the CI tasks failed.
Task TSan, depends, gui: https://github.com/bitcoin/bitcoin/runs/43405323838
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.
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.
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:
0tx_relay->m_tx_inventory_to_send.erase(peer->m_wtxid_relay ? GenTxidVariant{Wtxid::FromUint256(inv.hash)} : GenTxidVariant{Txid::FromUint256(inv.hash)});
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:
0 auto gtxid{peer.m_wtxid_relay ? GenTxid{wtxid} : GenTxid{txid}};
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.
0 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.”
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:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index 1246628b75..06b8a5e39f 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -1578,13 +1578,13 @@ void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_service
5
6 void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler)
7 {
8- std::set<uint256> unbroadcast_txids = m_mempool.GetUnbroadcastTxs();
9+ std::set<Txid> unbroadcast_txids = m_mempool.GetUnbroadcastTxs();
10
11 for (const auto& txid : unbroadcast_txids) {
12 CTransactionRef tx = m_mempool.get(txid);
13
14 if (tx != nullptr) {
15- RelayTransaction(Txid::FromUint256(txid), tx->GetWitnessHash());
16+ RelayTransaction(txid, tx->GetWitnessHash());
17 } else {
18 m_mempool.RemoveUnbroadcastTx(txid, true);
19 }
20diff --git a/src/node/mempool_persist.cpp b/src/node/mempool_persist.cpp
21index 437645f139..488f3ad195 100644
22--- a/src/node/mempool_persist.cpp
23+++ b/src/node/mempool_persist.cpp
24@@ -127,7 +127,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active
25 }
26 }
27
28- std::set<uint256> unbroadcast_txids;
29+ std::set<Txid> unbroadcast_txids;
30 file >> unbroadcast_txids;
31 if (opts.apply_unbroadcast_set) {
32 unbroadcast = unbroadcast_txids.size();
33@@ -152,7 +152,7 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock
34
35 std::map<uint256, CAmount> mapDeltas;
36 std::vector<TxMempoolInfo> vinfo;
37- std::set<uint256> unbroadcast_txids;
38+ std::set<Txid> unbroadcast_txids;
39
40 static Mutex dump_mutex;
41 LOCK(dump_mutex);
42diff --git a/src/txmempool.cpp b/src/txmempool.cpp
43index 625d88d807..f311df2581 100644
44--- a/src/txmempool.cpp
45+++ b/src/txmempool.cpp
46@@ -1083,7 +1083,7 @@ size_t CTxMemPool::DynamicMemoryUsage() const {
47 return memusage::MallocUsage(sizeof(CTxMemPoolEntry) + 15 * sizeof(void*)) * mapTx.size() + memusage::DynamicUsage(mapNextTx) + memusage::DynamicUsage(mapDeltas) + memusage::DynamicUsage(txns_randomized) + cachedInnerUsage;
48 }
49
50-void CTxMemPool::RemoveUnbroadcastTx(const uint256& txid, const bool unchecked) {
51+void CTxMemPool::RemoveUnbroadcastTx(const Txid& txid, const bool unchecked) {
52 LOCK(cs);
53
54 if (m_unbroadcast_txids.erase(txid))
55diff --git a/src/txmempool.h b/src/txmempool.h
56index e61ba9d4ca..5175b8bea8 100644
57--- a/src/txmempool.h
58+++ b/src/txmempool.h
59@@ -411,7 +411,7 @@ private:
60 /**
61 * Track locally submitted transactions to periodically retry initial broadcast.
62 */
63- std::set<uint256> m_unbroadcast_txids GUARDED_BY(cs);
64+ std::set<Txid> m_unbroadcast_txids GUARDED_BY(cs);
65
66
67 /**
68 @@ -678,26 +678,26 @@ public:
69 size_t DynamicMemoryUsage() const;
70
71 /** Adds a transaction to the unbroadcast set */
72- void AddUnbroadcastTx(const uint256& txid)
73+ void AddUnbroadcastTx(const Txid& txid)
74 {
75 LOCK(cs);
76 // Sanity check the transaction is in the mempool & insert into
77 // unbroadcast set.
78- if (exists(Txid::FromUint256(txid))) m_unbroadcast_txids.insert(txid);
79+ if (exists(txid)) m_unbroadcast_txids.insert(txid);
80 };
81
82 /** Removes a transaction from the unbroadcast set */
83- void RemoveUnbroadcastTx(const uint256& txid, const bool unchecked = false);
84+ void RemoveUnbroadcastTx(const Txid& txid, const bool unchecked = false);
85
86 /** Returns transactions in unbroadcast set */
87- std::set<uint256> GetUnbroadcastTxs() const
88+ std::set<Txid> GetUnbroadcastTxs() const
89 {
90 LOCK(cs);
91 return m_unbroadcast_txids;
92 }
93
94 /** Returns whether a txid is in the unbroadcast set */
95- bool IsUnbroadcastTx(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs)
96+ bool IsUnbroadcastTx(const Txid& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs)
97 {
98 AssertLockHeld(cs);
99 return m_unbroadcast_txids.count(txid) != 0;
Edit: Added missing part of diff.
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.
0const GenTxidVariant& hash = *it;
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))};
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.
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).
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
0 std::holds_alternative<::Wtxid>(variant),
(but no big deal, especially since this code is deleted in the last commit anyways)
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.
0class GenTxid : public std::variant<Txid, Wtxid>
1{
2public:
3 .....
4 bool IsWtxid() const { return std::holds_alternative<Wtxid>(*this); }
5 .....
6}
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:
0 const uint256& ToUint256() const LIFETIMEBOUND
1 {
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). */
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:
0 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:
0diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
1index 54c2fb578f..adadb2a3e7 100644
2--- a/src/primitives/transaction.h
3+++ b/src/primitives/transaction.h
4@@ -438,17 +438,18 @@ public:
5 friend bool operator==(const GenTxid& a, const GenTxid& b) { return a.m_is_wtxid == b.m_is_wtxid && a.m_hash == b.m_hash; }
6 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); }
7
8- GenTxidVariant ToVariant() const {
9+ GenTxidVariant ToVariant() const
10+ {
11 return m_is_wtxid ?
12- GenTxidVariant{Wtxid::FromUint256(m_hash)} :
13- GenTxidVariant{Txid::FromUint256(m_hash)};
14+ GenTxidVariant{Wtxid::FromUint256(m_hash)} :
15+ GenTxidVariant{Txid::FromUint256(m_hash)};
16 }
17
18- static GenTxid FromVariant(const GenTxidVariant& variant) {
19+ static GenTxid FromVariant(const GenTxidVariant& variant)
20+ {
21 return GenTxid{
22 std::holds_alternative<::Wtxid>(variant),
23- variant.ToUint256()
24- };
25+ variant.ToUint256()};
26 }
27 };
28
29diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
30index 4137dd7040..39104a1a5c 100644
31--- a/src/util/transaction_identifier.h
32+++ b/src/util/transaction_identifier.h
33@@ -88,11 +88,13 @@ public:
34
35 bool IsWtxid() const { return std::holds_alternative<Wtxid>(*this); }
36
37- const uint256& ToUint256() const LIFETIMEBOUND {
38+ const uint256& ToUint256() const LIFETIMEBOUND
39+ {
40 return std::visit([](const auto& id) -> const uint256& { return id.ToUint256(); }, *this);
41 }
42
43- friend auto operator<=>(const GenTxidVariant& a, const GenTxidVariant& b) {
44+ friend auto operator<=>(const GenTxidVariant& a, const GenTxidVariant& b)
45+ {
46 return std::tuple(a.IsWtxid(), a.ToUint256()) <=> std::tuple(b.IsWtxid(), b.ToUint256());
47 }
48 };
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:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index df66d4c93d..a0ed1e2620 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -2440,8 +2440,8 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
5 }
6
7 CTransactionRef tx{inv.IsMsgWtx() ?
8- FindTxForGetData(*tx_relay, Wtxid::FromUint256(inv.hash)) :
9- FindTxForGetData(*tx_relay, Txid::FromUint256(inv.hash))};
10+ FindTxForGetData(*tx_relay, Wtxid::FromUint256(inv.hash)) :
11+ FindTxForGetData(*tx_relay, Txid::FromUint256(inv.hash))};
12 if (tx) {
13 // WTX and WITNESS_TX imply we serialize with witness
14 const auto maybe_with_witness = (inv.IsMsgTx() ? TX_NO_WITNESS : TX_WITH_WITNESS);
15diff --git a/src/validation.cpp b/src/validation.cpp
16index 9d788b50f6..c0bc5f28fc 100644
17--- a/src/validation.cpp
18+++ b/src/validation.cpp
19@@ -1135,8 +1135,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
20 AssertLockHeld(m_pool.cs);
21
22 // CheckPackageLimits expects the package transactions to not already be in the mempool.
23- assert(std::all_of(txns.cbegin(), txns.cend(), [this](const auto& tx)
24- { return !m_pool.exists(tx->GetHash());}));
25+ assert(std::all_of(txns.cbegin(), txns.cend(), [this](const auto& tx) { return !m_pool.exists(tx->GetHash()); }));
26
27 assert(txns.size() == workspaces.size());
28
29@@ -1346,8 +1345,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
30 AssertLockHeld(m_pool.cs);
31 // Sanity check: none of the transactions should be in the mempool, and none of the transactions
32 // should have a same-txid-different-witness equivalent in the mempool.
33- assert(std::all_of(workspaces.cbegin(), workspaces.cend(), [this](const auto& ws){
34- return !m_pool.exists(ws.m_ptx->GetHash()); }));
35+ assert(std::all_of(workspaces.cbegin(), workspaces.cend(), [this](const auto& ws) { return !m_pool.exists(ws.m_ptx->GetHash()); }));
36
37 bool all_submitted = true;
38 FinalizeSubpackage(args);
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:
0diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
1index c169c3bf41..054c8fb7dd 100644
2--- a/src/node/txdownloadman_impl.cpp
3+++ b/src/node/txdownloadman_impl.cpp
4@@ -126,21 +126,21 @@ void TxDownloadManagerImpl::BlockDisconnected()
5 bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxidVariant& gtxid, bool include_reconsiderable)
6 {
7 bool in_orphanage = std::visit(util::Overloaded{
8- // Normal query by wtxid.
9- [this](const Wtxid& wtxid) { return m_orphanage.HaveTx(wtxid); },
10- // Never query by txid: it is possible that the transaction in the orphanage has the same
11- // txid but a different witness, which would give us a false positive result. If we decided
12- // not to request the transaction based on this result, an attacker could prevent us from
13- // downloading a transaction by intentionally creating a malleated version of it. While
14- // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
15- // discerning which one that is, so the orphanage can store multiple transactions with the
16- // same txid.
17- //
18- // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
19- // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
20- // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
21- [this](const Txid& txid) { return m_orphanage.HaveTx(Wtxid::FromUint256(txid.ToUint256())); }
22- }, gtxid);
23+ // Normal query by wtxid.
24+ [this](const Wtxid& wtxid) { return m_orphanage.HaveTx(wtxid); },
25+ // Never query by txid: it is possible that the transaction in the orphanage has the same
26+ // txid but a different witness, which would give us a false positive result. If we decided
27+ // not to request the transaction based on this result, an attacker could prevent us from
28+ // downloading a transaction by intentionally creating a malleated version of it. While
29+ // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
30+ // discerning which one that is, so the orphanage can store multiple transactions with the
31+ // same txid.
32+ //
33+ // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
34+ // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
35+ // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
36+ [this](const Txid& txid) { return m_orphanage.HaveTx(Wtxid::FromUint256(txid.ToUint256())); }},
37+ gtxid);
38
39 if (in_orphanage) return true;
40
41@@ -183,7 +183,7 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxidVariant&
42 if (const auto* wtxid = std::get_if<Wtxid>(>xid)) {
43 if (auto orphan_tx{m_orphanage.GetTx(*wtxid)}) {
44 auto unique_parents{GetUniqueParents(*orphan_tx)};
45- std::erase_if(unique_parents, [&](const auto& txid){
46+ std::erase_if(unique_parents, [&](const auto& txid) {
47 return AlreadyHaveTx(txid, /*include_reconsiderable=*/false);
48 });
49
50@@ -398,7 +398,7 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction
51 // Filter parents that we already have.
52 // Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been
53 // previously rejected for being too low feerate. This orphan might CPFP it.
54- std::erase_if(unique_parents, [&](const auto& txid){
55+ std::erase_if(unique_parents, [&](const auto& txid) {
56 return AlreadyHaveTx(txid, /*include_reconsiderable=*/false);
57 });
58 const auto now{GetTime<std::chrono::microseconds>()};
59diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp
60index bdb38242f6..f2fb186b8e 100644
61--- a/src/test/fuzz/txdownloadman.cpp
62+++ b/src/test/fuzz/txdownloadman.cpp
63@@ -263,8 +263,7 @@ FUZZ_TARGET(txdownloadman, .init = initialize)
64 // returned true.
65 Assert(expect_work);
66 }
67- }
68- );
69+ });
70 // Jump forwards or backwards
71 auto time_skip = fuzzed_data_provider.PickValueInArray(TIME_SKIPS);
72 if (fuzzed_data_provider.ConsumeBool()) time_skip *= -1;
73@@ -438,8 +437,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize)
74 state_missing_inputs.Invalid(TxValidationResult::TX_MISSING_INPUTS, "");
75 txdownload_impl.MempoolRejectedTx(ptx, state_missing_inputs, rand_peer, fuzzed_data_provider.ConsumeBool());
76 }
77- }
78- );
79+ });
80
81 auto time_skip = fuzzed_data_provider.PickValueInArray(TIME_SKIPS);
82 if (fuzzed_data_provider.ConsumeBool()) time_skip *= -1;
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:
0diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
1index 8db30d21d1..a514ac8143 100644
2--- a/src/node/txdownloadman_impl.cpp
3+++ b/src/node/txdownloadman_impl.cpp
4@@ -275,12 +275,12 @@ std::vector<GenTxidVariant> TxDownloadManagerImpl::GetRequestsToSend(NodeId node
5 auto requestable = m_txrequest.GetRequestable(nodeid, current_time, &expired);
6 for (const auto& entry : expired) {
7 LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "wtx" : "tx",
8- entry.second.ToUint256().ToString(), entry.first);
9+ entry.second.ToUint256().ToString(), entry.first);
10 }
11 for (const GenTxidVariant& gtxid : requestable) {
12 if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) {
13 LogDebug(BCLog::NET, "Requesting %s %s peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx",
14- gtxid.ToUint256().ToString(), nodeid);
15+ gtxid.ToUint256().ToString(), nodeid);
16 requests.emplace_back(gtxid);
17 m_txrequest.RequestedTx(nodeid, gtxid, current_time + GETDATA_TX_INTERVAL);
18 } else {
19diff --git a/src/test/txrequest_tests.cpp b/src/test/txrequest_tests.cpp
20index 298586db37..e6b3e8bc15 100644
21--- a/src/test/txrequest_tests.cpp
22+++ b/src/test/txrequest_tests.cpp
23@@ -161,17 +161,18 @@ public:
24 * backwards (but note that the ordering of this event only follows the scenario's m_now.
25 */
26 void Check(NodeId peer, const std::vector<GenTxidVariant>& expected, size_t candidates, size_t inflight,
27- size_t completed, const std::string& checkname,
28- std::chrono::microseconds offset = std::chrono::microseconds{0})
29+ size_t completed, const std::string& checkname,
30+ std::chrono::microseconds offset = std::chrono::microseconds{0})
31 {
32 const auto comment = m_testname + " " + checkname;
33 auto& runner = m_runner;
34 const auto now = m_now;
35 assert(offset.count() <= 0);
36- runner.actions.emplace_back(m_now, [=,&runner]() {
37+ runner.actions.emplace_back(m_now, [=, &runner]() {
38 std::vector<std::pair<NodeId, GenTxidVariant>> expired_now;
39 auto ret = runner.txrequest.GetRequestable(peer, now + offset, &expired_now);
40- for (const auto& entry : expired_now) runner.expired.insert(entry);
41+ for (const auto& entry : expired_now)
42+ runner.expired.insert(entry);
43 runner.txrequest.SanityCheck();
44 runner.txrequest.PostGetRequestableSanityCheck(now + offset);
45 size_t total = candidates + inflight + completed;
46@@ -193,7 +194,7 @@ public:
47 {
48 const auto& testname = m_testname;
49 auto& runner = m_runner;
50- runner.actions.emplace_back(m_now, [=,&runner]() {
51+ runner.actions.emplace_back(m_now, [=, &runner]() {
52 auto it = runner.expired.find(std::pair<NodeId, GenTxidVariant>{peer, gtxid});
53 BOOST_CHECK_MESSAGE(it != runner.expired.end(), "[" + testname + "] missing expiration");
54 if (it != runner.expired.end()) runner.expired.erase(it);
55diff --git a/src/txrequest.cpp b/src/txrequest.cpp
56index ec2ebe978b..0d0e7543d1 100644
57--- a/src/txrequest.cpp
58+++ b/src/txrequest.cpp
59@@ -575,7 +575,7 @@ public:
60 }
61
62 void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred,
63- std::chrono::microseconds reqtime)
64+ std::chrono::microseconds reqtime)
65 {
66 // Bail out if we already have a CANDIDATE_BEST announcement for this (txhash, peer) combination. The case
67 // where there is a non-CANDIDATE_BEST announcement already will be caught by the uniqueness property of the
68@@ -595,7 +595,7 @@ public:
69
70 //! Find the GenTxids to request now from peer.
71 std::vector<GenTxidVariant> GetRequestable(NodeId peer, std::chrono::microseconds now,
72- std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
73+ std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
74 {
75 // Move time.
76 SetTimePoint(now, expired);
77@@ -730,7 +730,7 @@ void TxRequestTracker::PostGetRequestableSanityCheck(std::chrono::microseconds n
78 }
79
80 void TxRequestTracker::ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred,
81- std::chrono::microseconds reqtime)
82+ std::chrono::microseconds reqtime)
83 {
84 m_impl->ReceivedInv(peer, gtxid, preferred, reqtime);
85 }
86@@ -746,7 +746,7 @@ void TxRequestTracker::ReceivedResponse(NodeId peer, const GenTxidVariant& txhas
87 }
88
89 std::vector<GenTxidVariant> TxRequestTracker::GetRequestable(NodeId peer, std::chrono::microseconds now,
90- std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
91+ std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
92 {
93 return m_impl->GetRequestable(peer, now, expired);
94 }
95diff --git a/src/txrequest.h b/src/txrequest.h
96index f32fb1f2b9..3f2acef3a3 100644
97--- a/src/txrequest.h
98+++ b/src/txrequest.h
99@@ -133,7 +133,7 @@ public:
100 * from the specified gtxid.
101 */
102 void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred,
103- std::chrono::microseconds reqtime);
104+ std::chrono::microseconds reqtime);
105
106 /** Deletes all announcements for a given peer.
107 *
108@@ -164,7 +164,7 @@ public:
109 * simultaneously by one peer, and end up being requested from them, the requests will happen in announcement order.
110 */
111 std::vector<GenTxidVariant> GetRequestable(NodeId peer, std::chrono::microseconds now,
112- std::vector<std::pair<NodeId, GenTxidVariant>>* expired = nullptr);
113+ std::vector<std::pair<NodeId, GenTxidVariant>>* expired = nullptr);
114
115 /** Marks a transaction as requested, with a specified expiry.
116 *
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?
0 auto j{std::visit([&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(cs) { return GetIter(id); }, hashb)};
1 if (!j.has_value()) return false;
2 auto i{std::visit([&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(cs) { return GetIter(id); }, hasha)};
3 if (!i.has_value()) return true;
4 uint64_t counta = i.value()->GetCountWithAncestors();
5 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)
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
0 template <typename T>
1 requires std::is_same_v<T, Txid> || std::is_same_v<T, Wtxid>
2 TxMempoolInfo info(const T& txid) const
3 {
4 LOCK(cs);
5 auto i{GetIter(txid)};
6 return i.has_value() ? GetInfo(*i) : TxMempoolInfo{};
7 }
8
9 /** Returns info for a transaction if its entry_sequence < last_sequence */
10 template <typename T>
11 requires std::is_same_v<T, Txid> || std::is_same_v<T, Wtxid>
12 TxMempoolInfo info_for_relay(const T& txid, uint64_t last_sequence) const
13 {
14 LOCK(cs);
15 auto i{GetIter(txid)};
16 return (i.has_value() && i.value()->GetSequence() < last_sequence) ? GetInfo(*i) : TxMempoolInfo{};
17 }
See the diff here as part of branch https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2025-06/overload-getiter
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.
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>
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-
txrequest changes should be quite a bit easier to review now as well.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/45122218183
LLM reason (✨ experimental): The CI failure is caused by a lint error in the script 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.