Improve logging of peer disconnects due to subprotocol triggered reas…

…on (hyperledger#6886) Signed-off-by: Jason Frame <jason.frame@consensys.net> Signed-off-by: amsmota <antonio.mota@citi.com>
Citi · Apr 16, 2024 · 87ef9cb · 87ef9cb
1 parent cb41867
commit 87ef9cb
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 13 deletions.
diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManager.java
@@ -314,7 +314,8 @@ public void processMessage(final Capability cap, final Message message) {
             .setMessage("Post-merge disconnect: peer still gossiping blocks {}")
             .addArgument(ethPeer::toString)
             .log();
-        handleDisconnect(ethPeer.getConnection(), DisconnectReason.SUBPROTOCOL_TRIGGERED, false);
+        handleDisconnect(
+            ethPeer.getConnection(), DisconnectReason.SUBPROTOCOL_TRIGGERED_POW_BLOCKS, false);
         return;
       }
     }
@@ -441,30 +442,31 @@ private void handleStatusMessage(final EthPeer peer, final Message message) {
             .addArgument(status::networkId)
             .addArgument(() -> getPeerOrPeerId(peer))
             .log();
-        peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED);
+        peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED_MISMATCHED_NETWORK);
       } else if (!forkIdManager.peerCheck(forkId) && status.protocolVersion() > 63) {
         LOG.atDebug()
             .setMessage("{} has matching network id ({}), but non-matching fork id: {}")
             .addArgument(() -> getPeerOrPeerId(peer))
             .addArgument(networkId::toString)
             .addArgument(forkId)
             .log();
-        peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED);
+        peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED_MISMATCHED_FORKID);
       } else if (forkIdManager.peerCheck(status.genesisHash())) {
         LOG.atDebug()
             .setMessage("{} has matching network id ({}), but non-matching genesis hash: {}")
             .addArgument(() -> getPeerOrPeerId(peer))
             .addArgument(networkId::toString)
             .addArgument(status::genesisHash)
             .log();
-        peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED);
+        peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED_MISMATCHED_GENESIS_HASH);
       } else if (mergePeerFilter.isPresent()
           && mergePeerFilter.get().disconnectIfPoW(status, peer)) {
         LOG.atDebug()
             .setMessage("Post-merge disconnect: peer still PoW {}")
             .addArgument(() -> getPeerOrPeerId(peer))
             .log();
-        handleDisconnect(peer.getConnection(), DisconnectReason.SUBPROTOCOL_TRIGGERED, false);
+        handleDisconnect(
+            peer.getConnection(), DisconnectReason.SUBPROTOCOL_TRIGGERED_POW_DIFFICULTY, false);
       } else {
         LOG.atDebug()
             .setMessage("Received status message from {}: {} with connection {}")
@@ -486,7 +488,7 @@ private void handleStatusMessage(final EthPeer peer, final Message message) {
           .log();
       // Parsing errors can happen when clients broadcast network ids outside the int range,
       // So just disconnect with "subprotocol" error rather than "breach of protocol".
-      peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED);
+      peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED_UNPARSABLE_STATUS);
     }
   }
 

diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/MergePeerFilter.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/MergePeerFilter.java
@@ -47,7 +47,7 @@ public boolean disconnectIfPoW(final StatusMessage status, final EthPeer peer) {
         LOG.debug(
             "Disconnecting peer with difficulty {}, likely still on PoW chain",
             status.totalDifficulty());
-        peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED);
+        peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED_POW_DIFFICULTY);
         return true;
       } else {
         return false;
@@ -61,7 +61,7 @@ public boolean disconnectIfGossipingBlocks(final Message message, final EthPeer
     final int code = message.getData().getCode();
     if (isFinalized() && (code == EthPV62.NEW_BLOCK || code == EthPV62.NEW_BLOCK_HASHES)) {
       LOG.debug("disconnecting peer for sending new blocks after transition to PoS");
-      peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED);
+      peer.disconnect(DisconnectReason.SUBPROTOCOL_TRIGGERED_POW_BLOCKS);
       return true;
     } else {
       return false;

diff --git a/...m/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManagerTest.java b/...m/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/EthProtocolManagerTest.java
@@ -257,8 +257,7 @@ public void disconnectNewPoWPeers() {
       assertThat(workPeer.isDisconnected()).isTrue();
       assertThat(workPeer.getDisconnectReason()).isPresent();
       assertThat(workPeer.getDisconnectReason())
-          .get()
-          .isEqualTo(DisconnectReason.SUBPROTOCOL_TRIGGERED);
+          .hasValue(DisconnectReason.SUBPROTOCOL_TRIGGERED_POW_DIFFICULTY);
       assertThat(stakePeer.isDisconnected()).isFalse();
     }
   }

diff --git a/...src/main/java/org/hyperledger/besu/ethereum/p2p/rlpx/wire/messages/DisconnectMessage.java b/...src/main/java/org/hyperledger/besu/ethereum/p2p/rlpx/wire/messages/DisconnectMessage.java
@@ -118,10 +118,17 @@ public enum DisconnectReason {
     UNEXPECTED_ID((byte) 0x09),
     LOCAL_IDENTITY((byte) 0x0a),
     TIMEOUT((byte) 0x0b),
-    SUBPROTOCOL_TRIGGERED((byte) 0x10);
+    SUBPROTOCOL_TRIGGERED((byte) 0x10),
+    SUBPROTOCOL_TRIGGERED_MISMATCHED_NETWORK((byte) 0x10, "Mismatched network id"),
+    SUBPROTOCOL_TRIGGERED_MISMATCHED_FORKID((byte) 0x10, "Mismatched fork id"),
+    SUBPROTOCOL_TRIGGERED_MISMATCHED_GENESIS_HASH((byte) 0x10, "Mismatched genesis hash"),
+    SUBPROTOCOL_TRIGGERED_UNPARSABLE_STATUS((byte) 0x10, "Unparsable status message"),
+    SUBPROTOCOL_TRIGGERED_POW_DIFFICULTY((byte) 0x10, "Peer has difficulty greater than POS TTD"),
+    SUBPROTOCOL_TRIGGERED_POW_BLOCKS((byte) 0x10, "Peer sent blocks after POS transition");
 
     private static final DisconnectReason[] BY_ID;
     private final Optional<Byte> code;
+    private final Optional<String> message;
 
     static {
       final int maxValue =
@@ -132,7 +139,7 @@ public enum DisconnectReason {
               .getAsInt();
       BY_ID = new DisconnectReason[maxValue + 1];
       Stream.of(DisconnectReason.values())
-          .filter(r -> r.code.isPresent())
+          .filter(r -> r.code.isPresent() && r.message.isEmpty())
           .forEach(r -> BY_ID[r.code.get()] = r);
     }
 
@@ -146,15 +153,25 @@ public static DisconnectReason forCode(final Byte code) {
 
     DisconnectReason(final Byte code) {
       this.code = Optional.ofNullable(code);
+      this.message = Optional.empty();
+    }
+
+    DisconnectReason(final Byte code, final String message) {
+      this.code = Optional.ofNullable(code);
+      this.message = Optional.of(message);
     }
 
     public Bytes getValue() {
       return code.map(Bytes::of).orElse(Bytes.EMPTY);
     }
 
+    public String getMessage() {
+      return message.orElse("");
+    }
+
     @Override
     public String toString() {
-      return getValue().toString() + " " + name();
+      return getValue().toString() + " " + name() + " " + getMessage();
     }
   }
 }
diff --git a/...test/java/org/hyperledger/besu/ethereum/p2p/rlpx/wire/messages/DisconnectMessageTest.java b/...test/java/org/hyperledger/besu/ethereum/p2p/rlpx/wire/messages/DisconnectMessageTest.java
@@ -89,4 +89,14 @@ public void createWithUnknownReason() {
     assertThat(disconnectMessage.getReason()).isEqualTo(DisconnectReason.UNKNOWN);
     assertThat(disconnectMessage.getData().toString()).isEqualToIgnoringCase("0xC180");
   }
+
+  @Test
+  public void readFromWithSubprotocolTriggeredUsesGenericReason() {
+    MessageData messageData =
+        new RawMessage(WireMessageCodes.DISCONNECT, Bytes.fromHexString("0xC110"));
+    DisconnectMessage disconnectMessage = DisconnectMessage.readFrom(messageData);
+
+    DisconnectReason reason = disconnectMessage.getReason();
+    assertThat(reason).isEqualTo(DisconnectReason.SUBPROTOCOL_TRIGGERED);
+  }
 }