Merge bitcoin#28903: refactor: Make CTxMemPoolEntry only explicitly copyable

705e3f1 refactor: Make CTxMemPoolEntry only explicitly copyable (TheCharlatan)

Pull request description:

  This has the goal of prohibiting users from accidentally creating runtime failures, e.g. by interacting with iterator_to with a copied entry. This was brought up here:  bitcoin#28886 (comment).

  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.

ACKs for top commit:
  maflcko:
    ACK 705e3f1 🌯
  achow101:
    ACK 705e3f1
  ajtowns:
    ACK 705e3f1
  ismaelsadeeq:
    ACK 705e3f1

Tree-SHA512: 62056905c679c919d00f9ae065ed66ac986e7e7062015aea542843d8deecda57104d7a68d002f7b20afa3164f8e9215d2d2d002c167224129540e3b1bd0712cc

Author: achow101

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

@@ -50,7 +50,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);
@@ -59,7 +59,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};