net: Make m_mempool optional in PeerManager #22850

sriramdvt · 2021-08-31T19:24:38Z

Make m_mempool an optional pointer in PeerManager, instead of a reference. This is a prerequisite for the introduction of a -nomempool option.

To test that the null-pointer-dereference checks in PeerManagerImpl work, build the test-peerman-mempool-ptr branch that does not have a mempool initialized, and run it on signet/testnet (the functional tests would not work on test-peerman-mempool-ptr since the mempool is absent).

- 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

DrahtBot · 2021-08-31T19:43:22Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22976 (scripted-diff: Rename overloaded int GetArg to GetIntArg by ryanofsky)
#22955 (p2p: Rename fBlocksOnly, Add test by MarcoFalke)
#22777 (net processing: don't request tx relay on feeler connections by jnewbery)
#22677 (cut the validation <-> txmempool circular dependency by glozow)
#22564 ([WIP] refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager by dongcarl)
#22340 (p2p: Use legacy relaying to download blocks in blocks-only mode by dergoegge)
#21527 (net_processing: lock clean up by ajtowns)
#20799 (net processing: Only support version 2 compact blocks by jnewbery)

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.

maflcko

Really nice work.

review ACK 0e07e48, but the BLOCKTXN issue might need a fix 🏠

Show signature and timestamp

Signature:

Timestamp of file with hash 87c58d73cd67339e790adac6db8ebc82ad4d33283928162ce202a7ff346e816b -

maflcko · 2021-09-02T10:24:02Z

src/net_processing.cpp

-        // Ignore cmpctblock received while importing
-        if (fImporting || fReindex) {
+        // Ignore cmpctblock received while importing, or when there is no mempool
+        if (fImporting || fReindex || !m_mempool) {


Wouldn't the same check be needed for BLOCKTXN?

maflcko · 2021-09-02T10:26:53Z

src/net_processing.cpp


    std::list<QueuedBlock>::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(),
-            {&block, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)});
+            {&block, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(m_mempool) : nullptr)});


Wouldn't it be safer to set the block to nullptr when the mempool is nullptr?

If you pick this suggestion, you can also add a separate commit to document the expectation that PartiallyDownloadedBlock assumes a reference (doesn't accept nullptr) and that InitData without a pool is UB/Crash.

diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index aa111b5939..8f77c4afd6 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -45,8 +45,9 @@ uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const uint256& txhash) const { } - -ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<uint256, CTransactionRef>>& extra_txn) { +ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<uint256, CTransactionRef>>& extra_txn) +{ + Assert(pool); if (cmpctblock.header.IsNull() || (cmpctblock.shorttxids.empty() && cmpctblock.prefilledtxn.empty())) return READ_STATUS_INVALID; if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size() > MAX_BLOCK_WEIGHT / MIN_SERIALIZABLE_TRANSACTION_WEIGHT) diff --git a/src/blockencodings.h b/src/blockencodings.h index 326db1b4a7..ce3284bfd5 100644 --- a/src/blockencodings.h +++ b/src/blockencodings.h @@ -6,6 +6,7 @@ #define BITCOIN_BLOCKENCODINGS_H #include <primitives/block.h> +#include <util/check.h> class CTxMemPool; @@ -127,9 +128,10 @@ protected: std::vector<CTransactionRef> txn_available; size_t prefilled_count = 0, mempool_count = 0, extra_count = 0; const CTxMemPool* pool; + public: CBlockHeader header; - explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {} + explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(Assume(poolIn)) {} // extra_txn is a list of extra transactions to look at, in <witness hash, reference> form ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<uint256, CTransactionRef>>& extra_txn);

maflcko · 2021-09-02T10:45:02Z

src/net_processing.cpp


 bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid)
 {
+    if (!m_mempool) return false;


Suggested change

if (!m_mempool) return false;

if (!Assume(m_mempool)) return false;

It would be nice if this function wasn't called when no mempool exists. If you agree, I think it makes sense to skip calling this function when txs aren't expected at all:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3ad34e83ba..68e381db60 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2949,16 +2949,17 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, best_block = &inv.hash; } } else if (inv.IsGenTxMsg()) { + if (fBlocksOnly) { + LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol, disconnecting peer=%d\n", inv.hash.ToString(), pfrom.GetId()); + pfrom.fDisconnect = true; + return; + } const GenTxid gtxid = ToGenTxid(inv); const bool fAlreadyHave = AlreadyHaveTx(gtxid); LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); pfrom.AddKnownTx(inv.hash); - if (fBlocksOnly) { - LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol, disconnecting peer=%d\n", inv.hash.ToString(), pfrom.GetId()); - pfrom.fDisconnect = true; - return; - } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) { + if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) { AddTxAnnouncement(pfrom, gtxid, current_time); } } else {

Should be done in a separate commit, or even separate pull

Done in #23042

theStack · 2021-09-05T12:02:57Z

Concept ACK

DrahtBot · 2021-09-16T16:01:31Z

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

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

jnewbery · 2021-09-28T08:55:11Z

Concept ACK

ghost

This is a prerequisite for the introduction of a -nomempool option.

Concept NACK

I dont see any benefits for introducing this option or missing context. Based on related discussions on IRC and mailing list, I don't agree with the concept.

maflcko · 2021-12-22T14:28:34Z

If the -nomempool setting is too abstract (or controversial), I think the changes make a lot of sense on their own.

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. (Maybe that additional motivation can be included in the OP)

Moreover, the mempool is already treated optional in pretty much every other part of the codebase (validation, RPC, ...), so doing it consistently is preferable.

DrahtBot · 2022-01-03T09:11:51Z

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 'Up for grabs' with the label, so that it can be picked up in the future.

jnewbery

Approach ACK. It'd be great to see this picked up again.

jnewbery · 2022-04-15T09:00:30Z

src/net_processing.cpp


-        // Allow peers with relay permission to send data other than blocks in blocks only mode
-        if (pfrom.HasPermission(NetPermissionFlags::Relay)) {
+        // Allow peers with relay permission to send data other than blocks in blocks only mode, and if


This sentence is difficult to understand. Perhaps "If this peer has relay permissions and we have a mempool, allow transactions announcements."

jnewbery · 2022-04-15T09:01:12Z

src/net_processing.cpp

 PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman,
                                 BanMan* banman, ChainstateManager& chainman,
-                                 CTxMemPool& pool, bool ignore_incoming_txs)
+                                 CTxMemPool* pool, bool ignore_incoming_txs)


Since you're touching this line, consider changing the parameter name to mempool, to match the member variable name m_mempool. Same for the PeerManager ctor.

DrahtBot · 2022-07-25T07:41:32Z

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 'Up for grabs' with the label, so that it can be picked up in the future.

fanquake · 2022-10-02T14:47:35Z

There is certainly agreement that this is a change we want to make, but this PR has been dead for some time (and multiple pings). Closing and marking as up for grabs.

sriramdvt force-pushed the peerman-mempool-ptr branch from 8d21453 to 0e07e48 Compare August 31, 2021 19:29

DrahtBot added the P2P label Aug 31, 2021

maflcko approved these changes Sep 2, 2021

View reviewed changes

DrahtBot added the Needs rebase label Sep 16, 2021

ghost suggested changes Dec 22, 2021

View reviewed changes

jnewbery reviewed Apr 15, 2022

View reviewed changes

fanquake added the Up for grabs label Oct 2, 2022

fanquake closed this Oct 2, 2022

maflcko mentioned this pull request Oct 4, 2022

refactor: Make m_mempool optional in PeerManager #26247

Closed

fanquake removed the Up for grabs label Oct 4, 2022

bitcoin locked and limited conversation to collaborators Oct 4, 2023

	if (!m_mempool) return false;
	if (!Assume(m_mempool)) return false;

net: Make m_mempool optional in PeerManager #22850

net: Make m_mempool optional in PeerManager #22850

Uh oh!

Conversation

sriramdvt commented Aug 31, 2021

Uh oh!

DrahtBot commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

theStack commented Sep 5, 2021

Uh oh!

DrahtBot commented Sep 16, 2021

Uh oh!

jnewbery commented Sep 28, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 22, 2021

Uh oh!

DrahtBot commented Jan 3, 2022

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

jnewbery Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 25, 2022

Uh oh!

fanquake commented Oct 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Aug 31, 2021 •

edited

Loading