p2p/discover: pass node instead of node ID to TALKREQ handler

[thinkAfCod]

This is for the implementation of Portal Network in the Shisui client. Their handler needs access to the node object in order to send further calls to the requesting node. This is a breaking API change but it should be fine, since there are basically no known users of TALKREQ outside of Portal network.

[thinkAfCod]

fix #31094

[qizhou]

Should we consider thread-safe if the method is public?

[thinkAfCod]

Each method called within GetNode already has locks, so there is no need to add additional locks to ensure thread safety.

[fjl]

We discussed this in a meeting with Shisui developers, and the need for calling GetNode arises because we do not provide the *enode.Node to the TALKREQ handler. They get the enode.ID from us, but really need the complete node.

So we should change talk handling to pass the node to the handler function. Doing this will require changing the discv5 session cache to include the node alongside the keys.

[fjl]

I think the approach you chose is too complicated, and the message flag should not be exposed. I might be wrong since I didn't try to implement myself, but here is my suggestion:

In p2p/discover/v5wire/encoding.go, a new session will be stored into the cache in two places:

1. go-ethereum/p2p/discover/v5wire/encoding.go

   Line 344 in d85f796

   c.sc.storeNewSession(toID, addr, session)

2. go-ethereum/p2p/discover/v5wire/encoding.go

   Line 519 in d85f796

   c.sc.storeNewSession(auth.h.SrcID, fromAddr, session)

In both of these places, the node is available:

1. in challenge.Node
2. in node

So we could just add the node as a parameter in storeNewSession and put it into the cache then.

[GrapeBaBa]

I think the approach you chose is too complicated, and the message flag should not be exposed. I might be wrong since I didn't try to implement myself, but here is my suggestion:

In p2p/discover/v5wire/encoding.go, a new session will be stored into the cache in two places:

1. go-ethereum/p2p/discover/v5wire/encoding.go

   Line 344 in d85f796

   c.sc.storeNewSession(toID, addr, session)

2. go-ethereum/p2p/discover/v5wire/encoding.go

   Line 519 in d85f796

   c.sc.storeNewSession(auth.h.SrcID, fromAddr, session)

In both of these places, the node is available:

1. in challenge.Node
2. in node

So we could just add the node as a parameter in storeNewSession and put it into the cache then.

Seems make sense, however how to get node from session cache, session cache is only available in Codec.

[fjl]

Yeah, an accessor method has to be added to the Codec for this.

[thinkAfCod]

Oh, I missed a place to call storeNewSession.

And you mentioned the necessity of exposing the Flag. I've also been struggling with this.

The codec field in UDPv5 is an interface type, it only provides the Encode and Decode methods.

In UDPv5, its cannot directly use codec.sc to query session.

So I tried to make the codec's Decode method return the node most of the time, rather than only returning the node after a successful handshake.

handlePacket origin code:

func (t *UDPv5) handlePacket(rawpacket []byte, fromAddr netip.AddrPort) error {
	addr := fromAddr.String()
	fromID, fromNode, packet, err := t.codec.Decode(rawpacket, addr)
	if err != nil {
		// handle invalid packet and return...
	}
	if fromNode != nil {
		// Handshake succeeded, add to table.
		t.tab.addInboundNode(fromNode)
	}
	// ignoring...
	return nil
}

Here got a problem, which is that now fromNode is almost always not nil, causing addInboundNode to be called every time.

I need a new Flag to indicate whether it is the successful handshake.

And I have other two alternative options:

1. Add a bool flag():

fromID, fromNode, packet, handshakeSuccess, err := t.codec.Decode(rawpacket, addr)
// blabla...
if fromNode != nil && handshakeSuccess {
	// Handshake succeeded, add to table.
	t.tab.addInboundNode(fromNode)
}

2. use a closure:

fromID, fromNode, packet, isHandShake, err := t.codec.Decode(rawpacket, addr, func(node *enode.Node) {
        // Handshake succeeded, add to table.
	t.tab.addInboundNode(fromNode)
})

Or if there are other ways.

[thinkAfCod]

Yeah, an accessor method has to be added to the Codec for this.

I considered it as well, but I feel that adding a new method to the Codec to get the Session seems a bit out of place for its functionality.