fix(rpc/admin): missing enode/enr in admin_peers endpoint

[jsvisa]

currently, we're returning the peer information in the net/session module, which misses the peer's discovery port, so can't recover the enode info, and in this PR I combine the peer-info both of the session and underearth socket info.

[mattsse]

not immediately obvious to me what the actual issue is here.

could you please elaborate?
is the issue here that admin_peers endpoint does not propagate the udp port?

because I think we can solve this without being super invasive wrt the Noderecord type

[jsvisa]

is the issue here that admin_peers endpoint does not propagate the udp port?

Yes, without the UDP port, we can't encapsulate the full enode endpoint. And the fix seems do too much work, I'll try to find a more sufficient way.

[mattsse]

the full enode endpoint

which one is this?

[jsvisa]

the full enode endpoint

which one is this?

sorry, I didn't explain it clearly. If the UDP port differs to TCP port, then we needd to append ?discport={UDPPort}, but currently it's not available to fetch the udp port

[mattsse]

so this affects the PeerInfo.id field?

I assume the more appropriate fix here would be to also support AddPeer variant that accepts NodeRecord

[jsvisa]

@mattsse sorry for the delay response,

so this affects the PeerInfo.id field?
sorry, I don't get the point why the id is affected, btw currently the response of id in admin_peers and admin_nodeInfo are incorrect, we should return the hash of pubkey, ref https://github.com/ethereum/go-ethereum/blob/7cf6a63687da2d9300fec495ff4bc978c4b70b65/p2p/enode/urlv4.go#L198-L203

I assume the more appropriate fix here would be to also support AddPeer variant that accepts NodeRecord

agree with it, that will make the code more clear. btw, just curious why we store the SocketAddress in Peers

reth/crates/net/network/src/peers/manager.rs

Lines 1017 to 1034 in b36bd58

	pub struct Peer {
	/// Where to reach the peer
	addr: SocketAddr,
	/// Reputation of the peer.
	reputation: i32,
	/// The state of the connection, if any.
	state: PeerConnectionState,
	/// The [`ForkId`] that the peer announced via discovery.
	fork_id: Option<ForkId>,
	/// Whether the entry should be removed after an existing session was terminated.
	remove_after_disconnect: bool,
	/// The kind of peer
	kind: PeerKind,
	/// Whether the peer is currently backed off.
	backed_off: bool,
	/// Counts number of times the peer was backed off due to a severe [`BackoffKind`].
	severe_backoff_counter: u8,
	}

It seems the socket address is only used to return, not used inner. I think we can store IpAddr + Port instead, because sizeof(SocketAddr) is 32, but sizeof(IpAddr) + 2*ports is 21(if need tcp+udp ports)

[mattsse]

sorry for the delay response,

all good, thank you for taking this on 🙏

t seems the socket address is only used to return, not used inner. I think we can store IpAddr + Port instead, because sizeof(SocketAddr) is 32, but sizeof(IpAddr) + 2*ports is 21(if need tcp+udp ports)

yeah good point, we will have a bunch of these entries so if we can minimize, we def should

[mattsse]

just a heads up, we're about to merge a big rpc pr that will introduce a ton of conflicts here.

could we first do the net changes in one pr and then do the rpc changes?
I'm fine with many small incremental prs

[jsvisa]

just a heads up, we're about to merge a big rpc pr that will introduce a ton of conflicts here.

could we first do the net changes in one pr and then do the rpc changes?

I'm fine with many small incremental prs

Thanks for the suggestion, I'll split them into smaller ones

[jsvisa]

waiting for #9201 to be merged first

[mattsse]

some nits

[mattsse]

this returns the number of peers in the peer set wich is larger than connected peers. this should allocated based on active sessions

[jsvisa]

But we only return the peer who have peer record, so I think the num_known_peers is sufficient

reth/crates/net/network/src/manager.rs

Lines 873 to 875 in ad96373

	if let Some((record, kind)) = peer_manager.peer_by_id(*peer_id) {
	peers.push(session.peer_info(&record, kind));
	}

[mattsse]

this will allocate more capacity then necessary because known_peers > active

you can use

        self.swarm
            .sessions()
            .active_sessions()
            .into_iter()
            .filter_map(|(peer_id, session)| {
                let (record, kind) = self.swarm.state().peers().peer_by_id(*peer_id)?;
                Some(session.peer_info(&record, kind))
            })
            .collect()

[jsvisa]

thanks for the advice, I previously thought of know_peers < active, don't expect know_peers > active, but in which case is known_peers > active, the connection is established, but the session not starting?

[jsvisa]

@mattsse one last thing, I can't find an available way to calculate the enr from enode, maybe missing some information, so the enr fields are not calculated in this pr, I'll file another pr after finding a suitable way, or do you have any suggestions.

reth/crates/net/network/src/session/handle.rs

Line 144 in ad96373

enr: None,

[mattsse]

last nit and one q

[mattsse]

this will allocate more capacity then necessary because known_peers > active

you can use

        self.swarm
            .sessions()
            .active_sessions()
            .into_iter()
            .filter_map(|(peer_id, session)| {
                let (record, kind) = self.swarm.state().peers().peer_by_id(*peer_id)?;
                Some(session.peer_info(&record, kind))
            })
            .collect()

[mattsse]

tysm!