-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Defer Snappy decompression of P2P messages until worker thread processing #10048
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
Changes from 13 commits
21a3ffd
a496219
b0b7cf4
6750ea8
19e8a1e
3f2e28e
09827a1
bf5fd2a
8bc3321
301ad6d
df28b61
4ca652e
0797ffe
ca61602
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ | |
| import org.hyperledger.besu.ethereum.p2p.network.ProtocolManager; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection.PeerNotConnected; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.framing.FramingException; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.wire.Capability; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.wire.Message; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; | ||
|
|
@@ -298,12 +299,12 @@ public void processMessage(final Capability capability, final Message message) { | |
| return; | ||
| } | ||
|
|
||
| // This will handle responses | ||
| ethPeers.dispatchMessage(ethPeer, ethMessage, getSupportedProtocol()); | ||
|
|
||
| // This will handle requests | ||
| Optional<MessageData> maybeResponseData = Optional.empty(); | ||
| try { | ||
| // This will handle responses | ||
| ethPeers.dispatchMessage(ethPeer, ethMessage, getSupportedProtocol()); | ||
|
|
||
| // This will handle requests | ||
| if (EthProtocol.requestIdCompatible(code)) { | ||
| final Map.Entry<BigInteger, MessageData> requestIdAndEthMessage = | ||
| ethMessage.getData().unwrapMessageData(); | ||
|
|
@@ -314,6 +315,17 @@ public void processMessage(final Capability capability, final Message message) { | |
| } else { | ||
| maybeResponseData = ethMessages.dispatch(ethMessage, capability); | ||
| } | ||
| } catch (final FramingException e) { | ||
| LOG.atDebug() | ||
| .setMessage( | ||
| "Failed to decompress message with code {} (BREACH_OF_PROTOCOL), disconnecting: {}, {}") | ||
| .addArgument(code) | ||
| .addArgument(ethPeer::toString) | ||
| .addArgument(e::toString) | ||
| .log(); | ||
|
|
||
| ethPeer.disconnect( | ||
|
Contributor
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. Same here, would be good to add a test for disconnecting the peer |
||
| DisconnectMessage.DisconnectReason.BREACH_OF_PROTOCOL_MALFORMED_MESSAGE_RECEIVED); | ||
| } catch (final RLPException e) { | ||
| LOG.atDebug() | ||
| .setMessage( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ | |
| import org.hyperledger.besu.ethereum.eth.sync.snapsync.SnapSyncConfiguration; | ||
| import org.hyperledger.besu.ethereum.p2p.network.ProtocolManager; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.connections.PeerConnection; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.framing.FramingException; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.wire.AbstractSnapMessageData; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.wire.Capability; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.wire.Message; | ||
|
|
@@ -113,18 +114,25 @@ public void processMessage(final Capability cap, final Message message) { | |
| return; | ||
| } | ||
|
|
||
| // This will handle responses | ||
| ethPeers.dispatchMessage(ethPeer, ethMessage, getSupportedProtocol()); | ||
|
|
||
| // This will handle requests | ||
| Optional<MessageData> maybeResponseData = Optional.empty(); | ||
| try { | ||
| // This will handle responses | ||
| ethPeers.dispatchMessage(ethPeer, ethMessage, getSupportedProtocol()); | ||
|
|
||
| // This will handle requests | ||
| final Map.Entry<BigInteger, MessageData> requestIdAndEthMessage = | ||
| ethMessage.getData().unwrapMessageData(); | ||
| maybeResponseData = | ||
| snapMessages | ||
| .dispatch(new EthMessage(ethPeer, requestIdAndEthMessage.getValue()), cap) | ||
| .map(responseData -> responseData.wrapMessageData(requestIdAndEthMessage.getKey())); | ||
| } catch (final FramingException e) { | ||
| LOG.debug( | ||
| "Failed to decompress message with code {} (BREACH_OF_PROTOCOL), disconnecting: {}", | ||
| code, | ||
| ethPeer, | ||
| e); | ||
| ethPeer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL_MALFORMED_MESSAGE_RECEIVED); | ||
|
Contributor
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. Same here, would be good to add a test for disconnecting the peer |
||
| } catch (final RLPException e) { | ||
| LOG.debug( | ||
| "Received malformed message code={} data={} (BREACH_OF_PROTOCOL), disconnecting: {}", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| import org.hyperledger.besu.ethereum.eth.manager.PendingPeerRequest; | ||
| import org.hyperledger.besu.ethereum.eth.manager.RequestManager; | ||
| import org.hyperledger.besu.ethereum.eth.manager.exceptions.PeerBreachedProtocolException; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.framing.FramingException; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.wire.messages.DisconnectMessage.DisconnectReason; | ||
| import org.hyperledger.besu.ethereum.rlp.RLPException; | ||
|
|
@@ -111,6 +112,14 @@ private void handleMessage( | |
| promise.complete(r); | ||
| peer.recordUsefulResponse(); | ||
| }); | ||
| } catch (final FramingException e) { | ||
| // Peer sent us data that failed to decompress - disconnect | ||
| LOG.debug( | ||
|
Contributor
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. It would be good to have same for the disconnect due to decompression debug messages for here and in |
||
| "Disconnecting with BREACH_OF_PROTOCOL due to decompression failure: {}", | ||
| peer.getLoggableId(), | ||
| e); | ||
| peer.disconnect(DisconnectReason.BREACH_OF_PROTOCOL_MALFORMED_MESSAGE_RECEIVED); | ||
|
Contributor
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. Can you add a test for this since it's new behaviour
Contributor
Author
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 is actually not new behaviour, because we disconnected a peer for that in the original code, only now we are doing the decompression later - that is the reason why we are catching it here now.
Contributor
Author
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. It is actually new behaviour for this class, so I will add a test for that! |
||
| promise.completeExceptionally(new PeerBreachedProtocolException()); | ||
| } catch (final RLPException e) { | ||
| // Peer sent us malformed data - disconnect | ||
| LOG.debug( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,12 +22,19 @@ | |
| import org.hyperledger.besu.ethereum.eth.manager.EthScheduler; | ||
| import org.hyperledger.besu.ethereum.eth.messages.NewPooledTransactionHashesMessage; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.wire.Capability; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData; | ||
| import org.hyperledger.besu.ethereum.p2p.rlpx.wire.messages.DisconnectMessage.DisconnectReason; | ||
|
|
||
| import java.time.Duration; | ||
| import java.time.Instant; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| class NewPooledTransactionHashesMessageHandler implements EthMessages.MessageCallback { | ||
| private static final Logger LOG = | ||
| LoggerFactory.getLogger(NewPooledTransactionHashesMessageHandler.class); | ||
|
|
||
| private final NewPooledTransactionHashesMessageProcessor transactionsMessageProcessor; | ||
| private final EthScheduler scheduler; | ||
|
|
@@ -47,13 +54,30 @@ public NewPooledTransactionHashesMessageHandler( | |
| public void exec(final EthMessage message) { | ||
| if (isEnabled.get()) { | ||
| final Capability capability = message.getPeer().getConnection().capability(EthProtocol.NAME); | ||
| final NewPooledTransactionHashesMessage transactionsMessage = | ||
| NewPooledTransactionHashesMessage.readFrom(message.getData(), capability); | ||
| final MessageData rawMessage = message.getData(); | ||
| final Instant startedAt = now(); | ||
| scheduler.scheduleTxWorkerTask( | ||
| () -> | ||
| transactionsMessageProcessor.processNewPooledTransactionHashesMessage( | ||
| message.getPeer(), transactionsMessage, startedAt, txMsgKeepAlive)); | ||
| () -> { | ||
| if (message.getPeer().isDisconnected()) { | ||
| return; | ||
| } | ||
| final NewPooledTransactionHashesMessage transactionsMessage; | ||
| try { | ||
| transactionsMessage = | ||
| NewPooledTransactionHashesMessage.readFrom(rawMessage, capability); | ||
| } catch (final Exception e) { | ||
| LOG.debug( | ||
| "Malformed pooled transaction hashes message received (BREACH_OF_PROTOCOL), disconnecting: {}", | ||
| message.getPeer(), | ||
| e); | ||
| message | ||
|
Contributor
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. Same here, would be good to add a test for disconnecting the peer |
||
| .getPeer() | ||
| .disconnect(DisconnectReason.BREACH_OF_PROTOCOL_MALFORMED_MESSAGE_RECEIVED); | ||
| return; | ||
| } | ||
| transactionsMessageProcessor.processNewPooledTransactionHashesMessage( | ||
| message.getPeer(), transactionsMessage, startedAt, txMsgKeepAlive); | ||
| }); | ||
|
pinges marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.