Address code review comments

Signed-off-by: Yacov Manevich <[email protected]>
ava-labs · Sep 23, 2024 · a839591 · a839591
1 parent 4a200b1
commit a839591
Show file tree

Hide file tree

Showing 8 changed files with 24 additions and 309 deletions.
diff --git a/chains/manager.go b/chains/manager.go
@@ -891,20 +891,7 @@ func (m *manager) createAvalancheChain(
 	var halter common.Halter
 
 	// Asynchronously passes messages from the network to the consensus engine
-	h, err := handler.New(
-		halter.Halt,
-		ctx,
-		vdrs,
-		msgChan,
-		m.FrontierPollFrequency,
-		m.ConsensusAppConcurrency,
-		m.ResourceTracker,
-		validators.UnhandledSubnetConnector, // avalanche chains don't use subnet connector
-		sb,
-		connectedValidators,
-		peerTracker,
-		handlerReg,
-	)
+	h, err := handler.New(ctx, handlerReg, vdrs, msgChan, m.FrontierPollFrequency, m.ConsensusAppConcurrency, m.ResourceTracker, validators.UnhandledSubnetConnector, sb, connectedValidators, peerTracker, halter.Halt)
 	if err != nil {
 		return nil, fmt.Errorf("error initializing network handler: %w", err)
 	}
@@ -1296,20 +1283,7 @@ func (m *manager) createSnowmanChain(
 	var halter common.Halter
 
 	// Asynchronously passes messages from the network to the consensus engine
-	h, err := handler.New(
-		halter.Halt,
-		ctx,
-		vdrs,
-		msgChan,
-		m.FrontierPollFrequency,
-		m.ConsensusAppConcurrency,
-		m.ResourceTracker,
-		subnetConnector,
-		sb,
-		connectedValidators,
-		peerTracker,
-		handlerReg,
-	)
+	h, err := handler.New(ctx, handlerReg, vdrs, msgChan, m.FrontierPollFrequency, m.ConsensusAppConcurrency, m.ResourceTracker, subnetConnector, sb, connectedValidators, peerTracker, halter.Halt)
 	if err != nil {
 		return nil, fmt.Errorf("couldn't initialize message handler: %w", err)
 	}

diff --git a/snow/engine/snowman/bootstrap/config.go b/snow/engine/snowman/bootstrap/config.go
@@ -14,7 +14,6 @@ import (
 )
 
 type Config struct {
-	ShouldHalt func() bool
 	common.AllGetsServer
 
 	Ctx     *snow.ConsensusContext
@@ -43,4 +42,6 @@ type Config struct {
 	NonVerifyingParse block.ParseFunc
 
 	Bootstrapped func()
+
+	ShouldHalt func() bool
 }
diff --git a/snow/networking/handler/handler.go b/snow/networking/handler/handler.go
@@ -131,20 +131,7 @@ type handler struct {
 
 // Initialize this consensus handler
 // [engine] must be initialized before initializing this handler
-func New(
-	haltBootstrapping func(),
-	ctx *snow.ConsensusContext,
-	validators validators.Manager,
-	msgFromVMChan <-chan common.Message,
-	gossipFrequency time.Duration,
-	threadPoolSize int,
-	resourceTracker tracker.ResourceTracker,
-	subnetConnector validators.SubnetConnector,
-	subnet subnets.Subnet,
-	peerTracker commontracker.Peers,
-	p2pTracker *p2p.PeerTracker,
-	reg prometheus.Registerer,
-) (Handler, error) {
+func New(ctx *snow.ConsensusContext, reg prometheus.Registerer, validators validators.Manager, msgFromVMChan <-chan common.Message, gossipFrequency time.Duration, threadPoolSize int, resourceTracker tracker.ResourceTracker, subnetConnector validators.SubnetConnector, subnet subnets.Subnet, peerTracker commontracker.Peers, p2pTracker *p2p.PeerTracker, haltBootstrapping func()) (Handler, error) {
 	h := &handler{
 		haltBootstrapping: haltBootstrapping,
 		ctx:               ctx,

diff --git a/snow/networking/handler/handler_test.go b/snow/networking/handler/handler_test.go
@@ -68,20 +68,7 @@ func TestHandlerDropsTimedOutMessages(t *testing.T) {
 	)
 	require.NoError(err)
 
-	handlerIntf, err := New(
-		func() {},
-		ctx,
-		vdrs,
-		nil,
-		time.Second,
-		testThreadPoolSize,
-		resourceTracker,
-		validators.UnhandledSubnetConnector,
-		subnets.New(ctx.NodeID, subnets.Config{}),
-		commontracker.NewPeers(),
-		peerTracker,
-		prometheus.NewRegistry(),
-	)
+	handlerIntf, err := New(ctx, prometheus.NewRegistry(), vdrs, nil, time.Second, testThreadPoolSize, resourceTracker, validators.UnhandledSubnetConnector, subnets.New(ctx.NodeID, subnets.Config{}), commontracker.NewPeers(), peerTracker, func() {})
 	require.NoError(err)
 	handler := handlerIntf.(*handler)
 
@@ -176,20 +163,7 @@ func TestHandlerClosesOnError(t *testing.T) {
 	)
 	require.NoError(err)
 
-	handlerIntf, err := New(
-		func() {},
-		ctx,
-		vdrs,
-		nil,
-		time.Second,
-		testThreadPoolSize,
-		resourceTracker,
-		validators.UnhandledSubnetConnector,
-		subnets.New(ctx.NodeID, subnets.Config{}),
-		commontracker.NewPeers(),
-		peerTracker,
-		prometheus.NewRegistry(),
-	)
+	handlerIntf, err := New(ctx, prometheus.NewRegistry(), vdrs, nil, time.Second, testThreadPoolSize, resourceTracker, validators.UnhandledSubnetConnector, subnets.New(ctx.NodeID, subnets.Config{}), commontracker.NewPeers(), peerTracker, func() {})
 	require.NoError(err)
 	handler := handlerIntf.(*handler)
 
@@ -280,20 +254,7 @@ func TestHandlerDropsGossipDuringBootstrapping(t *testing.T) {
 	)
 	require.NoError(err)
 
-	handlerIntf, err := New(
-		func() {},
-		ctx,
-		vdrs,
-		nil,
-		1,
-		testThreadPoolSize,
-		resourceTracker,
-		validators.UnhandledSubnetConnector,
-		subnets.New(ctx.NodeID, subnets.Config{}),
-		commontracker.NewPeers(),
-		peerTracker,
-		prometheus.NewRegistry(),
-	)
+	handlerIntf, err := New(ctx, prometheus.NewRegistry(), vdrs, nil, 1, testThreadPoolSize, resourceTracker, validators.UnhandledSubnetConnector, subnets.New(ctx.NodeID, subnets.Config{}), commontracker.NewPeers(), peerTracker, func() {})
 	require.NoError(err)
 	handler := handlerIntf.(*handler)
 
@@ -372,20 +333,7 @@ func TestHandlerDispatchInternal(t *testing.T) {
 	)
 	require.NoError(err)
 
-	handler, err := New(
-		func() {},
-		ctx,
-		vdrs,
-		msgFromVMChan,
-		time.Second,
-		testThreadPoolSize,
-		resourceTracker,
-		validators.UnhandledSubnetConnector,
-		subnets.New(ctx.NodeID, subnets.Config{}),
-		commontracker.NewPeers(),
-		peerTracker,
-		prometheus.NewRegistry(),
-	)
+	handler, err := New(ctx, prometheus.NewRegistry(), vdrs, msgFromVMChan, time.Second, testThreadPoolSize, resourceTracker, validators.UnhandledSubnetConnector, subnets.New(ctx.NodeID, subnets.Config{}), commontracker.NewPeers(), peerTracker, func() {})
 	require.NoError(err)
 
 	bootstrapper := &enginetest.Bootstrapper{
@@ -459,20 +407,7 @@ func TestHandlerSubnetConnector(t *testing.T) {
 	)
 	require.NoError(err)
 
-	handler, err := New(
-		func() {},
-		ctx,
-		vdrs,
-		nil,
-		time.Second,
-		testThreadPoolSize,
-		resourceTracker,
-		connector,
-		subnets.New(ctx.NodeID, subnets.Config{}),
-		commontracker.NewPeers(),
-		peerTracker,
-		prometheus.NewRegistry(),
-	)
+	handler, err := New(ctx, prometheus.NewRegistry(), vdrs, nil, time.Second, testThreadPoolSize, resourceTracker, connector, subnets.New(ctx.NodeID, subnets.Config{}), commontracker.NewPeers(), peerTracker, func() {})
 	require.NoError(err)
 
 	bootstrapper := &enginetest.Bootstrapper{
@@ -642,20 +577,7 @@ func TestDynamicEngineTypeDispatch(t *testing.T) {
 			)
 			require.NoError(err)
 
-			handler, err := New(
-				func() {},
-				ctx,
-				vdrs,
-				nil,
-				time.Second,
-				testThreadPoolSize,
-				resourceTracker,
-				validators.UnhandledSubnetConnector,
-				subnets.New(ids.EmptyNodeID, subnets.Config{}),
-				commontracker.NewPeers(),
-				peerTracker,
-				prometheus.NewRegistry(),
-			)
+			handler, err := New(ctx, prometheus.NewRegistry(), vdrs, nil, time.Second, testThreadPoolSize, resourceTracker, validators.UnhandledSubnetConnector, subnets.New(ids.EmptyNodeID, subnets.Config{}), commontracker.NewPeers(), peerTracker, func() {})
 			require.NoError(err)
 
 			bootstrapper := &enginetest.Bootstrapper{
@@ -726,20 +648,7 @@ func TestHandlerStartError(t *testing.T) {
 	)
 	require.NoError(err)
 
-	handler, err := New(
-		func() {},
-		ctx,
-		validators.NewManager(),
-		nil,
-		time.Second,
-		testThreadPoolSize,
-		resourceTracker,
-		nil,
-		subnets.New(ctx.NodeID, subnets.Config{}),
-		commontracker.NewPeers(),
-		peerTracker,
-		prometheus.NewRegistry(),
-	)
+	handler, err := New(ctx, prometheus.NewRegistry(), validators.NewManager(), nil, time.Second, testThreadPoolSize, resourceTracker, nil, subnets.New(ctx.NodeID, subnets.Config{}), commontracker.NewPeers(), peerTracker, func() {})
 	require.NoError(err)
 
 	// Starting a handler with an unprovided engine should immediately cause the

diff --git a/snow/networking/handler/health_test.go b/snow/networking/handler/health_test.go
@@ -82,20 +82,7 @@ func TestHealthCheckSubnet(t *testing.T) {
 			)
 			require.NoError(err)
 
-			handlerIntf, err := New(
-				func() {},
-				ctx,
-				vdrs,
-				nil,
-				time.Second,
-				testThreadPoolSize,
-				resourceTracker,
-				validators.UnhandledSubnetConnector,
-				sb,
-				peerTracker,
-				p2pTracker,
-				prometheus.NewRegistry(),
-			)
+			handlerIntf, err := New(ctx, prometheus.NewRegistry(), vdrs, nil, time.Second, testThreadPoolSize, resourceTracker, validators.UnhandledSubnetConnector, sb, peerTracker, p2pTracker, func() {})
 			require.NoError(err)
 
 			bootstrapper := &enginetest.Bootstrapper{