flow-go icon indicating copy to clipboard operation
flow-go copied to clipboard

receipt validator error handling

Open AlexHentschel opened this issue 1 year ago • 1 comments

This PR updates and refines ReceiptValidator's internal error handling.

Due to our consistent work on error handling, some of the lower-level components that ReceiptValidator calls into document their errors better now compared to the time the ReceiptValidator was initially implemented 2 years ago. With a few minor documentation improvements (part of this PR) of some storage components, it is now better possible to comprehensively specify the error handling and error propagation within the ReceiptValidator, which is the main focus of this PR. Details:

  • The central algorithmic improvement this PR brings is that the receipt validator now clearly identifies via dedicated sentinel errors (UnknownResultError and UnknownBlockError) which data is missing to verify receipts. Previously this was all lumped together into UnverifiableInputError. But depending on the caller, a missing parent block might indicate state corruption or just be expected. Similarly, a missing parent result could be fatal or expected. Though, UnverifiableInputError is too coarse, for the calling code to easily differentiate expected from unexpected error scenarios.
  • we had technical debt where code calling into the receipt validator was checking for errors that were not documented in the API: https://github.com/onflow/flow-go/blob/645923211a85641ade7d335c773d627390de4901/state/protocol/badger/mutator.go#L444-L446 However, the error propagation in the receipt validator internally was only sparsely documented so it was very hard to see where the storage.ErrNotFound could be possibly originating from and whether this check was still useful. This PR consistently documents error propagation within the ReceiptValidator and cleans up this tech debt in the ParticipantState
  • We had the following TODO in the ParticipantState, where our understanding has evolved since: https://github.com/onflow/flow-go/blob/645923211a85641ade7d335c773d627390de4901/state/protocol/badger/mutator.go#L443 It turned out that this error actually indicates fatal state corruption, but not just some missing data that could be requested. This PR updates and documents this detail.
  • While the error documentation of Snapshot was lacking: https://github.com/onflow/flow-go/blob/f3bb713dcad80d86ea686bc2aa6ba34513756fdf/state/protocol/snapshot.go#L36-L37 we were checking for specific error types already in the ReceiptValidator. This PR extends the error documentation of Snapshot.Head() (I reviewed its implementation to make sure that ErrUnknownSnapshotReference is really the only possible benign error type).
  • I think the FollowerState had an edge case, where it would erroneously interpret a missing parent block as the candidate block being invalid. This could lead to a wrong slashing challenge! This PR fixes this problem.
  • updated and extended test

This PR should be fully downwards compatible, i.e. it can be merged into master and rolled out to nodes at arbitrary times.

AlexHentschel avatar Jan 06 '24 04:01 AlexHentschel