1891 Gossipsub RPC validation inspector false positive E2E testing #4371

kc1116 · 2023-05-19T04:01:33Z

This PR adds an E2E test that covers false positive validation errors in the gossipsub RPC control message validation inspector. This is crucial since the current implementation relies on certain assumptions about GossipSub. These assumptions are based on our analysis of the GossipSub code, but the GossipSub team does not guarantee that it will hold in the long term. This PR leverages the fact that an invalid control message notification is distributed by the rpc inspector notification distributor, under normal operations no invalid control message notifications are expected to be disseminated.

…esting

codecov-commenter · 2023-05-19T04:09:36Z

Codecov Report

Merging #4371 (67d0eef) into master (18b4de2) will increase coverage by 7.52%.
The diff coverage is n/a.

❗ Current head 67d0eef differs from pull request most recent head e07f51d. Consider uploading reports for the commit e07f51d to get more accurate results

@@            Coverage Diff             @@
##           master    #4371      +/-   ##
==========================================
+ Coverage   54.14%   61.66%   +7.52%     
==========================================
  Files         888      170     -718     
  Lines       83754    18985   -64769     
==========================================
- Hits        45348    11708   -33640     
+ Misses      34888     6364   -28524     
+ Partials     3518      913    -2605

Flag	Coverage Δ
unittests	`61.66% <ø> (+7.52%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 726 files with indirect coverage changes

…esting

yhassanzadeh13

I like the general approach that the test is taking, though a few concerns need to be addressed. Appreciate your consideration; thanks!

integration/testnet/network.go

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

integration/tests/bft/gossipsub/rpc_inspector/false_positive_test.go

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

gomisha · 2023-06-02T13:09:39Z

integration/tests/bft/base_suite.go

@@ -102,7 +102,9 @@ func (b *BaseSuite) SetupSuite() {
 func (b *BaseSuite) TearDownSuite() {
 	b.Net.Remove()
 	b.Cancel()
-	unittest.RequireCloseBefore(b.T(), b.OrchestratorNetwork.Done(), 1*time.Second, "could not stop orchestrator network on time")
+	if b.OrchestratorNetwork != nil {


this is needed because the BaseSuite is now being used by integration BFT tests outside BFTune, so not every BFT integration test will use an Orchestrator? Might be good to add a comment addressing this.

gomisha · 2023-06-02T13:20:49Z

integration/tests/bft/gossipsub/rpc_inspector/false_positive_test.go

+	suite.Run(t, new(GossipsubRPCInstpectorFalePostiveNotificationsTestSuite))
+}
+
+// TestGossipsubRPCInstpectorFalePostiveNotifications this test ensures that any underlying changes or updates to any of the underlying libp2p libraries


multiple typos in function name at start of comment (TestGossipsubRPCInstpectorFalePostiveNotifications should be TestGossipsubRPCInspectorFalsePositiveNotifications) along with actual function name that was already flagged in https://github.com/onflow/flow-go/pull/4371/files#r1210747930

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

…est.go Co-authored-by: Yahya Hassanzadeh <yhassanzadeh@ieee.org>

…thub.com:onflow/flow-go into khalil/1891-rpc-inspector-false-positive-testing

Co-authored-by: Yahya Hassanzadeh <yhassanzadeh@ieee.org>

…thub.com:onflow/flow-go into khalil/1891-rpc-inspector-false-positive-testing

Co-authored-by: Yahya Hassanzadeh <yhassanzadeh@ieee.org>

…thub.com:onflow/flow-go into khalil/1891-rpc-inspector-false-positive-testing

…est.go Co-authored-by: Yahya Hassanzadeh <yhassanzadeh@ieee.org>

- wait for n number of finalized heights before checking metrics

…thub.com:onflow/flow-go into khalil/1891-rpc-inspector-false-positive-testing

…transaction

Co-authored-by: Misha <misha.rybalov@dapperlabs.com>

…esting

…thub.com:onflow/flow-go into khalil/1891-rpc-inspector-false-positive-testing

yhassanzadeh13

🚀 thanks for developing the test. Except for one correction that I pointed out, the rest looks great!

yhassanzadeh13 · 2023-06-09T21:59:15Z

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

+	require.Eventually(s.T(), func() bool {
+		currStateComm := s.getCurrERFinalStateCommitment(ctx)
+		if startStateComm != currStateComm {
+			numOfStateCommChanges++


Suggested change

numOfStateCommChanges++

numOfStateCommChanges++

startStateComm = currStateComm

⚠️ Correction Needed: It's imperative to update the startStateComm whenever the current state commitment changes. This ensures we accurately evaluate the expected n times state changes. The existing method is problematic because it constantly compares the current state with the start state. Consequently, even a single state transition would pass this test, which is different from the intended behavior.

Additionally, as a suggestion for improving clarity, renaming startStateComm to prevStateComm would be more descriptive and appropriate.

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

gomisha

Thanks for addressing all my feedback.

- reduce number of state commits to wait for

Co-authored-by: Yahya Hassanzadeh <yhassanzadeh@ieee.org>

…thub.com:onflow/flow-go into khalil/1891-rpc-inspector-false-positive-testing

…esting

add false positive testing for rpc validation inspector

dc06e9f

kc1116 requested review from yhassanzadeh13 and peterargue May 19, 2023 04:01

kc1116 added 2 commits May 19, 2023 00:03

lint fixes

f4f9c48

Merge branch 'master' into khalil/1891-rpc-inspector-false-positive-t…

393fe41

…esting

kc1116 requested review from gomisha and removed request for peterargue May 19, 2023 13:54

kc1116 added 2 commits May 20, 2023 03:13

Merge branch 'master' into khalil/1891-rpc-inspector-false-positive-t…

8e6d863

…esting

Merge branch 'master' into khalil/1891-rpc-inspector-false-positive-t…

f30332f

…esting

yhassanzadeh13 reviewed May 30, 2023

View reviewed changes

gomisha reviewed Jun 2, 2023

View reviewed changes

integration/tests/bft/gossipsub/rpc_inspector/suite.go Outdated Show resolved Hide resolved

kc1116 and others added 16 commits June 6, 2023 09:13

rename containerName -> cName

56e4b41

rename port -> portName

45de641

rename mets -> metricEndpoints

6174b18

Update integration/tests/bft/gossipsub/rpc_inspector/false_positive_t…

730a8f4

…est.go Co-authored-by: Yahya Hassanzadeh <yhassanzadeh@ieee.org>

Merge branch 'khalil/1891-rpc-inspector-false-positive-testing' of gi…

be2d75c

…thub.com:onflow/flow-go into khalil/1891-rpc-inspector-false-positive-testing

Update integration/testnet/network.go

b75ebab

Co-authored-by: Yahya Hassanzadeh <yhassanzadeh@ieee.org>

Update integration/testnet/network.go

fedb11d

Co-authored-by: Yahya Hassanzadeh <yhassanzadeh@ieee.org>

Merge branch 'khalil/1891-rpc-inspector-false-positive-testing' of gi…

b360299

…thub.com:onflow/flow-go into khalil/1891-rpc-inspector-false-positive-testing

Update network.go

c24feeb

Update integration/tests/bft/gossipsub/rpc_inspector/suite.go

ca54256

Co-authored-by: Yahya Hassanzadeh <yhassanzadeh@ieee.org>

Merge branch 'khalil/1891-rpc-inspector-false-positive-testing' of gi…

9cd8505

…thub.com:onflow/flow-go into khalil/1891-rpc-inspector-false-positive-testing

Update integration/tests/bft/gossipsub/rpc_inspector/false_positive_t…

27c43d3

…est.go Co-authored-by: Yahya Hassanzadeh <yhassanzadeh@ieee.org>

Update integration/tests/bft/gossipsub/rpc_inspector/false_positive_t…

b64766a

…est.go Co-authored-by: Yahya Hassanzadeh <yhassanzadeh@ieee.org>

add loader loop to suite

3678788

- wait for n number of finalized heights before checking metrics

Merge branch 'khalil/1891-rpc-inspector-false-positive-testing' of gi…

fd0a1dc

…thub.com:onflow/flow-go into khalil/1891-rpc-inspector-false-positive-testing

wait for n number of state commitment changes to ensure execution of …

6da7dd0

…transaction

kc1116 and others added 8 commits June 6, 2023 17:37

Update false_positive_test.go

c4fe2f5

add base suite comment

433e266

fix typos

05f793b

Update integration/tests/bft/gossipsub/rpc_inspector/suite.go

ec7e380

Co-authored-by: Misha <misha.rybalov@dapperlabs.com>

Update false_positive_test.go

884579d

Merge branch 'master' into khalil/1891-rpc-inspector-false-positive-t…

c0211fa

…esting

increase number of test accounts and state commitments

c000dae

Merge branch 'khalil/1891-rpc-inspector-false-positive-testing' of gi…

c692cd7

…thub.com:onflow/flow-go into khalil/1891-rpc-inspector-false-positive-testing

kc1116 requested review from yhassanzadeh13 and gomisha June 6, 2023 21:53

kc1116 added 2 commits June 6, 2023 18:04

Update network.go

38a4137

Update suite.go

7c6a0da

yhassanzadeh13 approved these changes Jun 9, 2023

View reviewed changes

gomisha approved these changes Jun 10, 2023

View reviewed changes

kc1116 and others added 9 commits June 12, 2023 10:13

fix state comm stracking

d24265f

- reduce number of state commits to wait for

Update integration/tests/bft/gossipsub/rpc_inspector/suite.go

025641e

Co-authored-by: Yahya Hassanzadeh <yhassanzadeh@ieee.org>

Update integration/tests/bft/gossipsub/rpc_inspector/suite.go

bcdbe5a

Co-authored-by: Yahya Hassanzadeh <yhassanzadeh@ieee.org>

Merge branch 'khalil/1891-rpc-inspector-false-positive-testing' of gi…

980a6fd

…thub.com:onflow/flow-go into khalil/1891-rpc-inspector-false-positive-testing

Update suite.go

b08bab7

Update false_positive_test.go

40bcf18

Merge branch 'master' into khalil/1891-rpc-inspector-false-positive-t…

583905c

…esting

Update network.go

67d0eef

lint fix

e07f51d

kc1116 merged commit 463ed82 into master Jun 12, 2023

kc1116 deleted the khalil/1891-rpc-inspector-false-positive-testing branch June 12, 2023 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1891 Gossipsub RPC validation inspector false positive E2E testing #4371

1891 Gossipsub RPC validation inspector false positive E2E testing #4371

kc1116 commented May 19, 2023

codecov-commenter commented May 19, 2023 •

edited

Loading

yhassanzadeh13 left a comment

gomisha Jun 2, 2023

kc1116 Jun 6, 2023

gomisha Jun 2, 2023

kc1116 Jun 6, 2023

yhassanzadeh13 left a comment

yhassanzadeh13 Jun 9, 2023 •

edited

Loading

kc1116 Jun 12, 2023

gomisha left a comment

	numOfStateCommChanges++
	numOfStateCommChanges++
	startStateComm = currStateComm

1891 Gossipsub RPC validation inspector false positive E2E testing #4371

1891 Gossipsub RPC validation inspector false positive E2E testing #4371

Conversation

kc1116 commented May 19, 2023

codecov-commenter commented May 19, 2023 • edited Loading

Codecov Report

yhassanzadeh13 left a comment

Choose a reason for hiding this comment

gomisha Jun 2, 2023

Choose a reason for hiding this comment

kc1116 Jun 6, 2023

Choose a reason for hiding this comment

gomisha Jun 2, 2023

Choose a reason for hiding this comment

kc1116 Jun 6, 2023

Choose a reason for hiding this comment

yhassanzadeh13 left a comment

Choose a reason for hiding this comment

yhassanzadeh13 Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

kc1116 Jun 12, 2023

Choose a reason for hiding this comment

gomisha left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 19, 2023 •

edited

Loading

yhassanzadeh13 Jun 9, 2023 •

edited

Loading