remove request parse check in network forwarding (#977) (#1593)

Signed-off-by: Ceyhun Onur <[email protected]>
Signed-off-by: Ceyhun Onur <[email protected]>
Co-authored-by: Copilot <[email protected]>

Author: ceyonur

peer/network.go

@@ -228,9 +228,8 @@ func (n *network) AppRequest(ctx context.Context, nodeID ids.NodeID, requestID u
 
 	log.Debug("received AppRequest from node", "nodeID", nodeID, "requestID", requestID, "requestLen", len(request))
 
-	var req message.Request
-	if _, err := n.codec.Unmarshal(request, &req); err != nil {
-		log.Debug("forwarding AppRequest to SDK network", "nodeID", nodeID, "requestID", requestID, "requestLen", len(request), "err", err)
+	if !IsNetworkRequest(requestID) {
+		log.Debug("forwarding AppRequest to SDK network", "nodeID", nodeID, "requestID", requestID, "requestLen", len(request))
 		return n.p2pNetwork.AppRequest(ctx, nodeID, requestID, deadline, request)
 	}
 
@@ -240,6 +239,12 @@ func (n *network) AppRequest(ctx context.Context, nodeID ids.NodeID, requestID u
 		return nil
 	}
 
+	var req message.Request
+	if _, err := n.codec.Unmarshal(request, &req); err != nil {
+		log.Debug("failed to unmarshal AppRequest", "nodeID", nodeID, "requestID", requestID, "err", err)
+		return nil
+	}
+
 	log.Debug("processing incoming request", "nodeID", nodeID, "requestID", requestID, "req", req)
 	// We make a new context here because we don't want to cancel the context
 	// passed into n.AppSender.SendAppResponse below
@@ -435,3 +440,10 @@ func (n *network) nextRequestID() uint32 {
 
 	return next
 }
+
+// IsNetworkRequest checks if the given requestID is a request for this network handler (even-numbered requestIDs)
+// SDK requests are odd-numbered requestIDs
+// (see invariant: https://github.com/ava-labs/avalanchego/blob/v1.13.0/network/p2p/router.go#L83)
+func IsNetworkRequest(requestID uint32) bool {
+	return requestID%2 == 0
+}

peer/network_test.go

@@ -22,6 +22,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/ava-labs/subnet-evm/peer/peertest"
 	"github.com/ava-labs/subnet-evm/plugin/evm/message"
 
 	"github.com/ava-labs/avalanchego/codec"
@@ -477,8 +478,7 @@ func TestOnRequestHonoursDeadline(t *testing.T) {
 
 	requestHandler.response, err = marshalStruct(codecManager, TestMessage{Message: "hi there"})
 	assert.NoError(t, err)
-	err = net.AppRequest(context.Background(), nodeID, 1, time.Now().Add(1*time.Millisecond), requestBytes)
-	assert.NoError(t, err)
+	assert.NoError(t, net.AppRequest(context.Background(), nodeID, 0, time.Now().Add(1*time.Millisecond), requestBytes))
 	// ensure the handler didn't get called (as peer.Network would've dropped the request)
 	assert.EqualValues(t, requestHandler.calls, 0)
 
@@ -492,7 +492,7 @@ func TestOnRequestHonoursDeadline(t *testing.T) {
 func TestHandleInvalidMessages(t *testing.T) {
 	codecManager := buildCodec(t, HelloGossip{}, TestMessage{})
 	nodeID := ids.GenerateTestNodeID()
-	requestID := uint32(1)
+	requestID := peertest.TestSDKRequestID
 	sender := &enginetest.Sender{
 		SendAppErrorF: func(context.Context, ids.NodeID, uint32, int32, string) error {
 			return nil
@@ -544,7 +544,7 @@ func TestHandleInvalidMessages(t *testing.T) {
 func TestNetworkPropagatesRequestHandlerError(t *testing.T) {
 	codecManager := buildCodec(t, TestMessage{})
 	nodeID := ids.GenerateTestNodeID()
-	requestID := uint32(1)
+	requestID := peertest.TestPeerRequestID
 	sender := testAppSender{}
 
 	p2pNetwork, err := p2p.NewNetwork(logging.NoLog{}, nil, prometheus.NewRegistry(), "")
@@ -585,6 +585,7 @@ func TestNetworkRouting(t *testing.T) {
 		},
 	}
 	protocol := 0
+	requestID := peertest.TestSDKRequestID
 	handler := &testSDKHandler{}
 	p2pNetwork, err := p2p.NewNetwork(logging.NoLog{}, sender, prometheus.NewRegistry(), "")
 	require.NoError(err)
@@ -595,14 +596,14 @@ func TestNetworkRouting(t *testing.T) {
 
 	nodeID := ids.GenerateTestNodeID()
 	foobar := append([]byte{byte(protocol)}, []byte("foobar")...)
-	err = network.AppRequest(context.Background(), nodeID, 0, time.Time{}, foobar)
-	require.NoError(err)
+	// forward it to the sdk handler
+	require.NoError(network.AppRequest(context.Background(), nodeID, requestID, time.Now().Add(5*time.Second), foobar))
 	require.True(handler.appRequested)
 
-	err = network.AppResponse(context.Background(), ids.GenerateTestNodeID(), 0, foobar)
+	err = network.AppResponse(context.Background(), ids.GenerateTestNodeID(), requestID, foobar)
 	require.ErrorIs(err, p2p.ErrUnrequestedResponse)
 
-	err = network.AppRequestFailed(context.Background(), nodeID, 0, common.ErrTimeout)
+	err = network.AppRequestFailed(context.Background(), nodeID, requestID, common.ErrTimeout)
 	require.ErrorIs(err, p2p.ErrUnrequestedResponse)
 }
 

peer/peertest/request_id.go

@@ -0,0 +1,14 @@
+// (c) 2019-2022, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package peertest
+
+const (
+	// TestSDKRequestID is the request ID for the SDK request, which is odd-numbered.
+	// See peer.IsNetworkRequest for more details.
+	TestSDKRequestID uint32 = 1
+
+	// TestPeerRequestID is the request ID for the peer request, which is even-numbered.
+	// See peer.IsNetworkRequest for more details.
+	TestPeerRequestID uint32 = 0
+)

plugin/evm/message/request.go

@@ -21,15 +21,6 @@ type Request interface {
 	Handle(ctx context.Context, nodeID ids.NodeID, requestID uint32, handler RequestHandler) ([]byte, error)
 }
 
-// BytesToRequest unmarshals the given requestBytes into Request object
-func BytesToRequest(codec codec.Manager, requestBytes []byte) (Request, error) {
-	var request Request
-	if _, err := codec.Unmarshal(requestBytes, &request); err != nil {
-		return nil, err
-	}
-	return request, nil
-}
-
 // RequestToBytes marshals the given request object into bytes
 func RequestToBytes(codec codec.Manager, request Request) ([]byte, error) {
 	return codec.Marshal(Version, &request)

plugin/evm/vm_warp_test.go

@@ -35,6 +35,7 @@ import (
 	"github.com/ava-labs/subnet-evm/eth/tracers"
 	"github.com/ava-labs/subnet-evm/params"
 	"github.com/ava-labs/subnet-evm/params/extras"
+	"github.com/ava-labs/subnet-evm/peer/peertest"
 	customheader "github.com/ava-labs/subnet-evm/plugin/evm/header"
 	"github.com/ava-labs/subnet-evm/plugin/evm/message"
 	"github.com/ava-labs/subnet-evm/precompile/contract"
@@ -798,8 +799,7 @@ func TestMessageSignatureRequestsToVM(t *testing.T) {
 
 			// Send the app request and make sure we called SendAppResponseFn
 			deadline := time.Now().Add(60 * time.Second)
-			err = vm.Network.AppRequest(context.Background(), ids.GenerateTestNodeID(), 1, deadline, requestBytes)
-			require.NoError(t, err)
+			require.NoError(t, vm.Network.AppRequest(context.Background(), ids.GenerateTestNodeID(), peertest.TestPeerRequestID, deadline, requestBytes))
 			require.True(t, calledSendAppResponseFn)
 		})
 	}
@@ -856,8 +856,7 @@ func TestBlockSignatureRequestsToVM(t *testing.T) {
 
 			// Send the app request and make sure we called SendAppResponseFn
 			deadline := time.Now().Add(60 * time.Second)
-			err = vm.Network.AppRequest(context.Background(), ids.GenerateTestNodeID(), 1, deadline, requestBytes)
-			require.NoError(t, err)
+			require.NoError(t, vm.Network.AppRequest(context.Background(), ids.GenerateTestNodeID(), peertest.TestPeerRequestID, deadline, requestBytes))
 			require.True(t, calledSendAppResponseFn)
 		})
 	}