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

jnewbery commented at 5:15 PM on August 24, 2020: member

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

This PR also renames PeerLogicValidation to PeerManager as suggested in #10756#pullrequestreview-53892618.

jnewbery added the label P2P on Aug 24, 2020

jnewbery added the label Refactoring on Aug 24, 2020

theStack commented at 5:41 PM on August 24, 2020: member

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 commented at 5:48 PM on August 24, 2020: member

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 commented at 5:55 PM on August 24, 2020: member

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 commented at 9:27 PM on August 24, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19872 (Avoid locking CTxMemPool::cs recursively in some cases by hebasto)
#19858 (Periodically make block-relay connections and sync headers by sdaftuar)
#19829 ([net processing] Move block inventory state to net_processing by jnewbery)
#19498 (Tidy up ProcessOrphanTx by jnewbery)
#19438 (Introduce deploymentstatus by ajtowns)
#19339 (validation: re-delegate absurd fee checking from mempool to clients by gzhao408)
#19107 (p2p: Move all header verification into the network layer, extend logging by troygiorshev)
#17785 (p2p: Unify Send and Receive protocol versions by hebasto)
#17479 (Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery)
#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.

MarcoFalke commented at 7:39 AM on August 25, 2020: member

Concept ACK

MarcoFalke commented at 5:33 AM on August 26, 2020: member

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

jnewbery force-pushed on Aug 28, 2020

jnewbery commented at 5:12 PM on August 28, 2020: member

( 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 force-pushed on Aug 28, 2020

jnewbery commented at 7:26 PM on August 28, 2020: member

Rebased

jnewbery force-pushed on Aug 28, 2020

practicalswift commented at 1:04 AM on August 29, 2020: contributor

Concept ACK

MarcoFalke commented at 7:07 AM on August 29, 2020: member

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 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)

<details><summary>Show signature and timestamp</summary>

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 -

</details>

jnewbery force-pushed on Aug 29, 2020

jnewbery commented at 9:34 AM on August 29, 2020: member

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

MarcoFalke commented at 10:16 AM on August 29, 2020: member

re-ACK ab9e416812 🔋

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 8ca020057d8fe8423b0403eeebc5772bea83a87127a66a83c6409a6a47884972 -

</details>

hebasto commented at 5:07 PM on September 4, 2020: member

Concept ACK.

in src/net_processing.h:108 in 4ca3570c4a outdated

 104 | @@ -105,6 +105,7 @@ class PeerManager final : public CValidationInterface, public NetEventsInterface
 105 |       */
 106 |      bool MaybeDiscourageAndDisconnect(CNode& pnode);
 107 |  
 108 | +    void ProcessOrphanTx(std::set<uint256>& orphan_work_set, std::list<CTransactionRef>& removed_txn) EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_cs_orphans);

hebasto commented at 6:37 AM on September 5, 2020:

4ca3570c4a6a1332dbfc1f97eda6b717b21df468, nit:

    void ProcessOrphanTx(std::set<uint256>& orphan_work_set, std::list<CTransactionRef>& removed_txn)
        EXCLUSIVE_LOCKS_REQUIRED(::cs_main, g_cs_orphans);

jnewbery commented at 10:05 AM on September 5, 2020:

Done!

hebasto commented at 7:04 AM on September 5, 2020: member

IIUC, the goal of this PR is a better encapsulation. Therefore, functions that do not require an access to private data members should be placed into a dedicated namespace as non-member ones. I mean MaybePunishNodeForBlock(), MaybePunishNodeForTx() and Misbehaving().

In commit 30dbd283cc3c4bcc55cefe5925581dbf9da78c2d "scripted-diff: [net processing] Rename PeerLogicValidation to PeerManager" the commit message is incomplete.

jnewbery commented at 9:33 AM on September 5, 2020: member

Please see #19607 (review) for motivation. The idea is to move the g_peer_map to be a member of PeerLogicValidation. To do that, we need to move these functions into PeerLogicValidation since they need to access that data.

jnewbery force-pushed on Sep 5, 2020

hebasto commented at 2:16 PM on September 5, 2020: member

In commit 30dbd28 "scripted-diff: [net processing] Rename PeerLogicValidation to PeerManager" the commit message is incomplete.

I meant that before the latest push the commit message was

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

I didn't mean to drop the scripted-diff part. Sorry.

hebasto commented at 2:21 PM on September 5, 2020: member

The idea is to move the g_peer_map to be a member of PeerLogicValidation. To do that, we need to move these functions into PeerLogicValidation since they need to access that data.

But MaybePunishNodeForBlock(), MaybePunishNodeForTx() and Misbehaving() do not access to g_peer_map, no?

MarcoFalke commented at 2:31 PM on September 5, 2020: member

But MaybePunishNodeForBlock(), MaybePunishNodeForTx() and Misbehaving() do not access to g_peer_map, no?

Misbehaving calls GetPeerRef

hebasto commented at 2:39 PM on September 5, 2020: member

But MaybePunishNodeForBlock(), MaybePunishNodeForTx() and Misbehaving() do not access to g_peer_map, no?

Misbehaving calls GetPeerRef

Correct. But it seems that MaybePunishNodeForBlock() and MaybePunishNodeForTx() have neither direct nor indirect access to g_peer_map.

MarcoFalke commented at 2:41 PM on September 5, 2020: member

They call Misbehaving, which calls GetPeerRef, which accesses g_peer_map

hebasto commented at 2:58 PM on September 5, 2020: member

They call Misbehaving, which calls GetPeerRef, which accesses g_peer_map

Indeed. Sorry for noise.

jnewbery force-pushed on Sep 5, 2020

jnewbery commented at 7:05 PM on September 5, 2020: member

I didn't mean to drop the scripted-diff part. Sorry.

Oops. I didn't mean to do that. Restored the scripted-diff and fixed the commit log.

They call Misbehaving, which calls GetPeerRef, which accesses g_peer_map

Indeed. Sorry for noise.

No problem. Thanks for reviewing!

hebasto approved

hebasto commented at 7:13 AM on September 6, 2020: member

ACK 7ed1612715785bb48053c50167c136f1d46ac352

MarcoFalke commented at 7:24 AM on September 6, 2020: member

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

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 3fb7d2e970f192a638e72cead0f90dc92194e177f7fcf6b6a23b27665d46f3a5 -

</details>

DrahtBot added the label Needs rebase on Sep 7, 2020

[move only] Collect all private members of PeerLogicValidation together

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.

824bbd1ffb

[net_processing] Pass chainparams to PeerLogicValidation constructor

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.

2297b26b3c

scripted-diff: [net processing] Rename PeerLogicValidation to PeerManager

-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
https://github.com/bitcoin/bitcoin/pull/10756#pullrequestreview-53892618.

58bd369b0d

[whitespace] tidy up indentation after scripted diff 64f6162651

[net processing] Move ProcessHeadersMessage to PeerManager d7778351bf

[net processing] Move MaybePunishNodeForBlock into PeerManager b70cd890e3

[net processing] Move ProcessOrphanTx to PeerManager e662e2d42a

[net processing] Move MaybePunishPeerForTx to PeerManager 3115e00f75

[net_processing] Move SendBlockTransactions into PeerManager aa114b1c9b

[net processing] Move Misbehaving() to PeerManager bb6a32ce99

jnewbery force-pushed on Sep 7, 2020

jnewbery commented at 10:22 AM on September 7, 2020: member

rebased on master.

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

DrahtBot removed the label Needs rebase on Sep 7, 2020

MarcoFalke commented at 3:23 PM on September 7, 2020: member

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

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 65490dd75525720cc364871d69f74f20cb23d460478111dc409134823950f8b3 -

</details>

hebasto approved

hebasto commented at 3:57 PM on September 7, 2020: member

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

MarcoFalke merged this on Sep 7, 2020

MarcoFalke closed this on Sep 7, 2020

Fabcien referenced this in commit e490dccb87 on Jan 18, 2021

Fabcien referenced this in commit 63c9d13451 on Jan 22, 2021

deadalnix referenced this in commit 09281847a8 on Jan 23, 2021

Fabcien referenced this in commit 6a28c545f1 on Jan 23, 2021

Fabcien referenced this in commit 0225d4f0ef on Jan 23, 2021

Fabcien referenced this in commit 94c2f558ee on Jan 23, 2021

deadalnix referenced this in commit 7330ea64ea on Jan 25, 2021

Fabcien referenced this in commit 86c08ac054 on Jan 25, 2021

Fabcien referenced this in commit 245041dc23 on Jan 26, 2021

DrahtBot locked this on Feb 15, 2022