refactor: Make m_mempool optional in PeerManager #26247

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

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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

<!--174a7506f384e20aa4161008e828411d-->

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.

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?

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.

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.

Wondering if people here would have an opinion on #26471?

<!--cf906140f33d8803c4a75a2196329ecb-->

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