diff options
author | fanquake <fanquake@gmail.com> | 2023-07-25 10:24:27 +0100 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2023-07-25 10:42:20 +0100 |
commit | c97270d722569933f2e8972675ac3cca8589febc (patch) | |
tree | dab464da8c9a72dee63cab34646835985cfd2e53 | |
parent | d23fda05842ba4539b225bbab01b94df0060f697 (diff) | |
parent | 23c7b51ddd2888cf7fb260c439f004bd28768473 (diff) | |
download | bitcoin-c97270d722569933f2e8972675ac3cca8589febc.tar.xz |
Merge bitcoin/bitcoin#27499: net processing, refactor: Decouple PeerManager from gArgs
23c7b51ddd2888cf7fb260c439f004bd28768473 [net processing] Move -capturemessages to PeerManager::Options (dergoegge)
bd59bda26b67b53293a5cef7433e992203da325a [net processing] Move -blockreconstructionextratxn to PeerManager::Options (dergoegge)
567c4e0b6a3fadd2fd1be732076026bf491519b2 [net processing] Move -maxorphantx to PeerManager::Options (dergoegge)
fa9e6d80d1c55f8b1bb2691bfd67e8c2b7189b38 [net processing] Move -txreconciliation to PeerManager::Options (dergoegge)
4cfb7b925f8fea818f03433882138a7d3d7e179a [net processing] Use ignore_incoming_txs from m_opts (dergoegge)
8b877259217c6da316153afb136a55c2dbd401c2 [net processing] Introduce PeerManager options (dergoegge)
Pull request description:
This PR decouples `PeerManager` from our global args manager by introducing `PeerManager::Options`.
ACKs for top commit:
stickies-v:
re-ACK 23c7b51ddd2888cf7fb260c439f004bd28768473
TheCharlatan:
ACK 23c7b51ddd2888cf7fb260c439f004bd28768473
Tree-SHA512: cd807b36ec018010e11935d3539fa7ed5015fdfb531d13a042a65b54ee8533a35a919a6a6c5fa293b5cba76000e9403c64dfd790fb9c649b7838544929b1fee8
-rw-r--r-- | src/Makefile.am | 2 | ||||
-rw-r--r-- | src/init.cpp | 13 | ||||
-rw-r--r-- | src/net_processing.cpp | 40 | ||||
-rw-r--r-- | src/net_processing.h | 13 | ||||
-rw-r--r-- | src/node/peerman_args.cpp | 24 | ||||
-rw-r--r-- | src/node/peerman_args.h | 12 | ||||
-rw-r--r-- | src/node/txreconciliation.h | 2 | ||||
-rw-r--r-- | src/test/denialofservice_tests.cpp | 12 | ||||
-rw-r--r-- | src/test/util/setup_common.cpp | 6 |
9 files changed, 87 insertions, 37 deletions
diff --git a/src/Makefile.am b/src/Makefile.am index e1ae049b15..aba6f00756 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -224,6 +224,7 @@ BITCOIN_CORE_H = \ node/miner.h \ node/mini_miner.h \ node/minisketchwrapper.h \ + node/peerman_args.h \ node/psbt.h \ node/transaction.h \ node/txreconciliation.h \ @@ -421,6 +422,7 @@ libbitcoin_node_a_SOURCES = \ node/miner.cpp \ node/mini_miner.cpp \ node/minisketchwrapper.cpp \ + node/peerman_args.cpp \ node/psbt.cpp \ node/transaction.cpp \ node/txreconciliation.cpp \ diff --git a/src/init.cpp b/src/init.cpp index f726fe54ca..997996ef12 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -50,7 +50,7 @@ #include <node/mempool_args.h> #include <node/mempool_persist_args.h> #include <node/miner.h> -#include <node/txreconciliation.h> +#include <node/peerman_args.h> #include <node/validation_cache_args.h> #include <policy/feerate.h> #include <policy/fees.h> @@ -1539,9 +1539,16 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) ChainstateManager& chainman = *Assert(node.chainman); + + PeerManager::Options peerman_opts{ + .ignore_incoming_txs = ignores_incoming_txs, + }; + ApplyArgsManOptions(args, peerman_opts); + assert(!node.peerman); - node.peerman = PeerManager::make(*node.connman, *node.addrman, node.banman.get(), - chainman, *node.mempool, ignores_incoming_txs); + node.peerman = PeerManager::make(*node.connman, *node.addrman, + node.banman.get(), chainman, + *node.mempool, peerman_opts); RegisterValidationInterface(node.peerman.get()); // ********************************************************* Step 8: start indexers diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8da2c701d3..be6777d14b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -10,7 +10,6 @@ #include <blockencodings.h> #include <blockfilter.h> #include <chainparams.h> -#include <common/args.h> #include <consensus/amount.h> #include <consensus/validation.h> #include <deploymentstatus.h> @@ -487,7 +486,7 @@ class PeerManagerImpl final : public PeerManager public: PeerManagerImpl(CConnman& connman, AddrMan& addrman, BanMan* banman, ChainstateManager& chainman, - CTxMemPool& pool, bool ignore_incoming_txs); + CTxMemPool& pool, Options opts); /** Overridden from CValidationInterface. */ void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected) override @@ -515,7 +514,7 @@ public: std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; } + bool IgnoresIncomingTxs() override { return m_opts.ignore_incoming_txs; } void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void RelayTransaction(const uint256& txid, const uint256& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void SetBestHeight(int height) override { m_best_height = height; }; @@ -718,8 +717,7 @@ private: /** Next time to check for stale tip */ std::chrono::seconds m_stale_tip_check_time GUARDED_BY(cs_main){0s}; - /** Whether this node is running in -blocksonly mode */ - const bool m_ignore_incoming_txs; + const Options m_opts; bool RejectIncomingTxs(const CNode& peer) const; @@ -1212,7 +1210,7 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) // When in -blocksonly mode, never request high-bandwidth mode from peers. Our // mempool will not contain the transactions necessary to reconstruct the // compact block. - if (m_ignore_incoming_txs) return; + if (m_opts.ignore_incoming_txs) return; CNodeState* nodestate = State(nodeid); if (!nodestate || !nodestate->m_provides_cmpctblocks) { @@ -1650,13 +1648,12 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx) { - size_t max_extra_txn = gArgs.GetIntArg("-blockreconstructionextratxn", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN); - if (max_extra_txn <= 0) + if (m_opts.max_extra_txs <= 0) return; if (!vExtraTxnForCompact.size()) - vExtraTxnForCompact.resize(max_extra_txn); + vExtraTxnForCompact.resize(m_opts.max_extra_txs); vExtraTxnForCompact[vExtraTxnForCompactIt] = std::make_pair(tx->GetWitnessHash(), tx); - vExtraTxnForCompactIt = (vExtraTxnForCompactIt + 1) % max_extra_txn; + vExtraTxnForCompactIt = (vExtraTxnForCompactIt + 1) % m_opts.max_extra_txs; } void PeerManagerImpl::Misbehaving(Peer& peer, int howmuch, const std::string& message) @@ -1809,25 +1806,25 @@ std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl std::unique_ptr<PeerManager> PeerManager::make(CConnman& connman, AddrMan& addrman, BanMan* banman, ChainstateManager& chainman, - CTxMemPool& pool, bool ignore_incoming_txs) + CTxMemPool& pool, Options opts) { - return std::make_unique<PeerManagerImpl>(connman, addrman, banman, chainman, pool, ignore_incoming_txs); + return std::make_unique<PeerManagerImpl>(connman, addrman, banman, chainman, pool, opts); } PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman, BanMan* banman, ChainstateManager& chainman, - CTxMemPool& pool, bool ignore_incoming_txs) + CTxMemPool& pool, Options opts) : m_chainparams(chainman.GetParams()), m_connman(connman), m_addrman(addrman), m_banman(banman), m_chainman(chainman), m_mempool(pool), - m_ignore_incoming_txs(ignore_incoming_txs) + m_opts{opts} { // While Erlay support is incomplete, it must be enabled explicitly via -txreconciliation. // This argument can go away after Erlay support is complete. - if (gArgs.GetBoolArg("-txreconciliation", DEFAULT_TXRECONCILIATION_ENABLE)) { + if (opts.reconcile_txs) { m_txreconciliation = std::make_unique<TxReconciliationTracker>(TXRECONCILIATION_VERSION); } } @@ -2729,7 +2726,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c last_header.nHeight); } if (vGetData.size() > 0) { - if (!m_ignore_incoming_txs && + if (!m_opts.ignore_incoming_txs && nodestate->m_provides_cmpctblocks && vGetData.size() == 1 && mapBlocksInFlight.size() == 1 && @@ -3434,7 +3431,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // - we are not in -blocksonly mode. const auto* tx_relay = peer->GetTxRelay(); if (tx_relay && WITH_LOCK(tx_relay->m_bloom_filter_mutex, return tx_relay->m_relay_txs) && - !pfrom.IsAddrFetchConn() && !m_ignore_incoming_txs) { + !pfrom.IsAddrFetchConn() && !m_opts.ignore_incoming_txs) { const uint64_t recon_salt = m_txreconciliation->PreRegisterPeer(pfrom.GetId()); m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDTXRCNCL, TXRECONCILIATION_VERSION, recon_salt)); @@ -4239,8 +4236,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, m_txrequest.ForgetTxHash(tx.GetWitnessHash()); // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) - unsigned int nMaxOrphanTx = (unsigned int)std::max((int64_t)0, gArgs.GetIntArg("-maxorphantx", DEFAULT_MAX_ORPHAN_TRANSACTIONS)); - m_orphanage.LimitOrphans(nMaxOrphanTx); + m_orphanage.LimitOrphans(m_opts.max_orphan_txs); } else { LogPrint(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s\n",tx.GetHash().ToString()); // We will continue to reject this tx since it has rejected @@ -5008,7 +5004,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt msg.m_recv.data() ); - if (gArgs.GetBoolArg("-capturemessages", false)) { + if (m_opts.capture_messages) { CaptureMessage(pfrom->addr, msg.m_type, MakeUCharSpan(msg.m_recv), /*is_incoming=*/true); } @@ -5358,7 +5354,7 @@ void PeerManagerImpl::MaybeSendSendHeaders(CNode& node, Peer& peer) void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::microseconds current_time) { - if (m_ignore_incoming_txs) return; + if (m_opts.ignore_incoming_txs) return; if (pto.GetCommonVersion() < FEEFILTER_VERSION) return; // peers with the forcerelay permission should not filter txs to us if (pto.HasPermission(NetPermissionFlags::ForceRelay)) return; @@ -5426,7 +5422,7 @@ bool PeerManagerImpl::RejectIncomingTxs(const CNode& peer) const if (peer.IsBlockOnlyConn()) return true; if (peer.IsFeelerConn()) return true; // In -blocksonly mode, peers need the 'relay' permission to send txs to us - if (m_ignore_incoming_txs && !peer.HasPermission(NetPermissionFlags::Relay)) return true; + if (m_opts.ignore_incoming_txs && !peer.HasPermission(NetPermissionFlags::Relay)) return true; return false; } diff --git a/src/net_processing.h b/src/net_processing.h index deebb24c94..a0cbe92289 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -14,6 +14,8 @@ class CChainParams; class CTxMemPool; class ChainstateManager; +/** Whether transaction reconciliation protocol should be enabled by default. */ +static constexpr bool DEFAULT_TXRECONCILIATION_ENABLE{false}; /** Default for -maxorphantx, maximum number of orphan transactions kept in memory */ static const unsigned int DEFAULT_MAX_ORPHAN_TRANSACTIONS = 100; /** Default number of orphan+recently-replaced txn to keep around for block reconstruction */ @@ -43,9 +45,18 @@ struct CNodeStateStats { class PeerManager : public CValidationInterface, public NetEventsInterface { public: + struct Options { + /** Whether this node is running in -blocksonly mode */ + bool ignore_incoming_txs{DEFAULT_BLOCKSONLY}; + bool reconcile_txs{DEFAULT_TXRECONCILIATION_ENABLE}; + uint32_t max_orphan_txs{DEFAULT_MAX_ORPHAN_TRANSACTIONS}; + size_t max_extra_txs{DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN}; + bool capture_messages{false}; + }; + static std::unique_ptr<PeerManager> make(CConnman& connman, AddrMan& addrman, BanMan* banman, ChainstateManager& chainman, - CTxMemPool& pool, bool ignore_incoming_txs); + CTxMemPool& pool, Options opts); virtual ~PeerManager() { } /** diff --git a/src/node/peerman_args.cpp b/src/node/peerman_args.cpp new file mode 100644 index 0000000000..e5868ead12 --- /dev/null +++ b/src/node/peerman_args.cpp @@ -0,0 +1,24 @@ +#include <node/peerman_args.h> + +#include <common/args.h> +#include <net_processing.h> + +namespace node { + +void ApplyArgsManOptions(const ArgsManager& argsman, PeerManager::Options& options) +{ + if (auto value{argsman.GetBoolArg("-txreconciliation")}) options.reconcile_txs = *value; + + if (auto value{argsman.GetIntArg("-maxorphantx")}) { + options.max_orphan_txs = uint32_t(std::max(int64_t{0}, *value)); + } + + if (auto value{argsman.GetIntArg("-blockreconstructionextratxn")}) { + options.max_extra_txs = size_t(std::max(int64_t{0}, *value)); + } + + if (auto value{argsman.GetBoolArg("-capturemessages")}) options.capture_messages = *value; +} + +} // namespace node + diff --git a/src/node/peerman_args.h b/src/node/peerman_args.h new file mode 100644 index 0000000000..73dbdb446c --- /dev/null +++ b/src/node/peerman_args.h @@ -0,0 +1,12 @@ +#ifndef BITCOIN_NODE_PEERMAN_ARGS_H +#define BITCOIN_NODE_PEERMAN_ARGS_H + +#include <net_processing.h> + +class ArgsManager; + +namespace node { +void ApplyArgsManOptions(const ArgsManager& argsman, PeerManager::Options& options); +} // namespace node + +#endif // BITCOIN_NODE_PEERMAN_ARGS_H diff --git a/src/node/txreconciliation.h b/src/node/txreconciliation.h index 4591dd5df7..3bbb077366 100644 --- a/src/node/txreconciliation.h +++ b/src/node/txreconciliation.h @@ -11,8 +11,6 @@ #include <memory> #include <tuple> -/** Whether transaction reconciliation protocol should be enabled by default. */ -static constexpr bool DEFAULT_TXRECONCILIATION_ENABLE{false}; /** Supported transaction reconciliation protocol version */ static constexpr uint32_t TXRECONCILIATION_VERSION{1}; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 13349329ff..9193d9a8b3 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -132,8 +132,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) { NodeId id{0}; auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); - auto peerLogic = PeerManager::make(*connman, *m_node.addrman, nullptr, - *m_node.chainman, *m_node.mempool, false); + auto peerLogic = PeerManager::make(*connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, {}); constexpr int max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS; CConnman::Options options; @@ -209,8 +208,7 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction) { NodeId id{0}; auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); - auto peerLogic = PeerManager::make(*connman, *m_node.addrman, nullptr, - *m_node.chainman, *m_node.mempool, false); + auto peerLogic = PeerManager::make(*connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, {}); constexpr int max_outbound_block_relay{MAX_BLOCK_RELAY_ONLY_CONNECTIONS}; constexpr int64_t MINIMUM_CONNECT_TIME{30}; @@ -273,8 +271,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); - auto peerLogic = PeerManager::make(*connman, *m_node.addrman, banman.get(), - *m_node.chainman, *m_node.mempool, false); + auto peerLogic = PeerManager::make(*connman, *m_node.addrman, banman.get(), *m_node.chainman, *m_node.mempool, {}); CNetAddr tor_netaddr; BOOST_REQUIRE( @@ -376,8 +373,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); - auto peerLogic = PeerManager::make(*connman, *m_node.addrman, banman.get(), - *m_node.chainman, *m_node.mempool, false); + auto peerLogic = PeerManager::make(*connman, *m_node.addrman, banman.get(), *m_node.chainman, *m_node.mempool, {}); banman->ClearBanned(); int64_t nStartTime = GetTime(); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 6ae2187974..65c657da96 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -27,6 +27,7 @@ #include <node/kernel_notifications.h> #include <node/mempool_args.h> #include <node/miner.h> +#include <node/peerman_args.h> #include <node/validation_cache_args.h> #include <noui.h> #include <policy/fees.h> @@ -251,9 +252,12 @@ TestingSetup::TestingSetup( m_node.args->GetIntArg("-checkaddrman", 0)); m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); m_node.connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); // Deterministic randomness for tests. + PeerManager::Options peerman_opts; + ApplyArgsManOptions(*m_node.args, peerman_opts); m_node.peerman = PeerManager::make(*m_node.connman, *m_node.addrman, m_node.banman.get(), *m_node.chainman, - *m_node.mempool, false); + *m_node.mempool, peerman_opts); + { CConnman::Options options; options.m_msgproc = m_node.peerman.get(); |