foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-03 09:56:38 -05:00
Code Issues Activity

Merge bitcoin/bitcoin#23325: mempool: delete exists(uint256) function

Browse source 
4307849256 [mempool] delete exists(uint256) function (glozow)
d50fbd4c5b create explicit GenTxid::{Txid, Wtxid} ctors (glozow)

Pull request description:

  We use the same type for txids and wtxids, `uint256`. In places where we want the ability to pass either one, we distinguish them using `GenTxid`.

  The (overloaded) `CTxMemPool::exists()` function is defined as follows:
  ```c
  bool exists(const uint256& txid) const { return exists(GenTxid{false, txid}); }
  ```
  It always assumes that a uint256 is a txid, which is a footgunny interface.
  Querying by wtxid returns a false negative if the transaction has a witness. 🐛

  Another approach would be to try both:
  ```c
  bool exists(const uint256& txid) const { return exists(GenTxid{false, txid}) || exists(GenTxid{false, txid}); }
  ```
  But that's slower and wrongfully placing the burden on the callee; the caller always knows whether the hash is a txid or a wtxid.

ACKs for top commit:
  laanwj:
    Code review ACK 4307849256
  jnewbery:
    Tested and code review ACK 4307849256
  MarcoFalke:
    review ACK 4307849256 👘

Tree-SHA512: 8ed167a96f3124b6c14e41073c8358658114ce121a15a4cca2db7a5ac565903a6236e34e88ac03382b8bb8b68e3999abbfc5718bc8c22476554d6b49a5298eec
This commit is contained in:
MarcoFalke 2021-10-22 12:29:23 +02:00
commit c001da306b
No known key found for this signature in database
GPG key ID: CE2B75697E69A548
9 changed files with 31 additions and 30 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/net_processing.cpp
View file

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

2
src/node/interfaces.cpp
View file

@ -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
{

4
src/policy/rbf.cpp
View file

@ -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<std::string> 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);
}

2
src/primitives/transaction.h
View file

@ -393,6 +393,8 @@ class GenTxid
uint256 m_hash;
public:
GenTxid(bool is_wtxid, const uint256& hash) : m_is_wtxid(is_wtxid), m_hash(hash) {}
static GenTxid Txid(const uint256& hash) { return GenTxid{false, hash}; }
static GenTxid Wtxid(const uint256& hash) { return GenTxid{true, hash}; }
bool IsWtxid() const { return m_is_wtxid; }
const uint256& GetHash() const { return m_hash; }
friend bool operator==(const GenTxid& a, const GenTxid& b) { return a.m_is_wtxid == b.m_is_wtxid && a.m_hash == b.m_hash; }

2
src/rpc/blockchain.cpp
View file

@ -516,7 +516,7 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool
std::set<std::string> 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());
}

36
src/test/mempool_tests.cpp
View file

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

4
src/txmempool.cpp
View file

@ -969,7 +969,7 @@ CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>& 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<COutPoint>* 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);
}
}

3
src/txmempool.h
View file

@ -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 */

6
src/validation.cpp
View file

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