Speed up ENR update #196
Speed up ENR update #196
Conversation
ackintosh
requested review from
divagant-martian and
AgeManning
and removed request for
divagant-martian
src/service.rs
Outdated
| self.connection_updated(node_id, ConnectionStatus::Connected(enr.clone(), direction)); | ||
|
|
||
| // PING immediately if the direction is outgoing. This allows us to receive a PONG without | ||
| // waiting for the ping_interval, making ENR updates faster. | ||
| if matches!(direction, ConnectionDirection::Outgoing) { | ||
| self.send_ping(enr, None); | ||
| } |
There was a problem hiding this comment.
looking to reduce the clone I suggest moving this to the connection_updated fn. This way the clone is only done when the direction is outgoing instead of every time ackintosh#2
There was a problem hiding this comment.
I have merged your suggestion, thank you!
…rnative suggested alternative that reduces cloning
|
Yeah nice. I think we should move this statement into the match just below it tho, so that we only ping peers that manage to make it into the routing table. The reason being, is I'm a little worried that if we do a FINDNODE to a malicious peer, that can direct to us to connect (in an outgoing direction) to a bunch of other malicious peers that want us to edit our IP address. If we limit this PING approach to only peers that make it into our routing table we get the added security measures that are applied there, like bucket and table limits which would help mitigate this scenario. |
to get the added security measures, like bucket and table limits
|
@ackintosh can you test this works as expected in testground? I see no reason to believe it won't but let's be sure. I'll merge after your confirmation 🎉 |
|
Yeah, I was just testing this again. I have tested the latest change with Testground and Lighthouse, and confirmed that this works as expected. 🙆♂️ |
Description
closes #144.
By this PR, we send PING immediately when the session is established if the direction is
Outgoing.Notes & open questions
I have tested this PR with the enr-update test plan. You can run the test plan locally.
ackintosh/discv5-testground#109
Also I have tested with lighthouse (current unstable
Lighthouse/v4.2.0-cc780aa). The ENR update had happened within 113sec avg from start-up.Change checklist