feat!: distinguish peer and peer_id on api level #136

morph-dev · 2025-01-28T20:59:37Z

This PR allows ConnectionPeer to expose PeerId type and merge function.

This allows utp library to not require Peer object itself, but only PeerId object.

If multiple peer objects are provided for the same connection, they will be merged together and passed during AsyncUdpSocket::send_to.

The ethereum/trin#1652 exposes how this works within trin client.

Note: I updated circleci rust version to 1.81.0 because old version was complaining about "Associated type bounds" syntax P: ConnectionPeer<Id: Unpin> + Unpin + 'static

KolbyML

PR looks good, I really don't like the name merge() as I find it very confusing, but it isn't a blocker

KolbyML · 2025-01-28T21:20:15Z

src/peer.rs

+    /// Merge the given object into `Self` whilst consuming it.
+    ///
+    /// Should panic if ids are not matching.
+    fn merge(&mut self, other: Self);


This is kinda of confusing, merge normally means to combine 2 things, but this is more liking updating the internal copy to the latest copy seen. So when I see merge being used, I am confused, also the doc confuses me. Forcing me to read read the code and how it is used.

Why not use like update_peer() or something make its action a bit more described.

This isn't really a blocker, I am naming this function merge just really confuses me

I agree that merge is not the best name. But update_peer also is not. Let me explain.

Since this is part of the trait, each implementation can choose what it means for that one.
Some (e.g. SocketAddr) will choose to do nothing. Others (e.g. Enr that trin uses) will pick whichever is the latest (i.e. using Enr's seq field).

So while merge isn't the best and it's confusing, I would say that update_peer is also not correct (as with Enr case, it's not always updating).

Happy to consider any other name and to add clarification to the documentation (but I would avoid update).

Then maybe compare_and_swap makes more sense https://doc.rust-lang.org/std/sync/atomic/struct.AtomicU16.html#method.compare_and_swap

This would fit both cases no? as for the SockerAddr we are comparing but since they are already the same we aren't swapping

for the Enr case we are literally doing a compare_and_swap operation

I would argue both SockerAddr and the Enr cases are generally following the compare_and_swap operation nomenclature and hence would be a better name then merge

Discussed offline. I ended up using "consolidate" and provided more detailed explanation.

I also slightly changed the signature. but only for ConnectionPeer, not for the Peer. If you feel strongly about both having the same signature, let me know (in which case, I would probably use signature from ConnectionPeer).

KolbyML

looks good

carver

LGTM

I like the name consolidate().

Just added a couple thoughts, nothing blocking or opinionated.

carver · 2025-02-04T07:28:32Z

src/peer.rs

+    /// Consolidates two peers into one.
+    ///
+    /// It's possible that we have two instances that represent the same peer (equal `peer_id`),
+    /// and we need to consolidate them into one. This can happen when [Peer]-s passed with


Silly question: what does this syntax [Peer]-s mean in the docstring?

I'm not sure if you are unsure about [Peer] or the -s part. First one is link to the struct definition (see more here) and -s one is my attempt on making a plural of a term that is linkified.

carver · 2025-02-04T07:39:47Z

src/socket.rs

                        let packet = match Packet::decode(&buf[..n]) {
                            Ok(pkt) => pkt,
                            Err(..) => {
-                                tracing::warn!(?src, "unable to decode uTP packet");
+                                tracing::warn!(?peer_id, "unable to decode uTP packet");


If it feels right to you, we could log out the full peer instead of just the ID here, now that we have it.

carver · 2025-02-04T07:43:39Z

src/socket.rs

@@ -107,31 +115,34 @@ where
                            }
                            None => {
                                if std::matches!(packet.packet_type(), PacketType::Syn) {
-                                    let cid = cid_from_packet(&packet, &src, IdType::RecvId);
+                                    let cid = cid_from_packet::<P>(&packet, peer_id, IdType::RecvId);


I know it was like this already, but does it make any sense to you that we re-extract the cid here? It seems like 12 lines up, we make the same function call, on the same packet, with the same peer_id.

Is it just because it's borrowed in the conns.get()? If so, maybe we should just let cid = acc_cid.clone().

I noticed the same thing, but I didn't understand why it was done like that in the first place. So I just left it as it was.

But since you also noticed and commented on it, I reused acc_cid.

carver · 2025-02-04T08:02:27Z

src/conn.rs

@@ -232,15 +237,15 @@ impl<const N: usize, P: ConnectionPeer> Connection<N, P> {
        mut writes: mpsc::UnboundedReceiver<Write>,
        mut shutdown: oneshot::Receiver<()>,
    ) -> io::Result<()> {
-        tracing::debug!("uTP conn starting... {:?}", self.cid.peer);
+        tracing::debug!("uTP conn starting... {:?}", self.cid.peer_id);


Is it still valuable to see the ConnectionPeer embedded in the Peer here? I imagine it could be, sometimes

Suggested change

tracing::debug!("uTP conn starting... {:?}", self.cid.peer_id);

tracing::debug!("uTP conn starting... {:?}", self.peer);

morph-dev requested review from carver and KolbyML January 28, 2025 20:59

morph-dev self-assigned this Jan 28, 2025

morph-dev mentioned this pull request Jan 28, 2025

feat: update utp-rs dependency ethereum/trin#1652

Merged

1 task

KolbyML previously approved these changes Jan 28, 2025

View reviewed changes

morph-dev added 3 commits February 3, 2025 18:29

feat!: distinguish peer and peer_id on api level

85af306

build: update rust version to 1.81.0

b810965

refactor: rename "merge" to "consolidate" and update documentation

da5ae3e

morph-dev dismissed KolbyML’s stale review via da5ae3e February 3, 2025 18:11

morph-dev force-pushed the peer_id branch from 16087be to da5ae3e Compare February 3, 2025 18:11

morph-dev requested a review from KolbyML February 3, 2025 18:15

KolbyML previously approved these changes Feb 3, 2025

View reviewed changes

carver previously approved these changes Feb 4, 2025

View reviewed changes

fix: pr comments

d50d831

morph-dev dismissed stale reviews from carver and KolbyML via d50d831 February 4, 2025 08:53

morph-dev merged commit a4da863 into ethereum:master Feb 4, 2025
2 checks passed

morph-dev deleted the peer_id branch February 4, 2025 09:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: distinguish peer and peer_id on api level #136

feat!: distinguish peer and peer_id on api level #136

morph-dev commented Jan 28, 2025 •

edited

Loading

KolbyML left a comment

KolbyML Jan 28, 2025

morph-dev Jan 28, 2025

KolbyML Jan 28, 2025 •

edited

Loading

morph-dev Feb 3, 2025

KolbyML left a comment

carver left a comment

carver Feb 4, 2025

morph-dev Feb 4, 2025

carver Feb 4, 2025

carver Feb 4, 2025

morph-dev Feb 4, 2025

carver Feb 4, 2025

	tracing::debug!("uTP conn starting... {:?}", self.cid.peer_id);
	tracing::debug!("uTP conn starting... {:?}", self.peer);

feat!: distinguish peer and peer_id on api level #136

feat!: distinguish peer and peer_id on api level #136

Conversation

morph-dev commented Jan 28, 2025 • edited Loading

KolbyML left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML left a comment

Choose a reason for hiding this comment

carver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morph-dev commented Jan 28, 2025 •

edited

Loading

KolbyML Jan 28, 2025 •

edited

Loading