Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate consistency of chunk Start and End states #6871

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tim-barry
Copy link
Contributor

@tim-barry tim-barry commented Jan 10, 2025

Ensure the receipt validator (consensus node) verifies that each chunk's end state is the same as the next chunk's start state. This PR also adds a test with an inconsistent chunklist that should trigger this error, as well as updates tests to have consistent chunklists by default (previously, the chunklists generated for tests were inconsistent.)

The requirement that the first chunk's starting state matches the end state from the previous execution result is already checked in resultChainCheck.

Issue: https://github.com/onflow/flow-go-internal/issues/5491

Receipt validator checks that `chunk[i].EndState == chunk[i+1].StartState`
Modify ChunkListFixture to have consistent chunk start and end states.
Previously, chunk start and end states were assigned independently,
resulting in invalid execution receipts being used in tests.
@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.

Project coverage is 41.11%. Comparing base (0d2b34f) to head (fc33301).

Files with missing lines Patch % Lines
utils/unittest/fixtures.go 0.00% 7 Missing ⚠️
engine/verification/utils/unittest/fixture.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6871   +/-   ##
=======================================
  Coverage   41.11%   41.11%           
=======================================
  Files        2116     2116           
  Lines      185737   185742    +5     
=======================================
+ Hits        76368    76373    +5     
+ Misses     102952   102951    -1     
- Partials     6417     6418    +1     
Flag Coverage Δ
unittests 41.11% <50.00%> (+<0.01%) ⬆️

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.

@tim-barry tim-barry marked this pull request as ready for review January 13, 2025 16:55
@tim-barry tim-barry requested a review from a team as a code owner January 13, 2025 16:55
Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. Only comments are some optional stylistic changes.


// We have at least one chunk, check chunk state consistency
chunks := result.Chunks.Items()
for i := range chunks[:len(chunks)-1] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for i := range chunks[:len(chunks)-1] {
for i := range len(chunks) - 1 {

As of Go 1.22, you can range over a number directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I recall trying this (range over a number) in a previous PR but it failed the linter. At the time I thought it was because I was using Go 1.23 locally and the linter was running on Go 1.22, and the language feature was only supported in Go 1.23; but if it was added in 1.22 maybe it's just the linter that's behind?

Copy link
Contributor Author

@tim-barry tim-barry Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this commit: link the linter complained about "cannot range over untyped int constant" - maybe it was the fact that it wasn't explicitly typed, rather than the syntax itself?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Did a bit of digging and it seems like:

  • the linter is compatible with Go versions up to the version it was compiled with
  • older releases are not re-compiled with new Go versions

The linter version we use v1.54.2 was compiled with go1.21, hence the lack of compatibility. We will need to update the linter version.

% ./golangci-lint version
golangci-lint has version 1.54.2 built with go1.21.0 from 411e0bbb on 2023-08-21T12:04:32Z

module/validation/receipt_validator_test.go Outdated Show resolved Hide resolved
Co-authored-by: Jordan Schalm <jordan.schalm@flowfoundation.org>
jordanschalm added a commit that referenced this pull request Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants