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

jnewbery · 2020-08-24T17:15:39Z

Continues the work of moving net_processing logic into PeerLogicValidation. See #19704 and #19607 (comment) for motivation.

This PR also renames PeerLogicValidation to PeerManager as suggested in #10756 (review).

theStack · 2020-08-24T17:41:44Z

The PR title only addresses one of the many commits (the last one), I assume you either wanted to only include this commit or change the title to be more general to also include all other changes.

jnewbery · 2020-08-24T17:48:46Z

The PR title only addresses one of the many commits (the last one), I assume you either wanted to only include this commit or change the title to be more general to also include all other changes.

The object of this PR is to move Misbehaving to PeerLogicValidation. The other commits are necessary to do that because they all eventually call in to Misbehaving: https://doxygen.bitcoincore.org/net__processing_8cpp.html#ad2983e754c4d90bd68adf91b208664f5

theStack · 2020-08-24T17:55:10Z

The PR title only addresses one of the many commits (the last one), I assume you either wanted to only include this commit or change the title to be more general to also include all other changes.

The object of this PR is to move Misbehaving to PeerLogicValidation. The other commits are necessary to do that because they all eventually call in to Misbehaving: https://doxygen.bitcoincore.org/net__processing_8cpp.html#ad2983e754c4d90bd68adf91b208664f5

Thanks for clarifying, that makes sense. (And it's the first time in my life that a generated caller graph by Doxygen actually is of any help and not confusing ;-))
Concept ACK

DrahtBot · 2020-08-24T21:27:21Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Avoid locking CTxMemPool::cs recursively in some cases #19872 (Avoid locking CTxMemPool::cs recursively in some cases by hebasto)
Periodically make block-relay connections and sync headers #19858 (Periodically make block-relay connections and sync headers by sdaftuar)
net processing: Move block inventory state to net_processing #19829 ([net processing] Move block inventory state to net_processing by jnewbery)
Tidy up ProcessOrphanTx #19498 (Tidy up ProcessOrphanTx by jnewbery)
Introduce deploymentstatus #19438 (Introduce deploymentstatus by ajtowns)
validation: re-delegate absurd fee checking from mempool to clients #19339 (validation: re-delegate absurd fee checking from mempool to clients by gzhao408)
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)
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)
Drop IO priority to idle while reading blocks for peer requests and startup verification #9245 (Drop IO priority to idle while reading blocks for peer requests and startup verification by luke-jr)

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 · 2020-08-25T07:39:56Z

Concept ACK

maflcko · 2020-08-26T05:33:39Z

( Note to myself: commit eb17009 with this diff #19704 (comment) should pass the unit tests again )

jnewbery · 2020-08-28T17:12:04Z

( Note to myself: commit eb17009 with this diff #19704 (comment) should pass the unit tests again )

Thanks @MarcoFalke . I've cherrypicked that commit into this branch, which reduced further the number of parameters in some of these moved functions.

jnewbery · 2020-08-28T19:26:05Z

Rebased

practicalswift · 2020-08-29T01:04:59Z

Concept ACK

maflcko · 2020-08-29T07:07:24Z

Almost ACK. Please use a scripted diff where appropriate:

 7:  534b4a2d86 !  7:  fffffffffff [net processing] Rename PeerLogicValidation to PeerManager
    @@ Metadata
     Author: John Newbery <john@johnnewbery.com>
     
      ## Commit message ##
    -    [net processing] Rename PeerLogicValidation to PeerManager
    +    scripted-diff: [net processing] Rename PeerLogicValidation to PeerManager
    +
    +    -BEGIN VERIFY SCRIPT-
    +    sed -i 's/PeerLogicValidation/PeerManager/g' $(git grep -l PeerLogicValidation ./src ./test)
    +    -END VERIFY SCRIPT-
     
         PeerLogicValidation was originally net_processing's implementation to
         the validation interface. It has since grown to contain much of

ACK b9486cc 🍳

Exclusively move-only and rename-only changes. The only risk I see would be
uninteded changes due to shadowing, but that shouldn't be a problem because all
local symbols are aliases to the global ones anyway. (m_mempool == ::mempool,
m_params == Params(), ...)

d45f2bb --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
e4f6ec6 --word-diff-regex=. -U0
534b4a2 I have not reviewed this. Why is this not a scripted diff ???
66f20e7 --word-diff-regex=. -U0
f9a76b6 --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
e83b149 --word-diff-regex=. -U0
689b2b3 --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
f789dd7 --word-diff-regex=. -U0
b9486cc --word-diff-regex=. -U0 (also reviewed with b9486cc --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space)

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK b9486cc514 🍳

Exclusively move-only and rename-only changes. The only risk I see would be
uninteded changes due to shadowing, but that shouldn't be a problem because all
local symbols are aliases to the global ones anyway. (m_mempool == ::mempool,
m_params == Params(), ...)

* d45f2bb682 --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
* e4f6ec6aa3 --word-diff-regex=. -U0
* 534b4a2d86 I have not reviewed this. Why is this not a scripted diff ???
* 66f20e7738 --word-diff-regex=. -U0
* f9a76b620b --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space 
* e83b1497a7 --word-diff-regex=. -U0
* 689b2b3926 --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
* f789dd7214 --word-diff-regex=. -U0
* b9486cc514 --word-diff-regex=. -U0 (also reviewed with b9486cc514 --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space)
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUi44wv9EqnP0JBiaYaFXQOP16rPsjbXFLjfNTDcxy44OW6fdjz+y3twK6r5dSvj
QIsy5XuOL9Wb+RONSaBIfuABsKHZPxP3gPP6PrWd/ukD4ypUyvbi97UrCYGgaGkb
A0H6n4OIF2o9QXo+pu/fLFfPucuy/CWML5BrhUAmVwjjB7WLs5wFfGyX7nleL9nd
pM4R/JK9wgttCScaVBhF+hvWJazV86UqtktLU++AR675mH4YjRBx2Ng3BQRrmeD5
bScdeICbZ9uKYKDjZiyegfwmr7ni+zP7vIc1LVNbCEXBYzfMSyeB3+fKwQl0+bRt
ihgTP9FhuMcfGt6NLTAr5/J/Ar4IFlFYWceB7n/DG38Tt0cK1cs+9yiTyhRXttsK
ZL5u1oYqA9OMf6tLm1Ey0MvNgYRJ7iQMj1xM4BWZJlztUdaOseOT4tTaY9yQYuQg
lZJwmTGfUXodw65qfU07ARGigKsCQzL9qchrzHlyc+oQHSAcwrSTMmFKlPxit53W
pmMky4Ak
=kiZz
-----END PGP SIGNATURE-----

Timestamp of file with hash 5be0c45ea49e947755a8bba146134d0a71a0de60deb5f538b4a22c3d9a457df9 -

jnewbery · 2020-08-29T09:34:57Z

Thanks for the review @MarcoFalke ! I've taken your suggestion of making the rename a scripted diff (and added a follow-up commit that fixes the indentation).

maflcko · 2020-08-29T10:16:48Z

re-ACK ab9e416 🔋

Show signature and timestamp

Signature:

Timestamp of file with hash 8ca020057d8fe8423b0403eeebc5772bea83a87127a66a83c6409a6a47884972 -

hebasto

ACK 7ed1612

maflcko · 2020-09-06T07:24:17Z

re-ACK 7ed1612 only change is in commit message 💮

Show signature and timestamp

Signature:

Timestamp of file with hash 3fb7d2e970f192a638e72cead0f90dc92194e177f7fcf6b6a23b27665d46f3a5 -

We don't have a project style for ordering class members, but it always makes sense to have no more than one of each public/protected/private specifier. Also move documentation for MaybeDiscourageAndDisconnect to the header.

Keep a references to chainparams, rather than calling the global Params() function every time it's needed. This is fine, since globalChainParams does not get updated once it's been set, and it's available at the point of constructing the PeerLogicValidation object.

…ager -BEGIN VERIFY SCRIPT- sed -i 's/PeerLogicValidation/PeerManager/g' $(git grep -l PeerLogicValidation ./src ./test) sed -i 's/peer_logic/peerman/g' $(git grep -l peer_logic ./src ./test) -END VERIFY SCRIPT- PeerLogicValidation was originally net_processing's implementation to the validation interface. It has since grown to contain much of net_processing's logic. Therefore rename it to reflect its responsibilities. Suggested in bitcoin#10756 (review).

jnewbery · 2020-09-07T10:22:10Z

rebased on master.

Also updated NodeContext.peer_logic to NodeContext.peerman to be consistent with the new class name.

maflcko · 2020-09-07T15:23:14Z

re-ACK bb6a32c only change is rebase due to conflict in struct NodeContext and variable rename 🤸

Show signature and timestamp

Signature:

Timestamp of file with hash 65490dd75525720cc364871d69f74f20cb23d460478111dc409134823950f8b3 -

hebasto

re-ACK bb6a32c, only rebased, and added renaming s/peer_logic/peerman/ into scripted-diff since my previous review (verified with git range-diff).

Summary: ``` We don't have a project style for ordering class members, but it always makes sense to have no more than one of each public/protected/private specifier. Also move documentation for MaybeDiscourageAndDisconnect to the header. ``` Partial backport of core [[bitcoin/bitcoin#19791 | PR19791]] (1/9): bitcoin/bitcoin@824bbd1 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8940

Summary: ``` Keep a references to chainparams, rather than calling the global Params() function every time it's needed. This is fine, since globalChainParams does not get updated once it's been set, and it's available at the point of constructing the PeerLogicValidation object. ``` Partial backport of core [[bitcoin/bitcoin#19791 | PR19791]] (2/9): bitcoin/bitcoin@2297b26 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8942

…ager Summary: ``` -BEGIN VERIFY SCRIPT- sed -i 's/PeerLogicValidation/PeerManager/g' $(git grep -l PeerLogicValidation ./src ./test) sed -i 's/peer_logic/peerman/g' $(git grep -l peer_logic ./src ./test) -END VERIFY SCRIPT- PeerLogicValidation was originally net_processing's implementation to the validation interface. It has since grown to contain much of net_processing's logic. Therefore rename it to reflect its responsibilities. ``` Partial backport (3/9) of core [[bitcoin/bitcoin#19791 | PR19791]]: bitcoin/bitcoin@58bd369 Since the name conflicts with avalanche's PeerManager in tests, the full namespaced class name is used. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D9037

Summary: Partial backport (4/9) of core [[bitcoin/bitcoin#19791 | PR19791]]: bitcoin/bitcoin@d777835 Depends on D9037. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9038

Summary: Partial backport (5/9) of core [[bitcoin/bitcoin#19791 | PR19791]]: bitcoin/bitcoin@b70cd89 Depends on D9038. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9043

Summary: Partial backport (6/9) of core [[bitcoin/bitcoin#19791 | PR19791]]: bitcoin/bitcoin@e662e2d Depends on D9043. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9044

Summary: Partial backport (7/9) of core [[bitcoin/bitcoin#19791 | PR19791]]: bitcoin/bitcoin@3115e00 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9047

Summary: Partial backport (8/9) of core [[bitcoin/bitcoin#19791 | PR19791]]: bitcoin/bitcoin@aa114b1 Depends on D9047. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9048

Summary: Completes backport (9/9) of core [[bitcoin/bitcoin#19791 | PR19791]]: bitcoin/bitcoin@bb6a32c Depends on D9048. Also moves the other `Misbehaving()` prototype (specific to our codebase) as a private method. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9049

jnewbery mentioned this pull request Aug 24, 2020

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

Merged

jnewbery added P2P Refactoring labels Aug 24, 2020

This was referenced Aug 24, 2020

Tidy up ProcessOrphanTx #19498

Merged

refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

Closed

Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails #17479

Closed

DrahtBot mentioned this pull request Aug 25, 2020

validation: re-delegate absurd fee checking from mempool to clients #19339

Merged

DrahtBot mentioned this pull request Aug 27, 2020

Erlay: bandwidth-efficient transaction relay protocol #18261

Closed

jnewbery force-pushed the 2020-08-misbehaving-in-plv branch from c7b23e4 to 072bee4 Compare August 28, 2020 17:11

jnewbery force-pushed the 2020-08-misbehaving-in-plv branch from 072bee4 to f062ca2 Compare August 28, 2020 17:41

jnewbery force-pushed the 2020-08-misbehaving-in-plv branch from f062ca2 to b9486cc Compare August 28, 2020 19:26

This was referenced Aug 28, 2020

net processing: Move block inventory state to net_processing #19829

Merged

Introduce deploymentstatus #19438

Merged

This was referenced Aug 29, 2020

p2p: Move all header verification into the network layer, extend logging #19107

Merged

Drop IO priority to idle while reading blocks for peer requests and startup verification #9245

Closed

jnewbery force-pushed the 2020-08-misbehaving-in-plv branch from b9486cc to ab9e416 Compare August 29, 2020 09:34

This was referenced Aug 31, 2020

Remove mempool global #19556

Merged

Periodically make block-relay connections and sync headers #19858

Merged

hebasto approved these changes Sep 6, 2020

View reviewed changes

DrahtBot added the Needs rebase label Sep 7, 2020

jnewbery added 10 commits September 7, 2020 11:13

[whitespace] tidy up indentation after scripted diff

64f6162

[net processing] Move ProcessHeadersMessage to PeerManager

d777835

[net processing] Move MaybePunishNodeForBlock into PeerManager

b70cd89

[net processing] Move ProcessOrphanTx to PeerManager

e662e2d

[net processing] Move MaybePunishPeerForTx to PeerManager

3115e00

[net_processing] Move SendBlockTransactions into PeerManager

aa114b1

[net processing] Move Misbehaving() to PeerManager

bb6a32c

jnewbery force-pushed the 2020-08-misbehaving-in-plv branch from 7ed1612 to bb6a32c Compare September 7, 2020 10:21

DrahtBot removed the Needs rebase label Sep 7, 2020

hebasto approved these changes Sep 7, 2020

View reviewed changes

maflcko merged commit 147d50d into bitcoin:master Sep 7, 2020

hebasto mentioned this pull request Sep 7, 2020

p2p: Unify Send and Receive protocol versions #17785

Merged

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

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

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

Uh oh!

Conversation

jnewbery commented Aug 24, 2020

Uh oh!

theStack commented Aug 24, 2020

Uh oh!

jnewbery commented Aug 24, 2020

Uh oh!

theStack commented Aug 24, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

maflcko commented Aug 25, 2020

Uh oh!

maflcko commented Aug 26, 2020

Uh oh!

jnewbery commented Aug 28, 2020

Uh oh!

jnewbery commented Aug 28, 2020

Uh oh!

practicalswift commented Aug 29, 2020

Uh oh!

maflcko commented Aug 29, 2020

Uh oh!

jnewbery commented Aug 29, 2020

Uh oh!

maflcko commented Aug 29, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 6, 2020

Uh oh!

jnewbery commented Sep 7, 2020

Uh oh!

maflcko commented Sep 7, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Aug 24, 2020 •

edited

Loading