net: make m_mempool optional in PeerManager

naiyoma commented at 4:08 pm on July 21, 2025: contributor

This PR is a continuation of →https://github.com/bitcoin/bitcoin/pull/22850, to make m_mempool an optional pointer in PeerManager instead of a reference.

some benefits, as mentioned here:https://github.com/bitcoin/bitcoin/pull/26247#issue-1396302764 are:

m_mempool is already a pointer in other parts like: In Chainstate: https://github.com/bitcoin/bitcoin/blob/master/src/validation.h#L532 In BlockAssembler: https://github.com/bitcoin/bitcoin/blob/master/src/node/miner.h#L168 So adding this to PeerManager improves consistency.
Having m_mempool as a pointer also makes it easier to test scenarios where the mempool is not required.
I’m not sure if -nomempool is still being considered, but if it is, then this pointer approach would be a necessary prerequisite.

There was a suggestion here -> #26247 (comment) to refactor/introduce helper functions like RejectIncomingTxs that would return m_mempool, but I’m not entirely sure about this approach.this wouldn’t be ideal since in some call sites like PushNodeVersion and NetMsgType::SENDTXRCNCL, mempool isn’t needed at all.

I’d like to continue working on this, and I’m looking for feedback on whether this is still relevant and which approach to go with. Should I follow the suggestion to add helper functions that return the m_mempool pointer, or should I stick with checking if m_mempool is null, or perhaps consider a different approach altogether?

net: 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

1dc5470498

DrahtBot renamed this:
~~net: make m_mempool optional in PeerManager~~
net: make m_mempool optional in PeerManager
on Jul 21, 2025

DrahtBot added the label P2P on Jul 21, 2025

DrahtBot commented at 4:08 pm on July 21, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33029.

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:

#33005 (refactor: GenTxid type safety followups by marcofleon)
#30277 ([DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation) by sr-gi)
#30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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.

naiyoma force-pushed on Jul 21, 2025

DrahtBot added the label CI failed on Jul 21, 2025

DrahtBot commented at 4:30 pm on July 21, 2025: contributor

🚧 At least one of the CI tasks failed. Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/46403303885 LLM reason (✨ experimental): The CI failure is caused by a type mismatch error in p2p_handshake.cpp: cannot convert ‘CTxMemPool’ to ‘CTxMemPool*’.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

naiyoma force-pushed on Jul 21, 2025

DrahtBot removed the label CI failed on Jul 21, 2025

in src/net_processing.cpp:1239 in 4a2a1596a3 outdated

1235@@ -1236,7 +1236,7 @@ bool PeerManagerImpl::BlockRequested(NodeId nodeid, const CBlockIndex& block, st
1236     RemoveBlockRequest(hash, nodeid);
1237 
1238     std::list<QueuedBlock>::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(),
1239-            {&block, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)});
1240+            {&block, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(m_mempool) : nullptr)});

maflcko commented at 9:31 am on July 22, 2025:

I don’t understand why you removed the assert (https://github.com/bitcoin/bitcoin/pull/26247/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1161)

PartiallyDownloadedBlock can’t deal with nullptr and dereferencing a nullptr will be UB and will usually lead to a crash.

naiyoma commented at 7:07 pm on July 24, 2025:

this wasn’t intentional — I might have missed some changes since i rebased from the original branch.

naiyoma commented at 7:10 pm on July 24, 2025:

I have added it back

maflcko commented at 9:33 am on July 22, 2025: member

I think the changes are conceptually nice, but hard to see a user-visible benefit from this alone. Combined with the difficulty to review this (in its current state) around nullptr deref, it will probably be a hard sell.

net: rename pool parameter to mempool for consistency 1ad5238f0c

naiyoma force-pushed on Jul 24, 2025

DrahtBot added the label CI failed on Jul 25, 2025

DrahtBot commented at 3:23 pm on July 25, 2025: contributor

🚧 At least one of the CI tasks failed. Task previous releases, depends DEBUG: https://github.com/bitcoin/bitcoin/runs/46675117145 LLM reason (✨ experimental): The CI failure is caused by a compilation error due to an incorrect argument type being passed to a function, specifically converting a CTxMemPool object to a pointer where a pointer is expected.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

net: make m_mempool optional in PeerManager #33029

Code Coverage & Benchmarks

Reviews

Conflicts