Skip to content

net processing: Move remaining globals into PeerManagerImpl #24543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maflcko merged 7 commits into from
Apr 30, 2022
Merged

net processing: Move remaining globals into PeerManagerImpl #24543

maflcko merged 7 commits into from
Apr 30, 2022

Conversation

@dergoegge
Copy link
Member

@dergoegge dergoegge commented Mar 12, 2022 •
edited
Loading

This PR moves the remaining net processing globals into PeerManagerImpl. This will make testing the peer manager in isolation easier and also acts as a code clean up.

@fanquake fanquake added the P2P label Mar 12, 2022
@dergoegge dergoegge force-pushed the deglobl_net_processing branch from 406704a to 4e57b8a Compare March 12, 2022 18:28
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 12, 2022 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24970 (net processing: Move cleanSubVer, fPreferredDownload and nLocalHostNonce to Peer by jnewbery)
  • #24931 (Strengthen thread safety assertions by ajtowns)
  • #24474 (Additional thread safety annotations for CNode/Peer by ajtowns)
  • #24062 (refactor: replace RecursiveMutex cs_most_recent_block with Mutex (and rename) by theStack)
  • #24008 (assumeutxo: net_processing changes by jamesob)
  • #20799 (net processing: Only support version 2 compact blocks by jnewbery)

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.

This was referenced Mar 13, 2022
Closed
Closed
Merged
Merged
Closed
Merged
Merged
Closed
Merged
@dergoegge dergoegge force-pushed the deglobl_net_processing branch from 4e57b8a to 7cba3e8 Compare March 14, 2022 11:32
@w0xlt
Copy link
Contributor

w0xlt commented Mar 16, 2022

Concept ACK.

jnewbery
jnewbery reviewed Mar 17, 2022
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept and approach ACK. A few minor suggestions inline.

@dergoegge dergoegge force-pushed the deglobl_net_processing branch from 7cba3e8 to 8c08e33 Compare March 17, 2022 18:00
@dergoegge
Copy link
Member Author

dergoegge commented Mar 17, 2022

Thanks @jnewbery for the review! I took all your suggestions.

jnewbery
jnewbery reviewed Mar 18, 2022
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 8c08e33f678bd10254fc4ca1ed6f47ad52b027a5.

A couple of small suggestions inline.

@dergoegge dergoegge force-pushed the deglobl_net_processing branch from 8c08e33 to 822152c Compare March 18, 2022 22:59
This was referenced Mar 20, 2022
Closed
Merged
jnewbery
jnewbery reviewed Mar 21, 2022
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 822152c50654228a926ec56819878a5429d75c2e

@dergoegge dergoegge force-pushed the deglobl_net_processing branch from 822152c to 41b08b1 Compare March 21, 2022 15:37
@jnewbery
Copy link
Contributor

jnewbery commented Mar 21, 2022

Code review ACK 41b08b114f7494aa452dd5c15c10242651a9d12d

Thank you for such quick responses to my review comments!

This was referenced Mar 21, 2022
Merged
Closed
@dergoegge dergoegge force-pushed the deglobl_net_processing branch from 41b08b1 to 45f3b1d Compare March 22, 2022 09:26
@dergoegge
Copy link
Member Author

dergoegge commented Mar 22, 2022

Rebased

@jnewbery
Copy link
Contributor

jnewbery commented Mar 22, 2022

ACK 45f3b1dbebf799b386907de1a4eed20777e92073

Verified rebase by doing it myself and comparing.

@dergoegge dergoegge force-pushed the deglobl_net_processing branch from 45f3b1d to 79f6a55 Compare March 25, 2022 17:51
@dergoegge
Copy link
Member Author

dergoegge commented Mar 25, 2022

Rebased

@jnewbery
Copy link
Contributor

jnewbery commented Mar 25, 2022

Code review ACK 79f6a554354fa923167979faf8cf8f3120fc9bfa

I noticed the following line in PeerManagerImpl::NewPOWValidBlock():

    static int nHighestFastAnnounce = 0;

It's perhaps out of scope for this PR, but perhaps that should be made a member variable of PeerManagerImpl alongside the cached recent block members.

This was referenced Apr 15, 2022
Merged
Merged
@dergoegge
[net processing] Move CNodeState declaration above PeerManagerImpl
37ecaf3
@dergoegge
[net processing] Move mapNodeState into PeerManagerImpl
a292df2
@dergoegge
[net processing] Move nPreferredDownload into PeerManagerImpl
490c08f
@dergoegge
[net processing] Inline and simplify UpdatePreferredDownload
a4c55a9 
We inline `UpdatePreferredDownload` because it is only used in one
location during the version handshake. We simplify it by removing the
initial subtraction of `state->fPreferredDownload` from
`nPreferredDownload`. This is ok since the version handshake is only
called once per peer and `state->fPreferredDownload` will always be
false before the newly inlined code is called, making the subtraction a
noop.
@dergoegge
[net processing] Move block cache state into PeerManagerImpl
10b83e2
@dergoegge dergoegge force-pushed the deglobl_net_processing branch from 79f6a55 to 8fa7fa0 Compare April 20, 2022 14:09
@dergoegge
Copy link
Member Author

dergoegge commented Apr 20, 2022

Rebased and added a commit (da9e51a1fe704d4a6a93de34e6837fab37655860) to move nHighestFastAnnounce into PeerManagerImpl.

@DrahtBot DrahtBot mentioned this pull request Apr 20, 2022
Merged
jnewbery
jnewbery reviewed Apr 25, 2022
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 8fa7fa0cb04661c06fc1fd44667bfebcc6716a9d

@DrahtBot DrahtBot mentioned this pull request Apr 25, 2022
Closed
@dergoegge
[net processing] Move nHighestFastAnnounce into PeerManagerImpl
91c3392
@dergoegge
scripted-diff: Rename PeerManagerImpl members
778343a 
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }

ren mapNodeState                              m_node_states
ren cs_most_recent_block                      m_most_recent_block_mutex
ren most_recent_block                         m_most_recent_block
ren most_recent_compact_block                 m_most_recent_compact_block
ren most_recent_block_hash                    m_most_recent_block_hash
ren fWitnessesPresentInMostRecentCompactBlock m_most_recent_compact_block_has_witnesses
ren nPreferredDownload                        m_num_preferred_download_peers
ren nHighestFastAnnounce                       m_highest_fast_announce
-END VERIFY SCRIPT-
@dergoegge dergoegge force-pushed the deglobl_net_processing branch from 8fa7fa0 to 778343a Compare April 26, 2022 09:20
@jnewbery
Copy link
Contributor

jnewbery commented Apr 26, 2022

Code review ACK 778343a

maflcko
maflcko approved these changes Apr 30, 2022
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 778343a 🗒

Show signature

Signature:

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

ACK 778343a379026ef233dffea67f5226565f6d5720 🗒
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjBIwv+MjgYz0poUymP0rv825e0CNNMrgOXg5hJmGXAF7dDpzn6Kqje2icEX1Iy
vNbpPsRDachm/67cOdTJinNgw8TStVI+KxURKCSCl1A30Xz01groLbTyXjVn2gwy
Bk9HoIRpa0CLbvGenk2OW6jjxYFgbK81TKDTHqHEM6laayMlKHFaloO/AlB+SfYL
TRNCBjxGnzoBzPfG3d5JMI5Rt15lBSvKRm6dCe5pnUdDLJyfJLVT5CIb606Tkxoz
0ASF4H0nvsdPCR5t3T+spy8fFDgmsUwtUNbpOAKKkm3Vmr1EGxHxyGCW3Uyq3eLQ
YKefQrFjp21dcxeCeCcsZp30Ropssp1Dc1bOBgPIworvUzTlb8H/WU1aDr16Eis+
/GITCLQ75eifYYaPPEPnJxLwW8QLE4k38zhmYKtR8EafRpB8Zh1NAt+TSWB8iSOl
yjRfMQGD7TeIDVnXzOz+fEwzI7JVsexqhu1BD14Xfx3pUsXfclbnFWHH2/0XaTwp
59A5jsEa
=56fQ
-----END PGP SIGNATURE-----

@maflcko maflcko merged commit 5d53cf3 into bitcoin:master Apr 30, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 30, 2022
Merge bitcoin#24543: net processing: Move remaining globals into Peer…
064218a 
…Manager…
@bitcoin bitcoin deleted a comment from Meru852 May 1, 2022
@bitcoin bitcoin locked and limited conversation to collaborators May 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@jnewbery jnewbery jnewbery left review comments

@maflcko maflcko maflcko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dergoegge @DrahtBot @w0xlt @jnewbery @maflcko @fanquake