[refactor] Remove BlockAssembler m_mempool member #28843

1. 
   TheCharlatan commented at 11:06 am on November 10, 2023: contributor

   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 (review).
2. 
   DrahtBot commented at 11:06 am on November 10, 2023: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	stickies-v
   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.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30200 (Introduce Mining interface by Sjors)

   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.

3. maflcko approved
4. 
   maflcko commented at 11:10 am on November 10, 2023: member

   lgtm
5. TheCharlatan marked this as ready for review on Nov 10, 2023
6. dergoegge approved
7. 
   dergoegge commented at 1:27 pm on November 10, 2023: member

   Code review ACK ed71a7b9e1e67d6155fe1def5acf51beb12c9baf
8. in src/node/miner.cpp:143 in ed71a7b9e1 outdated

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

   ---

   ---

   

   ajtowns commented at 0:23 am on November 11, 2023:

   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

   ---

   

   TheCharlatan commented at 8:27 am on November 11, 2023:

   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.

   ---

   

   ajtowns commented at 1:18 am on November 12, 2023:

   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…)

9. DrahtBot added the label Needs rebase on Dec 14, 2023
10. [refactor] Remove BlockAssembler m_mempool member

    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.

    a323388036
11. TheCharlatan force-pushed on Mar 13, 2024
12. 
    TheCharlatan commented at 11:15 am on March 13, 2024: contributor

    Rebased ed71a7b9e1e67d6155fe1def5acf51beb12c9baf -> a323388036d4f8b9fe8ff0915252f20abb91633e (blockAssemblerRemoveMempool_0 -> blockAssemblerRemoveMempool_1, compare)
13. DrahtBot removed the label Needs rebase on Mar 13, 2024
14. 
    stickies-v commented at 3:07 pm on March 15, 2024: contributor

    Concept ACK
15. 
    DrahtBot commented at 0:06 am on June 25, 2024: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

16. DrahtBot added the label Needs rebase on Jun 25, 2024

Contributors
TheCharlatan DrahtBot maflcko dergoegge ajtowns stickies-v

Labels
Needs rebase