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
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion engine/consensus/approvals/testutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func (s *BaseApprovalsTestSuite) SetupTest() {
verifiers := make(flow.IdentifierList, 0)
s.AuthorizedVerifiers = make(map[flow.Identifier]*flow.Identity)
s.ChunksAssignment = chunks.NewAssignment()
s.Chunks = unittest.ChunkListFixture(50, s.Block.ID())
s.Chunks = unittest.ChunkListFixture(50, s.Block.ID(), unittest.StateCommitmentFixture())
// mock public key to mock signature verifications
s.PublicKey = &module.PublicKey{}

Expand Down
2 changes: 1 addition & 1 deletion engine/verification/utils/unittest/fixture.go
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ func ExecutionResultForkFixture(t *testing.T) (*flow.ExecutionResult, *flow.Exec
resultB := &flow.ExecutionResult{
PreviousResultID: resultA.PreviousResultID,
BlockID: resultA.BlockID,
Chunks: append(flow.ChunkList{resultA.Chunks[0]}, unittest.ChunkListFixture(1, resultA.BlockID)...),
Chunks: append(flow.ChunkList{resultA.Chunks[0]}, unittest.ChunkListFixture(1, resultA.BlockID, resultA.Chunks[0].EndState)...),
ServiceEvents: nil,
}

Expand Down
4 changes: 2 additions & 2 deletions insecure/wintermute/attackOrchestrator.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,14 @@ func (o *Orchestrator) corruptExecutionResult(receipt *flow.ExecutionReceipt) *f
BlockID: receipt.ExecutionResult.BlockID,
// replace all chunks with new ones to simulate chunk corruption
Chunks: flow.ChunkList{
unittest.ChunkFixture(receipt.ExecutionResult.BlockID, 0, unittest.WithChunkStartState(receiptStartState)),
unittest.ChunkFixture(receipt.ExecutionResult.BlockID, 0, receiptStartState),
},
ServiceEvents: receipt.ExecutionResult.ServiceEvents,
ExecutionDataID: receipt.ExecutionResult.ExecutionDataID,
}

if chunksNum > 1 {
result.Chunks = append(result.Chunks, unittest.ChunkListFixture(uint(chunksNum-1), receipt.ExecutionResult.BlockID)...)
result.Chunks = append(result.Chunks, unittest.ChunkListFixture(uint(chunksNum-1), receipt.ExecutionResult.BlockID, result.Chunks[0].EndState)...)
}

return result
Expand Down
4 changes: 2 additions & 2 deletions model/flow/chunk_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func TestDistinctChunkIDs_FullChunks(t *testing.T) {
require.NotEqual(t, blockIdA, blockIdB)

// generates a chunk associated with blockA
chunkA := unittest.ChunkFixture(blockIdA, 42)
chunkA := unittest.ChunkFixture(blockIdA, 42, unittest.StateCommitmentFixture())

// generates a deep copy of chunkA in chunkB
chunkB := *chunkA
Expand All @@ -80,7 +80,7 @@ func TestDistinctChunkIDs_FullChunks(t *testing.T) {

// TestChunkList_Indices evaluates the Indices method of ChunkList on lists of different sizes.
func TestChunkList_Indices(t *testing.T) {
cl := unittest.ChunkListFixture(5, unittest.IdentifierFixture())
cl := unittest.ChunkListFixture(5, unittest.IdentifierFixture(), unittest.StateCommitmentFixture())
t.Run("empty chunk subset indices", func(t *testing.T) {
// subset of chunk list that is empty should return an empty list
subset := flow.ChunkList{}
Expand Down
8 changes: 8 additions & 0 deletions module/validation/receipt_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,14 @@ func (v *receiptValidator) verifyChunksFormat(result *flow.ExecutionResult) erro
if result.Chunks.Len() != requiredChunks {
return engine.NewInvalidInputErrorf("invalid number of chunks, expected %d got %d", requiredChunks, result.Chunks.Len())
}

// 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

if chunks[i].EndState != chunks[i+1].StartState {
return engine.NewInvalidInputErrorf("chunk state mismatch at index %v, EndState %v but next StartState %v", i, chunks[i].EndState, chunks[i+1].StartState)
}
}
return nil
}

Expand Down
22 changes: 22 additions & 0 deletions module/validation/receipt_validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,28 @@ func (s *ReceiptValidationSuite) TestReceiptForBlockWith0Collections() {
})
}

// TestReceiptInconsistentChunkList tests that we reject receipts when the Start and End states
// within the chunk list are inconsistent (e.g. chunk[0].EndState != chunk[1].StartState).
func (s *ReceiptValidationSuite) TestReceiptInconsistentChunkList() {
s.publicKey.On("Verify", mock.Anything, mock.Anything, mock.Anything).Return(true, nil).Maybe()
valSubgrph := s.ValidSubgraphFixture()
chunks := valSubgrph.Result.Chunks
require.GreaterOrEqual(s.T(), chunks.Len(), 1)
// swap last chunk's start and end states
lastChunk := chunks[len(chunks)-1]
tmp := lastChunk.StartState
lastChunk.StartState = lastChunk.EndState
lastChunk.EndState = tmp
tim-barry marked this conversation as resolved.
Show resolved Hide resolved

receipt := unittest.ExecutionReceiptFixture(unittest.WithExecutorID(s.ExeID),
unittest.WithResult(valSubgrph.Result))
s.AddSubgraphFixtureToMempools(valSubgrph)

err := s.receiptValidator.Validate(receipt)
s.Require().Error(err, "should reject with invalid chunks")
s.Assert().True(engine.IsInvalidInputError(err))
}

// TestReceiptTooManyChunks tests that we reject receipt with more chunks than expected
func (s *ReceiptValidationSuite) TestReceiptTooManyChunks() {
valSubgrph := s.ValidSubgraphFixture()
Expand Down
15 changes: 8 additions & 7 deletions utils/unittest/fixtures.go
Original file line number Diff line number Diff line change
Expand Up @@ -893,15 +893,14 @@ func WithBlock(block *flow.Block) func(*flow.ExecutionResult) {
return func(result *flow.ExecutionResult) {
startState := result.Chunks[0].StartState // retain previous start state in case it was user-defined
result.BlockID = blockID
result.Chunks = ChunkListFixture(uint(chunks), blockID)
result.Chunks[0].StartState = startState // set start state to value before update
result.Chunks = ChunkListFixture(uint(chunks), blockID, startState)
result.PreviousResultID = previousResultID
}
}

func WithChunks(n uint) func(*flow.ExecutionResult) {
return func(result *flow.ExecutionResult) {
result.Chunks = ChunkListFixture(n, result.BlockID)
result.Chunks = ChunkListFixture(n, result.BlockID, StateCommitmentFixture())
}
}

Expand Down Expand Up @@ -966,7 +965,7 @@ func ExecutionResultFixture(opts ...func(*flow.ExecutionResult)) *flow.Execution
result := &flow.ExecutionResult{
PreviousResultID: IdentifierFixture(),
BlockID: executedBlockID,
Chunks: ChunkListFixture(2, executedBlockID),
Chunks: ChunkListFixture(2, executedBlockID, StateCommitmentFixture()),
ExecutionDataID: IdentifierFixture(),
}

Expand Down Expand Up @@ -1322,12 +1321,13 @@ func WithChunkStartState(startState flow.StateCommitment) func(chunk *flow.Chunk
func ChunkFixture(
blockID flow.Identifier,
collectionIndex uint,
startState flow.StateCommitment,
opts ...func(*flow.Chunk),
) *flow.Chunk {
chunk := &flow.Chunk{
ChunkBody: flow.ChunkBody{
CollectionIndex: collectionIndex,
StartState: StateCommitmentFixture(),
StartState: startState,
EventCollection: IdentifierFixture(),
TotalComputationUsed: 4200,
NumberOfTransactions: 42,
Expand All @@ -1344,12 +1344,13 @@ func ChunkFixture(
return chunk
}

func ChunkListFixture(n uint, blockID flow.Identifier) flow.ChunkList {
func ChunkListFixture(n uint, blockID flow.Identifier, startState flow.StateCommitment) flow.ChunkList {
chunks := make([]*flow.Chunk, 0, n)
for i := uint64(0); i < uint64(n); i++ {
chunk := ChunkFixture(blockID, uint(i))
chunk := ChunkFixture(blockID, uint(i), startState)
chunk.Index = i
chunks = append(chunks, chunk)
startState = chunk.EndState
}
return chunks
}
Expand Down
Loading