Node: Clean up feature flags (#4352)

* Node: Clean up feature flags

* Node: Clean up feature flags

node/cmd/guardiand/node.go

@@ -966,6 +966,13 @@ func runNode(cmd *cobra.Command, args []string) {
 		if parseErr != nil {
 			logger.Fatal("transferVerifierEnabledChainIDs input is invalid", zap.Error(parseErr))
 		}
+
+		// Format the feature string in the form "txverifier:ethereum|sui" and append it to the feature flags.
+		chainNames := make([]string, 0, len(txVerifierChains))
+		for _, cid := range txVerifierChains {
+			chainNames = append(chainNames, cid.String())
+		}
+		featureFlags = append(featureFlags, fmt.Sprintf("txverifier:%s", strings.Join(chainNames, "|")))
 	}
 
 	var publicRpcLogDetail common.GrpcLogDetail
@@ -1212,7 +1219,6 @@ func runNode(cmd *cobra.Command, args []string) {
 		if err != nil {
 			logger.Fatal("failed to connect to wormchain", zap.Error(err), zap.String("component", "gwrelayer"))
 		}
-
 	}
 	usingPromRemoteWrite := *promRemoteURL != ""
 	if usingPromRemoteWrite {
@@ -1917,11 +1923,28 @@ func runNode(cmd *cobra.Command, args []string) {
 		node.GuardianOptionGatewayRelayer(*gatewayRelayerContract, gatewayRelayerWormchainConn),
 		node.GuardianOptionQueryHandler(*ccqEnabled, *ccqAllowedRequesters),
 		node.GuardianOptionAdminService(*adminSocketPath, ethRPC, ethContract, rpcMap),
-		node.GuardianOptionP2P(p2pKey, *p2pNetworkID, *p2pBootstrap, *nodeName, *subscribeToVAAs, *disableHeartbeatVerify, *p2pPort, *ccqP2pBootstrap, *ccqP2pPort, *ccqAllowedPeers,
-			*gossipAdvertiseAddress, ibc.GetFeatures, protectedPeers, ccqProtectedPeers, featureFlags),
 		node.GuardianOptionStatusServer(*statusAddr),
 		node.GuardianOptionAlternatePublisher(guardianAddrAsBytes, *additionalPublishers),
 		node.GuardianOptionProcessor(*p2pNetworkID),
+
+		// Keep this last so that all of its dependencies are met.
+		node.GuardianOptionP2P(
+			p2pKey,
+			*p2pNetworkID,
+			*p2pBootstrap,
+			*nodeName,
+			*subscribeToVAAs,
+			*disableHeartbeatVerify,
+			*p2pPort,
+			*ccqP2pBootstrap,
+			*ccqP2pPort,
+			*ccqAllowedPeers,
+			*gossipAdvertiseAddress,
+			ibcWatcherConfig != nil,
+			protectedPeers,
+			ccqProtectedPeers,
+			featureFlags,
+		),
 	}
 
 	if shouldStart(publicGRPCSocketPath) {

node/pkg/altpub/alternate_pub.go

@@ -334,6 +334,7 @@ func parseEndpoint(config string) (*Endpoint, error) {
 
 // GetFeatures returns the status string to be published in P2P heartbeats. For now, it just returns a static string
 // listing the enabled endpoints, but in the future, it might return the actual status of each endpoint or something.
+// NOTE: `node.getStaticFeatureFlags` assumes that this does not change after initialization.
 func (ap *AlternatePublisher) GetFeatures() string {
 	return ap.status
 }

node/pkg/node/node_test.go

@@ -190,14 +190,17 @@ func mockGuardianRunnable(t testing.TB, gs []*mockGuardian, mockGuardianIndex ui
 			GuardianOptionNoAccountant(), // disable accountant
 			GuardianOptionGovernor(true, false, ""),
 			GuardianOptionGatewayRelayer("", nil), // disable gateway relayer
-			GuardianOptionP2P(gs[mockGuardianIndex].p2pKey, networkID, bootstrapPeers, nodeName, false, false, cfg.p2pPort, "", 0, "", "", func() string { return "" }, []string{}, []string{}, []string{}),
+			GuardianOptionQueryHandler(false, ""), // disable queries
 			GuardianOptionPublicRpcSocket(cfg.publicSocket, publicRpcLogDetail),
 			GuardianOptionPublicrpcTcpService(cfg.publicRpc, publicRpcLogDetail),
 			GuardianOptionPublicWeb(cfg.publicWeb, cfg.publicSocket, "", false, ""),
 			GuardianOptionAdminService(cfg.adminSocket, nil, nil, rpcMap),
 			GuardianOptionStatusServer(fmt.Sprintf("[::]:%d", cfg.statusPort)),
-			GuardianOptionAlternatePublisher([]byte{}, []string{}),
+			GuardianOptionAlternatePublisher([]byte{}, []string{}), // disable alternate publisher
 			GuardianOptionProcessor(networkID),
+
+			// Keep this last so that all of its dependencies are met.
+			GuardianOptionP2P(gs[mockGuardianIndex].p2pKey, networkID, bootstrapPeers, nodeName, false, false, cfg.p2pPort, "", 0, "", "", false, []string{}, []string{}, []string{}),
 		}
 
 		guardianNode := NewGuardianNode(

node/pkg/node/options.go

@@ -38,7 +38,7 @@ type GuardianOption struct {
 }
 
 // GuardianOptionP2P configures p2p networking.
-// Dependencies: Accountant, Governor
+// Dependencies: See below.
 func GuardianOptionP2P(
 	p2pKey libp2p_crypto.PrivKey,
 	networkId string,
@@ -51,14 +51,14 @@ func GuardianOptionP2P(
 	ccqPort uint,
 	ccqAllowedPeers string,
 	gossipAdvertiseAddress string,
-	ibcFeaturesFunc func() string,
+	ibcEnabled bool,
 	protectedPeers []string,
 	ccqProtectedPeers []string,
 	featureFlags []string,
 ) *GuardianOption {
 	return &GuardianOption{
 		name:         "p2p",
-		dependencies: []string{"accountant", "governor", "gateway-relayer"},
+		dependencies: []string{"accountant", "alternate-publisher", "gateway-relayer", "governor", "query"},
 		f: func(ctx context.Context, logger *zap.Logger, g *G) error {
 			components := p2p.DefaultComponents()
 			components.Port = port
@@ -77,9 +77,14 @@ func GuardianOptionP2P(
 			// Add the gossip advertisement address
 			components.GossipAdvertiseAddress = gossipAdvertiseAddress
 
-			var alternatePublisherFeaturesFunc func() string
-			if g.alternatePublisher != nil {
-				alternatePublisherFeaturesFunc = g.alternatePublisher.GetFeatures
+			// Get the static feature flags and add them to what was passed in.
+			featureFlags = getStaticFeatureFlags(g, featureFlags)
+
+			// Create the list of dynamic feature flag functions.
+			featureFlagFuncs := []func() string{}
+			if ibcEnabled {
+				// IBC has a dynamic feature flag because it reports the Wormchain version.
+				featureFlagFuncs = append(featureFlagFuncs, ibc.GetFeatures)
 			}
 
 			params, err := p2p.NewRunParams(
@@ -102,9 +107,7 @@ func GuardianOptionP2P(
 					g.gov,
 					disableHeartbeatVerify,
 					components,
-					ibcFeaturesFunc,
-					(g.gatewayRelayer != nil), // gatewayRelayerEnabled,
-					(g.queryHandler != nil),   // ccqEnabled,
+					(g.queryHandler != nil), // ccqEnabled,
 					g.signedQueryReqC.writeC,
 					g.queryResponsePublicationC.readC,
 					ccqBootstrapPeers,
@@ -113,9 +116,8 @@ func GuardianOptionP2P(
 					protectedPeers,
 					ccqProtectedPeers,
 					featureFlags,
+					featureFlagFuncs,
 				),
-				p2p.WithProcessorFeaturesFunc(processor.GetFeatures),
-				p2p.WithAlternatePublisherFeaturesFunc(alternatePublisherFeaturesFunc),
 			)
 			if err != nil {
 				return err
@@ -619,12 +621,12 @@ func GuardianOptionAlternatePublisher(guardianAddr []byte, configs []string) *Gu
 }
 
 // GuardianOptionProcessor enables the default processor, which is required to make consensus on messages.
-// Dependencies: db, governor, accountant
+// Dependencies: See below.
 func GuardianOptionProcessor(networkId string) *GuardianOption {
 	return &GuardianOption{
 		name: "processor",
 		// governor and accountant may be set to nil, but that choice needs to be made before the processor is configured
-		dependencies: []string{"db", "governor", "accountant", "gateway-relayer", "alternate-publisher"},
+		dependencies: []string{"accountant", "alternate-publisher", "db", "gateway-relayer", "governor"},
 
 		f: func(ctx context.Context, logger *zap.Logger, g *G) error {
 
@@ -650,3 +652,33 @@ func GuardianOptionProcessor(networkId string) *GuardianOption {
 			return nil
 		}}
 }
+
+// getStaticFeatureFlags creates the list of feature flags that do not change after initialization and adds them to the ones passed in.
+// Note: Any objects referenced here should be listed as dependencies in `GuardianOptionP2P`.
+func getStaticFeatureFlags(g *G, featureFlags []string) []string {
+	if g.gov != nil {
+		flag := "gov"
+		if g.gov.IsFlowCancelEnabled() {
+			flag = "gov:fc"
+		}
+		featureFlags = append(featureFlags, flag)
+	}
+
+	if g.acct != nil {
+		featureFlags = append(featureFlags, g.acct.FeatureString())
+	}
+
+	if g.queryHandler != nil {
+		featureFlags = append(featureFlags, "ccq")
+	}
+
+	if g.gatewayRelayer != nil {
+		featureFlags = append(featureFlags, "gwrelayer")
+	}
+
+	if g.alternatePublisher != nil {
+		featureFlags = append(featureFlags, g.alternatePublisher.GetFeatures())
+	}
+
+	return featureFlags
+}

node/pkg/p2p/p2p.go

@@ -5,6 +5,7 @@ import (
 	"encoding/hex"
 	"errors"
 	"fmt"
+	"slices"
 	"strings"
 	"sync"
 	"time"
@@ -503,6 +504,7 @@ func Run(params *RunParams) func(ctx context.Context) error {
 
 		// Start up heartbeating if it is enabled.
 		if params.nodeName != "" {
+			slices.Sort(params.featureFlags)
 			go func() {
 				ourAddr := ethcrypto.PubkeyToAddress(params.guardianSigner.PublicKey(ctx))
 
@@ -530,43 +532,16 @@ func Run(params *RunParams) func(ctx context.Context) error {
 								networks = append(networks, v)
 							}
 
-							features := make([]string, 0)
-							if params.processorFeaturesFunc != nil {
-								flag := params.processorFeaturesFunc()
-								if flag != "" {
-									features = append(features, flag)
-								}
-							}
-							if params.alternatePublisherFeaturesFunc != nil {
-								flag := params.alternatePublisherFeaturesFunc()
-								if flag != "" {
-									features = append(features, flag)
-								}
-							}
-							if params.gov != nil {
-								if params.gov.IsFlowCancelEnabled() {
-									features = append(features, "governor:fc")
-								} else {
-									features = append(features, "governor")
-								}
-							}
-							if params.acct != nil {
-								features = append(features, params.acct.FeatureString())
-							}
-							if params.ibcFeaturesFunc != nil {
-								ibcFlags := params.ibcFeaturesFunc()
-								if ibcFlags != "" {
-									features = append(features, ibcFlags)
+							features := make([]string, len(params.featureFlags))
+							copy(features, params.featureFlags)
+							if len(params.featureFlagFuncs) != 0 {
+								for _, f := range params.featureFlagFuncs {
+									flag := f()
+									if flag != "" {
+										features = append(features, flag)
+									}
 								}
-							}
-							if params.gatewayRelayerEnabled {
-								features = append(features, "gwrelayer")
-							}
-							if params.ccqEnabled {
-								features = append(features, "ccq")
-							}
-							if len(params.featureFlags) != 0 {
-								features = append(features, params.featureFlags...)
+								slices.Sort(features)
 							}
 
 							heartbeat := &gossipv1.Heartbeat{

node/pkg/p2p/run_params.go

@@ -42,28 +42,25 @@ type (
 		disableHeartbeatVerify bool
 
 		// The following options are guardian specific. Set with `WithGuardianOptions`.
-		nodeName                       string
-		guardianSigner                 guardiansigner.GuardianSigner
-		gossipControlSendC             chan []byte
-		gossipAttestationSendC         chan []byte
-		gossipVaaSendC                 chan []byte
-		obsvReqSendC                   <-chan *gossipv1.ObservationRequest
-		acct                           *accountant.Accountant
-		gov                            *governor.ChainGovernor
-		components                     *Components
-		ibcFeaturesFunc                func() string
-		processorFeaturesFunc          func() string
-		alternatePublisherFeaturesFunc func() string
-		gatewayRelayerEnabled          bool
-		ccqEnabled                     bool
-		signedQueryReqC                chan<- *gossipv1.SignedQueryRequest
-		queryResponseReadC             <-chan *query.QueryResponsePublication
-		ccqBootstrapPeers              string
-		ccqPort                        uint
-		ccqAllowedPeers                string
-		protectedPeers                 []string
-		ccqProtectedPeers              []string
-		featureFlags                   []string
+		nodeName               string
+		guardianSigner         guardiansigner.GuardianSigner
+		gossipControlSendC     chan []byte
+		gossipAttestationSendC chan []byte
+		gossipVaaSendC         chan []byte
+		obsvReqSendC           <-chan *gossipv1.ObservationRequest
+		acct                   *accountant.Accountant
+		gov                    *governor.ChainGovernor
+		components             *Components
+		ccqEnabled             bool
+		signedQueryReqC        chan<- *gossipv1.SignedQueryRequest
+		queryResponseReadC     <-chan *query.QueryResponsePublication
+		ccqBootstrapPeers      string
+		ccqPort                uint
+		ccqAllowedPeers        string
+		protectedPeers         []string
+		ccqProtectedPeers      []string
+		featureFlags           []string
+		featureFlagFuncs       []func() string
 	}
 
 	// RunOpt is used to specify optional parameters.
@@ -110,22 +107,6 @@ func WithComponents(components *Components) RunOpt {
 	}
 }
 
-// WithProcessorFeaturesFunc is used to set the processor features function.
-func WithProcessorFeaturesFunc(processorFeaturesFunc func() string) RunOpt {
-	return func(p *RunParams) error {
-		p.processorFeaturesFunc = processorFeaturesFunc
-		return nil
-	}
-}
-
-// WithAlternatePublisherFeaturesFunc is used to set the alternate publisher features function.
-func WithAlternatePublisherFeaturesFunc(alternatePublisherFeaturesFunc func() string) RunOpt {
-	return func(p *RunParams) error {
-		p.alternatePublisherFeaturesFunc = alternatePublisherFeaturesFunc
-		return nil
-	}
-}
-
 // WithSignedObservationBatchListener is used to set the channel to receive `SignedObservationBatch` messages.
 func WithSignedObservationBatchListener(batchObsvC chan<- *common.MsgWithTimeStamp[gossipv1.SignedObservationBatch]) RunOpt {
 	return func(p *RunParams) error {
@@ -205,8 +186,6 @@ func WithGuardianOptions(
 	gov *governor.ChainGovernor,
 	disableHeartbeatVerify bool,
 	components *Components,
-	ibcFeaturesFunc func() string,
-	gatewayRelayerEnabled bool,
 	ccqEnabled bool,
 	signedQueryReqC chan<- *gossipv1.SignedQueryRequest,
 	queryResponseReadC <-chan *query.QueryResponsePublication,
@@ -216,6 +195,7 @@ func WithGuardianOptions(
 	protectedPeers []string,
 	ccqProtectedPeers []string,
 	featureFlags []string,
+	featureFlagFuncs []func() string,
 ) RunOpt {
 	return func(p *RunParams) error {
 		p.nodeName = nodeName
@@ -231,8 +211,6 @@ func WithGuardianOptions(
 		p.gov = gov
 		p.disableHeartbeatVerify = disableHeartbeatVerify
 		p.components = components
-		p.ibcFeaturesFunc = ibcFeaturesFunc
-		p.gatewayRelayerEnabled = gatewayRelayerEnabled
 		p.ccqEnabled = ccqEnabled
 		p.signedQueryReqC = signedQueryReqC
 		p.queryResponseReadC = queryResponseReadC
@@ -242,6 +220,7 @@ func WithGuardianOptions(
 		p.protectedPeers = protectedPeers
 		p.ccqProtectedPeers = ccqProtectedPeers
 		p.featureFlags = featureFlags
+		p.featureFlagFuncs = featureFlagFuncs
 		return nil
 	}
 }

node/pkg/p2p/run_params_test.go

@@ -201,8 +201,6 @@ func TestRunParamsWithGuardianOptions(t *testing.T) {
 	gov := &governor.ChainGovernor{}
 	disableHeartbeatVerify := false
 	components := &Components{}
-	ibcFeaturesFunc := func() string { return "Hello, World!" }
-	gatewayRelayerEnabled := true
 
 	ccqEnabled := true
 	signedQueryReqC := make(chan<- *gossipv1.SignedQueryRequest, 42)
@@ -233,8 +231,6 @@ func TestRunParamsWithGuardianOptions(t *testing.T) {
 			gov,
 			disableHeartbeatVerify,
 			components,
-			ibcFeaturesFunc,
-			gatewayRelayerEnabled,
 			ccqEnabled,
 			signedQueryReqC,
 			queryResponseReadC,
@@ -243,7 +239,8 @@ func TestRunParamsWithGuardianOptions(t *testing.T) {
 			ccqAllowedPeers,
 			protectedPeers,
 			ccqProtectedPeers,
-			[]string{}, // featureFlags
+			[]string{},        // featureFlags
+			[]func() string{}, // featureFlagFuncs
 		),
 	)
 
@@ -259,8 +256,6 @@ func TestRunParamsWithGuardianOptions(t *testing.T) {
 	assert.Equal(t, acct, params.acct)
 	assert.Equal(t, gov, params.gov)
 	assert.Equal(t, components, params.components)
-	assert.NotNil(t, params.ibcFeaturesFunc) // Can't compare function pointers, so just verify it's set.
-	assert.True(t, params.gatewayRelayerEnabled)
 	assert.True(t, params.ccqEnabled)
 	assert.Equal(t, signedQueryReqC, params.signedQueryReqC)
 	assert.Equal(t, queryResponseReadC, params.queryResponseReadC)

node/pkg/p2p/watermark_test.go

@@ -190,17 +190,16 @@ func startGuardian(t *testing.T, ctx context.Context, g *G) {
 			g.gov,
 			g.disableHeartbeatVerify,
 			g.components,
-			nil,        //g.ibcFeaturesFunc,
-			false,      // gateway relayer enabled
-			false,      // ccqEnabled
-			nil,        // signed query request channel
-			nil,        // query response channel
-			"",         // query bootstrap peers
-			0,          // query port
-			"",         // query allowed peers),
-			[]string{}, // protected peers
-			[]string{}, // ccq protected peers
-			[]string{}, // featureFlags
+			false,             // ccqEnabled
+			nil,               // signed query request channel
+			nil,               // query response channel
+			"",                // query bootstrap peers
+			0,                 // query port
+			"",                // query allowed peers),
+			[]string{},        // protected peers
+			[]string{},        // ccq protected peers
+			[]string{},        // featureFlags
+			[]func() string{}, // featureFlagFuncs
 		))
 	require.NoError(t, err)
 

node/pkg/processor/processor.go

@@ -447,8 +447,3 @@ func (p *Processor) vaaWriter(ctx context.Context) error {
 		}
 	}
 }
-
-// GetFeatures returns the processor feature string that can be published in heartbeat messages.
-func GetFeatures() string {
-	return ""
-}

node/pkg/watchers/ibc/watcher.go

@@ -165,16 +165,9 @@ func NewWatcher(
 		}
 
 		chainMap[ce.chainID] = ce
-
-		if feats == "" {
-			feats = "ibc:"
-		} else {
-			feats += "|"
-		}
-		feats += ce.chainID.String()
 	}
 
-	setFeatures(feats)
+	setFeatures("ibc")
 
 	return &Watcher{
 		wsUrl:                 wsUrl,
@@ -429,7 +422,7 @@ func (w *Watcher) handleQueryBlockHeight(ctx context.Context, queryUrl string) e
 			}
 
 			readiness.SetReady(common.ReadinessIBCSyncing)
-			setFeatures(w.baseFeatures + ":" + abciInfo.Result.Response.Version)
+			setFeatures("ibc:" + abciInfo.Result.Response.Version)
 		}
 	}
 }