[Util] Add verify execution result cmd #6746

zhangchiqing · 2024-11-20T18:52:40Z

Working towards #6557

This PR implemented option 4 in the above issue.

It adds a util command verify-execution-result that takes execution node's data and verifies every single chunks for a range blocks.

Since we would like to make sure future cadence versions are backward compatible. This util allows us to capture any backward compatibilities issue from either FVM or cadence related changes.

For instance, if cadence introduces a breaking change, then it will be caught by running this verify-execution-result with a latest snapshot of EN. The util can verify the last 1M blocks, and 1 of the chunk might fail caused by the breaking change.

I have verified with the latest testnet snapshot, and it worked. It verified 200K blocks after 3 hours, roughly 18 blocks per sec. The memory needed is about 14G.

Future optimization can be done by parallelizing the verification.

zhangchiqing · 2024-11-20T18:53:27Z

engine/verification/fetcher/engine.go

@@ -661,42 +646,3 @@ func executorsOf(receipts []*flow.ExecutionReceipt, resultID flow.Identifier) (f

 	return agrees, disagrees
 }
-
-// EndStateCommitment computes the end state of the given chunk.
-func EndStateCommitment(result *flow.ExecutionResult, chunkIndex uint64, systemChunk bool) (flow.StateCommitment, error) {


moving these functions to a separate package, so that it can be reused.

codecov-commenter · 2024-11-20T18:56:10Z

Codecov Report

Attention: Patch coverage is 3.35821% with 259 lines in your changes missing coverage. Please review.

Project coverage is 41.17%. Comparing base (fb77044) to head (df4891d).

Files with missing lines	Patch %	Lines
engine/verification/verifier/verifiers.go	0.00%	133 Missing ⚠️
cmd/util/cmd/verify_execution_result/cmd.go	0.00%	56 Missing ⚠️
model/verification/convert/convert.go	0.00%	48 Missing ⚠️
fvm/initialize/options.go	0.00%	18 Missing ⚠️
cmd/scaffold.go	0.00%	3 Missing ⚠️
cmd/util/cmd/root.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6746      +/-   ##
==========================================
- Coverage   41.23%   41.17%   -0.07%     
==========================================
  Files        2061     2065       +4     
  Lines      182702   182905     +203     
==========================================
- Hits        75345    75307      -38     
- Misses     101049   101290     +241     
  Partials     6308     6308

Flag	Coverage Δ
unittests	`41.17% <3.35%> (-0.07%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

peterargue · 2024-11-21T19:50:40Z

cmd/util/cmd/verify_execution_result/cmd.go

+		"last k sealed blocks to verify")
+
+	Cmd.Flags().StringVar(&flagFromTo, "from_to", "",
+		"the height range to verify blocks, i.e, 1-1000, 1000-2000, 2000-3000, etc.")


Suggested change

"the height range to verify blocks, i.e, 1-1000, 1000-2000, 2000-3000, etc.")

"the height range to verify blocks (inclusive), i.e, 1-1000, 1000-2000, 2000-3000, etc.")

peterargue · 2024-11-21T19:56:28Z

engine/verification/verifier/verifiers.go

+
+	blockID := header.ID()
+
+	if err != nil {


looks like something was removed. should the also be removed?

peterargue · 2024-11-21T19:58:50Z

engine/verification/verifier/verifiers.go

+	return chunkVerifier
+}
+
+func initFvmOptions(chainID flow.ChainID, headers storage.Headers) []fvm.Option {


can this reuse code from the standard initialization? I worry that the config will drift and we will need to maintain multiple this init in multiple files

fxamacker

I mostly reviewed from Go programming perspective and left some comments.

fxamacker · 2024-11-27T22:21:54Z

cmd/util/cmd/verify_execution_result/cmd.go

+		}
+
+		log.Info().Msgf("verifying range from %d to %d", from, to)
+		err = verifier.VerifyRange(from, to, flow.Testnet, flagDatadir, flagChunkDataPackDir)


Need to use chain ID provided by user.

Suggested change

err = verifier.VerifyRange(from, to, flow.Testnet, flagDatadir, flagChunkDataPackDir)

err = verifier.VerifyRange(from, to, flow.ChainID(flagChain), flagDatadir, flagChunkDataPackDir)

fxamacker · 2024-11-27T22:22:45Z

cmd/util/cmd/verify_execution_result/cmd.go

+
+	} else {
+		log.Info().Msgf("verifying last %d sealed blocks", flagLastK)
+		err := verifier.VerifyLastKHeight(flagLastK, flow.Testnet, flagDatadir, flagChunkDataPackDir)


Same as above. Need to use chain ID provided by user.

Suggested change

err := verifier.VerifyLastKHeight(flagLastK, flow.Testnet, flagDatadir, flagChunkDataPackDir)

err := verifier.VerifyLastKHeight(flagLastK, flow.ChainID(flagChain), flagDatadir, flagChunkDataPackDir)

fxamacker · 2024-11-27T22:41:06Z

cmd/util/cmd/verify_execution_result/cmd.go

+		log.Info().Msgf("verifying range from %d to %d", from, to)
+		err = verifier.VerifyRange(from, to, flow.Testnet, flagDatadir, flagChunkDataPackDir)
+		if err != nil {
+			log.Fatal().Err(err).Msg("could not verify last k height")


Suggested change

log.Fatal().Err(err).Msg("could not verify last k height")

log.Fatal().Err(err).Msg("could not verify range from %d to %d", from, to)

fxamacker · 2024-11-27T22:54:40Z

engine/verification/verifier/verifiers.go

+	if err != nil {
+		return fmt.Errorf("could not init storages: %w", err)
+	}
+	defer db.Close()


Do we also need to close chunkDataPackDB (e.g. by using "chunkDataPackDB.Close()" in defer)?

Maybe have a separate initChunkDataPack() to return chunkDataPackDB so we can close it in defer.

engine/verification/verifier/verifiers.go

+	}
+
+	root := state.Params().SealedRoot().Height
+	from := lastSealed.Height - k + 1


fxamacker · 2024-11-27T23:14:03Z

engine/verification/verifier/verifiers.go

+	if err != nil {
+		return fmt.Errorf("could not init storages: %w", err)
+	}
+	defer db.Close()


Same as above about closing ChunkDataPackDB.

engine/verification/verifier/verifiers.go

+	}
+	defer db.Close()
+
+	for height := from; height <= to; height++ {


engine/verification/verifier/verifiers.go

cmd/util/cmd/verify_execution_result/cmd.go

+	Cmd.Flags().StringVar(&flagDatadir, "datadir", "/var/flow/data/protocol",
+		"directory that stores the protocol state")


cmd/util/cmd/verify_execution_result/cmd.go

+	Cmd.Flags().StringVar(&flagChunkDataPackDir, "chunk_data_pack_dir", "/var/flow/data/chunk_data_pack",
+		"directory that stores the protocol state")


fxamacker

Thanks for updating the PR! I left some comments.

cmd/util/cmd/verify_execution_result/cmd.go

engine/verification/verifier/verifiers.go

fxamacker · 2024-12-02T20:44:31Z

engine/verification/verifier/verifiers.go

+	defer func() {
+		err := closer()
+		if err != nil {
+			log.Error().Err(err).Msg("failed to close storages")
+		}
+	}()


Same as above to consider returning closer() error as well.

Co-authored-by: Faye Amacker <[email protected]>

zhangchiqing commented Nov 20, 2024

View reviewed changes

zhangchiqing requested review from janezpodhostnik, peterargue and fxamacker November 20, 2024 18:54

zhangchiqing mentioned this pull request Nov 20, 2024

Backwards-compatibility check automation #6557

Closed

peterargue approved these changes Nov 21, 2024

View reviewed changes

zhangchiqing force-pushed the leo/cmd-add-verify-execution-result branch from 89ae34a to 6453fb4 Compare November 22, 2024 14:30

fxamacker reviewed Nov 27, 2024

View reviewed changes

zhangchiqing requested a review from fxamacker December 2, 2024 17:12

fxamacker approved these changes Dec 2, 2024

View reviewed changes

zhangchiqing and others added 7 commits December 2, 2024 13:02

add verify execution result cmd

373bd67

add review comments

a240e54

apply review comments

9e924e8

apply review comments

f75569e

check from against first verifiable height

9d65319

Apply suggestions from code review

3cfb408

Co-authored-by: Faye Amacker <[email protected]>

fix lint

df4891d

zhangchiqing force-pushed the leo/cmd-add-verify-execution-result branch from 0c99a9e to df4891d Compare December 2, 2024 21:02

zhangchiqing enabled auto-merge December 2, 2024 21:02

zhangchiqing added this pull request to the merge queue Dec 2, 2024

Merged via the queue into master with commit c96b754 Dec 2, 2024
55 checks passed

zhangchiqing deleted the leo/cmd-add-verify-execution-result branch December 2, 2024 22:03

This was referenced Dec 10, 2024

Backport v0.37 cmd add verify execution result #6791

Merged

Backport v0.33 verify execution result #6806

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Util] Add verify execution result cmd #6746

[Util] Add verify execution result cmd #6746

zhangchiqing commented Nov 20, 2024 •

edited

Loading

zhangchiqing Nov 20, 2024

codecov-commenter commented Nov 20, 2024 •

edited

Loading

peterargue Nov 21, 2024

peterargue Nov 21, 2024

peterargue Nov 21, 2024

fxamacker left a comment

fxamacker Nov 27, 2024

fxamacker Nov 27, 2024

fxamacker Nov 27, 2024

fxamacker Nov 27, 2024

This comment was marked as resolved.

fxamacker Nov 27, 2024

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

fxamacker left a comment

fxamacker Dec 2, 2024

	"the height range to verify blocks, i.e, 1-1000, 1000-2000, 2000-3000, etc.")
	"the height range to verify blocks (inclusive), i.e, 1-1000, 1000-2000, 2000-3000, etc.")

	err = verifier.VerifyRange(from, to, flow.Testnet, flagDatadir, flagChunkDataPackDir)
	err = verifier.VerifyRange(from, to, flow.ChainID(flagChain), flagDatadir, flagChunkDataPackDir)

	err := verifier.VerifyLastKHeight(flagLastK, flow.Testnet, flagDatadir, flagChunkDataPackDir)
	err := verifier.VerifyLastKHeight(flagLastK, flow.ChainID(flagChain), flagDatadir, flagChunkDataPackDir)

	log.Fatal().Err(err).Msg("could not verify last k height")
	log.Fatal().Err(err).Msg("could not verify range from %d to %d", from, to)

		Cmd.Flags().StringVar(&flagDatadir, "datadir", "/var/flow/data/protocol",
		"directory that stores the protocol state")

		Cmd.Flags().StringVar(&flagChunkDataPackDir, "chunk_data_pack_dir", "/var/flow/data/chunk_data_pack",
		"directory that stores the protocol state")

[Util] Add verify execution result cmd #6746

[Util] Add verify execution result cmd #6746

Conversation

zhangchiqing commented Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Nov 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxamacker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

fxamacker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangchiqing commented Nov 20, 2024 •

edited

Loading

codecov-commenter commented Nov 20, 2024 •

edited

Loading