net/net processing: Move addr data into net

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

This continues the work of moving application layer data into net_processing, by moving all addr data into the new Peer object added in #19607.

For motivation, see #19398.

fanquake added the label P2P on Feb 15, 2021

jnewbery marked this as a draft on Feb 15, 2021

jnewbery force-pushed on Feb 17, 2021

DrahtBot commented at 5:16 am on February 18, 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:

#21837 ([POC] Rust based Cuckoo Filter for m_addr_known by fanquake)
#21527 (net_processing: lock clean up by ajtowns)
#21261 (p2p: update inbound eviction protection for multiple networks, add I2P peers by jonatack)

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.

MarcoFalke referenced this in commit 09eb46c943 on Feb 19, 2021

jnewbery force-pushed on Feb 19, 2021

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

jnewbery force-pushed on Mar 6, 2021

in src/net_processing.cpp:241 in 231562806b outdated

236@@ -216,7 +237,8 @@ struct Peer {
237     /** Work queue of items requested by this peer **/
238     std::deque<CInv> m_getdata_requests GUARDED_BY(m_getdata_requests_mutex);
239 
240-    explicit Peer(NodeId id) : m_id(id) {}
241+    explicit Peer(NodeId id, bool addr_relay)
242+        : m_id(id), m_addr_known{addr_relay ? nullptr : MakeUnique<CRollingBloomFilter>(5000, 0.001)} {}

fanquake commented at 0:52 am on March 12, 2021:

Please use std::make_unique in new code.

jnewbery renamed this:
~~Net/Net Processing: Move addr data into net_processing~~
net/net processing: Move addr data into net_processing
on Mar 30, 2021

[net processing] Take NodeId instead of CNode* as originator for RelayAddress()

This makes the following commit easier.

86acc96469

[net processing] Make RelayAddress() a member function of PeerManagerImpl caba7ae8a5

jnewbery force-pushed on Apr 1, 2021

jnewbery commented at 8:10 am on April 1, 2021: member

#21236 is merged. Moving this out of draft.

jnewbery marked this as ready for review on Apr 1, 2021

in src/net_processing.cpp:653 in 468db14693 outdated

651@@ -617,6 +652,39 @@ static CNodeState *State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
652     return &it->second;
653 }
654 
655+static bool RelayAddrsWithPeer(const Peer& peer) { return peer.m_addr_known != nullptr; }

ajtowns commented at 7:58 pm on April 1, 2021:

0static bool RelayAddrsWithPeer(const Peer& peer)
1{
2    return peer.m_addr_known != nullptr;
3}

jnewbery commented at 10:29 am on April 30, 2021:

Done.

hebasto commented at 11:02 pm on April 2, 2021: member

Concept ACK.

in src/net_processing.cpp:245 in 468db14693 outdated

240@@ -218,7 +241,10 @@ struct Peer {
241     /** Work queue of items requested by this peer **/
242     std::deque<CInv> m_getdata_requests GUARDED_BY(m_getdata_requests_mutex);
243 
244-    explicit Peer(NodeId id) : m_id(id) {}
245+    explicit Peer(NodeId id, bool addr_relay)
246+        : m_id(id)
247+        , m_addr_known{addr_relay ? nullptr : std::make_unique<CRollingBloomFilter>(5000, 0.001)}
248+    {}

hebasto commented at 0:03 am on April 3, 2021:

Out of curiosity, is there a way to teach the clang-format-diff.py to support such formatting, i.e., both a leading comma and {} on the the same line?

jnewbery commented at 1:18 pm on April 6, 2021:

Good question! I’m not an expert on clang-format.

jnewbery commented at 1:19 pm on April 6, 2021:

Perhaps BCIS_BeforeComma? (https://clang.llvm.org/docs/ClangFormatStyleOptions.html)

in src/net_processing.cpp:1576 in 468db14693 outdated

1590 
1591     // Relay reachable addresses to 2 peers. Unreachable addresses are relayed randomly to 1 or 2 peers.
1592     unsigned int nRelayNodes = (fReachable || (hasher.Finalize() & 1)) ? 2 : 1;
1593 
1594-    std::array<std::pair<uint64_t, CNode*>,2> best{{{0, nullptr}, {0, nullptr}}};
1595+    std::array<std::pair<uint64_t, Peer*>,2> best{{{0, nullptr}, {0, nullptr}}};

hebasto commented at 1:22 am on April 3, 2021:

style nit:

0    std::array<std::pair<uint64_t, Peer*>, 2> best{{{0, nullptr}, {0, nullptr}}};

jnewbery commented at 1:14 pm on April 6, 2021:

Fixed!

in src/net_processing.cpp:2742 in 468db14693 outdated

2739                 continue;
2740             }
2741             bool fReachable = IsReachable(addr);
2742-            if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 && addr.IsRoutable())
2743+            if (addr.nTime > nSince && !peer->m_getaddr_sent && vAddr.size() <= 10 && addr.IsRoutable())
2744             {

hebasto commented at 1:24 am on April 3, 2021:

style nit:

0            if (addr.nTime > nSince && !peer->m_getaddr_sent && vAddr.size() <= 10 && addr.IsRoutable()) {

jnewbery commented at 1:16 pm on April 6, 2021:

Done

hebasto commented at 1:24 am on April 3, 2021: member

Approach ACK 468db1469382ee11380f0667454b86ec1903eca8.

In "[net processing] Move addr relay data and logic into net processing" commit (6fdf4b62e3161ce6fdd27b778de53933dccde65e) four functions are moved from CNode members to the net_processing.cpp as free static functions. I agree that there’s no need to make them members of Peer, but why not place them in a namespace? It could be unnamed, or we could choose a pretty good name for it :)

Also CNode::AddAddressKnown and CNode::PushAddress are subjects for fuzzing in the master branch. Could new free functions also be fuzzed?

jnewbery force-pushed on Apr 6, 2021

jnewbery commented at 1:17 pm on April 6, 2021: member

Thanks for the review, @hebasto! I’ve addressed both of your style comments.

four functions are moved from CNode members to the net_processing.cpp as free static functions. I agree that there’s no need to make them members of Peer, but why not place them in a namespace? It could be unnamed, or we could choose a pretty good name for it :)

There’s almost no difference between declaring a free function static and placing it in the unnamed namespace. Both will prevent the name from being accessible outside the translation unit. See https://stackoverflow.com/a/154482/933705 for example.

Also CNode::AddAddressKnown and CNode::PushAddress are subjects for fuzzing in the master branch. Could new free functions also be fuzzed?

Both of these functions are called from ProcessMessage(), so are indirectly tested by the fuzz/process_messages.py fuzzer.

hebasto approved

hebasto commented at 1:33 pm on April 6, 2021: member

ACK c538369123b6c69358649255815cf331b2c39f52, I have reviewed the code and it looks OK, I agree it can be merged.

There’s almost no difference between declaring a free function static and placing it in the unnamed namespace. Both will prevent the name from being accessible outside the translation unit. See https://stackoverflow.com/a/154482/933705 for example.

#19176 (review)

jnewbery commented at 1:46 pm on April 6, 2021: member

#19176 (comment)

I don’t see a hugely compelling reason to prefer unnamed namespaces over static declaration in any of those links, but equally wouldn’t be opposed to updating the style guide to prefer unnnamed namespaces just for consistency.

One advantage of using static that isn’t mentioned is that it’s instantly obvious to the reader that the symbol has internal linkage. Unnamed namespaces often span many hundreds of lines, so it’s not obvious when something is in the namespace. For example, I’ve just realized that in this PR, all the new functions are in fact inside an unnamed namespace that spans lines 542-942.

hebasto commented at 2:13 pm on April 10, 2021: member

For example, I’ve just realized that in this PR, all the new functions are in fact inside an unnamed namespace that spans lines 542-942.

There are three adjacent unnamed namespaces…

Unnamed namespaces often span many hundreds of lines…

I think it is a code organizing problem, no?

jnewbery commented at 2:38 pm on April 10, 2021: member

There are three adjacent unnamed namespaces…

Yes. This is part of the unfinished work in #20758. Once that’s finished, these unnamed namespaces should be fixed up.

Unnamed namespaces often span many hundreds of lines…

I think it is a code organizing problem, no?

I’m not sure. I think if we use unnamed namespaces for all the internal functions that we don’t want exposed outside the translation unit then they’ll usually be hundreds of lines.

hebasto commented at 2:42 pm on April 10, 2021: member

I’m not sure. I think if we use unnamed namespaces for all the internal functions that we don’t want exposed outside the translation unit then they’ll usually be hundreds of lines.

Correct. I mean that our code base have some really huge translation units :)

in src/net_processing.cpp:242 in c538369123 outdated

240@@ -218,7 +241,10 @@ struct Peer {
241     /** Work queue of items requested by this peer **/
242     std::deque<CInv> m_getdata_requests GUARDED_BY(m_getdata_requests_mutex);
243 
244-    explicit Peer(NodeId id) : m_id(id) {}
245+    explicit Peer(NodeId id, bool addr_relay)

mzumsande commented at 11:21 pm on April 11, 2021:

It’s counterintuitive to me that we don’t relay addresses when addr_relay is true - I would have expected the opposite, or a different name like disable_addr_relay.

jnewbery commented at 9:30 am on April 12, 2021:

You’re right. The logic here is correct, but the naming is confusing. I’ve reversed the logic, and added comments to the place where this ctor is called to hopefully make this more intuitive.

in src/net_processing.cpp:3625 in c538369123 outdated

3621@@ -3566,20 +3622,20 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3622         // to users' AddrMan and later request them by sending getaddr messages.
3623         // Making nodes which are behind NAT and can only make outgoing connections ignore
3624         // the getaddr message mitigates the attack.
3625-        if (!pfrom.IsInboundConn()) {
3626+        if (!RelayAddrsWithPeer(*peer)) {

mzumsande commented at 11:34 pm on April 11, 2021:

This seems incorrect to me: The old code was specifically ignoring GETADDR requests from all outbound connections (for reasons explained in the comment above this line), the new code only ignores from block-relay-only connections.

jnewbery commented at 8:40 am on April 12, 2021:

oh wow. That’s terrible. Thank you for catching this.

This was a bad rebase on top of 49c10a9ca40, which combined the two error conditions:

 0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index 690b59476b..859b67755d 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -3521,11 +3521,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
 5         // Making nodes which are behind NAT and can only make outgoing connections ignore
 6         // the getaddr message mitigates the attack.
 7         if (!pfrom.IsInboundConn()) {
 8-            LogPrint(BCLog::NET, "Ignoring \"getaddr\" from outbound connection. peer=%d\n", pfrom.GetId());
 9-            return;
10-        }
11-        if (!pfrom.RelayAddrsWithConn()) {
12-            LogPrint(BCLog::NET, "Ignoring \"getaddr\" from block-relay-only connection. peer=%d\n", pfrom.GetId());
13+            LogPrint(BCLog::NET, "Ignoring \"getaddr\" from %s connection. peer=%d\n", pfrom.ConnectionTypeAsString(), pfrom.GetId());
14             return;
15         }

mzumsande commented at 10:46 am on April 12, 2021:

Oh, I see. There should be a functional test for this that would have failed - something which I once wanted to do anyway but then forgot about. I’ll write one soon!

jnewbery commented at 11:04 am on April 12, 2021:

I’ll very happily review once you open the PR.

amitiuttarwar commented at 8:18 pm on April 12, 2021:

heads up that I’ve recently written some similar tests here: https://github.com/bitcoin/bitcoin/pull/21528/files?file-filters%5B%5D=.py#diff-5a9f30817894348260893f195cea07da88271b5e01b1531f0cd839ee85898592R150. I don’t think it would cover this exact use case, but just mentioning incase its useful / incase the test improvements could play nice together.

mzumsande commented at 9:02 pm on April 12, 2021:

Cool - I didn’t know, I had only looked at the non-test commits in your PR! This use case is the reverse direction (inbound/outbound send the getaddr to our node and get back addrs/nothing) from your tests, so I agree that these belong together.

jnewbery commented at 7:25 am on April 29, 2021:

More tests added in #21707

in src/net.h:682 in c538369123 outdated

652-    {
653-        // Known checking here is only to save space from duplicates.
654-        // SendMessages will filter it again for knowns that were added
655-        // after addresses were pushed.
656-        assert(m_addr_known);
657-        if (_addr.IsValid() && !m_addr_known->contains(_addr.GetKey()) && IsAddrCompatible(_addr)) {

mzumsande commented at 11:50 pm on April 11, 2021:

Why did you drop the check for IsAddrCompatible() in the move? Since PushAddress() is called from several places (not only RelayAddress() where this is checked seperately) this looks like more than a pure refactor to me.

jnewbery commented at 9:31 am on April 12, 2021:

This is another rebase error. Thank you for catching!

mzumsande commented at 0:11 am on April 12, 2021: member

Concept ACK to moving addr-related logic to net_processing. See my other comments for some possible issues.

jnewbery force-pushed on Apr 12, 2021

jnewbery commented at 9:40 am on April 12, 2021: member

Thank you for the careful review @mzumsande. You caught two rebase errors :flushed:

I’ve fixed those and addressed your other comment. I’ve also made some minor edits to comments to clarify concepts, and also re-reviewed everything to make sure no other errors have crept in.

ajtowns commented at 6:16 am on April 20, 2021: member

The “Move addr relay data and logic into net processing” commit is doing a lot of things, and I think that makes it a bit hard to review (and leads to rebase errors?). Might be worth putting a bit of effort into splitting it up further if some of the changes can be isolated?

in src/net_processing.cpp:4210 in 7741ca5959 outdated

4208     // Nothing to do for non-address-relay peers
4209-    if (!node.RelayAddrsWithConn()) return;
4210+    if (!RelayAddrsWithPeer(peer)) return;
4211 
4212-    assert(node.m_addr_known);
4213+    assert(peer.m_addr_known);

amitiuttarwar commented at 9:59 pm on April 29, 2021:

this assert seems unnecessary / not useful. the previous line returns early if m_addr_known is not present.

jnewbery commented at 10:28 am on April 30, 2021:

Good point. Removed!

in src/net_processing.cpp:153 in 7741ca5959 outdated

149@@ -150,6 +150,8 @@ static constexpr uint32_t MAX_GETCFILTERS_SIZE = 1000;
150 static constexpr uint32_t MAX_GETCFHEADERS_SIZE = 2000;
151 /** the maximum percentage of addresses from our addrman to return in response to a getaddr message. */
152 static constexpr size_t MAX_PCT_ADDR_TO_SEND = 23;
153+/** The maximum number of new addresses to relay to a peer in an ADDR message. */

amitiuttarwar commented at 10:08 pm on April 29, 2021:

isn’t this more like… “The maximum number of addresses permitted in an ADDR message”? because we also enforce on incoming ADDR messages. I’m not sure what the “new” is supposed to indicate in this comment.

jnewbery commented at 10:28 am on April 30, 2021:

Much better. Thanks.

in src/net_processing.cpp:229 in 7741ca5959 outdated

224+    /** Time point to send the next ADDR message to this peer. */
225+    std::chrono::microseconds m_next_addr_send GUARDED_BY(m_addr_send_times_mutex){0};
226+    /** Time point to possibly re-announce our local address to this peer. */
227+    std::chrono::microseconds m_next_local_addr_send GUARDED_BY(m_addr_send_times_mutex){0};
228+    /** Whether the peer has signaled support for receiving ADDRv2 (BIP155)
229+     *  messages, implying a preference to receive ADDRv2 instead of ADDR ones. */

amitiuttarwar commented at 10:22 pm on April 29, 2021:

I know you just moved this comment, but s/implying/indicating? sending a message is pretty explicit =P

jnewbery commented at 10:28 am on April 30, 2021:

Fixed!

in src/net_processing.cpp:674 in 7741ca5959 outdated

666+{
667+    assert(peer.m_addr_known);
668+    peer.m_addr_known->insert(addr.GetKey());
669+}
670+
671+static void PushAddress(Peer& peer, const CAddress& addr, FastRandomContext& insecure_rand)

amitiuttarwar commented at 11:26 pm on April 29, 2021:

I’m curious- why did you opt to make this a standalone function that takes in a peer vs a member on peer itself (like it previously was on CNode)?

jnewbery commented at 10:28 am on April 30, 2021:

Because Peer isn’t a class. It’s a data structure containing information about a Peer, which PeerManager acts on.

I know the distinction is blurry in C++, but I think it’s generally better to treat things as purely a data structure (which carries data that is acted on by external logic) or purely a class (where the data is private and internal logic exists behind well-defined public interface functions). Mixing the two, where some logic is contained within the object but external logic can also reach into the object and manipulate the data, is confusing.

amitiuttarwar commented at 8:35 pm on April 30, 2021:

ok gotcha, thanks!

amitiuttarwar commented at 11:27 pm on April 29, 2021: contributor

started reviewing, looks good so far, will be awesome to have addr handling in net processing :)

[net processing] Move addr relay data and logic into net processing 76568a3351

scripted-diff: rename address relay fields

-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }

ren fGetAddr     m_getaddr_sent
ren fSentAddr    m_getaddr_recvd
ren vAddrToSend  m_addrs_to_send
-END VERIFY SCRIPT-

09cc66c00e

[refactor] Remove unused ForEachNodeThen() template 0829516d1f

jnewbery force-pushed on Apr 30, 2021

jnewbery commented at 10:40 am on April 30, 2021: member

Thanks for the reviews @ajtowns and @amitiuttarwar. I’ve addressed all of your comments.

The “Move addr relay data and logic into net processing” commit is doing a lot of things, and I think that makes it a bit hard to review (and leads to rebase errors?). Might be worth putting a bit of effort into splitting it up further if some of the changes can be isolated?

I did try that at one point, but it seemed more confusing/difficult than just moving across the addr data/logic in one commit.

practicalswift commented at 3:53 pm on April 30, 2021: contributor

Concept ACK

DrahtBot commented at 9:33 am on May 3, 2021: member

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

jnewbery added this to the "Blockers" column in a project

laanwj commented at 11:57 am on May 19, 2021: member

Code review ACK 0829516d1f3868c1c2ba507feee718325d81e329

jonatack commented at 7:11 pm on May 20, 2021: member

Concept ACK

mzumsande commented at 9:07 pm on May 20, 2021: member

ACK 0829516d1f3868c1c2ba507feee718325d81e329, reviewed the code and ran tests.

in src/net_processing.cpp:1019 in 76568a3351 outdated

1013@@ -954,7 +1014,9 @@ void PeerManagerImpl::InitializeNode(CNode *pnode)
1014         assert(m_txrequest.Count(nodeid) == 0);
1015     }
1016     {
1017-        PeerRef peer = std::make_shared<Peer>(nodeid);
1018+        // Addr relay is disabled for outbound block-relay-only peers to
1019+        // prevent adversaries from inferring these links from addr traffic.
1020+        PeerRef peer = std::make_shared<Peer>(nodeid, /* addr_relay = */ !pnode->IsBlockOnlyConn());

sipa commented at 8:54 pm on May 21, 2021:

Using !IsBlockOnlyConn() as proxy for “should we relay addresses” goes a bit in the opposite direction as the earlier connection types refactors went in. Any reason not just keep the function name (I realize there’s a derived one with that name added, but it’s ultimately still relying on this test.

I don’t have a particularly strong opinion here; I’m just noticing some flipflopping.

jnewbery commented at 11:11 am on May 24, 2021:

This logic of checking IsBlockOnlyConn() is used only when calling the Peer ctor. After that, we use the RelayAddrsWithPeer(), which replaces the old RelayAddrsWithConn() function. That’s a clearer separation between net and net_processing since we only check the connection type when initializing the Peer object.

sipa commented at 9:07 pm on May 21, 2021: member

utACK 0829516d1f3868c1c2ba507feee718325d81e329

hebasto approved

hebasto commented at 11:42 am on May 24, 2021: member

re-ACK 0829516d1f3868c1c2ba507feee718325d81e329

fanquake merged this on May 24, 2021

fanquake closed this on May 24, 2021

jnewbery deleted the branch on May 24, 2021

sidhujag referenced this in commit 8945437719 on May 25, 2021

laanwj removed this from the "Blockers" column in a project

deadalnix referenced this in commit 940da13ad0 on Jan 28, 2022

deadalnix referenced this in commit c27e533b76 on Jan 28, 2022

deadalnix referenced this in commit b694185bf6 on Jan 28, 2022

deadalnix referenced this in commit fa582ef49a on Jan 28, 2022

deadalnix referenced this in commit 3a389ad76c on Jan 28, 2022

gwillen referenced this in commit 092f386da0 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

net/net processing: Move addr data into net_processing #21186

Conflicts