[refactor] Remove BlockAssembler m_mempool member #28843
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
| @@ -138,9 +137,9 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc | |||
|
|
|||
| int nPackagesSelected = 0; | |||
| int nDescendantsUpdated = 0; | |||
| if (m_mempool) { | |||
| LOCK(m_mempool->cs); | |||
| addPackageTxs(*m_mempool, nPackagesSelected, nDescendantsUpdated); | |||
There was a problem hiding this comment.
Why aren't you just changing addPackageTxs to use m_mempool directly? Seems simpler, and doesn't change the public api. Something like ajtowns@944d8bc
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...)
Instead, pass the mempool pointer as an argument to CreateNewBlock and pass that on to addPackageTxs. Before, addPackageTxs had access to both the m_mempool member and the passed in mempool, which could be confusing.
TheCharlatan
force-pushed
the
blockAssemblerRemoveMempool
branch
from
ed71a7b
to
a323388
Compare
|
Rebased ed71a7b -> a323388 (blockAssemblerRemoveMempool_0 -> blockAssemblerRemoveMempool_1, compare) |
|
Concept ACK |
Instead, pass the mempool pointer as an argument to CreateNewBlock and pass that on to addPackageTxs. Before, addPackageTxs had access to both the m_mempool member and the passed in mempool, which could be confusing. This was noticed in this PR review: #25223 (comment).