From 77c274ea76162100805dbab575551d227191d8a3 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 22 Nov 2023 19:43:35 +0200 Subject: [PATCH 01/10] Added a constant for verifying how many execution receipts are needed for execution result to be considered valid --- model/flow/constants.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/model/flow/constants.go b/model/flow/constants.go index 6b03c36a6db..7e4e9b4d634 100644 --- a/model/flow/constants.go +++ b/model/flow/constants.go @@ -61,6 +61,13 @@ const DefaultRequiredApprovalsForSealConstruction = uint(1) // - Full protocol should be +2/3 of all currently authorized verifiers. const DefaultRequiredApprovalsForSealValidation = 0 +// DefaultRequiredReceiptsCommittingToExecutionResult is the default number of receipts that should be committing to an execution +// result which is being incorporated into a candidate block. +// ATTENTION: +// For each result in the candidate block, there must be k receipts included in the candidate block, with k strictly larger than 0. +// This value has to be strictly larger than 0, otherwise leader can incorporate a result which was not executed. +const DefaultRequiredReceiptsCommittingToExecutionResult = 1 + // DefaultChunkAssignmentAlpha is the default number of verifiers that should be // assigned to each chunk. const DefaultChunkAssignmentAlpha = 3 From b76556eb58d829f985bf99b6d717b085fde66210 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 22 Nov 2023 19:44:14 +0200 Subject: [PATCH 02/10] Added implementation of checks that ensure that we have seen at least k receipts in candidate block --- module/validation/receipt_validator.go | 43 ++++++++++++++++++++------ 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/module/validation/receipt_validator.go b/module/validation/receipt_validator.go index dae906a982a..9f031ccdfd0 100644 --- a/module/validation/receipt_validator.go +++ b/module/validation/receipt_validator.go @@ -3,6 +3,7 @@ package validation import ( "errors" "fmt" + "github.com/onflow/flow-go/module" "github.com/onflow/flow-go/crypto/hash" "github.com/onflow/flow-go/engine" @@ -17,20 +18,24 @@ import ( // receiptValidator holds all needed context for checking // receipt validity against current protocol state. type receiptValidator struct { - headers storage.Headers - seals storage.Seals - state protocol.State - index storage.Index - results storage.ExecutionResults - signatureHasher hash.Hasher + headers storage.Headers + seals storage.Seals + state protocol.State + index storage.Index + results storage.ExecutionResults + signatureHasher hash.Hasher + requiredReceiptsIncludedForExecutionResult uint } +var _ module.ReceiptValidator = (*receiptValidator)(nil) + func NewReceiptValidator(state protocol.State, headers storage.Headers, index storage.Index, results storage.ExecutionResults, seals storage.Seals, -) *receiptValidator { + requiredReceiptsIncludedForExecutionResult uint, +) module.ReceiptValidator { rv := &receiptValidator{ state: state, headers: headers, @@ -38,8 +43,8 @@ func NewReceiptValidator(state protocol.State, results: results, signatureHasher: signature.NewBLSHasher(signature.ExecutionReceiptTag), seals: seals, + requiredReceiptsIncludedForExecutionResult: requiredReceiptsIncludedForExecutionResult, } - return rv } @@ -272,12 +277,30 @@ func (v *receiptValidator) ValidatePayload(candidate *flow.Block) error { return fmt.Errorf("internal error while traversing the ancestor fork of unsealed blocks: %w", err) } + // tracks the number of receipts committing to each result. + // it's ok to only index receipts at this point, because we will perform + // all needed checks after we have validated all results. + receiptsByResult := make(map[flow.Identifier]uint) + for _, receipt := range payload.Receipts { + receiptsByResult[receipt.ResultID]++ + } + // first validate all results that were included into payload - // if one of results is invalid we fail the whole check because it could be violating - // parent-children relationship + // if one of results is invalid we fail the whole check because it could be violating parent-children relationship + // each execution for i, result := range payload.Results { resultID := result.ID() + // check if there are enough execution receipts included in the payload corresponding to the execution result + if numberOfReceipts, hasReceipt := receiptsByResult[resultID]; hasReceipt { + if numberOfReceipts < v.requiredReceiptsIncludedForExecutionResult { + return engine.NewInvalidInputErrorf("execution result %v has only %d receipts, but at least %d are required", + resultID, numberOfReceipts, v.requiredReceiptsIncludedForExecutionResult) + } + } else { + return engine.NewInvalidInputErrorf("no receipts for result %v at index %d", resultID, i) + } + // check for duplicated results if _, isDuplicate := executionTree[resultID]; isDuplicate { return engine.NewInvalidInputErrorf("duplicate result %v at index %d", resultID, i) From e10d39b65d9aa8044311148b586525fc1cf2a18a Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 22 Nov 2023 19:44:33 +0200 Subject: [PATCH 03/10] Added new tests to cover happy and failure paths --- module/validation/receipt_validator_test.go | 160 +++++++++++++++++++- 1 file changed, 159 insertions(+), 1 deletion(-) diff --git a/module/validation/receipt_validator_test.go b/module/validation/receipt_validator_test.go index ec6a31d9475..a438a026c27 100644 --- a/module/validation/receipt_validator_test.go +++ b/module/validation/receipt_validator_test.go @@ -29,7 +29,14 @@ func (s *ReceiptValidationSuite) SetupTest() { s.SetupChain() s.publicKey = &fmock.PublicKey{} s.Identities[s.ExeID].StakingPubKey = s.publicKey - s.receiptValidator = NewReceiptValidator(s.State, s.HeadersDB, s.IndexDB, s.ResultsDB, s.SealsDB) + s.receiptValidator = NewReceiptValidator( + s.State, + s.HeadersDB, + s.IndexDB, + s.ResultsDB, + s.SealsDB, + 1, + ) } // TestReceiptValid try submitting valid receipt @@ -745,3 +752,154 @@ func (s *ReceiptValidationSuite) TestValidateReceiptAfterBootstrap() { err := s.receiptValidator.ValidatePayload(candidate) s.Require().NoError(err) } + +// TestValidateReceiptResultWithoutReceipt tests a case when a malicious leader incorporates a made-up execution result +// into their proposal. ReceiptValidator must ensure that for each result included in the block, there must be +// at least one receipt included in that block as well. +func (s *ReceiptValidationSuite) TestValidateReceiptResultWithoutReceipt() { + // assuming signatures are all good + s.publicKey.On("Verify", mock.Anything, mock.Anything, mock.Anything).Return(true, nil) + + // G <- A <- B + blocks, result0, seal := unittest.ChainFixture(2) + s.SealsIndex[blocks[0].ID()] = seal + + receipts := unittest.ReceiptChainFor(blocks, result0) + blockA, blockB := blocks[1], blocks[2] + receiptA, receiptB := receipts[1], receipts[2] + + blockA.Payload.Receipts = []*flow.ExecutionReceiptMeta{} + blockB.Payload.Receipts = []*flow.ExecutionReceiptMeta{receiptA.Meta()} + blockB.Payload.Results = []*flow.ExecutionResult{&receiptA.ExecutionResult} + // update block header so that blocks are chained together + unittest.ReconnectBlocksAndReceipts(blocks, receipts) + // assuming all receipts are executed by the correct executor + for _, r := range receipts { + r.ExecutorID = s.ExeID + } + + for _, b := range blocks { + s.Extend(b) + } + s.PersistedResults[result0.ID()] = result0 + + candidate := unittest.BlockWithParentFixture(blockB.Header) + candidate.Payload = &flow.Payload{ + Receipts: []*flow.ExecutionReceiptMeta{}, + Results: []*flow.ExecutionResult{&receiptB.ExecutionResult}, + } + + err := s.receiptValidator.ValidatePayload(candidate) + s.Require().Error(err) + s.Require().True(engine.IsInvalidInputError(err)) +} + +// TestValidateReceiptResultHasNotEnoughReceipts tests a case when a malicious leader incorporates an execution result +// into their proposal, which has a receipt, but ReceiptValidator is configured to accept at least 2 receipts per execution result. +func (s *ReceiptValidationSuite) TestValidateReceiptResultHasNotEnoughReceipts() { + s.receiptValidator = NewReceiptValidator( + s.State, + s.HeadersDB, + s.IndexDB, + s.ResultsDB, + s.SealsDB, + 2, // at least 2 receipts per execution result + ) + // assuming signatures are all good + s.publicKey.On("Verify", mock.Anything, mock.Anything, mock.Anything).Return(true, nil) + + // G <- A <- B + blocks, result0, seal := unittest.ChainFixture(2) + s.SealsIndex[blocks[0].ID()] = seal + + receipts := unittest.ReceiptChainFor(blocks, result0) + blockA, blockB := blocks[1], blocks[2] + receiptA, receiptB := receipts[1], receipts[2] + + blockA.Payload.Receipts = []*flow.ExecutionReceiptMeta{} + blockB.Payload.Receipts = []*flow.ExecutionReceiptMeta{receiptA.Meta()} + blockB.Payload.Results = []*flow.ExecutionResult{&receiptA.ExecutionResult} + // update block header so that blocks are chained together + unittest.ReconnectBlocksAndReceipts(blocks, receipts) + // assuming all receipts are executed by the correct executor + for _, r := range receipts { + r.ExecutorID = s.ExeID + } + + for _, b := range blocks { + s.Extend(b) + } + s.PersistedResults[result0.ID()] = result0 + + candidate := unittest.BlockWithParentFixture(blockB.Header) + candidate.Payload = &flow.Payload{ + Receipts: []*flow.ExecutionReceiptMeta{receiptB.Meta()}, + Results: []*flow.ExecutionResult{&receiptB.ExecutionResult}, + } + + err := s.receiptValidator.ValidatePayload(candidate) + s.Require().Error(err) + s.Require().True(engine.IsInvalidInputError(err)) +} + +// TestValidateReceiptResultHasEnoughReceipts tests a case where a leader incorporates an execution result +// into their proposal, which has multiple receipts and ReceiptValidator, +// is configured to accept at least 2 receipts per execution result. +func (s *ReceiptValidationSuite) TestValidateReceiptResultHasEnoughReceipts() { + k := uint(5) + s.receiptValidator = NewReceiptValidator( + s.State, + s.HeadersDB, + s.IndexDB, + s.ResultsDB, + s.SealsDB, + k, // at least 2 receipts per execution result + ) + // assuming signatures are all good + s.publicKey.On("Verify", mock.Anything, mock.Anything, mock.Anything).Return(true, nil) + + // G <- A <- B + blocks, result0, seal := unittest.ChainFixture(2) + s.SealsIndex[blocks[0].ID()] = seal + + receipts := unittest.ReceiptChainFor(blocks, result0) + blockA, blockB := blocks[1], blocks[2] + receiptA, receiptB := receipts[1], receipts[2] + + blockA.Payload.Receipts = []*flow.ExecutionReceiptMeta{} + blockB.Payload.Receipts = []*flow.ExecutionReceiptMeta{receiptA.Meta()} + blockB.Payload.Results = []*flow.ExecutionResult{&receiptA.ExecutionResult} + // update block header so that blocks are chained together + unittest.ReconnectBlocksAndReceipts(blocks, receipts) + // assuming all receipts are executed by the correct executor + for _, r := range receipts { + r.ExecutorID = s.ExeID + } + + for _, b := range blocks { + s.Extend(b) + } + s.PersistedResults[result0.ID()] = result0 + + candidateReceipts := []*flow.ExecutionReceiptMeta{receiptB.Meta()} + // add k-1 more receipts for the same execution result + for i := uint(1); i < k; i++ { + // use base receipt and change the executor ID, we don't care about signatures since we are not validating them + receipt := *receiptB.Meta() + // create a mock executor which submitted the receipt + executor := unittest.IdentityFixture(unittest.WithRole(flow.RoleExecution), unittest.WithStakingPubKey(s.publicKey)) + receipt.ExecutorID = executor.NodeID + // update local identity table so the receipt is considered valid + s.Identities[executor.NodeID] = executor + candidateReceipts = append(candidateReceipts, &receipt) + } + + candidate := unittest.BlockWithParentFixture(blockB.Header) + candidate.Payload = &flow.Payload{ + Receipts: candidateReceipts, + Results: []*flow.ExecutionResult{&receiptB.ExecutionResult}, + } + + err := s.receiptValidator.ValidatePayload(candidate) + s.Require().NoError(err) +} From 39852c073d2b313be4c6e4fc1966619ae47e7655 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 22 Nov 2023 19:49:02 +0200 Subject: [PATCH 04/10] Added a flag for configuring new parameter. Fixed tests --- cmd/consensus/main.go | 5 ++++- engine/testutil/nodes.go | 9 ++++++++- module/validation/receipt_validator.go | 2 +- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/cmd/consensus/main.go b/cmd/consensus/main.go index 41b48c352c5..ec317feb179 100644 --- a/cmd/consensus/main.go +++ b/cmd/consensus/main.go @@ -88,6 +88,7 @@ func main() { chunkAlpha uint requiredApprovalsForSealVerification uint requiredApprovalsForSealConstruction uint + requiredReceiptsCommittingToResult uint emergencySealing bool dkgControllerConfig dkgmodule.ControllerConfig dkgMessagingEngineConfig = dkgeng.DefaultMessagingEngineConfig() @@ -158,6 +159,7 @@ func main() { flags.UintVar(&chunkAlpha, "chunk-alpha", flow.DefaultChunkAssignmentAlpha, "number of verifiers that should be assigned to each chunk") flags.UintVar(&requiredApprovalsForSealVerification, "required-verification-seal-approvals", flow.DefaultRequiredApprovalsForSealValidation, "minimum number of approvals that are required to verify a seal") flags.UintVar(&requiredApprovalsForSealConstruction, "required-construction-seal-approvals", flow.DefaultRequiredApprovalsForSealConstruction, "minimum number of approvals that are required to construct a seal") + flags.UintVar(&requiredReceiptsCommittingToResult, "required-receipts-committing-to-result", flow.DefaultRequiredReceiptsCommittingToExecutionResult, "minimum number of receipts that are required to be included in the block for each execution result") flags.BoolVar(&emergencySealing, "emergency-sealing-active", flow.DefaultEmergencySealingActive, "(de)activation of emergency sealing") flags.BoolVar(&insecureAccessAPI, "insecure-access-api", false, "required if insecure GRPC connection should be used") flags.StringSliceVar(&accessNodeIDS, "access-node-ids", []string{}, fmt.Sprintf("array of access node IDs sorted in priority order where the first ID in this array will get the first connection attempt and each subsequent ID after serves as a fallback. Minimum length %d. Use '*' for all IDs in protocol state.", common.DefaultAccessNodeIDSMinimum)) @@ -265,7 +267,8 @@ func main() { node.Storage.Headers, node.Storage.Index, node.Storage.Results, - node.Storage.Seals) + node.Storage.Seals, + requiredReceiptsCommittingToResult) sealValidator := validation.NewSealValidator( node.State, diff --git a/engine/testutil/nodes.go b/engine/testutil/nodes.go index 7ff2faf0a20..063fa935ae6 100644 --- a/engine/testutil/nodes.go +++ b/engine/testutil/nodes.go @@ -457,7 +457,14 @@ func ConsensusNode(t *testing.T, hub *stub.Hub, identity *flow.Identity, identit assigner, err := chunks.NewChunkAssigner(flow.DefaultChunkAssignmentAlpha, node.State) require.Nil(t, err) - receiptValidator := validation.NewReceiptValidator(node.State, node.Headers, node.Index, resultsDB, node.Seals) + receiptValidator := validation.NewReceiptValidator( + node.State, + node.Headers, + node.Index, + resultsDB, + node.Seals, + flow.DefaultRequiredReceiptsCommittingToExecutionResult, + ) sealingEngine, err := sealing.NewEngine( node.Log, diff --git a/module/validation/receipt_validator.go b/module/validation/receipt_validator.go index 9f031ccdfd0..d92b0b67450 100644 --- a/module/validation/receipt_validator.go +++ b/module/validation/receipt_validator.go @@ -16,7 +16,7 @@ import ( ) // receiptValidator holds all needed context for checking -// receipt validity against current protocol state. +// receipt validity against the current protocol state. type receiptValidator struct { headers storage.Headers seals storage.Seals From 86ae19ac6e8e54a0d12c87a814b05a35f22996e5 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 22 Nov 2023 19:49:20 +0200 Subject: [PATCH 05/10] Linted --- 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 d92b0b67450..180b7912185 100644 --- a/module/validation/receipt_validator.go +++ b/module/validation/receipt_validator.go @@ -3,11 +3,11 @@ package validation import ( "errors" "fmt" - "github.com/onflow/flow-go/module" "github.com/onflow/flow-go/crypto/hash" "github.com/onflow/flow-go/engine" "github.com/onflow/flow-go/model/flow" + "github.com/onflow/flow-go/module" "github.com/onflow/flow-go/module/signature" "github.com/onflow/flow-go/state" "github.com/onflow/flow-go/state/fork" From 04f0d1cfb2b64b50cb5d1493bec2c8a90c37d816 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 22 Nov 2023 19:59:25 +0200 Subject: [PATCH 06/10] Simplified building of receipts by result index --- module/validation/receipt_validator.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/module/validation/receipt_validator.go b/module/validation/receipt_validator.go index 180b7912185..1716eb1671a 100644 --- a/module/validation/receipt_validator.go +++ b/module/validation/receipt_validator.go @@ -280,10 +280,7 @@ func (v *receiptValidator) ValidatePayload(candidate *flow.Block) error { // tracks the number of receipts committing to each result. // it's ok to only index receipts at this point, because we will perform // all needed checks after we have validated all results. - receiptsByResult := make(map[flow.Identifier]uint) - for _, receipt := range payload.Receipts { - receiptsByResult[receipt.ResultID]++ - } + receiptsByResult := payload.Receipts.GroupByResultID() // first validate all results that were included into payload // if one of results is invalid we fail the whole check because it could be violating parent-children relationship @@ -292,10 +289,11 @@ func (v *receiptValidator) ValidatePayload(candidate *flow.Block) error { resultID := result.ID() // check if there are enough execution receipts included in the payload corresponding to the execution result - if numberOfReceipts, hasReceipt := receiptsByResult[resultID]; hasReceipt { - if numberOfReceipts < v.requiredReceiptsIncludedForExecutionResult { + receiptsForResult := uint(len(receiptsByResult.GetGroup(resultID))) + if receiptsForResult > 0 { + if receiptsForResult < v.requiredReceiptsIncludedForExecutionResult { return engine.NewInvalidInputErrorf("execution result %v has only %d receipts, but at least %d are required", - resultID, numberOfReceipts, v.requiredReceiptsIncludedForExecutionResult) + resultID, receiptsForResult, v.requiredReceiptsIncludedForExecutionResult) } } else { return engine.NewInvalidInputErrorf("no receipts for result %v at index %d", resultID, i) From 9b0ba4c7ec4a8fe537e02512080928edcd5d73aa Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 27 Nov 2023 10:36:14 +0200 Subject: [PATCH 07/10] Made k not configurable and always be equal to 1 --- cmd/consensus/main.go | 5 +- engine/testutil/nodes.go | 1 - model/flow/constants.go | 7 --- module/validation/receipt_validator.go | 22 +++---- module/validation/receipt_validator_test.go | 69 +-------------------- 5 files changed, 10 insertions(+), 94 deletions(-) diff --git a/cmd/consensus/main.go b/cmd/consensus/main.go index ec317feb179..41b48c352c5 100644 --- a/cmd/consensus/main.go +++ b/cmd/consensus/main.go @@ -88,7 +88,6 @@ func main() { chunkAlpha uint requiredApprovalsForSealVerification uint requiredApprovalsForSealConstruction uint - requiredReceiptsCommittingToResult uint emergencySealing bool dkgControllerConfig dkgmodule.ControllerConfig dkgMessagingEngineConfig = dkgeng.DefaultMessagingEngineConfig() @@ -159,7 +158,6 @@ func main() { flags.UintVar(&chunkAlpha, "chunk-alpha", flow.DefaultChunkAssignmentAlpha, "number of verifiers that should be assigned to each chunk") flags.UintVar(&requiredApprovalsForSealVerification, "required-verification-seal-approvals", flow.DefaultRequiredApprovalsForSealValidation, "minimum number of approvals that are required to verify a seal") flags.UintVar(&requiredApprovalsForSealConstruction, "required-construction-seal-approvals", flow.DefaultRequiredApprovalsForSealConstruction, "minimum number of approvals that are required to construct a seal") - flags.UintVar(&requiredReceiptsCommittingToResult, "required-receipts-committing-to-result", flow.DefaultRequiredReceiptsCommittingToExecutionResult, "minimum number of receipts that are required to be included in the block for each execution result") flags.BoolVar(&emergencySealing, "emergency-sealing-active", flow.DefaultEmergencySealingActive, "(de)activation of emergency sealing") flags.BoolVar(&insecureAccessAPI, "insecure-access-api", false, "required if insecure GRPC connection should be used") flags.StringSliceVar(&accessNodeIDS, "access-node-ids", []string{}, fmt.Sprintf("array of access node IDs sorted in priority order where the first ID in this array will get the first connection attempt and each subsequent ID after serves as a fallback. Minimum length %d. Use '*' for all IDs in protocol state.", common.DefaultAccessNodeIDSMinimum)) @@ -267,8 +265,7 @@ func main() { node.Storage.Headers, node.Storage.Index, node.Storage.Results, - node.Storage.Seals, - requiredReceiptsCommittingToResult) + node.Storage.Seals) sealValidator := validation.NewSealValidator( node.State, diff --git a/engine/testutil/nodes.go b/engine/testutil/nodes.go index 063fa935ae6..da5d3feb9c5 100644 --- a/engine/testutil/nodes.go +++ b/engine/testutil/nodes.go @@ -463,7 +463,6 @@ func ConsensusNode(t *testing.T, hub *stub.Hub, identity *flow.Identity, identit node.Index, resultsDB, node.Seals, - flow.DefaultRequiredReceiptsCommittingToExecutionResult, ) sealingEngine, err := sealing.NewEngine( diff --git a/model/flow/constants.go b/model/flow/constants.go index 7e4e9b4d634..6b03c36a6db 100644 --- a/model/flow/constants.go +++ b/model/flow/constants.go @@ -61,13 +61,6 @@ const DefaultRequiredApprovalsForSealConstruction = uint(1) // - Full protocol should be +2/3 of all currently authorized verifiers. const DefaultRequiredApprovalsForSealValidation = 0 -// DefaultRequiredReceiptsCommittingToExecutionResult is the default number of receipts that should be committing to an execution -// result which is being incorporated into a candidate block. -// ATTENTION: -// For each result in the candidate block, there must be k receipts included in the candidate block, with k strictly larger than 0. -// This value has to be strictly larger than 0, otherwise leader can incorporate a result which was not executed. -const DefaultRequiredReceiptsCommittingToExecutionResult = 1 - // DefaultChunkAssignmentAlpha is the default number of verifiers that should be // assigned to each chunk. const DefaultChunkAssignmentAlpha = 3 diff --git a/module/validation/receipt_validator.go b/module/validation/receipt_validator.go index 1716eb1671a..fe82147125f 100644 --- a/module/validation/receipt_validator.go +++ b/module/validation/receipt_validator.go @@ -18,13 +18,12 @@ import ( // receiptValidator holds all needed context for checking // receipt validity against the current protocol state. type receiptValidator struct { - headers storage.Headers - seals storage.Seals - state protocol.State - index storage.Index - results storage.ExecutionResults - signatureHasher hash.Hasher - requiredReceiptsIncludedForExecutionResult uint + headers storage.Headers + seals storage.Seals + state protocol.State + index storage.Index + results storage.ExecutionResults + signatureHasher hash.Hasher } var _ module.ReceiptValidator = (*receiptValidator)(nil) @@ -34,7 +33,6 @@ func NewReceiptValidator(state protocol.State, index storage.Index, results storage.ExecutionResults, seals storage.Seals, - requiredReceiptsIncludedForExecutionResult uint, ) module.ReceiptValidator { rv := &receiptValidator{ state: state, @@ -43,7 +41,6 @@ func NewReceiptValidator(state protocol.State, results: results, signatureHasher: signature.NewBLSHasher(signature.ExecutionReceiptTag), seals: seals, - requiredReceiptsIncludedForExecutionResult: requiredReceiptsIncludedForExecutionResult, } return rv } @@ -290,12 +287,7 @@ func (v *receiptValidator) ValidatePayload(candidate *flow.Block) error { // check if there are enough execution receipts included in the payload corresponding to the execution result receiptsForResult := uint(len(receiptsByResult.GetGroup(resultID))) - if receiptsForResult > 0 { - if receiptsForResult < v.requiredReceiptsIncludedForExecutionResult { - return engine.NewInvalidInputErrorf("execution result %v has only %d receipts, but at least %d are required", - resultID, receiptsForResult, v.requiredReceiptsIncludedForExecutionResult) - } - } else { + if receiptsForResult == 0 { return engine.NewInvalidInputErrorf("no receipts for result %v at index %d", resultID, i) } diff --git a/module/validation/receipt_validator_test.go b/module/validation/receipt_validator_test.go index a438a026c27..21b9e21c113 100644 --- a/module/validation/receipt_validator_test.go +++ b/module/validation/receipt_validator_test.go @@ -35,7 +35,6 @@ func (s *ReceiptValidationSuite) SetupTest() { s.IndexDB, s.ResultsDB, s.SealsDB, - 1, ) } @@ -794,66 +793,15 @@ func (s *ReceiptValidationSuite) TestValidateReceiptResultWithoutReceipt() { s.Require().True(engine.IsInvalidInputError(err)) } -// TestValidateReceiptResultHasNotEnoughReceipts tests a case when a malicious leader incorporates an execution result -// into their proposal, which has a receipt, but ReceiptValidator is configured to accept at least 2 receipts per execution result. -func (s *ReceiptValidationSuite) TestValidateReceiptResultHasNotEnoughReceipts() { - s.receiptValidator = NewReceiptValidator( - s.State, - s.HeadersDB, - s.IndexDB, - s.ResultsDB, - s.SealsDB, - 2, // at least 2 receipts per execution result - ) - // assuming signatures are all good - s.publicKey.On("Verify", mock.Anything, mock.Anything, mock.Anything).Return(true, nil) - - // G <- A <- B - blocks, result0, seal := unittest.ChainFixture(2) - s.SealsIndex[blocks[0].ID()] = seal - - receipts := unittest.ReceiptChainFor(blocks, result0) - blockA, blockB := blocks[1], blocks[2] - receiptA, receiptB := receipts[1], receipts[2] - - blockA.Payload.Receipts = []*flow.ExecutionReceiptMeta{} - blockB.Payload.Receipts = []*flow.ExecutionReceiptMeta{receiptA.Meta()} - blockB.Payload.Results = []*flow.ExecutionResult{&receiptA.ExecutionResult} - // update block header so that blocks are chained together - unittest.ReconnectBlocksAndReceipts(blocks, receipts) - // assuming all receipts are executed by the correct executor - for _, r := range receipts { - r.ExecutorID = s.ExeID - } - - for _, b := range blocks { - s.Extend(b) - } - s.PersistedResults[result0.ID()] = result0 - - candidate := unittest.BlockWithParentFixture(blockB.Header) - candidate.Payload = &flow.Payload{ - Receipts: []*flow.ExecutionReceiptMeta{receiptB.Meta()}, - Results: []*flow.ExecutionResult{&receiptB.ExecutionResult}, - } - - err := s.receiptValidator.ValidatePayload(candidate) - s.Require().Error(err) - s.Require().True(engine.IsInvalidInputError(err)) -} - // TestValidateReceiptResultHasEnoughReceipts tests a case where a leader incorporates an execution result -// into their proposal, which has multiple receipts and ReceiptValidator, -// is configured to accept at least 2 receipts per execution result. +// into their proposal, which has multiple receipts and ReceiptValidator. func (s *ReceiptValidationSuite) TestValidateReceiptResultHasEnoughReceipts() { - k := uint(5) s.receiptValidator = NewReceiptValidator( s.State, s.HeadersDB, s.IndexDB, s.ResultsDB, s.SealsDB, - k, // at least 2 receipts per execution result ) // assuming signatures are all good s.publicKey.On("Verify", mock.Anything, mock.Anything, mock.Anything).Return(true, nil) @@ -881,22 +829,9 @@ func (s *ReceiptValidationSuite) TestValidateReceiptResultHasEnoughReceipts() { } s.PersistedResults[result0.ID()] = result0 - candidateReceipts := []*flow.ExecutionReceiptMeta{receiptB.Meta()} - // add k-1 more receipts for the same execution result - for i := uint(1); i < k; i++ { - // use base receipt and change the executor ID, we don't care about signatures since we are not validating them - receipt := *receiptB.Meta() - // create a mock executor which submitted the receipt - executor := unittest.IdentityFixture(unittest.WithRole(flow.RoleExecution), unittest.WithStakingPubKey(s.publicKey)) - receipt.ExecutorID = executor.NodeID - // update local identity table so the receipt is considered valid - s.Identities[executor.NodeID] = executor - candidateReceipts = append(candidateReceipts, &receipt) - } - candidate := unittest.BlockWithParentFixture(blockB.Header) candidate.Payload = &flow.Payload{ - Receipts: candidateReceipts, + Receipts: []*flow.ExecutionReceiptMeta{receiptB.Meta()}, Results: []*flow.ExecutionResult{&receiptB.ExecutionResult}, } From 856c8cca8a5c612fa30aafce276b85d189a9f986 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 27 Nov 2023 20:19:26 +0200 Subject: [PATCH 08/10] Added back test for k > 1 --- module/validation/receipt_validator_test.go | 25 +++++++++++++-------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/module/validation/receipt_validator_test.go b/module/validation/receipt_validator_test.go index 21b9e21c113..8947cee40ec 100644 --- a/module/validation/receipt_validator_test.go +++ b/module/validation/receipt_validator_test.go @@ -794,15 +794,9 @@ func (s *ReceiptValidationSuite) TestValidateReceiptResultWithoutReceipt() { } // TestValidateReceiptResultHasEnoughReceipts tests a case where a leader incorporates an execution result -// into their proposal, which has multiple receipts and ReceiptValidator. +// into their proposal, which has multiple receipts and ReceiptValidator accepts it as valid payload. func (s *ReceiptValidationSuite) TestValidateReceiptResultHasEnoughReceipts() { - s.receiptValidator = NewReceiptValidator( - s.State, - s.HeadersDB, - s.IndexDB, - s.ResultsDB, - s.SealsDB, - ) + k := uint(5) // assuming signatures are all good s.publicKey.On("Verify", mock.Anything, mock.Anything, mock.Anything).Return(true, nil) @@ -829,9 +823,22 @@ func (s *ReceiptValidationSuite) TestValidateReceiptResultHasEnoughReceipts() { } s.PersistedResults[result0.ID()] = result0 + candidateReceipts := []*flow.ExecutionReceiptMeta{receiptB.Meta()} + // add k-1 more receipts for the same execution result + for i := uint(1); i < k; i++ { + // use base receipt and change the executor ID, we don't care about signatures since we are not validating them + receipt := *receiptB.Meta() + // create a mock executor which submitted the receipt + executor := unittest.IdentityFixture(unittest.WithRole(flow.RoleExecution), unittest.WithStakingPubKey(s.publicKey)) + receipt.ExecutorID = executor.NodeID + // update local identity table so the receipt is considered valid + s.Identities[executor.NodeID] = executor + candidateReceipts = append(candidateReceipts, &receipt) + } + candidate := unittest.BlockWithParentFixture(blockB.Header) candidate.Payload = &flow.Payload{ - Receipts: []*flow.ExecutionReceiptMeta{receiptB.Meta()}, + Receipts: candidateReceipts, Results: []*flow.ExecutionResult{&receiptB.ExecutionResult}, } From 2590981e9a6ddc37495dbfc791f8a97c27b3b3d0 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 28 Nov 2023 10:02:31 +0200 Subject: [PATCH 09/10] Update module/validation/receipt_validator.go Co-authored-by: Jordan Schalm --- module/validation/receipt_validator.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/module/validation/receipt_validator.go b/module/validation/receipt_validator.go index fe82147125f..9ff06b1a2ce 100644 --- a/module/validation/receipt_validator.go +++ b/module/validation/receipt_validator.go @@ -285,7 +285,8 @@ func (v *receiptValidator) ValidatePayload(candidate *flow.Block) error { for i, result := range payload.Results { resultID := result.ID() - // check if there are enough execution receipts included in the payload corresponding to the execution result + // Every included result must be accompanied by a receipt with a corresponding `ResultID`, in the same block. + // If a result is included without a corresponding receipt, it cannot be attributed to any executor. receiptsForResult := uint(len(receiptsByResult.GetGroup(resultID))) if receiptsForResult == 0 { return engine.NewInvalidInputErrorf("no receipts for result %v at index %d", resultID, i) From ca26248fcbadb805a554751d6be2bf06007616d3 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 19 Dec 2023 13:12:19 +0200 Subject: [PATCH 10/10] Apply suggestions from PR review --- module/validation/receipt_validator.go | 4 +--- module/validation/receipt_validator_test.go | 9 +++++++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/module/validation/receipt_validator.go b/module/validation/receipt_validator.go index 9ff06b1a2ce..942c841b584 100644 --- a/module/validation/receipt_validator.go +++ b/module/validation/receipt_validator.go @@ -279,9 +279,7 @@ func (v *receiptValidator) ValidatePayload(candidate *flow.Block) error { // all needed checks after we have validated all results. receiptsByResult := payload.Receipts.GroupByResultID() - // first validate all results that were included into payload - // if one of results is invalid we fail the whole check because it could be violating parent-children relationship - // each execution + // validate all results that are incorporated into the payload. If one is malformed, the entire block is invalid. for i, result := range payload.Results { resultID := result.ID() diff --git a/module/validation/receipt_validator_test.go b/module/validation/receipt_validator_test.go index 8947cee40ec..81a4bed321a 100644 --- a/module/validation/receipt_validator_test.go +++ b/module/validation/receipt_validator_test.go @@ -793,8 +793,13 @@ func (s *ReceiptValidationSuite) TestValidateReceiptResultWithoutReceipt() { s.Require().True(engine.IsInvalidInputError(err)) } -// TestValidateReceiptResultHasEnoughReceipts tests a case where a leader incorporates an execution result -// into their proposal, which has multiple receipts and ReceiptValidator accepts it as valid payload. +// TestValidateReceiptResultHasEnoughReceipts tests the happy path of a block proposal, where a leader +// includes multiple Execution Receipts that commit to the same result. In this case, the Flow protocol +// prescribes that +// - the Execution Result is only incorporated once +// - from each Receipt the `ExecutionReceiptMeta` is to be included. +// +// The validator is expected to accept such payload as valid. func (s *ReceiptValidationSuite) TestValidateReceiptResultHasEnoughReceipts() { k := uint(5) // assuming signatures are all good