From 4307849256761fe2440d82bbec892d0e8e6b4dd4 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 20 Oct 2021 16:41:45 +0100 Subject: [mempool] delete exists(uint256) function Allowing callers to pass in a uint256 (which could be txid or wtxid) but then always assuming that it's a txid is a footgunny interface. --- src/net_processing.cpp | 2 +- src/node/interfaces.cpp | 2 +- src/policy/rbf.cpp | 4 ++-- src/rpc/blockchain.cpp | 2 +- src/test/mempool_tests.cpp | 36 ++++++++++++++++++------------------ src/txmempool.cpp | 4 ++-- src/txmempool.h | 3 +-- src/validation.cpp | 6 +++--- 8 files changed, 29 insertions(+), 30 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 66b99aa2bb..bb731c14a4 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3247,7 +3247,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // Always relay transactions received from peers with forcerelay // permission, even if they were already in the mempool, allowing // the node to function as a gateway for nodes hidden behind it. - if (!m_mempool.exists(tx.GetHash())) { + if (!m_mempool.exists(GenTxid::Txid(tx.GetHash()))) { LogPrintf("Not relaying non-mempool transaction %s from forcerelay peer=%d\n", tx.GetHash().ToString(), pfrom.GetId()); } else { LogPrintf("Force relaying tx %s from peer=%d\n", tx.GetHash().ToString(), pfrom.GetId()); diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 73f4036057..192caf7994 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -555,7 +555,7 @@ public: { if (!m_node.mempool) return false; LOCK(m_node.mempool->cs); - return m_node.mempool->exists(txid); + return m_node.mempool->exists(GenTxid::Txid(txid)); } bool hasDescendantsInMempool(const uint256& txid) override { diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 7ac2e22006..7e6b0cf245 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -22,7 +22,7 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) // If this transaction is not in our mempool, then we can't be sure // we will know about all its inputs. - if (!pool.exists(tx.GetHash())) { + if (!pool.exists(GenTxid::Txid(tx.GetHash()))) { return RBFTransactionState::UNKNOWN; } @@ -98,7 +98,7 @@ std::optional HasNoNewUnconfirmed(const CTransaction& tx, if (!parents_of_conflicts.count(tx.vin[j].prevout.hash)) { // Rather than check the UTXO set - potentially expensive - it's cheaper to just check // if the new input refers to a tx that's in the mempool. - if (pool.exists(tx.vin[j].prevout.hash)) { + if (pool.exists(GenTxid::Txid(tx.vin[j].prevout.hash))) { return strprintf("replacement %s adds unconfirmed input, idx %d", tx.GetHash().ToString(), j); } diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index dadd82e03f..aa7a55e7a9 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -516,7 +516,7 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool std::set setDepends; for (const CTxIn& txin : tx.vin) { - if (pool.exists(txin.prevout.hash)) + if (pool.exists(GenTxid::Txid(txin.prevout.hash))) setDepends.insert(txin.prevout.hash.ToString()); } diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index bf36f8a6c9..b3497b8ef8 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -444,12 +444,12 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) pool.addUnchecked(entry.Fee(5000LL).FromTx(tx2)); pool.TrimToSize(pool.DynamicMemoryUsage()); // should do nothing - BOOST_CHECK(pool.exists(tx1.GetHash())); - BOOST_CHECK(pool.exists(tx2.GetHash())); + BOOST_CHECK(pool.exists(GenTxid::Txid(tx1.GetHash()))); + BOOST_CHECK(pool.exists(GenTxid::Txid(tx2.GetHash()))); pool.TrimToSize(pool.DynamicMemoryUsage() * 3 / 4); // should remove the lower-feerate transaction - BOOST_CHECK(pool.exists(tx1.GetHash())); - BOOST_CHECK(!pool.exists(tx2.GetHash())); + BOOST_CHECK(pool.exists(GenTxid::Txid(tx1.GetHash()))); + BOOST_CHECK(!pool.exists(GenTxid::Txid(tx2.GetHash()))); pool.addUnchecked(entry.FromTx(tx2)); CMutableTransaction tx3 = CMutableTransaction(); @@ -462,14 +462,14 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) pool.addUnchecked(entry.Fee(20000LL).FromTx(tx3)); pool.TrimToSize(pool.DynamicMemoryUsage() * 3 / 4); // tx3 should pay for tx2 (CPFP) - BOOST_CHECK(!pool.exists(tx1.GetHash())); - BOOST_CHECK(pool.exists(tx2.GetHash())); - BOOST_CHECK(pool.exists(tx3.GetHash())); + BOOST_CHECK(!pool.exists(GenTxid::Txid(tx1.GetHash()))); + BOOST_CHECK(pool.exists(GenTxid::Txid(tx2.GetHash()))); + BOOST_CHECK(pool.exists(GenTxid::Txid(tx3.GetHash()))); pool.TrimToSize(GetVirtualTransactionSize(CTransaction(tx1))); // mempool is limited to tx1's size in memory usage, so nothing fits - BOOST_CHECK(!pool.exists(tx1.GetHash())); - BOOST_CHECK(!pool.exists(tx2.GetHash())); - BOOST_CHECK(!pool.exists(tx3.GetHash())); + BOOST_CHECK(!pool.exists(GenTxid::Txid(tx1.GetHash()))); + BOOST_CHECK(!pool.exists(GenTxid::Txid(tx2.GetHash()))); + BOOST_CHECK(!pool.exists(GenTxid::Txid(tx3.GetHash()))); CFeeRate maxFeeRateRemoved(25000, GetVirtualTransactionSize(CTransaction(tx3)) + GetVirtualTransactionSize(CTransaction(tx2))); BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), maxFeeRateRemoved.GetFeePerK() + 1000); @@ -529,19 +529,19 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) // we only require this to remove, at max, 2 txn, because it's not clear what we're really optimizing for aside from that pool.TrimToSize(pool.DynamicMemoryUsage() - 1); - BOOST_CHECK(pool.exists(tx4.GetHash())); - BOOST_CHECK(pool.exists(tx6.GetHash())); - BOOST_CHECK(!pool.exists(tx7.GetHash())); + BOOST_CHECK(pool.exists(GenTxid::Txid(tx4.GetHash()))); + BOOST_CHECK(pool.exists(GenTxid::Txid(tx6.GetHash()))); + BOOST_CHECK(!pool.exists(GenTxid::Txid(tx7.GetHash()))); - if (!pool.exists(tx5.GetHash())) + if (!pool.exists(GenTxid::Txid(tx5.GetHash()))) pool.addUnchecked(entry.Fee(1000LL).FromTx(tx5)); pool.addUnchecked(entry.Fee(9000LL).FromTx(tx7)); pool.TrimToSize(pool.DynamicMemoryUsage() / 2); // should maximize mempool size by only removing 5/7 - BOOST_CHECK(pool.exists(tx4.GetHash())); - BOOST_CHECK(!pool.exists(tx5.GetHash())); - BOOST_CHECK(pool.exists(tx6.GetHash())); - BOOST_CHECK(!pool.exists(tx7.GetHash())); + BOOST_CHECK(pool.exists(GenTxid::Txid(tx4.GetHash()))); + BOOST_CHECK(!pool.exists(GenTxid::Txid(tx5.GetHash()))); + BOOST_CHECK(pool.exists(GenTxid::Txid(tx6.GetHash()))); + BOOST_CHECK(!pool.exists(GenTxid::Txid(tx7.GetHash()))); pool.addUnchecked(entry.Fee(1000LL).FromTx(tx5)); pool.addUnchecked(entry.Fee(9000LL).FromTx(tx7)); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 5a93f30c8a..b945659c0d 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -969,7 +969,7 @@ CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set& hashes) c bool CTxMemPool::HasNoInputsOf(const CTransaction &tx) const { for (unsigned int i = 0; i < tx.vin.size(); i++) - if (exists(tx.vin[i].prevout.hash)) + if (exists(GenTxid::Txid(tx.vin[i].prevout.hash))) return false; return true; } @@ -1140,7 +1140,7 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector* pvNoSpends if (pvNoSpendsRemaining) { for (const CTransaction& tx : txn) { for (const CTxIn& txin : tx.vin) { - if (exists(txin.prevout.hash)) continue; + if (exists(GenTxid::Txid(txin.prevout.hash))) continue; pvNoSpendsRemaining->push_back(txin.prevout); } } diff --git a/src/txmempool.h b/src/txmempool.h index 460e9d0ceb..1fd0c70891 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -782,7 +782,6 @@ public: } return (mapTx.count(gtxid.GetHash()) != 0); } - bool exists(const uint256& txid) const { return exists(GenTxid{false, txid}); } CTransactionRef get(const uint256& hash) const; txiter get_iter_from_wtxid(const uint256& wtxid) const EXCLUSIVE_LOCKS_REQUIRED(cs) @@ -802,7 +801,7 @@ public: LOCK(cs); // Sanity check the transaction is in the mempool & insert into // unbroadcast set. - if (exists(txid)) m_unbroadcast_txids.insert(txid); + if (exists(GenTxid::Txid(txid))) m_unbroadcast_txids.insert(txid); }; /** Removes a transaction from the unbroadcast set */ diff --git a/src/validation.cpp b/src/validation.cpp index ff71020ebb..8938948d06 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -355,7 +355,7 @@ void CChainState::MaybeUpdateMempoolForReorg( // If the transaction doesn't make it in to the mempool, remove any // transactions that depend on it (which would now be orphans). m_mempool->removeRecursive(**it, MemPoolRemovalReason::REORG); - } else if (m_mempool->exists((*it)->GetHash())) { + } else if (m_mempool->exists(GenTxid::Txid((*it)->GetHash()))) { vHashUpdate.push_back((*it)->GetHash()); } ++it; @@ -908,7 +908,7 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) // trim mempool and check if tx was trimmed if (!bypass_limits) { LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip(), gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); - if (!m_pool.exists(hash)) + if (!m_pool.exists(GenTxid::Txid(hash))) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool full"); } return true; @@ -4500,7 +4500,7 @@ bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mocka // wallet(s) having loaded it while we were processing // mempool transactions; consider these as valid, instead of // failed, but mark them as 'already there' - if (pool.exists(tx->GetHash())) { + if (pool.exists(GenTxid::Txid(tx->GetHash()))) { ++already_there; } else { ++failed; -- cgit v1.2.3