chore(all): enable errorlint and leave error wrapping rule disabled

[qdm12]

Hi there 👋

I understand large scale refactors are a big no-no, but hear me out 🙊 🙏

We, and probably others, use portions of your code. We do like adding context to errors using fmt.Errorf("some context: %w", err) to have meaningful and detailed-enough error messages.

Unfortunately, there are a lot of places in this codebase using:

1. direct equality checks for errors (i.e. err == ErrXYZ)
2. direct type assertions for errors (i.e. err.(*ErrXYZ))

and that prevents wrapping errors with context, since these would fail for a wrapped error.

On the other hand, changing 1. to use errors.Is and 2. to use errors.As solve this, and works the same as before as well.

Since you already use golangci-lint, the errlint built-in linter can be enabled to show all the errors regarding the previously mentioned points 1 and 2. The errlint error wrapping rule should be disabled since modifying existing errors to be wrapped is out of scope.

All error checks and type assertions are changed to use errors.Is and errors.As respectively, with the errlint linter (partially) enabled.

If this is still too much of a large refactor, I'm happy to invest more of my free time into:

• splitting this per package/directory
• splitting this in two PRs, one for errors.Is and one for errors.As

Thank you for your reviews!

EDIT: if you don't like this PR at all, please comment why 😉

[fjl]

We don't use errors.Is everywhere because it's not necessary. errors.Is is a weaker check than==, but in unit tests we want to use the strongest check available. If we know a function is supposed to return pkg.ErrSomething for some condition, we can test for it directly. There are a lot of changes to unit tests in this PR. I'd argue these are wrong because the tests are passing without this change.

Another case where comparing with == is more appropriate is when the error is used as a signaling mechanism, like in rlp/decode.go. This code is very performance-sensitive, and uses the error value rlp.EOL as a sentinel during parsing.

Certainly, if you can point to a particular piece of code where it would be more sensible to use errors.Is or errors.As, we can discuss changing that particular place. But an all-around conversion of every single case will not be accepted.

[qdm12]

Thanks @fjl for the feedback! 🙏

There are a lot of changes to unit tests in this PR. I'd argue these are wrong because the tests are passing without this change.

I understand this, and I very much dislike changing tests as well. However this PR was to pave the way to be able to add production code error wrapping (in forks of go-ethereum for example) and have go-ethereum tests continue passing. Keeping tests as they were mean they would fail as soon as an error would be wrapped in production code.

Another case where comparing with == is more appropriate is when the error is used as a signaling mechanism, like in rlp/decode.go.

Certainly, performance sensitive error equality checks should be preserved as == (10ns vs 2ns), although benchmarking to check if the impact is negligible or not would be good to still limit direct equality checks. I'm happy to do some digging into this. If we can keep those direct checks as little as possible it becomes manageable to wrap errors without too much checking.

Finally, to clarify this "too much checking", and to explain further the full reasoning behind this PR:

• there is no automated program (linter or other) to detect wrapped errors being checked directly (it might not even be fully possible to implement)
• when wrapping an error, checking every caller code paths for direct error checks is kind of humanly not possible

We did completely give up on wrapping errors with the fear of one error being equality checked or type asserted directly, causing a subtle change with big consequences.

So I have one question left, should I:

1. Abandon this PR and try to implement a linter to determine which error direct checks in go-ethereum should be changed - sounds great even to me, but I doubt it's feasible, especially when crossing Go modules boundaries.
2. Continue this branch and I can work my way to revert checks for performance critical code (rlp decoding reverted notably). Reverting tests has been done, and conversions for tests can be PR-ed later on an as-needed basis (at least they won't have bad production consequences). This would still be an "all-around" conversion with exceptions for performance critical paths and test files, would that be of interest eventually?

[fjl]

Can you please let me know which specific errors you would like to wrap in your fork of go-ethereum, so that we can assess where changes will be required? We do use errors.Is in some places where it matters, and this can be expanded, but we need to know what you want to achieve.

[qdm12]

Thanks, I will eventually suggest PRs for each single errors.Is we would need then, with an explanation. 👍