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

Apply expiration checks in PacketData classes #8186

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+313 −108
Open

Apply expiration checks in PacketData classes #8186

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e32cbe7
Apply expiration checks in PacketData classes
Matilda-Clerke Jan 29, 2025
5febb43
Fix broken unit tests
Matilda-Clerke Jan 29, 2025
853261f
spotless
Matilda-Clerke Jan 29, 2025
6d9183b
Fix failing test
Matilda-Clerke Jan 29, 2025
831388c
Refactor to use clock to validate expiry
Matilda-Clerke Jan 29, 2025
7485106
Merge branch 'main' into check-expiry-of-several-packet-data-classes
Matilda-Clerke Jan 30, 2025
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
3 changes: 2 additions & 1 deletion ...2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/VertxPeerDiscoveryAgent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import java.net.InetSocketAddress;
import java.net.SocketException;
import java.nio.channels.UnsupportedAddressTypeException;
import java.time.Clock;
import java.util.Optional;
import java.util.concurrent.CompletableFuture;
import java.util.function.IntSupplier;
Expand Down Expand Up @@ -290,7 +291,7 @@ private void handlePacket(final DatagramPacket datagram) {
vertx.<Packet>executeBlocking(
future -> {
try {
future.complete(Packet.decode(datagram.data()));
future.complete(Packet.decode(datagram.data(), Clock.systemUTC()));
} catch (final Throwable t) {
future.fail(t);
}
Expand Down
18 changes: 13 additions & 5 deletions .../main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/ENRRequestPacketData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,31 +19,39 @@
import org.hyperledger.besu.ethereum.rlp.RLPInput;
import org.hyperledger.besu.ethereum.rlp.RLPOutput;

import java.time.Clock;

public class ENRRequestPacketData implements PacketData {
/* In seconds after epoch. */
private final long expiration;

private ENRRequestPacketData(final long expiration) {
checkArgument(expiration >= 0, "expiration cannot be negative");

this.expiration = expiration;
}

public static ENRRequestPacketData create() {
return create(PacketData.defaultExpiration());
return create(PacketData.defaultExpiration(), Clock.systemUTC());
}

static ENRRequestPacketData create(final long expirationSec) {
static ENRRequestPacketData create(final long expirationSec, final Clock clock) {
validateParameters(expirationSec, clock);
return new ENRRequestPacketData(expirationSec);
}

public static ENRRequestPacketData readFrom(final RLPInput in) {
public static ENRRequestPacketData readFrom(final RLPInput in, final Clock clock) {
in.enterList();
final long expiration = in.readLongScalar();
in.leaveListLenient();
validateParameters(expiration, clock);
return new ENRRequestPacketData(expiration);
}

private static void validateParameters(final long expiration, final Clock clock) {
checkArgument(expiration >= 0, "expiration cannot be negative");
checkArgument(
expiration >= clock.instant().getEpochSecond(), "expiration cannot be in the past");
Copy link
Contributor

@garyschulte garyschulte Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we need to plumb Clock UTC everywhere, is it a testing concern? AFAIK Instant.now().toEpochMilli() is always in UTC.

If it is a testing concern about wanting to use a static clock, couldn't we spy and mock a non-static validateParameters method, or move this as a default method into PacketData for example? and then just directly test the real method in isolation. It should be the same coverage without the need to add clock to constructors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a testing concern, yes. In many tests, we're using old hard-coded packets encoded in RLP, so the only way to ensure they're considered "valid" is to compare them against an appropriate instant, now provided by the supplied clock. Ideally, dependency injection of the clock into a PacketDataFactory, would remove this from the method parameters we see here, but that's a refactoring out of scope for this PR.

I suppose we could use a spy to mock calls against a non-static validateParameters method, but truthfully, I'd rather refactor the code and maybe utilise dagger. It'll be much easier to maintain moving forward. Since you're the third person to mention it this morning, I'll branch off and work on the refactoring now.

}

@Override
public void writeTo(final RLPOutput out) {
out.startList();
Expand Down
4 changes: 3 additions & 1 deletion ...main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/ENRResponsePacketData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import org.hyperledger.besu.ethereum.rlp.RLPInput;
import org.hyperledger.besu.ethereum.rlp.RLPOutput;

import java.time.Clock;

import org.apache.tuweni.bytes.Bytes;
import org.ethereum.beacon.discovery.schema.NodeRecord;
import org.ethereum.beacon.discovery.schema.NodeRecordFactory;
Expand All @@ -42,7 +44,7 @@ public static ENRResponsePacketData create(final Bytes requestHash, final NodeRe
return new ENRResponsePacketData(requestHash, enr);
}

public static ENRResponsePacketData readFrom(final RLPInput in) {
public static ENRResponsePacketData readFrom(final RLPInput in, final Clock unused) {
in.enterList();
final Bytes requestHash = in.readBytes();
in.leaveListLenient();
Expand Down
22 changes: 16 additions & 6 deletions ...in/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/FindNeighborsPacketData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import org.hyperledger.besu.ethereum.rlp.RLPInput;
import org.hyperledger.besu.ethereum.rlp.RLPOutput;

import java.time.Clock;

import org.apache.tuweni.bytes.Bytes;

public class FindNeighborsPacketData implements PacketData {
Expand All @@ -31,18 +33,17 @@ public class FindNeighborsPacketData implements PacketData {
private final long expiration;

private FindNeighborsPacketData(final Bytes target, final long expiration) {
checkArgument(target != null && target.size() == TARGET_SIZE, "target must be a valid node id");
checkArgument(expiration >= 0, "expiration must be positive");

this.target = target;
this.expiration = expiration;
}

public static FindNeighborsPacketData create(final Bytes target) {
return create(target, PacketData.defaultExpiration());
return create(target, PacketData.defaultExpiration(), Clock.systemUTC());
}

static FindNeighborsPacketData create(final Bytes target, final long expirationSec) {
static FindNeighborsPacketData create(
final Bytes target, final long expirationSec, final Clock clock) {
validateParameters(target, expirationSec, clock);
return new FindNeighborsPacketData(target, expirationSec);
}

Expand All @@ -54,14 +55,23 @@ public void writeTo(final RLPOutput out) {
out.endList();
}

public static FindNeighborsPacketData readFrom(final RLPInput in) {
public static FindNeighborsPacketData readFrom(final RLPInput in, final Clock clock) {
in.enterList();
final Bytes target = in.readBytes();
final long expiration = in.readLongScalar();
in.leaveListLenient();
validateParameters(target, expiration, clock);
return new FindNeighborsPacketData(target, expiration);
}

private static void validateParameters(
final Bytes target, final long expiration, final Clock clock) {
checkArgument(target != null && target.size() == TARGET_SIZE, "target must be a valid node id");
checkArgument(expiration >= 0, "expiration cannot be negative");
checkArgument(
expiration >= clock.instant().getEpochSecond(), "expiration cannot be in the past");
}

public long getExpiration() {
return expiration;
}
Expand Down
21 changes: 15 additions & 6 deletions ...c/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/NeighborsPacketData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.hyperledger.besu.ethereum.rlp.RLPInput;
import org.hyperledger.besu.ethereum.rlp.RLPOutput;

import java.time.Clock;
import java.util.List;

public class NeighborsPacketData implements PacketData {
Expand All @@ -30,25 +31,33 @@ public class NeighborsPacketData implements PacketData {
private final long expiration;

private NeighborsPacketData(final List<DiscoveryPeer> peers, final long expiration) {
checkArgument(peers != null, "peer list cannot be null");
checkArgument(expiration >= 0, "expiration must be positive");

this.peers = peers;
this.expiration = expiration;
}

public static NeighborsPacketData create(final List<DiscoveryPeer> peers) {
return new NeighborsPacketData(peers, PacketData.defaultExpiration());
public static NeighborsPacketData create(final List<DiscoveryPeer> peers, final Clock clock) {
final long expiration = PacketData.defaultExpiration();
validateParameters(peers, expiration, clock);
return new NeighborsPacketData(peers, expiration);
}

public static NeighborsPacketData readFrom(final RLPInput in) {
public static NeighborsPacketData readFrom(final RLPInput in, final Clock clock) {
in.enterList();
final List<DiscoveryPeer> peers = in.readList(DiscoveryPeer::readFrom);
final long expiration = in.readLongScalar();
in.leaveListLenient();
validateParameters(peers, expiration, clock);
return new NeighborsPacketData(peers, expiration);
}

private static void validateParameters(
final List<DiscoveryPeer> peers, final long expiration, final Clock clock) {
checkArgument(peers != null, "peer list cannot be null");
checkArgument(expiration >= 0, "expiration cannot be negative");
checkArgument(
expiration >= clock.instant().getEpochSecond(), "expiration cannot be in the past");
}

@Override
public void writeTo(final RLPOutput out) {
out.startList();
Expand Down
6 changes: 4 additions & 2 deletions ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/Packet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.hyperledger.besu.ethereum.rlp.RLPException;

import java.math.BigInteger;
import java.time.Clock;
import java.util.Arrays;
import java.util.Optional;

Expand Down Expand Up @@ -99,7 +100,7 @@ public static Packet create(
return new Packet(packetType, packetData, nodeKey);
}

public static Packet decode(final Buffer message) {
public static Packet decode(final Buffer message, final Clock clock) {
checkGuard(
message.length() >= PACKET_DATA_INDEX,
PeerDiscoveryPacketDecodingException::new,
Expand All @@ -122,7 +123,8 @@ public static Packet decode(final Buffer message) {
deserializer.deserialize(
RLP.input(
Bytes.wrapBuffer(
message, PACKET_DATA_INDEX, message.length() - PACKET_DATA_INDEX)));
message, PACKET_DATA_INDEX, message.length() - PACKET_DATA_INDEX)),
clock);
return new Packet(packetType, packetData, Bytes.wrapBuffer(message));
} catch (final RLPException e) {
throw new PeerDiscoveryPacketDecodingException("Malformed packet of type: " + packetType, e);
Expand Down
3 changes: 2 additions & 1 deletion ...um/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PacketType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import org.hyperledger.besu.ethereum.rlp.RLPInput;

import java.time.Clock;
import java.util.Arrays;
import java.util.Optional;
import javax.annotation.concurrent.Immutable;
Expand Down Expand Up @@ -63,6 +64,6 @@ public Deserializer<?> getDeserializer() {
@FunctionalInterface
@Immutable
public interface Deserializer<T extends PacketData> {
T deserialize(RLPInput in);
T deserialize(RLPInput in, Clock clock);
}
}
3 changes: 2 additions & 1 deletion ...in/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.hyperledger.besu.plugin.services.metrics.Counter;
import org.hyperledger.besu.plugin.services.metrics.LabelledMetric;

import java.time.Clock;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -684,7 +685,7 @@ private void respondToFindNeighbors(
// 88 * 13 = 1144 bytes
// To fit under 1280 bytes, we must return just 13 peers maximum.
final List<DiscoveryPeer> peers = peerTable.nearestBondedPeers(packetData.getTarget(), 13);
final PacketData data = NeighborsPacketData.create(peers);
final PacketData data = NeighborsPacketData.create(peers, Clock.systemUTC());
sendPacket(sender, PacketType.NEIGHBORS, data);
}

Expand Down
22 changes: 15 additions & 7 deletions ...2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PingPacketData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.hyperledger.besu.ethereum.rlp.RLPInput;
import org.hyperledger.besu.ethereum.rlp.RLPOutput;

import java.time.Clock;
import java.util.Optional;

import org.apache.tuweni.units.bigints.UInt64;
Expand Down Expand Up @@ -50,8 +51,6 @@ private PingPacketData(
final Endpoint to,
final long expiration,
final UInt64 enrSeq) {
checkArgument(to != null, "destination endpoint cannot be null");
checkArgument(expiration >= 0, "expiration cannot be negative");

this.maybeFrom = maybeFrom;
this.to = to;
Expand All @@ -61,22 +60,22 @@ private PingPacketData(

public static PingPacketData create(
final Optional<Endpoint> from, final Endpoint to, final UInt64 enrSeq) {
checkArgument(
enrSeq != null && UInt64.ZERO.compareTo(enrSeq) < 0, "enrSeq cannot be null or negative");
return create(from, to, PacketData.defaultExpiration(), enrSeq);
return create(from, to, PacketData.defaultExpiration(), enrSeq, Clock.systemUTC());
}

static PingPacketData create(
final Optional<Endpoint> from,
final Endpoint to,
final long expirationSec,
final UInt64 enrSeq) {
final UInt64 enrSeq,
final Clock clock) {
checkArgument(
enrSeq != null && UInt64.ZERO.compareTo(enrSeq) < 0, "enrSeq cannot be null or negative");
validateParameters(Optional.ofNullable(to), expirationSec, clock);
return new PingPacketData(from, to, expirationSec, enrSeq);
}

public static PingPacketData readFrom(final RLPInput in) {
public static PingPacketData readFrom(final RLPInput in, final Clock clock) {
in.enterList();
// The first element signifies the "version", but this value is ignored as of EIP-8
in.readBigIntegerScalar();
Expand Down Expand Up @@ -105,9 +104,18 @@ public static PingPacketData readFrom(final RLPInput in) {
}
}
in.leaveListLenient();
validateParameters(to, expiration, clock);
return new PingPacketData(from, to.get(), expiration, enrSeq);
}

private static void validateParameters(
final Optional<Endpoint> to, final long expiration, final Clock clock) {
checkArgument(to.isPresent(), "destination endpoint cannot be null");
checkArgument(expiration >= 0, "expiration cannot be negative");
checkArgument(
expiration >= clock.instant().getEpochSecond(), "expiration cannot be in the past");
}

/**
* Used by test classes to read legacy encodes of Pings which used a byte array for the ENR field
*
Expand Down
12 changes: 11 additions & 1 deletion ...2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PongPacketData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,14 @@
*/
package org.hyperledger.besu.ethereum.p2p.discovery.internal;

import static com.google.common.base.Preconditions.checkArgument;

import org.hyperledger.besu.ethereum.p2p.discovery.Endpoint;
import org.hyperledger.besu.ethereum.rlp.MalformedRLPInputException;
import org.hyperledger.besu.ethereum.rlp.RLPInput;
import org.hyperledger.besu.ethereum.rlp.RLPOutput;

import java.time.Clock;
import java.util.Optional;

import org.apache.tuweni.bytes.Bytes;
Expand Down Expand Up @@ -54,7 +57,7 @@ public static PongPacketData create(
return new PongPacketData(to, pingHash, PacketData.defaultExpiration(), enrSeq);
}

public static PongPacketData readFrom(final RLPInput in) {
public static PongPacketData readFrom(final RLPInput in, final Clock clock) {
in.enterList();
final Endpoint to = Endpoint.decodeStandalone(in);
final Bytes hash = in.readBytes();
Expand All @@ -70,9 +73,16 @@ public static PongPacketData readFrom(final RLPInput in) {
}
}
in.leaveListLenient();
validateParameters(expiration, clock);
return new PongPacketData(to, hash, expiration, enrSeq);
}

private static void validateParameters(final long expiration, final Clock clock) {
checkArgument(expiration >= 0, "expiration cannot be negative");
checkArgument(
expiration >= clock.instant().getEpochSecond(), "expiration cannot be in the past");
}

@Override
public void writeTo(final RLPOutput out) {
out.startList();
Expand Down
3 changes: 2 additions & 1 deletion ...p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@

import java.net.InetAddress;
import java.net.UnknownHostException;
import java.time.Clock;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
Expand Down Expand Up @@ -177,7 +178,7 @@ public void neighborsPacketFromUnbondedPeerIsDropped() {

// Generate an out-of-band NEIGHBORS message.
final List<DiscoveryPeer> peers = helper.createDiscoveryPeers(5);
final NeighborsPacketData data = NeighborsPacketData.create(peers);
final NeighborsPacketData data = NeighborsPacketData.create(peers, Clock.systemUTC());
final Packet packet = Packet.create(PacketType.NEIGHBORS, data, otherNode.getNodeKey());
helper.sendMessageBetweenAgents(otherNode, agent, packet);

Expand Down
Loading