net processing, refactor: Decouple PeerManager from gArgs #27499

dergoegge · 2023-04-20T12:13:36Z

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

DrahtBot · 2023-04-20T12:13:39Z

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.

src/init.cpp

src/net_processing.cpp

src/init.cpp

sedited · 2023-04-21T10:42:26Z

Concept ACK

stickies-v

Concept ACK

src/test/util/setup_common.cpp

src/node/peerman_args.cpp

src/net_processing.cpp

src/init.cpp

src/net_processing.cpp

src/test/denialofservice_tests.cpp

src/net_processing.cpp

src/net_processing.h

stickies-v

ACK 064ac73d0715fb2c371e68f8d4d234fee002299b

src/node/txreconciliation.h

src/node/peerman_args.cpp

src/net_processing.h

glozow

concept ACK

src/net_processing.h

hebasto · 2023-07-24T14:16:41Z

Concept ACK.

…tions

dergoegge · 2023-07-24T16:38:30Z

Touched up all the nits, also rebased

stickies-v

re-ACK 23c7b51

sedited

ACK 23c7b51

theuni

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 · 2023-07-25T09:31:09Z

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.

maflcko · 2023-07-25T10:40:11Z

src/node/peerman_args.cpp

+    }
+
+    if (auto value{argsman.GetBoolArg("-capturemessages")}) options.capture_messages = *value;
+}


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 (comment) (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.

stickies-v · 2023-07-25T15:03:19Z

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 #28149

…ptions for PeerManager::Options 8a31597 refactor: deduplicate ignores_incoming_txs (stickies-v) 5f41afc refactor: set ignore_incoming_txs in ApplyArgsManOptions (stickies-v) Pull request description: Consistently use `ApplyArgsManOptions` for `PeerManager::Options`, and initialize `PeerManager::Options` early to avoid reading `"-blocksonly"` twice. Suggested in bitcoin/bitcoin#27499 (comment) and also requested in bitcoin/bitcoin#27499 (comment). No behaviour change, but the [`TestingSetup`](https://github.com/bitcoin/bitcoin/blob/e35fb7bc48d360585b80d0c7f89ac5087c1d405e/src/test/util/setup_common.cpp#L255-L256) is now also able to access `"-blocksonly"`. ACKs for top commit: MarcoFalke: lgtm ACK 8a31597 achow101: ACK 8a31597 TheCharlatan: ACK 8a31597 dergoegge: utACK 8a31597 Tree-SHA512: 6cb489d79ac2a87e8faedb76c96973ab3fc597426f274a90a3ffd0bc5fe3f2b25db9c7ec2e55a0c806c2bcbc0fdded6e228adb43d2cd81f14fd6552863847698

…ns user input 547fa52 net processing: clamp -blockreconstructionextratxn to uint32_t bounds (stickies-v) e451d1e net processing: clamp -maxorphantx to uint32_t bounds (stickies-v) aa89e04 doc: document PeerManager::Options members (stickies-v) Pull request description: Avoid out-of-bounds user input for `PeerManager::Options` by safely clamping `-maxorphantx` and `-blockreconstructionextratxn`, and avoid platform-specific behaviour by changing `PeerManager::Options::max_extra_txs` from `size_t` to a `uint32_t`. Addresses bitcoin/bitcoin#27499 (review). Also documents all `PeerManager::Options` members, addressing bitcoin/bitcoin#27499 (comment). ACKs for top commit: dergoegge: Code review ACK 547fa52 glozow: reACK 547fa52 Tree-SHA512: 042d47b35bb8a7b29ef3dadd4c0c5d26f13a8f174f33687855d603c19f8de0fcbbda94418453331e149885412d4edd5f402d640d938f6d94b4dcf54e2fdbbcc9

…rom gArgs

Summary: [net processing] Introduce PeerManager options bitcoin/bitcoin@8b87725 ----- [net processing] Use ignore_incoming_txs from m_opts bitcoin/bitcoin@4cfb7b9 ----- [net processing] Move -maxorphantx to PeerManager::Options bitcoin/bitcoin@567c4e0 ----- [net processing] Move -blockreconstructionextratxn to PeerManager::Options bitcoin/bitcoin@bd59bda ----- [net processing] Move -capturemessages to PeerManager::Options bitcoin/bitcoin@23c7b51 Note that we cannot yet remove `include <common/args.h>` The remaining calls to gArgs in net_processing.cpp are mostly related to avalanche options, which I will move in a separate diff. ----- refactor: set ignore_incoming_txs in ApplyArgsManOptions Refactor to consistently use ApplyArgsManOptions to set all PeerManager::Options, including ignore_incoming_txs. bitcoin/bitcoin@8a31597 Note we don't have the duplicated reading of -blocksonly, this is used by core for initializing some fee estimation machinery. ----- [net_processing] move -maxaddrtosend to PeerManager::Options Note that this option was added by us in D10823 ----- This is a backport of [[bitcoin/bitcoin#27499 | core#27499]] and [[bitcoin/bitcoin#28148 | core#28148]] It it missing the -txreconciliation related commit which is part of a feature that we have not yet backported. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D16455

DrahtBot added the CI failed label Apr 20, 2023

dergoegge force-pushed the 2023-04-peerman-opts branch from c3a0d5f to e044437 Compare April 20, 2023 13:54

DrahtBot removed the CI failed label Apr 20, 2023

dergoegge mentioned this pull request Apr 20, 2023

meta: Isolated fuzzing of net processing #27502

Closed

11 tasks

This was referenced Apr 20, 2023

refactor: Continue moving application data from CNode to Peer #26621

Closed

refactor: Introduce EvictionManager and use it for the inbound eviction logic #25572

Closed

refactor: Introduce EvictionManager #25268

Closed

sedited reviewed Apr 21, 2023

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

dergoegge force-pushed the 2023-04-peerman-opts branch 2 times, most recently from 05dbe25 to f5d1a76 Compare April 21, 2023 16:09

DrahtBot added the CI failed label Apr 21, 2023

DrahtBot mentioned this pull request Apr 21, 2023

net, refactor: extract Network and BIP155Network logic to node/network #27385

Closed

dergoegge force-pushed the 2023-04-peerman-opts branch from f5d1a76 to f221a16 Compare April 22, 2023 13:26

DrahtBot removed the CI failed label Apr 22, 2023

glozow added Refactoring P2P labels May 8, 2023

This was referenced May 24, 2023

[NO MERGE] BIP331 Ancestor Package Relay #27742

Closed

Make non-standard tx acceptance a peer option. #27772

Closed

DrahtBot mentioned this pull request Jul 5, 2023

Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling #28031

Closed

dergoegge force-pushed the 2023-04-peerman-opts branch from f221a16 to 4a31d4c Compare July 7, 2023 08:43

dergoegge mentioned this pull request Jul 7, 2023

fuzz: Test headers pre-sync through p2p interface #28043

Closed

stickies-v reviewed Jul 18, 2023

View reviewed changes

dergoegge force-pushed the 2023-04-peerman-opts branch 2 times, most recently from d962c7f to 499222f Compare July 19, 2023 12:29

stickies-v reviewed Jul 19, 2023

View reviewed changes

src/test/denialofservice_tests.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

dergoegge force-pushed the 2023-04-peerman-opts branch from 499222f to 3c291dc Compare July 19, 2023 12:57

DrahtBot added the CI failed label Jul 19, 2023

dergoegge force-pushed the 2023-04-peerman-opts branch from 3c291dc to f317796 Compare July 19, 2023 13:03

stickies-v reviewed Jul 19, 2023

View reviewed changes

src/net_processing.h Outdated Show resolved Hide resolved

DrahtBot removed the CI failed label Jul 19, 2023

dergoegge force-pushed the 2023-04-peerman-opts branch from f317796 to e3ddc5c Compare July 19, 2023 15:08

stickies-v approved these changes Jul 19, 2023

View reviewed changes

src/node/txreconciliation.h Outdated Show resolved Hide resolved

src/node/peerman_args.cpp Outdated Show resolved Hide resolved

src/net_processing.h Outdated Show resolved Hide resolved

dergoegge requested a review from sedited July 24, 2023 12:11

glozow reviewed Jul 24, 2023

View reviewed changes

src/net_processing.h Outdated Show resolved Hide resolved

dergoegge added 2 commits July 24, 2023 18:30

[net processing] Introduce PeerManager options

8b87725

[net processing] Use ignore_incoming_txs from m_opts

4cfb7b9

dergoegge force-pushed the 2023-04-peerman-opts branch from e3ddc5c to c8bc812 Compare July 24, 2023 16:33

dergoegge added 4 commits July 24, 2023 18:35

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

fa9e6d8

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

567c4e0

[net processing] Move -blockreconstructionextratxn to PeerManager::Op…

bd59bda

…tions

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

23c7b51

dergoegge force-pushed the 2023-04-peerman-opts branch from c8bc812 to 23c7b51 Compare July 24, 2023 16:37

stickies-v approved these changes Jul 24, 2023

View reviewed changes

sedited approved these changes Jul 24, 2023

View reviewed changes

theuni approved these changes Jul 24, 2023

View reviewed changes

fanquake merged commit c97270d into bitcoin:master Jul 25, 2023

maflcko reviewed Jul 25, 2023

View reviewed changes

This was referenced Jul 25, 2023

refactor: consistently use ApplyArgsManOptions for PeerManager::Options #28148

Merged

net processing: clamp PeerManager::Options user input #28149

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 9, 2023

Merge bitcoin#27499: net processing, refactor: Decouple PeerManager f…

1d2c550

…rom gArgs

bitcoin locked and limited conversation to collaborators Jul 24, 2024

net processing, refactor: Decouple PeerManager from gArgs #27499

net processing, refactor: Decouple PeerManager from gArgs #27499

Uh oh!

Conversation

dergoegge commented Apr 20, 2023

Uh oh!

DrahtBot commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sedited commented Apr 21, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Jul 24, 2023

Uh oh!

dergoegge commented Jul 24, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

dergoegge commented Jul 25, 2023

Uh oh!

maflcko Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

stickies-v Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

stickies-v Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

stickies-v commented Jul 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DrahtBot commented Apr 20, 2023 •

edited

Loading