Skip to content

net: make m_mempool optional in PeerManager #33029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
naiyoma wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

net: make m_mempool optional in PeerManager #33029

naiyoma wants to merge 1 commit into from

Conversation

@naiyoma
Copy link
Contributor

@naiyoma naiyoma commented Jul 21, 2025

This PR is a continuation of →#22850, to make m_mempool an optional pointer in PeerManager instead of a reference.

some benefits, as mentioned here:#26247 (comment) are:

  1. 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.
  2. Having m_mempool as a pointer also makes it easier to test scenarios where the mempool is not required.
  3. 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?

@DrahtBot DrahtBot changed the title net: make m_mempool optional in PeerManager net: make m_mempool optional in PeerManager Jul 21, 2025
@DrahtBot DrahtBot added the P2P label Jul 21, 2025
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 21, 2025 •
edited
Loading

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:

  • #33448 (net/rpc: Report inv information for debugging by ajtowns)
  • #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)

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 naiyoma force-pushed the 2025_3/peer_mempool_ptr branch from 80efcb1 to e3f9f2a Compare July 21, 2025 16:30
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 21, 2025

🚧 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*’.

Hints

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 naiyoma force-pushed the 2025_3/peer_mempool_ptr branch from e3f9f2a to 4a2a159 Compare July 21, 2025 16:38
@DrahtBot DrahtBot mentioned this pull request Jul 22, 2025
Merged
maflcko
maflcko reviewed Jul 22, 2025
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This was referenced Jul 22, 2025
Draft
Open
@naiyoma naiyoma force-pushed the 2025_3/peer_mempool_ptr branch from 4a2a159 to 1ad5238 Compare July 24, 2025 19:02
@DrahtBot DrahtBot mentioned this pull request Jul 25, 2025
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 25, 2025

🚧 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.

Hints

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 naiyoma force-pushed the 2025_3/peer_mempool_ptr branch from 1ad5238 to 6f5f78b Compare August 6, 2025 07:37
@naiyoma naiyoma force-pushed the 2025_3/peer_mempool_ptr branch from 6f5f78b to 140a1cd Compare August 27, 2025 11:40
@sriramdvt @naiyoma
net: make m_mempool optional in PeerManager
448a67c 
- 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
- rename pool parameter to mempool for consistency
@naiyoma naiyoma force-pushed the 2025_3/peer_mempool_ptr branch from 140a1cd to 448a67c Compare August 27, 2025 12:02
@DrahtBot DrahtBot mentioned this pull request Aug 27, 2025
Draft
@DrahtBot DrahtBot mentioned this pull request Sep 21, 2025
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 23, 2025

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 21, 2025

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it with one of the labels 'Up for grabs' or 'Insufficient Review', so that it can be picked up in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Needs rebase P2P

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@naiyoma @DrahtBot @maflcko @sriramdvt