[net processing] Move Misbehaving() to PeerManager

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:

 0 7:  534b4a2d86 !  7:  fffffffffff [net processing] Rename PeerLogicValidation to PeerManager
 1    @@ Metadata
 2     Author: John Newbery <john@johnnewbery.com>
 3     
 4      ## Commit message ##
 5    -    [net processing] Rename PeerLogicValidation to PeerManager
 6    +    scripted-diff: [net processing] Rename PeerLogicValidation to PeerManager
 7    +
 8    +    -BEGIN VERIFY SCRIPT-
 9    +    sed -i 's/PeerLogicValidation/PeerManager/g' $(git grep -l PeerLogicValidation ./src ./test)
10    +    -END VERIFY SCRIPT-
11     
12         PeerLogicValidation was originally net_processing's implementation to
13         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)

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK b9486cc514 🍳
 4
 5Exclusively move-only and rename-only changes. The only risk I see would be
 6uninteded changes due to shadowing, but that shouldn't be a problem because all
 7local symbols are aliases to the global ones anyway. (m_mempool == ::mempool,
 8m_params == Params(), ...)
 9
10* d45f2bb682 --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
11* e4f6ec6aa3 --word-diff-regex=. -U0
12* 534b4a2d86 I have not reviewed this. Why is this not a scripted diff ???
13* 66f20e7738 --word-diff-regex=. -U0
14* f9a76b620b --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space 
15* e83b1497a7 --word-diff-regex=. -U0
16* 689b2b3926 --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
17* f789dd7214 --word-diff-regex=. -U0
18* b9486cc514 --word-diff-regex=. -U0 (also reviewed with b9486cc514 --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space)
19-----BEGIN PGP SIGNATURE-----
20
21iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
22pUi44wv9EqnP0JBiaYaFXQOP16rPsjbXFLjfNTDcxy44OW6fdjz+y3twK6r5dSvj
23QIsy5XuOL9Wb+RONSaBIfuABsKHZPxP3gPP6PrWd/ukD4ypUyvbi97UrCYGgaGkb
24A0H6n4OIF2o9QXo+pu/fLFfPucuy/CWML5BrhUAmVwjjB7WLs5wFfGyX7nleL9nd
25pM4R/JK9wgttCScaVBhF+hvWJazV86UqtktLU++AR675mH4YjRBx2Ng3BQRrmeD5
26bScdeICbZ9uKYKDjZiyegfwmr7ni+zP7vIc1LVNbCEXBYzfMSyeB3+fKwQl0+bRt
27ihgTP9FhuMcfGt6NLTAr5/J/Ar4IFlFYWceB7n/DG38Tt0cK1cs+9yiTyhRXttsK
28ZL5u1oYqA9OMf6tLm1Ey0MvNgYRJ7iQMj1xM4BWZJlztUdaOseOT4tTaY9yQYuQg
29lZJwmTGfUXodw65qfU07ARGigKsCQzL9qchrzHlyc+oQHSAcwrSTMmFKlPxit53W
30pmMky4Ak
31=kiZz
32-----END PGP SIGNATURE-----

Timestamp of file with hash 5be0c45ea49e947755a8bba146134d0a71a0de60deb5f538b4a22c3d9a457df9 -

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 🔋

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK ab9e416812 🔋
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgOmAv/XiLCgaJ+N1uXLqQGle28I8T65NwC7lN8op1+AcF0HdvG4P/nr9XAKJGO
 8HGutxFBbwYhY0gdFiShEKPI/LdTIPF+HEARqQ3bt4HCM1XxFf01UZOKIRjDpEOIl
 9Zij/8/JIbYQnrl5ROhLlDc89QQKxOyfcviYkshahiXK3QUEDbLrb8axuWl4OJ7jR
10eXvlwHqY54k8DbT8ofjCxaHpADgWjLWxsjW4a3QhYM0MnI+KI8g0UryLBvGs2A23
11X6VTgJ0hXQX+QJTLVUjy0jwIalrBVWpMXRcDa+LF6PTsZnFKIH3i9ej2p+91MHJk
122kxiAUFYcbpYIhhAi21m4TUSwZ2OjxXPv1Fa3X122caj2eHmotlrH+ACo6/O0uWe
133QWyAB5caNLs0KAxtCC5F7A2caYKjlVw/BnR8krn8PQXP+H3kjZAjUzDQkT9xOhx
14eHAVHR5w7+dDbuZQ39fdC9inTJqXSYzwtvfMPpxpCIPECleG0SxvrtRTw5UWzOnU
15OUb2Ix1f
16=ffIE
17-----END PGP SIGNATURE-----

Timestamp of file with hash 8ca020057d8fe8423b0403eeebc5772bea83a87127a66a83c6409a6a47884972 -

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:

0    void ProcessOrphanTx(std::set<uint256>& orphan_work_set, std::list<CTransactionRef>& removed_txn)
1        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

0scripted-diff: [net processing] Rename PeerLogicValidation to PeerManager
1
2-BEGIN VERIFY SCRIPT-
3sed -i 's/PeerLogicValidation/PeerManager/g' $(git grep -l PeerLogicValidation ./src ./test)
4-END VERIFY SCRIPT-
5
6PeerLogicValidation was originally net_processing's implementation to
7the 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 💮

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 7ed1612715 only change is in commit message 💮
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjjSAv/eL83MPVWHHsG87sMVTjfZH87F5ioMxNs/Rj/ZwuFkj3yMLwXFPDlJMsw
 8cBoTZS/i1PBCZwueOjGJd5E5RFY98FsLsUG6zsxcnxZvbiUCnGvc6gnPqHnFLDzN
 9yJGDOLlFrT9NRMsuo+sQ9fZ0dGRB3UmXkX9g3PRIFqLAvW9++ZHgHsHFWEDaWjXo
10eZ40OcEAZ0/y8OiZLNcyP5m/XUrmirxLQvLBbnBa7knP9ieE8Gz1XIgTPXrbJxno
11PzPIwFrB0z6iyKB1v8HW1mO/qK+zGTKkPAjev0KucuFmlApTp+WcH98yBvhazuQQ
12GMBk64LQLOhb2RiHTrjKHUXfcr05gc68or4LbB40E/8FoSHWu5H3U6h3rUgOWp5+
13xklUznWZix1ajWjIJxDBDALKgxJNkDQrtRTVX0fCjjrgLkYsZo22ciMjE9VNg4z9
141x+aja5E7olxX6mLq61JvXLA+fLUrLYOa5kUs3+vcSjr1FkEGKOB8jYUcsnwx1Ot
15jvSFZpV0
16=Ghuq
17-----END PGP SIGNATURE-----

Timestamp of file with hash 3fb7d2e970f192a638e72cead0f90dc92194e177f7fcf6b6a23b27665d46f3a5 -

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 🤸

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK bb6a32ce99 only change is rebase due to conflict in struct NodeContext and variable rename 🤸
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUj5EwwAgJp5hQNNMrXFOSwWHrMZfCS1OfNXgNYhlArGTaksEIeWnJDhRQ4IsH+V
 8Y/9eTNx/HALYtCI6B+JcJ9BKpKFyvkCcBAReHCvpq0f47Q97bBcfn6630pH03c++
 9joxTTZxFv8shXm5VTvr+5GFDJFQZ6UvnoPqByZHvLvB69knjpHqX6E46m2Ki5QYE
10pK5FKoD2wFMjj82+YDoELfVx98r6zw0h7uVeFQbBH4I0EL28RYsXTj5WAAF+wWCj
11Nzfx+ELzNtjpPQ4Kp4PItWSpJWa96+7HD/UmZB6dWcqE38sLF+nMWFHR1r1oDRCW
12s45QqvuQUYDgQW42OnxNd4O8t0vMO6yc0VM5VWd7w7jEDzh03MQSBzrhOIr9Cnd9
13QolSfQmFuvSHnXCoyM8NHC4CW6FFarQU0mjmb765p1sX5Z6im01ivS15c1KCKuCA
14SmsoydGdO7GtKV6ZdG8ut+ztkvBK1146HxpxZBmDjIC9P0nnHzzYIjAztq/qd4sv
15iU/NLnck
16=HoQu
17-----END PGP SIGNATURE-----

Timestamp of file with hash 65490dd75525720cc364871d69f74f20cb23d460478111dc409134823950f8b3 -

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

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

Conflicts