Net: Move ping data to net_processing #20721

jnewbery · 2020-12-19T11:24:22Z

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

For motivation, see #19398.

jnewbery · 2020-12-22T13:46:17Z

Rebased on master. This is now ready for review.

ajtowns

Seems okay. Though, "jnewbery deleted a comment from sipa 3 days ago" ?

src/net_processing.cpp

src/net_processing.h

src/test/denialofservice_tests.cpp

DrahtBot · 2020-12-23T21:54:44Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net: Avoid discouraging the onion proxy when one inbound onion misbehaves #21190 (net: Avoid discouraging the onion proxy when one inbound onion misbehaves by MarcoFalke)
Make all of net_processing (and some of net) use std::chrono types #21015 (Make all of net_processing (and some of net) use std::chrono types by dhruv)
net: Log to net debug in MaybeDiscourageAndDisconnect except for noban and manual peers #20845 (net: Log to net debug in MaybeDiscourageAndDisconnect except for noban and manual peers by MarcoFalke)
net-processing refactoring -- lose globals, move implementation details from .h to .cpp #20758 (net-processing refactoring -- lose globals, move implementation details from .h to .cpp by ajtowns)
Erlay: bandwidth-efficient transaction relay protocol #18261 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)

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.

jnewbery · 2020-12-23T22:19:02Z

"jnewbery deleted a comment from sipa 3 days ago" ?

ha. There were two comments from sheffine saying "why are you emailing me?" and one from sipa saying "sheffine, you're confused", so I deleted them all to reduce noise.

sipa · 2020-12-23T22:20:53Z

Censorship!!!

(yes, that's indeed what happened)

jnewbery · 2020-12-23T23:27:10Z

Thanks for the review @ajtowns. I'll move the commits around a bit to address your comments.

murchandamus

Can't really comment on the overall goal, as I don't really have a lot of context regarding either locks, mutexes, or peer management, but the linked motivation sounded good to me. The PR is easy to follow and each step makes sense to me. I found the first commit message to be a bit confusing:

No need to create another shared_ptr if ProcessMessages() already
has one in hand.

I take it to mean something along the lines of "Instead of creating another shared ptr, let's pass one that the callsite already has on hand.".

code review ACK 6f0309b

glozow

Concept ACK, although I have a bit of confusion about what constitutes net vs application layer, namely why best ping is still in CNode. You broke a few functional tests by removing minping from copyStats() which I assume is accidental?
For renaming, I personally think "best" is more informative than "min" but there's some inconsistency in naming now. It's still called 'minping' in getpeerinfo and m_min_ping_usec in CNodeStats.

src/net_processing.h

src/net.h

src/net.cpp

jonatack

Concept ACK

A couple of thoughts on preliminary quick skim-through.

src/net.h

src/rpc/net.cpp

jnewbery · 2020-12-29T10:56:50Z

Thanks for the reviews, @glozow and @jonatack. I've addressed your comments: https://github.com/bitcoin/bitcoin/compare/6f0309b4743d05c512fc86d2d206f72d512c5c93..d666804b7b2d5006c37b86f5f88e344216ad0862.

jnewbery · 2020-12-29T11:01:58Z

rebased

jnewbery · 2021-02-15T16:25:09Z

Rebased

glozow · 2021-02-15T20:07:06Z

reACK a5e15ae

rebase-only by git range-diff a3ec04b...a5e15ae

amitiuttarwar

ACK a5e15ae

lost track of the different versions so just re-reviewed the whole diff commit by commit.

definitely like having the m_peer_connect_timeout check as a function in net more than duplicating system time check in net_processing

in the future, I think it could make sense to have -peertimeout be limited to regtest. it's hidden behind debug-help, but technically possible on mainnet. I don't see how it'd be useful on mainnet though.

src/net.cpp

src/net.h

maflcko · 2021-02-16T09:02:44Z

In the scripted diff I don't like that it may modify non-git user files (e.g. a net.h.rej file) in the same folder. Would be better to use git grep -l '...', which would also simplify review, as reviewers don't have to call git grep to verify the replacement was complete over all files, including docs.

review ACK a5e15ae 🥉

Show signature and timestamp

Signature:

Timestamp of file with hash f094bc0ee463f6bec56659b9f20c20ab75a2f829388e57c839500e90f3c196a3 -

src/rpc/net.cpp

src/net_processing.cpp

- rename to ShouldRunInactivityChecks (bitcoin#20721 (comment)) - take optional time now (bitcoin#20721 (comment)) - call from within InactivityChecks (bitcoin#20721 (comment)) - update comment (bitcoin#20721 (comment)) - change ordering of inequality (bitcoin#20721 (comment)) - make inline (bitcoin#20721 (comment))

- rename to ShouldRunInactivityChecks (bitcoin#20721 (comment)) - take optional time now (bitcoin#20721 (comment)) - call from within InactivityChecks (bitcoin#20721 (comment)) - update comment (bitcoin#20721 (comment)) - change ordering of inequality (bitcoin#20721 (comment))

5ed535a [net] Changes to RunInactivityChecks (John Newbery) Pull request description: Updates the RunInactivityChecks() function: - rename to ShouldRunInactivityChecks (#20721 (comment)) - take optional time now (#20721 (comment)) - call from within InactivityChecks (#20721 (comment)) - update comment (#20721 (comment)) - change ordering of inequality (#20721 (comment)) - ~make inline (#20721 (comment) ACKs for top commit: laanwj: Code review ACK 5ed535a Tree-SHA512: e6ac8e8cce5cddc84a52a40c908634c25f58be74512d642840d7bd7fa65c3d90a0f46cc19e4865b3fae7c933138247f58356167a60a5c519305cfd6d05e51f51

…20721 5ed535a [net] Changes to RunInactivityChecks (John Newbery) Pull request description: Updates the RunInactivityChecks() function: - rename to ShouldRunInactivityChecks (bitcoin#20721 (comment)) - take optional time now (bitcoin#20721 (comment)) - call from within InactivityChecks (bitcoin#20721 (comment)) - update comment (bitcoin#20721 (comment)) - change ordering of inequality (bitcoin#20721 (comment)) - ~make inline (bitcoin#20721 (comment) ACKs for top commit: laanwj: Code review ACK 5ed535a Tree-SHA512: e6ac8e8cce5cddc84a52a40c908634c25f58be74512d642840d7bd7fa65c3d90a0f46cc19e4865b3fae7c933138247f58356167a60a5c519305cfd6d05e51f51

Summary: Refactor only. No change in behaviour. Partial backport of [[bitcoin/bitcoin#20721 | core#20721]]: bitcoin/bitcoin@f8b3058 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/D10873

Summary: ``` Moves the logic to prevent running inactivity checks until the peer has been connected for -peertimeout time into its own function. This will be reused by net_processing later. ``` Partial backport of [[bitcoin/bitcoin#20721 | core#20721]]: bitcoin/bitcoin@1a07600 Depends on D10873. 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/D10875

Summary: Partial backport of [[bitcoin/bitcoin#20721 | core#20721]]: bitcoin/bitcoin@0b43b81 Ref T1696. Depends on D10875. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10876

Summary: ``` Ping messages are an application-level mechanism. Move timeout logic from net to net processing. ``` Partial backport of [[bitcoin/bitcoin#20721 | core#20721]]: bitcoin/bitcoin@dd2646d 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/D10877

Summary: Partial backport of [[bitcoin/bitcoin#20721 | core#20721]]: bitcoin/bitcoin@45dcf22 Depends on D10898. 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/D10899

Summary: Original script: ``` -BEGIN VERIFY SCRIPT- sed -i 's/fPingQueued/m_ping_queued/g' src/net_processing.cpp sed -i 's/nMinPingUsecTime/m_min_ping_time/g' src/net.* src/net_processing.cpp src/test/net_tests.cpp sed -i 's/nPingNonceSent/m_ping_nonce_sent/g' src/net_processing.cpp sed -i 's/nPingUsecTime/m_last_ping_time/g' src/net.* -END VERIFY SCRIPT- ``` Completes backport of [[bitcoin/bitcoin#20721 | core#20721]]: bitcoin/bitcoin@a5e15ae Depends on D10899. 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/D10900

jnewbery mentioned this pull request Dec 19, 2020

net processing: Move block inventory state to net_processing #19829

Merged

fanquake added the P2P label Dec 19, 2020

DrahtBot added the Needs rebase label Dec 19, 2020

bitcoin deleted a comment from sheffine Dec 21, 2020

bitcoin deleted a comment from sipa Dec 21, 2020

jnewbery marked this pull request as draft December 21, 2020 10:04

jnewbery force-pushed the 2020-12-ping-in-peer branch from 8417bb2 to 7f736c7 Compare December 22, 2020 13:45

jnewbery marked this pull request as ready for review December 22, 2020 13:46

jnewbery mentioned this pull request Dec 22, 2020

Move remaining application layer data to net processing #19398

Closed

16 tasks

DrahtBot removed the Needs rebase label Dec 22, 2020

This was referenced Dec 23, 2020

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

Closed

[rpc] Remove deprecated fields from getpeerinfo #20755

Merged

ajtowns reviewed Dec 23, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.h Outdated Show resolved Hide resolved

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

jnewbery force-pushed the 2020-12-ping-in-peer branch from 7f736c7 to 6f0309b Compare December 24, 2020 16:13

DrahtBot mentioned this pull request Dec 25, 2020

Erlay: bandwidth-efficient transaction relay protocol #18261

Closed

murchandamus reviewed Dec 28, 2020

View reviewed changes

glozow reviewed Dec 28, 2020

View reviewed changes

src/net_processing.h Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

src/net.cpp Show resolved Hide resolved

jonatack reviewed Dec 28, 2020

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

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

DrahtBot added the Needs rebase label Dec 28, 2020

jnewbery force-pushed the 2020-12-ping-in-peer branch from 6f0309b to d666804 Compare December 29, 2020 10:54

jnewbery force-pushed the 2020-12-ping-in-peer branch from d666804 to 9ee8d46 Compare December 29, 2020 11:02

DrahtBot removed the Needs rebase label Dec 29, 2020

jnewbery force-pushed the 2020-12-ping-in-peer branch from a3ec04b to a5e15ae Compare February 15, 2021 16:25

DrahtBot removed the Needs rebase label Feb 15, 2021

DrahtBot mentioned this pull request Feb 15, 2021

net: Avoid discouraging the onion proxy when one inbound onion misbehaves #21190

Closed

amitiuttarwar approved these changes Feb 15, 2021

View reviewed changes

src/net.cpp Show resolved Hide resolved

src/net.h Show resolved Hide resolved

fanquake merged commit 9bbf08b into bitcoin:master Feb 16, 2021

jnewbery deleted the 2020-12-ping-in-peer branch February 16, 2021 10:55

maflcko reviewed Feb 16, 2021

View reviewed changes

src/rpc/net.cpp Show resolved Hide resolved

src/net_processing.cpp Show resolved Hide resolved

jnewbery mentioned this pull request Feb 16, 2021

net: Address outstanding review comments from PR20721 #21198

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 16, 2021

Merge bitcoin#20721: Net: Move ping data to net_processing

44a718f

ryanofsky mentioned this pull request Feb 20, 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: Move ping data to net_processing #20721

Net: Move ping data to net_processing #20721

jnewbery commented Dec 19, 2020 •

edited

Loading

jnewbery commented Dec 22, 2020

ajtowns left a comment

DrahtBot commented Dec 23, 2020 •

edited

Loading

jnewbery commented Dec 23, 2020

sipa commented Dec 23, 2020

jnewbery commented Dec 23, 2020

murchandamus left a comment

glozow left a comment

jonatack left a comment

jnewbery commented Dec 29, 2020

jnewbery commented Dec 29, 2020

jnewbery commented Feb 15, 2021

glozow commented Feb 15, 2021

amitiuttarwar left a comment

maflcko commented Feb 16, 2021

Net: Move ping data to net_processing #20721

Net: Move ping data to net_processing #20721

Conversation

jnewbery commented Dec 19, 2020 • edited Loading

jnewbery commented Dec 22, 2020

ajtowns left a comment

Choose a reason for hiding this comment

DrahtBot commented Dec 23, 2020 • edited Loading

Conflicts

jnewbery commented Dec 23, 2020

sipa commented Dec 23, 2020

jnewbery commented Dec 23, 2020

murchandamus left a comment

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

jonatack left a comment

Choose a reason for hiding this comment

jnewbery commented Dec 29, 2020

jnewbery commented Dec 29, 2020

jnewbery commented Feb 15, 2021

glozow commented Feb 15, 2021

amitiuttarwar left a comment

Choose a reason for hiding this comment

maflcko commented Feb 16, 2021

jnewbery commented Dec 19, 2020 •

edited

Loading

DrahtBot commented Dec 23, 2020 •

edited

Loading