eth, eth/tracers: process beacon root before transactions #29402

maoueh · 2024-03-29T16:57:13Z

The beacon root when applied in state_processor.go is performed right before executing transaction. That means that contract reliying on this value would query the same value found in the block header.

In that spirit, it means that any tracing/operation relying on state data which touches transaction must have updated the beacon root before any transaction processing. This PR brings this where I spotted where it was important through StateAtTransaction and those using with StateAtBlock and transactions.

For now I've put the beacon root update outside of StateAtBlock. I was reluctant to include it in StateAtBlock because technically the state at block doesn't include any state happening "in the block". But everywhere that StateAtBlock was used, it felt setting the beacon root was needed, so it would be posssible to change the semantic of the function and run any system pre-hooks.

Kinda relevant with PR #29372

…rect state The beacon root when applied in `state_processor.go` is performed right before executing transaction. That means that contract reliying on this value would query the same value found in the block header. In that spirit, it means that any tracing/operation relying on state data which touches transaction must have updated the beacon root before any transaction processing. This PR brings this where I spotted where it was important through `StateAtTransaction` and those using with `StateAtBlock` and transactions. For now I've put the beacon root update outside of `StateAtBlock`. I was reluctant to include it in `StateAtBlock` because technically the state at block doesn't include any state happening "in the block". But everywhere that `StateAtBlock` was used, it felt setting the beacon root was needed, so it would be posssible to change the semantic of the function and run any system pre-hooks. Kinda relevant with PR ethereum#29372

s1na · 2024-03-29T18:54:52Z

eth/tracers/api.go

@@ -916,6 +945,15 @@ func (api *API) TraceCall(ctx context.Context, args ethapi.TransactionArgs, bloc
 	}
 	defer release()

+	if config != nil && config.TxIndex != nil {


Do we still need this? you added it to StateAtTransaction

Yes but it's called conditionally above, that's why I've added it like this:

if config != nil && config.TxIndex != nil { _, _, statedb, release, err = api.backend.StateAtTransaction(ctx, block, int(*config.TxIndex), reexec) } else { statedb, release, err = api.backend.StateAtBlock(ctx, block, reexec, nil, true, false) } if err != nil { return nil, err } defer release()

Oh no I took the wrong condition, I need to run on the else clause but copied straight the if ... one, will invert the logic.

Updated, please take a look.

IMO we should not do this processing if txIdx is not provided. This is how I see it:

By default eth_call and debug_traceCall will simulate the call as if it's appended to the block. So they take the final state of existing block, and execute transaction on top. But because all parameters such as block number and timestamp are taken from the block itself, it is as if this call was the final transaction of that block. -> in this case we don't need to process beacon root. It has already been done.

When txIdx is provided, within the same block, we go backwards up to the index. Here because we must take block N-1 and apply all the transactions to get to index the system call processing is necessary. Still block context of execution is block N.

We should also allow users to simulate call in a potentially future block. They can already change the block context like number timestamp coinbase, etc. Now we need to add the beacon root field to BlockOverrides and if that's present do the processing too. Hmmm this creates a tricky scenario: what if I want to simulate N+3. Because the beacon root contract allows me to query all last 8192 beacon roots, I wonder how we should handle this scenario. It is sorta similar to BLOCKHASH. I believe there we default to 0x000...000 for hash of "phantom" blocks (e.g. N+2 in this case).

By default eth_call and debug_traceCall will simulate the call as if it's appended to the block. So they take the final state of existing block, and execute transaction on top. But because all parameters such as block number and timestamp are taken from the block itself, it is as if this call was the final transaction of that block. -> in this case we don't need to process beacon root. It has already been done.

I did not check how they retrieved their state actually, I thought the logic was to simply take state at beginning of N+1 since it's the same as the end of N. All in all, I think we agree that those shouldn't have to do any beacon root processing.

When txIdx is provided, within the same block, we go backwards up to the index. Here because we must take block N-1 and apply all the transactions to get to index the system call processing is necessary. Still block context of execution is block N.

If when txIdx != null the the logic is:

Take state at block N-1,

Re-apply transactions 0 -> txIdx-1

While the context is block N, technically if your re-apply all transactions before txIdx, then between 1. and 2. above, beacon root must be processed. Indeed, even the first transaction of the block should see the beacon root of block N.

Is that correct?

We should also allow users to simulate call in a potentially future block. They can already change the block context like number timestamp coinbase, etc. Now we need to add the beacon root field to BlockOverrides and if that's present do the processing too. Hmmm this creates a tricky scenario: what if I want to simulate N+3. Because the beacon root contract allows me to query all last 8192 beacon roots, I wonder how we should handle this scenario. It is sorta similar to BLOCKHASH. I believe there we default to 0x000...000 for hash of "phantom" blocks (e.g. N+2 in this case).

Of I wasn't aware it was possible to "simulate" future block. Indeed as soon as you start "to create" new blocks, the beacon root for unknown block doesn't exist. But I'm unsure about the code changes this comment should bring/remove.

Anything I need to change?

Sorry for the rambling. I wanted to have a thorough record of all the cases that could happen. As for this PR the only change I would like to see is regarding block simulations.

Please add beacon root as a field to internal/ethapi.BlockOverrides. It should be processed in eth_call and debug_traceCall optionally to manipulate the beacon root of a future block in the simulation.

I just realized technically it is possible to override the beacon root for a block through state overrides because the contract is part of the state. So let's skip that request for now.

s1na · 2024-04-16T09:30:48Z

eth/tracers/api.go

@@ -376,6 +376,14 @@ func (api *API) traceChain(start, end *types.Block, config *TraceConfig, closed
 				failed = err
 				break
 			}
+
+			if beaconRoot := block.BeaconRoot(); beaconRoot != nil {


block is the parent block here. We should use next.

holiman · 2024-04-16T10:04:50Z

eth/tracers/api.go

+	if beaconRoot := block.BeaconRoot(); beaconRoot != nil {
+		context := core.NewEVMBlockContext(block.Header(), api.chainContext(ctx), nil)
+		vmenv := vm.NewEVM(context, vm.TxContext{}, statedb, api.backend.ChainConfig(), vm.Config{})
+
+		core.ProcessBeaconBlockRoot(*block.BeaconRoot(), vmenv, statedb)
+	}
+


Please move this down, to right before transactions are iterated. Then you can also use the chainConfig which might contain overrides, rather than api.backend.ChainConfig(),. It might not make any difference for now, but it is the correct thing to do, IMO.

Yep good idea. Applied these changes locally. Will push when I get access.

s1na · 2024-04-16T10:27:01Z

Can you please enable push-access to the PR for maintainers? I'd like to push some changes.

maoueh · 2024-04-16T14:36:59Z

@s1na @holiman I've pushed @s1na commits back in my branch.

…9402) The beacon root when applied in `state_processor.go` is performed right before executing transaction. That means that contract reliying on this value would query the same value found in the block header. In that spirit, it means that any tracing/operation relying on state data which touches transaction must have updated the beacon root before any transaction processing.

maoueh requested review from s1na, karalabe, holiman and rjl493456442 as code owners March 29, 2024 16:57

s1na reviewed Mar 29, 2024

View reviewed changes

Inverted wrong logic to apply beacon root in TraceCall

c9c894c

s1na reviewed Apr 16, 2024

View reviewed changes

fix traceChain beacon root

43b91b3

holiman reviewed Apr 16, 2024

View reviewed changes

s1na added 2 commits April 16, 2024 12:25

minor

e1637b3

rm traceCall beacon root insertion

f7e87d1

move down beacn root processing

c3951c3

s1na added the status:triage label Apr 23, 2024

holiman changed the title ~~The beacon root must be processed before any transactions to have correct state~~ eth, eth/tracers: process beacon root before transactions Apr 24, 2024

holiman merged commit ade7515 into ethereum:master Apr 24, 2024
3 checks passed

holiman added this to the 1.14.0 milestone Apr 24, 2024

s1na mentioned this pull request May 17, 2024

Debug_traceBlockByHash shows output for tx hash as "Execution Reverted" falsely #29778

Closed

protolambda mentioned this pull request Aug 8, 2024

merge upstream geth 1.14.x (0-7) releases ethereum-optimism/op-geth#349

Merged

maoueh deleted the fix/wrong-state-at-transaction-due-to-beacon-root branch September 27, 2024 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eth, eth/tracers: process beacon root before transactions #29402

eth, eth/tracers: process beacon root before transactions #29402

maoueh commented Mar 29, 2024

s1na Mar 29, 2024

maoueh Mar 29, 2024

maoueh Mar 29, 2024

maoueh Mar 29, 2024

s1na Apr 1, 2024

maoueh Apr 9, 2024

s1na Apr 9, 2024

s1na Apr 15, 2024

s1na Apr 16, 2024

holiman Apr 16, 2024

s1na Apr 16, 2024

s1na commented Apr 16, 2024

maoueh commented Apr 16, 2024

eth, eth/tracers: process beacon root before transactions #29402

eth, eth/tracers: process beacon root before transactions #29402

Conversation

maoueh commented Mar 29, 2024

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

s1na commented Apr 16, 2024

maoueh commented Apr 16, 2024