Remove error from SDK AppGossip handler (#2252)

Signed-off-by: Joshua Kim <[email protected]>
Co-authored-by: Stephen Buttolph <[email protected]>

go.mod

@@ -11,7 +11,7 @@ require (
 	github.com/DataDog/zstd v1.5.2
 	github.com/Microsoft/go-winio v0.5.2
 	github.com/NYTimes/gziphandler v1.1.1
-	github.com/ava-labs/coreth v0.12.8-rc.1
+	github.com/ava-labs/coreth v0.12.9-rc.0
 	github.com/ava-labs/ledger-avalanche/go v0.0.0-20231102202641-ae2ebdaeac34
 	github.com/btcsuite/btcd/btcutil v1.1.3
 	github.com/cockroachdb/pebble v0.0.0-20230209160836-829675f94811

go.sum

@@ -66,8 +66,8 @@ github.com/allegro/bigcache v1.2.1-0.20190218064605-e24eb225f156 h1:eMwmnE/GDgah
 github.com/allegro/bigcache v1.2.1-0.20190218064605-e24eb225f156/go.mod h1:Cb/ax3seSYIx7SuZdm2G2xzfwmv3TPSk2ucNfQESPXM=
 github.com/antihax/optional v1.0.0/go.mod h1:uupD/76wgC+ih3iEmQUL+0Ugr19nfwCT1kdvxnR2qWY=
 github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8=
-github.com/ava-labs/coreth v0.12.8-rc.1 h1:tvJcxQTQzxIQqx8TnrxdyMhZYbdsMaiy6AEiOyjvaa4=
-github.com/ava-labs/coreth v0.12.8-rc.1/go.mod h1:GBH5SxHZdScSp95IijDs9+Gxw/QDIWvfoLKiJMNYLsE=
+github.com/ava-labs/coreth v0.12.9-rc.0 h1:Xvk/iJTY2MSBkkiOs9Eo92nxd67VXzRjaC/WmQXRIb0=
+github.com/ava-labs/coreth v0.12.9-rc.0/go.mod h1:rECKQfGFDeodrwGPlJSvFUJDbVr30jSMIVjQLi6pNX4=
 github.com/ava-labs/ledger-avalanche/go v0.0.0-20231102202641-ae2ebdaeac34 h1:mg9Uw6oZFJKytJxgxnl3uxZOs/SB8CVHg6Io4Tf99Zc=
 github.com/ava-labs/ledger-avalanche/go v0.0.0-20231102202641-ae2ebdaeac34/go.mod h1:pJxaT9bUgeRNVmNRgtCHb7sFDIRKy7CzTQVi8gGNT6g=
 github.com/aymerick/raymond v2.0.3-0.20180322193309-b565731e1464+incompatible/go.mod h1:osfaiScAUVup+UC9Nfq76eWqDhXlp+4UYaA8uhTBO6g=

network/p2p/handler.go

@@ -30,7 +30,7 @@ type Handler interface {
 		ctx context.Context,
 		nodeID ids.NodeID,
 		gossipBytes []byte,
-	) error
+	)
 	// AppRequest is called when handling an AppRequest message.
 	// Returns the bytes for the response corresponding to [requestBytes]
 	AppRequest(
@@ -53,9 +53,7 @@ type Handler interface {
 // NoOpHandler drops all messages
 type NoOpHandler struct{}
 
-func (NoOpHandler) AppGossip(context.Context, ids.NodeID, []byte) error {
-	return nil
-}
+func (NoOpHandler) AppGossip(context.Context, ids.NodeID, []byte) {}
 
 func (NoOpHandler) AppRequest(context.Context, ids.NodeID, time.Time, []byte) ([]byte, error) {
 	return nil, nil
@@ -69,14 +67,16 @@ func (NoOpHandler) CrossChainAppRequest(context.Context, ids.ID, time.Time, []by
 type ValidatorHandler struct {
 	Handler
 	ValidatorSet ValidatorSet
+	Log          logging.Logger
 }
 
-func (v ValidatorHandler) AppGossip(ctx context.Context, nodeID ids.NodeID, gossipBytes []byte) error {
+func (v ValidatorHandler) AppGossip(ctx context.Context, nodeID ids.NodeID, gossipBytes []byte) {
 	if !v.ValidatorSet.Has(ctx, nodeID) {
-		return ErrNotValidator
+		v.Log.Debug("dropping message", zap.Stringer("nodeID", nodeID))
+		return
 	}
 
-	return v.Handler.AppGossip(ctx, nodeID, gossipBytes)
+	v.Handler.AppGossip(ctx, nodeID, gossipBytes)
 }
 
 func (v ValidatorHandler) AppRequest(ctx context.Context, nodeID ids.NodeID, deadline time.Time, requestBytes []byte) ([]byte, error) {
@@ -89,15 +89,15 @@ func (v ValidatorHandler) AppRequest(ctx context.Context, nodeID ids.NodeID, dea
 
 // responder automatically sends the response for a given request
 type responder struct {
+	Handler
 	handlerID uint64
-	handler   Handler
 	log       logging.Logger
 	sender    common.AppSender
 }
 
 // AppRequest calls the underlying handler and sends back the response to nodeID
 func (r *responder) AppRequest(ctx context.Context, nodeID ids.NodeID, requestID uint32, deadline time.Time, request []byte) error {
-	appResponse, err := r.handler.AppRequest(ctx, nodeID, deadline, request)
+	appResponse, err := r.Handler.AppRequest(ctx, nodeID, deadline, request)
 	if err != nil {
 		r.log.Debug("failed to handle message",
 			zap.Stringer("messageOp", message.AppRequestOp),
@@ -113,21 +113,10 @@ func (r *responder) AppRequest(ctx context.Context, nodeID ids.NodeID, requestID
 	return r.sender.SendAppResponse(ctx, nodeID, requestID, appResponse)
 }
 
-func (r *responder) AppGossip(ctx context.Context, nodeID ids.NodeID, msg []byte) {
-	if err := r.handler.AppGossip(ctx, nodeID, msg); err != nil {
-		r.log.Debug("failed to handle message",
-			zap.Stringer("messageOp", message.AppGossipOp),
-			zap.Stringer("nodeID", nodeID),
-			zap.Uint64("handlerID", r.handlerID),
-			zap.Binary("message", msg),
-		)
-	}
-}
-
 // CrossChainAppRequest calls the underlying handler and sends back the response
 // to chainID
 func (r *responder) CrossChainAppRequest(ctx context.Context, chainID ids.ID, requestID uint32, deadline time.Time, request []byte) error {
-	appResponse, err := r.handler.CrossChainAppRequest(ctx, chainID, deadline, request)
+	appResponse, err := r.Handler.CrossChainAppRequest(ctx, chainID, deadline, request)
 	if err != nil {
 		r.log.Debug("failed to handle message",
 			zap.Stringer("messageOp", message.CrossChainAppRequestOp),

network/p2p/handler_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/ava-labs/avalanchego/ids"
+	"github.com/ava-labs/avalanchego/utils/logging"
 	"github.com/ava-labs/avalanchego/utils/set"
 )
 
@@ -32,34 +33,40 @@ func TestValidatorHandlerAppGossip(t *testing.T) {
 		name         string
 		validatorSet ValidatorSet
 		nodeID       ids.NodeID
-		expected     error
+		expected     bool
 	}{
 		{
 			name:         "message dropped",
 			validatorSet: testValidatorSet{},
 			nodeID:       nodeID,
-			expected:     ErrNotValidator,
 		},
 		{
 			name: "message handled",
 			validatorSet: testValidatorSet{
 				validators: validatorSet,
 			},
-			nodeID: nodeID,
+			nodeID:   nodeID,
+			expected: true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			require := require.New(t)
 
+			called := false
 			handler := ValidatorHandler{
-				Handler:      NoOpHandler{},
+				Handler: testHandler{
+					appGossipF: func(context.Context, ids.NodeID, []byte) {
+						called = true
+					},
+				},
 				ValidatorSet: tt.validatorSet,
+				Log:          logging.NoLog{},
 			}
 
-			err := handler.AppGossip(context.Background(), tt.nodeID, []byte("foobar"))
-			require.ErrorIs(err, tt.expected)
+			handler.AppGossip(context.Background(), tt.nodeID, []byte("foobar"))
+			require.Equal(tt.expected, called)
 		})
 	}
 }
@@ -96,6 +103,7 @@ func TestValidatorHandlerAppRequest(t *testing.T) {
 			handler := ValidatorHandler{
 				Handler:      NoOpHandler{},
 				ValidatorSet: tt.validatorSet,
+				Log:          logging.NoLog{},
 			}
 
 			_, err := handler.AppRequest(context.Background(), tt.nodeID, time.Time{}, []byte("foobar"))

network/p2p/router.go

@@ -174,8 +174,8 @@ func (r *Router) RegisterAppProtocol(handlerID uint64, handler Handler, nodeSamp
 
 	r.handlers[handlerID] = &meteredHandler{
 		responder: &responder{
+			Handler:   handler,
 			handlerID: handlerID,
-			handler:   handler,
 			log:       r.log,
 			sender:    r.sender,
 		},

network/p2p/throttler_handler.go

@@ -9,7 +9,10 @@ import (
 	"fmt"
 	"time"
 
+	"go.uber.org/zap"
+
 	"github.com/ava-labs/avalanchego/ids"
+	"github.com/ava-labs/avalanchego/utils/logging"
 )
 
 var (
@@ -20,14 +23,16 @@ var (
 type ThrottlerHandler struct {
 	Handler
 	Throttler Throttler
+	Log       logging.Logger
 }
 
-func (t ThrottlerHandler) AppGossip(ctx context.Context, nodeID ids.NodeID, gossipBytes []byte) error {
+func (t ThrottlerHandler) AppGossip(ctx context.Context, nodeID ids.NodeID, gossipBytes []byte) {
 	if !t.Throttler.Handle(nodeID) {
-		return fmt.Errorf("dropping message from %s: %w", nodeID, ErrThrottled)
+		t.Log.Debug("dropping message", zap.Stringer("nodeID", nodeID))
+		return
 	}
 
-	return t.Handler.AppGossip(ctx, nodeID, gossipBytes)
+	t.Handler.AppGossip(ctx, nodeID, gossipBytes)
 }
 
 func (t ThrottlerHandler) AppRequest(ctx context.Context, nodeID ids.NodeID, deadline time.Time, requestBytes []byte) ([]byte, error) {

network/p2p/throttler_handler_test.go

@@ -11,34 +11,44 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/ava-labs/avalanchego/ids"
+	"github.com/ava-labs/avalanchego/utils/logging"
 )
 
+var _ Handler = (*testHandler)(nil)
+
 func TestThrottlerHandlerAppGossip(t *testing.T) {
 	tests := []struct {
-		name        string
-		Throttler   Throttler
-		expectedErr error
+		name      string
+		Throttler Throttler
+		expected  bool
 	}{
 		{
-			name:      "throttled",
+			name:      "not throttled",
 			Throttler: NewSlidingWindowThrottler(time.Second, 1),
+			expected:  true,
 		},
 		{
-			name:        "throttler errors",
-			Throttler:   NewSlidingWindowThrottler(time.Second, 0),
-			expectedErr: ErrThrottled,
+			name:      "throttled",
+			Throttler: NewSlidingWindowThrottler(time.Second, 0),
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			require := require.New(t)
 
+			called := false
 			handler := ThrottlerHandler{
-				Handler:   NoOpHandler{},
+				Handler: testHandler{
+					appGossipF: func(context.Context, ids.NodeID, []byte) {
+						called = true
+					},
+				},
 				Throttler: tt.Throttler,
+				Log:       logging.NoLog{},
 			}
-			err := handler.AppGossip(context.Background(), ids.GenerateTestNodeID(), []byte("foobar"))
-			require.ErrorIs(err, tt.expectedErr)
+
+			handler.AppGossip(context.Background(), ids.GenerateTestNodeID(), []byte("foobar"))
+			require.Equal(tt.expected, called)
 		})
 	}
 }
@@ -50,11 +60,11 @@ func TestThrottlerHandlerAppRequest(t *testing.T) {
 		expectedErr error
 	}{
 		{
-			name:      "throttled",
+			name:      "not throttled",
 			Throttler: NewSlidingWindowThrottler(time.Second, 1),
 		},
 		{
-			name:        "throttler errors",
+			name:        "throttled",
 			Throttler:   NewSlidingWindowThrottler(time.Second, 0),
 			expectedErr: ErrThrottled,
 		},
@@ -66,9 +76,40 @@ func TestThrottlerHandlerAppRequest(t *testing.T) {
 			handler := ThrottlerHandler{
 				Handler:   NoOpHandler{},
 				Throttler: tt.Throttler,
+				Log:       logging.NoLog{},
 			}
 			_, err := handler.AppRequest(context.Background(), ids.GenerateTestNodeID(), time.Time{}, []byte("foobar"))
 			require.ErrorIs(err, tt.expectedErr)
 		})
 	}
 }
+
+type testHandler struct {
+	appGossipF            func(ctx context.Context, nodeID ids.NodeID, gossipBytes []byte)
+	appRequestF           func(ctx context.Context, nodeID ids.NodeID, deadline time.Time, requestBytes []byte) ([]byte, error)
+	crossChainAppRequestF func(ctx context.Context, chainID ids.ID, deadline time.Time, requestBytes []byte) ([]byte, error)
+}
+
+func (t testHandler) AppGossip(ctx context.Context, nodeID ids.NodeID, gossipBytes []byte) {
+	if t.appGossipF == nil {
+		return
+	}
+
+	t.appGossipF(ctx, nodeID, gossipBytes)
+}
+
+func (t testHandler) AppRequest(ctx context.Context, nodeID ids.NodeID, deadline time.Time, requestBytes []byte) ([]byte, error) {
+	if t.appRequestF == nil {
+		return nil, nil
+	}
+
+	return t.appRequestF(ctx, nodeID, deadline, requestBytes)
+}
+
+func (t testHandler) CrossChainAppRequest(ctx context.Context, chainID ids.ID, deadline time.Time, requestBytes []byte) ([]byte, error) {
+	if t.crossChainAppRequestF == nil {
+		return nil, nil
+	}
+
+	return t.crossChainAppRequestF(ctx, chainID, deadline, requestBytes)
+}