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.
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.
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:
PushAddress()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).
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.
Interesting. Will review.
Concept ACK
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.
@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.
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. */
I think the Doxygen style in developer-notes.md might be easier to read.
Line 1485: s/do/does/
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.
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)
<details><summary>a few suggestions for the test code</summary><p>
@@ -18,8 +18,6 @@ from test_framework.mininode import (
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
- assert_greater_than,
- assert_greater_than_or_equal,
)
import time
@@ -42,8 +40,8 @@ class AddrReceiver(P2PInterface):
def on_addr(self, message):
for addr in message.addrs:
assert_equal(addr.nServices, 9)
- assert_greater_than(8343, addr.port)
- assert_greater_than_or_equal(addr.port, 8333)
+ if not 8333 <= addr.port < 8343:
+ raise AssertionError("Invalid addr.port of {} (8333-8342 expected)".format(addr.port))
assert addr.ip.startswith('123.123.123.')
self.num_ipv4_received += 1
@@ -59,14 +57,14 @@ class AddrTest(BitcoinTestFramework):
msg = msg_addr()
self.log.info('Send too-large addr message')
- msg.addrs = ADDRS * 101 # more than 1000 addresses in one message
+ msg.addrs = ADDRS * 101 # more than 1000 addresses in one message
with self.nodes[0].assert_debug_log(['addr message size = 1010']):
addr_source.send_and_ping(msg)
self.log.info('Check that addr message content is relayed and added to addrman')
num_receivers = 7
receivers = []
- for i in range(num_receivers):
+ for _ in range(num_receivers):
receivers.append(self.nodes[0].add_p2p_connection(AddrReceiver()))
msg.addrs = ADDRS
- with self.nodes[0].assert_debug_log([
- 'Added {} addresses from 127.0.0.1: 0 tried'.format(num_ipv4_addrs),
- 'received: addr (301 bytes) peer=0',
- ]):
+ with self.nodes[0].assert_debug_log(
+ [
+ "Added {} addresses from 127.0.0.1: 0 tried".format(num_ipv4_addrs),
+ "received: addr (301 bytes) peer=0",
+ ]
+ ):
addr_source.send_and_ping(msg)
</p></details>
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)
Verified this assertion fails on master with relay to fewer nodes:
AssertionError: not(17 == 20)
AssertionError: not(17 == 20)
AssertionError: not(19 == 20)
ACK e80a80b25
A few comments below, feel free to pick and choose.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
Concept ACK.
Concept ACK, code change in net_processing LGTM.
Code review ACK e80a80b25b14b580644b2c29e471412dd932d7c4 Needs rebase.
Rebased to resolve conflicts.
Picked some suggestions.
Code review ACK fd897f89eea2486ccf12ac7b40a99dbcc6084b72
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
s/someone to unjustly give/a peer from unjustly giving/
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.
Code review ACK fd897f89eea2486ccf12ac7b40a99dbcc6084b72.
nit, could drop "try".
Rebased to resolve conflicts, picked up suggestion and dropped "try".
Code review re-ACK 0b8dd9555c57e64212656a3e801502ce9878bd15 per git range-diff a0a422c fd897f8 0b8dd95
Thanks for updating.
reACK 0b8dd9555c57e64212656a3e801502ce9878bd15
Concept ACK.
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?
@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:
Both of these effects can also be easily achieved by an attacker today by:
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).
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).
@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:
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.
as it was chosen before by us
Ah, I see now! Thanks for the elaboration.
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:
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?
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:
addr (according to n0)addr to n0addraddr (according to n0)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
@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).
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.
Maybe once someone implements the "best way"
When Erlay for addr messages?
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.
Concept ACK. This seems to be an incremental improvement. I can't think of any downside to doing this.
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.
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.
Changes since the ACKs - replace inline doxygen comments with @param ones:
ACK 7fabe0f359ae16ed36ce4ca2c33631d038c21448 (this time I looked at the test, and verified the test breaks in expected ways if I break the code).
Re-reviewing as soon as I finish 19858.
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?
utACK 7fabe0f359ae16ed36ce4ca2c33631d038c21448 (only net_processing changes. I haven't reviewed the test changes)
(ci can be ignored. Looks like a ci integration problem)
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
src/net_processing.cpp
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] originator The peer that sent us the address. We don't want to relay it back.
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] addr Address to relay.
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] fReachable Whether the address' network is reachable. We relay unreachable
- * addresses less.
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] connman Connection manager to choose nodes to relay to.
+ *
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] originator The peer that sent us the address. We don't want to relay it back.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] addr Address to relay.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] connman Connection manager to choose nodes to relay to.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] fReachable Whether the address' network is reachable. We relay unreachable addresses less.
*/
-static void RelayAddress(const CNode& originator,
- const CAddress& addr,
- bool fReachable,
- const CConnman& connman)
+static void RelayAddress(const CNode& originator, const CAddress& addr, const CConnman& connman, bool fReachable)
{
unsigned int nRelayNodes = fReachable ? 2 : 1; // limited relaying of addresses outside our network(s)
@@ -2653,7 +2650,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty
if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 && addr.IsRoutable())
{
// Relay to a limited number of other nodes
- RelayAddress(pfrom, addr, fReachable, m_connman);
+ RelayAddress(pfrom, addr, m_connman, fReachable);
}
(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).