Enhanced IPv6 support (discovery) #8410
Conversation
StefanBratanov
force-pushed
the
ipv6_discovery
branch
from
2d0cc3d to
6dc55fe
Compare
networking/p2p/src/main/java/tech/pegasys/teku/networking/p2p/network/config/NetworkConfig.java
Outdated
Show resolved
Hide resolved
| @@ -36,8 +36,10 @@ public class NodeRecordConverter { | |||
|
|
|||
| public Optional<DiscoveryPeer> convertToDiscoveryPeer( | |||
| final NodeRecord nodeRecord, final SchemaDefinitions schemaDefinitions) { | |||
| // TODO: https://github.com/Consensys/teku/issues/8069 | |||
There was a problem hiding this comment.
Not clear why it's needed here
We get tcp6 in or here, isn't it not enough? We need list here too?
There was a problem hiding this comment.
to be fair, probably or is enough. It's just that in this case, if we are dual-stack, we would always try to connect to IPv4 when ideally we would prefer IPv6
There was a problem hiding this comment.
Will tackle in future PR
There was a problem hiding this comment.
So my message was in case if someone other will take it in future, I want it to be clear for anyone else, what do we want there ideally
| final boolean userExplicitlySetAdvertisedIpOrPort, final int advertisedTcpPort) { | ||
| if (userExplicitlySetAdvertisedIpOrPort) { | ||
| private NewAddressHandler maybeUpdateNodeRecord(final NetworkConfig p2pConfig) { | ||
| if (p2pConfig.hasUserExplicitlySetAdvertisedIps()) { |
There was a problem hiding this comment.
I don't understand this, shouldn't it be inversed?
There was a problem hiding this comment.
Same behaviour as before. If user explicitly sets advertised IPs, they are added in the ENR. Otherwise, NewAddressHandler is communicated between nodes who don't have explicit advertised ips, in that case, we want to have a handler configured in discovery
There was a problem hiding this comment.
Needed to look into discovery to understand. So it fires when user address is changed and when advertised is configured it means that we should prefer advertised over anything changed, clear now
PR Description
--Xp2p-udp-port-ipv6and--Xp2p-advertised-udp-port-ipv624.6.0Optional<List<String>>inDiscV5Service.getDiscoveryAddressThis PR doesn't break any current functionality and everything else is hidden.
Fixed Issue(s)
related to #8069
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog