refactor: Make m_mempool optional in PeerManager #26247

1. 
   maflcko commented at 1:43 pm on October 4, 2022: member

   Make m_mempool an optional pointer in PeerManager, instead of a reference.

   • This is a prerequisite for the introduction of a -nomempool option. (To run a node with just libbitcoinkernel and a minimal P2P stack to receive blocks)
   • The mempool is already treated optional in pretty much every other part of the codebase (validation, RPC, …), so doing it consistently is preferable.
   • Removing the requirement of PeerManager to hold a reference to a mempool makes (unit/fuzz) testing easier if there is a test that doesn’t need the mempool. Also, it makes the coupling of CTxMemPool and PeerManager a bit more loose.

   Picked up from #22850

2. DrahtBot added the label Refactoring on Oct 4, 2022
3. 
   DrahtBot commented at 8:26 pm on October 4, 2022: contributor

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

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #26969 (net: net_processing, add ProcessCompactBlockTxns by brunoerg)
   • #26900 (refactor: Add BlockManager getters by MarcoFalke)
   • #26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
   • #25781 (Remove almost all blockstorage globals by MarcoFalke)
   • #25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
   • #25268 (refactor: Introduce EvictionManager by dergoegge)

   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.

4. 
   ajtowns commented at 3:49 am on October 5, 2022: contributor

   What’s the benefit of a -nomempool option? There doesn’t seem to be an open issue justifying it. This approach just seems to introduce a lot of places where you need to Assert(m_mempool) or if (!m_mempool) return; – ie, introduces potential bugs.

   If the goal is to limit memory usage by avoiding having 300MB dedicated to the mempool (which might be filled up during a large reorg, even if you don’t accept txs normally), wouldn’t it be better to still have the mempool data structure, and at most add an option that guarantees the mempool will remain empty by ignoring attempts to add txs to it; ie a const bool always_empty; in CTxMemPool or similar?

5. 
   ariard commented at 0:32 am on October 7, 2022: member

   What’s the benefit of a -nomempool option? There doesn’t seem to be an open issue justifying it. This approach just seems to introduce a lot of places where you need to Assert(m_mempool) or if (!m_mempool) return; – ie, introduces potential bugs.

   Embedded systems where the binary size matters and you would like to run a bitcoin-node daemon only connected to a stateful signer? Assuming you can also cut out the p2p stack. However, if this is a relevant use-case, the nomempool option could be introduced at the build-level, like --disable-mempool.

6. maflcko marked this as a draft on Oct 7, 2022
7. maflcko added the label Brainstorming on Oct 7, 2022
8. 
   maflcko commented at 1:32 pm on October 7, 2022: member

   Code wise there seems to be a preference to have a pointer than an empty struct, see also #22415 and #25223 . (Updated OP with motivation)

   Though, I agree that the code doesn’t look particularly nice. I was thinking about turning RejectIncomingTxs into a helper that returns a pointer to the mempool (if the peer is allowed to “see” the mempool), and similar methods for the other places where a mempool is used.

9. DrahtBot added the label Needs rebase on Oct 13, 2022
10. maflcko force-pushed on Oct 18, 2022
11. DrahtBot removed the label Needs rebase on Oct 18, 2022
12. DrahtBot added the label Needs rebase on Oct 21, 2022
13. 
    glozow commented at 4:10 pm on January 19, 2023: member

    Wondering if people here would have an opinion on #26471?
14. maflcko force-pushed on Jan 19, 2023
15. maflcko force-pushed on Jan 19, 2023
16. DrahtBot removed the label Needs rebase on Jan 19, 2023
17. DrahtBot added the label Needs rebase on Jan 26, 2023
18. Make m_mempool optional in PeerManager

    - Make m_mempool a pointer in the initialization of PeerManager
    - Check that m_mempool exists before dereferencing the pointer in PeerManager functions
    - If m_mempool is a nullptr, turn off transaction relay with all peers by setting m_tx_relay to nullptr

    f413f4061d
19. maflcko force-pushed on Jan 26, 2023
20. DrahtBot removed the label Needs rebase on Jan 26, 2023
21. 
    DrahtBot commented at 5:22 pm on January 27, 2023: contributor

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

22. DrahtBot added the label Needs rebase on Jan 27, 2023
23. maflcko closed this on Feb 20, 2023

    ---

24. maflcko deleted the branch on Feb 20, 2023
25. bitcoin locked this on Feb 20, 2024

Contributors
maflcko DrahtBot ajtowns ariard glozow

Labels
Brainstorming Refactoring Needs rebase