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 <[email protected]>

Author: TheCharlatan

src/kernel/mempool_entry.h

@@ -71,6 +71,11 @@ class CTxMemPoolEntry
     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 @@ class CTxMemPoolEntry
           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; }

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

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