Merge bitcoin#19879: [p2p] miscellaneous wtxid followups

a8a64ac [BroadcastTransaction] Remove unsafe move operator (Amiti Uttarwar)
125c038 [p2p] Remove dead code (Amiti Uttarwar)
fc66d0a [p2p] Check for nullptr before dereferencing pointer (Adam Jonas)
cb79b9d [mempool] Revert unbroadcast set to tracking just txid (Amiti Uttarwar)

Pull request description:

  Addresses some outstanding review comments from bitcoin#18044

  - reverts unbroadcast txids to a set instead of a map (simpler, communicates intent better, takes less space, no efficiency advantages of map)
  - adds safety around two touchpoints (check for nullptr before dereferencing pointer, remove an inaccurate std::move operator)
  - removes some dead code

  Links to comments on wtxid PR: [1](bitcoin#18044 (comment)) [2](bitcoin#18044 (comment)) [3](bitcoin#18044 (comment))

  thanks to jnewbery & adamjonas for flagging these ! !

ACKs for top commit:
  sdaftuar:
    utACK a8a64ac
  naumenkogs:
    utACK a8a64ac
  jnewbery:
    utACK a8a64ac

Tree-SHA512: 7be669cb30cc17fb9e06b50e636ef7887c6a27354697987e4e4d38dba4b8f50e175647587430cd9bc3295bec01ce8b1e6639a50a4249d8fff9b1ca1b9ead3277

Author: fanquake

src/net_processing.cpp

@@ -1035,15 +1035,16 @@ void PeerManager::InitializeNode(CNode *pnode) {
 
 void PeerManager::ReattemptInitialBroadcast(CScheduler& scheduler) const
 {
-    std::map<uint256, uint256> unbroadcast_txids = m_mempool.GetUnbroadcastTxs();
+    std::set<uint256> unbroadcast_txids = m_mempool.GetUnbroadcastTxs();
 
-    for (const auto& elem : unbroadcast_txids) {
-        // Sanity check: all unbroadcast txns should exist in the mempool
-        if (m_mempool.exists(elem.first)) {
+    for (const auto& txid : unbroadcast_txids) {
+        CTransactionRef tx = m_mempool.get(txid);
+
+        if (tx != nullptr) {
             LOCK(cs_main);
-            RelayTransaction(elem.first, elem.second, m_connman);
+            RelayTransaction(txid, tx->GetWitnessHash(), m_connman);
         } else {
-            m_mempool.RemoveUnbroadcastTx(elem.first, true);
+            m_mempool.RemoveUnbroadcastTx(txid, true);
         }
     }
 
@@ -1676,8 +1677,9 @@ void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman&
         if (!pnode->fMasternode)
         {
             LockAssertion lock(::cs_main);
-            CNodeState &state = *State(pnode->GetId());
-            if (state.m_wtxid_relay) {
+            CNodeState* state = State(pnode->GetId());
+            if (state == nullptr) return;
+            if (state->m_wtxid_relay) {
                 pnode->PushTxInventory(wtxid);
             } else {
                 pnode->PushTxInventory(txid);
@@ -3501,8 +3503,6 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
                 if (RecursiveDynamicUsage(*ptx) < 100000) {
                     AddToCompactExtraTransactions(ptx);
                 }
-            } else if (tx.HasWitness() && RecursiveDynamicUsage(*ptx) < 100000) {
-                AddToCompactExtraTransactions(ptx);
             }
 
             if (pfrom.HasPermission(PF_FORCERELAY)) {

src/node/transaction.cpp

@@ -38,8 +38,8 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
     if (!node.mempool->exists(hashTx)) {
         // Transaction is not already in the mempool. Submit it.
         TxValidationState state;
-        if (!AcceptToMemoryPool(*node.mempool, state, std::move(tx),
-                nullptr /* plTxnReplaced */, false /* bypass_limits */, max_tx_fee)) {
+        if (!AcceptToMemoryPool(*node.mempool, state, tx,
+                                nullptr /* plTxnReplaced */, false /* bypass_limits */, max_tx_fee)) {
             err_string = state.ToString();
             if (state.IsInvalid()) {
                 if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {
@@ -80,7 +80,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
     if (relay) {
         // the mempool tracks locally submitted transactions to make a
         // best-effort of initial broadcast
-        node.mempool->AddUnbroadcastTx(hashTx, tx->GetWitnessHash());
+        node.mempool->AddUnbroadcastTx(hashTx);
 
         LOCK(cs_main);
         RelayTransaction(hashTx, tx->GetWitnessHash(), *node.connman);

src/txmempool.h

@@ -607,10 +607,9 @@ class CTxMemPool
     std::vector<indexed_transaction_set::const_iterator> GetSortedDepthAndScore() const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     /**
-     * track locally submitted transactions to periodically retry initial broadcast
-     * map of txid -> wtxid
+     * Track locally submitted transactions to periodically retry initial broadcast.
      */
-    std::map<uint256, uint256> m_unbroadcast_txids GUARDED_BY(cs);
+    std::set<uint256> m_unbroadcast_txids GUARDED_BY(cs);
     // SYSCOIN
     std::multimap<uint256, uint256> mapProTxRefs; // proTxHash -> transaction (all TXs that refer to an existing proTx)
     std::map<CService, uint256> mapProTxAddresses;
@@ -787,19 +786,20 @@ class CTxMemPool
     size_t DynamicMemoryUsage() const;
 
     /** Adds a transaction to the unbroadcast set */
-    void AddUnbroadcastTx(const uint256& txid, const uint256& wtxid) {
+    void AddUnbroadcastTx(const uint256& txid)
+    {
         LOCK(cs);
-        // Sanity Check: the transaction should also be in the mempool
-        if (exists(txid)) {
-            m_unbroadcast_txids[txid] = wtxid;
-        }
-    }
+        // Sanity check the transaction is in the mempool & insert into
+        // unbroadcast set.
+        if (exists(txid)) m_unbroadcast_txids.insert(txid);
+    };
 
     /** Removes a transaction from the unbroadcast set */
     void RemoveUnbroadcastTx(const uint256& txid, const bool unchecked = false);
 
     /** Returns transactions in unbroadcast set */
-    std::map<uint256, uint256> GetUnbroadcastTxs() const {
+    std::set<uint256> GetUnbroadcastTxs() const
+    {
         LOCK(cs);
         return m_unbroadcast_txids;
     }

src/validation.cpp

@@ -5470,21 +5470,18 @@ bool LoadMempool(CTxMemPool& pool)
         }
 
         // TODO: remove this try except in v0.22
-        std::map<uint256, uint256> unbroadcast_txids;
+        std::set<uint256> unbroadcast_txids;
         try {
           file >> unbroadcast_txids;
           unbroadcast = unbroadcast_txids.size();
         } catch (const std::exception&) {
           // mempool.dat files created prior to v0.21 will not have an
           // unbroadcast set. No need to log a failure if parsing fails here.
         }
-        for (const auto& elem : unbroadcast_txids) {
-            // Don't add unbroadcast transactions that didn't get back into the
-            // mempool.
-            const CTransactionRef& added_tx = pool.get(elem.first);
-            if (added_tx != nullptr) {
-                pool.AddUnbroadcastTx(elem.first, added_tx->GetWitnessHash());
-            }
+        for (const auto& txid : unbroadcast_txids) {
+            // Ensure transactions were accepted to mempool then add to
+            // unbroadcast set.
+            if (pool.get(txid) != nullptr) pool.AddUnbroadcastTx(txid);
         }
     } catch (const std::exception& e) {
         LogPrintf("Failed to deserialize mempool data on disk: %s. Continuing anyway.\n", e.what());
@@ -5501,7 +5498,7 @@ bool DumpMempool(const CTxMemPool& pool)
 
     std::map<uint256, CAmount> mapDeltas;
     std::vector<TxMempoolInfo> vinfo;
-    std::map<uint256, uint256> unbroadcast_txids;
+    std::set<uint256> unbroadcast_txids;
 
     static Mutex dump_mutex;
     LOCK(dump_mutex);