net/net processing: Move addr data into net_processing by jnewbery · Pull Request #21186 · bitcoin/bitcoin

jnewbery · 2021-02-15T12:04:37Z

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

For motivation, see #19398.

DrahtBot · 2021-02-18T05:16:52Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

[POC] Rust based Cuckoo Filter for m_addr_known #21837 ([POC] Rust based Cuckoo Filter for m_addr_known by fanquake)
net_processing: lock clean up #21527 (net_processing: lock clean up by ajtowns)
p2p: update inbound eviction protection for multiple networks, add I2P peers #21261 (p2p: update inbound eviction protection for multiple networks, add I2P peers by jonatack)

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.

…sing 3e68efa [net] Move checks from GetLocalAddrForPeer to caller (John Newbery) d21d2b2 [net] Change AdvertiseLocal to GetLocalAddrForPeer (John Newbery) Pull request description: This is the first part of #21186. It slightly disentangles addr handling in net/net_processing by making it explicit that net_processing is responsible for pushing addr records into `vAddrToSend`. ACKs for top commit: MarcoFalke: re-ACK 3e68efa 🍅 Tree-SHA512: 9af50c41f5a977e2e277f24a589db38e2980b353401def5e74b108ac5f493d9b5d6b1b8bf15323a4d66321495f04bc271450fcef7aa7d1c095f051a4f8e9b15f

…_processing 3e68efa [net] Move checks from GetLocalAddrForPeer to caller (John Newbery) d21d2b2 [net] Change AdvertiseLocal to GetLocalAddrForPeer (John Newbery) Pull request description: This is the first part of bitcoin#21186. It slightly disentangles addr handling in net/net_processing by making it explicit that net_processing is responsible for pushing addr records into `vAddrToSend`. ACKs for top commit: MarcoFalke: re-ACK 3e68efa 🍅 Tree-SHA512: 9af50c41f5a977e2e277f24a589db38e2980b353401def5e74b108ac5f493d9b5d6b1b8bf15323a4d66321495f04bc271450fcef7aa7d1c095f051a4f8e9b15f

src/net_processing.cpp

…yAddress() This makes the following commit easier.

…Impl

jnewbery · 2021-04-01T08:10:47Z

#21236 is merged. Moving this out of draft.

hebasto · 2021-04-02T23:02:24Z

Concept ACK.

jnewbery · 2021-04-12T09:40:02Z

Thank you for the careful review @mzumsande. You caught two rebase errors 😳

I've fixed those and addressed your other comment. I've also made some minor edits to comments to clarify concepts, and also re-reviewed everything to make sure no other errors have crept in.

ajtowns

The "Move addr relay data and logic into net processing" commit is doing a lot of things, and I think that makes it a bit hard to review (and leads to rebase errors?). Might be worth putting a bit of effort into splitting it up further if some of the changes can be isolated?

src/net_processing.cpp

amitiuttarwar

started reviewing, looks good so far, will be awesome to have addr handling in net processing :)

src/net_processing.cpp

-BEGIN VERIFY SCRIPT- ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); } ren fGetAddr m_getaddr_sent ren fSentAddr m_getaddr_recvd ren vAddrToSend m_addrs_to_send -END VERIFY SCRIPT-

jnewbery · 2021-04-30T10:40:23Z

Thanks for the reviews @ajtowns and @amitiuttarwar. I've addressed all of your comments.

The "Move addr relay data and logic into net processing" commit is doing a lot of things, and I think that makes it a bit hard to review (and leads to rebase errors?). Might be worth putting a bit of effort into splitting it up further if some of the changes can be isolated?

I did try that at one point, but it seemed more confusing/difficult than just moving across the addr data/logic in one commit.

practicalswift · 2021-04-30T15:53:09Z

Concept ACK

DrahtBot · 2021-05-03T09:33:58Z

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

laanwj · 2021-05-19T11:57:09Z

Code review ACK 0829516

jonatack · 2021-05-20T19:11:57Z

Concept ACK

mzumsande · 2021-05-20T21:07:15Z

ACK 0829516, reviewed the code and ran tests.

sipa

utACK 0829516

sipa · 2021-05-21T20:54:51Z

src/net_processing.cpp

-        PeerRef peer = std::make_shared<Peer>(nodeid);
+        // Addr relay is disabled for outbound block-relay-only peers to
+        // prevent adversaries from inferring these links from addr traffic.
+        PeerRef peer = std::make_shared<Peer>(nodeid, /* addr_relay = */ !pnode->IsBlockOnlyConn());


Using !IsBlockOnlyConn() as proxy for "should we relay addresses" goes a bit in the opposite direction as the earlier connection types refactors went in. Any reason not just keep the function name (I realize there's a derived one with that name added, but it's ultimately still relying on this test.

I don't have a particularly strong opinion here; I'm just noticing some flipflopping.

This logic of checking IsBlockOnlyConn() is used only when calling the Peer ctor. After that, we use the RelayAddrsWithPeer(), which replaces the old RelayAddrsWithConn() function. That's a clearer separation between net and net_processing since we only check the connection type when initializing the Peer object.

hebasto

re-ACK 0829516

…essing

…yAddress() Summary: This is a backport of [[bitcoin/bitcoin#21186 | core#21186]] [1/5] bitcoin/bitcoin@86acc96 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10927

…Impl Summary: This is a backport of [[bitcoin/bitcoin#21186 | core#21186]] [2/5] bitcoin/bitcoin@caba7ae Depends on D10927 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10928

Summary: This is a backport of [[bitcoin/bitcoin#21186 | core#21186]] [3/5] bitcoin/bitcoin@76568a3 Depends on D10928 Test Plan: `ninja all check-all` `ninja bitcoin-fuzzers` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10929

Summary: ``` -BEGIN VERIFY SCRIPT- ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); } ren fGetAddr m_getaddr_sent ren fSentAddr m_getaddr_recvd ren vAddrToSend m_addrs_to_send -END VERIFY SCRIPT- ``` This is a backport of [[bitcoin/bitcoin#21186 | core#21186]] [4/5] bitcoin/bitcoin@09cc66c Depends on D10929 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10930

Summary: This is a backport of [[bitcoin/bitcoin#21186 | core#21186]] [5/5] bitcoin/bitcoin@0829516 Depends on D10930 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10931

fanquake added the P2P label Feb 15, 2021

jnewbery marked this pull request as draft February 15, 2021 12:06

This was referenced Feb 15, 2021

Net processing: Only call PushAddress() from net_processing #21187

Merged

Move remaining application layer data to net processing #19398

Closed

jnewbery force-pushed the 2020-02-addr-in-peer branch from b88830d to 53177dd Compare February 17, 2021 19:58

jnewbery force-pushed the 2020-02-addr-in-peer branch from 53177dd to 79ac127 Compare February 19, 2021 13:12

DrahtBot mentioned this pull request Feb 19, 2021

net processing: Extract addr send functionality into MaybeSendAddr() #21236

Merged

This was referenced Feb 22, 2021

p2p: update inbound eviction protection for multiple networks, add I2P peers #21261

Merged

[Bundle 4/n] Prune g_chainman usage in validation-adjacent modules #21270

Merged

jnewbery force-pushed the 2020-02-addr-in-peer branch from 79ac127 to 2315628 Compare March 6, 2021 14:36

This was referenced Mar 6, 2021

net: Address outstanding review comments from PR20721 #21198

Merged

[Bundle 5/n] Prune g_chainman usage in RPC modules #21391

Merged

refactor: Remove MakeUnique<T>() #21404

Merged

refactor: remove Optional & nullopt #21415

Merged

fanquake reviewed Mar 12, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery changed the title ~~Net/Net Processing: Move addr data into net_processing~~ net/net processing: Move addr data into net_processing Mar 30, 2021

jnewbery added 2 commits April 1, 2021 08:08

[net processing] Take NodeId instead of CNode* as originator for Rela…

86acc96

…yAddress() This makes the following commit easier.

[net processing] Make RelayAddress() a member function of PeerManager…

caba7ae

…Impl

jnewbery force-pushed the 2020-02-addr-in-peer branch from 2315628 to 468db14 Compare April 1, 2021 08:10

jnewbery marked this pull request as ready for review April 1, 2021 08:10

naumenkogs mentioned this pull request Apr 16, 2021

Refactoring and minor improvement for self-advertisements #19843

Closed

ajtowns mentioned this pull request Apr 20, 2021

net_processing: lock clean up #21527

Closed

ajtowns reviewed Apr 20, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 27, 2021

net processing: Only support version 2 compact blocks #20799

Merged

amitiuttarwar reviewed Apr 29, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery added 3 commits April 30, 2021 11:29

[net processing] Move addr relay data and logic into net processing

76568a3

scripted-diff: rename address relay fields

09cc66c

-BEGIN VERIFY SCRIPT- ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); } ren fGetAddr m_getaddr_sent ren fSentAddr m_getaddr_recvd ren vAddrToSend m_addrs_to_send -END VERIFY SCRIPT-

[refactor] Remove unused ForEachNodeThen() template

0829516

jnewbery force-pushed the 2020-02-addr-in-peer branch from 0a7e2ff to 0829516 Compare April 30, 2021 10:29

DrahtBot mentioned this pull request May 3, 2021

[POC] Rust based Cuckoo Filter for m_addr_known #21837

Closed

sipa reviewed May 21, 2021

View reviewed changes

hebasto approved these changes May 24, 2021

View reviewed changes

fanquake merged commit d3fa42c into bitcoin:master May 24, 2021

jnewbery deleted the 2020-02-addr-in-peer branch May 24, 2021 12:30

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 25, 2021

Merge bitcoin#21186: net/net processing: Move addr data into net_proc…

8945437

…essing

JaredTate mentioned this pull request Oct 30, 2021

WIP: Feature/8.22.0 Random Crash Bug & Missing Dandelion Relay TX Code DigiByte-Core/digibyte#77

Closed

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge d3fa42c into merged_master (Bitcoin PR bitcoin/bitcoin#21186)

092f386

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

Conversation

jnewbery commented Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

jnewbery commented Apr 1, 2021

Uh oh!

hebasto commented Apr 2, 2021

Uh oh!

jnewbery commented Apr 12, 2021

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amitiuttarwar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Apr 30, 2021

Uh oh!

practicalswift commented Apr 30, 2021

Uh oh!

DrahtBot commented May 3, 2021

Uh oh!

laanwj commented May 19, 2021

Uh oh!

jonatack commented May 20, 2021

Uh oh!

mzumsande commented May 20, 2021

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

sipa May 21, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery May 24, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

jnewbery commented Feb 15, 2021 •

edited

Loading

DrahtBot commented Feb 18, 2021 •

edited

Loading