From cb79b9dbf4cd06e17c8c65b36bf15c3ea2641de4 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 27 Jul 2020 21:30:50 -0700 Subject: [PATCH 1/4] [mempool] Revert unbroadcast set to tracking just txid When I originally implemented the unbroadcast set in 18038, it just tracked txids. After 18038 was merged, I offered a patch to 18044 to make the unbroadcast changes compatible with wtxid relay. In this patch, I updated `unbroadcast_txids` to a map of txid -> wtxid. Post merge review comments shed light on the fact that this update was unnecessary, and distracting. So, this commit updates the unbroadcast ids back to a set. --- src/net_processing.cpp | 13 +++++++------ src/node/transaction.cpp | 2 +- src/txmempool.h | 20 ++++++++++---------- src/validation.cpp | 15 ++++++--------- 4 files changed, 24 insertions(+), 26 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index ce4ac3cd75..e1007e071f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -889,15 +889,16 @@ void PeerLogicValidation::InitializeNode(CNode *pnode) { void PeerLogicValidation::ReattemptInitialBroadcast(CScheduler& scheduler) const { - std::map unbroadcast_txids = m_mempool.GetUnbroadcastTxs(); + std::set 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); } } diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 5633abe817..586bc8d2a9 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -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); diff --git a/src/txmempool.h b/src/txmempool.h index f773cd4825..62a836ce67 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -574,10 +574,9 @@ private: std::vector 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 m_unbroadcast_txids GUARDED_BY(cs); + std::set m_unbroadcast_txids GUARDED_BY(cs); public: indirectmap mapNextTx GUARDED_BY(cs); @@ -739,19 +738,20 @@ public: 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 GetUnbroadcastTxs() const { + std::set GetUnbroadcastTxs() const + { LOCK(cs); return m_unbroadcast_txids; } diff --git a/src/validation.cpp b/src/validation.cpp index 58af8744d9..e17f804859 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5116,7 +5116,7 @@ bool LoadMempool(CTxMemPool& pool) } // TODO: remove this try except in v0.22 - std::map unbroadcast_txids; + std::set unbroadcast_txids; try { file >> unbroadcast_txids; unbroadcast = unbroadcast_txids.size(); @@ -5124,13 +5124,10 @@ bool LoadMempool(CTxMemPool& pool) // 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()); @@ -5147,7 +5144,7 @@ bool DumpMempool(const CTxMemPool& pool) std::map mapDeltas; std::vector vinfo; - std::map unbroadcast_txids; + std::set unbroadcast_txids; static Mutex dump_mutex; LOCK(dump_mutex); From fc66d0a65cdc52a3b259effe0c29b5eafb1b5ff5 Mon Sep 17 00:00:00 2001 From: Adam Jonas Date: Mon, 17 Aug 2020 16:35:03 -0400 Subject: [PATCH 2/4] [p2p] Check for nullptr before dereferencing pointer --- src/net_processing.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e1007e071f..578d931505 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1509,8 +1509,9 @@ void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& { 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); From 125c0381266e0e05a408f8e1818501ab73d29110 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 24 Aug 2020 17:00:05 -0700 Subject: [PATCH 3/4] [p2p] Remove dead code The else clause is dead code because the only way to not enter the if branch is if TX_WITNESS_STRIPPED is true. In that case, it would not have a witness to match the `tx.HasWitness()` else condition. Co-authored-by: Adam Jonas Co-authored-by: John Newbery --- src/net_processing.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 578d931505..3135fcb20e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3121,8 +3121,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty if (RecursiveDynamicUsage(*ptx) < 100000) { AddToCompactExtraTransactions(ptx); } - } else if (tx.HasWitness() && RecursiveDynamicUsage(*ptx) < 100000) { - AddToCompactExtraTransactions(ptx); } if (pfrom.HasPermission(PF_FORCERELAY)) { From a8a64acaf32ac21feeb885671772282b531ef9a2 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 25 Aug 2020 10:49:38 -0700 Subject: [PATCH 4/4] [BroadcastTransaction] Remove unsafe move operator Previously, `tx` was being read after having `std::move` called on it. The std::move operator indicates to the compiler that this object may be "moved from", so we shouldn't subsequently read from it. The current code is not problematic since tx is passed in as a const ref. But this `std::move` is at best misleading & at worst problematic, so remove it. --- src/node/transaction.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 586bc8d2a9..9ae4700743 100644 --- a/src/node/transaction.cpp +++ b/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) {