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

[Sealing] Validation of multiple receipts in payload #323

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions engine/consensus/matching/engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func (ms *MatchingSuite) TestOnReceiptPendingReceipt() {
unittest.WithResult(unittest.ExecutionResultFixture(unittest.WithBlock(&ms.UnfinalizedBlock))),
)

ms.receiptValidator.On("Validate", receipt).Return(nil)
ms.receiptValidator.On("Validate", []*flow.ExecutionReceipt{receipt}).Return(nil)

// setup the receipts mempool to check if we attempted to add the receipt to
// the mempool, and return false as we are testing the case where it was already in the mempool
Expand All @@ -162,7 +162,7 @@ func (ms *MatchingSuite) TestOnReceiptPendingResult() {
unittest.WithResult(unittest.ExecutionResultFixture(unittest.WithBlock(&ms.UnfinalizedBlock))),
)

ms.receiptValidator.On("Validate", receipt).Return(nil)
ms.receiptValidator.On("Validate", []*flow.ExecutionReceipt{receipt}).Return(nil)

// setup the receipts mempool to check if we attempted to add the receipt to
// the mempool
Expand Down Expand Up @@ -196,7 +196,7 @@ func (ms *MatchingSuite) TestOnReceiptValid() {
unittest.WithResult(unittest.ExecutionResultFixture(unittest.WithBlock(&ms.UnfinalizedBlock))),
)

ms.receiptValidator.On("Validate", receipt).Return(nil).Once()
ms.receiptValidator.On("Validate", []*flow.ExecutionReceipt{receipt}).Return(nil).Once()

// we expect that receipt is added to mempool
ms.ReceiptsPL.On("Add", receipt).Return(true).Once()
Expand Down Expand Up @@ -224,7 +224,7 @@ func (ms *MatchingSuite) TestOnReceiptInvalid() {
unittest.WithExecutorID(originID),
unittest.WithResult(unittest.ExecutionResultFixture(unittest.WithBlock(&ms.UnfinalizedBlock))),
)
ms.receiptValidator.On("Validate", receipt).Return(engine.NewInvalidInputError("")).Once()
ms.receiptValidator.On("Validate", []*flow.ExecutionReceipt{receipt}).Return(engine.NewInvalidInputError("")).Once()

err := ms.matching.onReceipt(receipt.ExecutorID, receipt)
ms.Require().Error(err, "should reject receipt that does not pass ReceiptValidator")
Expand Down
31 changes: 28 additions & 3 deletions module/validation/receipt_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ import (
"github.com/onflow/flow-go/storage"
)

// Functor that is used to retrieve parent of ExecutionResult.
type GetPreviousResult func(*flow.ExecutionResult) (*flow.ExecutionResult, error)

// receiptValidator holds all needed context for checking
// receipt validity against current protocol state.
type receiptValidator struct {
Expand Down Expand Up @@ -172,16 +175,38 @@ func (v *receiptValidator) resultChainCheck(result *flow.ExecutionResult, prevRe
// * execution result has a valid parent
// Returns nil if all checks passed successfully
func (v *receiptValidator) Validate(receipts []*flow.ExecutionReceipt) error {
// lookup cache to avoid linear search when checking for previous result that is
// part of payload
payloadExecutionResults := make(map[flow.Identifier]*flow.ExecutionResult)
for _, receipt := range receipts {
payloadExecutionResults[receipt.ExecutionResult.ID()] = &receipt.ExecutionResult
}
// Build a functor that performs lookup first in receipts that were passed as payload and only then in
// local storage. This is needed to handle a case when same block payload contains receipts that
// reference each other.
// ATTENTION: Here we assume that ER is valid, this lookup can return a result which is actually invalid.
// Eventually invalid result will be detected and fail the whole validation.
previousResult := func(executionResult *flow.ExecutionResult) (*flow.ExecutionResult, error) {
Copy link
Member

Choose a reason for hiding this comment

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

when validating a receipt, we assume the previous result from the same payload is "valid", and so that if this assumption fails, eventually the entire validation will fail.

The solution is simple and elegant, however I'm still not feeling very comfortable about it, even though I couldn't think of case that leads to an incorrect behavior yet

prevResult, found := payloadExecutionResults[executionResult.PreviousResultID]
if found {
return prevResult, nil
}

return v.previousResult(executionResult)
}

for i, r := range receipts {
err := v.validate(r)
err := v.validate(r, previousResult)
if err != nil {
// It's very important that we fail the whole validation if one of the receipts is invalid.
// It allows us to make assumptions as stated in previous comment.
return fmt.Errorf("could not validate receipt %v at index %v: %w", r.ID(), i, err)
}
}
return nil
}

func (v *receiptValidator) validate(receipt *flow.ExecutionReceipt) error {
func (v *receiptValidator) validate(receipt *flow.ExecutionReceipt, previousResult GetPreviousResult) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

Did it this way to postpone storage lookup till it's needed and not too put validation in loop body.

durkmurder marked this conversation as resolved.
Show resolved Hide resolved
identity, err := identityForNode(v.state, receipt.ExecutionResult.BlockID, receipt.ExecutorID)
if err != nil {
return fmt.Errorf(
Expand All @@ -206,7 +231,7 @@ func (v *receiptValidator) validate(receipt *flow.ExecutionReceipt) error {
return fmt.Errorf("invalid chunks format for result %v: %w", receipt.ExecutionResult.ID(), err)
}

prevResult, err := v.previousResult(&receipt.ExecutionResult)
prevResult, err := previousResult(&receipt.ExecutionResult)
if err != nil {
return err
}
Expand Down