Merge bitcoin#23336: refactor: Make GenTxid boolean constructor private

fa4ec1c Make GenTxid boolean constructor private (MarcoFalke)
faeb9a5 remove unused CTxMemPool::info(const uint256& txid) (MarcoFalke)

Pull request description:

  This boolean argument is either verbose (when used with a named arg) or unintuitive and dangerous (when used as a plain bool).

  Fix that by making the constructor private.

ACKs for top commit:
  laanwj:
    Code review ACK fa4ec1c
  jnewbery:
    Code review ACK fa4ec1c
  glozow:
    code review ACK fa4ec1c

Tree-SHA512: bf08ee09168885cfda71e5a01ec412b93964662a90dd9d91e75f7fdf2eaff7c21a95204d0e90b00438bfeab564d0aea66bdb9c0394ee7a05743e65a817159446

Author: MarcoFalke

src/net_processing.cpp

@@ -3243,7 +3243,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         // already; and an adversary can already relay us old transactions
         // (older than our recency filter) if trying to DoS us, without any need
         // for witness malleation.
-        if (AlreadyHaveTx(GenTxid(/* is_wtxid=*/true, wtxid))) {
+        if (AlreadyHaveTx(GenTxid::Wtxid(wtxid))) {
             if (pfrom.HasPermission(NetPermissionFlags::ForceRelay)) {
                 // Always relay transactions received from peers with forcerelay
                 // permission, even if they were already in the mempool, allowing
@@ -3313,7 +3313,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                     // wtxidrelay peers.
                     // Eventually we should replace this with an improved
                     // protocol for getting all unconfirmed parents.
-                    const GenTxid gtxid{/* is_wtxid=*/false, parent_txid};
+                    const auto gtxid{GenTxid::Txid(parent_txid)};
                     pfrom.AddKnownTx(parent_txid);
                     if (!AlreadyHaveTx(gtxid)) AddTxAnnouncement(pfrom, gtxid, current_time);
                 }

src/primitives/transaction.h

@@ -391,8 +391,9 @@ class GenTxid
 {
     bool m_is_wtxid;
     uint256 m_hash;
-public:
     GenTxid(bool is_wtxid, const uint256& hash) : m_is_wtxid(is_wtxid), m_hash(hash) {}
+
+public:
     static GenTxid Txid(const uint256& hash) { return GenTxid{false, hash}; }
     static GenTxid Wtxid(const uint256& hash) { return GenTxid{true, hash}; }
     bool IsWtxid() const { return m_is_wtxid; }

src/protocol.cpp

@@ -223,5 +223,5 @@ std::vector<std::string> serviceFlagsToStr(uint64_t flags)
 GenTxid ToGenTxid(const CInv& inv)
 {
     assert(inv.IsGenTxMsg());
-    return {inv.IsMsgWtx(), inv.hash};
+    return inv.IsMsgWtx() ? GenTxid::Wtxid(inv.hash) : GenTxid::Txid(inv.hash);
 }

src/test/fuzz/txrequest.cpp

@@ -204,7 +204,7 @@ class Tester
         }
 
         // Call TxRequestTracker's implementation.
-        m_tracker.ReceivedInv(peer, GenTxid{is_wtxid, TXHASHES[txhash]}, preferred, reqtime);
+        m_tracker.ReceivedInv(peer, is_wtxid ? GenTxid::Wtxid(TXHASHES[txhash]) : GenTxid::Txid(TXHASHES[txhash]), preferred, reqtime);
     }
 
     void RequestedTx(int peer, int txhash, std::chrono::microseconds exptime)
@@ -252,7 +252,7 @@ class Tester
             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, GenTxid{ann2.m_is_wtxid, TXHASHES[txhash]});
+                    expected_expired.emplace_back(peer2, ann2.m_is_wtxid ? GenTxid::Wtxid(TXHASHES[txhash]) : GenTxid::Txid(TXHASHES[txhash]));
                     ann2.m_state = State::COMPLETED;
                     break;
                 }

src/test/txrequest_tests.cpp

@@ -221,7 +221,7 @@ class Scenario
     /** Generate a random GenTxid; the txhash follows NewTxHash; the is_wtxid flag is random. */
     GenTxid NewGTxid(const std::vector<std::vector<NodeId>>& orders = {})
     {
-        return {InsecureRandBool(), NewTxHash(orders)};
+        return InsecureRandBool() ? GenTxid::Wtxid(NewTxHash(orders)) : GenTxid::Txid(NewTxHash(orders));
     }
 
     /** Generate a new random NodeId to use as peer. The same NodeId is never returned twice
@@ -494,8 +494,8 @@ void BuildWtxidTest(Scenario& scenario, int config)
     auto peerT = scenario.NewPeer();
     auto peerW = scenario.NewPeer();
     auto txhash = scenario.NewTxHash();
-    GenTxid txid{false, txhash};
-    GenTxid wtxid{true, txhash};
+    auto txid{GenTxid::Txid(txhash)};
+    auto wtxid{GenTxid::Wtxid(txhash)};
 
     auto reqtimeT = InsecureRandBool() ? MIN_TIME : scenario.Now() + RandomTime8s();
     auto reqtimeW = InsecureRandBool() ? MIN_TIME : scenario.Now() + RandomTime8s();

src/txmempool.cpp

@@ -895,8 +895,6 @@ TxMempoolInfo CTxMemPool::info(const GenTxid& gtxid) const
     return GetInfo(i);
 }
 
-TxMempoolInfo CTxMemPool::info(const uint256& txid) const { return info(GenTxid{false, txid}); }
-
 void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeDelta)
 {
     {

src/txmempool.h

@@ -789,7 +789,6 @@ class CTxMemPool
         AssertLockHeld(cs);
         return mapTx.project<0>(mapTx.get<index_by_wtxid>().find(wtxid));
     }
-    TxMempoolInfo info(const uint256& hash) const;
     TxMempoolInfo info(const GenTxid& gtxid) const;
     std::vector<TxMempoolInfo> infoAll() const;
 

src/txrequest.cpp

@@ -300,7 +300,7 @@ std::map<uint256, TxHashInfo> ComputeTxHashInfo(const Index& index, const Priori
 
 GenTxid ToGenTxid(const Announcement& ann)
 {
-    return {ann.m_is_wtxid, ann.m_txhash};
+    return ann.m_is_wtxid ? GenTxid::Wtxid(ann.m_txhash) : GenTxid::Txid(ann.m_txhash);
 }
 
 }  // namespace

src/validation.cpp

@@ -585,10 +585,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     if (!CheckFinalTx(m_active_chainstate.m_chain.Tip(), tx, STANDARD_LOCKTIME_VERIFY_FLAGS))
         return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "non-final");
 
-    if (m_pool.exists(GenTxid(true, tx.GetWitnessHash()))) {
+    if (m_pool.exists(GenTxid::Wtxid(tx.GetWitnessHash()))) {
         // Exact transaction already exists in the mempool.
         return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-already-in-mempool");
-    } else if (m_pool.exists(GenTxid(false, tx.GetHash()))) {
+    } else if (m_pool.exists(GenTxid::Txid(tx.GetHash()))) {
         // Transaction with the same non-witness data but different witness (same txid, different
         // wtxid) already exists in the mempool.
         return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-same-nonwitness-data-in-mempool");