Several fixes #425
Several fixes #425
Conversation
| // be charged. | ||
| if chain.Config().IsEIP4762(header.Number, header.Time) { | ||
| state.AccessEvents().BalanceGas(w.Address[:], true) | ||
| } |
There was a problem hiding this comment.
This is removed because AccessEvent is only used for gas accounting. The full witness construction could potentially be handled in another structure.
Let's just focus the gas accounting for now.
There was a problem hiding this comment.
My PR is also used to simplify a rebase. While I get this is just gas accounting from this PR's point of view, it is a layer that does build the witness.
There was a problem hiding this comment.
Myeah, my gut feeling is that witness should be something different. Basically a list of flat states needed for state execution along with their proofs, and also the input for post-state hashing.
AccessEvent just tracks which part of trie is touched.
I think accessEvent can be used as an input for witness construction, but not the witness itself.
There was a problem hiding this comment.
I agree it will be something different, but I need to be able to keep rebasing as painlessly as possible until the final shape of things is built.
| @@ -158,11 +157,6 @@ func ApplyTransactionWithEVM(msg *Message, config *params.ChainConfig, gp *GasPo | |||
| if msg.To == nil { | |||
| receipt.ContractAddress = crypto.CreateAddress(evm.TxContext.Origin, tx.Nonce()) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
same, accessEvent is only for gas accounting, let's decouple the witness construction from the accessEvent now.
There was a problem hiding this comment.
Same thing as above: sure, I get the point, but let's not make my rebase more difficult than it needs to be. There are a series of very important optimizations that rely on this behavior and I would like to keep it working while we figure out the prefetcher.
|
This pull request implements fixes and removes unnecessary functions/operations for building witnesses. I think that witness construction should be managed by another struct, with the logic shared by both Merkle and Verkle. However, it's important to note that even with these changes, the solution could still be problematic. One open question is: what if the VM operation is reverted which mutate the |
There was a problem hiding this comment.
Thanks for the suggestions. The biggest issue is the rewrite of where the point cache is created/handled, as this will have a large performance impact.
For the removal of the "witness construction" aspects, I'm against it at this time since it makes the rebase potentially more complicated, but if you insist then I guess I'll manage.
| @@ -158,11 +157,6 @@ func ApplyTransactionWithEVM(msg *Message, config *params.ChainConfig, gp *GasPo | |||
| if msg.To == nil { | |||
| receipt.ContractAddress = crypto.CreateAddress(evm.TxContext.Origin, tx.Nonce()) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Same thing as above: sure, I get the point, but let's not make my rebase more difficult than it needs to be. There are a series of very important optimizations that rely on this behavior and I would like to keep it working while we figure out the prefetcher.
| @@ -208,8 +202,5 @@ func ProcessBeaconBlockRoot(beaconRoot common.Hash, vmenv *vm.EVM, statedb *stat | |||
| statedb.AddAddressToAccessList(params.BeaconRootsAddress) | |||
| } | |||
| _, _, _ = vmenv.Call(vm.AccountRef(msg.From), *msg.To, msg.Data, 30_000_000, common.U2560) | |||
| if vmenv.ChainConfig().Rules(vmenv.Context.BlockNumber, true, vmenv.Context.Time).IsEIP4762 { | |||
| statedb.AccessEvents().Merge(txctx.AccessEvents) | |||
There was a problem hiding this comment.
Same comment here, sure it's not directly connected to gas consumption but it's necessary for witness building and this is something I'd like not to lose if there is a rebase from hell, which there will be.
| // be charged. | ||
| if chain.Config().IsEIP4762(header.Number, header.Time) { | ||
| state.AccessEvents().BalanceGas(w.Address[:], true) | ||
| } |
There was a problem hiding this comment.
I agree it will be something different, but I need to be able to keep rebasing as painlessly as possible until the final shape of things is built.
| @@ -440,7 +440,7 @@ func gasCallCode(evm *EVM, contract *Contract, stack *Stack, mem *Memory, memory | |||
| address := common.Address(stack.Back(1).Bytes20()) | |||
| transfersValue := !stack.Back(2).IsZero() | |||
| if transfersValue { | |||
| gas, overflow = math.SafeAdd(gas, evm.AccessEvents.ValueTransferGas(contract.Address().Bytes()[:], address.Bytes()[:])) | |||
| gas, overflow = math.SafeAdd(gas, evm.StateDB.AccessEvents().ValueTransferGas(contract.Address().Bytes()[:], address.Bytes()[:])) | |||
There was a problem hiding this comment.
While we're at it, it might make sense to just define a AccessEvents() method on interpreter, so that we reduce the verbosity.
There was a problem hiding this comment.
My hunch is that we might finally define a list of AccessEvent functions in vm.StateDB interface, e.g. AddAccountBalanceForRead, AddAccountBalanceForWrite, etc. Probably we finally need to handle the AccessEvent mutation revert? It makes more sense to interact with StateDB imo
I still hope we can remove the witness construction in this pull request.
As your current pull request is already complicated enough, i hope we can focus on gas accounting and make it right. |
No description provided.