This repository has been archived by the owner on Sep 26, 2019. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 130
[PAN-2614] Add simple PeerPermissions interface #1446
Merged
mbaxter
merged 24 commits into
PegaSysEng:master
from
mbaxter:PAN-2614/add-peer-permissions-interface
May 17, 2019
Merged
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
d818464
Add LimitedSet utility
mbaxter 885389d
Add a minimal PeerPermissions interface
mbaxter 5467e30
Break up PeerBlacklist into smaller components using new interface
mbaxter 69c244d
Dedupe identical functions
mbaxter 7dc7111
Modify PeerPermissions callback to consume info on type of update
mbaxter c252f00
PeerPermissions update supports an optional list of affected peers
mbaxter 3d8926d
Simplify permissions checks in discovery controller
mbaxter 83c93e9
Remove peer dropped event handling (dead code)
mbaxter 0b3b7f4
Add PeerDiscoveryController Builder
mbaxter b95a78e
Be more selective when dropping peers from table
mbaxter 08ca7b3
Validate banned-node-ids as valid node ids
mbaxter 6d63cd3
Connect to peers that have not been recently contacted
mbaxter 650c05b
Fix PantheonCommandTest by using valid node id
mbaxter 0d6d45e
Merge branch 'master' into PAN-2614/add-peer-permissions-interface
mbaxter ab94628
Check permissions at discovery layer considering directionality
mbaxter 13033b2
Rename interface for clarity
mbaxter 0bfb168
Validate peers before initiating connection
mbaxter e1b0045
Merge branch 'master' into PAN-2614/add-peer-permissions-interface
mbaxter 8740f74
Clean up PeerPermissions utils
mbaxter 2f5daf7
Merge branch 'master' into PAN-2614/add-peer-permissions-interface
mbaxter ff332f4
Merge branch 'master' into PAN-2614/add-peer-permissions-interface
mbaxter 69f8d0a
Use ImmutableList
mbaxter e05836f
Move utility method to EnodeURL
mbaxter 01a3a1c
Merge branch 'master' into PAN-2614/add-peer-permissions-interface
mbaxter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,25 +14,19 @@ | |
|
||
import static com.google.common.base.Preconditions.checkArgument; | ||
import static com.google.common.base.Preconditions.checkNotNull; | ||
import static java.util.concurrent.TimeUnit.MILLISECONDS; | ||
import static tech.pegasys.pantheon.util.bytes.BytesValue.wrapBuffer; | ||
|
||
import tech.pegasys.pantheon.crypto.SECP256K1; | ||
import tech.pegasys.pantheon.ethereum.p2p.api.DisconnectCallback; | ||
import tech.pegasys.pantheon.ethereum.p2p.api.PeerConnection; | ||
import tech.pegasys.pantheon.ethereum.p2p.config.DiscoveryConfiguration; | ||
import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryEvent.PeerBondedEvent; | ||
import tech.pegasys.pantheon.ethereum.p2p.discovery.PeerDiscoveryEvent.PeerDroppedEvent; | ||
import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.Packet; | ||
import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerDiscoveryController; | ||
import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerDiscoveryController.AsyncExecutor; | ||
import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerRequirement; | ||
import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PeerTable; | ||
import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.PingPacketData; | ||
import tech.pegasys.pantheon.ethereum.p2p.discovery.internal.TimerUtil; | ||
import tech.pegasys.pantheon.ethereum.p2p.peers.DefaultPeerId; | ||
import tech.pegasys.pantheon.ethereum.p2p.peers.PeerBlacklist; | ||
import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage; | ||
import tech.pegasys.pantheon.ethereum.p2p.peers.PeerId; | ||
import tech.pegasys.pantheon.ethereum.p2p.permissions.PeerPermissions; | ||
import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningController; | ||
import tech.pegasys.pantheon.metrics.MetricsSystem; | ||
import tech.pegasys.pantheon.util.NetworkUtility; | ||
|
@@ -47,7 +41,6 @@ | |
import java.util.OptionalInt; | ||
import java.util.concurrent.CompletableFuture; | ||
import java.util.concurrent.CopyOnWriteArrayList; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.function.Consumer; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.Stream; | ||
|
@@ -61,17 +54,16 @@ | |
* The peer discovery agent is the network component that sends and receives peer discovery messages | ||
* via UDP. | ||
*/ | ||
public abstract class PeerDiscoveryAgent implements DisconnectCallback { | ||
public abstract class PeerDiscoveryAgent { | ||
protected static final Logger LOG = LogManager.getLogger(); | ||
|
||
// The devp2p specification says only accept packets up to 1280, but some | ||
// clients ignore that, so we add in a little extra padding. | ||
private static final int MAX_PACKET_SIZE_BYTES = 1600; | ||
private static final long PEER_REFRESH_INTERVAL_MS = MILLISECONDS.convert(30, TimeUnit.MINUTES); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved this constant into |
||
|
||
protected final List<DiscoveryPeer> bootstrapPeers; | ||
private final List<PeerRequirement> peerRequirements = new CopyOnWriteArrayList<>(); | ||
private final PeerBlacklist peerBlacklist; | ||
private final PeerPermissions peerPermissions; | ||
private final Optional<NodePermissioningController> nodePermissioningController; | ||
private final MetricsSystem metricsSystem; | ||
/* The peer controller, which takes care of the state machine of peers. */ | ||
|
@@ -80,7 +72,6 @@ public abstract class PeerDiscoveryAgent implements DisconnectCallback { | |
/* The keypair used to sign messages. */ | ||
protected final SECP256K1.KeyPair keyPair; | ||
private final BytesValue id; | ||
private final PeerTable peerTable; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
protected final DiscoveryConfiguration config; | ||
|
||
/* This is the {@link tech.pegasys.pantheon.ethereum.p2p.Peer} object holding who we are. */ | ||
|
@@ -89,12 +80,11 @@ public abstract class PeerDiscoveryAgent implements DisconnectCallback { | |
/* Is discovery enabled? */ | ||
private boolean isActive = false; | ||
private final Subscribers<Consumer<PeerBondedEvent>> peerBondedObservers = new Subscribers<>(); | ||
private final Subscribers<Consumer<PeerDroppedEvent>> peerDroppedObservers = new Subscribers<>(); | ||
|
||
public PeerDiscoveryAgent( | ||
final SECP256K1.KeyPair keyPair, | ||
final DiscoveryConfiguration config, | ||
final PeerBlacklist peerBlacklist, | ||
final PeerPermissions peerPermissions, | ||
final Optional<NodePermissioningController> nodePermissioningController, | ||
final MetricsSystem metricsSystem) { | ||
this.metricsSystem = metricsSystem; | ||
|
@@ -103,14 +93,13 @@ public PeerDiscoveryAgent( | |
|
||
validateConfiguration(config); | ||
|
||
this.peerBlacklist = peerBlacklist; | ||
this.peerPermissions = peerPermissions; | ||
this.nodePermissioningController = nodePermissioningController; | ||
this.bootstrapPeers = | ||
config.getBootnodes().stream().map(DiscoveryPeer::fromEnode).collect(Collectors.toList()); | ||
|
||
this.config = config; | ||
this.keyPair = keyPair; | ||
this.peerTable = new PeerTable(keyPair.getPublicKey().getEncodedBytes(), 16); | ||
|
||
id = keyPair.getPublicKey().getEncodedBytes(); | ||
} | ||
|
@@ -164,21 +153,19 @@ private void startController() { | |
} | ||
|
||
private PeerDiscoveryController createController() { | ||
return new PeerDiscoveryController( | ||
keyPair, | ||
advertisedPeer, | ||
peerTable, | ||
bootstrapPeers, | ||
this::handleOutgoingPacket, | ||
createTimer(), | ||
createWorkerExecutor(), | ||
PEER_REFRESH_INTERVAL_MS, | ||
PeerRequirement.combine(peerRequirements), | ||
peerBlacklist, | ||
nodePermissioningController, | ||
peerBondedObservers, | ||
peerDroppedObservers, | ||
metricsSystem); | ||
return PeerDiscoveryController.builder() | ||
.keypair(keyPair) | ||
.localPeer(advertisedPeer) | ||
.bootstrapNodes(bootstrapPeers) | ||
.outboundMessageHandler(this::handleOutgoingPacket) | ||
.timerUtil(createTimer()) | ||
.workerExecutor(createWorkerExecutor()) | ||
.peerRequirement(PeerRequirement.combine(peerRequirements)) | ||
.peerPermissions(peerPermissions) | ||
.nodePermissioningController(nodePermissioningController) | ||
.peerBondedObservers(peerBondedObservers) | ||
.metricsSystem(metricsSystem) | ||
.build(); | ||
} | ||
|
||
protected boolean validatePacketSize(final int packetSize) { | ||
|
@@ -240,6 +227,10 @@ public Stream<DiscoveryPeer> streamDiscoveredPeers() { | |
return controller.map(PeerDiscoveryController::streamDiscoveredPeers).orElse(Stream.empty()); | ||
} | ||
|
||
public void dropPeer(final PeerId peer) { | ||
controller.ifPresent(c -> c.dropPeer(peer)); | ||
} | ||
|
||
public Optional<DiscoveryPeer> getAdvertisedPeer() { | ||
return Optional.ofNullable(advertisedPeer); | ||
} | ||
|
@@ -272,29 +263,6 @@ public boolean removePeerBondedObserver(final long observerId) { | |
return peerBondedObservers.unsubscribe(observerId); | ||
} | ||
|
||
/** | ||
* Adds an observer that will get called when a peer is dropped from the peer table. | ||
* | ||
* <p><i>No guarantees are made about the order in which observers are invoked.</i> | ||
* | ||
* @param observer The observer to call. | ||
* @return A unique ID identifying this observer, to that it can be removed later. | ||
*/ | ||
public long observePeerDroppedEvents(final Consumer<PeerDroppedEvent> observer) { | ||
checkNotNull(observer); | ||
return peerDroppedObservers.subscribe(observer); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was dead code - listeners were never actually notified of dropped peers |
||
} | ||
|
||
/** | ||
* Removes an previously added peer dropped observer. | ||
* | ||
* @param observerId The unique ID identifying the observer to remove. | ||
* @return Whether the observer was located and removed. | ||
*/ | ||
public boolean removePeerDroppedObserver(final long observerId) { | ||
return peerDroppedObservers.unsubscribe(observerId); | ||
} | ||
|
||
/** | ||
* Returns the count of observers that are registered on this controller. | ||
* | ||
|
@@ -320,15 +288,6 @@ private static void validateConfiguration(final DiscoveryConfiguration config) { | |
checkArgument(config.getBucketSize() > 0, "bucket size cannot be negative nor zero"); | ||
} | ||
|
||
@Override | ||
public void onDisconnect( | ||
final PeerConnection connection, | ||
final DisconnectMessage.DisconnectReason reason, | ||
final boolean initiatedByPeer) { | ||
final BytesValue nodeId = connection.getPeerInfo().getNodeId(); | ||
peerTable.tryEvict(new DefaultPeerId(nodeId)); | ||
} | ||
|
||
/** | ||
* Returns the current state of the PeerDiscoveryAgent. | ||
* | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We no longer need to configure an empty set of banned nodes