net processing, refactor: Decouple PeerManager from gArgs

dergoegge commented at 12:13 pm on April 20, 2023: member

This PR decouples PeerManager from our global args manager by introducing PeerManager::Options.

DrahtBot commented at 12:13 pm on April 20, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, TheCharlatan
Concept ACK	glozow, hebasto, theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28043 (fuzz: Test headers pre-sync through p2p interface by dergoegge)
#28031 (Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module by glozow)
#27385 (net, refactor: extract Network and BIP155Network logic to node/network by jonatack)
#26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
#25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
#25268 (refactor: Introduce EvictionManager by dergoegge)

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.

DrahtBot added the label CI failed on Apr 20, 2023

dergoegge force-pushed on Apr 20, 2023

DrahtBot removed the label CI failed on Apr 20, 2023

in src/init.cpp:1559 in 2bf38a3f22 outdated

1552@@ -1553,8 +1553,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1553     ChainstateManager& chainman = *Assert(node.chainman);
1554 
1555     assert(!node.peerman);
1556-    node.peerman = PeerManager::make(*node.connman, *node.addrman, node.banman.get(),
1557-                                     chainman, *node.mempool, ignores_incoming_txs);
1558+    node.peerman = PeerManager::make(*node.connman, *node.addrman,
1559+                                     chainman, *node.mempool,
1560+                                     PeerManager::Options{
1561+                                         .banman = node.banman.get(),

TheCharlatan commented at 10:01 am on April 21, 2023:

Why is the banman moved into the options?

dergoegge commented at 10:34 am on April 21, 2023:

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.

in src/net_processing.cpp:1820 in 2bf38a3f22 outdated

1797+                                 Options opts)
1798     : m_chainparams(chainman.GetParams()),
1799       m_connman(connman),
1800       m_addrman(addrman),
1801-      m_banman(banman),
1802+      m_banman(opts.banman),

TheCharlatan commented at 10:09 am on April 21, 2023:

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

dergoegge commented at 10:35 am on April 21, 2023:

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

stickies-v commented at 2:33 pm on July 18, 2023:

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?

dergoegge commented at 3:33 pm on July 18, 2023:

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

stickies-v commented at 12:14 pm on July 19, 2023:

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.

dergoegge commented at 12:29 pm on July 19, 2023:

Removed banman from the options on latest push

in src/init.cpp:1562 in b51fdf2ce7 outdated

1557@@ -1558,6 +1558,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1558                                      PeerManager::Options{
1559                                          .banman = node.banman.get(),
1560                                          .ignore_incoming_txs = ignores_incoming_txs,
1561+                                         .reconcile_txs =
1562+                                             args.GetBoolArg("-txreconciliation", DEFAULT_TXRECONCILIATION_ENABLE),

TheCharlatan commented at 10:15 am on April 21, 2023:

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.

dergoegge commented at 3:50 pm on April 21, 2023:

Done on the latest push

TheCharlatan commented at 10:42 am on April 21, 2023: contributor

Concept ACK

dergoegge force-pushed on Apr 21, 2023

DrahtBot added the label CI failed on Apr 21, 2023

dergoegge force-pushed on Apr 22, 2023

DrahtBot removed the label CI failed on Apr 22, 2023

glozow added the label Refactoring on May 8, 2023

glozow added the label P2P on May 8, 2023

dergoegge force-pushed on Jul 7, 2023

in src/test/util/setup_common.cpp:1 in 450866412a outdated

stickies-v commented at 1:37 pm on July 18, 2023:

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

stickies-v commented at 2:09 pm on July 18, 2023:

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

dergoegge commented at 3:33 pm on July 18, 2023:

Done

dergoegge commented at 3:35 pm on July 18, 2023:

Done

stickies-v commented at 12:37 pm on July 19, 2023:

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.

dergoegge commented at 1:04 pm on July 19, 2023:

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

in src/node/peerman_args.cpp:14 in 4a31d4ce13 outdated

 9+void ApplyArgsManOptions(const ArgsManager& argsman, PeerManager::Options& options)
10+{
11+    options.reconcile_txs =
12+        argsman.GetBoolArg("-txreconciliation", DEFAULT_TXRECONCILIATION_ENABLE);
13+    options.max_orphan_txs =
14+        (uint32_t)std::max(int64_t{0}, argsman.GetIntArg("-maxorphantx", DEFAULT_MAX_ORPHAN_TRANSACTIONS));

stickies-v commented at 1:53 pm on July 18, 2023:

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

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

dergoegge commented at 3:24 pm on July 18, 2023:

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

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

Changed it to a functional cast.

in src/init.cpp:1545 in 4a31d4ce13 outdated

1538@@ -1538,9 +1539,16 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1539 
1540     ChainstateManager& chainman = *Assert(node.chainman);
1541 
1542+
1543+    PeerManager::Options peerman_opts;
1544+    peerman_opts.banman = node.banman.get();
1545+    peerman_opts.ignore_incoming_txs = ignores_incoming_txs;

stickies-v commented at 2:15 pm on July 18, 2023:

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

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

dergoegge commented at 3:33 pm on July 18, 2023:

Done

in src/net_processing.cpp:489 in 4a31d4ce13 outdated

485@@ -486,8 +486,8 @@ class PeerManagerImpl final : public PeerManager
486 {
487 public:
488     PeerManagerImpl(CConnman& connman, AddrMan& addrman,
489-                    BanMan* banman, ChainstateManager& chainman,
490-                    CTxMemPool& pool, bool ignore_incoming_txs);
491+                    ChainstateManager& chainman, CTxMemPool& pool,
492+                    Options opts);

stickies-v commented at 2:28 pm on July 18, 2023:

Maybe worth introducing move semantics here?

 0diff --git a/src/init.cpp b/src/init.cpp
 1index 923da24a7..0b276d7cd 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -1540,15 +1540,17 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 5     ChainstateManager& chainman = *Assert(node.chainman);
 6 
 7 
 8-    PeerManager::Options peerman_opts;
 9-    peerman_opts.banman = node.banman.get();
10-    peerman_opts.ignore_incoming_txs = ignores_incoming_txs;
11-    ApplyArgsManOptions(args, peerman_opts);
12-
13-    assert(!node.peerman);
14-    node.peerman = PeerManager::make(*node.connman, *node.addrman,
15-                                     chainman, *node.mempool,
16-                                     peerman_opts);
17+    {
18+        PeerManager::Options peerman_opts;
19+        peerman_opts.banman = node.banman.get();
20+        peerman_opts.ignore_incoming_txs = ignores_incoming_txs;
21+        ApplyArgsManOptions(args, peerman_opts);
22+
23+        assert(!node.peerman);
24+        node.peerman = PeerManager::make(*node.connman, *node.addrman,
25+                                        chainman, *node.mempool,
26+                                        std::move(peerman_opts));
27+    }
28     RegisterValidationInterface(node.peerman.get());
29 
30     // ********************************************************* Step 8: start indexers
31diff --git a/src/net_processing.cpp b/src/net_processing.cpp
32index 3298a280a..7ceb01618 100644
33--- a/src/net_processing.cpp
34+++ b/src/net_processing.cpp
35@@ -487,7 +487,7 @@ class PeerManagerImpl final : public PeerManager
36 public:
37     PeerManagerImpl(CConnman& connman, AddrMan& addrman,
38                     ChainstateManager& chainman, CTxMemPool& pool,
39-                    Options opts);
40+                    Options&& opts);
41 
42     /** Overridden from CValidationInterface. */
43     void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected) override
44@@ -1807,14 +1807,14 @@ std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl
45 
46 std::unique_ptr<PeerManager> PeerManager::make(CConnman& connman, AddrMan& addrman,
47                                                ChainstateManager& chainman, CTxMemPool& pool,
48-                                               Options opts)
49+                                               Options&& opts)
50 {
51-    return std::make_unique<PeerManagerImpl>(connman, addrman, chainman, pool, opts);
52+    return std::make_unique<PeerManagerImpl>(connman, addrman, chainman, pool, std::move(opts));
53 }
54 
55 PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman,
56                                  ChainstateManager& chainman, CTxMemPool& pool,
57-                                 Options opts)
58+                                 Options&& opts)
59     : m_chainparams(chainman.GetParams()),
60       m_connman(connman),
61       m_addrman(addrman),
62diff --git a/src/net_processing.h b/src/net_processing.h
63index e192277ec..db82ca911 100644
64--- a/src/net_processing.h
65+++ b/src/net_processing.h
66@@ -54,7 +54,7 @@ public:
67 
68     static std::unique_ptr<PeerManager> make(CConnman& connman, AddrMan& addrman,
69                                              ChainstateManager& chainman, CTxMemPool& pool,
70-                                             Options opts);
71+                                             Options&& opts);
72     virtual ~PeerManager() { }
73 
74     /**
75diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
76index 026d5913b..c8f5fe096 100644
77--- a/src/test/util/setup_common.cpp
78+++ b/src/test/util/setup_common.cpp
79@@ -272,12 +272,14 @@ TestingSetup::TestingSetup(
80                                                m_node.args->GetIntArg("-checkaddrman", 0));
81     m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
82     m_node.connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); // Deterministic randomness for tests.
83-    PeerManager::Options peerman_opts;
84-    peerman_opts.banman = m_node.banman.get();
85-    ApplyArgsManOptions(*m_node.args, peerman_opts);
86-    m_node.peerman = PeerManager::make(*m_node.connman, *m_node.addrman,
87-                                       *m_node.chainman, *m_node.mempool,
88-                                       peerman_opts);
89+    {
90+        PeerManager::Options peerman_opts;
91+        peerman_opts.banman = m_node.banman.get();
92+        ApplyArgsManOptions(*m_node.args, peerman_opts);
93+        m_node.peerman = PeerManager::make(*m_node.connman, *m_node.addrman,
94+                                        *m_node.chainman, *m_node.mempool,
95+                                        std::move(peerman_opts));
96+    }
97     {
98         CConnman::Options options;
99         options.m_msgproc = m_node.peerman.get();

dergoegge commented at 3:33 pm on July 18, 2023:

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.

stickies-v commented at 12:22 pm on July 19, 2023:

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.

stickies-v commented at 2:34 pm on July 18, 2023: contributor

Concept ACK

dergoegge force-pushed on Jul 18, 2023

dergoegge force-pushed on Jul 19, 2023

in src/net_processing.cpp:489 in 499222f82f outdated

484@@ -486,8 +485,8 @@ class PeerManagerImpl final : public PeerManager
485 {
486 public:
487     PeerManagerImpl(CConnman& connman, AddrMan& addrman,
488-                    BanMan* banman, ChainstateManager& chainman,
489-                    CTxMemPool& pool, bool ignore_incoming_txs);
490+                    ChainstateManager& chainman, CTxMemPool& pool,
491+                    BanMan* banman, Options opts);

stickies-v commented at 12:37 pm on July 19, 2023:

Probably no need to switch the parameter placement of banman?

dergoegge force-pushed on Jul 19, 2023

DrahtBot added the label CI failed on Jul 19, 2023

dergoegge force-pushed on Jul 19, 2023

in src/net_processing.h:54 in f317796ad2 outdated

42@@ -43,9 +43,17 @@ struct CNodeStateStats {
43 class PeerManager : public CValidationInterface, public NetEventsInterface
44 {
45 public:
46+    struct Options {
47+        bool ignore_incoming_txs{false};
48+        bool reconcile_txs{false};
49+        uint32_t max_orphan_txs{DEFAULT_MAX_ORPHAN_TRANSACTIONS};
50+        size_t max_extra_txs{DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN};
51+        bool capture_messages{false};

stickies-v commented at 1:50 pm on July 19, 2023:

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.

dergoegge commented at 3:09 pm on July 19, 2023:

Done

DrahtBot removed the label CI failed on Jul 19, 2023

dergoegge force-pushed on Jul 19, 2023

in src/node/txreconciliation.h:15 in e3ddc5cc96 outdated

10@@ -11,8 +11,6 @@
11 #include <memory>
12 #include <tuple>
13 
14-/** Whether transaction reconciliation protocol should be enabled by default. */
15-static constexpr bool DEFAULT_TXRECONCILIATION_ENABLE{false};

stickies-v commented at 4:49 pm on July 19, 2023:

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

TheCharlatan commented at 2:15 pm on July 24, 2023:

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

in src/node/peerman_args.cpp:5 in e3ddc5cc96 outdated

0@@ -0,0 +1,25 @@
1+#include <node/peerman_args.h>
2+
3+#include <common/args.h>
4+#include <net_processing.h>
5+#include <node/txreconciliation.h>

stickies-v commented at 4:50 pm on July 19, 2023:

nit: no longer needed

dergoegge commented at 4:38 pm on July 24, 2023:

done

in src/net_processing.h:49 in e3ddc5cc96 outdated

44@@ -43,9 +45,17 @@ struct CNodeStateStats {
45 class PeerManager : public CValidationInterface, public NetEventsInterface
46 {
47 public:
48+    struct Options {
49+        bool ignore_incoming_txs{false};

stickies-v commented at 5:26 pm on July 19, 2023:

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

0        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:

 0diff --git a/src/init.cpp b/src/init.cpp
 1index dd7a359a2..8b62690f7 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -1169,7 +1169,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 5 
 6     fListen = args.GetBoolArg("-listen", DEFAULT_LISTEN);
 7     fDiscover = args.GetBoolArg("-discover", true);
 8-    const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)};
 9+
10+    PeerManager::Options peerman_opts{
11+        .ignore_incoming_txs = ignores_incoming_txs,
12+    };
13+    ApplyArgsManOptions(args, peerman_opts);
14 
15     {
16 
17@@ -1217,7 +1221,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
18     assert(!node.fee_estimator);
19     // Don't initialize fee estimation with old data if we don't relay transactions,
20     // as they would never get updated.
21-    if (!ignores_incoming_txs) {
22+    if (!peerman_opts.ignore_incoming_txs) {
23         bool read_stale_estimates = args.GetBoolArg("-acceptstalefeeestimates", DEFAULT_ACCEPT_STALE_FEE_ESTIMATES);
24         if (read_stale_estimates && (chainparams.GetChainType() != ChainType::REGTEST)) {
25             return InitError(strprintf(_("acceptstalefeeestimates is not supported on %s chain."), chainparams.GetChainTypeString()));
26@@ -1539,12 +1543,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
27 
28     ChainstateManager& chainman = *Assert(node.chainman);
29 
30-
31-    PeerManager::Options peerman_opts{
32-        .ignore_incoming_txs = ignores_incoming_txs,
33-    };
34-    ApplyArgsManOptions(args, peerman_opts);
35-
36     assert(!node.peerman);
37     node.peerman = PeerManager::make(*node.connman, *node.addrman,
38                                      node.banman.get(), chainman,
39diff --git a/src/net_processing.h b/src/net_processing.h
40index 041dd2c73..afafa169f 100644
41--- a/src/net_processing.h
42+++ b/src/net_processing.h
43@@ -46,7 +46,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
44 {
45 public:
46     struct Options {
47-        bool ignore_incoming_txs{false};
48+        bool ignore_incoming_txs{DEFAULT_BLOCKSONLY};
49         bool reconcile_txs{DEFAULT_TXRECONCILIATION_ENABLE};
50         uint32_t max_orphan_txs{DEFAULT_MAX_ORPHAN_TRANSACTIONS};
51         size_t max_extra_txs{DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN};
52diff --git a/src/node/peerman_args.cpp b/src/node/peerman_args.cpp
53index 871f23f9d..c21a2ce32 100644
54--- a/src/node/peerman_args.cpp
55+++ b/src/node/peerman_args.cpp
56@@ -8,6 +8,8 @@ namespace node {
57 
58 void ApplyArgsManOptions(const ArgsManager& argsman, PeerManager::Options& options)
59 {
60+    if (auto value{argsman.GetBoolArg("-blocksonly")}) options.ignore_incoming_txs = *value;
61+
62     if (auto value{argsman.GetBoolArg("-txreconciliation")}) options.reconcile_txs = *value;
63 
64     if (auto value{argsman.GetIntArg("-maxorphantx")}) {

dergoegge commented at 4:38 pm on July 24, 2023:

done

stickies-v approved

stickies-v commented at 6:44 pm on July 19, 2023: contributor

ACK 064ac73d0715fb2c371e68f8d4d234fee002299b

dergoegge requested review from TheCharlatan on Jul 24, 2023

in src/net_processing.h:47 in 864a95cbee outdated

42@@ -43,9 +43,13 @@ struct CNodeStateStats {
43 class PeerManager : public CValidationInterface, public NetEventsInterface
44 {
45 public:
46+    struct Options {
47+        bool ignore_incoming_txs{false};

glozow commented at 1:56 pm on July 24, 2023:

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.

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

dergoegge commented at 4:38 pm on July 24, 2023:

Done

stickies-v commented at 3:02 pm on July 25, 2023:

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

glozow commented at 2:05 pm on July 24, 2023: member

concept ACK

hebasto commented at 2:16 pm on July 24, 2023: member

Concept ACK.

[net processing] Introduce PeerManager options 8b87725921

[net processing] Use ignore_incoming_txs from m_opts 4cfb7b925f

dergoegge force-pushed on Jul 24, 2023

[net processing] Move -txreconciliation to PeerManager::Options fa9e6d80d1

[net processing] Move -maxorphantx to PeerManager::Options 567c4e0b6a

[net processing] Move -blockreconstructionextratxn to PeerManager::Options bd59bda26b

[net processing] Move -capturemessages to PeerManager::Options 23c7b51ddd

dergoegge force-pushed on Jul 24, 2023

dergoegge commented at 4:38 pm on July 24, 2023: member

Touched up all the nits, also rebased

stickies-v approved

stickies-v commented at 5:24 pm on July 24, 2023: contributor

re-ACK 23c7b51ddd2888cf7fb260c439f004bd28768473

TheCharlatan approved

TheCharlatan commented at 6:52 pm on July 24, 2023: contributor

ACK 23c7b51ddd2888cf7fb260c439f004bd28768473

theuni approved

theuni commented at 7:23 pm on July 24, 2023: member

Concept ACK and quick review ACK.

My only nit is that size_t max_extra_txs is platform dependent and says nothing about its upper bound. Similarly, from a quick glance, I think max_orphan_txs could wrap from the command line?

(I realize these aren’t new problems, they’re just highlighted here)

Maybe as a followup we could add some pedantic checks in init to make sure they’re sane values that fit into 32bits and make max_orphan_txs a uint32_t. Please ignore if those checks exist and I’m not seeing them.

dergoegge commented at 9:31 am on July 25, 2023: member

Maybe as a followup we could add some pedantic checks in init to make sure they’re sane values that fit into 32bits and make max_orphan_txs a uint32_t. Please ignore if those checks exist and I’m not seeing them.

Thanks for pointing this out. I agree that some sane limits/checks would make sense for these settings as users could shoot themselves in the foot otherwise. I will leave this for a follow-up though as this isn’t a new problem.

fanquake merged this on Jul 25, 2023

fanquake closed this on Jul 25, 2023

in src/node/peerman_args.cpp:21 in 23c7b51ddd

16+    if (auto value{argsman.GetIntArg("-blockreconstructionextratxn")}) {
17+        options.max_extra_txs = size_t(std::max(int64_t{0}, *value));
18+    }
19+
20+    if (auto value{argsman.GetBoolArg("-capturemessages")}) options.capture_messages = *value;
21+}

maflcko commented at 10:40 am on July 25, 2023:

You forgot to read -blocksonly in this function

stickies-v commented at 10:55 am on July 25, 2023:

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.

maflcko commented at 11:06 am on July 25, 2023:

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?

stickies-v commented at 3:01 pm on July 25, 2023:

Agreed. Addressed in #28148.

stickies-v commented at 3:03 pm on July 25, 2023: contributor

Maybe as a followup we could add some pedantic checks in init to make sure they’re sane values that fit into 32bits and make max_orphan_txs a uint32_t.

Addressed in https://github.com/bitcoin/bitcoin/pull/28149

achow101 referenced this in commit 272c4f3f10 on Jul 27, 2023

glozow referenced this in commit 0d9a13ddd8 on Aug 9, 2023

sidhujag referenced this in commit 1d2c550a15 on Aug 9, 2023

Fabcien referenced this in commit cdd9740bd3 on Jul 12, 2024

bitcoin locked this on Jul 24, 2024

net processing, refactor: Decouple PeerManager from gArgs #27499

Reviews

Conflicts