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

consensus/beacon,core, params, trie: verkle-compatible access list #29234

Closed
wants to merge 1 commit into from

Conversation

gballet
Copy link
Member

@gballet gballet commented Mar 12, 2024

Augment the interface to accessList to support more use cases that are necessary for verkle, in order to merge more data.

Highlights:

  • Functions to check for the presence of an account/slot in the list are removed, as they lose meaning in a verkle context
  • Instead, the function to add an account/slot to the list will directly return the consumed gas. This is necessary since the verkle gas model is quite different.
    • As a result, the constantGas is removed and the warm costs are directly returned by AddSlot/AddAddress when needed.
  • AddAddress is modified to specify which account item is accessed. This does not matter for 2929 but is necessary for verkle.

https://eips.ethereum.org/EIPS/eip-4762

@gballet gballet requested a review from fjl March 12, 2024 11:17
Co-authored-by: Ignacio Hagopian <jsign.uy@gmail.com>
Comment on lines -1314 to -1320
if addrMod {
// In practice, this should not happen, since there is no way to enter the
// scope of 'address' without having the 'address' become already added
// to the access list (via call-variant, create, etc).
// Better safe than sorry, though
s.journal.append(accessListAddAccountChange{&addr})
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This change breaks tests. The comment itself says that this should not happen. My preference is to get rid of it and fix the tests: the reason is that in verkle, state and account are dissociated. It would be possible to link them, but it would be work done for a situation that should not occur in the first place.

Otherwise, I can add a addrMod return

ContainsAddress(address common.Address) bool
Contains(address common.Address, slot common.Hash) (addressPresent bool, slotPresent bool)
Copy() AccessList
AddAddress(address common.Address, items ALAccountItem, isWrite ALAccessMode) uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is e.g. isWrite needed?
For example

statedb.Witness().AddAddress(w.Address, state.ALAllItems, state.AccessListWrite)

Why do we need to specify state.ALAllItems and state.AccessListWrite ?

Copy link
Member Author

Choose a reason for hiding this comment

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

because verkle charges a different amount of gas whether you are writing or reading the state: https://eips.ethereum.org/EIPS/eip-4762#witness-gas-costs

Comment on lines +363 to +365
if chain.Config().IsVerkle(header.Number, header.Time) {
statedb.Witness().AddAddress(w.Address, state.ALAllItems, state.AccessListWrite)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't the operation statedb.AddBalance(w.Address, amount) implicitly just add it to the witness without it needing to be done explicitly here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be done in some (but not all) places, but the AddBalance function then has to be modified to return gas.


type ALAccountItem uint64

const (
Copy link
Contributor

Choose a reason for hiding this comment

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

pls document what these mean / do

AddSlot(address common.Address, slot common.Hash, isWrite ALAccessMode) uint64
DeleteSlot(address common.Address, slot common.Hash)
DeleteAddress(address common.Address)

Copy link
Contributor

Choose a reason for hiding this comment

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

pls document these

@holiman
Copy link
Contributor

holiman commented Mar 15, 2024

I have two questions/thoughts

First of all, the goal of these two things are quite different, aren't they?

  • The access list is a way to declare "the execution will at some point access X, and I want to pay for that up front, so the execution does not run out of gas at that point".
  • The verkle witness serves a totally different purpose, doesn't it?

And a second question

  • The access list is journalled: if we enter a scope, access X,Y and Z, and the scope reverts, then we want those to not be in the access list (otherwise there's a backdoor way to add an item to an access list without paying the cost: only paying the cost of a failed call, which may be lower).
  • But for a witness building, any access in any scope needs to be preserved. So you intrinsically do not want journalling on a witness builder.

@holiman
Copy link
Contributor

holiman commented Mar 15, 2024

A short summary of my thought from our chat today.

  • I don't think we should mix eip2929 access lists and verkle-witnesses. Those are IMO two orthogonal things.
  • We should instead change some locations where we now do
if rules.IsBerlin{ 
   // do things with access lists
} 

into

if rules.Is2929Enabled{ 
   // do things with access lists
} 

And make verkle-fork set Is2929Enabled to false.

Most of all witness-building can happen in the dynmic gas functions of the following functions: SSTORE, SLOAD, SELFDESTRUCT, BALANCE, EXTCODESIZE, EXTCODEHASH, CALL, CALLCODE, DELEGATECALL, STATICCALL. The remaining places that adds to the witness are

  • Transaction paying: sender and destination. This can be souped up where buyGas for transaction happens
  • Block rewards
  • Withdrawals

All three location can be handled in state_transition.go, I think.

Intrinsically, the witness-builder is a simpler thing than the access list, since there's no journalling. I think the implementation will be much less messy if it is not mingled with accesslist management.

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.

2 participants