Update EIP-4788: initial stab at v2 #7456

lightclient · 2023-08-04T17:46:38Z

Based on the conversation in ACD #167, I wanted to take a look at rewriting this EIP to use a regular contract instead of a precompile. The main motivating factor for this is to reduce the surface of consensus failures. Instead of N implementations of the precompile, we can rely on the same EVM logic as we do for any other contract execution and execute a standardized bytecode implementation of the beacon roots contract.

The changes are as follows:

require timestamp input to be exactly 32 bytes
revert if timestamp input does not match stored value (instead of returning zeroed word)
remove precompile concept, use regular smart contract with provided bytecode

The main open question here is where and how to deploy the code for the beacon roots contract. As I see it, there are three options:

deploy as a normal contract
predeploy the code via enshirement during the hard fork to the same address as the original precompile (0x0b)
predeploy the code to an arbitrary address in the address space (maybe a create/create2 calc'd address?)

I prefer option 1) as it aligns with the original motivation of the v2 change, which is to minimize the surface of consensus failures and reuse EVM infrastructure where possible. The addresses in the EIP were generated using this script.

--

I have also taken a stab at writing an assembly version of the beacon roots contract. It can be found here: https://github.com/lightclient/4788asm

I think that is roughly the minimal bytecode representation of the logic we wish to enshrine.

eth-bot · 2023-08-04T17:47:03Z

✅ All reviewers have approved.

djrtwo

nice!

djrtwo · 2023-08-04T17:56:44Z

EIPS/eip-4788.md

+  "input": "0x605a8060095f395ff33373fffffffffffffffffffffffffffffffffffffffe14604257602036146024575f5ffd5b620180005f350680545f351415603d576201800001545f525b60205ff35b42620180004206555f3562018000420662018000015500",
+  "accessList": [],
+  "v": "0x0",
+  "r": "0x539",


I don't love this method with a fake signature and stuff...

Can we not just drop bytecode X at address Y at the fork?
e.g. like 1011 proposed -- https://eips.ethereum.org/EIPS/eip-1011#deploying-casper-contract

I think it's more straight forward to just place it exactly where we want it, rather than trying to use a TX of sorts but without gas or signature verification which requires a lot more exceptional logic

Although this style of contract creation is not tied to any specific initcode like create2 is, the synthetic address is cryptographically bound to the input data of the transaction (e.g. the initcode).

and then we don't have to think about things like this.

rather than trying to use a TX of sorts but without gas or signature verification which requires a lot more exceptional logic

To be clear, this tx does still abide by gas and signature verification. The signature is constructed arbitrarily and therefore the sender is a one-time sender address (pk is not known).

--

In an effort to minimize protocol baggage, I think it is better to deploy the contract using standard methods. Once we are satisfied with the bytecode we'd like to deploy, it could be deployed by anyone. We simply have to agree on the address at which it is deployed. It doesn't have to be a synthetic tx, although I do slightly prefer this method as it is permissionless (anyone can fund the account and submit the tx).

djrtwo · 2023-08-04T18:00:12Z

EIPS/eip-4788.md


-#### Block processing
+The beacon roots contract has two operations: `get` and `set`. The input itself is not used to determine which function to execute, for that the result of `caller` is used. If `caller` is equal to `SYSTEM_ADDRESS` then the operation to perform is `set`. Otherwise, `get`.


oh, funny. seems okay though

This somewhat seems to imply that this contract has an ABI (which it has not, at least not in current assembly). I would really like to do the ABI-like approach as in lightclient/4788asm#5 since this will also be very helpful for solidity users (you can now BeaconRootContract(address).get(timest).

I think it is a bad idea to enshrine solidity ABI behavior. It is ubiquitous today, but in the future I expect other language will have different calling conventions. Solidity should directly support this contract like they do for ecrecover.

EIPS/eip-4788.md

djrtwo · 2023-08-04T18:03:24Z

EIPS/eip-4788.md

-The 32 bytes of the `parent_beacon_block_root` (as provided) are the value to write behind the `root_index`.
+* If `caller` is not equal to `SYSTEM_ADDRESS`, the contract must revert.
+* Caller provides the parent beacon block root as calldata to the contract.
+* Set the storage value at `header.timestamp % HISTORICAL_ROOTS_MODULUS` to be `header.timestamp`


I kind of want to make the case for avoiding a system-TX and just setting these storage values at the top of the block. Then the code is just get and we don't have to test/specify how to do system-TXs

weaker held opinion than the deploy method

I think if we're going to use a standard contract we should embrace it and allow it to have storing functionality.

Wrote this already in discord, posting here too for visibility.

With the spec being 'system call', it means a reference client can just do a simple call. A client implementor can still choose not to, and instead bypass the whole call and update the values directly.

If the eip standardizes 'direct update', however, and omits the system-update path from the contract, then we remove all optionality, and force more special-case code into clients

Therefore I think the 'system call' approach is the best way forward.

I think the EIP should mention that "direct update" is a valid/possible client implementation/optimization detail.

system-tx would be a good generic functionality to have in long term imo

djrtwo · 2023-08-04T18:04:22Z

EIPS/eip-4788.md

+  "maxPriorityFeePerGas": "0x9c7652400",
+  "maxFeePerGas": "0xe8d4a51000",
+  "value": "0x0",
+  "input": "0x605a8060095f395ff33373fffffffffffffffffffffffffffffffffffffffe14604257602036146024575f5ffd5b620180005f350680545f351415603d576201800001545f525b60205ff35b42620180004206555f3562018000420662018000015500",


I might put many of these values behind a VAR -- e.g. input and the stubbed r, s

EIPS/eip-4788.md

djrtwo · 2023-08-04T18:07:38Z

EIPS/eip-4788.md

+* the call must execute to completion, therefore the available gas can be considered as infinite
+* the call does not count against the block's gas limit
+* the call does not follow the [EIP-1559](./eip-1559.md) burn semantics - no value should be transferred as part of the call
+* if no code exists at `BEACON_ROOTS_ADDRESS`, the call must fail silently


Assuming the prior section specifies when to deploy the bytecode, this bullet will become unnecessary

As mentioned in other thread, I don't think it is important to specify the deployment. But I suppose the bullet is unnecessary because that is the semantics of an evm call anyways.

This bullet is unnecessary, other than as clarification: any non-value call to account with non-existing code has no discernible effect on state.
If you prefix the bullet with Note: or Clarification:, it becomes apparent that this bullet does not intend to add any behavioural changes.

@holiman But shouldn't it then rather say:

if no code exists at BEACON_ROOTS_ADDRESS, the call must succeed as with any other account without code

I find "fail" a bit misleading here.

I agree: fail or succeed depends on your point of view. Whole bullet is moot and should not exist

EIPS/eip-4788.md

ralexstokes · 2023-08-05T16:52:49Z

EIPS/eip-4788.md

+  "maxPriorityFeePerGas": "0x9c7652400",
+  "maxFeePerGas": "0xe8d4a51000",


where do these gas values come from?

to echo what others have said, it seems fragile to enshrine a "synthetic transaction"

easier to just drop byte code a la EIP-1011

Just arbitrary values that would be included on mainnet. It isn't really enshrining the tx. I would think more of it as two steps: 1) deploy contract 2) set address in clients to call as part of pre block processing. A synthetic tx is just one way of deploying the contract. I think it is best because it is clear, transparent, and permissionless. We could just as easily have someone hand deploy the contract before the fork (now even, if the byte code is ready) and then set the address in client configurations.

Co-authored-by: Alex Stokes <r.alex.stokes@gmail.com>

holiman · 2023-08-07T08:00:38Z

I also think that the 'get'-path should have an abi signature: getBeaconRoot(uint256), thus takign 36 bytes input.
It can either check the abi, or if we for some reason want to, we can omit the signature-check and ignore the first four bytes. But I don't see any reason to be overly "clever".

The ABI encoding is ubiquitous, not merely solidity, it's the calling-convention used in the ethereum application layer, and I think it makes sense to have the precompile use that callpattern.

With abi, even with zero compiler support, this is possible:

root = BeaconRoot( address ).getBeaconRoot( tstamp );

As opposed to something like this:

(bool ret, bytes memory data) = address.call(bytes.concat(tstamp));

(I'm happy to update that example, I'm not really sure of the syntax for doing a call that way. )

holiman · 2023-08-08T08:29:51Z

In the original EIP, the contract was a precompile, and this 'subsidized' such that for call-purposes, the address is considered already included in the transaction access list. The difference is, as far as I can tell, 700 instead of 2600 gas.

I don't really care which way we go, but the "least changes to the original eip" route suggests that the hardfork should add the address to the access list.

EIPS/eip-4788.md

Co-authored-by: Martin Holst Swende <martin@swende.se>

yperbasis · 2023-08-09T11:09:24Z

In the original EIP, the contract was a precompile, and this 'subsidized' such that for call-purposes, the address is considered already included in the transaction access list. The difference is, as far as I can tell, 700 instead of 2600 gas.

I don't really care which way we go, but the "least changes to the original eip" route suggests that the hardfork should add the address to the access list.

Yes, adding the address to the access list (from Cancun) makes sense, similar to how EIP-3651 did it for coinbase in Shanghai.

lightclient · 2023-08-10T15:44:17Z

@yperbasis there was some discussion on discord about warming the address. After your comment I added it to this PR, but I now realize the comparison with EIP-3651 isn't quite correct because the EIP-4788 contract is only touched once before any transactions execute.

me from discord:

so we treat it more like a precompile - but naively adding it to the list of precompiled addresses causes issues with vm calls because we usually check if the call destination is a precompile and intercept it before creating an actual EVM frame (which we do want in the case of 4788 contract)
so we'd have to add the 4788 address in some different way, check if cancun, etc

Anyways, to minimize complexity and special handling I would prefer to not add this to the warm list.

pinges · 2023-08-11T05:13:18Z

EIPS/eip-4788.md


-The precompile costs `G_beacon_root` gas to reflect the two (2) implicit `SLOAD`s from the precompile's state.
+Client may decide to omit an explicit EVM call and directly set the storage values.


I think that we have to decide whether we want to do the EVM call or to directly set the storage values. I understand that when we set the value directly and the the account is empty we do remove the account at the end of the block, but there are the edge case where someone puts some eth into the account, which means it is not empty anymore.
I would prefer to set the values directly, as that is 10x faster and we do not require all clients to implement the system operation.

I think that we have to decide whether we want to do the EVM call

The consensus-definition is that an EVM call is made. A client implementor may choose to optimize this, and do a direct update instead. This choice may cause incompatibilities in non-mainnet scenarios, e.g. if the code is replaced with different code.

I understand that when we set the value directly and the the account is empty we do remove the account at the end of the block, but there are the edge case where someone puts some eth into the account, which means it is not empty anymore.

No, there is now code on the account, so it would not be removed, since it is not empty, according to the definition of empty.

I would prefer to set the values directly, as that is 10x faster and we do not require all clients to implement the system operation.

You are at liberty to do so, since "we do not require all clients to implement the system operation" -- however, the consensus-correct behaviour, in any given scenario, is determined by how the system-call would execute

IMO this is a pretty good middle ground.

EIPS/eip-4788.md

jaydub74 · 2023-08-21T09:36:05Z

James Carroll

…

On Fri, Aug 4, 2023, 2:10 PM lightclient ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In EIPS/eip-4788.md <#7456 (comment)>: > -If the `timestamp` **does** match, the client **MUST** read the root from the contract storage and return those 32 bytes in the caller's return buffer. +#### Deployment + +The beacon roots contract is deployed like any other smart contract. A special synthetic address is generated +by working backwards from the desired deployment transaction: Submission of the deployment tx is permissionless and can occur at anytime before or after fork. — Reply to this email directly, view it on GitHub <#7456 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AXGFGVGXVHC7K7VBOBTAJNLXTVCKTANCNFSM6AAAAAA3EQHRDI> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

Co-authored-by: Martin Holst Swende <martin@swende.se>

EIPS/eip-4788.md

holiman · 2023-08-21T11:50:31Z

BEACON_ROOTS_ADDRESS should be 0xBeAC00541d49391ED88ABF392bfC1F4dEa8c4143 now AFAIK

@yperbasis nope, should be 0xbEac00dDB15f3B6d645C48263dC93862413A222D. Sorry for the mixups

holiman

LGTM

holiman · 2023-08-21T13:26:40Z

EIPS/eip-4788.md


-timestamp_reduced = to_uint64_be(timestamp) % HISTORICAL_ROOTS_MODULUS
-timestamp_index = to_uint256_be(timestamp_reduced)
+* the call must execute to completion, therefore the available gas can be considered as infinite


Why?
I would rather specify some hard limit, like 30M gas, as suggested by @yperbasis . I suspect most actual implementation will need to set a limit anyway.

#8055) [devnet-8](https://notes.ethereum.org/@ethpandaops/dencun-devnet-8) doesn't include ethereum/EIPs#7431 since it's rendered obsolete by upcoming ethereum/EIPs#7456 (to be implemented by PR #8038). This reverts PR #7952.

holiman · 2023-08-24T12:40:08Z

Why no merge? I thought this was mostly agreed upon on by ACD?

yperbasis · 2023-08-24T12:43:15Z

Please change the gas limit to 30M. Geth, Erigon, and probably Nethermind have implementations with the gas limit of 30M.

holiman · 2023-08-24T13:37:59Z

I agree with @yperbasis . To clarify what I said earlier:

I suspect most actual implementation will need to set a limit anyway.

Doing metered calls is the natural way that clients implement calls. Doing an "infinite-gas" call, or "unmetered call" is not a common thing, and it wouldn't surprise me is there are clients which do not even support unmetered calls. So some limit needs to be set, in those cases, and in many cases maxuint64 is the closes to infinite gas there is. So let's just set it to something and avoid the confusion.

See ethereum/EIPs#7456 & ethereum/go-ethereum#27849. Also set the gas limit for system calls to 30M (previously 2^64-1), which is in line with the [Gnosis spec](https://github.com/gnosischain/specs/blob/master/execution/withdrawals.md#specification), but should be doubled checked for Gnosis Chain.

ralexstokes · 2023-08-24T17:04:53Z

Why no merge? I thought this was mostly agreed upon on by ACD?

I see two outstanding changes we want to make:

Set gas limit of the "system execution" to 30M
change HISTORICAL_ROOTS_MODULUS to HISTORY_BUFFER_LENGTH (following a review w/ @djrtwo )

ralexstokes · 2023-08-24T17:43:34Z

here's a PR to address the gas limit:

Set gas limit of the "system execution" to 30M

lightclient#4

and for the name change:

lightclient#5

add explicit gas limit to EIP-4788 system write

change name of length of the EIP-4788 ring buffer

ralexstokes

lots of work has gone into this update, thanks everyone for taking a look and contributing!

going to go ahead and merge, if we need further tweaks we can update in future PRs

eth-bot

All Reviewers Have Approved; Performing Automatic Merge...

yperbasis · 2023-08-25T12:50:02Z

@lightclient Section "Gas cost of precompile" is obsolete now and should be removed. Also, "precompile" is still mentioned in a couple of other places in the Rationale.

#8055) [devnet-8](https://notes.ethereum.org/@ethpandaops/dencun-devnet-8) doesn't include ethereum/EIPs#7431 since it's rendered obsolete by upcoming ethereum/EIPs#7456 (to be implemented by PR #8038). This reverts PR #7952.

See ethereum/EIPs#7456 & ethereum/go-ethereum#27849. Also set the gas limit for system calls to 30M (previously 2^64-1), which is in line with the [Gnosis spec](https://github.com/gnosischain/specs/blob/master/execution/withdrawals.md#specification), but should be doubled checked for Gnosis Chain.

4788: initial stab at v2

5cb47b4

lightclient requested a review from eth-bot as a code owner August 4, 2023 17:46

github-actions bot added c-update Modifies an existing proposal s-draft This EIP is a Draft t-core labels Aug 4, 2023

eth-bot changed the title ~~4788: initial stab at v2~~ Update EIP-4788: initial stab at v2 Aug 4, 2023

eth-bot added the a-review Waiting on author to review label Aug 4, 2023

4788: fix markdown lints

68acfa5

djrtwo reviewed Aug 4, 2023

View reviewed changes

jochem-brouwer reviewed Aug 4, 2023

View reviewed changes

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

lightclient added 2 commits August 4, 2023 13:07

4788: rm superfluous condition on get/set

2eee682

4788: add martin to coauthor

9f5179e

ralexstokes reviewed Aug 5, 2023

View reviewed changes

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

ralexstokes reviewed Aug 5, 2023

View reviewed changes

4788: fix typo

c4caa4a

Co-authored-by: Alex Stokes <r.alex.stokes@gmail.com>

timbeiko mentioned this pull request Aug 7, 2023

Dencun Interop Testing Call #28 ethereum/pm#839

Closed

marioevz mentioned this pull request Aug 8, 2023

EIP 4788 Devnet 8 Testing Changes ethereum/hive#818

Closed

LukaszRozmej reviewed Aug 8, 2023

View reviewed changes

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

4788: ymmv on syscall optimization

9ae7e6f

Co-authored-by: Martin Holst Swende <martin@swende.se>

marioevz mentioned this pull request Aug 8, 2023

simulators/ethereum/engine: Pre-deploy EIP-4788 ethereum/hive#820

Merged

lightclient added 2 commits August 9, 2023 08:04

4788: update code and deploy addr

45b5803

4788: add address to 2929 warm list

72edbee

lightclient force-pushed the 4788-v2 branch from f0f6365 to 72edbee Compare August 9, 2023 14:13

pinges suggested changes Aug 11, 2023

View reviewed changes

holiman reviewed Aug 11, 2023

View reviewed changes

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

holiman reviewed Aug 21, 2023

View reviewed changes

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

4788: fix sender addr

998943a

Co-authored-by: Martin Holst Swende <martin@swende.se>

holiman reviewed Aug 21, 2023

View reviewed changes

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

Update EIPS/eip-4788.md

a5a4047

holiman previously approved these changes Aug 21, 2023

View reviewed changes

holiman reviewed Aug 21, 2023

View reviewed changes

yperbasis mentioned this pull request Aug 23, 2023

Revert "EIP-4788: set nonce of beacon root history address to nonzero… ledgerwatch/erigon#8055

Merged

ralexstokes and others added 3 commits August 24, 2023 11:44

add explicit gas limit to EIP-4788 system write

3d048ad

change name of length of the EIP-4788 ring buffer

bb28b8d

Merge pull request #4 from ralexstokes/4788-explicit-gas-limit

29cfc56

add explicit gas limit to EIP-4788 system write

lightclient dismissed holiman’s stale review via 29cfc56 August 24, 2023 20:46

Merge pull request #5 from ralexstokes/4788-change-modulus-name

240f522

change name of length of the EIP-4788 ring buffer

ralexstokes approved these changes Aug 24, 2023

View reviewed changes

eth-bot enabled auto-merge (squash) August 24, 2023 21:07

eth-bot approved these changes Aug 24, 2023

View reviewed changes

eth-bot merged commit 46f8d5b into ethereum:master Aug 24, 2023
10 checks passed

ralexstokes mentioned this pull request Aug 29, 2023

Update EIP-4788: 4788 cleanups #7532

Merged

tersec mentioned this pull request Sep 1, 2023

Dencun devnet-9 status-im/nimbus-eth2#5383

Closed


		#### Block processing
		The beacon roots contract has two operations: `get` and `set`. The input itself is not used to determine which function to execute, for that the result of `caller` is used. If `caller` is equal to `SYSTEM_ADDRESS` then the operation to perform is `set`. Otherwise, `get`.

		"maxPriorityFeePerGas": "0x9c7652400",
		"maxFeePerGas": "0xe8d4a51000",


		The precompile costs `G_beacon_root` gas to reflect the two (2) implicit `SLOAD`s from the precompile's state.
		Client may decide to omit an explicit EVM call and directly set the storage values.

Update EIP-4788: initial stab at v2 #7456

Update EIP-4788: initial stab at v2 #7456

Conversation

lightclient commented Aug 4, 2023 • edited

eth-bot commented Aug 4, 2023 • edited

djrtwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g11tech Aug 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman commented Aug 7, 2023 • edited

holiman commented Aug 8, 2023

yperbasis commented Aug 9, 2023 • edited

lightclient commented Aug 10, 2023

Choose a reason for hiding this comment

holiman Aug 11, 2023 • edited

Choose a reason for hiding this comment

jaydub74 commented Aug 21, 2023 via email

holiman commented Aug 21, 2023

holiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman commented Aug 24, 2023

yperbasis commented Aug 24, 2023

holiman commented Aug 24, 2023

ralexstokes commented Aug 24, 2023

ralexstokes commented Aug 24, 2023 • edited

ralexstokes left a comment

Choose a reason for hiding this comment

eth-bot left a comment

Choose a reason for hiding this comment

yperbasis commented Aug 25, 2023

lightclient commented Aug 4, 2023 •

edited

eth-bot commented Aug 4, 2023 •

edited

g11tech Aug 7, 2023 •

edited

holiman commented Aug 7, 2023 •

edited

yperbasis commented Aug 9, 2023 •

edited

holiman Aug 11, 2023 •

edited

ralexstokes commented Aug 24, 2023 •

edited