internal/ethapi: support for beacon root and withdrawals in simulate api #31304

rezbera · 2025-03-01T20:59:47Z

Addresses #31264

rezbera · 2025-03-01T21:13:32Z

internal/ethapi/simulate.go

@@ -267,20 +276,22 @@ func (sim *simulator) processBlock(ctx context.Context, block *simBlock, header,
 		// EIP-7251
 		core.ProcessConsolidationQueue(&requests, evm)
 	}
-	header.Root = sim.state.IntermediateRoot(true)


Root calculation moved to the end, i.e. after withdrawals since they change state.

Update: Root Calculation is now done in FinalizeAndAssemble

rezbera · 2025-03-01T21:14:23Z

internal/ethapi/simulate.go

-	header.GasUsed = gasUsed
-	if sim.chainConfig.IsCancun(header.Number, header.Time) {
-		header.BlobGasUsed = &blobGasUsed
-	}


gas and blob setting moved up, i.e. closer to where gasUsed and blobGasUsed are last used

rezbera · 2025-03-01T21:39:04Z

internal/ethapi/simulate.go

+	if block.BlockOverrides.Withdrawals != nil {
+		// We assume that if the user provides Withdrawals, we're operating on Beacon consensus
+		// which can have nil ChainHeadReader
+		sim.b.Engine().Finalize(nil, header, sim.state, blockBody)


Is there a better way to do this? e.g. create a ChainHeadReader (seems non-trivial)

I like your idea of using Finalize here. I think we can take it a step further and use FinalizeAndAssemble. Would you be up for doing it? otherwise I will submit a follow-up PR.

I would not assume anything about the beacon engine tho. User can still provide withdrawal and request it to be based on a pre-shanghai block. This can crash the node. Please mock the chainHeaderReader. All of those methods are available on the backend so it should be straightforward.

Update simulate.go Update simulate.go Update simulate.go Update consensus.go passed withdrawals - now state root mismartch working but withdrawals issue Added support

rezbera · 2025-03-01T21:49:53Z

internal/ethapi/simulate.go

 	}
-	if requests != nil {


This was moved up, closer where requests is calculated

phearnot · 2025-03-03T10:53:31Z

Hey @rezbera, we also need withdrawal support in simulateV1 for our project. I've tweaked it like so. My idea was to handle withdrawals in the same way as they are handled in the "real" engine. State root hash is adjusted when address balance changes, so there's no need to manually change it.
I've run some smoke tests against my patch. I was able to pass the block with withdrawals generated by simulateV1 to newPayload/forkchoiceUpdated, and everything seemed to work fine.

rezbera · 2025-03-03T19:25:20Z

Hey @rezbera, we also need withdrawal support in simulateV1 for our project. I've tweaked it like so. My idea was to handle withdrawals in the same way as they are handled in the "real" engine. State root hash is adjusted when address balance changes, so there's no need to manually change it. I've run some smoke tests against my patch. I was able to pass the block with withdrawals generated by simulateV1 to newPayload/forkchoiceUpdated, and everything seemed to work fine.

Good call, that could work as well and avoids the need to deal with consensus engine but also not a fan of duplicating logic like that. Will see was maintainers think

rezbera · 2025-03-15T23:15:04Z

Anything I can do to support getting this through @s1na ?

s1na · 2025-03-17T18:33:57Z

Sorry for not getting to this earlier. If you update it based on the above comment it's good to go 💯

rezbera · 2025-03-18T20:45:04Z

internal/ethapi/simulate.go

-	if sim.chainConfig.IsCancun(header.Number, header.Time) {
-		header.BlobGasUsed = &blobGasUsed
-	}
-	var withdrawals types.Withdrawals


withdrawals are now managed in FinalizeAndAssemble

rezbera · 2025-03-18T20:53:30Z

internal/ethapi/simulate.go

+	return m.Backend.CurrentHeader()
+}
+
+func (m *simChainHeadReader) GetHeader(hash common.Hash, number uint64) *types.Header {


noting that there doesn't seem to be a clear standard as to whether to ignore hash or number, so going with hash instead.

HeaderByHash can also return non-canonical blocks. That's the reason for GetHeader existing. It asks for a block at a particular height with a assumed hash.

So we can improve this here by doing HeaderByNumber first and double-checking the hash.

I apppreciate the additional context. I've updated as suggested

rezbera · 2025-03-18T21:14:04Z

Sorry for not getting to this earlier. If you update it based on the above comment it's good to go 💯

Thanks for the review @s1na ! I've updated as requested - let me know if you'd like any further modifications

s1na · 2025-03-19T15:03:40Z

internal/ethapi/simulate.go

 	if requests != nil {
 		reqHash := types.CalcRequestsHash(requests)
 		header.RequestsHash = &reqHash
 	}
-	b := types.NewBlock(header, &types.Body{Transactions: txes, Withdrawals: withdrawals}, receipts, trie.NewStackTrie(nil))
+
+	blockBody := &types.Body{Transactions: txes, Withdrawals: *block.BlockOverrides.Withdrawals}


this can panic that's why tests are failing

Thank you - have addressed this by checking withdrawals in sanitizeChain

rezbera · 2025-03-19T21:09:58Z

internal/ethapi/simulate.go

@@ -346,6 +389,9 @@ func (sim *simulator) sanitizeChain(blocks []simBlock) ([]simBlock, error) {
 			n := new(big.Int).Add(prevNumber, big.NewInt(1))
 			block.BlockOverrides.Number = (*hexutil.Big)(n)
 		}
+		if block.BlockOverrides.Withdrawals == nil {
+			block.BlockOverrides.Withdrawals = &types.Withdrawals{}


Noting that we do an empty slice rather than nil, which semantically is fine as checks in consensus are done as length checks rather than nil checks.

if len(body.Withdrawals) > 0 { return nil, errors.New("clique does not support withdrawals") }

s1na

Looks good to me!

s1na · 2025-03-21T19:46:13Z

Thanks for the quick responses and bringing this so far. I was about to merge this before I realized two things:

We are not replacing the header's parentBeaconRoot value with that of the override (should prob do in MakeHeader)
Adding these fields to BlockOverride means that users can set them also for eth_call variants. I don't want to stretch this PR more by changing those methods too. But we should at least change BlockOverride.Apply to return an error in case BeaconRoot or Withdrawals are set to indicate lack of support

rezbera · 2025-03-22T22:49:03Z

Thanks for the quick responses and bringing this so far. I was about to merge this before I realized two things:

We are not replacing the header's parentBeaconRoot value with that of the override (should prob do in MakeHeader)

Adding these fields to BlockOverride means that users can set them also for eth_call variants. I don't want to stretch this PR more by changing those methods too. But we should at least change BlockOverride.Apply to return an error in case BeaconRoot or Withdrawals are set to indicate lack of support

Great catch. I've addressed this. Happy to fix any other concerns or nits

s1na

Thanks it's a good one

…api (ethereum#31304) Adds block override fields for beacon block root and withdrawals to the eth_simulateV1. Addresses ethereum#31264

rezbera requested review from fjl, s1na and lightclient as code owners March 1, 2025 20:59

rezbera commented Mar 1, 2025

View reviewed changes

rezbera marked this pull request as draft March 1, 2025 21:17

rezbera commented Mar 1, 2025

View reviewed changes

rezbera marked this pull request as ready for review March 1, 2025 21:39

Add support

2407255

Update simulate.go Update simulate.go Update simulate.go Update consensus.go passed withdrawals - now state root mismartch working but withdrawals issue Added support

rezbera force-pushed the enhance-simulation-api branch from fb1dd96 to 2407255 Compare March 1, 2025 21:40

rezbera added 2 commits March 2, 2025 08:45

Update simulate.go

d71a990

Update simulate.go

6a21744

rezbera commented Mar 1, 2025

View reviewed changes

internal/ethapi/simulate.go

}

if requests != nil {

Copy link

Contributor Author

rezbera Mar 1, 2025

Choose a reason for hiding this comment

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

This was moved up, closer where requests is calculated

rezbera mentioned this pull request Mar 1, 2025

feat(simulated): Add Simulation Client for EVM State Transitions (4/n) berachain/beacon-kit#2554

Merged

rezbera added 2 commits March 7, 2025 05:58

Merge branch 'master' into enhance-simulation-api

cde1056

Merge branch 'master' into enhance-simulation-api

fa245e2

rezbera added 3 commits March 18, 2025 06:28

Merge branch 'master' into enhance-simulation-api

170f75c

added mock sim

762ad05

Merge branch 'master' into enhance-simulation-api

ea7217b

rezbera commented Mar 18, 2025

View reviewed changes

s1na reviewed Mar 19, 2025

View reviewed changes

rezbera added 3 commits March 20, 2025 07:10

fix withdrawal nil pointer dereference

6d443ca

Add withdrawals to sanitizeChain

2209cb4

Fix GetHeader

26f20b2

rezbera added 2 commits March 20, 2025 08:00

Fixes to overrides

cb24543

minor lint

bddacef

rezbera commented Mar 19, 2025

View reviewed changes

rezbera added 2 commits March 20, 2025 08:30

Merge branch 'master' into enhance-simulation-api

e7b76da

Merge branch 'master' into enhance-simulation-api

c0f717f

s1na previously approved these changes Mar 21, 2025

View reviewed changes

rezbera added 4 commits March 23, 2025 08:08

Merge branch 'master' into enhance-simulation-api

795f3ec

Added overriden beacon root to header

2a99b55

Add error handling in Apply

9dae1ba

Added tests for unsupported

0ced839

rezbera dismissed s1na’s stale review via 0ced839 March 22, 2025 22:39

rezbera requested review from holiman and rjl493456442 as code owners March 22, 2025 22:39

enhance error message

5623a35

s1na approved these changes Mar 24, 2025

View reviewed changes

s1na merged commit 71e9c9b into ethereum:master Mar 24, 2025
3 of 4 checks passed

s1na added this to the 1.15.6 milestone Mar 24, 2025

fjl mentioned this pull request Mar 25, 2025

Add Support for BeaconRoot and Withdrawals as part of eth_simulateV1 API. #31264

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/ethapi: support for beacon root and withdrawals in simulate api #31304

internal/ethapi: support for beacon root and withdrawals in simulate api #31304

rezbera commented Mar 1, 2025 •

edited

Loading

rezbera Mar 1, 2025

rezbera Mar 18, 2025

rezbera Mar 1, 2025

rezbera Mar 1, 2025

s1na Mar 17, 2025

rezbera Mar 1, 2025

phearnot commented Mar 3, 2025

rezbera commented Mar 3, 2025

rezbera commented Mar 15, 2025

s1na commented Mar 17, 2025

rezbera Mar 18, 2025

rezbera Mar 18, 2025

s1na Mar 19, 2025

rezbera Mar 19, 2025

rezbera commented Mar 18, 2025

s1na Mar 19, 2025

rezbera Mar 19, 2025

rezbera Mar 19, 2025

s1na left a comment

s1na commented Mar 21, 2025

rezbera commented Mar 22, 2025

s1na left a comment

internal/ethapi: support for beacon root and withdrawals in simulate api #31304

internal/ethapi: support for beacon root and withdrawals in simulate api #31304

Conversation

rezbera commented Mar 1, 2025 • edited Loading

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

phearnot commented Mar 3, 2025

rezbera commented Mar 3, 2025

rezbera commented Mar 15, 2025

s1na commented Mar 17, 2025

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

rezbera commented Mar 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1na left a comment

Choose a reason for hiding this comment

s1na commented Mar 21, 2025

rezbera commented Mar 22, 2025

s1na left a comment

Choose a reason for hiding this comment

rezbera commented Mar 1, 2025 •

edited

Loading