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 |
|---|---|
| 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.
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.
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.
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:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index 7a97071052..f390b0f944 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -5442,18 +5442,15 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi
5 namespace {
6 class CompareInvMempoolOrder
7 {
8- CTxMemPool* mp;
9+ const CTxMemPool* m_mempool;
10 public:
11- explicit CompareInvMempoolOrder(CTxMemPool* _mempool)
12- {
13- mp = _mempool;
14- }
15+ explicit CompareInvMempoolOrder(CTxMemPool* mempool) : m_mempool{mempool} {}
16
17 bool operator()(std::set<GenTxidVariant>::iterator a, std::set<GenTxidVariant>::iterator b)
18 {
19 /* As std::make_heap produces a max-heap, we want the entries with the
20 * fewest ancestors/highest fee to sort later. */
21- return mp->CompareDepthAndScore(*b, *a);
22+ return m_mempool->CompareDepthAndScore(*b, *a);
23 }
24 };
25 } // namespace
26diff --git a/src/txmempool.cpp b/src/txmempool.cpp
27index 819e7e0550..e3660d9aea 100644
28--- a/src/txmempool.cpp
29+++ b/src/txmempool.cpp
30@@ -794,7 +794,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
31 assert(innerUsage == cachedInnerUsage);
32 }
33
34-bool CTxMemPool::CompareDepthAndScore(const GenTxidVariant& hasha, const GenTxidVariant& hashb)
35+bool CTxMemPool::CompareDepthAndScore(const GenTxidVariant& hasha, const GenTxidVariant& hashb) const
36 {
37 /* Return `true` if hasha should be considered sooner than hashb. Namely when:
38 * a is not in the mempool, but b is
39diff --git a/src/txmempool.h b/src/txmempool.h
40index 205cfa346b..ca1d1e870e 100644
41--- a/src/txmempool.h
42+++ b/src/txmempool.h
43@@ -467,7 +467,7 @@ public:
44 void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
45 void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs);
46
47- bool CompareDepthAndScore(const GenTxidVariant& hasha, const GenTxidVariant& hashb);
48+ bool CompareDepthAndScore(const GenTxidVariant& hasha, const GenTxidVariant& hashb) const;
49 bool isSpent(const COutPoint& outpoint) const;
50 unsigned int GetTransactionsUpdated() const;
51 void AddTransactionsUpdated(unsigned int n);
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>;
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.
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.
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index 785f6d799a..128a188d00 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -947,8 +947,7 @@ private:
5 std::atomic<std::chrono::seconds> m_last_tip_update{0s};
6
7 /** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */
8- template <TransactionIdentifier T>
9- CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const T& txid)
10+ CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const CInv& inv)
11 EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, NetEventsInterface::g_msgproc_mutex);
12
13 void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc)
14@@ -2392,11 +2391,15 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
15 }
16 }
17
18-template <TransactionIdentifier T>
19-CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const T& txid)
20+CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const CInv& inv)
21 {
22+ auto gtxid{ToGenTxid(inv).ToVariant()};
23 // If a tx was in the mempool prior to the last INV for this peer, permit the request.
24- auto txinfo = m_mempool.info_for_relay(txid, tx_relay.m_last_inv_sequence);
25+ auto txinfo{std::visit(
26+ [&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) {
27+ return m_mempool.info_for_relay(id, tx_relay.m_last_inv_sequence);
28+ },
29+ gtxid)};
30 if (txinfo.tx) {
31 return std::move(txinfo.tx);
32 }
33@@ -2405,7 +2408,7 @@ CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay,
34 {
35 LOCK(m_most_recent_block_mutex);
36 if (m_most_recent_block_txs != nullptr) {
37- auto it = m_most_recent_block_txs->find(txid.ToUint256());
38+ auto it = m_most_recent_block_txs->find(gtxid.ToUint256());
39 if (it != m_most_recent_block_txs->end()) return it->second;
40 }
41 }
42@@ -2439,10 +2442,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
43 continue;
44 }
45
46- CTransactionRef tx{inv.IsMsgWtx() ?
47- FindTxForGetData(*tx_relay, Wtxid::FromUint256(inv.hash)) :
48- FindTxForGetData(*tx_relay, Txid::FromUint256(inv.hash))};
49- if (tx) {
50+ if (auto tx{FindTxForGetData(*tx_relay, inv)}) {
51 // WTX and WITNESS_TX imply we serialize with witness
52 const auto maybe_with_witness = (inv.IsMsgTx() ? TX_NO_WITNESS : TX_WITH_WITNESS);
53 MakeAndPushMessage(pfrom, NetMsgType::TX, maybe_with_witness(*tx));
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.
0diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
1index f6e831624d..511ba41568 100644
2--- a/src/interfaces/chain.h
3+++ b/src/interfaces/chain.h
4@@ -208,7 +208,7 @@ public:
5 virtual RBFTransactionState isRBFOptIn(const CTransaction& tx) = 0;
6
7 //! Check if transaction is in mempool.
8- virtual bool isInMempool(const uint256& txid) = 0;
9+ virtual bool isInMempool(const Txid& txid) = 0;
10
11 //! Check if transaction has descendants in mempool.
12 virtual bool hasDescendantsInMempool(const uint256& txid) = 0;
13diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
14index 6bead3ea1e..0b7cea347c 100644
15--- a/src/node/interfaces.cpp
16+++ b/src/node/interfaces.cpp
17@@ -669,11 +669,11 @@ public:
18 LOCK(m_node.mempool->cs);
19 return IsRBFOptIn(tx, *m_node.mempool);
20 }
21- bool isInMempool(const uint256& txid) override
22+ bool isInMempool(const Txid& txid) override
23 {
24 if (!m_node.mempool) return false;
25 LOCK(m_node.mempool->cs);
26- return m_node.mempool->exists(Txid::FromUint256(txid));
27+ return m_node.mempool->exists(txid);
28 }
29 bool hasDescendantsInMempool(const uint256& txid) override
30 {
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:
0diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
1index a16490ceb9..87a47b30ee 100644
2--- a/src/node/txdownloadman_impl.cpp
3+++ b/src/node/txdownloadman_impl.cpp
4@@ -9,7 +9,6 @@
5 #include <consensus/validation.h>
6 #include <logging.h>
7 #include <txmempool.h>
8-#include <util/overloaded.h>
9 #include <validation.h>
10 #include <validationinterface.h>
11
12@@ -125,26 +124,19 @@ void TxDownloadManagerImpl::BlockDisconnected()
13
14 bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxidVariant& gtxid, bool include_reconsiderable)
15 {
16- if (std::visit(util::Overloaded{
17- // Normal query by wtxid.
18- [this](const Wtxid& wtxid) { return m_orphanage.HaveTx(wtxid); },
19- // Never query by txid: it is possible that the transaction in the orphanage has the same
20- // txid but a different witness, which would give us a false positive result. If we decided
21- // not to request the transaction based on this result, an attacker could prevent us from
22- // downloading a transaction by intentionally creating a malleated version of it. While
23- // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
24- // discerning which one that is, so the orphanage can store multiple transactions with the
25- // same txid.
26- //
27- // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
28- // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
29- // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
30- [this](const Txid& txid) { return m_orphanage.HaveTx(Wtxid::FromUint256(txid.ToUint256())); }},
31- gtxid)) {
32- return true;
33- }
34-
35 const uint256& hash = gtxid.ToUint256();
36+ // Never query by txid: it is possible that the transaction in the orphanage has the same
37+ // txid but a different witness, which would give us a false positive result. If we decided
38+ // not to request the transaction based on this result, an attacker could prevent us from
39+ // downloading a transaction by intentionally creating a malleated version of it. While
40+ // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
41+ // discerning which one that is, so the orphanage can store multiple transactions with the
42+ // same txid.
43+ //
44+ // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
45+ // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
46+ // help us find non-segwit transactions, saving bandwidth, and should have no false positives.
47+ if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true;
48
49 if (include_reconsiderable && RecentRejectsReconsiderableFilter().contains(hash)) return true;
50
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);
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:
0diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
1index 7116c433be..32f3ae7931 100644
2--- a/src/node/txdownloadman_impl.cpp
3+++ b/src/node/txdownloadman_impl.cpp
4@@ -273,9 +273,9 @@ std::vector<GenTxidVariant> TxDownloadManagerImpl::GetRequestsToSend(NodeId node
5 std::vector<GenTxidVariant> requests;
6 std::vector<std::pair<NodeId, GenTxidVariant>> expired;
7 auto requestable = m_txrequest.GetRequestable(nodeid, current_time, &expired);
8- for (const auto& entry : expired) {
9- LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "wtx" : "tx",
10- entry.second.ToUint256().ToString(), entry.first);
11+ for (const auto& [node_id, gtxid] : expired) {
12+ LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx",
13+ gtxid.ToUint256().ToString(), node_id);
14 }
15 for (const GenTxidVariant& gtxid : requestable) {
16 if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) {
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);
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());
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:
0 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 };
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).
left two nits that can be ignored or followed-up later on.
review ACK a60f863d3e276534444571282f432b913d3967db 🎽
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK a60f863d3e276534444571282f432b913d3967db 🎽
3Y4M+5Nl4FOcgYfClv1qRnd5797LuKUVoDluobop5CMpD3562zy81q/G5Wgo0lri3tMAlaUzCOjdjwFe/zausCw==
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.,
0std::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);
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);
AssertLockHeld in pre-existing TxId overload?
Follow ups to these last few comments can be found here: #33005
edit: after I fix CI…