-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: add support for detecting and logging per address reachability via libp2p AutoNAT v2 #16100
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
base: develop
Are you sure you want to change the base?
Conversation
|
Is it ready for review? If not, could you convert it to draft? |
|
@nalepae Just waiting on a green CI. Will add you as a reviewer once green. |
|
@nalepae Will add Hoodi test instructions and then move out of Draft status. |
bazel run //cmd/beacon-chain:beacon-chain --config=release -- \
--hoodi \
--execution-endpoint=http://localhost:8551 \
--jwt-secret={jwt_file}
--checkpoint-sync-url=https://hoodi.beaconstate.ethstaker.cc/ \
--genesis-beacon-api-url=https://hoodi.beaconstate.ethstaker.cc/ \
--verbosity=info \
--accept-terms-of-use \
--datadir=beacon-hoodi-data \
--log-file=beacon-hoodi.log \
--enable-autonat
The log line to print out reachability will only be emitted if your node has public addresses configured AND all peers in the network upgrade to the AutoNATV2 protocol. (which will happen after they run this change). I've also added a unit test that verifies that the logs are outputted correctly once the corresponding libp2p event fires. |
|
Hey @aarshkshah1992, you beat me to it! That was fast. diff --git a/beacon-chain/p2p/monitoring.go b/beacon-chain/p2p/monitoring.go
index b454d2587d..37fa5ac9d6 100644
--- a/beacon-chain/p2p/monitoring.go
+++ b/beacon-chain/p2p/monitoring.go
@@ -185,6 +185,16 @@ var (
Help: "The number of publish messages sent via rpc for a particular topic",
},
[]string{"topic"})
+
+ // AutoNAT metrics
+ autonatReachableAddrs = promauto.NewGauge(prometheus.GaugeOpts{
+ Name: "p2p_autonat_reachable_addresses",
+ Help: "Number of addresses confirmed reachable via AutoNAT.",
+ })
+ autonatUnreachableAddrs = promauto.NewGauge(prometheus.GaugeOpts{
+ Name: "p2p_autonat_unreachable_addresses",
+ Help: "Number of addresses confirmed unreachable via AutoNAT.",
+ })
)
func (s *Service) updateMetrics() {
diff --git a/beacon-chain/p2p/service.go b/beacon-chain/p2p/service.go
index b9a4242c15..27270e08da 100644
--- a/beacon-chain/p2p/service.go
+++ b/beacon-chain/p2p/service.go
@@ -587,6 +587,10 @@ func (s *Service) subscribeReachabilityEvents() {
"unreachable": multiaddrsToStrings(event.Unreachable),
"unknown": multiaddrsToStrings(event.Unknown),
}).Info("Address reachability changed")
+
+ // Update Prometheus metrics
+ autonatReachableAddrs.Set(float64(len(event.Reachable)))
+ autonatUnreachableAddrs.Set(float64(len(event.Unreachable)))
}
}
} |
beacon-chain/p2p/service.go
Outdated
| log.WithFields(logrus.Fields{"peerID": peerID, "reason": reason, "newScore": newScore}).Debug("Downscore peer") | ||
| } | ||
|
|
||
| func (s *Service) subscribeReachabilityEvents() { |
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.
Please return an error an let the caller decide what to do instead of logging an error.
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.
Done
beacon-chain/p2p/service.go
Outdated
| case <-s.ctx.Done(): | ||
| return | ||
| case ev := <-sub.Out(): | ||
|
|
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.
Why an empty line here?
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.
Done
|
What does reachable, unknown and unreachable mean? Then later: |
|
Now the node knows its public IP address, could it use it directly to advertise it on its own ENR instead of having to manually add the |
|
@willianpaixao I am not sure these metrics will be super helpful. Like you would want to know which addresses are reachable/unreachable but what exactly will you do with the count of those ? The logs should be good enough for your use case but let me know if you have any use for these metrics in mind. |
"reachable" means that enough peers running AutoNATV2 "servers" have dialed that address and confirmed that the dial went through. unreachable means that enough peers running AutoNATV2 "servers" have dialed that address and confirmed that the dial failed. "unknown" means that we haven't recieved enough confirmations in either direction yet. |
@nalepae This is done. We should still keep the "--p2p-host-ip=" flag but fallback to AutoNATV2 reachability if it is not configured. |
beacon-chain/p2p/service.go
Outdated
| }).Info("Address reachability changed") | ||
|
|
||
| // Update ENR with reachable addresses if ENR has no addresses configured | ||
| if len(event.Reachable) > 0 { |
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.
Can you explain how did you test that?
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.
@nalepae Please see "TestService_SubscribeReachabilityEvents"
ce13bce to
0865dab
Compare
|
@nalepae Are you happy to approve and merge this ? I've reverted the ENR update change. |
What type of PR is this?
Feature
What does this PR do? Why is it needed?
This PR adds support for detecting and logging per address reachability via libp2p AutoNAT v2. See https://github.com/libp2p/go-libp2p/releases/tag/v0.42.0 for details. This PR also upgrades Prysm to libp2p v0.42.0
Which issues(s) does this PR fix?
Fixes ##16098
Other notes for review
Acknowledgements