From 654fecb621e8a21aea1efc5d1d8069e4ae154d7a Mon Sep 17 00:00:00 2001 From: Tim Barry Date: Fri, 10 Jan 2025 14:02:42 -0800 Subject: [PATCH 1/5] Verify chunk states are consistent in execution receipt Receipt validator checks that `chunk[i].EndState == chunk[i+1].StartState` --- module/validation/receipt_validator.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/module/validation/receipt_validator.go b/module/validation/receipt_validator.go index 9fb2569e21c..17c3a113ed2 100644 --- a/module/validation/receipt_validator.go +++ b/module/validation/receipt_validator.go @@ -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] { + 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 } From 0095cdf029dc70ab571175f89da0eeeed59b558c Mon Sep 17 00:00:00 2001 From: Tim Barry Date: Fri, 10 Jan 2025 14:10:49 -0800 Subject: [PATCH 2/5] Add TestReceiptInconsistentChunkList for receipt validator --- module/validation/receipt_validator_test.go | 22 +++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/module/validation/receipt_validator_test.go b/module/validation/receipt_validator_test.go index 8d953978d50..9ab4bb2cc5a 100644 --- a/module/validation/receipt_validator_test.go +++ b/module/validation/receipt_validator_test.go @@ -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 + + 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() From 178a9a20f811014636598d0482bf88e46efb5ace Mon Sep 17 00:00:00 2001 From: Tim Barry Date: Fri, 10 Jan 2025 14:20:24 -0800 Subject: [PATCH 3/5] Ensure chunk consistency by default in tests 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. --- engine/consensus/approvals/testutil.go | 2 +- engine/verification/utils/unittest/fixture.go | 2 +- insecure/wintermute/attackOrchestrator.go | 4 ++-- model/flow/chunk_test.go | 4 ++-- utils/unittest/fixtures.go | 15 ++++++++------- 5 files changed, 14 insertions(+), 13 deletions(-) diff --git a/engine/consensus/approvals/testutil.go b/engine/consensus/approvals/testutil.go index f112761072d..b0dbcdf0ed0 100644 --- a/engine/consensus/approvals/testutil.go +++ b/engine/consensus/approvals/testutil.go @@ -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{} diff --git a/engine/verification/utils/unittest/fixture.go b/engine/verification/utils/unittest/fixture.go index 25cea9d934c..e2dd14318e7 100644 --- a/engine/verification/utils/unittest/fixture.go +++ b/engine/verification/utils/unittest/fixture.go @@ -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, } diff --git a/insecure/wintermute/attackOrchestrator.go b/insecure/wintermute/attackOrchestrator.go index 9ff3bd3c1f5..1f90f7f0a3f 100644 --- a/insecure/wintermute/attackOrchestrator.go +++ b/insecure/wintermute/attackOrchestrator.go @@ -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 diff --git a/model/flow/chunk_test.go b/model/flow/chunk_test.go index 9da330dcaaa..0c1cbe7729f 100644 --- a/model/flow/chunk_test.go +++ b/model/flow/chunk_test.go @@ -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 @@ -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{} diff --git a/utils/unittest/fixtures.go b/utils/unittest/fixtures.go index 9c3f9784493..de86955d25d 100644 --- a/utils/unittest/fixtures.go +++ b/utils/unittest/fixtures.go @@ -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()) } } @@ -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(), } @@ -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, @@ -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 } From 8669b79c263ec936628f1359acf4824433d40a73 Mon Sep 17 00:00:00 2001 From: Tim Barry <21149133+tim-barry@users.noreply.github.com> Date: Tue, 14 Jan 2025 14:44:58 -0800 Subject: [PATCH 4/5] Update module/validation/receipt_validator_test.go Co-authored-by: Jordan Schalm --- module/validation/receipt_validator_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/module/validation/receipt_validator_test.go b/module/validation/receipt_validator_test.go index 9ab4bb2cc5a..93606ad1a94 100644 --- a/module/validation/receipt_validator_test.go +++ b/module/validation/receipt_validator_test.go @@ -233,9 +233,7 @@ func (s *ReceiptValidationSuite) TestReceiptInconsistentChunkList() { 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 + lastChunk.StartState, lastChunk.EndState = lastChunk.EndState, lastChunk.StartState receipt := unittest.ExecutionReceiptFixture(unittest.WithExecutorID(s.ExeID), unittest.WithResult(valSubgrph.Result)) From fc33301fbfc695b8445f7ae62aa9d52649ae95e8 Mon Sep 17 00:00:00 2001 From: Tim Barry Date: Wed, 15 Jan 2025 09:44:14 -0800 Subject: [PATCH 5/5] Suggestion from code review: use range over integer Co-authored-by: Jordan Schalm --- module/validation/receipt_validator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/validation/receipt_validator.go b/module/validation/receipt_validator.go index 17c3a113ed2..947d05cf166 100644 --- a/module/validation/receipt_validator.go +++ b/module/validation/receipt_validator.go @@ -99,7 +99,7 @@ func (v *receiptValidator) verifyChunksFormat(result *flow.ExecutionResult) erro // We have at least one chunk, check chunk state consistency chunks := result.Chunks.Items() - for i := range chunks[:len(chunks)-1] { + for i := range len(chunks) - 1 { 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) }