Net processing: Only call PushAddress() from net

jnewbery commented at 12:08 pm on February 15, 2021: member

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.

fanquake added the label P2P on Feb 15, 2021

jnewbery renamed this:
~~[net processing] Add Peer& arg to MaybeDiscourageAndDisconnect()~~
Net processing: only call PushAddress() from net_processing
on Feb 15, 2021

jnewbery renamed this:
~~Net processing: only call PushAddress() from net_processing~~
Net processing: Only call PushAddress() from net_processing
on Feb 15, 2021

jnewbery commented at 1:22 pm on February 16, 2021: member

Rebased. Moving out of draft status.

jnewbery force-pushed on Feb 16, 2021

jnewbery marked this as ready for review on Feb 16, 2021

DrahtBot commented at 4:02 pm on February 16, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21186 (Net/Net Processing: Move addr data into net_processing by jnewbery)
#20196 (net: fix GetListenPort() to derive the proper port by vasild)

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.

in src/net.cpp:224 in 4ed5aedf28 outdated

220@@ -222,10 +221,12 @@ void AdvertiseLocal(CNode *pnode)
221         }
222         if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false))
223         {
224-            LogPrint(BCLog::NET, "AdvertiseLocal: advertising address %s\n", addrLocal.ToString());
225-            pnode->PushAddress(addrLocal, rng);
226+            LogPrint(BCLog::NET, "GetPeerLocalAddr: advertising address %s\n", addrLocal.ToString());

sdaftuar commented at 4:43 pm on February 17, 2021:

Since you’re touching this line anyway, you could consider adding the id of the peer that we’re sending this to – it’s helpful for tracking which addresses we give to which peers, which is otherwise not obvious from our logging.

This is unrelated to the goal of this PR so feel free to ignore, too.

jnewbery commented at 7:48 pm on February 17, 2021:

Done

in src/net.cpp:204 in 4ed5aedf28 outdated

200@@ -201,8 +201,7 @@ bool IsPeerAddrLocalGood(CNode *pnode)
201            IsReachable(addrLocal.GetNetwork());
202 }
203 
204-// pushes our own address to a peer
205-void AdvertiseLocal(CNode *pnode)
206+Optional<CAddress> GetPeerLocalAddr(CNode *pnode)

sdaftuar commented at 4:48 pm on February 17, 2021:

nit: perhaps GetLocalAddrForPeer() might be a clearer name? PeerLocalAddr sounds like it could be the peer’s address, rather than the address of ours that we think is appropriate for the peer.

jnewbery commented at 7:48 pm on February 17, 2021:

Yes, that’s better. Changed.

sdaftuar approved

sdaftuar commented at 5:32 pm on February 17, 2021: member

utACK, just nits.

jnewbery force-pushed on Feb 17, 2021

jnewbery commented at 7:48 pm on February 17, 2021: member

Thanks for the review @sdaftuar! I’ve taken both of your suggestions.

sdaftuar commented at 9:08 pm on February 17, 2021: member

utACK 3c0325e7137f0423c75326b25faa856c5f67b7eb

in src/net.cpp:224 in 9f7d0e42b5 outdated

220@@ -222,10 +221,12 @@ void AdvertiseLocal(CNode *pnode)
221         }
222         if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false))
223         {
224-            LogPrint(BCLog::NET, "AdvertiseLocal: advertising address %s\n", addrLocal.ToString());
225-            pnode->PushAddress(addrLocal, rng);
226+            LogPrint(BCLog::NET, "GetLocalAddrForPeer: advertising address %s to peer=%d\n", addrLocal.ToString(), pnode->GetId());

MarcoFalke commented at 8:44 am on February 18, 2021:

in the first commit: Would be good to remove the function name when touching this. You can use #19809 to get the function name in the log message.

jnewbery commented at 9:45 am on February 18, 2021:

Done

MarcoFalke approved

MarcoFalke commented at 8:54 am on February 18, 2021: member

cr ACK 3c0325e7137f0423c75326b25faa856c5f67b7eb 🔼

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3cr ACK 3c0325e7137f0423c75326b25faa856c5f67b7eb 🔼
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhx2Av/ciCyJfD1yNDR55VEg+5wswy1krjwSeDjeJIlxwwbr5pH5GGB0opCpf7Z
 8OMODIQ9/qtqNnerNcqLvAuru+waN4pBOFgV6qkoAfCPjsfwfqwz7BHXmfdbOVD3g
 9how3qn2gsEKoxPPz9E1Ei7yJeI9a2sg5fmpaWVI8rWXtLFRvIkT9PC0jRauJyRY9
10EsW+OCKnb0LCcmg+DlVvGx/S9op4vnV2e2LHSPaSoEdSnDZ73HxxDn7Z5mMuwQZV
11fceOyU0JAQKvK7HCL/cKffrCnTNX79KD0uRL6mxPQwssM6N4W4CIYaXosj83KBgt
12FtU3SxjFg4GcVChqLLjWXlZt2iCw3ApheqWRJwsWLJgRvKUL8pmsLsOipDT40r6j
138dX/BCWWJg+XlJg4vvY7UPHWNZjWW7BW99fDv7ejeUk8tMldyRbX8DQZSCuaSp96
14CsAr0KtRsuvxfJquxH7vIdizUK0aYrXS/MFyhsSPswwWxQXbljViWMDYuYH+5DLh
15LatvFqnM
16=sO8d
17-----END PGP SIGNATURE-----

Timestamp of file with hash 11e4333572630de023790d12cb37c30526dcd13d95c1e10af4452856fb74e1a7 -

[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.

d21d2b264c

[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.

3e68efa615

jnewbery force-pushed on Feb 18, 2021

jnewbery commented at 9:45 am on February 18, 2021: member

Thanks for the review @MarcoFalke! I’ve taken your suggestion.

MarcoFalke approved

MarcoFalke commented at 2:17 pm on February 18, 2021: member

re-ACK 3e68efa615968e0c9d68a7f197c7852478f6be78 🍅

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 3e68efa615968e0c9d68a7f197c7852478f6be78 🍅
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiTQAv/UHXsGIiH4mmC9FCK7uV0sMXzxWfaNPXlbzc8HRJ5Q5BxO/hpC+vQSpNN
 8Rcg4eWWI+GdpqeX2EXyLCUTckSfQGOtCc+TRztjjYzWIcXojpV/XstKDgqPXj4kk
 9hIAa4BH67gYN3XSX5qqCVwtqcxiCAfb7lGi3oQ2wH/qaQXylIe9lPTx8Ijm1iXtD
10iks+MHRcUhzhKbWIGcY+3lxIAZn7QiBLEyR8FQUYqf4tG1oNftMb8lwaml/ti1GC
11IlxB1ast/LIkSNW4boMHX+QnxODoK+hLteY+7qdUQyZ732wlhq75vWIjNoEVRpki
12G7P/1lPZvsQ47iprz0TBtnrSvZbK80uKtpX+IQsVyMku6brNyA7eB7rbeIir9awu
13TXFMt19dR/VwfX+5BadfcmIAso9S4tAGoKLBb86BosaN21TUxqwt+sxcLyFO5bv8
14vw9tItFxMlpbdJ1fOl39qZ/eiaITn6QyJEfoS7S7TNrdrcENbfJQJdW8xfymbnwQ
15cd7wi0jV
16=z/AJ
17-----END PGP SIGNATURE-----

Timestamp of file with hash b508a08160a657b0e132e8bbdb91d64571be968c585f6ca2a374baecd1fb4875 -

sdaftuar commented at 5:55 pm on February 18, 2021: member

re-ACK

MarcoFalke merged this on Feb 19, 2021

MarcoFalke closed this on Feb 19, 2021

jnewbery deleted the branch on Feb 19, 2021

sidhujag referenced this in commit 750732aa95 on Feb 19, 2021

Fabcien referenced this in commit be3a89e814 on Jan 28, 2022

DrahtBot locked this on Aug 16, 2022

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

Conflicts