Net processing: move ProcessMessage() to PeerLogicValidation #19704

jnewbery · 2020-08-12T11:38:27Z

Rather than ProcessMessage() being a static function in net_processing.cpp, make it a private member function of PeerLogicValidation. This is the start of moving static functions and global variables into PeerLogicValidation to make it better encapsulated.

jnewbery · 2020-08-12T11:42:19Z

This was requested by @MarcoFalke @sdaftuar and @theuni in #19607 (#19607 (comment)).

Changing CConnman* connman to CConnman& m_connman is the continuation of work in #19174. See #19542 (comment) and #19174 (comment) for further discussion. Prior to this PR, connman was a pointer in PeerLogicValidation, but a reference everywhere else in net_processing. Changing it to a reference in PeerLogicValidation is required before moving the rest of the functions into PeerLogicValidation.

EDIT: I've started by moving ProcessMessage() into PLV because in order to move the new Peer struct into PLV, Misbehaving() needs to be a member function of PLV, which means everything above it in this call tree https://doxygen.bitcoincore.org/net__processing_8cpp.html#ad2983e754c4d90bd68adf91b208664f5 needs to be a member (or hold a reference to peer_logic, which I don't think makes sense)

Crypt-iQ · 2020-08-12T12:23:08Z

Fuzzers fail because ProcessMessage is used in the harnesses.

theStack · 2020-08-12T12:57:22Z

Concept ACK -- this is definitely a more appropriate location for ProcessMessage than the whole net_processing module scope.

jnewbery · 2020-08-12T13:20:08Z

I've fixed the fuzz build (I think). Let's see what Travis thinks.

Hold a reference to connman rather than a pointer because: - PeerLogicValidation can't run without a connman - The pointer never gets reseated The alternative is to always assert that the pointer is non-null before dereferencing. Change the name from connman to m_connman at the same time to conform with current style guidelines.

jnewbery · 2020-08-12T13:30:52Z

rebased

jonatack · 2020-08-13T20:09:10Z

Light approach ACK, if there is a follow-up code branch to see where this is heading I'd look at it.

JeremyRubin · 2020-08-13T20:39:00Z

Concept Ack and lite-cr ack (doesn't seem to change any logic, just refactor).

In terms of approach might be nice to encapsulate processmessage as a subclass that only has access to the relevant references out of the PeerLogicValidation state, but I don't think that's required (just thinking ahead that we might want all the individual message handlers in their own class).

edit: another reason for this is you can appropriately bind things in the interior class as const where relevant.

jnewbery · 2020-08-14T09:44:51Z

Lots of discussion on this at http://www.erisian.com.au/meetbot/bitcoin-core-dev/2020/bitcoin-core-dev.2020-08-13-19.00.log.html#l-104

promag · 2020-08-14T14:03:52Z

Code review ACK cbcb80a.

Am I missing something or this should have refactoring label?

First 2 commits could be merged on its own.

DrahtBot · 2020-08-20T20:37:57Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net: don't try to relay to the address' originator #19763 (net: don't try to relay to the address' originator by vasild)
p2p: don't add AlreadyHave transactions to recentRejects #19753 (p2p: don't add AlreadyHave transactions to recentRejects by troygiorshev)
[net] Cleanup connection types- followups #19724 ([net] Cleanup connection types- followups by amitiuttarwar)
Improvements on ADDR caching #19697 (Improvements on ADDR caching by naumenkogs)
Do not hide compile-time thread safety warnings #19668 (Do not hide compile-time thread safety warnings by hebasto)
p2p: refactor AlreadyHave(), CInv::type, INV/TX processing #19610 (p2p: refactor AlreadyHave(), CInv::type, INV/TX processing by jonatack)
Tidy up ProcessOrphanTx #19498 (Tidy up ProcessOrphanTx by jnewbery)
validation: re-delegate absurd fee checking from mempool to clients #19339 (validation: re-delegate absurd fee checking from mempool to clients by gzhao408)
refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306 (refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto)
p2p: Move all header verification into the network layer, extend logging #19107 (p2p: Move all header verification into the network layer, extend logging by troygiorshev)
Implement ADDRv2 support (part of BIP155) #19031 (Implement ADDRv2 support (part of BIP155) by vasild)
p2p: Unify Send and Receive protocol versions #17785 (p2p: Unify Send and Receive protocol versions by hebasto)
Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails #17479 (Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery)
assumeutxo #15606 ([experimental] UTXO snapshots by jamesob)

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.

maflcko

cr ACK cbcb80a 🎤

Show signature and timestamp

Signature:

Timestamp of file with hash c58fbfd8242739033e1f88dae62117af90263272c9149109ad99aba5f1597e88 -

src/test/denialofservice_tests.cpp

src/net_processing.h

jnewbery · 2020-08-21T10:07:43Z

Great suggestions @MarcoFalke . I've taken them all.

jonatack · 2020-08-21T11:08:58Z

travis and cirrus:

MemorySanitizer: use-of-uninitialized-value 
src/net_processing.cpp:656:82 in (anonymous namespace)::TipMayBeStale(Consensus::Params const&)

https://travis-ci.org/github/bitcoin/bitcoin/jobs/719886303

https://github.com/bitcoin/bitcoin/pull/19704/checks?check_run_id=1011864356

jonatack

Re-reviewing, I like these changes. One question below, in addition to the msan use-of-uninitialized-value issue.

src/net_processing.h

jnewbery · 2020-08-21T12:13:10Z

travis and cirrus:

MemorySanitizer: use-of-uninitialized-value
src/net_processing.cpp:656:82 in (anonymous namespace)::TipMayBeStale(Consensus::Params const&)

It looks like that happens because SelectParams() is called twice in the TestChain100Setup constructor, including after
the PeerLogicValidation object is constructed (see 666696b).

Rather than fight against the unit test framework, I've removed that commit. It's not essential for this PR.

theStack

Code Review ACK daed542
It's nice to see that the interface of ProcessMessage() shrinks down from 10 to 6 parameters (or 7, if you also count the implicit this pointer).

maflcko · 2020-08-24T08:57:07Z

re-ACK daed542, only change is removing second commit 🎴

Show signature and timestamp

Signature:

Timestamp of file with hash 08eeaa12e1f73cc25bec71f7f0ba83ae3ad3bc6225f47932f8a0d313daf8ffd8 -

jonatack

ACK daed542 code review and debug tested

promag · 2020-08-24T09:36:17Z

Code review ACK daed542.

…icValidation daed542 [net_processing] Move ProcessMessage to PeerLogicValidation (John Newbery) c556770 [net_processing] Change PeerLogicValidation to hold a connman reference (John Newbery) Pull request description: Rather than ProcessMessage() being a static function in net_processing.cpp, make it a private member function of PeerLogicValidation. This is the start of moving static functions and global variables into PeerLogicValidation to make it better encapsulated. ACKs for top commit: jonatack: ACK daed542 code review and debug tested promag: Code review ACK daed542. MarcoFalke: re-ACK daed542, only change is removing second commit 🎴 theStack: Code Review ACK daed542 Tree-SHA512: ddebf410d114d9ad5a9e536950018ff333a347c035d74fcc101fb4a3f20a281782c7eac2b7d1bd1c8f6bc7e59f5b5630fb52c2e1b4c32df454fa584673bd021e

fad84b7 test: Activate segwit in TestChain100Setup (MarcoFalke) fa11ff2 test: Pass empty tx pool to block assembler (MarcoFalke) fa96574 test: Move doxygen comment to header (MarcoFalke) Pull request description: This fixes not only a TODO in the code, but also prevents a never ending source of uninitialized reads. E.g. * #18376 * #19704 (comment) * ... ACKs for top commit: jnewbery: utACK fad84b7 Tree-SHA512: 64cf16a59656d49e022b603f3b06441ceae35a33a4253b4382bc8a89a56e08ad5412c8fa734d0fc7b58586f40ea6d57b348a3b4838bc6890a41ae2ec3902e378

fa9d590 scripted-diff: gArgs -> args (MarcoFalke) fa33bc2 init: Capture copy of blocknotify setting for BlockNotifyCallback (MarcoFalke) fa40017 init: Pass reference to ArgsManager around instead of relying on global (MarcoFalke) Pull request description: The gArgs global has several issues: * gArgs is used by each process (bitcoind, bitcoin-qt, bitcoin-wallet, bitcoin-cli, bitcoin-tx, ...), but it is hard to determine which arguments are actually used by each process. For example arguments that have never been registered, but are still used, will always return the fallback value. * Tests may run several sub-tests, which need different settings. So globals will have to be overwritten, but that is fragile on its own: e.g. #19704 (comment) or #19511 The goal is to remove gArgs, but as a first step in that direction this pull will change gArgs in init to use a passed-in reference instead. ACKs for top commit: ryanofsky: Code review ACK fa9d590. Looks good. Nice day to remove some globals, and add some lambdas 👍 fanquake: ACK fa9d590 - I'm not as familiar with the settings & argument handling code, but this make sense, and is a step in the right direction towards a reduction in the usage of globals. Not a huge fan of the clang-formatting in the scripted diff. jonasschnelli: Concept ACK fa9d590 Tree-SHA512: ed00db5f826566c7e3b4d0b3d2ee0fc1a49a6e748e04e5c93bdd694ac7da5598749e73937047d5fce86150d764a067d2ca344ba4ae3eb2704cc5c4fa0d20940f

fad84b7 test: Activate segwit in TestChain100Setup (MarcoFalke) fa11ff2 test: Pass empty tx pool to block assembler (MarcoFalke) fa96574 test: Move doxygen comment to header (MarcoFalke) Pull request description: This fixes not only a TODO in the code, but also prevents a never ending source of uninitialized reads. E.g. * bitcoin#18376 * bitcoin#19704 (comment) * ... ACKs for top commit: jnewbery: utACK fad84b7 Tree-SHA512: 64cf16a59656d49e022b603f3b06441ceae35a33a4253b4382bc8a89a56e08ad5412c8fa734d0fc7b58586f40ea6d57b348a3b4838bc6890a41ae2ec3902e378

fa9d590 scripted-diff: gArgs -> args (MarcoFalke) fa33bc2 init: Capture copy of blocknotify setting for BlockNotifyCallback (MarcoFalke) fa40017 init: Pass reference to ArgsManager around instead of relying on global (MarcoFalke) Pull request description: The gArgs global has several issues: * gArgs is used by each process (bitcoind, bitcoin-qt, bitcoin-wallet, bitcoin-cli, bitcoin-tx, ...), but it is hard to determine which arguments are actually used by each process. For example arguments that have never been registered, but are still used, will always return the fallback value. * Tests may run several sub-tests, which need different settings. So globals will have to be overwritten, but that is fragile on its own: e.g. bitcoin#19704 (comment) or bitcoin#19511 The goal is to remove gArgs, but as a first step in that direction this pull will change gArgs in init to use a passed-in reference instead. ACKs for top commit: ryanofsky: Code review ACK fa9d590. Looks good. Nice day to remove some globals, and add some lambdas 👍 fanquake: ACK fa9d590 - I'm not as familiar with the settings & argument handling code, but this make sense, and is a step in the right direction towards a reduction in the usage of globals. Not a huge fan of the clang-formatting in the scripted diff. jonasschnelli: Concept ACK fa9d590 Tree-SHA512: ed00db5f826566c7e3b4d0b3d2ee0fc1a49a6e748e04e5c93bdd694ac7da5598749e73937047d5fce86150d764a067d2ca344ba4ae3eb2704cc5c4fa0d20940f

bb6a32c [net processing] Move Misbehaving() to PeerManager (John Newbery) aa114b1 [net_processing] Move SendBlockTransactions into PeerManager (John Newbery) 3115e00 [net processing] Move MaybePunishPeerForTx to PeerManager (John Newbery) e662e2d [net processing] Move ProcessOrphanTx to PeerManager (John Newbery) b70cd89 [net processing] Move MaybePunishNodeForBlock into PeerManager (John Newbery) d777835 [net processing] Move ProcessHeadersMessage to PeerManager (John Newbery) 64f6162 [whitespace] tidy up indentation after scripted diff (John Newbery) 58bd369 scripted-diff: [net processing] Rename PeerLogicValidation to PeerManager (John Newbery) 2297b26 [net_processing] Pass chainparams to PeerLogicValidation constructor (John Newbery) 824bbd1 [move only] Collect all private members of PeerLogicValidation together (John Newbery) Pull request description: Continues the work of moving net_processing logic into PeerLogicValidation. See bitcoin/bitcoin#19704 and bitcoin/bitcoin#19607 (comment) for motivation. This PR also renames `PeerLogicValidation` to `PeerManager` as suggested in bitcoin/bitcoin#10756 (review). ACKs for top commit: MarcoFalke: re-ACK bb6a32c only change is rebase due to conflict in struct NodeContext and variable rename 🤸 hebasto: re-ACK bb6a32c, only rebased, and added renaming `s/peer_logic/peerman/` into scripted-diff since my [previous](bitcoin/bitcoin#19791 (review)) review (verified with `git range-diff`). Tree-SHA512: a2de4a521688fd25125b401e5575402c52b328a0fa27b3010567008d4f596b960aabbd02b2d81f42658f88f4365443fadb1008150a62fbcea123fb42d85a2c21

Summary: ```` Hold a reference to connman rather than a pointer because: - PeerLogicValidation can't run without a connman - The pointer never gets reseated The alternative is to always assert that the pointer is non-null before dereferencing. Change the name from connman to m_connman at the same time to conform with current style guidelines. ``` Partial backport (1/2) of core [[bitcoin/bitcoin#19704 | PR19704]]: bitcoin/bitcoin@c556770 Depends on D8777. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8781

Summary: Completes backport (2/2) of core [[bitcoin/bitcoin#19704 | PR19704]]. Depends on D8781. Test Plan: ninja all check-all ninja bitcoin-fuzzers Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8782

fanquake added the P2P label Aug 12, 2020

jnewbery force-pushed the 2020-07-process-message-plv branch from 745346a to af783a5 Compare August 12, 2020 13:19

jnewbery force-pushed the 2020-07-process-message-plv branch from af783a5 to cbcb80a Compare August 12, 2020 13:31

jnewbery added the Refactoring label Aug 14, 2020

This was referenced Aug 20, 2020

net: don't try to relay to the address' originator #19763

Merged

p2p: don't add AlreadyHave transactions to recentRejects #19753

Merged

[net] Cleanup connection types- followups #19724

Merged

maflcko approved these changes Aug 21, 2020

View reviewed changes

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

src/net_processing.h Outdated Show resolved Hide resolved

jnewbery force-pushed the 2020-07-process-message-plv branch from cbcb80a to 4d6c4d2 Compare August 21, 2020 10:05

jonatack reviewed Aug 21, 2020

View reviewed changes

src/net_processing.h Show resolved Hide resolved

[net_processing] Move ProcessMessage to PeerLogicValidation

daed542

jnewbery force-pushed the 2020-07-process-message-plv branch from 4d6c4d2 to daed542 Compare August 21, 2020 12:11

maflcko mentioned this pull request Aug 21, 2020

test: Activate segwit in TestChain100Setup #19775

Merged

This was referenced Aug 21, 2020

refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

Closed

p2p: Unify Send and Receive protocol versions #17785

Merged

maflcko mentioned this pull request Aug 22, 2020

Remove gArgs global from init #19779

Merged

theStack approved these changes Aug 23, 2020

View reviewed changes

DrahtBot mentioned this pull request Aug 24, 2020

Improvements on ADDR caching #19697

Merged

jonatack reviewed Aug 24, 2020

View reviewed changes

fanquake merged commit 4fefd80 into bitcoin:master Aug 24, 2020

jnewbery deleted the 2020-07-process-message-plv branch August 24, 2020 14:04

This was referenced Aug 24, 2020

[net processing] Move Misbehaving() to PeerManager #19791

Merged

[p2p] Add Peer struct for per-peer data in net processing #19607

Merged

maflcko mentioned this pull request Aug 26, 2020

Pass argsmanager reference to intro instead of relying on global bitcoin-core/gui#75

Closed

hebasto mentioned this pull request Sep 7, 2020

p2p: Try to preserve outbound block-relay-only connections during restart #17428

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Net processing: move ProcessMessage() to PeerLogicValidation #19704

Net processing: move ProcessMessage() to PeerLogicValidation #19704

Uh oh!

Conversation

jnewbery commented Aug 12, 2020

Uh oh!

jnewbery commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Crypt-iQ commented Aug 12, 2020

Uh oh!

theStack commented Aug 12, 2020

Uh oh!

jnewbery commented Aug 12, 2020

Uh oh!

jnewbery commented Aug 12, 2020

Uh oh!

jonatack commented Aug 13, 2020

Uh oh!

JeremyRubin commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Aug 14, 2020

Uh oh!

promag commented Aug 14, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Aug 21, 2020

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented Aug 21, 2020

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 24, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 24, 2020

Uh oh!

Uh oh!

jnewbery commented Aug 12, 2020 •

edited

Loading

JeremyRubin commented Aug 13, 2020 •

edited

Loading

DrahtBot commented Aug 20, 2020 •

edited

Loading

jnewbery commented Aug 21, 2020 •

edited

Loading

jonatack left a comment •

edited

Loading