net: don’t try to relay to the address’ originator #19763

pull vasild wants to merge 1 commits into bitcoin:master from vasild:only_relay_to_unaware changing 2 files +48 −13
  1. vasild commented at 7:48 pm on August 18, 2020: member

    For each address to be relayed we “randomly” pick 2 nodes to send the address to (in RelayAddress()). However we do not take into consideration that it does not make sense to relay the address back to its originator (CNode::PushAddress() will do nothing in that case).

    This means that if the originator is among the “randomly” picked nodes, then we will relay to one node less than intended.

    Fix this by skipping the originating node when choosing candidates to relay to.

  2. vasild commented at 7:50 pm on August 18, 2020: member
    I stumbled into this while testing #19728.
  3. sipa commented at 7:53 pm on August 18, 2020: member

    This behavior is intentional, and the hash-based logic to determine where to send exploits it (there is a comment about m_addr_known in RelayAddress).

    The idea is that every incoming addr is only relayed to a fixed subset of 1 or 2 peers during windows of 24 hours. This means that if the same address is relayed again, it will go to the same peers (and likely not be sent at all as they already know it). This change would permit someone to unjustly give their address better propagation by sending it repeatedly.

  4. vasild marked this as a draft on Aug 18, 2020
  5. vasild commented at 8:33 pm on August 18, 2020: member

    Alright, I see the problem this would cause.

    The condition added in this PR !pnode->m_addr_known->contains(addr) is too strong. I only intended to avoid relaying to the node that sent us the address, trying to avoid this adverse scenario:

    • let node0 have connections to 8 other nodes: node1, …, node8.
    • node5 sends some address to node0
    • node0 is about to relay it to 2 “random” nodes and picks node3 and node5
    • node5 already knows the address and so is skipped by PushAddress()
    • the address is relayed to just node3 - one node instead of two :bomb:.

    Looks like the chance of this happening is 1/8, or 12.5% 1/4, or 25%.

    This means that if the same address is relayed again, it will go to the same peers (and likely not be sent at all as they already know it)

    Is this “likely” actually “for sure”? Is this correct: “This means that the same address will not be re-relayed again in 24h because we would attempt to send it to the same peers and this will be a noop by PushAddress()”? (assuming the list of peers does not change).

  6. sipa commented at 8:37 pm on August 18, 2020: member
    @vasild Ah, I see. That’s a good point, and should probably be improved. I think we can just make sure that the originating node is skipped in the list of considered candidates?
  7. DrahtBot added the label P2P on Aug 18, 2020
  8. vasild force-pushed on Aug 19, 2020
  9. vasild renamed this:
    net: don't try to relay to node that knows an addr
    net: don't try to relay to the address' originator
    on Aug 19, 2020
  10. vasild marked this as ready for review on Aug 19, 2020
  11. vasild commented at 10:44 am on August 19, 2020: member

    make sure that the originating node is skipped in the list of considered candidates

    Exactly! Done in e80a80b and extended a test so that it fails without this fix.

    Reopened for review.

  12. jonatack commented at 4:08 pm on August 19, 2020: member
    Interesting. Will review.
  13. sipa commented at 7:11 pm on August 19, 2020: member
    Concept ACK
  14. dergoegge commented at 7:49 pm on August 19, 2020: member
    What if the same address is relayed again but from a different peer? I think the list of nodes the address would be relayed to might not be the same, because now it depends on the address AND the origin and not just the address, so the list would only be deterministic for the same origin.
  15. vasild commented at 7:47 am on August 20, 2020: member

    @dergoegge, the hashKey of each relay-to candidate does not depend on the originator. Can you elaborate? I think the list of nodes the address would be relayed to does not depend on the originator, except that the originator will be skipped if he happens to be one of the two chosen nodes and in his place another node will be selected.

    Notice that CSipHasher(hasher).Write(pnode->GetId()).Finalize() creates a temporary which is destroyed before the next candidate is evaluated.

  16. dergoegge commented at 9:30 am on August 20, 2020: member
    @vasild while trying to come up with an example i realised that you are right and that i didn’t fully understand how sortfunc works (even though it has “sort” in the name 😅), sorry.
  17. in src/net_processing.cpp:1497 in e80a80b25b outdated
    1493+    const CNode* originator,
    1494+    /** [in] Address to relay. */
    1495+    const CAddress& addr,
    1496+    /** [in] Whether the address' network is reachable. We relay unreachable addresses less. */
    1497+    bool fReachable,
    1498+    /** [in] Connection manager to choose nodes to relay to. */
    


    jonatack commented at 4:23 pm on August 20, 2020:

    I think the Doxygen style in developer-notes.md might be easier to read.

    Line 1485: s/do/does/


    jnewbery commented at 12:47 pm on December 10, 2020:
    I agree. This is inconsistent with the style used in the rest of the project. If you want to update the project style for function comments, please open a PR to do that or raise in an irc meeting.

    vasild commented at 1:59 pm on December 10, 2020:

    The guidelines already say

    Use Doxygen-compatible comment blocks for functions

    and the above is (was) doxygen-compatible, so I don’t think there is anything to update in developer-notes.md.

    The used inline comments have numerous advantages over @param which I will summarize and try to get some feedback to eventually get some explicit “allow that in new code”.

    Anyway, no need to block this PR with that, so I have changed it to use @param.

  18. in test/functional/p2p_addr_relay.py:46 in e80a80b25b outdated
    41+
    42     def on_addr(self, message):
    43         for addr in message.addrs:
    44             assert_equal(addr.nServices, 9)
    45+            assert_greater_than(8343, addr.port)
    46+            assert_greater_than_or_equal(addr.port, 8333)
    


    jonatack commented at 4:25 pm on August 20, 2020:
     0@@ -18,8 +18,6 @@ from test_framework.mininode import (
     1 from test_framework.test_framework import BitcoinTestFramework
     2 from test_framework.util import (
     3     assert_equal,
     4-    assert_greater_than,
     5-    assert_greater_than_or_equal,
     6 )
     7 import time
     8 
     9@@ -42,8 +40,8 @@ class AddrReceiver(P2PInterface):
    10     def on_addr(self, message):
    11         for addr in message.addrs:
    12             assert_equal(addr.nServices, 9)
    13-            assert_greater_than(8343, addr.port)
    14-            assert_greater_than_or_equal(addr.port, 8333)
    15+            if not 8333 <= addr.port < 8343:
    16+                raise AssertionError("Invalid addr.port of {} (8333-8342 expected)".format(addr.port))
    17             assert addr.ip.startswith('123.123.123.')
    18             self.num_ipv4_received += 1
    19 
    20@@ -59,14 +57,14 @@ class AddrTest(BitcoinTestFramework):
    21         msg = msg_addr()
    22 
    23         self.log.info('Send too-large addr message')
    24-        msg.addrs = ADDRS * 101 # more than 1000 addresses in one message
    25+        msg.addrs = ADDRS * 101  # more than 1000 addresses in one message
    26         with self.nodes[0].assert_debug_log(['addr message size = 1010']):
    27             addr_source.send_and_ping(msg)
    28 
    29         self.log.info('Check that addr message content is relayed and added to addrman')
    30         num_receivers = 7
    31         receivers = []
    32-        for i in range(num_receivers):
    33+        for _ in range(num_receivers):
    34             receivers.append(self.nodes[0].add_p2p_connection(AddrReceiver()))
    35         msg.addrs = ADDRS
    36-        with self.nodes[0].assert_debug_log([
    37-                'Added {} addresses from 127.0.0.1: 0 tried'.format(num_ipv4_addrs),
    38-                'received: addr (301 bytes) peer=0',
    39-        ]):
    40+        with self.nodes[0].assert_debug_log(
    41+            [
    42+                "Added {} addresses from 127.0.0.1: 0 tried".format(num_ipv4_addrs),
    43+                "received: addr (301 bytes) peer=0",
    44+            ]
    45+        ):
    46             addr_source.send_and_ping(msg)
    
  19. in test/functional/p2p_addr_relay.py:84 in e80a80b25b outdated
    86+        total_ipv4_received = sum(r.num_ipv4_received for r in receivers)
    87+
    88+        # Every IPv4 address must be relayed to two peers, other than the
    89+        # originating node (addr_source).
    90+        ipv4_branching_factor = 2
    91+        assert_equal(total_ipv4_received, num_ipv4_addrs * ipv4_branching_factor)
    


    jonatack commented at 4:31 pm on August 20, 2020:

    Verified this assertion fails on master with relay to fewer nodes:

    0AssertionError: not(17 == 20)
    1AssertionError: not(17 == 20)
    2AssertionError: not(19 == 20)
    
  20. jonatack commented at 4:33 pm on August 20, 2020: member

    ACK e80a80b25

    A few comments below, feel free to pick and choose.

  21. DrahtBot commented at 7:53 pm on August 20, 2020: 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:

    • #18642 (Use std::chrono for the time to rotate destination of addr messages + tests by naumenkogs)

    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.

  22. naumenkogs commented at 2:12 pm on August 23, 2020: member
    Concept ACK.
  23. promag commented at 10:56 pm on August 23, 2020: member
    Concept ACK, code change in net_processing LGTM.
  24. DrahtBot added the label Needs rebase on Aug 24, 2020
  25. naumenkogs commented at 11:39 am on August 27, 2020: member
    Code review ACK e80a80b25b14b580644b2c29e471412dd932d7c4 Needs rebase.
  26. vasild force-pushed on Sep 2, 2020
  27. vasild commented at 8:46 am on September 2, 2020: member
    Rebased to resolve conflicts.
  28. vasild force-pushed on Sep 2, 2020
  29. vasild commented at 8:48 am on September 2, 2020: member
    Picked some suggestions.
  30. naumenkogs commented at 9:30 am on September 2, 2020: member
    Code review ACK fd897f89eea2486ccf12ac7b40a99dbcc6084b72
  31. in src/net_processing.cpp:1487 in fd897f89ee outdated
    1527+/**
    1528+ * Relay (gossip) an address to a few randomly chosen nodes.
    1529+ * We choose the same nodes within a given 24h window (if the list of connected
    1530+ * nodes does not change) and we don't relay to nodes that already know an
    1531+ * address. So within 24h we will likely relay a given address once. This is to
    1532+ * prevent someone to unjustly give their address better propagation by sending
    


    jonatack commented at 9:51 am on September 2, 2020:
    s/someone to unjustly give/a peer from unjustly giving/
  32. jonatack commented at 9:54 am on September 2, 2020: member

    Code review re-ACK fd897f8 diff review per git range-diff 3a3e21da e80a80b fd897f8, re-code review, re-verified the test fails on master.

    One doc fixup if you have to retouch.

  33. promag commented at 9:55 am on September 2, 2020: member

    Code review ACK fd897f89eea2486ccf12ac7b40a99dbcc6084b72.

    nit, could drop “try”.

  34. DrahtBot removed the label Needs rebase on Sep 2, 2020
  35. DrahtBot added the label Needs rebase on Sep 3, 2020
  36. vasild force-pushed on Sep 3, 2020
  37. vasild force-pushed on Sep 3, 2020
  38. vasild commented at 4:25 pm on September 3, 2020: member
    Rebased to resolve conflicts, picked up suggestion and dropped “try”.
  39. jonatack commented at 4:34 pm on September 3, 2020: member

    Code review re-ACK 0b8dd9555c57e64212656a3e801502ce9878bd15 per git range-diff a0a422c fd897f8 0b8dd95

    Thanks for updating.

  40. DrahtBot removed the label Needs rebase on Sep 3, 2020
  41. naumenkogs commented at 6:20 am on September 4, 2020: member
    reACK 0b8dd9555c57e64212656a3e801502ce9878bd15
  42. hebasto commented at 8:53 am on September 25, 2020: member
    Concept ACK.
  43. sipa commented at 2:33 am on September 26, 2020: member

    Thinking more about this, I have a slight concern here that this enables a sort of “augmentation” - if we relay to a peer, and they relay the same address immediately back to us, it will (with this PR) always result in the address being sent out to one extra peer.

    Does anyone see any issues with skipping addr relay in case addr is already in pnode->m_addr_known?

  44. naumenkogs commented at 8:11 am on September 26, 2020: member

    @sipa good catch! It’s very unlikely to happen in an honest scenario, but a malicious one is worth discussing. I see two potential issues with relaying to one more peer:

    1. overcoming our spam protection (rate limiting)
    2. easier link inference (if a malicious node was assigned to get a probe address from a victim being link-probed, it could force a victim to send a probe address to another connection, potentially non-malicious).

    Both of these effects can also be easily achieved by an attacker today by:

    • relaying to more random nodes in the network to get (1)
    • using more probe addresses (they don’t even have to be real nodes) to get (2)

    At the same time, the threat doesn’t seem to be that big.

    While the solution you’re suggesting doesn’t feel very safe (thus, not adequate to address this low threat). We don’t have any guarantees on m_addr_known at all, so, maybe a peer relayed some addr to us long time ago, and it would really benefit from getting a reannouncement of that addr, but it still sits in the m_addr_known so no luck. I’m already unhappy with how m_addr_known unpredictably affects AddrMan quality, and this would make it even deeper.

    Perhaps in future the issue I’m pointing out above might be addressed by making m_addr_known time-aware w.r.t when an addr record was relayed (or maybe just addr’s nTime). It would make the effect of m_addr_known on AddrMans much more predictable.

    I’ll see if an implementation makes sense (soon). Once we have better control of m_addr_known, I’d be more convinced in your suggestion :)


    A note for other readers: I was about to say we already do what Pieter suggests, but then realised the suggestion is to ignore it if it comes from a node already knowing it (not going to a knowing node).

  45. vasild commented at 3:16 pm on October 6, 2020: member

    if we relay to a peer, and they relay the same address immediately back to us,

    When we receive an address from a peer our behavior is the same no matter whether we have sent the same address to that peer before or not. In master and in this PR - the peer is marked as knowing the address when we receive it from him. If we have sent the address before to the same peer then it would have been marked as knowing the address already, but this is irrelevant - in either case m_addr_known will contain the address just after receiving it.

    it will (with this PR) always result in the address being sent out to one extra peer

    We pick 2 random peers and try to relay to them. This PR will only change the behavior in the rare case when one of the 2 randomly selected is the originator, so I don’t think “always” is the correct word. In this rare case we would relay to 2 peers as intended, not 1, so I don’t think “one extra peer” is correct either.

    Maybe I misunderstood but the above should read: “it will (with this PR) in the rare cases when the originator is chosen, result in the address being sent out to the intended 2 peers, not one peer less (to just 1).

  46. sipa commented at 5:01 pm on October 6, 2020: member

    @naumenkogs Those are good arguments. Consider my concern addressed. @vasild My thinking is that if an address is echoed back to us, then by definition, one of the two (currently) selected peers would be the originator (as it was chosen before by us). With this PR, the behavior would be:

    • Peer A announces an address to us
    • We select peers B and C to relay the address to, and send it.
    • B echoes the address back to us
    • We now select peers C and D to relay the address to, as B is the originator this time, causing an extra relay to D (as B and C have it in m_addr_known already).

    What conceptually changed here is that the set of potential destinations - across all originators - is now 3: B, C, and D. And it’s not random: B and C can deterministically force us to relay to a 3rd peer.

    I agree this isn’t a concern, but I wanted to highlight the potential here.

  47. vasild commented at 7:27 pm on October 6, 2020: member

    as it was chosen before by us

    Ah, I see now! Thanks for the elaboration.

  48. vasild commented at 9:08 am on October 13, 2020: member

    I gave some more thought on this.

    We pick 2 random nodes to relay to regardless of whether those nodes know the address or not. If we happen to pick a node that knows the address we will not relay to it and so we will relay to less than the intended 2 nodes.

    The originator of the address knows the address and that is just a special case of the above.

    So, assuming address propagation works, maybe this is not an issue. If it is to be addressed then maybe something like the following should be devised:

    • if this is the first time we are relaying a given address within the current 24h window, then pick 2 random destinations from the ones that don’t know it
    • if we have already relayed the given address within the current 24h window, then don’t relay it
  49. vasild closed this on Oct 13, 2020

  50. naumenkogs commented at 9:19 am on October 19, 2020: member

    Not sure why this was closed. I think the issue is worth addressing, or at least kept in mind…

    So, assuming address propagation works, maybe this is not an issue.

    I think it is an issue because it is unexpected behavior. We might think that we always relay to 2 (at least best-effort), but this issue makes it not true, and it’s trivial to fix.

    I’m wondering what you didn’t like about your solution?

  51. vasild commented at 9:57 am on October 19, 2020: member

    I closed it because it is fixing just a special case of a more widespread issue. Otherwise I don’t think there is a technical issue with the patch. It will be an improvement, thus reopening.

    Let me illustrate with an example:

    • let n0 have connections to 8 other nodes: n1, …, n8
    • n1, n2 and n3 already know some address addr (according to n0)
    • n5 sends addr to n0
    • n5 is immediately marked as knowing addr
    • now nodes n1, n2, n3 and n5 know addr (according to n0)
    • n0 is about to relay it to 2 “random” nodes and picks:
      • in master it would pick among all of n1, …, n8 and if it happens to choose a node that knows the address, then it will skip relaying to it
      • in this PR it will pick among n1, n2, n3, n4, n6, n7, n8 - an improvement but it could still end up skipping relay if it chooses one of the first 3
      • best way: pick among the nodes that don’t know addr: n4, n6, n7, n8. But notice this fix is not trivial - if we implement it naively and relay to e.g. n4 and n6 we will mark them as knowing the address and if after 1 hour we are about to relay the same address again, then we would choose from just n7 and n8. We want to keep our choice constant within 24h.
  52. vasild reopened this on Oct 19, 2020

  53. naumenkogs commented at 9:40 am on October 22, 2020: member
    @vasild I agree with the description. Maybe once someone implements the “best way” this can be closed, but for now I think it’s better keep this as a reference (and maybe merge this as an improvement with 0 overhead).
  54. sdaftuar commented at 6:57 pm on December 3, 2020: member

    utACK (other than the test code, which I did not review). I think this is an improvement even if addr relay can be further improved.

    I should add – I think on balance we should be more concerned about ensuring that honest actors have their addresses relay well, more than we should be concerned about adversaries getting additional relay (I think @naumenkogs made a similar point above). My intuition right now is that it’s easy for someone circumventing our addr relay system to get their own addresses to propagate very well, while honest peers spinning up take a long time to get their addresses gossiped on the network. This needs more research though.

  55. sipa commented at 7:04 pm on December 3, 2020: member

    Maybe once someone implements the “best way”

    When Erlay for addr messages?

  56. jonatack commented at 8:15 pm on December 8, 2020: member
    To summarize, if I’m non overlooking anything, this PR has ACKs by @naumenkogs, @sdaftuar, and myself, a fourth ACK (pre-rebase) by @promag, and a concept ACK by @hebasto.
  57. jnewbery commented at 1:07 pm on December 10, 2020: member
    Concept ACK. This seems to be an incremental improvement. I can’t think of any downside to doing this.
  58. net: don't relay to the address' originator
    For each address to be relayed we "randomly" pick 2 nodes to send the
    address to (in `RelayAddress()`). However we do not take into
    consideration that it does not make sense to relay the address back to
    its originator (`CNode::PushAddress()` will do nothing in that case).
    
    This means that if the originator is among the "randomly" picked nodes,
    then we will relay to one node less than intended.
    
    Fix this by skipping the originating node when choosing candidates to
    relay to.
    7fabe0f359
  59. vasild force-pushed on Dec 10, 2020
  60. vasild commented at 2:04 pm on December 10, 2020: member
    Looks like github just lost my comment. @jnewbery did you get it in email?
  61. vasild force-pushed on Dec 10, 2020
  62. vasild commented at 2:27 pm on December 10, 2020: member

    resurrected the comment by rewinding the PR to an old commit (e80a80b)

    I agree. This is inconsistent with the style used in the rest of the project. If you want to update the project style for function comments, please open a PR to do that or raise in an irc meeting.

    The guidelines already say

    Use Doxygen-compatible comment blocks for functions

    and the above is (was) doxygen-compatible, so I don’t think there is anything to update in developer-notes.md.

    The used inline comments have numerous advantages over @param which I will summarize and try to get some feedback to eventually get some explicit “allow that in new code”.

    Anyway, no need to block this PR with that, so I have changed it to use @param.

  63. vasild force-pushed on Dec 10, 2020
  64. vasild commented at 2:29 pm on December 10, 2020: member
  65. sdaftuar commented at 2:41 pm on December 10, 2020: member
    ACK 7fabe0f359ae16ed36ce4ca2c33631d038c21448 (this time I looked at the test, and verified the test breaks in expected ways if I break the code).
  66. jonatack commented at 2:50 pm on December 10, 2020: member
    Re-reviewing as soon as I finish 19858.
  67. sdaftuar commented at 3:10 pm on December 10, 2020: member
    Not sure what is going on with the failed appveyor or cirrus ci builds; can someone with a better understanding of how those work take a look?
  68. jnewbery commented at 3:17 pm on December 10, 2020: member
    utACK 7fabe0f359ae16ed36ce4ca2c33631d038c21448 (only net_processing changes. I haven’t reviewed the test changes)
  69. MarcoFalke commented at 3:43 pm on December 10, 2020: member
    (ci can be ignored. Looks like a ci integration problem)
  70. jonatack commented at 4:24 pm on December 10, 2020: member

    re-ACK 7fabe0f359ae16ed36ce4ca2c33631d038c21448 per git range-diff b76abae fd897f8 7fabe0f, change since last review is rebase and more readable Doxygen documentation

    If you retouch, it might be more readable as

     0src/net_processing.cpp
     1- * [@param](/bitcoin-bitcoin/contributor/param/)[in] originator The peer that sent us the address. We don't want to relay it back.
     2- * [@param](/bitcoin-bitcoin/contributor/param/)[in] addr Address to relay.
     3- * [@param](/bitcoin-bitcoin/contributor/param/)[in] fReachable Whether the address' network is reachable. We relay unreachable
     4- * addresses less.
     5- * [@param](/bitcoin-bitcoin/contributor/param/)[in] connman Connection manager to choose nodes to relay to.
     6+ *
     7+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] originator  The peer that sent us the address. We don't want to relay it back.
     8+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] addr        Address to relay.
     9+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] connman     Connection manager to choose nodes to relay to.
    10+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] fReachable  Whether the address' network is reachable. We relay unreachable addresses less.
    11  */
    12-static void RelayAddress(const CNode& originator,
    13-                         const CAddress& addr,
    14-                         bool fReachable,
    15-                         const CConnman& connman)
    16+static void RelayAddress(const CNode& originator, const CAddress& addr, const CConnman& connman, bool fReachable)
    17 {
    18     unsigned int nRelayNodes = fReachable ? 2 : 1; // limited relaying of addresses outside our network(s)
    19 
    20@@ -2653,7 +2650,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty
    21             if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 && addr.IsRoutable())
    22             {
    23                 // Relay to a limited number of other nodes
    24-                RelayAddress(pfrom, addr, fReachable, m_connman);
    25+                RelayAddress(pfrom, addr, m_connman, fReachable);
    26             }
    
  71. vasild commented at 9:59 am on December 14, 2020: member

    (ci can be ignored. Looks like a ci integration problem)

    Ok, I am leaving it as is then. I can rebase or push a minor whitespace change to trigger a fresh CI run (invalidade ACKs).

  72. MarcoFalke merged this on Dec 14, 2020
  73. MarcoFalke closed this on Dec 14, 2020

  74. vasild deleted the branch on Dec 14, 2020
  75. sidhujag referenced this in commit 76d428e6c5 on Dec 14, 2020
  76. Fabcien referenced this in commit e10e10c2b3 on Jan 21, 2022
  77. DrahtBot locked this on Feb 15, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me