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
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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) {
Wouldn't the same check be needed for 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)});
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);
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;
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
Really nice work.
review ACK 0e07e48a3b049eaa1c7b779187e56cf7ea85ead9, but the BLOCKTXN issue might need a fix 🏠
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK 0e07e48a3b049eaa1c7b779187e56cf7ea85ead9, but the BLOCKTXN issue might need a fix 🏠
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhRdgwAuw0KGr//qvSM+oqAA0grsBdt1Y86BZF9Gs7IGZJbuD453SgnPKBA6K10
BaoPE4cYiFEBXTe6AywC6hOONmsyl4VjOu8DKPD3OX2AXU2bDiQDZ87jUdEAnM9P
+9blZsyfdwf99wEKRFxUmJcYKIJp7TKAW/4OZnADIp+BjLUEncLRExK8pPIXI2iV
5hlLZ9ixdP3oTpcpsWZt8SsgGbH0tMgSpeT6DewnasXJke/kEp/dA1W7qb6bJkh/
BiZ/t9xRqwOUnHNJi1KW8VBn8mTPOrs7j3FP7IAKl+Y5ylNH7hCQiUA4m0h1n6Bh
ei1OOqUk2X4O3P2H7uzr+pCOEbK41zSG3okpRbGGxhvj4S2koQUdmFXVjMEMB835
P1r9RKnuIrC1M+O5zStTB9D4Kj+BnGUV5stZSnMknp9x60oyUywrxZ8o7mB7h8c1
FqVEp72xwL/O+VzHkrJFF/7cByqvJo3XpW1xnTAHTSR3+BrjdAbzL9mhyjre57z7
RBs48w0X
=K8hA
-----END PGP SIGNATURE-----
Timestamp of file with hash 87c58d73cd67339e790adac6db8ebc82ad4d33283928162ce202a7ff346e816b -
</details>
Concept ACK
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
Concept ACK
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.
<!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?
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
This sentence is difficult to understand. Perhaps "If this peer has relay permissions and we have a mempool, allow transactions announcements."
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)
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.
Approach ACK. It'd be great to see this picked up again.
<!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?
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.