Validation of execution receipts

[durkmurder]

This PR contains implementation of validation of execution receipts for consensus nodes.
Currently it's not finished, tests are not written. Created it to get approval for selected implementation approach.

[jordanschalm]

Nice work 👏

The chunk verification looks good to me, but alex/martin will be better suited to check that part.

[jordanschalm]

Do we need to instantiate a new ReceiptValidator each time? I believe we should be able to instantiate one instance when constructing the State and use that. The protocol.State and all storage.* components should be thread-safe.

[durkmurder]

Probably yes, but the problem is that we get access to state only in this place, and this factory gets injected into NewState so it should or this way or state has to be passed to factory after it's created.

[AlexHentschel]

It feels a little awkward, I admit. Nevertheless, I think this is the best starting point for me to work towards issue #5103

[AlexHentschel]

Some high-level comments regarding code structure.

[AlexHentschel]

some more detailed comments regarding the implementation details in receiptValidator

[AlexHentschel]

There are two sentinel errors which Identity(...) can return:

1. protocol.IdentityNotFoundErr: we have the block blockID but at this block, but nodeID does not reference an authorized network participant at that block
2. storage.ErrNotFound if there is no block with blockID known (meaning, we have gotten the receipt before the block)

Handling of case 2. is missing here. For now, I think we can treat the receipt as invalid (we might want to further differentiate in the future).

[durkmurder]

yes, we discussed this, I believe at this point we treat everything as error and will do that later. I can add TODO for this one.

[AlexHentschel]

Looks good to me.

• Some minor comments regarding style
• Lets make sure we get NewInvalidExtensionErrorf correct (see comment below)

[AlexHentschel]

would suggest to inject the module.Verifier implementation

[AlexHentschel]

Suggested change

	// checkIsStakedNodeWithRole checks whether, at the given block, `nodeID`
	// * has _positive_ weight
	// * and has the expected role
	// Returns the following errors:
	// * sentinel engine.InvalidInputError if any of the above-listed conditions are violated.
	// Note: the method receives the identity as proof of its existence.
	// Therefore, we consider the case where the respective identity is unknown to the
	// protocol state as a symptom of a fatal implementation bug.
	// ensureStakedNodeWithRole checks whether, at the given node identity
	// * has _positive_ weight
	// * and has the expected role
	// Returns the following errors:
	// * sentinel engine.InvalidInputError if any of the above-listed conditions are violated.

[AlexHentschel]

⚠️
I don't think NewInvalidExtensionErrorf supports using %w as a verb, because it internally uses Sprintf:

flow-go/state/errors.go

Lines 19 to 21 in c5671a2

	func NewInvalidExtensionErrorf(msg string, args ...interface{}) error {
	return NewInvalidExtensionError(fmt.Sprintf(msg, args...))
	}

made an example on the Go Playground: https://play.golang.org/p/SoRRu6Pppt8

However, it would work if we changed InvalidExtensionError to wrap an error, like this:

flow-go/engine/errors.go

Lines 10 to 42 in c5671a2

	// InvalidInputError are errors for caused by invalid inputs.
	// It's useful to distinguish these known errors from exceptions.
	// By distinguishing errors from exceptions, we can log them
	// differently.
	// For instance, log InvalidInputError error as a warn log, and log
	// other error as an error log.
	type InvalidInputError struct {
	err error
	}
	func NewInvalidInputError(msg string) error {
	return NewInvalidInputErrorf(msg)
	}
	func NewInvalidInputErrorf(msg string, args ...interface{}) error {
	return InvalidInputError{
	err: fmt.Errorf(msg, args...),
	}
	}
	func (e InvalidInputError) Unwrap() error {
	return e.err
	}
	func (e InvalidInputError) Error() string {
	return e.err.Error()
	}
	// IsInvalidInputError returns whether the given error is an InvalidInputError error
	func IsInvalidInputError(err error) bool {
	var errInvalidInputError InvalidInputError
	return errors.As(err, &errInvalidInputError)
	}

Note: you have to add the Unwrap() method

[AlexHentschel]

for Identifier, we generally use the verb %v (or %x) ... rarely %s (though, I think it should work). @jordanschalm would know best

[jordanschalm]

They all work :)

flow-go/model/flow/identifier.go

Lines 44 to 53 in 1e0a30e

	// Format handles formatting of id for different verbs. This is called when
	// formatting an identifier with fmt.
	func (id Identifier) Format(state fmt.State, verb rune) {
	switch verb {
	case 'x', 's', 'v':
	_, _ = state.Write([]byte(id.String()))
	default:
	_, _ = state.Write([]byte(fmt.Sprintf("%%!%c(%s=%s)", verb, reflect.TypeOf(id), id)))
	}
	}

[AlexHentschel]

not sure this is needed. By wrapping

flow-go/state/protocol/badger/mutator.go

Line 246 in c5671a2

return fmt.Errorf("payload receipts not compliant with chain state: %w", err)

the error type is preserved.

For example:

        err0 := NewInvalidExtensionError(msg string)
        err1 := fmt.Errorf("payload receipts not compliant with chain state: %w", err0)
 
        state.IsInvalidExtensionError(err0) // true
        state.IsInvalidExtensionError(err1) // true

In fact, it is good practise to wrap errors to make expose information about the error trace. In other words, even if err is an InvalidExtensionError, it would still be preferred to wrap the error here:

return fmt.Errorf("payload receipts not compliant with chain state: %w", err0)

I am sure you have reasons for wrapping the error / not wrapping the error depending on its type. Maybe my earlier suggestion would also remove the need to not wrap the error here?

[zhangchiqing]

Not sure if we need the error, if we know the error is about an invalid extension of the protocol state, can we just convert the error into an error message to tell what went wrong?

[AlexHentschel]

This is inspired by a pattern Max Walter introduced:

flow-go/engine/errors.go

Lines 10 to 42 in c5671a2

	// InvalidInputError are errors for caused by invalid inputs.
	// It's useful to distinguish these known errors from exceptions.
	// By distinguishing errors from exceptions, we can log them
	// differently.
	// For instance, log InvalidInputError error as a warn log, and log
	// other error as an error log.
	type InvalidInputError struct {
	err error
	}
	func NewInvalidInputError(msg string) error {
	return NewInvalidInputErrorf(msg)
	}
	func NewInvalidInputErrorf(msg string, args ...interface{}) error {
	return InvalidInputError{
	err: fmt.Errorf(msg, args...),
	}
	}
	func (e InvalidInputError) Unwrap() error {
	return e.err
	}
	func (e InvalidInputError) Error() string {
	return e.err.Error()
	}
	// IsInvalidInputError returns whether the given error is an InvalidInputError error
	func IsInvalidInputError(err error) bool {
	var errInvalidInputError InvalidInputError
	return errors.As(err, &errInvalidInputError)
	}

I quite liked Max's pattern and therefore I suggested this approach (comment above)

• it allows to wrap lower-level errors as InvalidExtensionError and have the higher-level business logic only check for InvalidExtensionError
• I find this fairly convenient, as it allows us to retain the lower-level error trace (with all the auxiliary information), which would not be the case, if we just retained the message.

[zhangchiqing]

A good practice is to wrap the error with more context, so that the error message could include the stack trace.

[durkmurder]

@AlexHentschel @jordanschalm @zhangchiqing I've added tests and made changes that were mentioned in comments. Waiting for final review.