[BFT] `ReceiptValidator` ensures `k` receipts committing to the execution result #5050

durkmurder · 2023-11-22T17:57:13Z

https://github.com/dapperlabs/flow-go/issues/6863

Context

As summarized in the issue currently ReceiptValidator is not ensuring that an execution result was actually executed, by not checking the availability of receipt we allow a malicious leader to incorporate made-up execution results. This PR fixes is by introducing a parameter k which controls how many different execution receipts have to be committing to the execution result

… for execution result to be considered valid

… k receipts in candidate block

codecov-commenter · 2023-11-22T18:08:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (456c131) 56.31% compared to head (103de18) 56.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5050      +/-   ##
==========================================
- Coverage   56.31%   56.30%   -0.01%     
==========================================
  Files         976      976              
  Lines       91752    91761       +9     
==========================================
- Hits        51673    51670       -3     
- Misses      36253    36265      +12     
  Partials     3826     3826

Flag	Coverage Δ
unittests	`56.30% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jordanschalm

Looks great, however I think we can simplify by removing the parameter k.

My interpretation of the issue is that k was meant to abstractly describe the set of valid payload inputs the implementation needs to accept, not to actually be a parameter we add to the implementation. I don't think there is a need for k to be configurable.

cmd/consensus/main.go

module/validation/receipt_validator.go

cmd/consensus/main.go

jordanschalm · 2023-11-23T16:12:03Z

module/validation/receipt_validator_test.go

+	s.Require().True(engine.IsInvalidInputError(err))
+}
+
+// TestValidateReceiptResultHasNotEnoughReceipts tests a case when a malicious leader incorporates an execution result


I think we can remove this test case. If k is constant, then this is equivalent to the previous test.

oh, alright, I thought that it's useful for some BFT cases in future development. I will remove it.

jordanschalm · 2023-11-27T16:56:13Z

module/validation/receipt_validator_test.go

@@ -881,22 +829,9 @@ func (s *ReceiptValidationSuite) TestValidateReceiptResultHasEnoughReceipts() {
 	}
 	s.PersistedResults[result0.ID()] = result0

-	candidateReceipts := []*flow.ExecutionReceiptMeta{receiptB.Meta()}


I think it is still worthwhile to test both valid cases:

Proposer includes result and 1 receipt in block

Proposer includes result and multiple receipts in block

added it back 856c8cc

jordanschalm

🎸

module/validation/receipt_validator.go

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

…-of-receipts

AlexHentschel

great code and detailed test 🎵

AlexHentschel · 2023-12-19T04:10:33Z

module/validation/receipt_validator.go

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


In this function ValidatePayload, we are checking the protocol compliance of all the results in the payload. Anything that violates protocol rules makes the entire block invalid. I think arguing at that level is totally fine. I don't think we need to go into the details of parent-children relationship (tbh, I didn't follow that argument about parent-children relationship ... it might have well been me adding or suggesting this comment).

Suggested change

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

// Validate all results that are incorporated into the payload. If one is malformed, the entire block is invalid.

AlexHentschel · 2023-12-19T04:23:32Z

module/validation/receipt_validator_test.go

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


I think it would help to extend this description a bit.

Suggested change

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

…-of-receipts

durkmurder added 5 commits November 22, 2023 19:43

Added a constant for verifying how many execution receipts are needed…

77c274e

… for execution result to be considered valid

Added implementation of checks that ensure that we have seen at least…

b76556e

… k receipts in candidate block

Added new tests to cover happy and failure paths

e10d39b

Added a flag for configuring new parameter. Fixed tests

39852c0

Linted

86ae19a

durkmurder assigned jordanschalm Nov 22, 2023

durkmurder requested review from AlexHentschel and jordanschalm as code owners November 22, 2023 17:57

durkmurder assigned AlexHentschel Nov 22, 2023

Simplified building of receipts by result index

04f0d1c

jordanschalm reviewed Nov 23, 2023

View reviewed changes

Made k not configurable and always be equal to 1

9b0ba4c

jordanschalm reviewed Nov 27, 2023

View reviewed changes

Added back test for k > 1

856c8cc

jordanschalm approved these changes Nov 27, 2023

View reviewed changes

module/validation/receipt_validator.go Outdated Show resolved Hide resolved

durkmurder and others added 4 commits November 28, 2023 10:02

Update module/validation/receipt_validator.go

2590981

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

Merge branch 'master' into yurii/6863-receipt-validator-checks-number…

fc664cf

…-of-receipts

Merge branch 'master' into yurii/6863-receipt-validator-checks-number…

103de18

…-of-receipts

Merge branch 'master' into yurii/6863-receipt-validator-checks-number…

b35446d

…-of-receipts

AlexHentschel approved these changes Dec 19, 2023

View reviewed changes

durkmurder added 2 commits December 19, 2023 13:12

Apply suggestions from PR review

ca26248

Merge branch 'master' into yurii/6863-receipt-validator-checks-number…

d99b7b9

…-of-receipts

durkmurder enabled auto-merge December 19, 2023 12:00

durkmurder added this pull request to the merge queue Dec 19, 2023

Merged via the queue into master with commit 9dfe842 Dec 19, 2023
51 checks passed

durkmurder deleted the yurii/6863-receipt-validator-checks-number-of-receipts branch December 19, 2023 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BFT] `ReceiptValidator` ensures `k` receipts committing to the execution result #5050

[BFT] `ReceiptValidator` ensures `k` receipts committing to the execution result #5050

durkmurder commented Nov 22, 2023

codecov-commenter commented Nov 22, 2023 •

edited

Loading

jordanschalm left a comment

jordanschalm Nov 23, 2023

durkmurder Nov 27, 2023

jordanschalm Nov 27, 2023

durkmurder Nov 27, 2023

jordanschalm left a comment

AlexHentschel left a comment

AlexHentschel Dec 19, 2023

AlexHentschel Dec 19, 2023

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

[BFT] ReceiptValidator ensures k receipts committing to the execution result #5050

[BFT] ReceiptValidator ensures k receipts committing to the execution result #5050

Conversation

durkmurder commented Nov 22, 2023

Context

codecov-commenter commented Nov 22, 2023 • edited Loading

Codecov Report

jordanschalm left a comment

Choose a reason for hiding this comment

jordanschalm Nov 23, 2023

Choose a reason for hiding this comment

durkmurder Nov 27, 2023

Choose a reason for hiding this comment

jordanschalm Nov 27, 2023

Choose a reason for hiding this comment

durkmurder Nov 27, 2023

Choose a reason for hiding this comment

jordanschalm left a comment

Choose a reason for hiding this comment

AlexHentschel left a comment

Choose a reason for hiding this comment

AlexHentschel Dec 19, 2023

Choose a reason for hiding this comment

AlexHentschel Dec 19, 2023

Choose a reason for hiding this comment

[BFT] `ReceiptValidator` ensures `k` receipts committing to the execution result #5050

[BFT] `ReceiptValidator` ensures `k` receipts committing to the execution result #5050

codecov-commenter commented Nov 22, 2023 •

edited

Loading