Merge pull request #4652 from onflow/yahya/6857-bft-testing-ci-fix-2

[BFT Testing] Reducing load of BFT tests and improving test packages

integration/testnet/node_config.go

@@ -32,12 +32,15 @@ type NodeConfig struct {
 func (n NodeConfigs) Filter(filters ...NodeConfigFilter) NodeConfigs {
 	nodeConfigs := make(NodeConfigs, 0)
 	for _, config := range n {
-		filter := false
+		passedAllFilters := true
 		for _, f := range filters {
-			filter = f(config)
+			if !f(config) {
+				passedAllFilters = false
+				break
+			}
 		}
 
-		if filter {
+		if passedAllFilters {
 			nodeConfigs = append(nodeConfigs, config)
 		}
 	}

integration/testnet/node_config_test.go

@@ -0,0 +1,49 @@
+package testnet_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/onflow/flow-go/integration/testnet"
+	"github.com/onflow/flow-go/model/flow"
+)
+
+func TestFilter(t *testing.T) {
+	t.Run("filters by role", func(t *testing.T) {
+		configs := testnet.NewNodeConfigSet(5, flow.RoleAccess)
+
+		// add another role to the set to ensure it is filtered out
+		configs = append(configs, testnet.NewNodeConfig(flow.RoleExecution))
+		filters := configs.Filter(func(n testnet.NodeConfig) bool { return n.Role == flow.RoleAccess })
+		assert.Len(t, filters, 5) // should exclude execution node
+		for _, config := range filters {
+			assert.Equal(t, flow.RoleAccess, config.Role)
+		}
+	})
+
+	t.Run("filters by multiple conditions", func(t *testing.T) {
+		configs := testnet.NodeConfigs{
+			testnet.NewNodeConfig(flow.RoleAccess, testnet.WithDebugImage(true)),
+			testnet.NewNodeConfig(flow.RoleExecution, testnet.WithDebugImage(true)),
+		}
+
+		filters := configs.Filter(
+			func(n testnet.NodeConfig) bool { return n.Role == flow.RoleAccess },
+			func(n testnet.NodeConfig) bool {
+				return n.Debug
+			},
+		)
+
+		assert.Len(t, filters, 1) // should exclude execution node
+		assert.True(t, filters[0].Debug)
+	})
+
+	t.Run("no matching filters", func(t *testing.T) {
+		configs := testnet.NewNodeConfigSet(5, flow.RoleConsensus)
+
+		filters := configs.Filter(func(n testnet.NodeConfig) bool { return n.Role == flow.RoleAccess })
+
+		assert.Len(t, filters, 0)
+	})
+}

integration/tests/bft/base_suite.go

@@ -1,3 +1,4 @@
+// Package bft provides testing facilities for Flow BFT protocols.
 package bft
 
 import (
@@ -18,6 +19,19 @@ import (
 	"github.com/onflow/flow-go/utils/unittest"
 )
 
+// BaseSuite serves as a base test suite offering various utility functions
+// and default setup/teardown steps. It facilitates the creation of Flow networks
+// with pre-configured nodes and allows for easier interaction with the network,
+// reducing boilerplate code in individual tests.
+//
+// BaseSuite comes with a lot of functionality out-of-the-box, including the ability to:
+// - Create a bare-minimum Flow network.
+// - Start and stop the network.
+// - Track messages over testnet using TestnetStateTracker.
+// - Tear down the testnet environment.
+// - Handle Ghost nodes and Orchestrator network.
+//
+// BaseSuite embeds testify's Suite to leverage setup, teardown, and assertion capabilities.
 type BaseSuite struct {
 	suite.Suite
 	Log                     zerolog.Logger
@@ -30,32 +44,34 @@ type BaseSuite struct {
 	OrchestratorNetwork     *orchestrator.Network
 }
 
-// Ghost returns a client to interact with the Ghost node on testnet.
+// Ghost returns a client to interact with the Ghost node on the testnet.
+// It is essential for observing the messages exchanged in the network.
 func (b *BaseSuite) Ghost() *client.GhostClient {
-	client, err := b.Net.ContainerByID(b.GhostID).GhostClient()
+	c, err := b.Net.ContainerByID(b.GhostID).GhostClient()
 	require.NoError(b.T(), err, "could not get ghost client")
-	return client
+	return c
 }
 
 // AccessClient returns a client to interact with the access node api on testnet.
 func (b *BaseSuite) AccessClient() *testnet.Client {
-	client, err := b.Net.ContainerByName(testnet.PrimaryAN).TestnetClient()
+	c, err := b.Net.ContainerByName(testnet.PrimaryAN).TestnetClient()
 	require.NoError(b.T(), err, "could not get access client")
-	return client
+	return c
 }
 
-// SetupSuite sets up node configs to run a bare minimum Flow network to function correctly.
+// SetupSuite initializes the BaseSuite, setting up a bare-minimum Flow network.
+// It configures nodes with roles such as access, consensus, verification, execution,
+// and collection. It also sets up a Ghost node for observing messages exchanged on the network.
 func (b *BaseSuite) SetupSuite() {
 	b.Log = unittest.LoggerForTest(b.Suite.T(), zerolog.InfoLevel)
 
-	// setup access nodes
+	// setup single access node
 	b.NodeConfigs = append(b.NodeConfigs,
 		testnet.NewNodeConfig(flow.RoleAccess, testnet.WithLogLevel(zerolog.FatalLevel)),
-		testnet.NewNodeConfig(flow.RoleAccess, testnet.WithLogLevel(zerolog.FatalLevel)),
 	)
 
 	// setup consensus nodes
-	for _, nodeID := range unittest.IdentifierListFixture(4) {
+	for _, nodeID := range unittest.IdentifierListFixture(3) {
 		nodeConfig := testnet.NewNodeConfig(flow.RoleConsensus,
 			testnet.WithID(nodeID),
 			testnet.WithLogLevel(zerolog.FatalLevel),
@@ -66,10 +82,9 @@ func (b *BaseSuite) SetupSuite() {
 		b.NodeConfigs = append(b.NodeConfigs, nodeConfig)
 	}
 
-	// setup verification nodes
+	// setup single verification node
 	b.NodeConfigs = append(b.NodeConfigs,
 		testnet.NewNodeConfig(flow.RoleVerification, testnet.WithLogLevel(zerolog.FatalLevel)),
-		testnet.NewNodeConfig(flow.RoleVerification, testnet.WithLogLevel(zerolog.FatalLevel)),
 	)
 
 	// setup execution nodes
@@ -96,7 +111,8 @@ func (b *BaseSuite) SetupSuite() {
 	b.NodeConfigs = append(b.NodeConfigs, ghostConfig)
 }
 
-// TearDownSuite tears down the test network of Flow as well as the BFT testing orchestrator network.
+// TearDownSuite cleans up the resources, stopping both the Flow network and the
+// orchestrator network if they have been initialized.
 func (b *BaseSuite) TearDownSuite() {
 	b.Net.Remove()
 	b.Cancel()
@@ -106,7 +122,10 @@ func (b *BaseSuite) TearDownSuite() {
 	}
 }
 
-// StartCorruptedNetwork starts the corrupted network with the configured node configs, this func should be used after test suite is setup.
+// StartCorruptedNetwork initializes and starts a corrupted Flow network.
+// This should be called after the test suite is set up. The function accepts
+// configurations like the name of the network, the number of views in the staking auction,
+// the number of views in an epoch, and a function to attack the orchestrator.
 func (b *BaseSuite) StartCorruptedNetwork(name string, viewsInStakingAuction, viewsInEpoch uint64, attackOrchestrator func() insecure.AttackOrchestrator) {
 	// generates, initializes, and starts the Flow network
 	netConfig := testnet.NewNetworkConfig(

integration/tests/bft/framework/README.md

@@ -0,0 +1,10 @@
+# Framework BFT Tests
+The framework BFT tests are designed to assess the health of the BFT testing framework. 
+
+## Passthrough Sealing and Verification Test
+The `PassThroughTestSuite` test within the `framework` package includes a specific test method `TestSealingAndVerificationPassThrough`.
+This test evaluates the health of the BFT testing framework for Byzantine Fault Tolerance (BFT) testing.
+1. **Simulates a Scenario**: It sets up a scenario with two corrupt execution nodes and one corrupt verification node, controlled by a dummy orchestrator that lets all incoming events pass through.
+2. **Deploys Transaction and Verifies Chunks**: Deploys a transaction leading to an execution result with multiple chunks, assigns them to a verification node, and verifies the generation of result approvals for all chunks.
+3. **Sealing and Verification**: Enables sealing based on result approvals and verifies the sealing of a block with a specific multi-chunk execution result.
+4. **Evaluates Events**: The test also assesses whether critical sealing-and-verification-related events from corrupt nodes are passed through the orchestrator, by checking both egress and ingress events.

integration/tests/bft/gossipsub/README.md

@@ -0,0 +1,32 @@
+# GossipSub BFT Tests
+GossipSub BFT tests are designed to test the behavior of the GossipSub protocol in a network environment with Byzantine nodes.
+
+## Topic Validator Test
+The `TopicValidatorTestSuite` in the `topicvalidator` package is specifically designed to test the functionality of the
+libp2p topic validator within a network scenario.
+This suite includes an end-to-end test to verify the topic validator's behavior in different situations, 
+focusing on both unauthorized and authorized message handling.
+The method `TestTopicValidatorE2E` is a comprehensive test that mimics an environment with a corrupt byzantine attacker node
+attempting to send unauthorized messages to a victim node. 
+These messages should be dropped by the topic validator, as they fail the message authorization validation. 
+The test simultaneously sends authorized messages to the victim node, ensuring that they are processed correctly, 
+demonstrating the validator's correct operation.
+The test confirms two main aspects:
+1. Unauthorized messages must be dropped, and the victim node should not receive any of them.
+2. Authorized messages should be correctly delivered and processed by the victim node.
+
+## Signature Requirement Test
+The `TestGossipSubSignatureRequirement` test sets up a test environment consisting of three corrupt nodes: two attackers and one victim.
+One (malicious) attacker is configured without message signing, intending to send unsigned messages that should be rejected by the victim.
+The other (benign) attacker sends valid signed messages that should be received by the victim.
+The test is broken down into the following main parts:
+1. **Unauthorized Messages Testing**: The victim node should not receive any messages sent without correct signatures from the unauthorized attacker. The test checks for zero unauthorized messages received by the victim.
+2. **Authorized Messages Testing**: Messages sent by the authorized attacker, with the correct signature, must pass the libp2p signature verification process and be delivered to the victim. The test checks for all authorized messages received by the victim within a certain time frame.
+
+## RPC Inspector False Positive Test
+The `GossipsubRPCInspectorFalsePositiveNotificationsTestSuite` test within the `rpc_inspector` package test suite aims to ensure that the underlying libp2p libraries related to GossipSub RPC control message inspection do not trigger false positives during their validation processes.
+Here's a breakdown of the `TestGossipsubRPCInspectorFalsePositiveNotifications` method:
+1. **Configuration and Context Setup**: A specific duration for loading and intervals is defined, and a context with a timeout is created for the test scenario.
+2. **Simulating Network Activity**: The method triggers a "loader loop" with a specific number of test accounts and intervals, intending to create artificial network activity. It does this by submitting transactions to create Flow accounts, waiting for them to be sealed.
+3. **State Commitments**: The method waits for 20 state commitment changes, ensuring that the simulated network load behaves as expected.
+4. **Verification of Control Messages**: After simulating network activity, the method checks to ensure that no node in the network has disseminated an invalid control message notification. This is done by collecting metrics from the network containers and verifying that no false notifications are detected.

integration/tests/bft/gossipsub/signature/requirement/orchestrator.go

@@ -21,10 +21,16 @@ const (
 	// The numOfAuthorizedEvents allows us to wait for a certain number of authorized messages to be received, this should
 	// give the network enough time to process the unauthorized messages. This ensures us that the unauthorized messages
 	// were indeed dropped and not unprocessed.
-	numOfAuthorizedEvents = 50
+	// This threshold must be set to a low value to make the test conclude faster
+	// by waiting for fewer events, which is beneficial when running the test
+	// on an asynchronous network where event delivery can be unpredictable.
+	numOfAuthorizedEvents = 5
 
 	// numOfUnauthorizedEvents the number of unauthorized events to send by the test orchestrator.
-	numOfUnauthorizedEvents = 10
+	// This threshold must be set to a low value to make the test conclude faster
+	// by waiting for fewer events, which is beneficial when running the test
+	// on an asynchronous network where event delivery can be unpredictable.
+	numOfUnauthorizedEvents = 5
 )
 
 // Orchestrator represents a simple `insecure.AttackOrchestrator` that tracks any unsigned messages received by victim nodes as well as the typically expected messages.

integration/tests/bft/gossipsub/signature/requirement/signature_requirement_test.go

@@ -35,5 +35,7 @@ func (s *GossipSubSignatureRequirementTestSuite) TestGossipSubSignatureRequireme
 	require.Equal(s.T(), int64(0), s.Orchestrator.unauthorizedEventsReceived.Load(), fmt.Sprintf("expected to not receive any unauthorized messages instead got: %d", s.Orchestrator.unauthorizedEventsReceived.Load()))
 
 	// messages with correct message signatures are expected to always pass libp2p signature verification and be delivered to the victim EN.
-	require.Equal(s.T(), int64(numOfAuthorizedEvents), s.Orchestrator.authorizedEventsReceived.Load(), fmt.Sprintf("expected to receive %d authorized events got: %d", numOfAuthorizedEvents, s.Orchestrator.unauthorizedEventsReceived.Load()))
+	require.Eventually(s.T(), func() bool {
+		return s.Orchestrator.authorizedEventsReceived.Load() == int64(numOfAuthorizedEvents)
+	}, 5*time.Second, 500*time.Millisecond, fmt.Sprintf("expected to receive %d authorized events got: %d", numOfAuthorizedEvents, s.Orchestrator.unauthorizedEventsReceived.Load()))
 }

integration/tests/bft/gossipsub/topicvalidator/orchestrator.go

@@ -21,10 +21,16 @@ const (
 	// The numOfAuthorizedEvents allows us to wait for a certain number of authorized messages to be received, this should
 	// give the network enough time to process the unauthorized messages. This ensures us that the unauthorized messages
 	// were indeed dropped and not unprocessed.
-	numOfAuthorizedEvents = 100
+	// This threshold must be set to a low value to make the test conclude faster
+	// by waiting for fewer events, which is beneficial when running the test
+	// on an asynchronous network where event delivery can be unpredictable.
+	numOfAuthorizedEvents = 5
 
 	// numOfUnauthorizedEvents the number of unauthorized events per type to send by the test orchestrator.
-	numOfUnauthorizedEvents = 10
+	// This threshold must be set to a low value to make the test conclude faster
+	// by waiting for fewer events, which is beneficial when running the test
+	// on an asynchronous network where event delivery can be unpredictable.
+	numOfUnauthorizedEvents = 5
 )
 
 // Orchestrator represents an insecure.AttackOrchestrator track incoming unauthorized messages and authorized messages received by victim nodes.

integration/tests/bft/gossipsub/topicvalidator/suite.go

@@ -38,8 +38,6 @@ func (s *Suite) SetupSuite() {
 		return n.Role != flow.RoleExecution && n.Role != flow.RoleVerification && n.Role != flow.RoleAccess
 	})
 
-	s.NodeConfigs = append(s.NodeConfigs, testnet.NewNodeConfig(flow.RoleAccess, testnet.WithLogLevel(zerolog.FatalLevel)))
-
 	// create corrupt access node
 	s.attackerANID = unittest.IdentifierFixture()
 	s.NodeConfigs = append(s.NodeConfigs, testnet.NewNodeConfig(flow.RoleAccess,

integration/tests/bft/gossipsub/topicvalidator/topic_validator_test.go

@@ -28,12 +28,23 @@ func TestTopicValidator(t *testing.T) {
 func (s *TopicValidatorTestSuite) TestTopicValidatorE2E() {
 	s.Orchestrator.sendUnauthorizedMsgs(s.T())
 	s.Orchestrator.sendAuthorizedMsgs(s.T())
-	unittest.RequireReturnsBefore(s.T(), s.Orchestrator.authorizedEventReceivedWg.Wait, 5*time.Second, "could not send authorized messages on time")
+	unittest.RequireReturnsBefore(
+		s.T(),
+		s.Orchestrator.authorizedEventReceivedWg.Wait,
+		5*time.Second,
+		"could not send authorized messages on time")
 
 	// Victim nodes are configured with the topic validator enabled, therefore they should not have
 	// received any of the unauthorized messages.
-	require.Equal(s.T(), 0, len(s.Orchestrator.unauthorizedEventsReceived), fmt.Sprintf("expected to not receive any unauthorized messages instead got: %d", len(s.Orchestrator.unauthorizedEventsReceived)))
+	require.Equal(
+		s.T(),
+		0,
+		len(s.Orchestrator.unauthorizedEventsReceived),
+		fmt.Sprintf("expected to not receive any unauthorized messages instead got: %d", len(s.Orchestrator.unauthorizedEventsReceived)))
 
 	// Victim nodes should receive all the authorized events sent.
-	require.Equal(s.T(), numOfAuthorizedEvents, len(s.Orchestrator.authorizedEventsReceived), fmt.Sprintf("expected to receive %d authorized events got: %d", numOfAuthorizedEvents, len(s.Orchestrator.unauthorizedEventsReceived)))
+	require.Eventually(s.T(), func() bool {
+		return len(s.Orchestrator.authorizedEventsReceived) == numOfAuthorizedEvents
+	}, 5*time.Second, 500*time.Millisecond,
+		fmt.Sprintf("expected to receive %d authorized events got: %d", numOfAuthorizedEvents, len(s.Orchestrator.unauthorizedEventsReceived)))
 }

integration/tests/bft/protocol/README.md

@@ -0,0 +1,22 @@
+# Protocol BFT Tests
+This package contains BFT tests concerning the core Flow protocol. These tests are run as part of the integration test suite.
+
+
+## Admin Command Disallow List Test
+The `AdminCommandDisallowListTestSuite` in the `disallowlisting` package is designed to test the functionality of the disallow-list admin command within a network context. 
+It ensures that connections to a blocked node are immediately pruned and incoming connection requests are blocked.
+The test simulates the setup of two corrupt nodes (a sender and a receiver) and examines the behavior of the network before and after the sender node is disallowed. 
+It includes steps to send authorized messages to verify normal behavior, implement disallow-listing, send unauthorized messages, and validate the expectation that no unauthorized messages are received. 
+Various timing controls are put in place to handle asynchronous processes and potential race conditions. 
+The entire suite assures that the disallow-listing command behaves as intended, safeguarding network integrity.
+
+## Wintermute Attack Test
+The `WintermuteTestSuite` in the `wintermute` package is focused on validating a specific attack scenario within the network, termed the "wintermute attack." 
+This attack involves an Attack Orchestrator corrupting an execution result and then leveraging corrupt verification nodes to verify it.
+The suite includes a constant timeout to define the attack window and a detailed test sequence.
+The `TestWintermuteAttack` method carries out the attack process. 
+It first waits for an execution result to be corrupted and identifies the corresponding victim block. 
+It ensures that the corrupt execution nodes generate the correct result for the victim block and then waits for a specific number of approvals from corrupt verification nodes for each chunk of the corrupted result.
+Further, the test waits for a block height equal to the victim block height to be sealed and verifies that the original victim block is correctly identified. 
+Additional methods and logging information assist in detailing and controlling the flow of the attack.
+The entire suite is instrumental in evaluating the system's behavior under this specific attack condition and ensuring that the expected actions and responses are observed.