Add tracing to t8n
#3953
Add tracing to t8n
#3953
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| @@ -84,7 +89,7 @@ export function getArguments() { | |||
| reward: BigInt((<any>args)['state.reward']), | |||
| chainid: BigInt((<any>args)['state.chainid']), | |||
| } | |||
|
|
|||
| args.trace = (<any>args)['trace'] | |||
There was a problem hiding this comment.
Note for myself: I should update these types, not sure why the types are not cast or set. Should do something similar as in the client where the CLI arguments are correctly typed (not this PR)
packages/vm/test/t8n/t8ntool.ts
Outdated
| } | ||
| trace.push(JSON.stringify(summary)) | ||
| writeFileSync( | ||
| `trace-${index}-${bytesToHex(event.transaction.hash())}.json`, |
There was a problem hiding this comment.
I think this should be written to the output dir args.output.basedir (maybe even in a dedicated trace folder, take a look what Geth does in this case if they have a specific trace folder or just dump all in the output dir). Also note that in the current setup it will depend on from which path the t8ntool is being called (will write to current path, not something relative to it).
packages/vm/test/t8n/t8ntool.ts
Outdated
| `trace-${index}-${bytesToHex(event.transaction.hash())}.json`, | ||
| `[${trace.join(',\n')}]`, | ||
| ) | ||
| trace = [] |
There was a problem hiding this comment.
This logic could be moved to this.afterTx
There was a problem hiding this comment.
I've debated about this. I kind of like all the trace logic being in one spot so it's easy to see what's going on. Could be persuaded otherwise though.
packages/vm/test/t8n/t8ntool.ts
Outdated
| @@ -218,10 +253,10 @@ export class TransitionTool { | |||
| if (args.log === true) { | |||
| this.vm.events.on('beforeTx', (_, resolve) => { | |||
| // eslint-disable-next-line no-console | |||
| console.log('Processing new transaction...') | |||
| if (args.log === true) console.log('Processing new transaction...') | |||
There was a problem hiding this comment.
This is wrapped inside an if (args.log === true) and I don't think the log flag can change, so I believe this is not necessary?
acolytec3
added
package: evm
type: test all hardforks
packages/evm/src/index.ts
Outdated
| @@ -58,7 +58,7 @@ export { | |||
| RustBN254, | |||
| validateEOF, | |||
| } | |||
|
|
|||
| export { JSONifyStepTrace } from './opcodes/util.ts' | |||
There was a problem hiding this comment.
Suggestion to rename this to StepTraceJSON. Why is this exported?
There was a problem hiding this comment.
We use it in t8n and that's in vm 🙃
There was a problem hiding this comment.
Yer its fine, sorry for some reason I thought it was a type and not a method, and AFAIK you dont have to explicitly export types in order to be picked up by the IDE
| npx tsx --conditions=typescript "$SCRIPT_DIR/launchT8N.ts" "$@" |
There was a problem hiding this comment.
Do we want to keep this flag?
There was a problem hiding this comment.
I like it since most of your edits will be in evm but this lives in the vm. Otherwise, you have to rebuild evm every time.
There was a problem hiding this comment.
Ah yes sorry my question should have gotten more context: this will thus only work on node versions where this flag is available. I'm not sure about this one, this means that people need to have node v22 installed (right?)
There was a problem hiding this comment.
No, the conditions flag was introduced in either 18 or 20
packages/evm/src/eof/container.ts
Outdated
| } | ||
|
|
||
| for (let i = 0; i < this.codeSizes.length; i++) { | ||
| if (codePosition < this.codeStartPos[i] + this.codeSizes[i]) { |
There was a problem hiding this comment.
This only works if the codeStartPos cache is initialized for this index. (See getCodePosition(section: number) {, this is called for EOF opcodes like CALLF and JUMPF).
There was a problem hiding this comment.
Can we do this here? Seems like it would make sense to do so.
There was a problem hiding this comment.
This would normally make a lot of sense, but we want to reduce the amount of code inside the EVM as much as possible. So, we do not want to initialize the entire code start position container each time we enter a new EVM frame (only want to calculate it when necessary). (This is the same as the JUMPDEST calculation, we only do this when necessary i.e. we hit a JUMP opcode)
packages/evm/src/interpreter.ts
Outdated
| * @property {number} section the current EOF code section referenced by the PC | ||
| * @property {Uint8Array} immediate the immediate argument of the opcode | ||
| * @property {Uint8Array} error the error data of the opcode (only present for REVERT) | ||
| * @property {number} functionDepth the depth of the function call (only present for EOF) |
There was a problem hiding this comment.
Storage is not added here yet
There was a problem hiding this comment.
Thinking about it, is this documentation necessary? Doesn't a dev immediately get the docs from the emitted event type?
There was a problem hiding this comment.
I wondered about that. I think we need to move all of this to the InterpreterStep interface (or just remove it)
packages/evm/src/opcodes/util.ts
Outdated
| @@ -271,3 +272,24 @@ export function updateSstoreGas( | |||
| return common.param('sstoreSetGas') | |||
| } | |||
| } | |||
|
|
|||
| export const getImmediate = (opcode: number, code: Uint8Array, pc: number) => { | |||
| let immediate: Uint8Array | |||
There was a problem hiding this comment.
Yes, for this, use the eof/stackdelta.ts and then the immediate value. The only exception is RJUMPV, which is thus dynamic. (Immediates depend upon the table length)
| * @param memory whether to include the memory in the trace | ||
| * @returns a JSON object that matches the EIP-7756 trace format | ||
| */ | ||
| export const stepTraceJSON = ( |
There was a problem hiding this comment.
Can't we directly JSON.stringify the output of the step event?
There was a problem hiding this comment.
Yes, was on the fence about whether to do that or not since it would return a string instead of a JSON object.
Also closes #3561
This adds JSON tracing to our implementation of
t8nin compliance with EIP-7756.To test
eesttest harness if you haven't already. Check out this commit 2a747431fb91c0be2ed959faad9c84ec9b8a3607uv run fill -k "eip4750_functions and test_callf_execution" tests/osaka --from=Osaka --until=Osaka --evm-bin=../ethjs/packages/vm/test/t8n/ethereumjs-t8ntool.sh --evm-dump-dir=./traces --traceswith this fork active from your local folder for theethereum-spec-tests.ethereum-spec-testsThese can be compared against geth by just replacing the path of
--evm-binwith a local geth instance.This PR addresses several issues along the way:
stepTraceJSON/summaryTraceJSONutility methods for converting the contents of theStepTraceandAfterTxobject emitted by theevm.eventsStepandvm.eventsAfterTxevent.ethereumjs/evm#3561 so we report the pre-expanded memory size in the Step Event.