Improve p2ptest.Client

[joshua-kim]

Why this should be merged

• Simplifies code for p2ptest
• Zero-value of Client is now useful

How this works

• Adds an interface and a testing implementation of p2p.ClientInterface

How this was tested

• UTs

[aaronbuchwald]

One small nit

[aaronbuchwald]

nit: can we change to nodeID and peerNodeID or myNodeID and peerNodeID?

[joshua-kim]

• NodeID -> MyNodeID seems unnecessary because Client.NodeID already disambiguates the name
• ServerNodeID -> PeerNodeID I could get behind... but I wouldn't be sure what to name Client as Client talking to Server seems more natural than Client talking to Peer. I guess we could call it Sender?

[aaronbuchwald]

Ya I think you're right client.NodeID should be clear. I prefer PeerNodeID since it's a p2p network rather than single server and updating Handler to PeerHandler or ServerHandler would make it a little more clear.

[aaronbuchwald]

Why make AppGossip send to ourselves?

[joshua-kim]

It doesn't, it sends it to the server handler with the sender as us. Maybe I can make this more clear by naming Handler to ServerHandler.

[aaronbuchwald]

Ah ya I think renaming would make that more clear

[aaronbuchwald]

Minor nit on potential name change - then this LGTM

[StephenButtolph]

I wish there was a better name for this ClientInterface just looks non-go-ie

[ARR4N]

What if you instead created a p2ptest.Sender that wraps a p2p.Handler and then use it in a real p2p.Client? I'm not as familiar with this code as you all are, but this reminded me of the gRPC testing strategy of using a real stack while faking the network connection with bufconn.

type Sender struct {
	Handler p2p.Handler
}

var _ common.AppSender = (*Sender)(nil)

func (s *Sender) SendAppRequest(ctx context.Context, nodeIDs set.Set[ids.NodeID], requestID uint32, appRequestBytes []byte) error {
    // ...
    s.Handler.AppRequest(ctx, nodeIDs.List()[0], time.Time{}, appRequestBytes)
    // How would this plumb to the response / error methods to maintain the expected behaviour?
    // ...
}

func (s *Sender) SendAppGossip(ctx context.Context, config common.SendConfig, appGossipBytes []byte) error {
    // ...
    s.Handler.AppGossip(ctx, config.NodeIDs.List()[0], appGossipBytes)
    // ...
}

// ... response and error methods

Context: @StephenButtolph asked if I could think of a different name for ClientInterface and my response was that I actually think it's a bit non-go-ie to have it defined alongside the *Client.

[ARR4N]

(style) This is too far from its usage to just be called c. I'd move the declaration to the end (as against renaming it) because I spent the whole time trying to understand where it's used. Some other variables (at least the handler) should also be addressed for the same reason.