Repository navigation
CL transport: QUIC first (the spec's order), QUIC-only peers dialable #731
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
Merged
Merged
Changes from all commits
Commits
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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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
30 changes: 30 additions & 0 deletions
30
node-core/src/test/java/io/myotis/node/TcpDialableClPeersTest.java
This file contains hidden or 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 |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| package io.myotis.node; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
|
||
| import java.util.List; | ||
| import java.util.Map; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| /** | ||
| * The shared CL peer cache can hold a QUIC-only peer the Rust engine found; | ||
| * the Java engine (TCP-only) must skip it rather than dial it and charge it | ||
| * failures that would evict it from the file both engines read. | ||
| */ | ||
| class TcpDialableClPeersTest { | ||
|
|
||
| @Test | ||
| void quicOnlyEntriesAreSkippedAndTcpOnesKeptInOrder() { | ||
| String tcp = "/ip4/10.0.0.1/tcp/9000/p2p/16Uiu2HAkyDsNGDq5pbFCqdKTcJxp4Rd5caoy1Xe2KJVtyc94M8S5"; | ||
| String quic = "/ip4/10.0.0.2/udp/9001/quic-v1/p2p/16Uiu2HAkyDsNGDq5pbFCqdKTcJxp4Rd5caoy1Xe2KJVtyc94M8S5"; | ||
| String dns = "/dns4/roost.example/tcp/9105/p2p/16Uiu2HAkyDsNGDq5pbFCqdKTcJxp4Rd5caoy1Xe2KJVtyc94M8S5"; | ||
| assertEquals(List.of(tcp, dns), ChainStack.tcpDialable(List.of(tcp, quic, dns))); | ||
| } | ||
|
|
||
| @Test | ||
| void quicOnlyKeysAreDroppedFromTheProvenRecords() { | ||
| String tcp = "/ip4/10.0.0.1/tcp/9000/p2p/16Uiu2HAkyDsNGDq5pbFCqdKTcJxp4Rd5caoy1Xe2KJVtyc94M8S5"; | ||
| String quic = "/ip4/10.0.0.2/udp/9001/quic-v1/p2p/16Uiu2HAkyDsNGDq5pbFCqdKTcJxp4Rd5caoy1Xe2KJVtyc94M8S5"; | ||
| assertEquals(Map.of(tcp, 1500L), ChainStack.tcpDialable(Map.of(tcp, 1500L, quic, 1600L))); | ||
| } | ||
| } |
Oops, something went wrong.
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.
Observation (pre-existing, adjacent — follow-up material, not a blocker): this fixes the read side, but the Java engine's write side can still mint dead TCP keys for the same peers. The discv5 callback above (line 1067) caches
Enr.toLibp2pMultiaddr(), which for a record with notcpfield falls back to the discv5 UDP port rendered as/tcp/<udp-port>(Enr.java:227-239, "CL convention: same port for both"). For a genuinely QUIC-only peer that fabricated address is dead: this engine dials it and charges it failures, and the Rust engine replays it too (as a distinct key beside its own/udp/<port>/quic-v1entry, first-in-file wins the pool identity) — the same eviction noise this PR removes on the read path, just under a fabricated key.The fallback predates this PR and was a reasonable heuristic while every client listened on TCP; now that QUIC-only records are an expected production shape (the premise of this PR), an ENR carrying a
quicfield but notcpfield is positive evidence the fallback is wrong for that record. Worth a follow-up: skip the UDP fallback when the record advertises QUIC without TCP.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.
Accepted: fixed in this PR (56ad76d) rather than as a follow-up. I checked
Enr.toLibp2pMultiaddr(core/.../enr/Enr.java): with notcpfield it falls back toudpAddress()and renders it as/tcp/<udp-port>, exactly as you describe. That fabricated key reaches both the discv5 callback (ChainStackline 1066,clPeerCache.add) and the DNS-tree seeding (buildAndStartBeacon). The fallback is now skipped when the record carries aquicfield: such a record yields no multiaddr. A record withudpand neithertcpnorquickeeps the old same-port fallback.EnrTest.aQuicOnlyRecordYieldsNoFabricatedTcpAddresspins both. It uses hand-built records withENR_STRING's key and a zero signature, sincedecodedoesn't verify signatures.:corecompiles locally, but its tests couldn't run in my sandbox (Maven Central returned 429), so CI has to confirm them.Generated by Claude Code