Convert txrequest to GenTxidVariant

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

src/net_processing.cpp

@@ -5962,8 +5962,8 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
         //
         {
             LOCK(m_tx_download_mutex);
-            for (const GenTxid& gtxid : m_txdownloadman.GetRequestsToSend(pto->GetId(), current_time)) {
+            for (const GenTxidVariant& gtxid : m_txdownloadman.GetRequestsToSend(pto->GetId(), current_time)) {
-                vGetData.emplace_back(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*peer)), gtxid.GetHash());
+                vGetData.emplace_back(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*peer)), gtxid.ToUint256());
                 if (vGetData.size() >= MAX_GETDATA_SZ) {
                     MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData);
                     vGetData.clear();

src/node/txdownloadman.h

@@ -141,7 +141,7 @@ public:
     bool AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtxid, std::chrono::microseconds now);
 
     /** Get getdata requests to send. */
-    std::vector<GenTxid> GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time);
+    std::vector<GenTxidVariant> GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time);
 
     /** Should be called when a notfound for a tx has been received. */
     void ReceivedNotFound(NodeId nodeid, const std::vector<GenTxidVariant>& gtxids);

src/node/txdownloadman_impl.cpp

@@ -43,7 +43,7 @@ bool TxDownloadManager::AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtx
 {
     return m_impl->AddTxAnnouncement(peer, gtxid, now);
 }
-std::vector<GenTxid> TxDownloadManager::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time)
+std::vector<GenTxidVariant> TxDownloadManager::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time)
 {
     return m_impl->GetRequestsToSend(nodeid, current_time);
 }
@@ -218,7 +218,7 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxidVariant&
     const bool overloaded = !info.m_relay_permissions && m_txrequest.CountInFlight(peer) >= MAX_PEER_TX_REQUEST_IN_FLIGHT;
     if (overloaded) delay += OVERLOADED_PEER_TX_DELAY;
 
-    m_txrequest.ReceivedInv(peer, GenTxid::FromVariant(gtxid), info.m_preferred, now + delay);
+    m_txrequest.ReceivedInv(peer, gtxid, info.m_preferred, now + delay);
 
     return false;
 }
@@ -255,31 +255,31 @@ bool TxDownloadManagerImpl::MaybeAddOrphanResolutionCandidate(const std::vector<
     // Treat finding orphan resolution candidate as equivalent to the peer announcing all missing parents.
     // In the future, orphan resolution may include more explicit steps
     for (const auto& parent_txid : unique_parents) {
-        m_txrequest.ReceivedInv(nodeid, GenTxid::Txid(parent_txid), info.m_preferred, now + delay);
+        m_txrequest.ReceivedInv(nodeid, parent_txid, info.m_preferred, now + delay);
     }
     LogDebug(BCLog::TXPACKAGES, "added peer=%d as a candidate for resolving orphan %s\n", nodeid, wtxid.ToString());
     return true;
 }
 
-std::vector<GenTxid> TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time)
+std::vector<GenTxidVariant> TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time)
 {
-    std::vector<GenTxid> requests;
+    std::vector<GenTxidVariant> requests;
-    std::vector<std::pair<NodeId, GenTxid>> expired;
+    std::vector<std::pair<NodeId, GenTxidVariant>> expired;
     auto requestable = m_txrequest.GetRequestable(nodeid, current_time, &expired);
-    for (const auto& entry : expired) {
+    for (const auto& [expired_nodeid, gtxid] : expired) {
-        LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "wtx" : "tx",
+        LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx",
-            entry.second.GetHash().ToString(), entry.first);
+                 gtxid.ToUint256().ToString(), expired_nodeid);
     }
-    for (const GenTxid& gtxid : requestable) {
+    for (const GenTxidVariant& gtxid : requestable) {
-        if (!AlreadyHaveTx(gtxid.ToVariant(), /*include_reconsiderable=*/false)) {
+        if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) {
             LogDebug(BCLog::NET, "Requesting %s %s peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx",
-                gtxid.GetHash().ToString(), nodeid);
+                     gtxid.ToUint256().ToString(), nodeid);
             requests.emplace_back(gtxid);
-            m_txrequest.RequestedTx(nodeid, gtxid.GetHash(), current_time + GETDATA_TX_INTERVAL);
+            m_txrequest.RequestedTx(nodeid, gtxid.ToUint256(), current_time + GETDATA_TX_INTERVAL);
         } else {
             // We have already seen this transaction, no need to download. This is just a belt-and-suspenders, as
             // this should already be called whenever a transaction becomes AlreadyHaveTx().
-            m_txrequest.ForgetTxHash(gtxid.GetHash());
+            m_txrequest.ForgetTxHash(gtxid.ToUint256());
         }
     }
     return requests;

src/node/txdownloadman_impl.h

@@ -166,7 +166,7 @@ public:
     bool AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtxid, std::chrono::microseconds now);
 
     /** Get getdata requests to send. */
-    std::vector<GenTxid> GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time);
+    std::vector<GenTxidVariant> GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time);
 
     /** Marks a tx as ReceivedResponse in txrequest. */
     void ReceivedNotFound(NodeId nodeid, const std::vector<GenTxidVariant>& gtxids);

src/test/fuzz/txdownloadman.cpp

@@ -382,7 +382,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize)
                 // TxDownloadManager should not be telling us to request things we already have.
                 // Exclude m_lazy_recent_rejects_reconsiderable because it may request low-feerate parent of orphan.
                 for (const auto& gtxid : getdata_requests) {
-                    Assert(!txdownload_impl.AlreadyHaveTx(gtxid.ToVariant(), /*include_reconsiderable=*/false));
+                    Assert(!txdownload_impl.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false));
                 }
             },
             [&] {

src/test/fuzz/txrequest.cpp

@@ -19,7 +19,7 @@ namespace {
 constexpr int MAX_TXHASHES = 16;
 constexpr int MAX_PEERS = 16;
 
-//! Randomly generated GenTxids used in this test (length is MAX_TXHASHES).
+//! Randomly generated txhashes used in this test (length is MAX_TXHASHES).
 uint256 TXHASHES[MAX_TXHASHES];
 
 //! Precomputed random durations (positive and negative, each ~exponentially distributed).
@@ -204,7 +204,8 @@ public:
         }
 
         // Call TxRequestTracker's implementation.
-        m_tracker.ReceivedInv(peer, is_wtxid ? GenTxid::Wtxid(TXHASHES[txhash]) : GenTxid::Txid(TXHASHES[txhash]), preferred, reqtime);
+        auto gtxid = is_wtxid ? GenTxidVariant{Wtxid::FromUint256(TXHASHES[txhash])} : GenTxidVariant{Txid::FromUint256(TXHASHES[txhash])};
+        m_tracker.ReceivedInv(peer, gtxid, preferred, reqtime);
     }
 
     void RequestedTx(int peer, int txhash, std::chrono::microseconds exptime)
@@ -246,13 +247,14 @@ public:
 
         //! list of (sequence number, txhash, is_wtxid) tuples.
         std::vector<std::tuple<uint64_t, int, bool>> result;
-        std::vector<std::pair<NodeId, GenTxid>> expected_expired;
+        std::vector<std::pair<NodeId, GenTxidVariant>> expected_expired;
         for (int txhash = 0; txhash < MAX_TXHASHES; ++txhash) {
             // Mark any expired REQUESTED announcements as COMPLETED.
             for (int peer2 = 0; peer2 < MAX_PEERS; ++peer2) {
                 Announcement& ann2 = m_announcements[txhash][peer2];
                 if (ann2.m_state == State::REQUESTED && ann2.m_time <= m_now) {
-                    expected_expired.emplace_back(peer2, ann2.m_is_wtxid ? GenTxid::Wtxid(TXHASHES[txhash]) : GenTxid::Txid(TXHASHES[txhash]));
+                    auto gtxid = ann2.m_is_wtxid ? GenTxidVariant{Wtxid::FromUint256(TXHASHES[txhash])} : GenTxidVariant{Txid::FromUint256(TXHASHES[txhash])};
+                    expected_expired.emplace_back(peer2, gtxid);
                     ann2.m_state = State::COMPLETED;
                     break;
                 }
@@ -270,7 +272,7 @@ public:
         std::sort(expected_expired.begin(), expected_expired.end());
 
         // Compare with TxRequestTracker's implementation.
-        std::vector<std::pair<NodeId, GenTxid>> expired;
+        std::vector<std::pair<NodeId, GenTxidVariant>> expired;
         const auto actual = m_tracker.GetRequestable(peer, m_now, &expired);
         std::sort(expired.begin(), expired.end());
         assert(expired == expected_expired);
@@ -278,7 +280,7 @@ public:
         m_tracker.PostGetRequestableSanityCheck(m_now);
         assert(result.size() == actual.size());
         for (size_t pos = 0; pos < actual.size(); ++pos) {
-            assert(TXHASHES[std::get<1>(result[pos])] == actual[pos].GetHash());
+            assert(TXHASHES[std::get<1>(result[pos])] == actual[pos].ToUint256());
             assert(std::get<2>(result[pos]) == actual[pos].IsWtxid());
         }
     }

src/test/txrequest_tests.cpp

@@ -61,7 +61,7 @@ struct Runner
     /** Which (peer, gtxid) combinations are known to be expired. These need to be accumulated here instead of
      *  checked directly in the GetRequestable return value to avoid introducing a dependency between the various
      *  parallel tests. */
-    std::multiset<std::pair<NodeId, GenTxid>> expired;
+    std::multiset<std::pair<NodeId, GenTxidVariant>> expired;
 };
 
 std::chrono::microseconds TxRequestTest::RandomTime8s() { return std::chrono::microseconds{1 + m_rng.randbits(23)}; }
@@ -103,17 +103,17 @@ public:
     void ForgetTxHash(const uint256& txhash)
     {
         auto& runner = m_runner;
-        runner.actions.emplace_back(m_now, [=,&runner]() {
+        runner.actions.emplace_back(m_now, [=, &runner]() {
             runner.txrequest.ForgetTxHash(txhash);
             runner.txrequest.SanityCheck();
         });
     }
 
     /** Schedule a ReceivedInv call at the Scheduler's current time. */
-    void ReceivedInv(NodeId peer, const GenTxid& gtxid, bool pref, std::chrono::microseconds reqtime)
+    void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool pref, std::chrono::microseconds reqtime)
     {
         auto& runner = m_runner;
-        runner.actions.emplace_back(m_now, [=,&runner]() {
+        runner.actions.emplace_back(m_now, [=, &runner]() {
             runner.txrequest.ReceivedInv(peer, gtxid, pref, reqtime);
             runner.txrequest.SanityCheck();
         });
@@ -123,7 +123,7 @@ public:
     void DisconnectedPeer(NodeId peer)
     {
         auto& runner = m_runner;
-        runner.actions.emplace_back(m_now, [=,&runner]() {
+        runner.actions.emplace_back(m_now, [=, &runner]() {
             runner.txrequest.DisconnectedPeer(peer);
             runner.txrequest.SanityCheck();
         });
@@ -133,7 +133,7 @@ public:
     void RequestedTx(NodeId peer, const uint256& txhash, std::chrono::microseconds exptime)
     {
         auto& runner = m_runner;
-        runner.actions.emplace_back(m_now, [=,&runner]() {
+        runner.actions.emplace_back(m_now, [=, &runner]() {
             runner.txrequest.RequestedTx(peer, txhash, exptime);
             runner.txrequest.SanityCheck();
         });
@@ -143,7 +143,7 @@ public:
     void ReceivedResponse(NodeId peer, const uint256& txhash)
     {
         auto& runner = m_runner;
-        runner.actions.emplace_back(m_now, [=,&runner]() {
+        runner.actions.emplace_back(m_now, [=, &runner]() {
             runner.txrequest.ReceivedResponse(peer, txhash);
             runner.txrequest.SanityCheck();
         });
@@ -160,18 +160,20 @@ public:
      * @param offset     Offset with the current time to use (must be <= 0). This allows simulations of time going
      *                   backwards (but note that the ordering of this event only follows the scenario's m_now.
      */
-    void Check(NodeId peer, const std::vector<GenTxid>& expected, size_t candidates, size_t inflight,
+    void Check(NodeId peer, const std::vector<GenTxidVariant>& expected, size_t candidates, size_t inflight,
-        size_t completed, const std::string& checkname,
+               size_t completed, const std::string& checkname,
-        std::chrono::microseconds offset = std::chrono::microseconds{0})
+               std::chrono::microseconds offset = std::chrono::microseconds{0})
     {
         const auto comment = m_testname + " " + checkname;
         auto& runner = m_runner;
         const auto now = m_now;
         assert(offset.count() <= 0);
-        runner.actions.emplace_back(m_now, [=,&runner]() {
+        runner.actions.emplace_back(m_now, [=, &runner]() {
-            std::vector<std::pair<NodeId, GenTxid>> expired_now;
+            std::vector<std::pair<NodeId, GenTxidVariant>> expired_now;
             auto ret = runner.txrequest.GetRequestable(peer, now + offset, &expired_now);
-            for (const auto& entry : expired_now) runner.expired.insert(entry);
+            for (const auto& entry : expired_now) {
+                runner.expired.insert(entry);
+            }
             runner.txrequest.SanityCheck();
             runner.txrequest.PostGetRequestableSanityCheck(now + offset);
             size_t total = candidates + inflight + completed;
@@ -189,12 +191,12 @@ public:
      *
      * Every expected expiration should be accounted for through exactly one call to this function.
      */
-    void CheckExpired(NodeId peer, GenTxid gtxid)
+    void CheckExpired(NodeId peer, GenTxidVariant gtxid)
     {
         const auto& testname = m_testname;
         auto& runner = m_runner;
-        runner.actions.emplace_back(m_now, [=,&runner]() {
+        runner.actions.emplace_back(m_now, [=, &runner]() {
-            auto it = runner.expired.find(std::pair<NodeId, GenTxid>{peer, gtxid});
+            auto it = runner.expired.find(std::pair<NodeId, GenTxidVariant>{peer, gtxid});
             BOOST_CHECK_MESSAGE(it != runner.expired.end(), "[" + testname + "] missing expiration");
             if (it != runner.expired.end()) runner.expired.erase(it);
         });
@@ -233,10 +235,11 @@ public:
         return ret;
     }
 
-    /** Generate a random GenTxid; the txhash follows NewTxHash; the is_wtxid flag is random. */
+    /** Generate a random GenTxid; the txhash follows NewTxHash; the transaction identifier is random. */
-    GenTxid NewGTxid(const std::vector<std::vector<NodeId>>& orders = {})
+    GenTxidVariant NewGTxid(const std::vector<std::vector<NodeId>>& orders = {})
     {
-        return m_rng.randbool() ? GenTxid::Wtxid(NewTxHash(orders)) : GenTxid::Txid(NewTxHash(orders));
+        const uint256 txhash{NewTxHash(orders)};
+        return m_rng.randbool() ? GenTxidVariant{Wtxid::FromUint256(txhash)} : GenTxidVariant{Txid::FromUint256(txhash)};
     }
 
     /** Generate a new random NodeId to use as peer. The same NodeId is never returned twice
@@ -285,7 +288,7 @@ void TxRequestTest::BuildSingleTest(Scenario& scenario, int config)
         scenario.AdvanceTime(RandomTime8s());
         auto expiry = RandomTime8s();
         scenario.Check(peer, {gtxid}, 1, 0, 0, "s5");
-        scenario.RequestedTx(peer, gtxid.GetHash(), scenario.Now() + expiry);
+        scenario.RequestedTx(peer, gtxid.ToUint256(), scenario.Now() + expiry);
         scenario.Check(peer, {}, 0, 1, 0, "s6");
 
         if ((config >> 3) == 1) { // The request will time out
@@ -299,7 +302,7 @@ void TxRequestTest::BuildSingleTest(Scenario& scenario, int config)
             scenario.AdvanceTime(std::chrono::microseconds{m_rng.randrange(expiry.count())});
             scenario.Check(peer, {}, 0, 1, 0, "s9");
             if ((config >> 3) == 3) { // A response will arrive for the transaction
-                scenario.ReceivedResponse(peer, gtxid.GetHash());
+                scenario.ReceivedResponse(peer, gtxid.ToUint256());
                 scenario.Check(peer, {}, 0, 0, 0, "s10");
                 return;
             }
@@ -309,7 +312,7 @@ void TxRequestTest::BuildSingleTest(Scenario& scenario, int config)
     if (config & 4) { // The peer will go offline
         scenario.DisconnectedPeer(peer);
     } else { // The transaction is no longer needed
-        scenario.ForgetTxHash(gtxid.GetHash());
+        scenario.ForgetTxHash(gtxid.ToUint256());
     }
     scenario.Check(peer, {}, 0, 0, 0, "s11");
 }
@@ -355,7 +358,7 @@ void TxRequestTest::BuildPriorityTest(Scenario& scenario, int config)
 
     // We possibly request from the selected peer.
     if (config & 8) {
-        scenario.RequestedTx(priopeer, gtxid.GetHash(), MAX_TIME);
+        scenario.RequestedTx(priopeer, gtxid.ToUint256(), MAX_TIME);
         scenario.Check(priopeer, {}, 0, 1, 0, "p7");
         scenario.Check(otherpeer, {}, 1, 0, 0, "p8");
         if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s());
@@ -365,7 +368,7 @@ void TxRequestTest::BuildPriorityTest(Scenario& scenario, int config)
     if (config & 16) {
         scenario.DisconnectedPeer(priopeer);
     } else {
-        scenario.ReceivedResponse(priopeer, gtxid.GetHash());
+        scenario.ReceivedResponse(priopeer, gtxid.ToUint256());
     }
     if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s());
     scenario.Check(priopeer, {}, 0, 0, !(config & 16), "p8");
@@ -449,7 +452,7 @@ void TxRequestTest::BuildBigPriorityTest(Scenario& scenario, int peers)
             scenario.DisconnectedPeer(peer);
             scenario.Check(peer, {}, 0, 0, 0, "b4");
         } else {
-            scenario.ReceivedResponse(peer, gtxid.GetHash());
+            scenario.ReceivedResponse(peer, gtxid.ToUint256());
             scenario.Check(peer, {}, 0, 0, request_order.size() > 0, "b5");
         }
         if (request_order.size()) {
@@ -510,8 +513,8 @@ void TxRequestTest::BuildWtxidTest(Scenario& scenario, int config)
     auto peerT = scenario.NewPeer();
     auto peerW = scenario.NewPeer();
     auto txhash = scenario.NewTxHash();
-    auto txid{GenTxid::Txid(txhash)};
+    auto txid{Txid::FromUint256(txhash)};
-    auto wtxid{GenTxid::Wtxid(txhash)};
+    auto wtxid{Wtxid::FromUint256(txhash)};
 
     auto reqtimeT = m_rng.randbool() ? MIN_TIME : scenario.Now() + RandomTime8s();
     auto reqtimeW = m_rng.randbool() ? MIN_TIME : scenario.Now() + RandomTime8s();
@@ -542,11 +545,11 @@ void TxRequestTest::BuildWtxidTest(Scenario& scenario, int config)
     // Let the preferred announcement be requested. It's not going to be delivered.
     auto expiry = RandomTime8s();
     if (config & 2) {
-        scenario.RequestedTx(peerT, txid.GetHash(), scenario.Now() + expiry);
+        scenario.RequestedTx(peerT, txid.ToUint256(), scenario.Now() + expiry);
         scenario.Check(peerT, {}, 0, 1, 0, "w5");
         scenario.Check(peerW, {}, 1, 0, 0, "w6");
     } else {
-        scenario.RequestedTx(peerW, wtxid.GetHash(), scenario.Now() + expiry);
+        scenario.RequestedTx(peerW, wtxid.ToUint256(), scenario.Now() + expiry);
         scenario.Check(peerT, {}, 1, 0, 0, "w7");
         scenario.Check(peerW, {}, 0, 1, 0, "w8");
     }
@@ -599,7 +602,7 @@ void TxRequestTest::BuildTimeBackwardsTest(Scenario& scenario)
     // Request from peer1.
     if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s());
     auto expiry = scenario.Now() + RandomTime8s();
-    scenario.RequestedTx(peer1, gtxid.GetHash(), expiry);
+    scenario.RequestedTx(peer1, gtxid.ToUint256(), expiry);
     scenario.Check(peer1, {}, 0, 1, 0, "r7");
     scenario.Check(peer2, {}, 1, 0, 0, "r8");
 
@@ -638,20 +641,20 @@ void TxRequestTest::BuildWeirdRequestsTest(Scenario& scenario)
 
     // We request gtxid2 from *peer1* - no effect.
     if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s());
-    scenario.RequestedTx(peer1, gtxid2.GetHash(), MAX_TIME);
+    scenario.RequestedTx(peer1, gtxid2.ToUint256(), MAX_TIME);
     scenario.Check(peer1, {gtxid1}, 1, 0, 0, "q4");
     scenario.Check(peer2, {gtxid2}, 1, 0, 0, "q5");
 
     // Now request gtxid1 from peer1 - marks it as REQUESTED.
     if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s());
     auto expiryA = scenario.Now() + RandomTime8s();
-    scenario.RequestedTx(peer1, gtxid1.GetHash(), expiryA);
+    scenario.RequestedTx(peer1, gtxid1.ToUint256(), expiryA);
     scenario.Check(peer1, {}, 0, 1, 0, "q6");
     scenario.Check(peer2, {gtxid2}, 1, 0, 0, "q7");
 
     // Request it a second time - nothing happens, as it's already REQUESTED.
     auto expiryB = expiryA + RandomTime8s();
-    scenario.RequestedTx(peer1, gtxid1.GetHash(), expiryB);
+    scenario.RequestedTx(peer1, gtxid1.ToUint256(), expiryB);
     scenario.Check(peer1, {}, 0, 1, 0, "q8");
     scenario.Check(peer2, {gtxid2}, 1, 0, 0, "q9");
 
@@ -668,7 +671,7 @@ void TxRequestTest::BuildWeirdRequestsTest(Scenario& scenario)
 
     // Requesting it yet again from peer1 doesn't do anything, as it's already COMPLETED.
     if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s());
-    scenario.RequestedTx(peer1, gtxid1.GetHash(), MAX_TIME);
+    scenario.RequestedTx(peer1, gtxid1.ToUint256(), MAX_TIME);
     scenario.Check(peer1, {}, 0, 0, 1, "q14");
     scenario.Check(peer2, {gtxid2, gtxid1}, 2, 0, 0, "q15");
 
@@ -680,13 +683,13 @@ void TxRequestTest::BuildWeirdRequestsTest(Scenario& scenario)
 
     // And request it from peer1 (weird as peer2 has the preference).
     if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s());
-    scenario.RequestedTx(peer1, gtxid2.GetHash(), MAX_TIME);
+    scenario.RequestedTx(peer1, gtxid2.ToUint256(), MAX_TIME);
     scenario.Check(peer1, {}, 0, 1, 1, "q18");
     scenario.Check(peer2, {gtxid1}, 2, 0, 0, "q19");
 
     // If peer2 now (normally) requests gtxid2, the existing request by peer1 becomes COMPLETED.
     if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s());
-    scenario.RequestedTx(peer2, gtxid2.GetHash(), MAX_TIME);
+    scenario.RequestedTx(peer2, gtxid2.ToUint256(), MAX_TIME);
     scenario.Check(peer1, {}, 0, 0, 2, "q20");
     scenario.Check(peer2, {gtxid1}, 1, 1, 0, "q21");
 

src/txrequest.cpp

@@ -60,7 +60,7 @@ using SequenceNumber = uint64_t;
 /** An announcement. This is the data we track for each txid or wtxid that is announced to us by each peer. */
 struct Announcement {
     /** Txid or wtxid that was announced. */
-    const uint256 m_txhash;
+    const GenTxidVariant m_gtxid;
     /** For CANDIDATE_{DELAYED,BEST,READY} the reqtime; for REQUESTED the expiry. */
     std::chrono::microseconds m_time;
     /** What peer the request was from. */
@@ -69,9 +69,6 @@ struct Announcement {
     const SequenceNumber m_sequence : 59;
     /** Whether the request is preferred. */
     const bool m_preferred : 1;
-    /** Whether this is a wtxid request. */
-    const bool m_is_wtxid : 1;
-
     /** What state this announcement is in. */
     State m_state : 3 {State::CANDIDATE_DELAYED};
     State GetState() const { return m_state; }
@@ -96,10 +93,9 @@ struct Announcement {
     }
 
     /** Construct a new announcement from scratch, initially in CANDIDATE_DELAYED state. */
-    Announcement(const GenTxid& gtxid, NodeId peer, bool preferred, std::chrono::microseconds reqtime,
+    Announcement(const GenTxidVariant& gtxid, NodeId peer, bool preferred, std::chrono::microseconds reqtime,
                  SequenceNumber sequence)
-        : m_txhash(gtxid.GetHash()), m_time(reqtime), m_peer(peer), m_sequence(sequence), m_preferred(preferred),
+        : m_gtxid(gtxid), m_time(reqtime), m_peer(peer), m_sequence(sequence), m_preferred(preferred) {}
-          m_is_wtxid{gtxid.IsWtxid()} {}
 };
 
 //! Type alias for priorities.
@@ -124,7 +120,7 @@ public:
 
     Priority operator()(const Announcement& ann) const
     {
-        return operator()(ann.m_txhash, ann.m_peer, ann.m_preferred);
+        return operator()(ann.m_gtxid.ToUint256(), ann.m_peer, ann.m_preferred);
     }
 };
 
@@ -148,7 +144,7 @@ struct ByPeerViewExtractor
     using result_type = ByPeerView;
     result_type operator()(const Announcement& ann) const
     {
-        return ByPeerView{ann.m_peer, ann.GetState() == State::CANDIDATE_BEST, ann.m_txhash};
+        return ByPeerView{ann.m_peer, ann.GetState() == State::CANDIDATE_BEST, ann.m_gtxid.ToUint256()};
     }
 };
 
@@ -172,7 +168,7 @@ public:
     result_type operator()(const Announcement& ann) const
     {
         const Priority prio = (ann.GetState() == State::CANDIDATE_READY) ? m_computer(ann) : 0;
-        return ByTxHashView{ann.m_txhash, ann.GetState(), prio};
+        return ByTxHashView{ann.m_gtxid.ToUint256(), ann.GetState(), prio};
     }
 };
 
@@ -280,7 +276,7 @@ std::map<uint256, TxHashInfo> ComputeTxHashInfo(const Index& index, const Priori
 {
     std::map<uint256, TxHashInfo> ret;
     for (const Announcement& ann : index) {
-        TxHashInfo& info = ret[ann.m_txhash];
+        TxHashInfo& info = ret[ann.m_gtxid.ToUint256()];
         // Classify how many announcements of each state we have for this txhash.
         info.m_candidate_delayed += (ann.GetState() == State::CANDIDATE_DELAYED);
         info.m_candidate_ready += (ann.GetState() == State::CANDIDATE_READY);
@@ -299,11 +295,6 @@ std::map<uint256, TxHashInfo> ComputeTxHashInfo(const Index& index, const Priori
     return ret;
 }
 
-GenTxid ToGenTxid(const Announcement& ann)
-{
-    return ann.m_is_wtxid ? GenTxid::Wtxid(ann.m_txhash) : GenTxid::Txid(ann.m_txhash);
-}
-
 }  // namespace
 
 /** Actual implementation for TxRequestTracker's data structure. */
@@ -409,7 +400,7 @@ private:
         // priority) comes last. Thus, if an existing _BEST exists for the same txhash that this announcement may
         // be preferred over, it must immediately follow the newly created _READY.
         auto it_next = std::next(it);
-        if (it_next == m_index.get<ByTxHash>().end() || it_next->m_txhash != it->m_txhash ||
+        if (it_next == m_index.get<ByTxHash>().end() || it_next->m_gtxid.ToUint256() != it->m_gtxid.ToUint256() ||
             it_next->GetState() == State::COMPLETED) {
             // This is the new best CANDIDATE_READY, and there is no IsSelected() announcement for this txhash
             // already.
@@ -435,7 +426,7 @@ private:
             auto it_prev = std::prev(it);
             // The next best CANDIDATE_READY, if any, immediately precedes the REQUESTED or CANDIDATE_BEST
             // announcement in the ByTxHash index.
-            if (it_prev->m_txhash == it->m_txhash && it_prev->GetState() == State::CANDIDATE_READY) {
+            if (it_prev->m_gtxid.ToUint256() == it->m_gtxid.ToUint256() && it_prev->GetState() == State::CANDIDATE_READY) {
                 // If one such CANDIDATE_READY exists (for this txhash), convert it to CANDIDATE_BEST.
                 Modify<ByTxHash>(it_prev, [](Announcement& ann){ ann.SetState(State::CANDIDATE_BEST); });
             }
@@ -451,10 +442,10 @@ private:
 
         // This announcement has a predecessor that belongs to the same txhash. Due to ordering, and the
         // fact that 'it' is not COMPLETED, its predecessor cannot be COMPLETED here.
-        if (it != m_index.get<ByTxHash>().begin() && std::prev(it)->m_txhash == it->m_txhash) return false;
+        if (it != m_index.get<ByTxHash>().begin() && std::prev(it)->m_gtxid.ToUint256() == it->m_gtxid.ToUint256()) return false;
 
         // This announcement has a successor that belongs to the same txhash, and is not COMPLETED.
-        if (std::next(it) != m_index.get<ByTxHash>().end() && std::next(it)->m_txhash == it->m_txhash &&
+        if (std::next(it) != m_index.get<ByTxHash>().end() && std::next(it)->m_gtxid.ToUint256() == it->m_gtxid.ToUint256() &&
             std::next(it)->GetState() != State::COMPLETED) return false;
 
         return true;
@@ -472,10 +463,10 @@ private:
 
         if (IsOnlyNonCompleted(it)) {
             // This is the last non-COMPLETED announcement for this txhash. Delete all.
-            uint256 txhash = it->m_txhash;
+            uint256 txhash = it->m_gtxid.ToUint256();
             do {
                 it = Erase<ByTxHash>(it);
-            } while (it != m_index.get<ByTxHash>().end() && it->m_txhash == txhash);
+            } while (it != m_index.get<ByTxHash>().end() && it->m_gtxid.ToUint256() == txhash);
             return false;
         }
 
@@ -490,7 +481,7 @@ private:
     //! - REQUESTED announcements with expiry <= now are turned into COMPLETED.
     //! - CANDIDATE_DELAYED announcements with reqtime <= now are turned into CANDIDATE_{READY,BEST}.
     //! - CANDIDATE_{READY,BEST} announcements with reqtime > now are turned into CANDIDATE_DELAYED.
-    void SetTimePoint(std::chrono::microseconds now, std::vector<std::pair<NodeId, GenTxid>>* expired)
+    void SetTimePoint(std::chrono::microseconds now, std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
     {
         if (expired) expired->clear();
 
@@ -501,7 +492,7 @@ private:
             if (it->GetState() == State::CANDIDATE_DELAYED && it->m_time <= now) {
                 PromoteCandidateReady(m_index.project<ByTxHash>(it));
             } else if (it->GetState() == State::REQUESTED && it->m_time <= now) {
-                if (expired) expired->emplace_back(it->m_peer, ToGenTxid(*it));
+                if (expired) expired->emplace_back(it->m_peer, it->m_gtxid);
                 MakeCompleted(m_index.project<ByTxHash>(it));
             } else {
                 break;
@@ -569,7 +560,7 @@ public:
     void ForgetTxHash(const uint256& txhash)
     {
         auto it = m_index.get<ByTxHash>().lower_bound(ByTxHashView{txhash, State::CANDIDATE_DELAYED, 0});
-        while (it != m_index.get<ByTxHash>().end() && it->m_txhash == txhash) {
+        while (it != m_index.get<ByTxHash>().end() && it->m_gtxid.ToUint256() == txhash) {
             it = Erase<ByTxHash>(it);
         }
     }
@@ -577,19 +568,19 @@ public:
     void GetCandidatePeers(const uint256& txhash, std::vector<NodeId>& result_peers) const
     {
         auto it = m_index.get<ByTxHash>().lower_bound(ByTxHashView{txhash, State::CANDIDATE_DELAYED, 0});
-        while (it != m_index.get<ByTxHash>().end() && it->m_txhash == txhash && it->GetState() != State::COMPLETED) {
+        while (it != m_index.get<ByTxHash>().end() && it->m_gtxid.ToUint256() == txhash && it->GetState() != State::COMPLETED) {
             result_peers.push_back(it->m_peer);
             ++it;
         }
     }
 
-    void ReceivedInv(NodeId peer, const GenTxid& gtxid, bool preferred,
+    void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred,
-        std::chrono::microseconds reqtime)
+                     std::chrono::microseconds reqtime)
     {
         // Bail out if we already have a CANDIDATE_BEST announcement for this (txhash, peer) combination. The case
         // where there is a non-CANDIDATE_BEST announcement already will be caught by the uniqueness property of the
         // ByPeer index when we try to emplace the new object below.
-        if (m_index.get<ByPeer>().count(ByPeerView{peer, true, gtxid.GetHash()})) return;
+        if (m_index.get<ByPeer>().count(ByPeerView{peer, true, gtxid.ToUint256()})) return;
 
         // Try creating the announcement with CANDIDATE_DELAYED state (which will fail due to the uniqueness
         // of the ByPeer index if a non-CANDIDATE_BEST announcement already exists with the same txhash and peer).
@@ -603,8 +594,8 @@ public:
     }
 
     //! Find the GenTxids to request now from peer.
-    std::vector<GenTxid> GetRequestable(NodeId peer, std::chrono::microseconds now,
+    std::vector<GenTxidVariant> GetRequestable(NodeId peer, std::chrono::microseconds now,
-        std::vector<std::pair<NodeId, GenTxid>>* expired)
+                                               std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
     {
         // Move time.
         SetTimePoint(now, expired);
@@ -624,10 +615,10 @@ public:
         });
 
         // Convert to GenTxid and return.
-        std::vector<GenTxid> ret;
+        std::vector<GenTxidVariant> ret;
         ret.reserve(selected.size());
         std::transform(selected.begin(), selected.end(), std::back_inserter(ret), [](const Announcement* ann) {
-            return ToGenTxid(*ann);
+            return ann->m_gtxid;
         });
         return ret;
     }
@@ -654,7 +645,7 @@ public:
             // found announcement had a different state than CANDIDATE_BEST. If it did, invariants guarantee that no
             // other CANDIDATE_BEST or REQUESTED can exist.
             auto it_old = m_index.get<ByTxHash>().lower_bound(ByTxHashView{txhash, State::CANDIDATE_BEST, 0});
-            if (it_old != m_index.get<ByTxHash>().end() && it_old->m_txhash == txhash) {
+            if (it_old != m_index.get<ByTxHash>().end() && it_old->m_gtxid.ToUint256() == txhash) {
                 if (it_old->GetState() == State::CANDIDATE_BEST) {
                     // The data structure's invariants require that there can be at most one CANDIDATE_BEST or one
                     // REQUESTED announcement per txhash (but not both simultaneously), so we have to convert any
@@ -738,8 +729,8 @@ void TxRequestTracker::PostGetRequestableSanityCheck(std::chrono::microseconds n
     m_impl->PostGetRequestableSanityCheck(now);
 }
 
-void TxRequestTracker::ReceivedInv(NodeId peer, const GenTxid& gtxid, bool preferred,
+void TxRequestTracker::ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred,
-    std::chrono::microseconds reqtime)
+                                   std::chrono::microseconds reqtime)
 {
     m_impl->ReceivedInv(peer, gtxid, preferred, reqtime);
 }
@@ -754,8 +745,8 @@ void TxRequestTracker::ReceivedResponse(NodeId peer, const uint256& txhash)
     m_impl->ReceivedResponse(peer, txhash);
 }
 
-std::vector<GenTxid> TxRequestTracker::GetRequestable(NodeId peer, std::chrono::microseconds now,
+std::vector<GenTxidVariant> TxRequestTracker::GetRequestable(NodeId peer, std::chrono::microseconds now,
-    std::vector<std::pair<NodeId, GenTxid>>* expired)
+                                                             std::vector<std::pair<NodeId, GenTxidVariant>>* expired)
 {
     return m_impl->GetRequestable(peer, now, expired);
 }

src/txrequest.h

@@ -132,8 +132,8 @@ public:
      * fetched. The new announcement is given the specified preferred and reqtime values, and takes its is_wtxid
      * from the specified gtxid.
      */
-    void ReceivedInv(NodeId peer, const GenTxid& gtxid, bool preferred,
+    void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred,
-        std::chrono::microseconds reqtime);
+                     std::chrono::microseconds reqtime);
 
     /** Deletes all announcements for a given peer.
      *
@@ -158,14 +158,13 @@ public:
      *    exists, and for which the specified peer is the best choice among all (reqtime <= now) CANDIDATE
      *    announcements with the same txhash (subject to preferredness rules, and tiebreaking using a deterministic
      *    salted hash of peer and txhash).
-     *  - The selected announcements are converted to GenTxids using their is_wtxid flag, and returned in
+     *  - The selected announcements are returned in announcement order (even if multiple were added at the same
-     *    announcement order (even if multiple were added at the same time, or when the clock went backwards while
+     *    time, or when the clock went backwards while they were being added). This is done to minimize disruption
-     *    they were being added). This is done to minimize disruption from dependent transactions being requested
+     *    from dependent transactions being requested out of order: if multiple dependent transactions are announced
-     *    out of order: if multiple dependent transactions are announced simultaneously by one peer, and end up
+     *    simultaneously by one peer, and end up being requested from them, the requests will happen in announcement order.
-     *    being requested from them, the requests will happen in announcement order.
      */
-    std::vector<GenTxid> GetRequestable(NodeId peer, std::chrono::microseconds now,
+    std::vector<GenTxidVariant> GetRequestable(NodeId peer, std::chrono::microseconds now,
-        std::vector<std::pair<NodeId, GenTxid>>* expired = nullptr);
+                                               std::vector<std::pair<NodeId, GenTxidVariant>>* expired = nullptr);
 
     /** Marks a transaction as requested, with a specified expiry.
      *