TheCharlatan
commented at 9:47 am on September 2, 2023:
contributor
Motivation
It seems preferable to use stdlib data structures instead of boost if they can achieve close to the same thing.
Code external to mempool should ideally use its public helper methods instead of accessing mapTx or its iterators directly.
Reduce the number of complex boost multi index type interactions
Also see #28335 for further context/motivation. This PR together with #28385 simplifies that one.
Overview of things done in this PR:
Make vTxHashes a vector of transaction references instead of a pair of transaction hash and iterator. The trade off here is that the data is retrieved on the fly with GetEntry instead of being cached in vTxHashes.
Introduce GetEntry helper method to replace the more involved GetIter where applicable
Replace mapTx access with CTxMemPool helper methods
Simplify checkChainLimits call in node/interfaces.cpp
Make CTxMemPoolEntrys lockPointsmutable such that they can be changed with a const iterator directly instead of going through mapTx
Make BlockAssembler’s inBlock and failedTx sets of transaction hashes.
DrahtBot
commented at 9:47 am on September 2, 2023:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#28368 (Fee Estimator updates from Validation Interface/CScheduler thread by ismaelsadeeq)
#28335 (RFC: Remove boost usage from kernel api / headers by TheCharlatan)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot added the label
Refactoring
on Sep 2, 2023
TheCharlatan renamed this:
refactor: Simplify CTxMempool / Miner fields, remove some external mapTx usage
refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx usage
on Sep 2, 2023
TheCharlatan force-pushed
on Sep 2, 2023
DrahtBot added the label
CI failed
on Sep 2, 2023
TheCharlatan renamed this:
refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx usage
refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access
on Sep 3, 2023
DrahtBot removed the label
CI failed
on Sep 4, 2023
TheCharlatan marked this as ready for review
on Sep 5, 2023
glozow
commented at 1:32 pm on September 5, 2023:
member
Concept ACK fwiw
As additional motivation, I imagine this would also make it easier to change the internal implementation of CTxMemPool without touching all these files.
TheCharlatan force-pushed
on Sep 6, 2023
DrahtBot added the label
Needs rebase
on Sep 14, 2023
TheCharlatan force-pushed
on Sep 15, 2023
DrahtBot removed the label
Needs rebase
on Sep 15, 2023
DrahtBot added the label
CI failed
on Sep 21, 2023
in
src/node/interfaces.cpp:704
in
a5bdd7eaceoutdated
in
src/test/blockencodings_tests.cpp:54
in
b0dda173e1outdated
50@@ -51,8 +51,8 @@ static CBlock BuildBlockTestCase() {
51 }
5253 // Number of shared use_counts we expect for a tx we haven't touched
54-// (block + mempool + our copy from the GetSharedTx call)
55-constexpr long SHARED_TX_OFFSET{3};
56+// (block + mempool entry + mempool txns_randomized + our copy from the GetSharedTx call)
ismaelsadeeq
commented at 5:56 pm on September 24, 2023:
In b0dda173e1784f177152349ff7002345f054f888 [refactor] rewrite vTxHashes as a vector of CTransactionRef
txns_randomized is introduced in next commit d7eec401ba7c7b4c8fc58e39e358ba4518da97d7
Does this belongs to this commit?
ismaelsadeeq
commented at 5:59 pm on September 24, 2023:
member
Concept ACK on the changes on in this PR.
This refactor all seems relevant.
The commits are really atomic and easier to review. 💯
TheCharlatan force-pushed
on Sep 25, 2023
TheCharlatan
commented at 10:56 am on September 25, 2023:
contributor
ismaelsadeeq
commented at 12:38 pm on November 1, 2023:
In 3c7543cc98efb8d5e38b8c18f2f184c9272cd092 " [refactor] remove access to mapTx in mempool_tests.cpp "
nit: In the commit message there is still mapTx access in mempool_tests.cppCheckSort test
maybe the commit message should be
[refactor] use CTxMemPool helper method to get a transaction mempool iterator.
in
src/policy/rbf.cpp:39
in
896684ec10outdated
35@@ -35,7 +36,7 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
3637 // If all the inputs have nSequence >= maxint-1, it still might be
38 // signaled for RBF if any unconfirmed parents have signaled.
39- const CTxMemPoolEntry entry{*pool.mapTx.find(tx.GetHash())};
40+ const CTxMemPoolEntry entry{**Assert(pool.GetIter(tx.GetHash()))};
ismaelsadeeq
commented at 1:36 pm on November 1, 2023:
Are you assuming that the callers of IsRBFOptIn must pass a mempool transaction with the Assert?
TheCharlatan
commented at 7:29 pm on November 1, 2023:
Yes, as I understand it that is what is checked in the code block above, so this should always hold.
ismaelsadeeq
commented at 1:50 pm on November 1, 2023:
member
Light Code review ACKe579bb98ba8977af284ba6914ffb2b1da7f34cdd
Non expert look at the changes here, there are no any external usage of CTxMemPoolmapTx.find.
The changes here looks good to me
Just a question and nits below.
I see there are still some external access of mapTx in the codebase like mapTx.size, mapTx.get and mapTx.project.
DrahtBot requested review from glozow
on Nov 1, 2023
TheCharlatan force-pushed
on Nov 1, 2023
TheCharlatan
commented at 7:48 pm on November 1, 2023:
contributor
ismaelsadeeq
commented at 12:11 pm on November 2, 2023:
member
Re ACK105a0f4db4ffdc25d3ad30300c949d46d5d8e647
in
src/node/mini_miner.cpp:77
in
bc21629f66outdated
70@@ -71,8 +71,11 @@ MiniMiner::MiniMiner(const CTxMemPool& mempool, const std::vector<COutPoint>& ou
7172 // Add every entry to m_entries_by_txid and m_entries, except the ones that will be replaced.
ismaelsadeeq
commented at 12:33 pm on November 2, 2023:
in
src/node/interfaces.cpp:809
in
105a0f4db4outdated
804@@ -806,8 +805,8 @@ class ChainImpl : public Chain
805 {
806 if (!m_node.mempool) return;
807 LOCK2(::cs_main, m_node.mempool->cs);
stickies-v
commented at 3:30 pm on November 2, 2023:
infoAll() holds its own CTxMempool::cs lock, and I think we don’t need that to add stuff to notifications? I’m also not quite sure if we need to be holding ::cs_main here?
TheCharlatan
commented at 10:54 am on November 10, 2023:
This seems a bit out of scope of this PR, but I think you are right, I don’t see the need for ::cs_main here either. Maybe open a separate PR for this?
TheCharlatan force-pushed
on Nov 3, 2023
TheCharlatan
commented at 10:24 am on November 3, 2023:
contributor
Added a new first commit to introduce a GetEntry helper for retrieving mempool entries. This allows avoiding the usage of GetIter just for retrieving a mempool entry.
Use this new GetEntry helper over GetIter where applicable.
Refactored the blockencodings_tests commit to use a simpler helper function.
Dropped commit 5633e6770538c83cc5a75cf926347a709ae7e2c0, since GetEntry is now used instead.
TheCharlatan force-pushed
on Nov 3, 2023
TheCharlatan
commented at 3:37 pm on November 3, 2023:
contributor
6b6793e6835dd2ddf367490362042bf34eb260a3: The only caller checkChainLimits of is the wallet’s CreateTransactionInternal, which will say “Transaction has too long of a mempool chain” regardless of which limit is hit.
A (wallet) followup should propagate err_string up.
Sjors
commented at 9:33 am on November 6, 2023:
member
Concept ACK. Lightly reviewed the refactoring (as of a20c1d2b81a702f97267d4f098f13ed80e9320cf) which all looks reasonable, but I’m not very familiar with mempool code.
in
src/validation.cpp:1488
in
ee79f05f1foutdated
1484@@ -1485,9 +1485,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
1485 // transactions that are already in the mempool, and only call AcceptMultipleTransactions() with
1486 // the new transactions. This ensures we don't double-count transaction counts and sizes when
1487 // checking ancestor/descendant limits, or double-count transaction fees for fee-related policy.
1488- auto iter = m_pool.GetIter(txid);
1489- assert(iter != std::nullopt);
1490- results_final.emplace(wtxid, MempoolAcceptResult::MempoolTx(iter.value()->GetTxSize(), iter.value()->GetFee()));
1491+ const CTxMemPoolEntry& entry = Assert(m_pool.GetEntry(txid)).value();
Instead of optional ref, could just use a pointer?
TheCharlatan
commented at 12:40 pm on November 6, 2023:
Yeah, I thought this would better convey its semantics, but with all the force pushes over the past two hours, it seems like it is not intuitive to work with, so I’ll revert to using a pointer.
TheCharlatan force-pushed
on Nov 6, 2023
DrahtBot removed the label
CI failed
on Nov 6, 2023
TheCharlatan force-pushed
on Nov 6, 2023
TheCharlatan
commented at 1:59 pm on November 6, 2023:
contributor
aedfef28af6e8c5bd8833fb63e2ef4f3264f934c
could use Txid for this as well
in
src/node/miner.cpp:301
in
aedfef28afoutdated
297@@ -298,7 +298,7 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
298 // because some of their txs are already in the block
299 indexed_modified_transaction_set mapModifiedTx;
300 // Keep track of entries that failed inclusion, to avoid duplicate work
301- CTxMemPool::setEntries failedTx;
302+ std::set<uint256> failedTx;
stickies-v
commented at 12:00 pm on November 9, 2023:
nit: could simplify
0const auto entry{CHECK_NONFATAL(pool.GetEntry(hash))};
in
src/policy/rbf.cpp:40
in
2be5e799baoutdated
35@@ -35,8 +36,9 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
3637 // If all the inputs have nSequence >= maxint-1, it still might be
38 // signaled for RBF if any unconfirmed parents have signaled.
39- const CTxMemPoolEntry entry{*pool.mapTx.find(tx.GetHash())};
40- auto ancestors{pool.AssumeCalculateMemPoolAncestors(__func__, entry, CTxMemPool::Limits::NoLimits(),
41+ const auto entry{pool.GetEntry(tx.GetHash())};
42+ Assert(entry != nullptr);
stickies-v
commented at 12:04 pm on November 9, 2023:
0const auto entry{Assert(pool.GetEntry(tx.GetHash()))};
in
src/txmempool.h:395
in
2be5e799baoutdated
391@@ -392,7 +392,7 @@ class CTxMemPool
392 indexed_transaction_set mapTx GUARDED_BY(cs);
393394 using txiter = indexed_transaction_set::nth_index<0>::type::const_iterator;
395- std::vector<std::pair<uint256, txiter>> vTxHashes GUARDED_BY(cs); //!< All tx witness hashes/entries in mapTx, in random order
396+ std::vector<CTransactionRef> txns_randomized GUARDED_BY(cs); //!< All transactions in mapTx, in random order
stickies-v
commented at 12:10 pm on November 9, 2023:
nit: I think a brief rationale behind why we need this change would be good to add to the commit message of “[refactor] rewrite vTxHashes as a vector of CTransactionRef”
in
src/txmempool.cpp:862
in
2be5e799baoutdated
857+ AssertLockHeld(cs);
858+ indexed_transaction_set::const_iterator i = mapTx.find(txid);
859+ if (i == mapTx.end()) {
860+ return nullptr;
861+ }
862+ return &*i;
stickies-v
commented at 12:16 pm on November 9, 2023:
nit: Given that we’re doing not any meaninful operations with i I think using auto makes sense, and the ternary operator also seems appropriate here, simplifying this to:
0const auto i{mapTx.find(txid)};
1return i == mapTx.end() ? nullptr : &(*i);
in
src/test/miniminer_tests.cpp:451
in
2be5e799baoutdated
446@@ -447,15 +447,15 @@ BOOST_FIXTURE_TEST_CASE(miniminer_overlap, TestChain100Setup)
447 // tx3's feerate is lower than tx2's. same fee, different weight.
448 BOOST_CHECK(tx2_feerate > tx3_feerate);
449 const auto tx3_anc_feerate = CFeeRate(low_fee + med_fee + high_fee + high_fee, tx_vsizes[0] + tx_vsizes[1] + tx_vsizes[2] + tx_vsizes[3]);
450- const auto tx3_iter = pool.GetIter(tx3->GetHash());
451- BOOST_CHECK(tx3_anc_feerate == CFeeRate(tx3_iter.value()->GetModFeesWithAncestors(), tx3_iter.value()->GetSizeWithAncestors()));
452+ const auto tx3_entry = pool.GetEntry(tx3->GetHash());
453+ BOOST_CHECK(tx3_anc_feerate == CFeeRate(tx3_entry->GetModFeesWithAncestors(), tx3_entry->GetSizeWithAncestors()));
stickies-v
commented at 12:23 pm on November 9, 2023:
This seems like a slight regression: tx3_iter.value() would throw if the hash was not found, whereas tx3_entry->GetSizeWithAncestors() is UB. I think it would be good to add inline Asserts to the entry initialization (and add list initialization while you’re at it)?
in
src/node/interfaces.cpp:811
in
2be5e799baoutdated
806@@ -806,8 +807,8 @@ class ChainImpl : public Chain
807 {
808 if (!m_node.mempool) return;
809 LOCK2(::cs_main, m_node.mempool->cs);
810- for (const CTxMemPoolEntry& entry : m_node.mempool->mapTx) {
811- notifications.transactionAddedToMempool(entry.GetSharedTx());
812+ for (const auto& txinfo : m_node.mempool->infoAll()) {
813+ notifications.transactionAddedToMempool(txinfo.tx);
stickies-v
commented at 12:48 pm on November 9, 2023:
It’s probably not too important, but this is a bit of a performance and memory regression to construct the vector of TxMempoolInfo when really we only need the underlying txref.
I think it could make sense to add std::vector<const CTxMemPoolEntry*> entryAll() const EXCLUSIVE_LOCKS_REQUIRED(cs); to reduce memory and computational overhead and as a bonus make the code a bit easier to read when we just want to iterate over the mempool entries?
0diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
1index a73530c142..155546c3a1 100644
2--- a/src/node/interfaces.cpp
3+++ b/src/node/interfaces.cpp
4@@ -807,8 +807,8 @@ public:
5 {
6 if (!m_node.mempool) return;
7 LOCK2(::cs_main, m_node.mempool->cs);
8- for (const auto& txinfo : m_node.mempool->infoAll()) {
9- notifications.transactionAddedToMempool(txinfo.tx);
10+ for (const auto& entry : m_node.mempool->entryAll()) {
11+ notifications.transactionAddedToMempool(Assert(entry)->GetSharedTx());
12 }
13 }
14 bool hasAssumedValidChain() override
15diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
16index 4093eb99af..82fe394b45 100644
17--- a/src/rpc/mempool.cpp
18+++ b/src/rpc/mempool.cpp
19@@ -343,16 +343,13 @@ UniValue MempoolToJSON(const CTxMemPool& pool, bool verbose, bool include_mempoo
20 }
21 LOCK(pool.cs);
22 UniValue o(UniValue::VOBJ);
23- for (const auto& txinfo : pool.infoAll()) {
24- const Txid& hash = txinfo.tx->GetHash();
25- const auto entry{pool.GetEntry(hash)};
26- CHECK_NONFATAL(entry != nullptr);
27+ for (const auto& entry : pool.entryAll()) {
28 UniValue info(UniValue::VOBJ);
29- entryToJSON(pool, info, *entry);
30+ entryToJSON(pool, info, *CHECK_NONFATAL(entry));
31 // Mempool has unique entries so there is no advantage in using
32 // UniValue::pushKV, which checks if the key already exists in O(N).
33 // UniValue::pushKVEnd is used instead which currently is O(1).
34- o.pushKVEnd(hash.ToString(), info);
35+ o.pushKVEnd(entry->GetTx().ToString(), info);
36 }
37 return o;
38 } else {
39diff --git a/src/txmempool.cpp b/src/txmempool.cpp
40index 684116c011..542c904a1a 100644
41--- a/src/txmempool.cpp
42+++ b/src/txmempool.cpp
43@@ -838,6 +838,20 @@ static TxMempoolInfo GetInfo(CTxMemPool::indexed_transaction_set::const_iterator
44 return TxMempoolInfo{it->GetSharedTx(), it->GetTime(), it->GetFee(), it->GetTxSize(), it->GetModifiedFee() - it->GetFee()};
45 }
4647+std::vector<const CTxMemPoolEntry*> CTxMemPool::entryAll() const
48+{
49+ AssertLockHeld(cs);
50+ auto iters{GetSortedDepthAndScore()};
51+
52+ std::vector<const CTxMemPoolEntry*> ret;
53+ ret.reserve(mapTx.size());
54+ for (const auto& it : iters) {
55+ ret.push_back(&(*it));
56+ }
57+
58+ return ret;
59+}
60+
61 std::vector<TxMempoolInfo> CTxMemPool::infoAll() const
62 {
63 LOCK(cs);
64diff --git a/src/txmempool.h b/src/txmempool.h
65index e15b2972a4..7eb205b2df 100644
66--- a/src/txmempool.h
67+++ b/src/txmempool.h
68@@ -697,6 +697,7 @@ public:
69 /** Returns info for a transaction if its entry_sequence < last_sequence */
70 TxMempoolInfo info_for_relay(const GenTxid& gtxid, uint64_t last_sequence) const;
7172+ std::vector<const CTxMemPoolEntry*> entryAll() const EXCLUSIVE_LOCKS_REQUIRED(cs);
73 std::vector<TxMempoolInfo> infoAll() const;
7475 size_t DynamicMemoryUsage() const;
TheCharlatan
commented at 5:01 pm on November 9, 2023:
Helpers are good, so I’ll add this patch as a separate commit.
ismaelsadeeq approved
ismaelsadeeq
commented at 1:11 pm on November 9, 2023:
member
Left a number of suggestions: some about performance improvements, some about avoiding UB, and some general style/concise nits, but they can all be done in a follow-up too.
TheCharlatan force-pushed
on Nov 9, 2023
TheCharlatan
commented at 8:57 pm on November 9, 2023:
contributor
Addressed @stickies-v’s comment, adding a new first commit implementing the suggested patch for a more efficient helper method for iterating through mempool transactions. The affected lines now retrieve a vector of CTxMemPoolEntry* through this new entryAll() instead of calling infoAll().
Addressed @stickies-v’s comment, adding a brief rationale to the message of the commit introducing vTxHashes as a vector of CTransactionRef.
Addressed @stickies-v’s comment, making GetEntry method body more concise.
Addressed @stickies-v’s comment, simplifying looping over GetMemPoolChildrenConst().
Addressed @stickies-v’s comment, explicitly calling value() on the option returned by GetIter(...).
Addressed @stickies-v’s comment, replacing the get_iter_from_wtxid call in removeUnchecked with the more widely used GetIter.
Addressed @stickies-v’s comment, replacing the for loop iterating over txns_randomized in blockencodings.cpp with a range based for loop.
DrahtBot added the label
CI failed
on Nov 9, 2023
TheCharlatan force-pushed
on Nov 9, 2023
DrahtBot removed the label
CI failed
on Nov 9, 2023
in
src/txmempool.h:700
in
f2d8e17851outdated
696@@ -695,6 +697,7 @@ class CTxMemPool
697 /** Returns info for a transaction if its entry_sequence < last_sequence */
698 TxMempoolInfo info_for_relay(const GenTxid& gtxid, uint64_t last_sequence) const;
699700+ std::vector<const CTxMemPoolEntry*> entryAll() const EXCLUSIVE_LOCKS_REQUIRED(cs);
maflcko
commented at 10:34 am on November 10, 2023:
first commit: I think here the result can not be nullptr, so a reference makes more sense, no?
diff:
0diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
1index e019ab9595..3930280797 100644
2--- a/src/node/interfaces.cpp
3+++ b/src/node/interfaces.cpp
4@@ -806,8 +806,8 @@ public:
5 {
6 if (!m_node.mempool) return;
7 LOCK2(::cs_main, m_node.mempool->cs);
8- for (const auto& entry : m_node.mempool->entryAll()) {
9- notifications.transactionAddedToMempool(Assert(entry)->GetSharedTx());
10+ for (const CTxMemPoolEntry& entry : m_node.mempool->entryAll()) {
11+ notifications.transactionAddedToMempool(entry.GetSharedTx());
12 }
13 }
14 bool hasAssumedValidChain() override
15diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
16index 3767ae0b5d..dc21385a3d 100644
17--- a/src/rpc/mempool.cpp
18+++ b/src/rpc/mempool.cpp
19@@ -345,13 +345,13 @@ UniValue MempoolToJSON(const CTxMemPool& pool, bool verbose, bool include_mempoo
20 }
21 LOCK(pool.cs);
22 UniValue o(UniValue::VOBJ);
23- for (const auto& e : pool.entryAll()) {
24+ for (const CTxMemPoolEntry& e : pool.entryAll()) {
25 UniValue info(UniValue::VOBJ);
26- entryToJSON(pool, info, *CHECK_NONFATAL(e));
27+ entryToJSON(pool, info, e);
28 // Mempool has unique entries so there is no advantage in using
29 // UniValue::pushKV, which checks if the key already exists in O(N).
30 // UniValue::pushKVEnd is used instead which currently is O(1).
31- o.pushKVEnd(e->GetTx().GetHash().ToString(), info);
32+ o.pushKVEnd(e.GetTx().GetHash().ToString(), info);
33 }
34 return o;
35 } else {
36diff --git a/src/txmempool.cpp b/src/txmempool.cpp
37index 3698374c26..e5347cbf06 100644
38--- a/src/txmempool.cpp
39+++ b/src/txmempool.cpp
40@@ -836,16 +836,15 @@ static TxMempoolInfo GetInfo(CTxMemPool::indexed_transaction_set::const_iterator
41 return TxMempoolInfo{it->GetSharedTx(), it->GetTime(), it->GetFee(), it->GetTxSize(), it->GetModifiedFee() - it->GetFee()};
42 }
4344-std::vector<const CTxMemPoolEntry*> CTxMemPool::entryAll() const
45+std::vector<std::reference_wrapper<const CTxMemPoolEntry>> CTxMemPool::entryAll() const
46 {
47 AssertLockHeld(cs);
4849- std::vector<const CTxMemPoolEntry*> ret;
50+ std::vector<std::reference_wrapper<const CTxMemPoolEntry>> ret;
51 ret.reserve(mapTx.size());
52 for (const auto& it : GetSortedDepthAndScore()) {
53- ret.push_back(&(*it));
54+ ret.push_back(*it);
55 }
56-
57 return ret;
58 }
5960diff --git a/src/txmempool.h b/src/txmempool.h
61index a4ba2c4db2..c58dbadd83 100644
62--- a/src/txmempool.h
63+++ b/src/txmempool.h
64@@ -695,7 +695,7 @@ public:
65 /** Returns info for a transaction if its entry_sequence < last_sequence */
66 TxMempoolInfo info_for_relay(const GenTxid& gtxid, uint64_t last_sequence) const;
6768- std::vector<const CTxMemPoolEntry*> entryAll() const EXCLUSIVE_LOCKS_REQUIRED(cs);
69+ std::vector<std::reference_wrapper<const CTxMemPoolEntry>> entryAll() const EXCLUSIVE_LOCKS_REQUIRED(cs);
70 std::vector<TxMempoolInfo> infoAll() const;
7172 size_t DynamicMemoryUsage() const;
TheCharlatan
commented at 10:38 am on November 10, 2023:
Thanks, will apply.
in
src/test/miniminer_tests.cpp:182
in
c14e589817outdated
178@@ -179,9 +179,9 @@ BOOST_FIXTURE_TEST_CASE(miniminer_1p1c, TestChain100Setup)
179 };
180 std::map<uint256, TxDimensions> tx_dims;
181 for (const auto& tx : all_transactions) {
182- const auto it = pool.GetIter(tx->GetHash()).value();
183- tx_dims.emplace(tx->GetHash(), TxDimensions{it->GetTxSize(), it->GetModifiedFee(),
184- CFeeRate(it->GetModifiedFee(), it->GetTxSize())});
185+ const auto entry{Assert(pool.GetEntry(tx->GetHash()))};
maflcko
commented at 10:50 am on November 10, 2023:
nit in c14e589817035e76e2707fbcd5cb99689d42de77: If you call Assert before an assignment, it could make sense to turn it into a reference by calling *Assert(). This makes later code smaller and easier to read.
in
src/validation.cpp:1486
in
c14e589817outdated
1484@@ -1485,9 +1485,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
1485 // transactions that are already in the mempool, and only call AcceptMultipleTransactions() with
1486 // the new transactions. This ensures we don't double-count transaction counts and sizes when
1487 // checking ancestor/descendant limits, or double-count transaction fees for fee-related policy.
1488- auto iter = m_pool.GetIter(txid);
1489- assert(iter != std::nullopt);
1490- results_final.emplace(wtxid, MempoolAcceptResult::MempoolTx(iter.value()->GetTxSize(), iter.value()->GetFee()));
1491+ const auto entry{Assert(m_pool.GetEntry(txid))};
maflcko
commented at 10:51 am on November 10, 2023:
same
TheCharlatan force-pushed
on Nov 10, 2023
TheCharlatan
commented at 10:52 am on November 10, 2023:
contributor
Applied @maflcko’s patch, making entryAll() return references to CTxMemPoolEntry instead of pointers.
in
src/policy/rbf.cpp:39
in
5f72a579deoutdated
35@@ -35,8 +36,8 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
3637 // If all the inputs have nSequence >= maxint-1, it still might be
38 // signaled for RBF if any unconfirmed parents have signaled.
39- const CTxMemPoolEntry entry{*pool.mapTx.find(tx.GetHash())};
40- auto ancestors{pool.AssumeCalculateMemPoolAncestors(__func__, entry, CTxMemPool::Limits::NoLimits(),
41+ const auto entry{Assert(pool.GetEntry(tx.GetHash()))};
maflcko
commented at 10:56 am on November 10, 2023:
5f72a579de74a6d911ca8529fe8754995025d61a: Same?
Also, could squash into the previous GetEntry commit?
TheCharlatan
commented at 1:14 pm on November 10, 2023:
Addressed this, but did not squash, since the change is a bit different to the others.
in
src/test/mempool_tests.cpp:298
in
6f8af55a42outdated
maflcko
commented at 11:23 am on November 10, 2023:
nit in 1598b906a57bcd5fc50c1945d8420f8ace1d9c34: Could use get or GetEntry, when only access to the tx pointer is needed, and not full access to the internal iterator?
in
src/txmempool.cpp:522
in
6635cffb1eoutdated
517@@ -518,8 +518,10 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
518 RemoveUnbroadcastTx(hash, true /* add logging because unchecked */ );
519520 if (vTxHashes.size() > 1) {
521+ // Update vTxHashesIdx of the to-be-moved entry.
522+ GetIter(vTxHashes.back()->GetHash()).value()->vTxHashesIdx = it->vTxHashesIdx;
maflcko
commented at 11:37 am on November 10, 2023:
6635cffb1e17ad5fdfdabe68cbdcaf1c68d8070d:
Not sure about turning (theoretical) UB into an exception throw. Might be better to turn it into an assert fail by using Assert(GetEntry())->?
in
src/test/validation_block_tests.cpp:286
in
91df77fde5outdated
283@@ -284,7 +284,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
284 // Check that all txs are in the pool
285 {
286 LOCK(m_node.mempool->cs);
maflcko
commented at 11:42 am on November 10, 2023:
91df77fde5adc7e2e785aa0d60b82a8515c9b538: Is the lock needed?
TheCharlatan
commented at 12:30 pm on November 10, 2023:
No, but does the other one in the while loop make sense?
in
src/txmempool.cpp:848
in
91df77fde5outdated
843+ AssertLockHeld(cs);
844+
845+ std::vector<std::reference_wrapper<const CTxMemPoolEntry>> ret;
846+ ret.reserve(mapTx.size());
847+ for (const auto& it : GetSortedDepthAndScore()) {
848+ ret.push_back(*it);
maflcko
commented at 11:44 am on November 10, 2023:
nit in the first commit:
0clang-tidy-17-p=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/txmempool.cpp
1txmempool.cpp:848:13: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
2848| ret.push_back(*it);
3|^~~~~~~~~~4| emplace_back(
maflcko
commented at 11:47 am on November 10, 2023:
member
Addressed @maflcko’s comment, using get to access transactions in mempool_tests instead of GetIter.
Addressed @maflcko’s comment, applying Assert(GetEntry(...)), instead of GetIter(...).value() to turn the potentially thrown exception into an assertion failure.
Addressed @maflcko’s comment, removing the redundant mempool.cs lock.
stickies-v
commented at 3:36 pm on November 10, 2023:
contributor
ACK3b73acb3ec678f5099aa0d77178a0b0d50670c2b
[refactor] Add helper for iterating through mempool entries
Instead of reaching into the mapTx data structure, use a helper method
that provides the required vector of CTxMemPoolEntry pointers.
453b4813eb
[refactor] Add helper for retrieving mempool entry
In places where the iterator is only needed for accessing the actual
entry, it should not be required to first retrieve the iterator.
1c6a73abbd
[refactor] remove access to mapTx from policy/rbf.cpp14804699e5
[refactor] use exists() instead of mapTx.find()9cd8cafb77
[refactor] get wtxid from entry instead of vTxHashesfad61aa561
[refactor] remove access to mapTx from rpc/mempool.cpp8892d6b744
[refactor] remove access to mapTx in blockencodings_tests.cppf80909e7a3
[refactor] remove access to mapTx.find in mempool_tests.cppdbc5bdbf59
[refactor] use CheckPackageLimits for checkChainLimits
The behavior is the same as CalculateMemPoolAncestors. The only
difference is the string returned, and the string is discarded anyway
since checkChainLimits only cares about pass/fail.
1bf4855016
[txmempool] make CTxMemPoolEntry::lockPoints mutable
Allows calling UpdateLockPoints() with a (const) txiter. Note that this
was already possible for caller using mapTx.modify(txiter). The point
here is to not be accessing mapTx when doing so.
333367a940
[refactor] remove access to mapTx in validation.cpp938643c3b2
[refactor] rewrite vTxHashes as a vector of CTransactionRef
vTxHashes exposes a complex mapTx iterator type that its external users
don't need. Directly populate it with CTransactionRef instead.
a03aef9cec
scripted-diff: rename vTxHashes to txns_randomized
DrahtBot removed review request from maflcko
on Nov 11, 2023
fanquake merged this
on Nov 13, 2023
fanquake closed this
on Nov 13, 2023
in
src/txmempool.cpp:845
in
453b4813eboutdated
840+{
841+ AssertLockHeld(cs);
842+
843+ std::vector<CTxMemPoolEntryRef> ret;
844+ ret.reserve(mapTx.size());
845+ for (const auto& it : GetSortedDepthAndScore()) {
sdaftuar
commented at 3:19 am on December 7, 2023:
Is there a reason that we are invoking GetSortedDepthAndScore() here, ie do any of the callers rely on a particular sort order?
(I’m planning to eliminate this function in #28676, so just want to understand if this behavior needs to be preserved for some reason.)
TheCharlatan
commented at 9:26 am on December 7, 2023:
This function is supposed to mimic infoAll, which was originally used in this patchset to retrieve mempool information instead of directly accessing mapTx. Looking at its usage you are right, the way I read this too there were no order promises made prior to this patch. I opened #29019 to instead just iterate through mapTx.
glozow referenced this in commit
dd391944dc
on Dec 18, 2023
glozow
commented at 4:42 pm on January 5, 2024:
member
Should we backport 453b4813ebc74859864803e9972b58e4be76a4d6?
glozow referenced this in commit
3d9f235207
on Jan 9, 2024
glozow referenced this in commit
203b92db27
on Jan 18, 2024
glozow referenced this in commit
fc62271015
on Jan 19, 2024
fanquake referenced this in commit
74df372750
on Feb 16, 2024
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-05-02 18:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
