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).
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28843.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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.
lgtm
Code review ACK ed71a7b9e1e67d6155fe1def5acf51beb12c9baf
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);
Why aren't you just changing addPackageTxs to use m_mempool directly? Seems simpler, and doesn't change the public api. Something like https://github.com/ajtowns/bitcoin/commit/944d8bc87690389de9bd51a6ea8a3223d10e4002
Looking at the review discussion in #25223 introducing a check in the function and using the arrow operator feels like a step back in code quality. I also feel like having raw pointers as class members is something we should try to avoid, since its ownership semantics are ambiguous. Relying on the member variable precludes this later comment on the one I linked in the PR description by making a refactor into fully static functions harder.
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...)
Rebased ed71a7b9e1e67d6155fe1def5acf51beb12c9baf -> a323388036d4f8b9fe8ff0915252f20abb91633e (blockAssemblerRemoveMempool_0 -> blockAssemblerRemoveMempool_1, compare)
Concept ACK
Reworked a323388036d4f8b9fe8ff0915252f20abb91633e -> 247ae848e669cc36c790362b766ff813cdfa3e66 (blockAssemblerRemoveMempool_1 -> blockAssemblerRemoveMempool_2, compare)
mempool argument instead of the m_mempool member.
ACK 60a0f2ac88c57fdf4482dade2ce409ef2da65998
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?
<details> <summary>git diff on 60a0f2ac88</summary>
diff --git a/src/node/miner.cpp b/src/node/miner.cpp
index 9e6954d257..afa410da10 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -141,7 +141,9 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
int nPackagesSelected = 0;
int nDescendantsUpdated = 0;
- addPackageTxs(nPackagesSelected, nDescendantsUpdated);
+ if (m_mempool) {
+ addPackageTxs(nPackagesSelected, nDescendantsUpdated);
+ }
const auto time_1{SteadyClock::now()};
@@ -291,9 +293,7 @@ void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, std::ve
// transaction package to work on next.
void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated)
{
- if (m_mempool == nullptr) return;
-
- const auto& mempool{*m_mempool};
+ const auto& mempool{*Assert(m_mempool)};
LOCK(mempool.cs);
// mapModifiedTx will store sorted packages after they are modified
diff --git a/src/node/miner.h b/src/node/miner.h
index a262fb7dc5..25ce110b34 100644
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -188,7 +188,9 @@ private:
/** Add transactions based on feerate including unconfirmed ancestors
* Increments nPackagesSelected / nDescendantsUpdated with corresponding
* statistics from the package selection (for logging statistics).
- * No-op if the BlockAssembler has been configured without a mempool.*/
+ *
+ * [@pre](/bitcoin-bitcoin/contributor/pre/) BlockAssembler::m_mempool must not be nullptr
+ */
void addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(!m_mempool->cs);
// helper functions for addPackageTxs()
</details>
Approach ACK, code LGTM 60a0f2ac88c57fdf4482dade2ce409ef2da65998 modulo my comment about no-op. I don't feel particularly strong about the current vs the previous approach, but this has the benefit of being a much smaller change, and it works well.
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.
re-ACK 192dac1
ACK 192dac1d3370edd579db235d69c034726f37c8da
ACK 192dac1d3370edd579db235d69c034726f37c8da