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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
::UnloadBlockIndex to BlockManager by dongcarl)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.
3414@@ -3396,8 +3415,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3415
3416 if (msg_type == NetMsgType::CMPCTBLOCK)
3417 {
3418- // Ignore cmpctblock received while importing
3419- if (fImporting || fReindex) {
3420+ // Ignore cmpctblock received while importing, or when there is no mempool
3421+ if (fImporting || fReindex || !m_mempool) {
BLOCKTXN?
866@@ -867,7 +867,7 @@ bool PeerManagerImpl::BlockRequested(NodeId nodeid, const CBlockIndex& block, st
867 RemoveBlockRequest(hash);
868
869 std::list<QueuedBlock>::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(),
870- {&block, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)});
871+ {&block, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(m_mempool) : 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.
0diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
1index aa111b5939..8f77c4afd6 100644
2--- a/src/blockencodings.cpp
3+++ b/src/blockencodings.cpp
4@@ -45,8 +45,9 @@ uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const uint256& txhash) const {
5 }
6
7
8-
9-ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<uint256, CTransactionRef>>& extra_txn) {
10+ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<uint256, CTransactionRef>>& extra_txn)
11+{
12+ Assert(pool);
13 if (cmpctblock.header.IsNull() || (cmpctblock.shorttxids.empty() && cmpctblock.prefilledtxn.empty()))
14 return READ_STATUS_INVALID;
15 if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size() > MAX_BLOCK_WEIGHT / MIN_SERIALIZABLE_TRANSACTION_WEIGHT)
16diff --git a/src/blockencodings.h b/src/blockencodings.h
17index 326db1b4a7..ce3284bfd5 100644
18--- a/src/blockencodings.h
19+++ b/src/blockencodings.h
20@@ -6,6 +6,7 @@
21 #define BITCOIN_BLOCKENCODINGS_H
22
23 #include <primitives/block.h>
24+#include <util/check.h>
25
26
27 class CTxMemPool;
28@@ -127,9 +128,10 @@ protected:
29 std::vector<CTransactionRef> txn_available;
30 size_t prefilled_count = 0, mempool_count = 0, extra_count = 0;
31 const CTxMemPool* pool;
32+
33 public:
34 CBlockHeader header;
35- explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {}
36+ explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(Assume(poolIn)) {}
37
38 // extra_txn is a list of extra transactions to look at, in <witness hash, reference> form
39 ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<uint256, CTransactionRef>>& extra_txn);
1639@@ -1632,6 +1640,8 @@ void PeerManagerImpl::BlockChecked(const CBlock& block, const BlockValidationSta
1640
1641 bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid)
1642 {
1643+ if (!m_mempool) return false;
0 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:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index 3ad34e83ba..68e381db60 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -2949,16 +2949,17 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
5 best_block = &inv.hash;
6 }
7 } else if (inv.IsGenTxMsg()) {
8+ if (fBlocksOnly) {
9+ LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol, disconnecting peer=%d\n", inv.hash.ToString(), pfrom.GetId());
10+ pfrom.fDisconnect = true;
11+ return;
12+ }
13 const GenTxid gtxid = ToGenTxid(inv);
14 const bool fAlreadyHave = AlreadyHaveTx(gtxid);
15 LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId());
16
17 pfrom.AddKnownTx(inv.hash);
18- if (fBlocksOnly) {
19- LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol, disconnecting peer=%d\n", inv.hash.ToString(), pfrom.GetId());
20- pfrom.fDisconnect = true;
21- return;
22- } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
23+ if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
24 AddTxAnnouncement(pfrom, gtxid, current_time);
25 }
26 } else {
Should be done in a separate commit, or even separate pull
Really nice work.
review ACK 0e07e48a3b049eaa1c7b779187e56cf7ea85ead9, but the BLOCKTXN issue might need a fix 🏠
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK 0e07e48a3b049eaa1c7b779187e56cf7ea85ead9, but the BLOCKTXN issue might need a fix 🏠
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUhRdgwAuw0KGr//qvSM+oqAA0grsBdt1Y86BZF9Gs7IGZJbuD453SgnPKBA6K10
8BaoPE4cYiFEBXTe6AywC6hOONmsyl4VjOu8DKPD3OX2AXU2bDiQDZ87jUdEAnM9P
9+9blZsyfdwf99wEKRFxUmJcYKIJp7TKAW/4OZnADIp+BjLUEncLRExK8pPIXI2iV
105hlLZ9ixdP3oTpcpsWZt8SsgGbH0tMgSpeT6DewnasXJke/kEp/dA1W7qb6bJkh/
11BiZ/t9xRqwOUnHNJi1KW8VBn8mTPOrs7j3FP7IAKl+Y5ylNH7hCQiUA4m0h1n6Bh
12ei1OOqUk2X4O3P2H7uzr+pCOEbK41zSG3okpRbGGxhvj4S2koQUdmFXVjMEMB835
13P1r9RKnuIrC1M+O5zStTB9D4Kj+BnGUV5stZSnMknp9x60oyUywrxZ8o7mB7h8c1
14FqVEp72xwL/O+VzHkrJFF/7cByqvJo3XpW1xnTAHTSR3+BrjdAbzL9mhyjre57z7
15RBs48w0X
16=K8hA
17-----END PGP SIGNATURE-----
Timestamp of file with hash 87c58d73cd67339e790adac6db8ebc82ad4d33283928162ce202a7ff346e816b -
🐙 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”.
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.
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.
2930@@ -2913,8 +2931,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2931 // block-relay-only peer
2932 bool fBlocksOnly = m_ignore_incoming_txs || (pfrom.m_tx_relay == nullptr);
2933
2934- // Allow peers with relay permission to send data other than blocks in blocks only mode
2935- if (pfrom.HasPermission(NetPermissionFlags::Relay)) {
2936+ // Allow peers with relay permission to send data other than blocks in blocks only mode, and if
1436 }
1437
1438 PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman,
1439 BanMan* banman, ChainstateManager& chainman,
1440- CTxMemPool& pool, bool ignore_incoming_txs)
1441+ CTxMemPool* pool, bool ignore_incoming_txs)
mempool, to match the member variable name m_mempool. Same for the PeerManager ctor.
