From 705e3f1de00bf30d728addd52a790a139d948e32 Mon Sep 17 00:00:00 2001
From: TheCharlatan <seb.kung@gmail.com>
Date: Fri, 17 Nov 2023 20:28:14 +0100
Subject: [PATCH] refactor: Make CTxMemPoolEntry only explicitly copyable

This has the goal of prohibiting users from accidentally creating
runtime failures, e.g. by interacting with iterator_to with a copied
entry.

CTxMemPoolEntry is already implicitly not move-constructable. So be
explicit about this and use a std::list to collect the values in the
policy_estimator fuzz test instead of a std::vector.

Co-authored-by: Anthony Towns <aj@erisian.com.au>
---
 src/kernel/mempool_entry.h         | 12 ++++++++++++
 src/test/fuzz/policy_estimator.cpp |  4 ++--
 src/txmempool.cpp                  |  2 +-
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h
index 7c905ca4f4a..b5c04990123 100644
--- a/src/kernel/mempool_entry.h
+++ b/src/kernel/mempool_entry.h
@@ -71,6 +71,11 @@ public:
     typedef std::set<CTxMemPoolEntryRef, CompareIteratorByHash> Children;
 
 private:
+    CTxMemPoolEntry(const CTxMemPoolEntry&) = default;
+    struct ExplicitCopyTag {
+        explicit ExplicitCopyTag() = default;
+    };
+
     const CTransactionRef tx;
     mutable Parents m_parents;
     mutable Children m_children;
@@ -122,6 +127,13 @@ public:
           nModFeesWithAncestors{nFee},
           nSigOpCostWithAncestors{sigOpCost} {}
 
+    CTxMemPoolEntry(ExplicitCopyTag, const CTxMemPoolEntry& entry) : CTxMemPoolEntry(entry) {}
+    CTxMemPoolEntry& operator=(const CTxMemPoolEntry&) = delete;
+    CTxMemPoolEntry(CTxMemPoolEntry&&) = delete;
+    CTxMemPoolEntry& operator=(CTxMemPoolEntry&&) = delete;
+
+    static constexpr ExplicitCopyTag ExplicitCopy{};
+
     const CTransaction& GetTx() const { return *this->tx; }
     CTransactionRef GetSharedTx() const { return this->tx; }
     const CAmount& GetFee() const { return nFee; }
diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp
index a6275d2fd26..ab885043d7e 100644
--- a/src/test/fuzz/policy_estimator.cpp
+++ b/src/test/fuzz/policy_estimator.cpp
@@ -49,7 +49,7 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator)
                 }
             },
             [&] {
-                std::vector<CTxMemPoolEntry> mempool_entries;
+                std::list<CTxMemPoolEntry> mempool_entries;
                 LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000)
                 {
                     const std::optional<CMutableTransaction> mtx = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider, TX_WITH_WITNESS);
@@ -58,7 +58,7 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator)
                         break;
                     }
                     const CTransaction tx{*mtx};
-                    mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx));
+                    mempool_entries.emplace_back(CTxMemPoolEntry::ExplicitCopy, ConsumeTxMemPoolEntry(fuzzed_data_provider, tx));
                 }
                 std::vector<const CTxMemPoolEntry*> ptrs;
                 ptrs.reserve(mempool_entries.size());
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index e057d7ece1c..f6b80514518 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -438,7 +438,7 @@ void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAnces
     // Add to memory pool without checking anything.
     // Used by AcceptToMemoryPool(), which DOES do
     // all the appropriate checks.
-    indexed_transaction_set::iterator newit = mapTx.insert(entry).first;
+    indexed_transaction_set::iterator newit = mapTx.emplace(CTxMemPoolEntry::ExplicitCopy, entry).first;
 
     // Update transaction for any feeDelta created by PrioritiseTransaction
     CAmount delta{0};