-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce pings and dns lookups #993
Conversation
That sorting can cause Pings to be sent. Now sorting is only done when receiving a new noderef and at random intervals. This should make the node less visible in DNS requests and Pings (more typical pattern instead of requesting all peers at the same time). Also it fixes the startup time regression by not doing a ping in the PeerNode constructor. The sorting result in the constructor was overwritten anyway …
Next thing I like to have is custom DNS server & DNS over HTTPS via dnsjava, to secure the DNS requests. |
@@ -788,9 +779,17 @@ private boolean parseARK(SimpleFieldSet fs, boolean onStartup, boolean forDiffNo | |||
*/ | |||
@Override | |||
public synchronized Peer getPeer() { | |||
if (detectedPeer == null && !nominalPeer.isEmpty()) { | |||
sortNominalPeer(); | |||
detectedPeer = nominalPeer.get(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shortToString
includes the detectedPeer
and should updated accordingly.
This should either call updateShortToString
after setting detectedPeer
, or a to be introduced setter that does that. Alternatively, we should get rid of the the cached shortToString and just compute it in shortToString()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 1cf9ed5
src/freenet/node/DNSRequester.java
Outdated
@@ -56,23 +56,24 @@ public void run() { | |||
private void realRun() { | |||
PeerNode[] nodes = node.getPeers().myPeers(); | |||
long now = System.currentTimeMillis(); | |||
if((now - lastLogTime) > 1000) { | |||
if(logMINOR) | |||
if((now - lastLogTime) > 100) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this (and the entire currentTimeMillis dance) into the if (logMINOR) { ... }
? We don't need the result if minor logging is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thanks!
src/freenet/node/DNSRequester.java
Outdated
pn.maybeUpdateHandshakeIPs(false); | ||
} | ||
// check a randomly chosen node to avoid sending bursts of DNS requests | ||
PeerNode pn = nodes[node.getFastWeakRandom().nextInt(nodes.length)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach turns the peer node DNS resolving into the Coupon collector's problem. For 80 peer nodes, the expected number of tries before all are hit is 397 (taking roughly 8 minutes), and it takes 563 attempts (~10 minutes) before 95% probability is reached - even assuming the peer node list is static, which it is not. This may not be desirable.
This can be resolved by ignoring connected nodes prior to random selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s a great catch — thank you!
Does only checking non-connected nodes really resolve it? I’d think it would only reduce the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bertm the new commit should fix this: it uses a set and a deque to ensure that we only check a node again if at least 80% of the other unconnected nodes have been checked (as cheapskate’s ordering trick ☺).
It uses the location to check for equality, because these must not be equal, and because a set and a deque of doubles is cheap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn’t DNS over HTTPS create a stronger way to track nodes? DNS requests via UDP are heavily cached, so watchers can never trust to see DNS requests. Also our connections are secured via signatures, so a false IP address cannot cause a MitM attack. Custom DNS servers would be neat, though. A great synergy could be DNS over GNS (gnunet name system). |
This PR is a blocker for 1499 — the only hard blocker, because it resolves a regression in 1498 — so it would be great if we could get it done soon. |
Watchers are not just at the core router. They can be just at the WiFi AP you use (with unknown firmware controlled by ISP with possible preinstalled spyware). For the mass surveillance by ISP, it doesn't matter whether DNS is cached, or which DNS server is used, but it does matter whether DNS is encrypted. Anyone requesting the domain name of a Freenet node is likely using Freenet. DNS over HTTPS can also use a HTTP proxy server. The only thing I'm concerned about is that |
Anyone sending UDP packets to a Freenet node is likely using Freenet too. Opennet nodes can be enumerated with little effort, that cannot be changed due to the open nature of the network. If you're worried about DNS being monitored specifically, better solve it at the OS level. If you're worried about others knowing you participate in Freenet, use Darknet. |
Darknet is why this PR reduces DNS requests: if a node is not known to be a Freenet node, sending large numbers of DNS requests in short, regular intervals could be used as pattern to find it. I do not think that this is a problem at the moment, because I think few people use dyndns hostnames in their node config, but if many people used them — for example to get connectivity for the first connection (where they cannot get the new IP via an ark request), then that would become a problem. This also points to the regular "1 second wait" as a problem. I’ll adjust that to "1 - 60s" now that we’re selecting the nodes to check efficiently. ⇒ 3bdf89a (randomized wait time) and 61e74fa (move random out of synchronized). |
// check a randomly chosen node that has not been checked | ||
// recently to avoid sending bursts of DNS requests | ||
int unconnectedNodesLength = nodesToCheck.length; | ||
PeerNode pn = nodesToCheck[node.getFastWeakRandom().nextInt(unconnectedNodesLength)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will throw ArrayIndexOutOfBoundsException
if unconnectedNodesLength
is zero. Can be solved by an early return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
early return sounds like the best fix — implemented. Thanks!
src/freenet/node/DNSRequester.java
Outdated
//Logger.minor(this, "Node: "+pn); | ||
|
||
// Try new DNS lookup | ||
//Logger.minor(this, "Doing lookup on "+pn+" of "+nodesToCheck.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get rid of the commented out debug code? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted ☺
// until at least 80% of the other unconnected nodes have been checked | ||
recentNodeIdentitySet.add(pn.getLocation()); | ||
recentNodeIdentityQueue.offerFirst(pn.getLocation()); | ||
while (recentNodeIdentityQueue.size() > (0.81 * unconnectedNodesLength)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says 80% but the implementation says 81%. I don't have a strong opinion on the actual percentage used, but please do align the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I aligned the comment with the code — 0.81 is just to ensure that 5 nodes with 4 already checked is not an edge case (exactly matching the boundary).
@bertm thank you for your review! I merged this and so 1499 is no longer blocked, but I did not expect the flurry of great PRs coming in today and my brain is already sloshed, so I’ll defer preparing 1499 to tomorrow so we all have some more time to review (and hopefully merge) PRs. ❤️ |
No worries, I'm just pushing a lot of things that accumulated on my local |
Only sort the list nominalPeer when necessary.
That sorting can cause Pings to be sent. Now sorting is only done when
receiving a new noderef and at random intervals.
This should make the node less visible in DNS requests and Pings (more
typical pattern instead of requesting all peers at the same time).
Also it fixes the startup time regression by not doing a ping in the
PeerNode constructor.
The sorting result in the constructor was overwritten anyway …