Skip to content

Conversation

@shemnon
Copy link
Contributor

@shemnon shemnon commented Apr 4, 2024

Update EOF opcodes so addresses are not trimmed during execution,
reverting with an exception if the address is invalid.

Update EOF opcodes so addresses are not trimmed during execution,
reverting with an exception if the address is invalid.
@shemnon shemnon requested a review from eth-bot as a code owner April 4, 2024 00:41
@github-actions github-actions bot added c-new Creates a brand new proposal c-update Modifies an existing proposal s-draft This EIP is a Draft t-core labels Apr 4, 2024
@eth-bot
Copy link
Collaborator

eth-bot commented Apr 4, 2024

✅ All reviewers have approved.

@eth-bot eth-bot added e-consensus Waiting on editor consensus e-review Waiting on editor to review labels Apr 4, 2024
@eth-bot eth-bot changed the title EOF - Prepare for Address Space Extension Add EIP: EOF - Prepare for Address Space Extension Apr 4, 2024
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Apr 4, 2024
EIPS/eip-ase.md Outdated

## Specification

The `BALANCE` operation, when invoked in code in an EOF container, will reauire the top 12 bytes of
Copy link
Contributor

Choose a reason for hiding this comment

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

An interesting side effect is that this provides a cheap (in terms of codesize, not gas) way of checking that there are no high bits set in a stack item.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cheap? It's going to get hit with a cold-account charge on success - https://eips.ethereum.org/EIPS/eip-2929 - and a revert on failure. I thing PUSH1 0x20 SHR ISZERO would be cheaper.

Copy link
Contributor

Choose a reason for hiding this comment

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

"cheap" - as in smaller bytecode. i don't think it is that practical unless you have really strong codesize constraints, i'm just pointing out that it's a new side effect.

@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Apr 9, 2024
EIPS/eip-7069.md Outdated
4. Peform (and charge for) memory expansion using `[input_offset, input_size]`.
5. If `target_address` is not in the `warm_account_list`, charge `COLD_ACCOUNT_ACCESS - WARM_STORAGE_READ_COST` (2500) gas.
6. If `target_address` is not in the state and the call configuration would result in account creation, charge `ACCOUNT_CREATION_COST` (25000) gas.
4. Halt with exception if any of the high 96 bits of the `target_address` are set.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an option to leave EIP-7069 intact, and put this step as an expansion of EXT*CALL logic into the new EIP-7676. The advantage is decoupling EIPs and not leaving this step without its due motivation described in EIP-7069 (or worse, having to reference circularly the EIP-7676).

But I'm not strongly against laying this out like is done now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the scattering of call across only two EIPs that concerns me. For vanilla validation it's already across multiple, and the rules focus on the operations in those EIPs. But splitting operational steps across EIPs I am concerned will result in implementations overlooking the detail.

@shemnon
Copy link
Contributor Author

shemnon commented Apr 15, 2024

@axic, @g11tech, @gcolvin, @lightclient, @SamWilsn is there anything keeping this from being merged?

@g11tech
Copy link
Contributor

g11tech commented Apr 16, 2024

@axic, @g11tech, @gcolvin, @lightclient, @SamWilsn is there anything keeping this from being merged?

could you separate out the update from new eip addition please

@github-actions github-actions bot removed the c-update Modifies an existing proposal label Apr 16, 2024
@shemnon
Copy link
Contributor Author

shemnon commented Apr 16, 2024

Will separating out parallel updates be a standard request of all new EIPs? If so, should that be baked into validation checks?

EIPS/eip-7676.md Outdated
Comment on lines 52 to 54
Any new operation with an address operand on the stack, or any operation that is un-banned from EOF
that has an address operand on the stack will have the same behavior as `BALANCE` when validating
the address operand.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Any new operation with an address operand on the stack, or any operation that is un-banned from EOF
that has an address operand on the stack will have the same behavior as `BALANCE` when validating
the address operand.

i think we shouldn't do an "auto" scope expansion, either this EIP needs to be edited (before going to final) or new EIPs need to come if any scope expansion needs to be done. for new operation, it should mention it follows this EIP

Still this could be a useful context/direction and could be worthwhile to put in the abstract.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to backwards compatability section and made it advisory.

@g11tech
Copy link
Contributor

g11tech commented Apr 18, 2024

Will separating out parallel updates be a standard request of all new EIPs? If so, should that be baked into validation checks?

yea I guess we should do that. added few nits else should be gtm

@shemnon
Copy link
Contributor Author

shemnon commented Apr 22, 2024

Nits addressed. Please merge.

@shemnon
Copy link
Contributor Author

shemnon commented Apr 24, 2024

@axic, @g11tech, @gcolvin, @lightclient, @SamWilsn is there anything keeping this from being merged?

g11tech
g11tech previously approved these changes Apr 24, 2024
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm

@g11tech
Copy link
Contributor

g11tech commented Apr 24, 2024

approved, not sure why bot isn't working

@github-actions
Copy link

The commit 1216936 (as a parent of dd54187) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Apr 24, 2024
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Apr 24, 2024
Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

Technically you shouldn't link to things in the ethereum/tests repository, but since @g11tech already approved, I'm not going to force the issue.

@eth-bot eth-bot enabled auto-merge (squash) April 26, 2024 16:48
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...

@shemnon
Copy link
Contributor Author

shemnon commented Apr 26, 2024

Not a link but a reference by name. Eth-bot shut that one down quick. How else would you propose acknowledge existing tests and reference implementations?

In this case calling out it's specific location seemed warranted because the organization of tests for that case is fairly non-intuitive, and expecting the readers to do a search themselves without signposts moves it in the direction of "trust me bro, tests exist."

@eth-bot eth-bot merged commit 6a0a792 into ethereum:master Apr 26, 2024
@SamWilsn
Copy link
Contributor

We still treat "references by name" as links. It's just easier to detect URLs than vague English 🤣

Interestingly we do allow links to EEST:

EIPs/config/eipw.toml

Lines 151 to 152 in 6a0a792

'^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}$',

Would it be useful to consider allowing links to ethereum/tests?

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-core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants