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 verkle support to evm t8n for test framework #432

Closed
wants to merge 29 commits into from

Conversation

gballet
Copy link
Owner

@gballet gballet commented May 6, 2024

Rebase of #406

TODO

  • rebase on kaustinen-with-shapella
  • change the key utility to take an alloc instead of a single key
  • offer both whole tree and single key conversions (i.e. partially revert previous change)
  • add mapping method -> no longer needed as the witness should cover all use cases
  • export witnesses as well

Not part of this PR: enable verkle for blocktests

@gballet gballet force-pushed the t8n-verkle-exec-rebased branch from 95eb3c9 to e23e237 Compare May 7, 2024 11:33
Copy link
Collaborator

@jsign jsign left a comment

Choose a reason for hiding this comment

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

LGTM, some things I'd have to look in more detail to be entirely convinced since I'm not used to how this tool is usually called from the testing infra -- but at some point we'll figure out soon if there's a problem.

Left some comments for your consideration.

trie/verkle_iterator.go Show resolved Hide resolved
@@ -415,7 +400,7 @@ func (db *cachingDB) OpenStorageTrie(stateRoot common.Hash, address common.Addre
}
}
if db.InTransition() {
fmt.Printf("OpenStorageTrie during transition, state root=%x root=%x\n", stateRoot, root)
// fmt.Printf("OpenStorageTrie during transition, state root=%x root=%x\n", stateRoot, root)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: wanna keep or remove?

cmd/evm/main.go Outdated Show resolved Hide resolved
err := vkt.UpdateAccount(addr, account)
if err != nil {
return fmt.Errorf("error inserting account: %w", err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the code missing chunkifying acc.Code and inserting that?

chunks := trie.ChunkifyCode(acc.Code)
vk.UpdateContractCode(...)

Copy link
Owner Author

Choose a reason for hiding this comment

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

yup, but let's see if this is still in use first

Copy link
Owner Author

Choose a reason for hiding this comment

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

it is

* t8n: add verkle-code-chunk-key cmd

Signed-off-by: Ignacio Hagopian <[email protected]>

* t8n: add verkle-chunkify-code cmd

Signed-off-by: Ignacio Hagopian <[email protected]>

* t8n: create separate verkle parent command

Signed-off-by: Ignacio Hagopian <[email protected]>

---------

Signed-off-by: Ignacio Hagopian <[email protected]>
@jsign
Copy link
Collaborator

jsign commented May 9, 2024

For PR readers, extra commands where added via #435

gballet and others added 3 commits May 16, 2024 20:34
* fix: add the storage processed flag to t8n.

* chore: cancun -> prague typo.
@gballet gballet force-pushed the t8n-verkle-exec-rebased branch from d5e3191 to 122366d Compare June 28, 2024 16:42
@@ -1772,7 +1772,7 @@ func (bc *BlockChain) insertChain(chain types.Blocks, setHead bool) (int, error)
// If the verkle activation time hasn't started, declare it as "not started".
// This is so that if the miner activates the conversion, the insertion happens
// in the correct mode.
bc.stateCache.InitTransitionStatus(false, false)
bc.stateCache.InitTransitionStatus(false, false, common.Hash{})
Copy link
Owner Author

Choose a reason for hiding this comment

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

note to self: this is probably what kills the shadowfork

Comment on lines -82 to -87
parent := p.bc.GetBlockByHash(block.ParentHash())
if !p.config.IsPrague(parent.Number(), parent.Time()) {
InsertBlockHashHistoryAtEip2935Fork(statedb, block.NumberU64()-1, block.ParentHash(), p.bc)
} else {
ProcessParentBlockHash(statedb, block.NumberU64()-1, block.ParentHash())
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

this was one of the issues: worker.go also need to be updated not to insert the 256 blocks

@gballet
Copy link
Owner Author

gballet commented Jun 29, 2024

This PR is good for merging, but before we can do that, we need to:

  • confirm that t8n: verkle genesis support #442 still works, and merge it if it does
  • rebase
  • remove the eip2935 stuff as it's broken and not the final version anyway, it should be fixed in the final branch.
    • The t8n implementation of that branch needs to be kept up to speed, the best way would be to call the base function directly

@gballet gballet changed the title rewrite verkle support in evm t8n add verkle support to evm t8n for test framework Jun 29, 2024
cmd/evm/internal/t8ntool/execution.go Show resolved Hide resolved
cmd/evm/internal/t8ntool/execution.go Outdated Show resolved Hide resolved
cmd/evm/internal/t8ntool/execution.go Outdated Show resolved Hide resolved
cmd/evm/internal/t8ntool/execution.go Outdated Show resolved Hide resolved
cmd/evm/internal/t8ntool/execution.go Show resolved Hide resolved
cmd/evm/internal/t8ntool/execution.go Outdated Show resolved Hide resolved
cmd/evm/internal/t8ntool/execution.go Outdated Show resolved Hide resolved
cmd/evm/internal/t8ntool/execution.go Show resolved Hide resolved
cmd/evm/internal/t8ntool/execution.go Show resolved Hide resolved
cmd/evm/internal/t8ntool/execution.go Show resolved Hide resolved
@gballet gballet mentioned this pull request Jul 29, 2024
@gballet
Copy link
Owner Author

gballet commented Jul 30, 2024

Rebased in another PR, closing this one

@gballet gballet closed this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants