Skip to content
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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Nov 30, 2024
Merged

Reduce pings and dns lookups #993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
472c0af
Only update the handshake IPs of one random node per check, but check…
ArneBab Nov 12, 2024
96b9241
Only sort the list nominalPeer when necessary.
ArneBab Nov 12, 2024
ced3b16
Avoid coupon collector’s problem — thanks to bertm!
ArneBab Nov 30, 2024
1cf9ed5
Update short to string if a detectedPeer is first set
ArneBab Nov 30, 2024
8957221
Fix: at less than 5 unconnected nodes no optimization is needed
ArneBab Nov 30, 2024
3bdf89a
Reduce DNS checking frequency now that node selection is more efficient
ArneBab Nov 30, 2024
61e74fa
Move wait time calculation out of try synchronized
ArneBab Nov 30, 2024
ae05c2c
Early return if nothing to do
ArneBab Nov 30, 2024
6220388
Align comment with code
ArneBab Nov 30, 2024
825bc26
Remove commented out debug code
ArneBab Nov 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 49 additions & 13 deletions src/freenet/node/DNSRequester.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@
* http://www.gnu.org/ for further details of the GPL. */
package freenet.node;

import java.util.ArrayDeque;
import java.util.Arrays;
import java.util.Deque;
import java.util.HashSet;
import java.util.Set;

import freenet.support.LogThresholdCallback;
import freenet.support.Logger;
import freenet.support.Logger.LogLevel;
Expand All @@ -16,6 +22,8 @@ public class DNSRequester implements Runnable {

final Node node;
private long lastLogTime;
private final Set<Double> recentNodeIdentitySet = new HashSet<>();
private final Deque<Double> recentNodeIdentityQueue = new ArrayDeque<>();
// Only set when doing simulations.
static boolean DISABLE = false;

Expand Down Expand Up @@ -54,25 +62,53 @@ public void run() {
}

private void realRun() {
PeerNode[] nodes = node.getPeers().myPeers();
long now = System.currentTimeMillis();
if((now - lastLogTime) > 1000) {
if(logMINOR)
Logger.minor(this, "Processing DNS Requests (log rate-limited)");
// run DNS requests for not recently checked, unconnected
// nodes to avoid the coupon collector's problem
PeerNode[] nodesToCheck = Arrays.stream(node.getPeers().myPeers())
.filter(peerNode -> !peerNode.isConnected())
// identify recent nodes by location, because the exact location cannot be used twice
// (that already triggers the simplest pitch black attack defenses)
// Double may not be comparable in general (floating point),
// but just checking for equality with itself is safe
.filter(peerNode -> !recentNodeIdentitySet.contains(peerNode.getLocation()))
.toArray(PeerNode[]::new);

if(logMINOR) {
long now = System.currentTimeMillis();
if((now - lastLogTime) > 100) {
Logger.minor(this, "Processing DNS Requests (log rate-limited)");
}
lastLogTime = now;
}
for(PeerNode pn: nodes) {
//Logger.minor(this, "Node: "+pn);
if(!pn.isConnected()) {
// Not connected
// Try new DNS lookup
//Logger.minor(this, "Doing lookup on "+pn+" of "+nodes.length);
pn.maybeUpdateHandshakeIPs(false);

int unconnectedNodesLength = nodesToCheck.length;
if (unconnectedNodesLength == 0) {
return; // nothing to do
}
// check a randomly chosen node that has not been checked
// recently to avoid sending bursts of DNS requests
PeerNode pn = nodesToCheck[node.getFastWeakRandom().nextInt(unconnectedNodesLength)];
Copy link
Contributor

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.

Copy link
Contributor Author

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!

if (unconnectedNodesLength < 5) {
// no need for optimizations: just clear all state
recentNodeIdentitySet.clear();
recentNodeIdentityQueue.clear();
} else {
// do not request this node again,
// until at least 81% of the other unconnected nodes have been checked
recentNodeIdentitySet.add(pn.getLocation());
recentNodeIdentityQueue.offerFirst(pn.getLocation());
while (recentNodeIdentityQueue.size() > (0.81 * unconnectedNodesLength)) {
Copy link
Contributor

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.

Copy link
Contributor Author

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

recentNodeIdentitySet.remove(recentNodeIdentityQueue.removeLast());
}
}

// Try new DNS lookup
pn.maybeUpdateHandshakeIPs(false);

int nextMaxWaitTime = 1000 + node.getFastWeakRandom().nextInt(60000);
try {
synchronized(this) {
wait(10000); // sleep 10s ...
wait(nextMaxWaitTime); // sleep 1-61s ...
}
} catch (InterruptedException e) {
// Ignore, just wake up. Just sleeping to not busy wait anyway
Expand Down
58 changes: 32 additions & 26 deletions src/freenet/node/PeerNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Comparator;
import java.util.EnumMap;
import java.util.GregorianCalendar;
import java.util.HashSet;
Expand Down Expand Up @@ -142,7 +141,7 @@ public abstract class PeerNode implements USKRetrieverCallback, BasePeerNode, Pe

protected long jfkContextLifetime = 0;
/** My low-level address for SocketManager purposes */
private Peer detectedPeer;
private Peer detectedPeer = null;
/** My OutgoingPacketMangler i.e. the object which encrypts packets sent to this node */
private final OutgoingPacketMangler outgoingMangler;
/** Advertised addresses */
Expand Down Expand Up @@ -456,7 +455,7 @@ public PeerNode(SimpleFieldSet fs, Node node2, NodeCrypto crypto, boolean fromLo

testnetEnabled = fs.getBoolean("testnet", false);
if(testnetEnabled) {
String err = "Ignoring incompatible testnet node " + detectedPeer;
String err = "Ignoring incompatible testnet node " + fs.toOrderedString();
Logger.error(this, err);
throw new PeerParseException(err);
}
Expand Down Expand Up @@ -543,8 +542,7 @@ public PeerNode(SimpleFieldSet fs, Node node2, NodeCrypto crypto, boolean fromLo
"\nNode: " + HexUtil.bytesToHex(nodeKey) +
"\nNode hash: " + HexUtil.bytesToHex(nodeKeyHash) +
"\nThis: " + HexUtil.bytesToHex(identityHash) +
"\nThis hash: " + HexUtil.bytesToHex(identityHashHash) +
"\nFor: " + getPeer());
"\nThis hash: " + HexUtil.bytesToHex(identityHashHash));

try {
incomingSetupCipher = new Rijndael(256, 256);
Expand Down Expand Up @@ -593,12 +591,6 @@ public PeerNode(SimpleFieldSet fs, Node node2, NodeCrypto crypto, boolean fromLo
}
if(nominalPeer.isEmpty()) {
Logger.normal(this, "No IP addresses found for identity '" + identityAsBase64String + "', possibly at location '" + location + ": " + userToString());
detectedPeer = null;
} else {
nominalPeer.sort(Peer.PEER_COMPARATOR);
// TODO this throws away all valid addresses but the first, without checking whether they can connect. Need to try a later one if connection fails.
// sort hostName first.
detectedPeer = nominalPeer.get(0);
}
updateShortToString();

Expand Down Expand Up @@ -692,7 +684,6 @@ public PeerNode(SimpleFieldSet fs, Node node2, NodeCrypto crypto, boolean fromLo
}
// populate handshakeIPs so handshakes can start ASAP
lastAttemptedHandshakeIPUpdateTime = 0;
maybeUpdateHandshakeIPs(true);

listeningHandshakeBurstCount = 0;
listeningHandshakeBurstSize = Node.MIN_BURSTING_HANDSHAKE_BURST_SIZE
Expand Down Expand Up @@ -788,9 +779,18 @@ private boolean parseARK(SimpleFieldSet fs, boolean onStartup, boolean forDiffNo
*/
@Override
public synchronized Peer getPeer() {
if (detectedPeer == null && !nominalPeer.isEmpty()) {
sortNominalPeer();
detectedPeer = nominalPeer.get(0);
Copy link
Contributor

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().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 1cf9ed5

updateShortToString();
}
return detectedPeer;
}

private void sortNominalPeer() {
nominalPeer.sort(Peer.PEER_COMPARATOR);
}

/**
* Returns an array with the advertised addresses and the detected one
*/
Expand Down Expand Up @@ -863,7 +863,7 @@ public void maybeUpdateHandshakeIPs(boolean ignoreHostnames) {
long now = System.currentTimeMillis();
Peer localDetectedPeer = null;
synchronized(this) {
localDetectedPeer = detectedPeer;
localDetectedPeer = getPeer();
if((now - lastAttemptedHandshakeIPUpdateTime) < MINUTES.toMillis(5)) {
//Logger.minor(this, "Looked up recently (localDetectedPeer = "+localDetectedPeer + " : "+((localDetectedPeer == null) ? "" : localDetectedPeer.getAddress(false).toString()));
return;
Expand Down Expand Up @@ -1187,7 +1187,7 @@ public void startRekeying() {
sendHandshakeTime = now; // Immediately
ctx = null;
}
Logger.normal(this, "We are asking for the key to be renewed (" + this.detectedPeer + ')');
Logger.normal(this, "We are asking for the key to be renewed (" + this.getPeer() + ')');
}

/**
Expand Down Expand Up @@ -1414,7 +1414,12 @@ public boolean shouldSendHandshake() {
boolean tempShouldSendHandshake = false;
synchronized(this) {
if(disconnecting) return false;
tempShouldSendHandshake = ((now > sendHandshakeTime) && (handshakeIPs != null) && (isRekeying || !isConnected()));
if (now > sendHandshakeTime) {
maybeUpdateHandshakeIPs(true);
tempShouldSendHandshake = ((now > sendHandshakeTime) && (getHandshakeIPs() != null) && (
isRekeying
|| !isConnected()));
}
}
if(logMINOR) Logger.minor(this, "shouldSendHandshake(): initial = "+tempShouldSendHandshake);
if(tempShouldSendHandshake && (hasLiveHandshake(now)))
Expand Down Expand Up @@ -1807,7 +1812,7 @@ private void setDetectedPeer(Peer newPeer) {
return;
}
synchronized(this) {
Peer oldPeer = detectedPeer;
Peer oldPeer = getPeer();
if((newPeer != null) && ((oldPeer == null) || !oldPeer.equals(newPeer))) {
this.detectedPeer = newPeer;
updateShortToString();
Expand Down Expand Up @@ -2256,7 +2261,7 @@ protected void sendInitialMessages() {
loadSender(true).setSendASAP();
loadSender(false).setSendASAP();
Message locMsg = DMT.createFNPLocChangeNotificationNew(node.getLocationManager().getLocation(), node.getPeers().getPeerLocationDoubles(true));
Message ipMsg = DMT.createFNPDetectedIPAddress(detectedPeer);
Message ipMsg = DMT.createFNPDetectedIPAddress(getPeer());
Message timeMsg = DMT.createFNPTime(System.currentTimeMillis());
Message dRoutingMsg = DMT.createRoutingStatus(!disableRoutingHasBeenSetLocally);
Message uptimeMsg = DMT.createFNPUptime((byte)(int)(100*node.getUptimeEstimator().getUptime()));
Expand All @@ -2276,7 +2281,7 @@ protected void sendInitialMessages() {
}

private void sendIPAddressMessage() {
Message ipMsg = DMT.createFNPDetectedIPAddress(detectedPeer);
Message ipMsg = DMT.createFNPDetectedIPAddress(getPeer());
try {
sendAsync(ipMsg, null, node.getNodeStats().changedIPCtr);
} catch(NotConnectedException e) {
Expand Down Expand Up @@ -2444,7 +2449,7 @@ protected synchronized boolean innerProcessNewNoderef(SimpleFieldSet fs, boolean
// Anything may be omitted for a differential node reference
boolean changedAnything = false;
if(!forDiffNodeRef && (false != fs.getBoolean("testnet", false))) {
String err = "Preventing connection to node " + detectedPeer +" - testnet is enabled!";
String err = "Preventing connection to node " + getPeer() +" - testnet is enabled!";
Logger.error(this, err);
throw new FSParseException(err);
}
Expand Down Expand Up @@ -2554,8 +2559,9 @@ else if(logMINOR)
nominalPeer.add(p);
}
}
sortNominalPeer();
// XXX should we trigger changedAnything on *any* change, or on just *addition* of new addresses
if(!Arrays.equals(oldPeers, nominalPeer.toArray(new Peer[nominalPeer.size()]))) {
if(!Arrays.equals(oldPeers, nominalPeer.toArray(new Peer[0]))) {
changedAnything = true;
if(logMINOR) Logger.minor(this, "Got new physical.udp for "+this+" : "+Arrays.toString(nominalPeer.toArray()));
// Look up the DNS names if any ASAP
Expand Down Expand Up @@ -2694,8 +2700,8 @@ public synchronized SimpleFieldSet exportDiskFieldSet() {
*/
public synchronized SimpleFieldSet exportMetadataFieldSet(long now) {
SimpleFieldSet fs = new SimpleFieldSet(true);
if(detectedPeer != null)
fs.putSingle("detected.udp", detectedPeer.toStringPrefNumeric());
if(getPeer() != null)
fs.putSingle("detected.udp", getPeer().toStringPrefNumeric());
if(lastReceivedPacketTime() > 0)
fs.put("timeLastReceivedPacket", timeLastReceivedPacket);
if(lastReceivedAckTime() > 0)
Expand Down Expand Up @@ -3956,7 +3962,7 @@ public WeakReference<PeerNode> getWeakRef() {
public Peer getHandshakeIP() {
Peer[] localHandshakeIPs;
if(!shouldSendHandshake()) {
if(logMINOR) Logger.minor(this, "Not sending handshake to "+getPeer()+" because pn.shouldSendHandshake() returned false");
if(logMINOR) Logger.minor(this, "Not sending handshake to "+detectedPeer+" because pn.shouldSendHandshake() returned false");
return null;
}
long firstTime = System.currentTimeMillis();
Expand Down Expand Up @@ -5427,7 +5433,7 @@ public Random paddingGen() {
}

public synchronized boolean matchesPeerAndPort(Peer peer) {
if(detectedPeer != null && detectedPeer.laxEquals(peer)) return true;
if(getPeer() != null && getPeer().laxEquals(peer)) return true;
if(nominalPeer != null) { // FIXME condition necessary???
for(Peer p : nominalPeer) {
if(p != null && p.laxEquals(peer)) return true;
Expand All @@ -5440,8 +5446,8 @@ public synchronized boolean matchesPeerAndPort(Peer peer) {
* @param strict If true, only match if the IP is actually in use. If false,
* also match from nominal IP addresses and domain names etc. */
public synchronized boolean matchesIP(FreenetInetAddress addr, boolean strict) {
if(detectedPeer != null) {
FreenetInetAddress a = detectedPeer.getFreenetAddress();
if(getPeer() != null) {
FreenetInetAddress a = getPeer().getFreenetAddress();
if(a != null) {
if(strict ? a.equals(addr) : a.laxEquals(addr))
return true;
Expand Down