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

Add EIP: EOF/EVM Trace Specification #8799

Merged
merged 10 commits into from
Aug 22, 2024
Merged

Conversation

shemnon
Copy link
Contributor

@shemnon shemnon commented Aug 14, 2024

Expand EIP-3155 to include features for EOF.

Expand EIP-3155 to include features for EOF.
@shemnon shemnon requested a review from eth-bot as a code owner August 14, 2024 04:21
@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-interface labels Aug 14, 2024
@eth-bot
Copy link
Collaborator

eth-bot commented Aug 14, 2024

✅ All reviewers have approved.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Aug 14, 2024
@eth-bot eth-bot added e-consensus Waiting on editor consensus e-review Waiting on editor to review labels Aug 14, 2024
@eth-bot eth-bot changed the title Create EIP: EOF/EVM Trace Specification Add EIP: EOF/EVM Trace Specification Aug 14, 2024
EIPS/eip-eof-tracing.md Outdated Show resolved Hide resolved
EIPS/eip-eof-tracing.md Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Aug 15, 2024
Copy link

The commit 8d96f07 (as a parent of 93e6bb9) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added w-ci Waiting on CI to pass and removed w-ci Waiting on CI to pass labels Aug 15, 2024
Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

Great spec, left some comments!

EIPS/eip-7756.md Outdated Show resolved Hide resolved
EIPS/eip-7756.md Outdated Show resolved Hide resolved
EIPS/eip-7756.md Outdated Show resolved Hide resolved
EIPS/eip-7756.md Outdated Show resolved Hide resolved
EIPS/eip-7756.md Outdated Show resolved Hide resolved
EIPS/eip-7756.md Outdated Show resolved Hide resolved
EIPS/eip-7756.md Show resolved Hide resolved
Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

2 comments, all other comments from my last review are resolved 😄

EIPS/eip-7756.md Outdated Show resolved Hide resolved
@@ -154,7 +155,7 @@ This is the trace output from the Ethereum Execution Specification Test from one
{"pc":13,"op":90,"gas":"0x2fa9e66","gasCost":"0x2","memSize":0,"stack":["0x0","0x0","0x0","0x0","0x0","0x1000"],"depth":1,"refund":0,"opName":"GAS"}
{"pc":14,"op":241,"gas":"0x2fa9e64","gasCost":"0x2eeb414","memSize":0,"stack":["0x0","0x0","0x0","0x0","0x0","0x1000","0x2fa9e64"],"depth":1,"refund":0,"opName":"CALL"}
{"pc":0,"section":0,"op":227,"immediate":"0x0001","gas":"0x2eea9ec","gasCost":"0x5","memSize":0,"stack":[],"depth":2,"refund":0,"opName":"CALLF"}
Copy link
Member

Choose a reason for hiding this comment

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

Ah interesting, I just noticed: so if functionDepth is 0 it is omitted in this trace. I think functionDepth 0 should be added here? Also to the STOP operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

functionDepth is optional when zero. I'll add it to the spec. Generally empty/zero/zero length is an optional member. Attempting to reduce repetition.

EIPS/eip-7756.md Outdated Show resolved Hide resolved
EIPS/eip-7756.md Outdated Show resolved Hide resolved
EIPS/eip-7756.md Outdated Show resolved Hide resolved
- The client MUST NOT output multiple lines for the same operation execution.
- The client MUST NOT output a line for the `STOP` operation if an error occurred, or if the contract runs out of instructions.

Each object contains the following members.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these are JSON objects, would you consider using JSON Schema to describe the format? The schema could also be included as an asset instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an update of 3155, not a reboot. Using JSON Schema would not guarantee backwards compatability with existing clients, which is a top priority.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can define only the properties introduced in this proposal and leave the rest undefined. For example:

{
    "$schema": "https://json-schema.org/draft/2019-09/schema",
    "type": "object",
    "properties": {
      "section": {
        "type": "number"
      },
      "immediate": {
        "$ref": "#/$defs/hex-string"
      },
      "functionDepth": {
        "type": "number"
      }
    },
    "$defs": {
      "hex-string": {
        "type": "string",
        "pattern": "^0x[0-9A-Fa-f]*$",
      }
    }
}

EIPS/eip-7756.md Outdated Show resolved Hide resolved
EIPS/eip-7756.md Outdated

## Test Cases

This is the trace output from the Ethereum Execution Specification Test from one of the parameterized executions of the test `test_eof_functions_contract_call_succeed` in `prague/eip7692_eof_v1/eip3540_eof_v1/test_execution_function.py`. Memory and return data are disabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can link to tests directly as long as it matches these regexes:

EIPs/config/eipw.toml

Lines 151 to 152 in 46e7845

'^https://(www\.)?github\.com/ethereum/execution-spec-tests/(blob|tree)/[a-f0-9]{40}/.+$',
'^https://(www\.)?github\.com/ethereum/execution-spec-tests/commit/[a-f0-9]{40}$',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

@shemnon shemnon left a comment

Choose a reason for hiding this comment

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

Some of these comments (such as JSON schema) are technical in nature. should they be raised in the Ethereum-Magicians discussion thread?

EIPS/eip-7756.md Outdated Show resolved Hide resolved
EIPS/eip-7756.md Outdated Show resolved Hide resolved
- The client MUST NOT output multiple lines for the same operation execution.
- The client MUST NOT output a line for the `STOP` operation if an error occurred, or if the contract runs out of instructions.

Each object contains the following members.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an update of 3155, not a reboot. Using JSON Schema would not guarantee backwards compatability with existing clients, which is a top priority.

@eth-bot eth-bot enabled auto-merge (squash) August 22, 2024 15:45
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eth-bot eth-bot merged commit e21a739 into ethereum:master Aug 22, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal e-consensus Waiting on editor consensus e-review Waiting on editor to review s-draft This EIP is a Draft t-interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants