net/net processing: Move addr data into net_processing #21186
Conversation
b88830d to
53177dd
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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
53177dd to
79ac127
Compare
…_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
79ac127 to
2315628
Compare
…yAddress() This makes the following commit easier.
2315628 to
468db14
Compare
|
#21236 is merged. Moving this out of draft. |
|
Concept ACK. |
|
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. |
There was a problem hiding this comment.
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?
-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-
0a7e2ff to
0829516
Compare
|
Thanks for the reviews @ajtowns and @amitiuttarwar. I've addressed all of your comments.
I did try that at one point, but it seemed more confusing/difficult than just moving across the addr data/logic in one commit. |
|
Concept ACK |
|
🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file. |
|
Code review ACK 0829516 |
|
Concept ACK |
|
ACK 0829516, reviewed the code and ran tests. |
| 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()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…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
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.