Merge pull request #39 from onflow/yahya/4885-chunk-id-uniqueness

VER: Yahya/4885-chunk-id-uniqueness

engine/consensus/matching/engine_test.go

@@ -651,7 +651,7 @@ func (ms *MatchingSuite) TestSealResultInvalidChunks() {
 	ms.sealedResults[previous.ID()] = previous
 
 	// add an extra chunk
-	chunk := unittest.ChunkFixture()
+	chunk := unittest.ChunkFixture(block.ID())
 	chunk.Index = uint64(len(block.Payload.Guarantees))
 	result.Chunks = append(result.Chunks, chunk)
 

engine/execution/ingestion/engine.go

@@ -694,8 +694,9 @@ func (e *Engine) saveExecutionResults(
 	defer span.Finish()
 
 	originalState := startState
+	blockID := executableBlock.ID()
 
-	err := e.execState.PersistStateInteractions(childCtx, executableBlock.ID(), stateInteractions)
+	err := e.execState.PersistStateInteractions(childCtx, blockID, stateInteractions)
 	if err != nil && !errors.Is(err, storage.ErrAlreadyExists) {
 		return nil, err
 	}
@@ -724,15 +725,15 @@ func (e *Engine) saveExecutionResults(
 			collectionID = flow.ZeroID
 		}
 
-		chunk := generateChunk(i, startState, endState, collectionID)
+		chunk := generateChunk(i, startState, endState, collectionID, blockID)
 
 		// chunkDataPack
 		allRegisters := view.AllRegisters()
 
 		values, proofs, err := e.execState.GetRegistersWithProofs(childCtx, chunk.StartState, allRegisters)
 		if err != nil {
 			return nil, fmt.Errorf(
-				"error reading registers with proofs for chunk number [%v] of block [%x] ", i, executableBlock.ID(),
+				"error reading registers with proofs for chunk number [%v] of block [%x] ", i, blockID,
 			)
 		}
 
@@ -748,7 +749,7 @@ func (e *Engine) saveExecutionResults(
 		startState = endState
 	}
 
-	err = e.execState.PersistStateCommitment(childCtx, executableBlock.ID(), endState)
+	err = e.execState.PersistStateCommitment(childCtx, blockID, endState)
 	if err != nil {
 		return nil, fmt.Errorf("failed to store state commitment: %w", err)
 	}
@@ -774,7 +775,6 @@ func (e *Engine) saveExecutionResults(
 		span, _ := e.tracer.StartSpanFromContext(childCtx, trace.EXESaveTransactionEvents)
 		defer span.Finish()
 
-		blockID := executableBlock.ID()
 		if len(events) > 0 {
 			err = e.events.Store(blockID, events)
 			if err != nil {
@@ -790,7 +790,7 @@ func (e *Engine) saveExecutionResults(
 	err = func() error {
 		span, _ := e.tracer.StartSpanFromContext(childCtx, trace.EXESaveTransactionResults)
 		defer span.Finish()
-		blockID := executableBlock.ID()
+
 		err = e.transactionResults.BatchStore(blockID, txResults)
 		if err != nil {
 			return fmt.Errorf("failed to store transaction result error: %w", err)
@@ -841,7 +841,9 @@ func (e *Engine) logExecutableBlock(eb *entity.ExecutableBlock) {
 }
 
 // generateChunk creates a chunk from the provided computation data.
-func generateChunk(colIndex int, startState, endState flow.StateCommitment, colID flow.Identifier) *flow.Chunk {
+func generateChunk(colIndex int,
+	startState, endState flow.StateCommitment,
+	colID, blockID flow.Identifier) *flow.Chunk {
 	return &flow.Chunk{
 		ChunkBody: flow.ChunkBody{
 			CollectionIndex: uint(colIndex),
@@ -850,6 +852,7 @@ func generateChunk(colIndex int, startState, endState flow.StateCommitment, colI
 			// Otherwise, the chances of there being chunks with the same ID before all these TODOs are done is large, since
 			// startState stays the same if blocks are empty
 			EventCollection: colID,
+			BlockID:         blockID,
 			// TODO: record gas used
 			TotalComputationUsed: 0,
 			// TODO: record number of txs

engine/verification/match/engine_test.go

@@ -812,6 +812,7 @@ func ChunkWithIndex(blockID flow.Identifier, index int) *flow.Chunk {
 		ChunkBody: flow.ChunkBody{
 			CollectionIndex: uint(index),
 			EventCollection: blockID, // ensure chunks from different blocks with the same index will have different chunk ID
+			BlockID:         blockID,
 		},
 	}
 	return chunk

engine/verification/utils/fixtures.go

@@ -171,50 +171,55 @@ func CompleteExecutionResultFixture(t *testing.T, chunkCount int, chain flow.Cha
 // everything is wired properly, but with the minimum viable content provided. This version is basically used
 // for profiling.
 func LightExecutionResultFixture(chunkCount int) CompleteExecutionResult {
-	chunks := make([]*flow.Chunk, 0)
 	collections := make([]*flow.Collection, 0, chunkCount)
 	guarantees := make([]*flow.CollectionGuarantee, 0, chunkCount)
 	chunkDataPacks := make([]*flow.ChunkDataPack, 0, chunkCount)
 
+	// creates collections and guarantees
 	for i := 0; i < chunkCount; i++ {
-		// creates one guaranteed collection per chunk
 		coll := unittest.CollectionFixture(1)
 		guarantee := coll.Guarantee()
 		collections = append(collections, &coll)
 		guarantees = append(guarantees, &guarantee)
+	}
+
+	payload := flow.Payload{
+		Guarantees: guarantees,
+	}
+
+	header := unittest.BlockHeaderFixture()
+	header.Height = 0
+	header.PayloadHash = payload.Hash()
+
+	block := flow.Block{
+		Header:  &header,
+		Payload: &payload,
+	}
+	blockID := block.ID()
 
+	// creates chunks
+	chunks := make([]*flow.Chunk, 0)
+	for i := 0; i < chunkCount; i++ {
 		chunk := &flow.Chunk{
 			ChunkBody: flow.ChunkBody{
 				CollectionIndex: uint(i),
+				BlockID:         blockID,
 				EventCollection: unittest.IdentifierFixture(),
 			},
 			Index: uint64(i),
 		}
 		chunks = append(chunks, chunk)
 
-		// creates a chunk data pack for the chunk
+		// creates a light (quite empty) chunk data pack for the chunk at bare minimum
 		chunkDataPack := flow.ChunkDataPack{
 			ChunkID: chunk.ID(),
 		}
 		chunkDataPacks = append(chunkDataPacks, &chunkDataPack)
 	}
 
-	payload := flow.Payload{
-		Guarantees: guarantees,
-	}
-
-	header := unittest.BlockHeaderFixture()
-	header.Height = 0
-	header.PayloadHash = payload.Hash()
-
-	block := flow.Block{
-		Header:  &header,
-		Payload: &payload,
-	}
-
 	result := flow.ExecutionResult{
 		ExecutionResultBody: flow.ExecutionResultBody{
-			BlockID: block.ID(),
+			BlockID: blockID,
 			Chunks:  chunks,
 		},
 	}
@@ -292,6 +297,7 @@ func executeCollection(
 			StartState:      startStateCommitment,
 			// TODO: include event collection hash
 			EventCollection: flow.ZeroID,
+			BlockID:         executableBlock.ID(),
 			// TODO: record gas used
 			TotalComputationUsed: 0,
 			// TODO: record number of txs

go.mod

@@ -41,6 +41,7 @@ require (
 	github.com/spf13/cobra v0.0.6
 	github.com/spf13/pflag v1.0.3
 	github.com/spf13/viper v1.4.0
+	github.com/stretchr/objx v0.2.0 // indirect
 	github.com/stretchr/testify v1.6.1
 	github.com/uber/jaeger-client-go v2.22.1+incompatible
 	github.com/uber/jaeger-lib v2.3.0+incompatible // indirect

go.sum

@@ -799,6 +799,8 @@ github.com/steakknife/hamming v0.0.0-20180906055917-c99c65617cd3/go.mod h1:hpGUW
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.1.1 h1:2vfRuCMp5sSVIDSqO8oNnWJq7mPa6KVP3iPIwFBuy8A=
 github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
+github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48=
+github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE=
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
 github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk=

integration/tests/consensus/sealing_test.go

@@ -180,8 +180,9 @@ SearchLoop:
 			CollectionIndex:      0,           // irrelevant for consensus node
 			StartState:           nil,         // irrelevant for consensus node
 			EventCollection:      flow.ZeroID, // irrelevant for consensus node
-			TotalComputationUsed: 0,           // irrelevant for consensus node
-			NumberOfTransactions: 0,           // irrelevant for consensus node
+			BlockID:              targetID,
+			TotalComputationUsed: 0, // irrelevant for consensus node
+			NumberOfTransactions: 0, // irrelevant for consensus node
 		},
 		Index:    0,                                 // should start at zero
 		EndState: unittest.StateCommitmentFixture(), // random end execution state

integration/tests/verification/system_chunk_test.go

@@ -0,0 +1,51 @@
+package verification
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"github.com/stretchr/testify/suite"
+)
+
+func TestSystemChunk(t *testing.T) {
+	suite.Run(t, new(SystemChunkTestSuite))
+}
+
+type SystemChunkTestSuite struct {
+	Suite
+}
+
+// TestSystemChunkIDsShouldBeDifferent evaluates that system chunk of consecutive blocks that
+// do not cause state change have different chunk Ids.
+func (st *SystemChunkTestSuite) TestSystemChunkIDsShouldBeDifferent() {
+	// waits for first finalized block, called blockA.
+	blockA := st.BlockState.WaitForFirstFinalized(st.T())
+	st.T().Logf("blockA generated, height: %v ID: %v", blockA.Header.Height, blockA.Header.ID())
+
+	// waits for the next finalized block after blockA, called blockB.
+	blockB := st.BlockState.WaitForFinalizedChild(st.T(), blockA)
+	st.T().Logf("blockB generated, height: %v ID: %v", blockB.Header.Height, blockB.Header.ID())
+
+	// waits for execution receipt for blockA from execution node, called receiptA.
+	receiptA := st.ReceiptState.WaitForReceiptFrom(st.T(), blockA.Header.ID(), st.exeID)
+	resultAId := receiptA.ExecutionResult.ID()
+	st.T().Logf("receipt for blockA generated: result ID: %x", resultAId)
+
+	// waits for execution receipt for blockB from execution node, called receiptB.
+	receiptB := st.ReceiptState.WaitForReceiptFrom(st.T(), blockB.Header.ID(), st.exeID)
+	resultBId := receiptB.ExecutionResult.ID()
+	st.T().Logf("receipt for blockB generated: result ID: %x", resultBId)
+
+	// Todo: drop this part once system chunk changes the state
+	// requires that execution state is not changed between block A and B
+	stateA := receiptA.ExecutionResult.FinalStateCommit
+	stateB := receiptB.ExecutionResult.FinalStateCommit
+	require.Equal(st.T(), stateA, stateB)
+
+	// computes ids of system chunk for result A and B
+	systemChunkAId := receiptA.ExecutionResult.Chunks[0].ID()
+	systemChunkBId := receiptB.ExecutionResult.Chunks[0].ID()
+
+	// requires that system chunk Id of execution results be different
+	require.NotEqual(st.T(), systemChunkAId, systemChunkBId)
+}

model/flow/chunk.go

@@ -6,6 +6,7 @@ type ChunkBody struct {
 	// execution info
 	StartState      StateCommitment // start state when starting executing this chunk
 	EventCollection Identifier      // Events generated by executing results
+	BlockID         Identifier      // Block id of the execution result this chunk belongs to
 
 	// Computation consumption info
 	TotalComputationUsed uint64 // total amount of computation used by running all txs in this chunk

model/flow/chunk_test.go

@@ -1,16 +1,19 @@
-package flow
+package flow_test
 
 import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+
+	"github.com/onflow/flow-go/model/flow"
+	"github.com/onflow/flow-go/utils/unittest"
 )
 
 // TestChunkList_ByIndex evaluates reliability of ByIndex method against within and
 // out of range indices
 func TestChunkList_ByIndex(t *testing.T) {
 	// creates a chunk list with the size of 10
-	var chunkList ChunkList = make([]*Chunk, 10)
+	var chunkList flow.ChunkList = make([]*flow.Chunk, 10)
 
 	// an out of index chunk by index
 	_, ok := chunkList.ByIndex(11)
@@ -19,5 +22,56 @@ func TestChunkList_ByIndex(t *testing.T) {
 	// a within range chunk by index
 	_, ok = chunkList.ByIndex(1)
 	require.True(t, ok)
+}
+
+// TestDistinctChunkIDs_EmptyChunks evaluates that two empty chunks
+// with the distinct block ids would have distinct chunk ids.
+func TestDistinctChunkIDs_EmptyChunks(t *testing.T) {
+	// generates two random block ids and requires them
+	// being distinct
+	blockIdA := unittest.IdentifierFixture()
+	blockIdB := unittest.IdentifierFixture()
+	require.NotEqual(t, blockIdA, blockIdB)
+
+	// generates a chunk associated with each block id
+	chunkA := &flow.Chunk{
+		ChunkBody: flow.ChunkBody{
+			BlockID: blockIdA,
+		},
+	}
+
+	chunkB := &flow.Chunk{
+		ChunkBody: flow.ChunkBody{
+			BlockID: blockIdB,
+		},
+	}
+
+	require.NotEqual(t, chunkA.ID(), chunkB.ID())
+}
+
+// TestDistinctChunkIDs_FullChunks evaluates that two full chunks
+// with completely identical fields but distinct block ids have
+// distinct chunk ids.
+func TestDistinctChunkIDs_FullChunks(t *testing.T) {
+	// generates two random block ids and requires them
+	// being distinct
+	blockIdA := unittest.IdentifierFixture()
+	blockIdB := unittest.IdentifierFixture()
+	require.NotEqual(t, blockIdA, blockIdB)
+
+	// generates a chunk associated with blockA
+	chunkA := unittest.ChunkFixture(blockIdA)
+
+	// generates a deep copy of chunkA in chunkB
+	chunkB := *chunkA
+
+	// since chunkB is a deep copy of chunkA their
+	// chunk ids should be the same
+	require.Equal(t, chunkA.ID(), chunkB.ID())
+
+	// changes block id in chunkB
+	chunkB.BlockID = blockIdB
 
+	// chunks with distinct block ids should have distinct chunk ids
+	require.NotEqual(t, chunkA.ID(), chunkB.ID())
 }

module/chunks/chunkVerifier_test.go

@@ -163,6 +163,7 @@ func GetBaselineVerifiableChunk(t *testing.T, script []byte) *verification.Verif
 		Header:  &header,
 		Payload: &payload,
 	}
+	blockID := block.ID()
 
 	// registerTouch and State setup
 	id1 := state.RegisterID(string(make([]byte, 32)), "", "")
@@ -197,6 +198,7 @@ func GetBaselineVerifiableChunk(t *testing.T, script []byte) *verification.Verif
 			ChunkBody: flow.ChunkBody{
 				CollectionIndex: 0,
 				StartState:      startState,
+				BlockID:         blockID,
 			},
 			Index: 0,
 		}
@@ -210,7 +212,7 @@ func GetBaselineVerifiableChunk(t *testing.T, script []byte) *verification.Verif
 		// ExecutionResult setup
 		result := flow.ExecutionResult{
 			ExecutionResultBody: flow.ExecutionResultBody{
-				BlockID: block.ID(),
+				BlockID: blockID,
 				Chunks:  flow.ChunkList{&chunk},
 			},
 		}

module/chunks/publicAssign_test.go

@@ -336,11 +336,14 @@ func (a *PublicAssignmentTestSuite) CreateChunks(num int, t *testing.T) flow.Chu
 		_, err := rand.Read(state)
 		require.NoError(t, err)
 
+		blockID := unittest.IdentifierFixture()
+
 		// creates chunk
 		c := &flow.Chunk{
 			Index: uint64(i),
 			ChunkBody: flow.ChunkBody{
 				StartState: state,
+				BlockID:    blockID,
 			},
 		}
 		list.Insert(c)