Refactoring and minor improvement for self-advertisements

naumenkogs commented at 11:20 am on August 31, 2020: member

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:

0// If discovery is enabled, sometimes give our peer the address it
1// tells us that it sees us as in case it has a better idea of our
2// 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.

fanquake added the label P2P on Aug 31, 2020

promag commented at 12:37 pm on August 31, 2020: member

Build failing

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

DrahtBot commented at 12:38 pm on August 31, 2020: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20864 (net: Move SocketSendData lock annotation to header by MarcoFalke)
#20729 (p2p: standardize on outbound-{full, block}-relay connection type naming by jonatack)
#20726 (p2p: Add DISABLETX message for negotiating block-relay-only connections by sdaftuar)
#20196 (net: fix GetListenPort() to derive the proper port by vasild)
#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.

naumenkogs force-pushed on Aug 31, 2020

DrahtBot added the label Needs rebase on Sep 3, 2020

naumenkogs force-pushed on Sep 4, 2020

DrahtBot removed the label Needs rebase on Sep 4, 2020

hebasto commented at 7:46 am on September 21, 2020: member

Concept ACK.

hebasto changes_requested

hebasto commented at 9:07 am on September 21, 2020: member

c0fc4ee10fbca1101e8ad4901139f0d59cdaeebb

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); ?

naumenkogs force-pushed on Sep 23, 2020

naumenkogs closed this on Sep 24, 2020

naumenkogs reopened this on Sep 24, 2020

in src/net_processing.cpp:4114 in ba4735db20 outdated

4129@@ -4140,7 +4130,9 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
4130         int64_t nNow = GetTimeMicros();
4131         auto current_time = GetTime<std::chrono::microseconds>();
4132 
4133-        if (pto->RelayAddrsWithConn() && !::ChainstateActive().IsInitialBlockDownload() && pto->m_next_local_addr_send < current_time) {
4134+        if (pto->RelayAddrsWithConn() && !m_chainman.ActiveChainstate().IsInitialBlockDownload() && fListen &&
4135+            pto->fSuccessfullyConnected && pto->m_next_local_addr_send < current_time) {
4136+
4137             AdvertiseLocal(pto);
4138             pto->m_next_local_addr_send = PoissonNextSend(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL);

amitiuttarwar commented at 4:53 pm on October 2, 2020:

shouldn’t the cs_sendProcessing be acquired here? I’m slightly confused because I’d expect a CI job to fail since m_next_local_addr_send has the GUARDED_BY(cs_sendProcessing) safety annotation. am I missing something?

robot-dreams commented at 6:47 pm on October 4, 2020:

Is it the case that this entire SendMessages call is locked? https://github.com/bitcoin/bitcoin/blob/df2129a2349b1877049f250551f49a4592e73765/src/net.cpp#L2164-L2168

amitiuttarwar commented at 11:19 pm on October 5, 2020:

ahhh 💡 that makes sense. thanks!

amitiuttarwar commented at 6:14 pm on October 2, 2020: contributor

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.

naumenkogs commented at 8:17 am on October 5, 2020: member

@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? :)

in src/net.cpp:207 in ba4735db20 outdated

192@@ -193,30 +193,30 @@ bool IsPeerAddrLocalGood(CNode *pnode)
193            IsReachable(addrLocal.GetNetwork());
194 }
195 
196-// pushes our own address to a peer
197 void AdvertiseLocal(CNode *pnode)
198 {
199-    if (fListen && pnode->fSuccessfullyConnected)
200+    assert(fListen);
201+    // First consider our "best" addr for the peer, as seen locally.

murchandamus commented at 5:01 pm on October 5, 2020:

0    // In absence of other information, we assume that the locally seen address is the "best" addr for peers to refer to our node.

murchandamus commented at 5:12 pm on October 5, 2020:

I am not sure whether I am understanding this right. Is this suggesting that we keep track of each addr that some peer refers to us as separately, or are we catering to the possibility that our IP from remote looks different than what we see locally?

naumenkogs commented at 7:17 am on October 6, 2020:

we keep track of each addr that some peer refers to us as separately, or are we catering to the possibility that our IP from remote looks different than what we see locally?

Sort of both :) A remote peer always tells us via which IP they see us (within VERSION message, see addrMe field there). Then, we store this to consider in future. Among all these options, we sometimes choose the best to advertise it to a remote peer instead of advertising what we think is our local addr.

fjahr commented at 9:14 pm on October 6, 2020:

@naumenkogs Was it on purpose that you left the original comment in while also adding the suggestion? Just checking…

naumenkogs commented at 6:25 am on October 7, 2020:

Yes it was. It still applies, I’m just adding a bit clarification I hope :)

in src/net.cpp:217 in ba4735db20 outdated

206+        addrLocal = CAddress(CService(LookupNumeric("127.0.0.1", GetListenPort())), pnode->GetLocalServices());
207+    }
208+
209+    // If discovery is enabled, sometimes give our peer the address it
210+    // tells us that it sees us as in case it has a better idea of our
211+    // address than we do.

murchandamus commented at 5:08 pm on October 5, 2020:

This comment confuses me. If Peer_A gave us “the address it tells us that it sees us as”, why would we need to give Peer_A our address?

Is this comment supposed to mean that we sometimes refer to ourselves as the address that another peer gave us? I.e. we heard from Peer_A that they know us as addrRemote, and we sometimes announce our address to another peer Peer_B as addrRemote instead of addrLocal?

naumenkogs commented at 7:20 am on October 6, 2020:

Hopefully this comment answers.

Why we give it to them? Because we ask them to forward our self-announcement to other nodes. Peer_A is not supposed currently to modify an ADDR they receive, so they just take what we give them and forward it.

You are right that they could, but it has other implications…

murchandamus commented at 8:49 pm on October 6, 2020:

Okay, so we are giving Peer_A the address they gave us to forward to its peers. Now that makes more sense to me. :)

in src/net.cpp:214 in ba4735db20 outdated

230-        if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false))
231-        {
232-            LogPrint(BCLog::NET, "AdvertiseLocal: advertising address %s\n", addrLocal.ToString());
233-            pnode->PushAddress(addrLocal, rng);
234-        }
235+        addrLocal.SetIP(pnode->GetAddrLocal());

murchandamus commented at 5:17 pm on October 5, 2020:

It looks to me as if we are updating here the address that we think we are reachable on per the information of a peer. Would it be possible for different peers to see us at different addresses (e.g. if our node has a client in our LAN)? How do we verify that the address is a good address to adopt?

naumenkogs commented at 7:25 am on October 6, 2020:

Would it be possible for different peers to see us at different addresses (e.g. if our node has a client in our LAN)?

Yes, see SeenLocal and GetLocal, it’s an under-documented scoring scheme.

You are right that it’s not the most robust feature, someone can trick us to believe something wrong, but the harm is limited I guess.

This feature is definitely worth extra research and documenting.

in src/net.h:620 in ba4735db20 outdated

613@@ -614,6 +614,11 @@ enum
614 };
615 
616 bool IsPeerAddrLocalGood(CNode *pnode);
617+
618+/**
619+ * Adds our "best" local address to the batch of addresses we are planning
620+ * to relay a given node as scheduled. The relay is not guaranteed.

murchandamus commented at 5:18 pm on October 5, 2020:

0 * to relay to a given node as scheduled. The relay is not guaranteed.

in src/net.cpp:216 in ba4735db20 outdated

232-            LogPrint(BCLog::NET, "AdvertiseLocal: advertising address %s\n", addrLocal.ToString());
233-            pnode->PushAddress(addrLocal, rng);
234-        }
235+        addrLocal.SetIP(pnode->GetAddrLocal());
236+    }
237+    if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false))

murchandamus commented at 5:37 pm on October 5, 2020:

Since addrLocal seems to be a local variable here, would it make sense to change it to addr_local? My understanding is that per style guide variable symbols should prefer to use snake_case.

naumenkogs commented at 7:27 am on October 6, 2020:

Even though it looks like I rewrote the whole method, most of the changes is just moving the code out of the {} block. I chose to preserve variable names to make review easier. I prefer to not touch existing variable names, unless it’s really easy, or if I’m changing their type for example.

jnewbery commented at 9:38 am on October 6, 2020:

add blank line above and join { below onto this line.

naumenkogs force-pushed on Oct 6, 2020

robot-dreams commented at 8:59 am on October 6, 2020: contributor

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

Refactoring-only changes: e4bca91105d580d11d977e397b78cee28728ef8f
Behavior changes: 1ca2235aaa3cba43e2a1dbccaa89aa8577f91152

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

a0a422c34cfd6514d0cc445bd784d3ee1a2d1749…e2c1c58d8d602cd413e435b03d3c634fb2c5bbd6

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.

in src/net.cpp:213 in 85c9839e57 outdated

210+    // tells us that it sees us as in case it has a better idea of our
211+    // address than we do.
212+    FastRandomContext rng;
213+    if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() ||
214+         rng.randbits((GetnScore(addrLocal) > LOCAL_MANUAL) ? 3 : 1) == 0))
215     {

jnewbery commented at 9:38 am on October 6, 2020:

join with line above while you’re touching this

in src/net.h:619 in 85c9839e57 outdated

613@@ -614,6 +614,11 @@ enum
614 };
615 
616 bool IsPeerAddrLocalGood(CNode *pnode);
617+
618+/**
619+ * Adds our "best" local address to the batch of addresses we are planning

jnewbery commented at 9:39 am on October 6, 2020:

s/Adds/Add/ (make function doxygen comments the indicative case)

in src/net.cpp:196 in 85c9839e57 outdated

192@@ -193,30 +193,30 @@ bool IsPeerAddrLocalGood(CNode *pnode)
193            IsReachable(addrLocal.GetNetwork());
194 }
195 
196-// pushes our own address to a peer
197 void AdvertiseLocal(CNode *pnode)

jnewbery commented at 9:40 am on October 6, 2020:

Since you’re completely refactoring this function and both call sites, you may as well fix the signature to take a CNode& instead of a CNode*. I think you may also be able to make it a const reference.

naumenkogs commented at 10:53 am on October 6, 2020:

I don’t think const reference is possible due to PushAddress call.

in src/net_processing.cpp:2479 in 85c9839e57 outdated

2475@@ -2476,19 +2476,12 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty
2476             // We skip these operations for BLOCK_RELAY peers to avoid
2477             // potentially leaking information about our BLOCK_RELAY
2478             // connections via the addrman or address relay.
2479-            if (fListen && !::ChainstateActive().IsInitialBlockDownload())
2480+            if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload())

jnewbery commented at 9:41 am on October 6, 2020:

Please don’t change ::ChainstateActive to m_chainman.ActiveChainstate in this PR. It adds noise for reviewers.

in src/net_processing.cpp:2483 in 85c9839e57 outdated

2490-                    LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString());
2491-                    pfrom.PushAddress(addr, insecure_rand);
2492-                }
2493+                AdvertiseLocal(&pfrom);
2494+                const auto current_time = GetTime<std::chrono::microseconds>();
2495+                LOCK(pfrom.cs_sendProcessing);

jnewbery commented at 9:42 am on October 6, 2020:

This mutex shouldn’t actually be guarding m_next_local_addr_send. See #13123 (comment). You may as well remove that annotation if you’re touching all the places where this variable is used.

jnewbery commented at 9:51 am on October 6, 2020: contributor

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.

naumenkogs commented at 11:11 am on October 6, 2020: member

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

naumenkogs force-pushed on Oct 6, 2020

in src/net.h:1008 in e8da279a38 outdated

965@@ -966,8 +966,8 @@ class CNode
966     std::vector<CAddress> vAddrToSend;
967     std::unique_ptr<CRollingBloomFilter> m_addr_known{nullptr};
968     bool fGetAddr{false};
969-    std::chrono::microseconds m_next_addr_send GUARDED_BY(cs_sendProcessing){0};
970-    std::chrono::microseconds m_next_local_addr_send GUARDED_BY(cs_sendProcessing){0};

mzumsande commented at 8:54 pm on October 6, 2020:

You removed too much here! Since m_next_local_addr_send is left uninitialized, p2p_node_network_limited.py fails intermittently.

in src/net.h:970 in e8da279a38 outdated

965@@ -966,8 +966,8 @@ class CNode
966     std::vector<CAddress> vAddrToSend;
967     std::unique_ptr<CRollingBloomFilter> m_addr_known{nullptr};
968     bool fGetAddr{false};
969-    std::chrono::microseconds m_next_addr_send GUARDED_BY(cs_sendProcessing){0};
970-    std::chrono::microseconds m_next_local_addr_send GUARDED_BY(cs_sendProcessing){0};
971+    std::chrono::microseconds m_next_addr_send{0};
972+    std::chrono::microseconds m_next_local_addr_send;

murchandamus commented at 8:55 pm on October 6, 2020:

In one line this is dropping only GUARDED_BY(cs_sendProcessing), in the other line the {0} is also dropped. Is it possible that both lines should either drop or retain the {0}?

in src/net.cpp:217 in a8d262bdbd outdated

238+        addrLocal = CAddress(CService(LookupNumeric("127.0.0.1", GetListenPort())), pnode.GetLocalServices());
239+    }
240+
241+    // If discovery is enabled, sometimes give our peer the address it
242+    // tells us that it sees us as in case it has a better idea of our
243+    // address than we do.

murchandamus commented at 9:02 pm on October 6, 2020:

How about:

0// If discovery is enabled, we request that peers advertise our node to their peers. 
1// Under some circumstances, our peers may have a more accurate perspective 
2// on our address than our own node. Sometimes, we ask our peer to advertise 
3// our node under the address they see us as at instead of the address that our 
4// node thinks it has.

naumenkogs commented at 6:31 am on October 7, 2020:

Sounds good, thanks!

in src/net.h:646 in a8d262bdbd outdated

639@@ -640,8 +640,13 @@ enum
640     LOCAL_MAX
641 };
642 
643-bool IsPeerAddrLocalGood(CNode *pnode);
644-void AdvertiseLocal(CNode *pnode);
645+bool IsPeerAddrLocalGood(CNode& pnode);
646+
647+/**
648+ * Add our "best" local address to the batch of addresses we are planning

murchandamus commented at 9:07 pm on October 6, 2020:

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

naumenkogs commented at 6:34 am on October 7, 2020:

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…

murchandamus commented at 9:08 pm on October 6, 2020: contributor

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

in src/net_processing.cpp:4109 in a8d262bdbd outdated

4104@@ -4105,8 +4105,11 @@ bool PeerManager::SendMessages(CNode* pto)
4105         int64_t nNow = GetTimeMicros();
4106         auto current_time = GetTime<std::chrono::microseconds>();
4107 
4108-        if (pto->RelayAddrsWithConn() && !::ChainstateActive().IsInitialBlockDownload() && pto->m_next_local_addr_send < current_time) {
4109-            AdvertiseLocal(pto);
4110+
4111+        if (pto->RelayAddrsWithConn() && !m_chainman.ActiveChainstate().IsInitialBlockDownload() && fListen &&

fjahr commented at 9:46 pm on October 6, 2020:

Same here as #19843 (review) concerning ::ChainstateActive()?

fjahr commented at 9:53 pm on October 6, 2020: contributor

Concept ACK

naumenkogs force-pushed on Oct 7, 2020

murchandamus commented at 5:26 pm on October 9, 2020: contributor

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

in src/net.cpp:218 in 6dc9c7535d outdated

239+    }
240+
241+    // If discovery is enabled, we request that peers advertise our node to their peers. 
242+    // Under some circumstances, our peers may have a more accurate perspective 
243+    // on our address than our own node. Sometimes, we ask our peer to advertise 
244+    // our node under the address they see us as at instead of the address that our

hebasto commented at 12:02 pm on October 10, 2020:

6dc9c7535daa931d7f747958d0dc6fa29417dd80 please remove trailing space to make linter happy:

0    // If discovery is enabled, we request that peers advertise our node to their peers.
1    // Under some circumstances, our peers may have a more accurate perspective
2    // on our address than our own node. Sometimes, we ask our peer to advertise
3    // our node under the address they see us as at instead of the address that our

naumenkogs force-pushed on Oct 12, 2020

in src/net.h:667 in fa16765e4c outdated

663@@ -658,8 +664,8 @@ bool AddLocal(const CNetAddr& addr, int nScore = LOCAL_NONE);
664 void RemoveLocal(const CService& addr);
665 bool SeenLocal(const CService& addr);
666 bool IsLocal(const CService& addr);
667-bool GetLocal(CService &addr, const CNetAddr *paddrPeer = nullptr);
668-CAddress GetLocalAddress(const CNetAddr *paddrPeer, ServiceFlags nLocalServices);
669+bool GetLocal(const CService &addr, const CNetAddr *paddrPeer = nullptr);

jnewbery commented at 1:29 pm on October 12, 2020:

This is wrong. CService shouldn’t be const, and making it const here makes this declaration different from the definition in the cpp file.

In fact, this declaration could just be removed entirely from this header file since the function isn’t used outside the translation unit. You could also make it static in the cpp file to remove external linkage.

in src/net.h:648 in fa16765e4c outdated

645+bool IsPeerAddrLocalGood(CNode& pnode);
646+
647+/**
648+ * Add our "best" local address (see GetLocal) to the batch of addresses
649+ * we are planning to relay to a given node as scheduled.
650+ * The relay is not guaranteed.

jnewbery commented at 1:30 pm on October 12, 2020:

I suggest removing this. The fact that addresses added to vAddrToSend are not guaranteed to be sent is not a property of AdvertiseLocal, so the comment shouldn’t be here.

jnewbery commented at 1:35 pm on October 12, 2020: contributor

Couple of suggestions inline.

You should update the git log of Drop unnecessary lock to actually describe why it’s ok to drop that lock. Git logs shouldn’t primarily rely on github comments for documentation, since people may want to read the logs offline or without access to github. It’s ok to reference a github comment for additional context, but the main description should be in the commit log.

in src/net.h:976 in fa16765e4c outdated

971@@ -966,8 +972,8 @@ class CNode
972     std::vector<CAddress> vAddrToSend;
973     std::unique_ptr<CRollingBloomFilter> m_addr_known{nullptr};
974     bool fGetAddr{false};
975-    std::chrono::microseconds m_next_addr_send GUARDED_BY(cs_sendProcessing){0};
976-    std::chrono::microseconds m_next_local_addr_send GUARDED_BY(cs_sendProcessing){0};
977+    std::chrono::microseconds m_next_addr_send{0};
978+    std::chrono::microseconds m_next_local_addr_send{0};

jnewbery commented at 1:41 pm on October 12, 2020:

It might actually be better to turn these into atomics now that they’re being accessed in both SendMessages and ProcessMessages. We know that those functions always run single threaded, but if someone exposes these values through an RPC then there’ the potential of a data race.

naumenkogs force-pushed on Oct 13, 2020

jnewbery commented at 11:35 am on October 13, 2020: contributor

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.

I’ve been trying to work out whether that really is a good change. When is it better that the first (and potentially only) address that we advertise to the peer is our own view of our best address and when is it better that we advertise the address that they told us?

I think we can make the logic much simpler by always calling PushAddress() with both addresses (as long as we think they’re routable). The vAddrToSend buffer means we’re not sending individual messages for each address, and the m_addr_known filter means we won’t send the same address multiple times. I can’t see any downside to doing this.

jnewbery commented at 11:38 am on October 13, 2020: contributor

The Refactor AdvertiseLocal commit is a bit of a jumble - it’s changing function signatures, removing declarations, moving comments around, changing where variables are checked from functions to their callers and more. I’m sorry, that might have been because I’ve asked you to do too much in this PR. I’d suggest either removing some of those tidy-ups, or split the commit out, so it’s obvious for reviewers what the point of each change is. I’ve done that here: https://github.com/jnewbery/bitcoin/tree/pr19843.1 . Feel free to take that branch if you think it’s an improvement.

in src/net_processing.cpp:2419 in 699d5ed312 outdated

2501-                    addr.SetIP(addrMe);
2502-                    LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString());
2503-                    pfrom.PushAddress(addr, insecure_rand);
2504-                }
2505+            if (fListen && !::ChainstateActive().IsInitialBlockDownload()) {
2506+                AdvertiseLocal(pfrom);

mzumsande commented at 8:08 pm on October 15, 2020:

A question I had during last week’s review club meeting: Since this PR unifies the two spots from which self-advertisements are sent (ProcessMessage -> VERSION only to outbound peers, and SendMessages also to inbound peers) - Does this call during VERSION processing still have a point? If we simply removed it, wouldn’t we self-advertise a few moment later anyway after we are successfully connected (because m_next_local_addr_send is initialized to 0 in the SendMessages() loop)?

sipa commented at 0:07 am on December 8, 2020:

In commit “Unify self-announcement behavior”.

There is no perfect overlap between the two mechanisms that trigger announcing. This one only sends to outbound connections, but does so even during IBD. The other periodic one also sends to inbound connections, but not during IBD.

I don’t know if there is a point to keeping this one.

naumenkogs commented at 9:50 am on December 9, 2020:

Yeah, I also don’t have a good answer here. As Pieter notices, it’s not that pointless, because of the send-during-IBD property. Now, whether that’s something we want is an open question and requires understanding the expectations from addr relay as a whole. I think this could be discussed/implemented separately in another PR.

naumenkogs commented at 1:26 pm on December 10, 2020:

Actually, both of them are non-IBD? Both are inside the !::ChainstateActive().IsInitialBlockDownload() check? So really, the periodic one should encompass the initial one.

The only reason to keep the initial one is if it somehow worked when the other one is not reached. But it’s not true in practice, because the periodic happens right before actually sending ADDR message out.

I think the initial one should be dropped.

DrahtBot added the label Needs rebase on Nov 3, 2020

in src/net_processing.cpp:4110 in 2a0e47909f outdated

4104@@ -4105,8 +4105,11 @@ bool PeerManager::SendMessages(CNode* pto)
4105         int64_t nNow = GetTimeMicros();
4106         auto current_time = GetTime<std::chrono::microseconds>();
4107 
4108-        if (pto->RelayAddrsWithConn() && !::ChainstateActive().IsInitialBlockDownload() && pto->m_next_local_addr_send.load() < current_time) {
4109-            AdvertiseLocal(pto);
4110+
4111+        if (pto->RelayAddrsWithConn() && !::ChainstateActive().IsInitialBlockDownload() && fListen &&
4112+            pto->fSuccessfullyConnected && pto->m_next_local_addr_send.load() < current_time) {

sipa commented at 11:03 pm on December 7, 2020:

In commit ““Refactor AdvertiseLocal”.

The addition of pto->fSuccessfullyConnected here is unnecessary; SendMessages() returns early if that isn’t the case (see line 4064 in commit 2a0e47909fab5c831031e35e871626b463dfee1c).

in src/net.cpp:201 in 2a0e47909f outdated

224-        if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false))
225-        {
226-            LogPrint(BCLog::NET, "AdvertiseLocal: advertising address %s\n", addrLocal.ToString());
227-            pnode->PushAddress(addrLocal, rng);
228-        }
229+    assert(fListen);

sipa commented at 11:06 pm on December 7, 2020:

In commit “Refactor AdvertiseLocal “:

The pnode->fSuccessfullyConnected condition is dropped here. It’s pointless as all AdvertiseLocal call sites guarantee that condition is satisfied anyway, but perhaps add it as an assert?

naumenkogs commented at 12:09 pm on December 9, 2020:

As Travis caught, it doesn’t work :)

We set it on receiving VERACK, but AdvertiseLocal is called on receiving VERSION, which might happen in either order… I assume AdvertiseLocal never worked properly in the first place? Like, it did work in half of the cases on a new connection :)

I suggest we do AdvertiseLocal on VERSION no matter whether we received VERACK or not.

naumenkogs commented at 12:19 pm on December 9, 2020:

Or even better, this should be just done on VERACK.

jnewbery commented at 10:35 am on December 10, 2020:

We set it on receiving VERACK, but AdvertiseLocal is called on receiving VERSION, which might happen in either order

This isn’t true. We must receive the version message before the verack message.

I agree that it’s better to do the initial PushAddress() on receiving the verack.

naumenkogs commented at 12:46 pm on December 10, 2020:

This isn’t true. We must receive the version message before the verack message.

You’re right, thanks. I was confused in my own code stack of commits I guess. This part was just fine before my changes. Let me just update my commit messages now.

naumenkogs force-pushed on Dec 9, 2020

naumenkogs commented at 11:28 am on December 9, 2020: member

Took @jnewbery’s branch and added suggestions from @sipa.

I think we can make the logic much simpler by always calling PushAddress() with both addresses (as long as we think they’re routable).

Yeah, good point. Not only it makes the code simpler, but also makes addr relay better without negative side-effects (I think from the first grasp). I think it deserves a separate PR though.

naumenkogs force-pushed on Dec 9, 2020

naumenkogs commented at 12:40 pm on December 9, 2020: member

~~As I noticed here, the current code had some ambiguity. We send initial AdvertiseLocal only if we receive VERSION before VERACK from the peer (see fSuccessfullyConnected).~~

~~I see two alternatives to fix this:~~ It’s unclear when we should really do AdvertiseLocal for the first time:

Do AdvertiseLocal on VERSION. There is a chance of the peer dropping us (due to undesirable VERSION content) before they get this ADDR message from us, so this may be wasteful.
Do AdvertiseLocal on VERACK. If we do this, we would never ask a peer who rejects our VERSION (and not send VERACK) to announce our addr forward.

That’s it, this is the trade-off I think.

Note that this matters only for outbound-non-IBD case (see discussion). Otherwise, another AdvertiseLocal is called anyway (before sending out local addr).

Also note that this has nothing to do with feelers, because we disconnect them earlier than sending addrs out to them.

So, see the commit “[net] Call AdvertiseLocal on VERACK” where I implement (2).

naumenkogs force-pushed on Dec 9, 2020

DrahtBot removed the label Needs rebase on Dec 9, 2020

naumenkogs closed this on Dec 9, 2020

naumenkogs reopened this on Dec 9, 2020

naumenkogs commented at 2:50 pm on December 9, 2020: member

No clue why fuzzing fails. cc @MarcoFalke

MarcoFalke commented at 3:04 pm on December 9, 2020: member

0fatal: unable to access 'https://github.com/bitcoin-core/qa-assets/': Failed to connect to github.com port 443: Connection timed out

DrahtBot added the label Needs rebase on Dec 9, 2020

naumenkogs force-pushed on Dec 10, 2020

DrahtBot removed the label Needs rebase on Dec 10, 2020

naumenkogs force-pushed on Dec 10, 2020

naumenkogs commented at 8:41 pm on December 10, 2020: member

Okay, so a bunch of new stuff.

I do think AdvertiseLocal encompasses PushAddress(self) on VERSION: both of them are non-IBD, and sending out ADDRs happens after both of them, so dropping the VERSION-one should have no impact. I do it in “[net] Don’t PushAddress(self) on VERSION”.
Well, the only another difference is that there’s a probability that VERSION-one will push how-we-see-us while at the same time AdvertiseLocal will push how-they-see-us (due to the randomization). And then we end up advertising both within the first ADDR message. So, to preserve the behavior, in AdvertiseLocal I always use both how-they-see-us and how-we-see-us (as suggested by John earlier).

The only problem I can think of is that how-they-see-us and how-we-see-us is gonna flow across the network in a batch. This kinda was happening before already with some probability, but still, I’m wondering if this is a privacy leak or not?

Otherwise, we can also switch to always choose one of two with some chance, as we did before, or even maybe do both but with some probability.

Interested in your opinion w.r.t privacy leak @jnewbery @sipa @sdaftuar @mzumsande

(PR post is slightly outdated now, will update it once I’m sure we’re on the same page here)

naumenkogs force-pushed on Dec 10, 2020

DrahtBot added the label Needs rebase on Dec 17, 2020

[net] Remove cs_sendProcessing guard from m_next_addr_send and m_next_local_addr_send

This locking was mistakenly introduced in PR #13123.
Related conversation:
https://github.com/bitcoin/bitcoin/pull/13123#issuecomment-647505130

Making these fields atomic would ensure safety
if multiple RPC accesses them.

ba6dc3576d

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

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

ecd2f5588c

[net] Change local address function to take references

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

b739f21a66

[net] Move comments for local address functions to header

Also make them doxygen comments.

d3f789e004

[net] remove fListen and fSuccessfullyConnected checks from AdvertiseLocal

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

43d38b5647

[net] Use both locally and remotely seen addresses of us for advertising

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.

10fc62f0c2

[net] Don't PushAddress(self) on VERSION

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.

99f67685a2

naumenkogs force-pushed on Dec 18, 2020

DrahtBot removed the label Needs rebase on Dec 18, 2020

DrahtBot added the label Needs rebase on Jan 7, 2021

DrahtBot commented at 3:06 pm on January 7, 2021: contributor

🐙 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 commented at 9:28 am on April 16, 2021: member

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.

naumenkogs closed this on Apr 16, 2021

jnewbery commented at 10:37 am on July 23, 2021: contributor

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 commented at 11:03 am on July 23, 2021: member

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

fanquake added the label Up for grabs on Jul 23, 2021

fanquake commented at 3:39 pm on August 5, 2022: member

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

mzumsande commented at 3:18 pm on September 27, 2022: contributor

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

fanquake removed the label Up for grabs on Sep 29, 2022

fanquake removed the label Needs rebase on Sep 29, 2022

MarcoFalke referenced this in commit 6061eb6564 on Dec 12, 2022

sidhujag referenced this in commit 6790331747 on Dec 12, 2022

bitcoin locked this on Sep 29, 2023

Refactoring and minor improvement for self-advertisements #19843

Conflicts