all: fix warning flagging the use of DeepEqual on error

[gballet]

gopls returns a warning that reflect.DeepEqual should not be used on error. This PR replaces it with a call to errors.Is introduced in Go 1.13.

Not all tests are replaced, only those for which Is(error) can be implemented. For example, some errors in the net package don't implement it.

[holiman]

Is this whole thing needed? I'm thinking we can replace it with

var ErrGenesisMismatch = errors.New("genesis mismatch")

And in the places where we now instantiate it, we simply do

return fmt.Errorf("%w: database has %x, new %x", ErrGenesisMismatch, have, new)

And then we can just do the generic errors.Is for it, instead of implementing custom Is support

[fjl]

For reasons explained previously, this method is not useful for any production use of the error.

[holiman]

Same here -- can we just simplify it?

[fjl]

@holiman ConfigCompatError cannot use the fmt.Errorf construction because the caller needs to know where to rewind:

go-ethereum/eth/backend.go

Line 198 in 5441a8f

eth.blockchain.SetHead(compat.RewindTo)

[fjl]

BTW, I think this PR is a bit weird. Using reflect.DeepEqual for errors is totally fine in tests, there is no downside to it. I think it complains about this because using DeepEqual for errors is discouraged in non-test code. This is because DeepEqual cannot find the error if it is wrapped in another one, but errors.Is can.

We need to think twice about the semantics of errors.Is here. The way it's implemented now, the error will only match if all fields are exactly the same. For example, in order to get a match with errors.Is(err, &params.ConfigCompatError{...}) you would need to know the exact error condition ahead of time. This situation is very unlikely to occur in production code, it will only ever occur in unit tests.

errors.Is is not meant for this kind of check, it is intended for matching against certain known constant errors such as os.ErrExist. For ConfigCompatError, it is more appropriate to use errors.As and then check the fields yourself:

var compatErr params.ConfigCompatError
if errors.As(err, &compatErr) {
    chain.SetHead(compatErr.RewindTo)
}

I'd argue that, since you will always need to deal with values of concrete type ConfigCompatError in order to use it properly, reflect.DeepEqual is appropriate for this type, and errors.Is cannot help.

[gballet]

errors.Is is not meant for this kind of check, it is intended for matching against certain known constant errors such as os.ErrExist. For ConfigCompatError, it is more appropriate to use errors.As and then check the fields yourself:

• While I agree that Go's errors.Is is weak compared to Rust's Result<T,E>/Option<T> or Zig's !/catch operators, limiting it to just "constant" error types is not doing it justice: its power comes from the %w directive, so if anything we should use that directive more;
• errors.As won't work in this case because you want to be able to compare the error with a generic wantErr, so you'd find yourself having to call reflect.DeepEqual anyway.

I tend to think along @holiman 's suggestion: use the %w directive more often. I can also restrict the changes to the bits that don't need an errors.Is implementation, and use %w as much as I can. Just let me know.

[fjl]

limiting it to just "constant" error types is not doing it justice: its power comes from the %w directive, so if anything we should use that directive more;

What I meant is: errors.Is works best if you are looking for an error that is constant, like in errors.Is(e, errSomething). errSomething may be wrapped in another error and will still be found.

For ConfigCompatError specifically, it is not possible to use errors.Is in the place where we look for it, which is here. But we could use errors.As there, it would work.

Also, it is not a good idea to replace this particular error with a constant + use of %w because ConfigCompatError also contains the block number where the mismatch is.

[fjl]

I can also restrict the changes to the bits that don't need an errors.Is implementation, and use %w as much as I can. Just let me know.

Most changes you made are fine. The change for ConfigCompatError needs to be reverted because errors.Is cannot be used with this error. I think we can live with one instance of reflect.DeepEqual in unit tests for this.

For GenesisMismatchError, we could technically use %w like Martin said. I personally think it is not worth the effort, and keeping existing API stable is preferred over changing it to satisfy the lint tool.

[holiman]

LGTM