Refactoring and minor improvement for self-advertisements #19843

naumenkogs · 2020-08-31T11:20:53Z

We have (almost) the same code chunk used twice. Improve that by slight refactoring.

The only behavior change is that the following will be also applied to the first (pre-VERACK) self-announcement:

// If discovery is enabled, sometimes give our peer the address it
// tells us that it sees us as in case it has a better idea of our
// address than we do.

And I think it's a good change anyway.

The last commit is just to consider the first self-announcement as regular and reset the timer, so that we don't send the second self-announcement too soon.
Note that since the first self-announcement is made at the end of processing VERSION, at that point addrLocal would be already filled with what the peers tells us we are, so this self-announcement would be no worse than any further one.

promag · 2020-08-31T12:37:40Z

Build failing

net_processing.cpp:2478:27: error: writing variable 'm_next_local_addr_send' requires holding mutex 'pfrom.cs_sendProcessing' exclusively [-Werror,-Wthread-safety-analysis]
                    pfrom.m_next_local_addr_send = PoissonNextSend(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL);

DrahtBot · 2020-08-31T12:38:51Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net: Move SocketSendData lock annotation to header #20864 (net: Move SocketSendData lock annotation to header by MarcoFalke)
p2p: standardize outbound full/block relay connection type naming #20729 (p2p: standardize on outbound-{full, block}-relay connection type naming by jonatack)
p2p: Add DISABLETX message for negotiating block-relay-only connections #20726 (p2p: Add DISABLETX message for negotiating block-relay-only connections by sdaftuar)
net: fix GetListenPort() to derive the proper port #20196 (net: fix GetListenPort() to derive the proper port by vasild)
netaddress: Simplify reachability logic #17944 (netaddress: Simplify reachability logic by dongcarl)

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.

hebasto · 2020-09-21T07:46:43Z

Concept ACK.

hebasto

c0fc4ee

While AdvertiseLocal() is refactored, mind adding a descriptive Doxygen comment to its declaration?

Moving the responsibility to check fListen to callers looks a bit fragile. Maybe it would better to self-document it with the Assert(fListen); ?

amitiuttarwar

concept ACK. have read through the code, but I'm still understanding the specifics of address semantics to work towards being able to leave a review ACK.

some structural feedback- it took a while for me to discern which parts were pure refactor vs behavioral change. breaking the first commit into 2 separate parts to clarify those boundaries would help ease review.

src/net_processing.cpp

naumenkogs · 2020-10-05T08:17:03Z

@amitiuttarwar how would you suggest to split the first commit into two? I'd be happy to take suggestions :)
I don't see a way to do it, because the refactor is to merge two a little different chunks of code into one (AdvertiseLocal).

The only way I see is to add a flag to AdvertiseLocal to discern the two, and then drop the flag right in the next commit ("behaviour change"). But do you think that would make the code easier to follow? :)

src/net.cpp

src/net.h

src/net.cpp

robot-dreams · 2020-10-06T08:59:03Z

Per @amitiuttarwar 's suggestion, here's one way I might split your first commit 89c77a0 into two separate parts for easier review:

Refactoring-only changes: e4bca91
Behavior changes: 1ca2235

Alternatively, here's a more fine-grained approach, in which I split your first commit 89c77a0 into 8 separate commits, 5 refactoring only commits and 3 behavior changes:

a0a422c...e2c1c58

The more fine-grained approach might also be helpful for PR Review Club participants who are trying to understand exactly what changed in your first commit.

jnewbery

The first commit seems ok, although it should be restructured to separate the pure refactor parts from the behaviour change parts.

I'm not sure that the second commit actually achieves anything. We have a rolling bloom filter m_addr_known for each peer that prevents us from resending the same address multiple times, so I think even if we do call AdvertiseLocal() again shortly after the verack, it won't actually result in us sending our address again.

src/net.cpp

src/net.h

src/net.cpp

src/net_processing.cpp

naumenkogs · 2020-10-06T11:11:13Z

@jnewbery thanks for the feedback, all addressed.
I agree with your hesitation regarding the further commit. It probably doesn't change behavior at all, I just thought it would make it easier to understand what's going on (despite adding 2 extra lines).

Let's see others' preferences, I'm fine with dropping this commit too.

src/net.h

murchandamus

Thanks, I think I have a better understanding what's going on after your response. Just some nitty ideas. :)

src/net.h

src/net.cpp

murchandamus · 2020-10-06T21:07:11Z

src/net.h

+bool IsPeerAddrLocalGood(CNode& pnode);
+
+/**
+ * Add our "best" local address to the batch of addresses we are planning


If we hear multiple addresses for ourselves from peers, how do we determine "best"?

Expanded this comment a bit to refer to GetLocal.
Evaluating the security of GetLocal is on my TODO list, but for now we can assume it works I guess...

fjahr

Concept ACK

src/net.cpp

src/net_processing.cpp

murchandamus

I've re-reviewed. My comments have been addressed and I have no further comments at the moment.

…_local_addr_send This locking was mistakenly introduced in PR bitcoin#13123. Related conversation: bitcoin#13123 (comment) Making these fields atomic would ensure safety if multiple RPC accesses them.

They're not called outside net.cpp, so they only need internal linkage.

These functions can't be called with null ptrs, so change the signatures to take references.

Also make them doxygen comments.

…Local This is a pure refactor commit and moves the checks to the calling function.

Previously AdvertiseLocal() used either the address a peer thinks we are, or the address we think we are. Use both instead (assuming the local one is routable), as it would increase our chances of getting connected to.

Previously, we would prepare to self-announce to a new peer while parsing a VERSION message from that peer. This is useless, because we do something very similar in AdvertiseLocal(), although there are a couple differences: 1) AdvertiseLocal() does this for all peers, not just outbound 2) AdvertiseLocal() always asks the peer to advertise based on what they think we are AND what we think we are (assuming it's routable), while PushAddress(self) on VERSION always does one of the two. (1) and the fact that AdvertiseLocal() is called right before actually sending out ADDR message with our address makes it fully encompassing PushAddress(self) on VERSION. Per (2), AdvertiseLocal() seems like a better version of PushAddress(self) on VERSION. Thus, it's fine to drop PushAddress(self) on VERSION.

DrahtBot · 2021-01-07T15:06:39Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

naumenkogs · 2021-04-16T09:28:08Z

Much changed since I opened this PR. Pretty sure dropping a redundant self-announcement from ProcessMessage still makes sense, but perhaps we should revisit that after #21186 lands.

jnewbery · 2021-07-23T10:37:46Z

Pretty sure dropping a redundant self-announcement from ProcessMessage still makes sense, but perhaps we should revisit that after #21186 lands.

@naumenkogs - do you plan on picking this up now that #21186 is merged? Should we mark this PR up for grabs?

naumenkogs · 2021-07-23T11:03:11Z

@jnewbery Yeah I'd be happy to let someone else lead this. Let's mark it up for grabs.

fanquake · 2022-08-05T15:39:16Z

@dergoegge @mzumsande either of you might be interested in following up with the final commit, 99f6768, if it's still relevant?

mzumsande · 2022-09-27T15:18:32Z

@dergoegge @mzumsande either of you might be interested in following up with the final commit, 99f6768, if it's still relevant?

Only saw this now, because I was looking at the self-advertising logic, and I think it's still relevant, so I'm planing on following up.

Another problem with the self-advertising within Version message processing is that it doesn't work well with addrv2:
If the local address we want to advertise is an outbound peer is an .onion address, then we don't actually advertise it (IsAddrCompatible() returns false) because we haven't received a sendaddrv2 yet at this point. So that is another reason why it makes sense to wait with the advertising until feature negotiation is finished.

956c670 refactor, doc: Improve SetupAddressRelay call in version processing (Martin Zumsande) 3c43d9d p2p: Don't self-advertise during VERSION processing (Gleb Naumenko) Pull request description: This picks up the last commit from bitcoin#19843. Previously, we would prepare to self-announce to a new peer while parsing a `version` message from that peer. This is redundant, because we do something very similar in `MaybeSendAddr()`, which is called from `SendMessages()` after the version handshake is finished. There are a couple of differences: 1) `MaybeSendAddr()` self-advertises to all peers we do address relay with, not just outbound ones. 2) `GetLocalAddrForPeer()` called from `MaybeSendAddr()` makes a probabilistic decision to either advertise what they think we are or what we think we are, while `PushAddress()` on `version` deterministically only does the former if the address from the latter is unroutable. 3) During `version` processing, we haven't received a potential sendaddrv2 message from our peer yet, so self-advertisements with addresses from addrV2-only networks would always be dropped in `PushAddress()`. Since it's confusing to have two slightly different mechanisms for self-advertising, and the one in `MaybeSendAddr()` is better, remove the one in `version`. ACKs for top commit: stratospher: ACK 956c670 naumenkogs: ACK 956c670 amitiuttarwar: reACK 956c670 Tree-SHA512: 933d40615289f055c022170dde7bad0ac0a1d4be377538bfe9ba64375cfeb03bcd803901591f0739ac4850c880e8475a68fd1ab0330800030ab7f19e38c00274

fanquake added the P2P label Aug 31, 2020

naumenkogs force-pushed the 2020-08-refactor-advertiselocal branch from 954e678 to 2fba385 Compare August 31, 2020 15:20

This was referenced Aug 31, 2020

[net] Cleanup connection types- followups #19724

Merged

Periodically make block-relay connections and sync headers #19858

Merged

p2p: Improve diversification of new connections #19860

Closed

DrahtBot added the Needs rebase label Sep 3, 2020

naumenkogs force-pushed the 2020-08-refactor-advertiselocal branch from 2fba385 to 49804a7 Compare September 4, 2020 07:04

DrahtBot removed the Needs rebase label Sep 4, 2020

hebasto requested changes Sep 21, 2020

View reviewed changes

naumenkogs force-pushed the 2020-08-refactor-advertiselocal branch 2 times, most recently from 3d4322c to af81f77 Compare September 23, 2020 07:12

naumenkogs closed this Sep 24, 2020

naumenkogs reopened this Sep 24, 2020

amitiuttarwar reviewed Oct 2, 2020

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

murchandamus reviewed Oct 5, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

naumenkogs force-pushed the 2020-08-refactor-advertiselocal branch from af81f77 to 85c9839 Compare October 6, 2020 07:29

jnewbery reviewed Oct 6, 2020

View reviewed changes

naumenkogs force-pushed the 2020-08-refactor-advertiselocal branch 2 times, most recently from eb59506 to b77272e Compare October 6, 2020 11:13

mzumsande reviewed Oct 6, 2020

View reviewed changes

src/net.h Show resolved Hide resolved

murchandamus reviewed Oct 6, 2020

View reviewed changes

fjahr reviewed Oct 6, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

naumenkogs force-pushed the 2020-08-refactor-advertiselocal branch from b77272e to 25e674e Compare October 7, 2020 06:38

murchandamus reviewed Oct 9, 2020

View reviewed changes

naumenkogs force-pushed the 2020-08-refactor-advertiselocal branch from 5cda36f to 314d755 Compare December 10, 2020 20:43

DrahtBot mentioned this pull request Dec 17, 2020

doc: Remove shouty enums in net_processing comments #20677

Merged

DrahtBot added the Needs rebase label Dec 17, 2020

naumenkogs and others added 7 commits December 18, 2020 10:07

[net] Remove cs_sendProcessing guard from m_next_addr_send and m_next…

ba6dc35

…_local_addr_send This locking was mistakenly introduced in PR bitcoin#13123. Related conversation: bitcoin#13123 (comment) Making these fields atomic would ensure safety if multiple RPC accesses them.

[net] Make GetLocal() and bool AddLocal(CNetAddr&, int) static

ecd2f55

They're not called outside net.cpp, so they only need internal linkage.

[net] Change local address function to take references

b739f21

These functions can't be called with null ptrs, so change the signatures to take references.

[net] Move comments for local address functions to header

d3f789e

Also make them doxygen comments.

[net] remove fListen and fSuccessfullyConnected checks from Advertise…

43d38b5

…Local This is a pure refactor commit and moves the checks to the calling function.

naumenkogs force-pushed the 2020-08-refactor-advertiselocal branch from 314d755 to 99f6768 Compare December 18, 2020 08:15

DrahtBot removed the Needs rebase label Dec 18, 2020

This was referenced Dec 20, 2020

p2p: Add DISABLETX message for negotiating block-relay-only connections #20726

Closed

p2p: standardize outbound full/block relay connection type naming #20729

Closed

DrahtBot mentioned this pull request Jan 6, 2021

net: Move SocketSendData lock annotation to header #20864

Merged

DrahtBot added the Needs rebase label Jan 7, 2021

naumenkogs closed this Apr 16, 2021

fanquake added the Up for grabs label Jul 23, 2021

mzumsande mentioned this pull request Sep 28, 2022

p2p: Don't self-advertise during version processing #26199

Merged

fanquake removed Up for grabs Needs rebase labels Sep 29, 2022

bitcoin locked and limited conversation to collaborators Sep 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring and minor improvement for self-advertisements #19843

Refactoring and minor improvement for self-advertisements #19843

naumenkogs commented Aug 31, 2020 •

edited

Loading

promag commented Aug 31, 2020

DrahtBot commented Aug 31, 2020 •

edited

Loading

hebasto commented Sep 21, 2020

hebasto left a comment

amitiuttarwar left a comment

naumenkogs commented Oct 5, 2020

robot-dreams commented Oct 6, 2020

jnewbery left a comment

naumenkogs commented Oct 6, 2020

murchandamus left a comment

murchandamus Oct 6, 2020

naumenkogs Oct 7, 2020

fjahr left a comment

murchandamus left a comment

DrahtBot commented Jan 7, 2021

naumenkogs commented Apr 16, 2021

jnewbery commented Jul 23, 2021

naumenkogs commented Jul 23, 2021

fanquake commented Aug 5, 2022

mzumsande commented Sep 27, 2022

Refactoring and minor improvement for self-advertisements #19843

Refactoring and minor improvement for self-advertisements #19843

Conversation

naumenkogs commented Aug 31, 2020 • edited Loading

promag commented Aug 31, 2020

DrahtBot commented Aug 31, 2020 • edited Loading

Conflicts

hebasto commented Sep 21, 2020

hebasto left a comment

Choose a reason for hiding this comment

amitiuttarwar left a comment

Choose a reason for hiding this comment

naumenkogs commented Oct 5, 2020

robot-dreams commented Oct 6, 2020

jnewbery left a comment

Choose a reason for hiding this comment

naumenkogs commented Oct 6, 2020

murchandamus left a comment

Choose a reason for hiding this comment

murchandamus Oct 6, 2020

Choose a reason for hiding this comment

naumenkogs Oct 7, 2020

Choose a reason for hiding this comment

fjahr left a comment

Choose a reason for hiding this comment

murchandamus left a comment

Choose a reason for hiding this comment

DrahtBot commented Jan 7, 2021

naumenkogs commented Apr 16, 2021

jnewbery commented Jul 23, 2021

naumenkogs commented Jul 23, 2021

fanquake commented Aug 5, 2022

mzumsande commented Sep 27, 2022

naumenkogs commented Aug 31, 2020 •

edited

Loading

DrahtBot commented Aug 31, 2020 •

edited

Loading