From ed70e6501648466b9ca91a39b83775363e9a726d Mon Sep 17 00:00:00 2001
From: dergoegge <n.goeggi@gmail.com>
Date: Wed, 19 Jul 2023 15:37:29 +0200
Subject: [PATCH] Introduce types for txids & wtxids

---
 src/Makefile.am                   |  1 +
 src/net_processing.cpp            | 29 +++++++------
 src/primitives/transaction.cpp    | 16 ++++----
 src/primitives/transaction.h      | 15 +++----
 src/test/fuzz/package_eval.cpp    |  2 +-
 src/test/fuzz/tx_pool.cpp         |  4 +-
 src/util/transaction_identifier.h | 67 +++++++++++++++++++++++++++++++
 src/validation.cpp                |  4 +-
 src/wallet/transaction.h          |  4 +-
 9 files changed, 109 insertions(+), 33 deletions(-)
 create mode 100644 src/util/transaction_identifier.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 8905c0ad1cd..8508d13b349 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -328,6 +328,7 @@ BITCOIN_CORE_H = \
   util/time.h \
   util/tokenpipe.h \
   util/trace.h \
+  util/transaction_identifier.h \
   util/translation.h \
   util/types.h \
   util/ui_change_type.h \
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index dea257f7cc5..58bf3ec5d1e 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1851,9 +1851,9 @@ void PeerManagerImpl::BlockConnected(const std::shared_ptr<const CBlock>& pblock
     {
         LOCK(m_recent_confirmed_transactions_mutex);
         for (const auto& ptx : pblock->vtx) {
-            m_recent_confirmed_transactions.insert(ptx->GetHash());
+            m_recent_confirmed_transactions.insert(ptx->GetHash().ToUint256());
             if (ptx->HasWitness()) {
-                m_recent_confirmed_transactions.insert(ptx->GetWitnessHash());
+                m_recent_confirmed_transactions.insert(ptx->GetWitnessHash().ToUint256());
             }
         }
     }
@@ -2967,7 +2967,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
                 // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034
                 // for concerns around weakening security of unupgraded nodes
                 // if we start doing this too early.
-                m_recent_rejects.insert(porphanTx->GetWitnessHash());
+                m_recent_rejects.insert(porphanTx->GetWitnessHash().ToUint256());
                 // If the transaction failed for TX_INPUTS_NOT_STANDARD,
                 // then we know that the witness was irrelevant to the policy
                 // failure, since this check depends only on the txid
@@ -2979,7 +2979,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
                 if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && porphanTx->HasWitness()) {
                     // We only add the txid if it differs from the wtxid, to
                     // avoid wasting entries in the rolling bloom filter.
-                    m_recent_rejects.insert(porphanTx->GetHash());
+                    m_recent_rejects.insert(porphanTx->GetHash().ToUint256());
                 }
             }
             m_orphanage.EraseTx(orphanHash);
@@ -4230,8 +4230,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                 // regardless of what witness is provided, we will not accept
                 // this, so we don't need to allow for redownload of this txid
                 // from any of our non-wtxidrelay peers.
-                m_recent_rejects.insert(tx.GetHash());
-                m_recent_rejects.insert(tx.GetWitnessHash());
+                m_recent_rejects.insert(tx.GetHash().ToUint256());
+                m_recent_rejects.insert(tx.GetWitnessHash().ToUint256());
                 m_txrequest.ForgetTxHash(tx.GetHash());
                 m_txrequest.ForgetTxHash(tx.GetWitnessHash());
             }
@@ -4250,7 +4250,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                 // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034
                 // for concerns around weakening security of unupgraded nodes
                 // if we start doing this too early.
-                m_recent_rejects.insert(tx.GetWitnessHash());
+                m_recent_rejects.insert(tx.GetWitnessHash().ToUint256());
                 m_txrequest.ForgetTxHash(tx.GetWitnessHash());
                 // If the transaction failed for TX_INPUTS_NOT_STANDARD,
                 // then we know that the witness was irrelevant to the policy
@@ -4261,7 +4261,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                 // transactions are later received (resulting in
                 // parent-fetching by txid via the orphan-handling logic).
                 if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && tx.HasWitness()) {
-                    m_recent_rejects.insert(tx.GetHash());
+                    m_recent_rejects.insert(tx.GetHash().ToUint256());
                     m_txrequest.ForgetTxHash(tx.GetHash());
                 }
                 if (RecursiveDynamicUsage(*ptx) < 100000) {
@@ -5694,9 +5694,14 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
                     LOCK(tx_relay->m_bloom_filter_mutex);
 
                     for (const auto& txinfo : vtxinfo) {
-                        const uint256& hash = peer->m_wtxid_relay ? txinfo.tx->GetWitnessHash() : txinfo.tx->GetHash();
-                        CInv inv(peer->m_wtxid_relay ? MSG_WTX : MSG_TX, hash);
-                        tx_relay->m_tx_inventory_to_send.erase(hash);
+                        CInv inv{
+                            peer->m_wtxid_relay ? MSG_WTX : MSG_TX,
+                            peer->m_wtxid_relay ?
+                                txinfo.tx->GetWitnessHash().ToUint256() :
+                                txinfo.tx->GetHash().ToUint256(),
+                        };
+                        tx_relay->m_tx_inventory_to_send.erase(inv.hash);
+
                         // Don't send transactions that peers will not put into their mempool
                         if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) {
                             continue;
@@ -5704,7 +5709,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
                         if (tx_relay->m_bloom_filter) {
                             if (!tx_relay->m_bloom_filter->IsRelevantAndUpdate(*txinfo.tx)) continue;
                         }
-                        tx_relay->m_tx_inventory_known_filter.insert(hash);
+                        tx_relay->m_tx_inventory_known_filter.insert(inv.hash);
                         vInv.push_back(inv);
                         if (vInv.size() == MAX_INV_SZ) {
                             m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv));
diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp
index 2c913bf4327..1ad8345fcb4 100644
--- a/src/primitives/transaction.cpp
+++ b/src/primitives/transaction.cpp
@@ -12,6 +12,7 @@
 #include <tinyformat.h>
 #include <uint256.h>
 #include <util/strencodings.h>
+#include <util/transaction_identifier.h>
 #include <version.h>
 
 #include <cassert>
@@ -65,22 +66,23 @@ std::string CTxOut::ToString() const
 CMutableTransaction::CMutableTransaction() : nVersion(CTransaction::CURRENT_VERSION), nLockTime(0) {}
 CMutableTransaction::CMutableTransaction(const CTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime) {}
 
-uint256 CMutableTransaction::GetHash() const
+Txid CMutableTransaction::GetHash() const
 {
-    return (CHashWriter{SERIALIZE_TRANSACTION_NO_WITNESS} << *this).GetHash();
+    return Txid::FromUint256((CHashWriter{SERIALIZE_TRANSACTION_NO_WITNESS} << *this).GetHash());
 }
 
-uint256 CTransaction::ComputeHash() const
+Txid CTransaction::ComputeHash() const
 {
-    return (CHashWriter{SERIALIZE_TRANSACTION_NO_WITNESS} << *this).GetHash();
+    return Txid::FromUint256((CHashWriter{SERIALIZE_TRANSACTION_NO_WITNESS} << *this).GetHash());
 }
 
-uint256 CTransaction::ComputeWitnessHash() const
+Wtxid CTransaction::ComputeWitnessHash() const
 {
     if (!HasWitness()) {
-        return hash;
+        return Wtxid::FromUint256(hash.ToUint256());
     }
-    return (CHashWriter{0} << *this).GetHash();
+
+    return Wtxid::FromUint256((CHashWriter{0} << *this).GetHash());
 }
 
 CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime), hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}
diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
index bd7eb16becf..2516647a840 100644
--- a/src/primitives/transaction.h
+++ b/src/primitives/transaction.h
@@ -11,6 +11,7 @@
 #include <script/script.h>
 #include <serialize.h>
 #include <uint256.h>
+#include <util/transaction_identifier.h> // IWYU pragma: export
 
 #include <cstddef>
 #include <cstdint>
@@ -309,11 +310,11 @@ public:
 
 private:
     /** Memory only. */
-    const uint256 hash;
-    const uint256 m_witness_hash;
+    const Txid hash;
+    const Wtxid m_witness_hash;
 
-    uint256 ComputeHash() const;
-    uint256 ComputeWitnessHash() const;
+    Txid ComputeHash() const;
+    Wtxid ComputeWitnessHash() const;
 
 public:
     /** Convert a CMutableTransaction into a CTransaction. */
@@ -334,8 +335,8 @@ public:
         return vin.empty() && vout.empty();
     }
 
-    const uint256& GetHash() const { return hash; }
-    const uint256& GetWitnessHash() const { return m_witness_hash; };
+    const Txid& GetHash() const { return hash; }
+    const Wtxid& GetWitnessHash() const { return m_witness_hash; };
 
     // Return sum of txouts.
     CAmount GetValueOut() const;
@@ -405,7 +406,7 @@ struct CMutableTransaction
     /** Compute the hash of this CMutableTransaction. This is computed on the
      * fly, as opposed to GetHash() in CTransaction, which uses a cached result.
      */
-    uint256 GetHash() const;
+    Txid GetHash() const;
 
     bool HasWitness() const
     {
diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
index 4c81c0b6799..7220c5d9973 100644
--- a/src/test/fuzz/package_eval.cpp
+++ b/src/test/fuzz/package_eval.cpp
@@ -238,7 +238,7 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
         }
         if (fuzzed_data_provider.ConsumeBool()) {
             const auto& txid = fuzzed_data_provider.ConsumeBool() ?
-                                   txs.back()->GetHash() :
+                                   txs.back()->GetHash().ToUint256() :
                                    PickValue(fuzzed_data_provider, mempool_outpoints).hash;
             const auto delta = fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(-50 * COIN, +50 * COIN);
             tx_pool.PrioritiseTransaction(txid, delta);
diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
index ee73f67f660..1ba0f17cc23 100644
--- a/src/test/fuzz/tx_pool.cpp
+++ b/src/test/fuzz/tx_pool.cpp
@@ -227,7 +227,7 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
         }
         if (fuzzed_data_provider.ConsumeBool()) {
             const auto& txid = fuzzed_data_provider.ConsumeBool() ?
-                                   tx->GetHash() :
+                                   tx->GetHash().ToUint256() :
                                    PickValue(fuzzed_data_provider, outpoints_rbf).hash;
             const auto delta = fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(-50 * COIN, +50 * COIN);
             tx_pool.PrioritiseTransaction(txid, delta);
@@ -344,7 +344,7 @@ FUZZ_TARGET(tx_pool, .init = initialize_tx_pool)
         }
         if (fuzzed_data_provider.ConsumeBool()) {
             const auto& txid = fuzzed_data_provider.ConsumeBool() ?
-                                   mut_tx.GetHash() :
+                                   mut_tx.GetHash().ToUint256() :
                                    PickValue(fuzzed_data_provider, txids);
             const auto delta = fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(-50 * COIN, +50 * COIN);
             tx_pool.PrioritiseTransaction(txid, delta);
diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
new file mode 100644
index 00000000000..77f4b8367d3
--- /dev/null
+++ b/src/util/transaction_identifier.h
@@ -0,0 +1,67 @@
+#ifndef BITCOIN_UTIL_TRANSACTION_IDENTIFIER_H
+#define BITCOIN_UTIL_TRANSACTION_IDENTIFIER_H
+
+#include <uint256.h>
+#include <util/types.h>
+
+/** transaction_identifier represents the two canonical transaction identifier
+ * types (txid, wtxid).*/
+template <bool has_witness>
+class transaction_identifier
+{
+    uint256 m_wrapped;
+
+    // Note: Use FromUint256 externally instead.
+    transaction_identifier(const uint256& wrapped) : m_wrapped{wrapped} {}
+
+    // TODO: Comparisons with uint256 should be disallowed once we have
+    // converted most of the code to using the new txid types.
+    constexpr int Compare(const uint256& other) const { return m_wrapped.Compare(other); }
+    constexpr int Compare(const transaction_identifier<has_witness>& other) const { return m_wrapped.Compare(other.m_wrapped); }
+    template <typename Other>
+    constexpr int Compare(const Other& other) const
+    {
+        static_assert(ALWAYS_FALSE<Other>, "Forbidden comparison type");
+        return 0;
+    }
+
+public:
+    transaction_identifier() : m_wrapped{} {}
+
+    template <typename Other>
+    bool operator==(const Other& other) const { return Compare(other) == 0; }
+    template <typename Other>
+    bool operator!=(const Other& other) const { return Compare(other) != 0; }
+    template <typename Other>
+    bool operator<(const Other& other) const { return Compare(other) < 0; }
+
+    uint256 ToUint256() const { return m_wrapped; }
+    static transaction_identifier FromUint256(const uint256& id) { return {id}; }
+
+    /** Wrapped `uint256` methods. */
+    constexpr bool IsNull() const { return m_wrapped.IsNull(); }
+    constexpr void SetNull() { m_wrapped.SetNull(); }
+    std::string GetHex() const { return m_wrapped.GetHex(); }
+    std::string ToString() const { return m_wrapped.ToString(); }
+    constexpr const std::byte* data() const { return reinterpret_cast<const std::byte*>(m_wrapped.data()); }
+    constexpr const std::byte* begin() const { return reinterpret_cast<const std::byte*>(m_wrapped.begin()); }
+    constexpr const std::byte* end() const { return reinterpret_cast<const std::byte*>(m_wrapped.end()); }
+    template <typename Stream> void Serialize(Stream& s) const { m_wrapped.Serialize(s); }
+    template <typename Stream> void Unserialize(Stream& s) { m_wrapped.Unserialize(s); }
+
+    /** Conversion function to `uint256`.
+     *
+     * Note: new code should use `ToUint256`.
+     *
+     * TODO: This should be removed once the majority of the code has switched
+     * to using the Txid and Wtxid types. Until then it makes for a smoother
+     * transition to allow this conversion. */
+    operator uint256() const { return m_wrapped; }
+};
+
+/** Txid commits to all transaction fields except the witness. */
+using Txid = transaction_identifier<false>;
+/** Wtxid commits to all transaction fields including the witness. */
+using Wtxid = transaction_identifier<true>;
+
+#endif // BITCOIN_UTIL_TRANSACTION_IDENTIFIER_H
diff --git a/src/validation.cpp b/src/validation.cpp
index 357b4d422d2..641529e6e9e 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -616,7 +616,7 @@ private:
 
         const CTransactionRef& m_ptx;
         /** Txid. */
-        const uint256& m_hash;
+        const Txid& m_hash;
         TxValidationState m_state;
         /** A temporary cache containing serialized transaction data for signature verification.
          * Reused across PolicyScriptChecks and ConsensusScriptChecks. */
@@ -1862,7 +1862,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
     // transaction).
     uint256 hashCacheEntry;
     CSHA256 hasher = g_scriptExecutionCacheHasher;
-    hasher.Write(tx.GetWitnessHash().begin(), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin());
+    hasher.Write(UCharCast(tx.GetWitnessHash().begin()), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin());
     AssertLockHeld(cs_main); //TODO: Remove this requirement by making CuckooCache not require external locks
     if (g_scriptExecutionCache.contains(hashCacheEntry, !cacheFullScriptStore)) {
         return true;
diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h
index 1a79d7db4ed..646fb4dcd1b 100644
--- a/src/wallet/transaction.h
+++ b/src/wallet/transaction.h
@@ -319,8 +319,8 @@ public:
     bool isInactive() const { return state<TxStateInactive>(); }
     bool isUnconfirmed() const { return !isAbandoned() && !isConflicted() && !isConfirmed(); }
     bool isConfirmed() const { return state<TxStateConfirmed>(); }
-    const uint256& GetHash() const { return tx->GetHash(); }
-    const uint256& GetWitnessHash() const { return tx->GetWitnessHash(); }
+    const Txid& GetHash() const { return tx->GetHash(); }
+    const Wtxid& GetWitnessHash() const { return tx->GetWitnessHash(); }
     bool IsCoinBase() const { return tx->IsCoinBase(); }
 
     // Disable copying of CWalletTx objects to prevent bugs where instances get