Net processing: Only call PushAddress() from net_processing #21187
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
16 tasks
|
Rebased. Moving out of draft status. |
331cc56 to
2c12d14
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. |
2c12d14 to
3c0325e
Compare
|
Thanks for the review @sdaftuar! I've taken both of your suggestions. |
|
utACK 3c0325e |
maflcko
approved these changes
Feb 18, 2021
There was a problem hiding this comment.
cr ACK 3c0325e 🔼
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
cr ACK 3c0325e7137f0423c75326b25faa856c5f67b7eb 🔼
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhx2Av/ciCyJfD1yNDR55VEg+5wswy1krjwSeDjeJIlxwwbr5pH5GGB0opCpf7Z
OMODIQ9/qtqNnerNcqLvAuru+waN4pBOFgV6qkoAfCPjsfwfqwz7BHXmfdbOVD3g
how3qn2gsEKoxPPz9E1Ei7yJeI9a2sg5fmpaWVI8rWXtLFRvIkT9PC0jRauJyRY9
EsW+OCKnb0LCcmg+DlVvGx/S9op4vnV2e2LHSPaSoEdSnDZ73HxxDn7Z5mMuwQZV
fceOyU0JAQKvK7HCL/cKffrCnTNX79KD0uRL6mxPQwssM6N4W4CIYaXosj83KBgt
FtU3SxjFg4GcVChqLLjWXlZt2iCw3ApheqWRJwsWLJgRvKUL8pmsLsOipDT40r6j
8dX/BCWWJg+XlJg4vvY7UPHWNZjWW7BW99fDv7ejeUk8tMldyRbX8DQZSCuaSp96
CsAr0KtRsuvxfJquxH7vIdizUK0aYrXS/MFyhsSPswwWxQXbljViWMDYuYH+5DLh
LatvFqnM
=sO8d
-----END PGP SIGNATURE-----
Timestamp of file with hash 11e4333572630de023790d12cb37c30526dcd13d95c1e10af4452856fb74e1a7 -
Gossiping addresses to peers is the responsibility of net processing. Change AdvertiseLocal() in net to just return an (optional) address for net processing to advertise. Update function name to reflect new responsibility.
GetLocalAddrForPeer() is only called in one place. The checks inside that function make more sense to be carried out be the caller: - fSuccessfullyConnected is already checked at the top of SendMessages(), so must be true when we call GetLocalAddrForPeer() - fListen can go into the conditional before GetLocalAddrForPeer() is called.
3c0325e to
3e68efa
Compare
|
Thanks for the review @MarcoFalke! I've taken your suggestion. |
maflcko
approved these changes
Feb 18, 2021
There was a problem hiding this comment.
re-ACK 3e68efa 🍅
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK 3e68efa615968e0c9d68a7f197c7852478f6be78 🍅
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiTQAv/UHXsGIiH4mmC9FCK7uV0sMXzxWfaNPXlbzc8HRJ5Q5BxO/hpC+vQSpNN
Rcg4eWWI+GdpqeX2EXyLCUTckSfQGOtCc+TRztjjYzWIcXojpV/XstKDgqPXj4kk
hIAa4BH67gYN3XSX5qqCVwtqcxiCAfb7lGi3oQ2wH/qaQXylIe9lPTx8Ijm1iXtD
iks+MHRcUhzhKbWIGcY+3lxIAZn7QiBLEyR8FQUYqf4tG1oNftMb8lwaml/ti1GC
IlxB1ast/LIkSNW4boMHX+QnxODoK+hLteY+7qdUQyZ732wlhq75vWIjNoEVRpki
G7P/1lPZvsQ47iprz0TBtnrSvZbK80uKtpX+IQsVyMku6brNyA7eB7rbeIir9awu
TXFMt19dR/VwfX+5BadfcmIAso9S4tAGoKLBb86BosaN21TUxqwt+sxcLyFO5bv8
vw9tItFxMlpbdJ1fOl39qZ/eiaITn6QyJEfoS7S7TNrdrcENbfJQJdW8xfymbnwQ
cd7wi0jV
=z/AJ
-----END PGP SIGNATURE-----
Timestamp of file with hash b508a08160a657b0e132e8bbdb91d64571be968c585f6ca2a374baecd1fb4875 -
|
re-ACK |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Feb 19, 2021
…_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
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jan 28, 2022
Summary: > [net] Change AdvertiseLocal to GetLocalAddrForPeer > > Gossiping addresses to peers is the responsibility of net processing. > Change AdvertiseLocal() in net to just return an (optional) address > for net processing to advertise. Update function name to reflect > new responsibility. > [net] Move checks from GetLocalAddrForPeer to caller > > GetLocalAddrForPeer() is only called in one place. The checks inside that > function make more sense to be carried out be the caller: > > - fSuccessfullyConnected is already checked at the top of > SendMessages(), so must be true when we call GetLocalAddrForPeer() > - fListen can go into the conditional before GetLocalAddrForPeer() is > called. This is a backport of [[bitcoin/bitcoin#21187 | core#21187]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10922
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.