Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: error management poc #1469

Closed
wants to merge 32 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
f58007f
vm: InvalidBlockHeader error
gabrocheleau Sep 10, 2021
c0500a4
vm: add Error Logger
gabrocheleau Sep 16, 2021
40c8df0
vm: remove makeError method
gabrocheleau Sep 16, 2021
0fcb6bf
Merge branch 'master' into vm/error-management-poc
gabrocheleau Sep 28, 2021
17831d2
vm: add makeError method to ErrorLogger
gabrocheleau Sep 29, 2021
db7cf88
vm: cleanup error logger makeError method
gabrocheleau Sep 29, 2021
4b6e9e4
vm: clean up error exports and test throwError method
gabrocheleau Sep 29, 2021
e9d758c
vm: simplify usage of throwError method
gabrocheleau Sep 29, 2021
b8aa92c
vm: fix linting errors
gabrocheleau Sep 29, 2021
3c8b8e1
vm: implement additional block header errors
gabrocheleau Sep 29, 2021
9eeafc2
vm: simplify throwError method further and fix missing ErrorCode
gabrocheleau Sep 29, 2021
3939a92
vm: reimplement commented out code
gabrocheleau Sep 29, 2021
b1f2a77
Merge branch 'master' into vm/error-management-poc
gabrocheleau Sep 30, 2021
f6f488d
vm: fix throwError type for dynamic inference in usage
gabrocheleau Sep 30, 2021
3f7a645
vm: add errorlogger tests
gabrocheleau Sep 30, 2021
91cf434
Merge branch 'master' into vm/error-management-poc
gabrocheleau Sep 30, 2021
a429d39
Merge branch 'master' into vm/error-management-poc
gabrocheleau Oct 13, 2021
4ace21e
chore: merge with master
gabrocheleau Feb 9, 2022
6285dd5
util: move errors.ts from vm to util package
gabrocheleau Feb 9, 2022
1c14b7d
vm: import error from util
gabrocheleau Feb 9, 2022
1b27ec7
util: more general Invalid Param ErrorCode
gabrocheleau Feb 10, 2022
cf127bb
util: cleanup makeError
gabrocheleau Feb 10, 2022
b1767f0
util: move error test from vm to util
gabrocheleau Feb 10, 2022
052687c
chore: remove unnecessary enum member
gabrocheleau Feb 10, 2022
0489ec8
Merge branch 'master' into vm/error-management-poc
gabrocheleau Feb 21, 2022
fd78bb9
feat: consistent stack trace and only export ErrorLogger
gabrocheleau Feb 21, 2022
e359bff
test: add stackTrace test and improve test descriptions
gabrocheleau Feb 21, 2022
30f8eec
fix: handle case where Error.captureStackTrace is not defined
gabrocheleau Feb 21, 2022
bfe30b9
Merge branch 'master' into vm/error-management-poc
gabrocheleau Feb 23, 2022
c083614
Merge branch 'master' into vm/error-management-poc
gabrocheleau Feb 28, 2022
f171fa3
Merge branch 'master' into vm/error-management-poc
gabrocheleau Mar 14, 2022
77c2170
Merge branch 'master' into vm/error-management-poc
gabrocheleau Mar 23, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 100 additions & 0 deletions packages/util/src/errors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
enum ErrorCode {
INVALID_PARAM = 'INVALID_PARAM',
UNKNOWN_ERROR = 'UNKNOWN_ERROR',
}

interface GeneralError<T extends ErrorCode = ErrorCode> extends Partial<Error> {
code?: T
}

interface InvalidParamError extends GeneralError<ErrorCode.INVALID_PARAM> {
param?: string
}

interface UnknownError extends GeneralError<ErrorCode.UNKNOWN_ERROR> {
[key: string]: any
}

// Convert an ErrorCode into its Typed Error
type CodedGeneralError<T> = T extends ErrorCode.INVALID_PARAM
? InvalidParamError
: T extends ErrorCode.UNKNOWN_ERROR
? UnknownError
: never

function isError<K extends ErrorCode, T extends CodedGeneralError<K>>(
error: GeneralError,
code: K
): error is T {
if (error && (<GeneralError>error).code === code) {
return true
}
return false
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still highly sceptical on all this TypeScript magic TBH. The code is very hard to read (and therefore to maintain), I am also fearing a bit some unforseable JavaScript usage side effects, not sure if this is valid or not.

I have a - somewhat - stronger tendency to radically simplify here and throw all this sub-interfacing with InvalidParamError, UnknownError,... away, remove as much advanced TypeScript specifics as possible and just rely with a basic CustomError interface/class/whatever and solely keep the code property for distinction and integrate the ability from unknown-error to pass in an arbitrary list of key-value pairs with error information.

What do others think? @ryanio, @g11tech am I too conservative here? 🤔 I still do not feel settled on this code, especially this is something we would apply so broadly within the code base.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like lodestar error approach, similar to the code approach: generic lodestar error defined here (allows one to specify how to serialize it for log because we had the issue of dumping circular objects): https://github.com/ChainSafe/lodestar/blob/master/packages/utils/src/errors.ts
then finally extended to be used e.g. https://github.com/ChainSafe/lodestar/tree/master/packages/lodestar/src/chain/errors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing Lodestar's approach, I quite like it and I feel like it would provide similar benefits to the approach used here but with simpler code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gabrocheleau ok, nice, I've some additional comments I might be able to do later the day or tomorrow, then we should be ready to rework this and get to some final state to be applied throughout the code base! 🙂

Copy link
Member

@holgerd77 holgerd77 May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When "tomorrow" gets to 16 days, lol 😬 😜.

Anyhow.

Yes, I think this Lodestar approach is going very much into a good direction, I am also feeling a lot more confident on something like this in favor to defining our own custom "throwSomething()" function, remaining on dealing with original "Errors" or sub classes seems to be a lot more robust and side effects free to me.

So I would roughly suggest that:

  1. We do the generic code - similar to errors.ts in a dedicated Utils package, as we already planned on the alternative layout

I think it then makes sense to define the respective errors like in the Lodestar errors folder on a "per package" basis on our side - e.g. for the block package - I don't see too much overlap on this between the packages. This simplifies things for structuring and drawing lines (and if we have some few redundancy here it also wouldn't hurt).

Additionally/eventually we can also define a few generic errors in Util as well, but rather as a second step if really necessary.

Just in between: here is a search for the Lodestar BlockErrorCode.PARENT_UNKNOWN error, just to get a glimpse on error usage.

If I get this right there are no "prose error strings" (like 'The parent hash of the uncle header is not part of the canonical chain') in Lodestar but just these capital AGGREGATOR_INDEX_TOO_HIGH = "ATTESTATION_ERROR_AGGREGATOR_INDEX_TOO_HIGH" aggregated error strings.

I would like to keep/stick to our prose approach, already for the reason that we have all this already written down. So I guess this would be one of the major deviations from the Lodestar approach.

WDYT? This is just a first round proposal - this would again need some draft implementation to verify - and is generally open for further discussion and evolution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree the more user friendly(readable) error strings are beneficial. 👍


function isInvalidParamError(error: GeneralError): error is InvalidParamError {
return isError(error, ErrorCode.INVALID_PARAM)
}

function isUnknownError(error: GeneralError): error is UnknownError {
return isError(error, ErrorCode.UNKNOWN_ERROR)
}

export class ErrorLogger {
static errors = ErrorCode

makeError<T>(codedError: CodedGeneralError<T>): Error {
const { code, message, ...params } = codedError
const messageDetails: Array<string> = []

if (isInvalidParamError(codedError) && typeof params.param !== 'undefined') {
messageDetails.push(`Invalid param=${codedError.param}`)
}

if (isUnknownError(codedError)) {
Object.keys(params).forEach((key) => {
const value = codedError[key]
try {
messageDetails.push(key + '=' + JSON.stringify(value))
} catch {
messageDetails.push(key + '=' + JSON.stringify(codedError[key].toString()))
}
})
}

messageDetails.push(`code=${code}`)

let errorMessage = message ?? ''

if (messageDetails.length) {
errorMessage += ' | Details: ' + messageDetails.join(', ')
}

const error = new Error(errorMessage) as CodedGeneralError<T>
error.code = codedError.code

Object.keys(params).forEach((key) => {
const typedKey = key as keyof typeof codedError
error[typedKey] = codedError[typedKey]
})

// captureStackTrace is not defined in some browsers, notably Firefox
if (Error.captureStackTrace) {
Error.captureStackTrace(error, this.throwError)
}

throw error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is misplaced here, the name of the method says something else, can you please move this to throwError?

}

throwError<T extends ErrorCode>(
code?: T,
params?: Omit<CodedGeneralError<T>, 'code' | 'stack'>
): void {
this.makeError({
code: code ?? ErrorCode.UNKNOWN_ERROR,
...params,
} as CodedGeneralError<T>)
}
}

export const errorLog = new ErrorLogger()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am generally not satisfied with the naming scheme we have here.

Can you do the following:

  1. Rename errors.ts (and related) to customError.ts (singular)
  2. Remove the class wrapping for ErrorLogger, have this as plain methods
  3. Export the ErrorCode enum as types
  4. Rename throwError to throw

Then we can use this with a single-item import as follows:

import { customError } from 'ethereumjs-util'

customError.throw(customError.types.INVALID_PARAM)

Not sure, we could also shorten and rename to just error, but I fear that this is too generic and might interfere in some unpredictable ways, it's also not so transparent regarding the use of our own custom error class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(then please also adopt the test file name)

5 changes: 5 additions & 0 deletions packages/util/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ export * from './account'
*/
export * from './address'

/**
* Errors
*/
export * from './errors'

/**
* Hash functions
*/
Expand Down
125 changes: 125 additions & 0 deletions packages/util/test/errors.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
import tape from 'tape'
import { ErrorLogger } from '../src/errors'

tape('ErrorLogger', (t) => {
const errorLog = new ErrorLogger()

t.test('should assign the UNKNOWN_ERROR code to errors without code', (st) => {
let error: any

try {
errorLog.throwError()
} catch (e) {
error = e
}

st.equal(error.code, ErrorLogger.errors.UNKNOWN_ERROR)
st.end()
}),
t.test('should preserve the stack trace of the throwError context', (st) => {
let error: any

try {
errorLog.throwError()
} catch (e) {
error = e
}

// captureStackTrace is not defined in some browsers, notably Firefox, so behavior can't be implemented/tested there
// @ts-ignore
if (Error.captureStackTrace) {
st.ok(/Error: {2}\| Details: code=UNKNOWN_ERROR\n {4}at Test./.test(error.stack))
}

st.end()
}),
t.test('should populate an error with UNKNOWN_ERROR code with all provided params', (st) => {
let error: any

try {
errorLog.throwError(ErrorLogger.errors.UNKNOWN_ERROR, {
errorInfo1: 'Information on the error',
errorInfo2: 'More information on the error',
})
} catch (e) {
error = e
}

st.equal(error.code, ErrorLogger.errors.UNKNOWN_ERROR)
st.equal(error.errorInfo1, 'Information on the error')
st.equal(error.errorInfo2, 'More information on the error')
st.end()
}),
t.test(
'should add all error params of UNKNOWN_ERROR to error mes21 02 2022 17:42:12.866:ERROR [launcher]: No binary for ChromeHeadless browser on your platform.sage details',
(st) => {
let error: any

try {
errorLog.throwError(ErrorLogger.errors.UNKNOWN_ERROR, {
errorInfo1: 'Information on the error',
errorInfo2: 'More information on the error',
})
} catch (e) {
error = e
}

st.equal(
error.message,
' | Details: errorInfo1="Information on the error", errorInfo2="More information on the error", code=UNKNOWN_ERROR'
)
st.end()
}
),
t.test('should append all error details to provided error message', (st) => {
let error: any

try {
errorLog.throwError(ErrorLogger.errors.UNKNOWN_ERROR, {
message: 'Error Message',
errorInfo1: 'Information on the error',
errorInfo2: 'More information on the error',
})
} catch (e) {
error = e
}

st.equal(
error.message,
'Error Message | Details: errorInfo1="Information on the error", errorInfo2="More information on the error", code=UNKNOWN_ERROR'
)
st.end()
})
t.test('should populate an error with INVALID_PARAM with the "param" prop', (st) => {
let error: any

try {
errorLog.throwError(ErrorLogger.errors.INVALID_PARAM, {
param: 'difficulty',
})
} catch (e) {
error = e
}

st.equal(error.param, 'difficulty')
st.end()
}),
t.test('should add the "param" prop to the INVALID_PARAM error message', (st) => {
let error: any

try {
errorLog.throwError(ErrorLogger.errors.INVALID_PARAM, {
message: 'Gas limit higher than maximum',
param: 'gasLimit',
})
} catch (e) {
error = e
}

st.equal(
error.message,
'Gas limit higher than maximum | Details: Invalid param=gasLimit, code=INVALID_PARAM'
)
st.end()
})
})
37 changes: 26 additions & 11 deletions packages/vm/src/runBlock.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { debug as createDebugLogger } from 'debug'
import { BaseTrie as Trie } from 'merkle-patricia-tree'
import { Account, Address, BN, intToBuffer, rlp } from 'ethereumjs-util'
import { Account, Address, BN, errorLog, ErrorLogger, intToBuffer, rlp } from 'ethereumjs-util'
import { Block } from '@ethereumjs/block'
import { ConsensusType } from '@ethereumjs/common'
import VM from './index'
Expand Down Expand Up @@ -209,8 +209,11 @@ export default async function runBlock(this: VM, opts: RunBlockOpts): Promise<Ru
)} expected=${block.header.receiptTrie.toString('hex')}`
)
}
const msg = _errorMsg('invalid receiptTrie', this, block)
throw new Error(msg)
const message = _errorMsg('invalid receiptTrie', this, block)
errorLog.throwError(ErrorLogger.errors.INVALID_PARAM, {
message,
param: 'receiptTrie',
})
}
if (!result.bloom.bitvector.equals(block.header.logsBloom)) {
if (this.DEBUG) {
Expand All @@ -220,15 +223,21 @@ export default async function runBlock(this: VM, opts: RunBlockOpts): Promise<Ru
)} expected=${block.header.logsBloom.toString('hex')}`
)
}
const msg = _errorMsg('invalid bloom', this, block)
throw new Error(msg)
const message = _errorMsg('invalid bloom', this, block)
errorLog.throwError(ErrorLogger.errors.INVALID_PARAM, {
message,
param: 'bloom',
})
}
if (!result.gasUsed.eq(block.header.gasUsed)) {
if (this.DEBUG) {
debug(`Invalid gasUsed received=${result.gasUsed} expected=${block.header.gasUsed}`)
}
const msg = _errorMsg('invalid gasUsed', this, block)
throw new Error(msg)
const message = _errorMsg('invalid gasUsed', this, block)
errorLog.throwError(ErrorLogger.errors.INVALID_PARAM, {
message,
param: 'gasUsed',
})
}
if (!stateRoot.equals(block.header.stateRoot)) {
if (this.DEBUG) {
Expand All @@ -238,8 +247,11 @@ export default async function runBlock(this: VM, opts: RunBlockOpts): Promise<Ru
)} expected=${block.header.stateRoot.toString('hex')}`
)
}
const msg = _errorMsg('invalid block stateRoot', this, block)
throw new Error(msg)
const message = _errorMsg('invalid block stateRoot', this, block)
errorLog.throwError(ErrorLogger.errors.INVALID_PARAM, {
message,
param: 'stateRoot',
})
}
}

Expand Down Expand Up @@ -285,8 +297,11 @@ async function applyBlock(this: VM, block: Block, opts: RunBlockOpts) {
// Validate block
if (!opts.skipBlockValidation) {
if (block.header.gasLimit.gte(new BN('8000000000000000', 16))) {
const msg = _errorMsg('Invalid block with gas limit greater than (2^63 - 1)', this, block)
throw new Error(msg)
const message = _errorMsg('Invalid block with gas limit greater than (2^63 - 1)', this, block)
errorLog.throwError(ErrorLogger.errors.INVALID_PARAM, {
message,
param: 'gasLimit',
})
} else {
if (this.DEBUG) {
debug(`Validate block`)
Expand Down
4 changes: 2 additions & 2 deletions packages/vm/tests/api/runBlock.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import tape from 'tape'
import { Address, BN, rlp, KECCAK256_RLP, Account } from 'ethereumjs-util'
import { Address, BN, rlp, KECCAK256_RLP, Account, ErrorLogger } from 'ethereumjs-util'
import Common, { Chain, Hardfork } from '@ethereumjs/common'
import { Block } from '@ethereumjs/block'
import {
Expand Down Expand Up @@ -212,7 +212,7 @@ tape('runBlock() -> API parameter usage/data errors', async (t) => {
await vm
.runBlock({ block })
.then(() => t.fail('should have returned error'))
.catch((e) => t.ok(e.message.includes('Invalid block')))
.catch((e) => t.ok(e.code === ErrorLogger.errors.INVALID_PARAM && e.param === 'gasLimit'))
})

t.test('should fail when block validation fails', async (t) => {
Expand Down