The addPackageTxs method of the BlockAssembler currently has access to two mempool variables, as an argument and as a member. Clean this up and clarify that they both are the same mempool instance by removing the argument and instead only using the member variable in the method.
This was noticed in this PR review: #25223 (review).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28843.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | danielabrozzoni, stickies-v, achow101 |
| Stale ACK | dergoegge |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
136@@ -138,9 +137,9 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
137
138 int nPackagesSelected = 0;
139 int nDescendantsUpdated = 0;
140- if (m_mempool) {
141- LOCK(m_mempool->cs);
142- addPackageTxs(*m_mempool, nPackagesSelected, nDescendantsUpdated);
addPackageTxs to use m_mempool directly? Seems simpler, and doesn’t change the public api. Something like https://github.com/ajtowns/bitcoin/commit/944d8bc87690389de9bd51a6ea8a3223d10e4002
I could see that being an argument for doing away with BlockAssembler entirely (at least as a public interface), and replacing BlockAssembler(chainstate, mempool, options).CreateNewBlock(scriptPubKeyIn) it with a function: CreateNewBlock(chainstate, mempool, scriptPubKeyIn, options) (effectively reverting #7598).
But if you’re keeping BlockAssembler around, the entire point of that class is to assemble a block with txs from the mempool – that’s why most of its private functions require CTxMemPool::setEntries references, eg – so it seems pretty strange to make it clumsier at doing that by pulling data it needs to do its job out of the class itself.
(Unrelated, but also seems a bit bogus to call the m_last_block_* things “members” when they’re really globals…)
Reworked a323388036d4f8b9fe8ff0915252f20abb91633e -> 247ae848e669cc36c790362b766ff813cdfa3e66 (blockAssemblerRemoveMempool_1 -> blockAssemblerRemoveMempool_2, compare)
mempool argument instead of the m_mempool member.
186@@ -187,8 +187,9 @@ class BlockAssembler
187 // Methods for how to add transactions to a block.
188 /** Add transactions based on feerate including unconfirmed ancestors
189 * Increments nPackagesSelected / nDescendantsUpdated with corresponding
190- * statistics from the package selection (for logging statistics). */
191- void addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs);
192+ * statistics from the package selection (for logging statistics).
193+ * No-op if the BlockAssembler has been configured without a mempool.*/
I feel a bit uneasy about adding this no-op, it makes understanding the program flow more difficult and I feel like it is a potential footgun. What do you think about asserting the mempool is configured instead?
0diff --git a/src/node/miner.cpp b/src/node/miner.cpp
1index 9e6954d257..afa410da10 100644
2--- a/src/node/miner.cpp
3+++ b/src/node/miner.cpp
4@@ -141,7 +141,9 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
5
6 int nPackagesSelected = 0;
7 int nDescendantsUpdated = 0;
8- addPackageTxs(nPackagesSelected, nDescendantsUpdated);
9+ if (m_mempool) {
10+ addPackageTxs(nPackagesSelected, nDescendantsUpdated);
11+ }
12
13 const auto time_1{SteadyClock::now()};
14
15@@ -291,9 +293,7 @@ void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, std::ve
16 // transaction package to work on next.
17 void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated)
18 {
19- if (m_mempool == nullptr) return;
20-
21- const auto& mempool{*m_mempool};
22+ const auto& mempool{*Assert(m_mempool)};
23 LOCK(mempool.cs);
24
25 // mapModifiedTx will store sorted packages after they are modified
26diff --git a/src/node/miner.h b/src/node/miner.h
27index a262fb7dc5..25ce110b34 100644
28--- a/src/node/miner.h
29+++ b/src/node/miner.h
30@@ -188,7 +188,9 @@ private:
31 /** Add transactions based on feerate including unconfirmed ancestors
32 * Increments nPackagesSelected / nDescendantsUpdated with corresponding
33 * statistics from the package selection (for logging statistics).
34- * No-op if the BlockAssembler has been configured without a mempool.*/
35+ *
36+ * [@pre](/bitcoin-bitcoin/contributor/pre/) BlockAssembler::m_mempool must not be nullptr
37+ */
38 void addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(!m_mempool->cs);
39
40 // helper functions for addPackageTxs()
The `addPackageTxs` method of the `BlockAssembler` currently has access
to two mempool variables, as an argument and as a member. Clean this up
and clarify that they both are the same mempool instance by removing the
argument and instead only using the member variable in the method.
Co-Authored-By: Anthony Towns <aj@erisian.com.au>
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Updated 60a0f2ac88c57fdf4482dade2ce409ef2da65998 -> 192dac1d3370edd579db235d69c034726f37c8da (blockAssemblerRemoveMempool_2 -> blockAssemblerRemoveMempool_3, compare)
BlockAssembler assert instead of immediately returning if no mempool is present.
