net processing: Move block inventory state to net_processing #19829

jnewbery · 2020-08-28T20:14:50Z

This continues the work of moving application layer data into net_processing, by moving all block inventory state into the new Peer object added in #19607.

For motivation, see #19398.

DrahtBot · 2020-08-28T21:21:18Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

p2p: standardize outbound full/block relay connection type naming #20729 (p2p: standardize on outbound-{full, block}-relay connection type naming by jonatack)
Cleanup of -debug=net log messages #20724 (Cleanup of -debug=net log messages by ajtowns)
tree-wide: De-globalize ChainstateManager #20158 (tree-wide: De-globalize ChainstateManager by dongcarl)

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.

ariard

Just calls for better comments.

src/net_processing.cpp

src/net.h

src/net_processing.cpp

jnewbery · 2020-08-29T09:27:33Z

Thanks for the review @ariard . I've addressed your comments.

jnewbery · 2020-09-02T15:26:13Z

Rebased

jonatack · 2020-09-02T15:34:43Z

Concept ACK

jnewbery · 2020-09-03T15:56:13Z

rebased

hebasto · 2020-09-05T07:06:41Z

Concept ACK.

src/net_processing.cpp

jnewbery · 2020-09-05T09:42:42Z

Thanks for the review @hebasto . I've addressed both of your comments.

hebasto

ACK 0684152, tested on Linux Mint 20 (x86_64).

jnewbery · 2020-09-07T18:51:40Z

rebased

jonatack

ACK 9aad3e4 modulo the RPC fields/help order (see comment below)

src/net_processing.cpp

src/net_processing.h

src/rpc/net.cpp

src/net_processing.h

practicalswift · 2020-12-19T19:48:31Z

Concept ACK

maflcko

review ACK 9aad3e4 🔱

Changes since previous review:

Add lock-order docs
Pass in Peer& to ProcessHeadersMessage
Shuffle order in RPC doc
Rebase
Doc fixups
New commit with a lock

Show signature and timestamp

Signature:

Timestamp of file with hash 431bceaaa7cf87a17743f11d77f4a61c5cdf9d1374a5f4fcd4e5683b564af330 -

src/net_processing.h

src/net_processing.cpp

Not done as a scripted diff to avoid misnaming the local variable in ProcessMessage().

-BEGIN VERIFY SCRIPT- sed -i 's/vBlockHashesToAnnounce/m_blocks_for_headers_relay/g' src/net_processing.* sed -i 's/vInventoryBlockToSend/m_blocks_for_inv_relay/g' src/net_processing.* -END VERIFY SCRIPT-

Also rename to m_continuation_block to better communicate meaning.

jnewbery · 2020-12-20T10:11:15Z

Thanks for the latest reviews @jonatack and @MarcoFalke. I've addressed/responded to all of your comments.

I'd like to stabilize this PR now unless any functional issues are found. We have ACKs on the code change at various times from @MarcoFalke, @jonatack, @hebasto, @sipa and @Sjors. If there are any further style suggestions, I can address them in the next PR in this series.

maflcko

re-ACK 3002b4a 🌓

only changes:

Move RPC help once more
Fix LOCK scope {} nit
Rename a member

Show signature and timestamp

Signature:

Timestamp of file with hash 03268669e27bdf510ed13b406639896a3bedb0573f1b2d13c4c88ed636966237 -

jonatack · 2020-12-20T12:30:16Z

re-ACK 3002b4a per git diff 9aad3e4 3002b4a

Sjors · 2020-12-22T11:37:42Z

Code review re-ACK 3002b4a

…t_proce…

Suggested here: - bitcoin#19607 (comment) - bitcoin#19829 (comment)

Summary: As per title, only comments. Partial backport of [[bitcoin/bitcoin#19829 | core#19829]]: bitcoin/bitcoin@717a374 Ref T1696. Test Plan: Read the comments. Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10865

Summary: Partial backport of [[bitcoin/bitcoin#19829 | core#19829]]: bitcoin/bitcoin@77a2c2f Ref T1696. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10866

Summary: ``` Not done as a scripted diff to avoid misnaming the local variable in ProcessMessage(). ``` Partial backport of [[ bitcoin/bitcoin#19829 | core#19829 ]]: bitcoin/bitcoin@78040f9 Depends on D10866. Ref T1696. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10867

Summary: Partial backport of [[bitcoin/bitcoin#19829 | core#19829]]: bitcoin/bitcoin@53b7ac1 Ref T1696. Depends on D10867. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10868

Summary: Original rename script: ``` -BEGIN VERIFY SCRIPT- sed -i 's/vBlockHashesToAnnounce/m_blocks_for_headers_relay/g' src/net_processing.* sed -i 's/vInventoryBlockToSend/m_blocks_for_inv_relay/g' src/net_processing.* -END VERIFY SCRIPT- ``` Partial backport of [[bitcoin/bitcoin#19829 | core#19829]]: bitcoin/bitcoin@c853ef0 Ref T1696. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10870

Summary: ``` Also rename to m_continuation_block to better communicate meaning. ``` Partial backport of [[bitcoin/bitcoin#19829 | core#19829]]: bitcoin/bitcoin@184557e Depends on D10870. Ref T1696. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10871

Summary: Completes backport of [[bitcoin/bitcoin#19829 | core#19829]]: bitcoin/bitcoin@3002b4a Depends on D10871. Ref T1696. Test Plan: With Clang and debug: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10872

DrahtBot mentioned this pull request Aug 28, 2020

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

Merged

DrahtBot added GUI P2P RPC/REST/ZMQ labels Aug 28, 2020

This was referenced Aug 28, 2020

net, rpc: expose high bandwidth mode state via getpeerinfo #19776

Merged

[RPC] Add connection type to getpeerinfo, improve logs #19725

Merged

[net] Cleanup connection types- followups #19724

Merged

ariard reviewed Aug 29, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net.h Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 2020-08-blocks-in-peer branch from f479be8 to f07c765 Compare August 29, 2020 09:27

This was referenced Sep 1, 2020

net: improve nLastBlockTime and nLastTXTime documentation #19857

Merged

Periodically make block-relay connections and sync headers #19858

Merged

DrahtBot added the Needs rebase label Sep 2, 2020

jnewbery force-pushed the 2020-08-blocks-in-peer branch from f07c765 to 1cc4d31 Compare September 2, 2020 15:26

DrahtBot removed the Needs rebase label Sep 2, 2020

DrahtBot mentioned this pull request Sep 2, 2020

p2p: Improve diversification of new connections #19860

Closed

DrahtBot added the Needs rebase label Sep 3, 2020

jnewbery force-pushed the 2020-08-blocks-in-peer branch from 1cc4d31 to aafa89b Compare September 3, 2020 15:54

DrahtBot removed the Needs rebase label Sep 3, 2020

hebasto reviewed Sep 5, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 2020-08-blocks-in-peer branch from aafa89b to 0684152 Compare September 5, 2020 09:42

hebasto approved these changes Sep 5, 2020

View reviewed changes

DrahtBot added the Needs rebase label Sep 7, 2020

jonatack reviewed Dec 19, 2020

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

src/net_processing.h Show resolved Hide resolved

src/rpc/net.cpp Outdated Show resolved Hide resolved

src/net_processing.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Dec 20, 2020

Cleanup of -debug=net log messages #20724

Merged

maflcko reviewed Dec 20, 2020

View reviewed changes

src/net_processing.h Show resolved Hide resolved

src/net_processing.h Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery added 6 commits December 20, 2020 10:01

[net processing] Move nStartingHeight to Peer

77a2c2f

[net processing] Rename nStartingHeight to m_starting_height

78040f9

Not done as a scripted diff to avoid misnaming the local variable in ProcessMessage().

[net processing] Move block inventory data to Peer

53b7ac1

scripted-diff: rename vBlockHashesToAnnounce and vInventoryBlockToSend

c853ef0

-BEGIN VERIFY SCRIPT- sed -i 's/vBlockHashesToAnnounce/m_blocks_for_headers_relay/g' src/net_processing.* sed -i 's/vInventoryBlockToSend/m_blocks_for_inv_relay/g' src/net_processing.* -END VERIFY SCRIPT-

[net processing] Move hashContinue to net processing

184557e

Also rename to m_continuation_block to better communicate meaning.

[net processing] Guard m_continuation_block with m_block_inv_mutex

3002b4a

jnewbery force-pushed the 2020-08-blocks-in-peer branch from 9aad3e4 to 3002b4a Compare December 20, 2020 10:08

maflcko approved these changes Dec 20, 2020

View reviewed changes

DrahtBot mentioned this pull request Dec 20, 2020

p2p: standardize outbound full/block relay connection type naming #20729

Closed

maflcko merged commit df127ec into bitcoin:master Dec 22, 2020

jnewbery deleted the 2020-08-blocks-in-peer branch December 22, 2020 11:46

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 22, 2020

Merge bitcoin#19829: net processing: Move block inventory state to ne…

ca4c150

…t_proce…

jnewbery mentioned this pull request Dec 23, 2020

net-processing refactoring -- lose globals, move implementation details from .h to .cpp #20758

Closed

mjdietzx pushed a commit to mjdietzx/bitcoin that referenced this pull request Dec 26, 2020

[net processing] Improve documentation for Peer destruction/locking

65e3729

Suggested here: - bitcoin#19607 (comment) - bitcoin#19829 (comment)

ryanofsky mentioned this pull request Jan 29, 2021

Multiprocess bitcoin #10102

Draft

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net processing: Move block inventory state to net_processing #19829

net processing: Move block inventory state to net_processing #19829

jnewbery commented Aug 28, 2020

DrahtBot commented Aug 28, 2020 •

edited

Loading

ariard left a comment

jnewbery commented Aug 29, 2020

jnewbery commented Sep 2, 2020

jonatack commented Sep 2, 2020

jnewbery commented Sep 3, 2020

hebasto commented Sep 5, 2020

jnewbery commented Sep 5, 2020

hebasto left a comment

jnewbery commented Sep 7, 2020

jonatack left a comment

practicalswift commented Dec 19, 2020

maflcko left a comment

jnewbery commented Dec 20, 2020

maflcko left a comment

jonatack commented Dec 20, 2020

Sjors commented Dec 22, 2020

net processing: Move block inventory state to net_processing #19829

net processing: Move block inventory state to net_processing #19829

Conversation

jnewbery commented Aug 28, 2020

DrahtBot commented Aug 28, 2020 • edited Loading

Conflicts

ariard left a comment

Choose a reason for hiding this comment

jnewbery commented Aug 29, 2020

jnewbery commented Sep 2, 2020

jonatack commented Sep 2, 2020

jnewbery commented Sep 3, 2020

hebasto commented Sep 5, 2020

jnewbery commented Sep 5, 2020

hebasto left a comment

Choose a reason for hiding this comment

jnewbery commented Sep 7, 2020

jonatack left a comment

Choose a reason for hiding this comment

practicalswift commented Dec 19, 2020

maflcko left a comment

Choose a reason for hiding this comment

jnewbery commented Dec 20, 2020

maflcko left a comment

Choose a reason for hiding this comment

jonatack commented Dec 20, 2020

Sjors commented Dec 22, 2020

DrahtBot commented Aug 28, 2020 •

edited

Loading