net processing, refactor: Decouple PeerManager from gArgs #27499

Why is the banman moved into the options?

I thought it makes sense because the banman is optional but happy to move it out, no strong opinion on this. Let me know what you prefer.

Will leave this unresolved for a bit to let others chime in as well.

Doesn't this leave us with two pointers, one in m_banman and the other in m_opts.banman that is never used?

Yea, could get rid of m_banman or not have banman in the options in the first place...

I agree that we shouldn't have it in both places. No view on what's better (and if we want to move it to opts at all). If no one has a strong view, probably best to minimize churn and leave it as is?

Will leave as is, can be done in a follow up

As discussed offline, what I meant with "leave it as is" is to not move banman in this PR, i.e. reverting the change in this PR.

Removed banman from the options on latest push

Could reading from the args be moved to their own ApplyArgsManOptions function like in various node/*_args.cpp files? Then the test code wouldn't have to repeat the logic here.

Done on the latest push

I think the entire diff in this file can be removed on commit 450866412a43088415b14d360997bd248c9c53a2?

I think you now get to remove #include <common/args.h> 🥳

Done

Done

What's the rationale for using m_node.banman.get() instead of nullptr or instead of the locally constructed banman.get()? This comment applies to all 4 changes in this file.

That was a mistake on my side. Reverted in the latest push

nit: I think we try to avoid C-style casts?

        static_cast<uint32_t>(std::max(int64_t{0}, argsman.GetIntArg("-maxorphantx", DEFAULT_MAX_ORPHAN_TRANSACTIONS)));

From the developer notes (see discussion in #23810):

When casting between integer types, use functional casts such as int(x) or int{x} instead of (int) x.

Changed it to a functional cast.

nit: I find designated initializers a bit more readable but maybe that's personal? (here and in setup_common.cpp)

    PeerManager::Options peerman_opts{
        .banman = node.banman.get(),
        .ignore_incoming_txs = ignores_incoming_txs,
    };

Done

Maybe worth introducing move semantics here?

<details> <summary>git diff on 4a31d4ce13436fb52ef15df4db3e2937e11d2ab7</summary>

diff --git a/src/init.cpp b/src/init.cpp
index 923da24a7..0b276d7cd 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1540,15 +1540,17 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     ChainstateManager& chainman = *Assert(node.chainman);
 
 
-    PeerManager::Options peerman_opts;
-    peerman_opts.banman = node.banman.get();
-    peerman_opts.ignore_incoming_txs = ignores_incoming_txs;
-    ApplyArgsManOptions(args, peerman_opts);
-
-    assert(!node.peerman);
-    node.peerman = PeerManager::make(*node.connman, *node.addrman,
-                                     chainman, *node.mempool,
-                                     peerman_opts);
+    {
+        PeerManager::Options peerman_opts;
+        peerman_opts.banman = node.banman.get();
+        peerman_opts.ignore_incoming_txs = ignores_incoming_txs;
+        ApplyArgsManOptions(args, peerman_opts);
+
+        assert(!node.peerman);
+        node.peerman = PeerManager::make(*node.connman, *node.addrman,
+                                        chainman, *node.mempool,
+                                        std::move(peerman_opts));
+    }
     RegisterValidationInterface(node.peerman.get());
 
     // ********************************************************* Step 8: start indexers
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 3298a280a..7ceb01618 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -487,7 +487,7 @@ class PeerManagerImpl final : public PeerManager
 public:
     PeerManagerImpl(CConnman& connman, AddrMan& addrman,
                     ChainstateManager& chainman, CTxMemPool& pool,
-                    Options opts);
+                    Options&& opts);
 
     /** Overridden from CValidationInterface. */
     void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected) override
@@ -1807,14 +1807,14 @@ std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl
 
 std::unique_ptr<PeerManager> PeerManager::make(CConnman& connman, AddrMan& addrman,
                                                ChainstateManager& chainman, CTxMemPool& pool,
-                                               Options opts)
+                                               Options&& opts)
 {
-    return std::make_unique<PeerManagerImpl>(connman, addrman, chainman, pool, opts);
+    return std::make_unique<PeerManagerImpl>(connman, addrman, chainman, pool, std::move(opts));
 }
 
 PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman,
                                  ChainstateManager& chainman, CTxMemPool& pool,
-                                 Options opts)
+                                 Options&& opts)
     : m_chainparams(chainman.GetParams()),
       m_connman(connman),
       m_addrman(addrman),
diff --git a/src/net_processing.h b/src/net_processing.h
index e192277ec..db82ca911 100644
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -54,7 +54,7 @@ public:
 
     static std::unique_ptr<PeerManager> make(CConnman& connman, AddrMan& addrman,
                                              ChainstateManager& chainman, CTxMemPool& pool,
-                                             Options opts);
+                                             Options&& opts);
     virtual ~PeerManager() { }
 
     /**
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 026d5913b..c8f5fe096 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -272,12 +272,14 @@ 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;
-    peerman_opts.banman = m_node.banman.get();
-    ApplyArgsManOptions(*m_node.args, peerman_opts);
-    m_node.peerman = PeerManager::make(*m_node.connman, *m_node.addrman,
-                                       *m_node.chainman, *m_node.mempool,
-                                       peerman_opts);
+    {
+        PeerManager::Options peerman_opts;
+        peerman_opts.banman = m_node.banman.get();
+        ApplyArgsManOptions(*m_node.args, peerman_opts);
+        m_node.peerman = PeerManager::make(*m_node.connman, *m_node.addrman,
+                                        *m_node.chainman, *m_node.mempool,
+                                        std::move(peerman_opts));
+    }
     {
         CConnman::Options options;
         options.m_msgproc = m_node.peerman.get();

<details>

We don't do this to any of the other Options we have (afaict). There is also little benefit since this only happens once at startup.

To summarize offline discussion: CNode uses move semantics for this, but as you pointed out we construct many more CNode instances than we do PeerManagers. I'm not sure if the decreased readability trade-off is worth the benefits/sticking to best practices, so happy to have this marked as resolved.

Probably no need to switch the parameter placement of banman?

I wouldn't use hardcoded default values here, but instead use the globals (e.g. DEFAULT_TXRECONCILIATION_ENABLE), and update the logic in peerman_args.cpp to only override Options members if they are set as cli args.

Done

nit: with this change, #include <node/txreconciliation.h> can be removed from init.cpp

Moving it to the place where it is actually used seems like a good idea, no?

nit: no longer needed

done

nit: probably best to use DEFAULT_BLOCKSONLY to make it explicit what false refers to, and keep it in sync?

        bool ignore_incoming_txs{DEFAULT_BLOCKSONLY};

Alternatively (but I don't think using peerman_opts like this is elegant, just throwing out ideas), could deduplicate it entirely:

<details> <summary>git diff on e3ddc5cc96cf9f6339e7828c570fa5ad5130918a</summary>

diff --git a/src/init.cpp b/src/init.cpp
index dd7a359a2..8b62690f7 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1169,7 +1169,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
     fListen = args.GetBoolArg("-listen", DEFAULT_LISTEN);
     fDiscover = args.GetBoolArg("-discover", true);
-    const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)};
+
+    PeerManager::Options peerman_opts{
+        .ignore_incoming_txs = ignores_incoming_txs,
+    };
+    ApplyArgsManOptions(args, peerman_opts);
 
     {
 
@@ -1217,7 +1221,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     assert(!node.fee_estimator);
     // Don't initialize fee estimation with old data if we don't relay transactions,
     // as they would never get updated.
-    if (!ignores_incoming_txs) {
+    if (!peerman_opts.ignore_incoming_txs) {
         bool read_stale_estimates = args.GetBoolArg("-acceptstalefeeestimates", DEFAULT_ACCEPT_STALE_FEE_ESTIMATES);
         if (read_stale_estimates && (chainparams.GetChainType() != ChainType::REGTEST)) {
             return InitError(strprintf(_("acceptstalefeeestimates is not supported on %s chain."), chainparams.GetChainTypeString()));
@@ -1539,12 +1543,6 @@ 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,
diff --git a/src/net_processing.h b/src/net_processing.h
index 041dd2c73..afafa169f 100644
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -46,7 +46,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
 {
 public:
     struct Options {
-        bool ignore_incoming_txs{false};
+        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};
diff --git a/src/node/peerman_args.cpp b/src/node/peerman_args.cpp
index 871f23f9d..c21a2ce32 100644
--- a/src/node/peerman_args.cpp
+++ b/src/node/peerman_args.cpp
@@ -8,6 +8,8 @@ namespace node {
 
 void ApplyArgsManOptions(const ArgsManager& argsman, PeerManager::Options& options)
 {
+    if (auto value{argsman.GetBoolArg("-blocksonly")}) options.ignore_incoming_txs = *value;
+
     if (auto value{argsman.GetBoolArg("-txreconciliation")}) options.reconcile_txs = *value;
 
     if (auto value{argsman.GetIntArg("-maxorphantx")}) {

</summary>

done

Lost the comment while moving this from net_processing.cpp. I think it would be good to have doxygen comments on all of these members of Options.

        /** Whether this node is running in -blocksonly mode */
          bool ignore_incoming_txs{false};

Done

@glozow's comment re documenting Options members addressed in https://github.com/bitcoin/bitcoin/pull/28149

You forgot to read -blocksonly in this function

I think this is on purpose since we use ignore_incoming_txs for multiple purposes, see also my suggested diff in #27499 (review) (although I now see it can be simplified even further by not passing ignores_incoming_txs to the Options constructor at all). So, I don't think it's forgotten, but room for improvement in follow-up? Happy to open a pull for this if you think it's an improvement.

Yeah, I don't see what is wrong with your diff (apart from it not compiling).

I just don't get the point of adding the ApplyArgsManOptions helper when it is not used consistently. Might as well not add it in the first place?

Agreed. Addressed in #28148.