refactor: Convert GenTxid to `std::variant`

marcofleon commented at 3:45 pm on May 28, 2025: contributor

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.

DrahtBot commented at 3:45 pm on May 28, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32631.

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32189 (refactor: Txid type safety (parent PR) by marcofleon)
#31829 (p2p: improve TxOrphanage denial of service bounds by glozow)
#30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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.

DrahtBot added the label Refactoring on May 28, 2025

marcofleon commented at 3:46 pm on May 28, 2025: contributor

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.

in src/test/fuzz/txdownloadman.cpp:384 in e66a5540e2 outdated

382+                GenTxid gtxid;
383+                if (fuzzed_data_provider.ConsumeBool()) {
384+                    rand_tx->GetHash();
385+                } else {
386+                    rand_tx->GetWitnessHash();
387+                }

dergoegge commented at 4:13 pm on May 28, 2025:

The return values for GetHash and GetWitnessHash need to be assigned to gtxid

marcofleon force-pushed on May 28, 2025

in src/net_processing.cpp:5819 in 98358c4d85 outdated

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());

dergoegge commented at 4:19 pm on May 28, 2025:

0                        Assume(peer->m_wtxid_relay == std::holds_alternative<Wtxid>(hash));
1                        CInv inv(peer->m_wtxid_relay ? MSG_WTX : MSG_TX, hash.ToUint256());

marcofleon commented at 5:41 pm on June 3, 2025:

done

in src/node/txdownloadman_impl.cpp:380 in 98358c4d85 outdated

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;

dergoegge commented at 4:26 pm on May 28, 2025:

not that it really matters but could do

0            std::optional<Txid> rejected_parent_reconsiderable;

marcofleon commented at 5:41 pm on June 3, 2025:

completed

in src/util/transaction_identifier.h:95 in 98358c4d85 outdated

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+    }

stickies-v commented at 9:12 am on May 29, 2025:

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>)

marcofleon commented at 3:24 pm on June 4, 2025:

done

in src/txmempool.h:658 in 98358c4d85 outdated

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     }

stickies-v commented at 9:51 am on May 29, 2025:

In 811d2f34a0309a62a69897a15db47214bbfec776:

Isn’t it more straightforward to just to have two bool exists(const Txid& txid) and bool exists(const Wtxid& wtxid) overloads?

marcofleon commented at 3:25 pm on June 4, 2025:

I gotchu

dergoegge commented at 9:52 am on May 29, 2025: member

Concept ACK

I’m not sure if the intermediate commits make things better here. Looking at the final diff seems simple enough.

in src/txmempool.cpp:907 in 98358c4d85 outdated

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);

stickies-v commented at 10:23 am on May 29, 2025:

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;

marcofleon commented at 3:25 pm on June 4, 2025:

Thank you for the patch, it helped.

stickies-v commented at 10:31 am on May 29, 2025: contributor

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.

instagibbs commented at 4:42 pm on May 29, 2025: member

concept ACK

Implement GenTxid as a variant

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.

715b81ef23

marcofleon force-pushed on Jun 3, 2025

DrahtBot added the label CI failed on Jun 3, 2025

DrahtBot commented at 5:42 pm on June 3, 2025: contributor

🚧 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.

marcofleon force-pushed on Jun 3, 2025

theStack commented at 11:21 am on June 4, 2025: contributor

Concept ACK

marcofleon force-pushed on Jun 4, 2025

marcofleon commented at 3:24 pm on June 4, 2025: contributor

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.

DrahtBot removed the label CI failed on Jun 4, 2025

in src/net_processing.cpp:5777 in ea58f45b43 outdated

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));

hodlinator commented at 10:19 am on June 9, 2025:

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)});

marcofleon commented at 2:44 pm on June 10, 2025:

Good catch, fixed. It’s eventually changed to ToGenTxid(inv) so I use ToVariant() here and remove that later on.

in src/net_processing.cpp:2175 in b740b6f947 outdated

2173+        GenTxid gtxid;
2174+        if (peer.m_wtxid_relay) {
2175+            gtxid = wtxid;
2176+        } else {
2177+            gtxid = txid;
2178         }

hodlinator commented at 10:45 am on June 9, 2025:

nit: Avoid default-initialized GenTxid:

0        auto gtxid{peer.m_wtxid_relay ? GenTxid{wtxid} : GenTxid{txid}};

marcofleon commented at 2:49 pm on June 10, 2025:

Fixed. I thought there was some reason I chose not to do this from the start, but I forgot.. so yeah I agree this is better.

in src/txmempool.cpp:809 in b740b6f947 outdated

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);

hodlinator commented at 11:26 am on June 9, 2025:

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.”

marcofleon commented at 2:51 pm on June 10, 2025:

Thanks for reviewing. I don’t think I can improve upon your suggested commit message, so hope you don’t mind that I took this directly.

in src/net_processing.cpp:1587 in b740b6f947 outdated

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());

hodlinator commented at 1:19 pm on June 9, 2025:

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.

marcofleon commented at 3:01 pm on June 10, 2025:

I’m leaving the rest of the mempool for the next (and final) type safety PR. My goal for this PR is to only touch GenTxid instances. I think if I start changing things here, it cascades and I would have to address a lot more of the mempool.

hodlinator commented at 7:49 am on June 11, 2025:

Yeah, makes sense to draw the line somewhere. (Noticed I had accidentally omitted the last 30 lines of the diff, fixed FWIW).

hodlinator commented at 1:20 pm on June 9, 2025: contributor

Concept ACK b740b6f9479a2351cd3b66a0f6e53848452b6bf8

marcofleon force-pushed on Jun 10, 2025

Convert `CompareInvMempoolOrder` to GenTxidVariant

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.

a105a10d71

Replace GenTxid with Txid/Wtxid overloads in `txmempool` ab954a169f

Convert `txdownloadman_impl` to GenTxidVariant

Convert all of `txdownloadman_impl` to the new variant except for
`GetRequestsToSend`, which will be easier to switch at the same
time as `txrequest`.

5b7119fd54

Convert `txrequest` to GenTxidVariant

Switch all instances of GenTxid to the new variant
in `txrequest` and complete `txdownloadman_impl` by
converting `GetRequestsToSend`.

88dc7a19c1

Convert remaining instances of GenTxid to GenTxidVariant 68d161c219

Remove old GenTxid class b75836613b

scripted-diff: Replace GenTxidVariant with GenTxid

-BEGIN VERIFY SCRIPT-
sed -i 's/GenTxidVariant/GenTxid/g' $(git grep -l 'GenTxidVariant')
-END VERIFY SCRIPT-

d5a6e7a0a4

marcofleon force-pushed on Jun 10, 2025

in src/net_processing.cpp:5813 in a105a10d71 outdated

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;

hodlinator commented at 7:58 am on June 11, 2025:

nit in a105a10d71c266cf7971fc66109c9ac8558e6d60: Could avoid copy as it will keep the instance alive.

0const GenTxidVariant& hash = *it;

marcofleon commented at 2:14 pm on June 12, 2025:

We erase the iterator soon after but still use the hash, so making a copy here is correct.

in src/net_processing.cpp:2444 in ab954a169f outdated

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))};

hodlinator commented at 8:06 am on June 11, 2025:

(nit in ab954a169f07c86f85e292d3f488df0cc066b722: Happy to see Freemasons have embraced Bitcoin. Might be neater to have 32 spaces for a multiple of 4 instead of current 33).

marcofleon commented at 2:14 pm on June 12, 2025:

I’ll include this with my next push.

in src/txrequest.cpp:429 in d5a6e7a0a4

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) {

hodlinator commented at 12:08 pm on June 11, 2025:

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.

marcofleon commented at 2:13 pm on June 12, 2025:

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 the point 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.

hodlinator commented at 12:29 pm on June 11, 2025: contributor

Code review d5a6e7a0a40bb78c2fcadd2662f2d64b86c374b7

Thanks for incorporating most of my feedback!

Mostly focused on reviewing non-test code.

refactor: Convert GenTxid to std::variant #32631

Code Coverage & Benchmarks

Reviews

Conflicts

refactor: Convert GenTxid to `std::variant` #32631