feat(parachain): send requests from subsystems

[haikoschol]

Changes

This PR adds support for sending requests of request/response network protocols via overseer/network bridge from parachain subsystems.

Tests

go test -run TestSendRequests ./dot/parachain/network-bridge

Issues

#4448

[haikoschol]

As we agreed on a recent call, this approach is using SendRequests messages passed through overseer to the network bridge. Although the plan was to propose types/structure before implementing, I had to code it to figure whether it would actually work. One issue I ran into repeatedly was cyclic imports. To get unstuck, I moved a few things into dot/parachain/network-bridge/messages that don't really belong there. That's a minor thing that can be changed later of course.

I've used the following existing types from the network package for the implementation:

• Message for requests
• ResponseMessage for responses
• the RequestMaker interface
• RequestResponseProtocol returned by network.Service.GetRequestResponseProtocol

I've added the following types:

// ReqProtocolMessage is a network message that can be sent over a request response protocol.
type ReqProtocolMessage interface {
	network.Message
	// Response returns an instance of the response type for this message, for the purpose of decoding into it.
	Response() network.ResponseMessage
	Protocol() ReqProtocolName
}

// ReqRespResult is the result of sending a request over a request response protocol. It contains either a response
// message or an error.
type ReqRespResult struct {
	Response network.ResponseMessage
	Error    error
}

// OutgoingRequest contains all data required to send a request over a request response protocol and receive the result.
type OutgoingRequest struct {
	Recipient peer.ID // TODO use a type that can contain either a peer ID or an authority ID
	Payload   ReqProtocolMessage
	Result    chan ReqRespResult
}

type IfDisconnectedBehavior int

const (
	TryConnect     IfDisconnectedBehavior = iota
	ImmediateError                        // TODO not implemented
)

// SendRequests is a subsystem message for sending requests over a request response protocol.
type SendRequests struct {
	Requests       []*OutgoingRequest
	IfDisconnected IfDisconnectedBehavior
}

Usage from a subsystem looks roughly as follows:

request := networkbridgemessages.NewOutgoingRequest(
	"recipient",
	networkbridgemessages.ChunkFetchingRequest{
		CandidateHash: parachaintypes.CandidateHash{Value: common.Hash{1}},
		Index:         42,
	})

sendRequests := networkbridgemessages.SendRequests{
	Requests:       []*networkbridgemessages.OutgoingRequest{request},
	IfDisconnected: networkbridgemessages.TryConnect,
}

ss.subsystemToOverseer <- sendRequests

result := <- request.Result

There are a few downsides/issues to discuss in this approach:

1. Concurrency

The goroutine in NetworkBridgeSender that handles overseer messages currently handles requests sequentially and blocks on each request until error/timeout/response. It's easy enough to start a new goroutine for each request, but unclear how cancellation/shutdown can be handled properly. RequestResponseProtocol itself does not have a mechanism for aborting requests.

2. Parameterization

RequestResponseProtocol requires setting a value for timeouts and the maximum response size. Right now, this is set to hard-coded values in NetworkBridgeSender. Ideally, subsystems would set these values according to their requirements. This could be accomplished by adding these parameters to the OutgoingRequest struct

3. Efficiency

With the current implementation, an instance of RequestResponseProtocol is created for each request. This causes a large buffer to be allocated every time, which was supposed to be avoided by keeping this buffer in the objects state.

[EclesioMeloJunior]

LGTM, just a small observation on context creation

[EclesioMeloJunior]

LGTM, just few comments

[EclesioMeloJunior]

yeah, I would say you can attach to the protocol string the max response size

[haikoschol]

The question then becomes what the values for requests other than PoVFetchingV1 should be. Would you be ok with deferring this change to a later PR, given that using smaller values here amounts to a potential performance optimization?

[EclesioMeloJunior]

just to add an extra layer, when the channel is closed ok is false. Actually, you don't need the select when you retrieve a tuple from the channel (even if the channel is not closed)

Suggested change

	case <-ch:
	_, ok := <-ch
	require.False(t, ok)

[haikoschol]

If the channel is empty but not closed, the receive will block though. The ok merely indicates whether the received value is the zero value for the type of the channel, i.e. whether the channel was empty.