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 🔼

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 11e4333572630de023790d12cb37c30526dcd13d95c1e10af4452856fb74e1a7 -

</details>

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

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash b508a08160a657b0e132e8bbdb91d64571be968c585f6ca2a374baecd1fb4875 -

</details>

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