fix enr request order #4179
Merged
fix enr request order #4179
Conversation
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
Signed-off-by: Karim TAAM <[email protected]>
Signed-off-by: Karim TAAM <[email protected]>
jflo
reviewed
Jul 27, 2022
.../main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java
Outdated
Show resolved
Hide resolved
jflo
approved these changes
Jul 27, 2022
mbaxter
reviewed
Jul 27, 2022
.../main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java
Show resolved
Hide resolved
mbaxter
reviewed
Jul 27, 2022
.../main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PeerDiscoveryController.java
Show resolved
Hide resolved
Signed-off-by: Karim TAAM <[email protected]>
mbaxter
approved these changes
Aug 1, 2022
| @@ -111,6 +111,8 @@ public class PeerDiscoveryController { | |||
| private final PeerTable peerTable; | |||
| private final Cache<Bytes, DiscoveryPeer> bondingPeers = | |||
| CacheBuilder.newBuilder().maximumSize(50).expireAfterWrite(10, TimeUnit.MINUTES).build(); | |||
| private final Cache<Bytes, Packet> cachedEnrRequests = | |||
| CacheBuilder.newBuilder().maximumSize(50).expireAfterWrite(30, SECONDS).build(); | |||
There was a problem hiding this comment.
The docs on CacheBuilder recommend migrating to Caffeine. This is probably something to look into across the codebase - maybe for a follow-up PR.
There was a problem hiding this comment.
Is 30 seconds overly generous? Maybe 10 seconds?
| @@ -314,9 +316,12 @@ public void onMessage(final Packet packet, final DiscoveryPeer sender) { | |||
| matchInteraction(packet) | |||
| .ifPresent( | |||
| interaction -> { | |||
| System.out.println("ici"); | |||
| @@ -1371,6 +1371,45 @@ public void shouldNotRespondToENRRequestForNonBondedPeer() { | |||
| .send(eq(peers.get(0)), matchPacketOfType(PacketType.ENR_REQUEST)); | |||
| } | |||
|
|
|||
| @Test | |||
| public void shouldNotRespondAndCacheENRRequestForBondingPeer() { | |||
There was a problem hiding this comment.
This test name is a bit confusing to me, what about:
Suggested change
| public void shouldNotRespondAndCacheENRRequestForBondingPeer() { | |
| public void shouldRespondToRecentENRRequestAfterBonding() { |
| final Packet pongPacket = | ||
| Packet.create(PacketType.PONG, pongRequestPacketData, nodeKeys.get(0)); | ||
|
|
||
| controller.onMessage(enrPacket, peers.get(0)); |
There was a problem hiding this comment.
I'd verify here that we did not respond to the enr request directly with something like:
verify(outboundMessageHandler, never())
.send(any(), matchPacketOfType(PacketType.ENR_RESPONSE));
| @@ -1371,6 +1371,45 @@ public void shouldNotRespondToENRRequestForNonBondedPeer() { | |||
| .send(eq(peers.get(0)), matchPacketOfType(PacketType.ENR_REQUEST)); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
(optional) Might be nice to add another test that checks that we don't respond to an ENR request received way before a PONG message. Could do this by injecting a CacheBuilder with a customer ticker that can be advanced from the test.
pinges
approved these changes
Aug 2, 2022
| } else if (PeerDiscoveryStatus.BONDING.equals(peer.getStatus())) { | ||
| LOG.trace("ENR_REQUEST cached for bonding peer Id: {}", peer.getId()); | ||
| // it may happen that we receive the ENR_REQUEST just before the PONG. | ||
| // Because peers want to send the ENR_REQUEST directly after the pong. |
There was a problem hiding this comment.
Do we want to mention that this can happen because discovery is using UDP?
Signed-off-by: Justin Florentine <[email protected]>
…omplete ping/pong handshakes Signed-off-by: Justin Florentine <[email protected]>
jflo
approved these changes
Aug 2, 2022
eum602
pushed a commit
to lacchain/besu
that referenced
this pull request
Nov 3, 2023
* fix enr request * add tests * removed errant debug * adds pr feedback and test to ensure ENR responses never go out to incomplete ping/pong handshakes Signed-off-by: Karim TAAM <[email protected]> Co-authored-by: Justin Florentine <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Signed-off-by: Karim TAAM [email protected]
PR description
After a Geth<>Besu peerring test we found that sometimes we had "ENR request failed id=68f8eb309602c1a6 addr=3.16.90.60:30303 err=“RPC timeout”
Besu does not respond to the ENR request. By investigating we found that sometimes ENR_REQUEST comes before the PONG and so Besu ignores the request.
This PR allows to cache the request if it arrives before the PONG to answer once the bond is done
Fixed Issue(s)
Documentation
doc-change-requiredlabel to this PR ifupdates are required.
Changelog