feat: update utp-rs dependency #1652
Conversation
crates/portalnet/src/discovery.rs
Outdated
|
|
||
| impl UtpEnr { | ||
| impl EnrPeer { |
There was a problem hiding this comment.
Is this struct only going to be used with uTP or will it be used elsewhere, as we should be specific if the usecase is specific, if UtpEnr is going to start being used more broadly I think it is fine to rename it. EnrPeer is just extremely broad, so if it is still exclusive for utp maybe keeping the old name makes sense as it is more to the point.
There was a problem hiding this comment.
For me, UtpEnr sounds like a special/different version of Enr that is used for utp. But I understand your comment as well.
So I settled for UtpPeer. I think it's the cleanest.
Let me know if you don't like it, as I don't care that much and I'm willing to revert it back to UtpEnr.
crates/portalnet/src/discovery.rs
Outdated
| let peer = peer.clone(); | ||
| let discv5 = Arc::clone(&self.discv5); | ||
| let target = target.0.clone(); | ||
| let enr_cache = Arc::clone(&self.enr_cache); | ||
| let header_oracle = Arc::clone(&self.header_oracle); | ||
| let data = buf.to_vec(); | ||
| tokio::spawn(async move { | ||
| match discv5.send_talk_req(target, Subnetwork::Utp, data).await { | ||
| let enr = match peer.peer() { | ||
| Some(enr) => enr.0.clone(), |
There was a problem hiding this comment.
we are already cloning the Enr above, shouldn't we be able to avoid cloning it twice?
looks good
What was wrong?
It's possible for wrong Enr to be passed to utp library if it was changed recently.
See #1596 for more details.
How was it fixed?
The utp-rs library is updated (see ethereum/utp#136) so only peer id (NodeId) is required when receiving the utp packet, while entire peer (Enr) is passed when calling
accept_with_cid.The biggest change is in
crates/portalnet/src/discovery.rs, where we now callfind_enronly if Enr is unknown when we have to send them packet (which should rarely happen), instead of every time we receive packet.Worth highlighting that due to the nature of #1596, we can't easily verify that this actually fixes it until it's deployed to the servers. I did some manual testing and I'm confident that this should fix it (but I plan to verify once this is merged and deployed).
To-Do