Properly handle user and implementation errors returned from atree

[turbolent]

Problem

Cadence uses the atree library for some of its data structures (arrays, dictionaries, and composite values).

Currently, all errors returned by atree are considered implementation errors. However, this is not always the case. For example atree might use FVM to read data from storage, and FVM may return a user error. Such a user error should be propagated to Cadence as is and not be treated like an implementation error.

Suggested solution

Audit uses of atree in Cadence and poperly handle / distinguish between user and implementation errors.

Notes

• https://github.com/onflow/cadence/blob/81c63bfef73cd4874d05b6274dbd2402a0a0b788/runtime/interpreter/value.go#L9216

[turbolent]

• Maybe use shared error library between FVM, Cadence, and atree

[turbolent]

TODO: Follow up on outcome from meeting with @fxamacker @janezpodhostnik @tarakby

[turbolent]

Want to have separate library for error types and use it across atree, Cadence, FVM, etc.

[turbolent]

New graceful error recovery added in https://github.com/dapperlabs/cadence-internal/pull/44 (port: #1362) should maybe now handle this. TODO:

• Write / check there is a Cadence test case which panics during storage write, performed through atree.
• Double check graceful error recovery: What types are bubbled up, which ones are treated as user errors? Need to ensure implementation errors are not treated as user errors

[j1010001]

Not blocking the release (not a security issue) but a tech debt we want to address ASAP. Also affects dev experience with error message that is goo generic.

[j1010001]

Post Secure Cadence cleanup

[j1010001]

bumping up the priority as this has been causing some pain in misleading errors: https://dapperlabs.slack.com/archives/C03C40ZV82C/p1672956533133159

[turbolent]

• [x] @fxamacker atree: Categorize errors into
  • Fatal errors (e.g. integrity check failing)
  • External errors (e.g. Ledger calls, callbacks (e.g. iteration); needs Unwrap function), and
  • User errors (e.g. index out-of-bounds)
• [x] @dsainati1 Cadence: Add Unwrap function to ExternalError, so FVM can unwrap

[janezpodhostnik]

This is the FVM code used to unwrap an ExternalError because it doesn't have an Unwrap method https://github.com/onflow/flow-go/blob/c92aafc/fvm/errors/errors.go#L105. This could be removed if ExternalError could be unwrapped.

This is an error report https://github.com/onflow/flow-go/issues/3729 of an instance where Cadence misinterprets an error and because of that adds the whole call stack to the error.

Here is the code of how the FVM differentiates an implementation error from a user error: https://github.com/onflow/flow-go/blob/c92aafc3b93c0668a5d5e7da048b08bdae174809/fvm/errors/errors.go#L70 It basically boils down to:

• if its a CodedError (FVM error) and if its code is a failure code then it is an implementation error, if not, unwrap it and check if inside is an implementation error
• if it is an unwrappable error check if it contains an implementation error
• if it is not unwrappable and not a CodedError, then it is an implementation error

[fxamacker]

onflow/atree#295 was merged, which should unblock this.

cc: @dsainati1 @j1010001 @turbolent

[j1010001]

• [x] update the atree version to the latest and test that it is all integrated correctly

[fxamacker]

PR #2426 uses latest atree and updates handling of user errors.

If additional integration is needed, please feel free to open separate PRs as that may require more knowledge of Cadence internals.

I can tag a new release of atree if we didn't miss anything and this works out.

cc: @j1010001 @turbolent

[fxamacker]

• [x] Tagged onflow/atree v0.6.0.
• [x] Updated PR #2426 to use atree v0.6.0.

[j1010001]

Keeping open for a bit longer in case we find new types of errors that are needed from Atree.