net processing: Move remaining globals into PeerManagerImpl by dergoegge · Pull Request #24543 · bitcoin/bitcoin

dergoegge · 2022-03-12T16:55:38Z

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.

DrahtBot · 2022-03-12T21:08:50Z

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.

w0xlt · 2022-03-16T03:50:05Z

Concept ACK.

jnewbery

Concept and approach ACK. A few minor suggestions inline.

src/net_processing.cpp

dergoegge · 2022-03-17T18:01:54Z

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

jnewbery

Code review ACK 8c08e33f678bd10254fc4ca1ed6f47ad52b027a5.

A couple of small suggestions inline.

src/net_processing.cpp

jnewbery

Code review ACK 822152c50654228a926ec56819878a5429d75c2e

src/test/denialofservice_tests.cpp

src/net_processing.cpp

jnewbery · 2022-03-21T16:13:38Z

Code review ACK 41b08b114f7494aa452dd5c15c10242651a9d12d

Thank you for such quick responses to my review comments!

dergoegge · 2022-03-22T09:28:38Z

Rebased

jnewbery · 2022-03-22T11:12:18Z

ACK 45f3b1dbebf799b386907de1a4eed20777e92073

Verified rebase by doing it myself and comparing.

dergoegge · 2022-03-25T17:52:27Z

Rebased

jnewbery · 2022-03-25T18:03:27Z

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.

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 · 2022-04-20T14:11:03Z

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

jnewbery

Code review ACK 8fa7fa0cb04661c06fc1fd44667bfebcc6716a9d

src/net_processing.cpp

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

jnewbery · 2022-04-26T09:58:31Z

Code review ACK 778343a

maflcko

ACK 778343a 🗒

Show signature

Signature:

…Manager…

fanquake added the P2P label Mar 12, 2022

dergoegge force-pushed the deglobl_net_processing branch from 406704a to 4e57b8a Compare March 12, 2022 18:28

dergoegge force-pushed the deglobl_net_processing branch from 4e57b8a to 7cba3e8 Compare March 14, 2022 11:32

jnewbery reviewed Mar 17, 2022

View reviewed changes

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

jnewbery reviewed Mar 18, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

dergoegge force-pushed the deglobl_net_processing branch from 8c08e33 to 822152c Compare March 18, 2022 22:59

This was referenced Mar 20, 2022

assumeutxo: net_processing changes #24008

Closed

net/net processing: Move tx inventory into net_processing #21160

Merged

jnewbery reviewed Mar 21, 2022

View reviewed changes

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

src/net_processing.cpp Outdated Show resolved Hide resolved

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

This was referenced Mar 21, 2022

test: Limit scope of id global which is shared between subtests #24627

Merged

test: Extend stale_tip_peer_management test #23352

Closed

DrahtBot added the Needs rebase label Mar 22, 2022

dergoegge force-pushed the deglobl_net_processing branch from 41b08b1 to 45f3b1d Compare March 22, 2022 09:26

DrahtBot added Needs rebase and removed Needs rebase labels Mar 22, 2022

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

DrahtBot removed the Needs rebase label Mar 25, 2022

maflcko added the Refactoring label Mar 29, 2022

This was referenced Apr 15, 2022

deploymentstatus: move g_versionbitscache global to ChainstateManager #24595

Merged

refactor: Move and rename pindexBestHeader, fHavePruned #24909

Merged

DrahtBot added the Needs rebase label Apr 20, 2022

dergoegge added 5 commits April 20, 2022 13:33

[net processing] Move CNodeState declaration above PeerManagerImpl

37ecaf3

[net processing] Move mapNodeState into PeerManagerImpl

a292df2

[net processing] Move nPreferredDownload into PeerManagerImpl

490c08f

[net processing] Move block cache state into PeerManagerImpl

10b83e2

dergoegge force-pushed the deglobl_net_processing branch from 79f6a55 to 8fa7fa0 Compare April 20, 2022 14:09

DrahtBot mentioned this pull request Apr 20, 2022

Strengthen thread safety assertions #24931

Merged

DrahtBot removed the Needs rebase label Apr 20, 2022

jnewbery reviewed Apr 25, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 25, 2022

net processing: Move cleanSubVer, fPreferredDownload and nLocalHostNonce to Peer #24970

Closed

dergoegge added 2 commits April 26, 2022 11:12

[net processing] Move nHighestFastAnnounce into PeerManagerImpl

91c3392

dergoegge force-pushed the deglobl_net_processing branch from 8fa7fa0 to 778343a Compare April 26, 2022 09:20

maflcko approved these changes Apr 30, 2022

View reviewed changes

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 deleted a comment from Meru852 May 1, 2022

bitcoin locked and limited conversation to collaborators May 1, 2023

Conversation

dergoegge commented Mar 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

w0xlt commented Mar 16, 2022

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dergoegge commented Mar 17, 2022

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Mar 21, 2022

Uh oh!

dergoegge commented Mar 22, 2022

Uh oh!

jnewbery commented Mar 22, 2022

Uh oh!

dergoegge commented Mar 25, 2022

Uh oh!

jnewbery commented Mar 25, 2022

Uh oh!

dergoegge commented Apr 20, 2022

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented Apr 26, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dergoegge commented Mar 12, 2022 •

edited

Loading

DrahtBot commented Mar 12, 2022 •

edited

Loading