core/state: add code to state reader #30808
Conversation
s1na
requested review from
karalabe,
holiman and
rjl493456442
as code owners
core/state/reader.go
Outdated
| // ContractCode returns the code associated with a particular account. | ||
| // | ||
| // - Returns an empty code if it does not exist | ||
| // - It can return an error to indicate code doesn't exist |
There was a problem hiding this comment.
This breaks consistency to the rest of the interface. I have done this to avoid any semantics change in this PR.
I am a bit puzzled why the statedb is recording "not found" errors for code but not for storage or account.
There was a problem hiding this comment.
It says "empty code if it does not exist". Pls clarify.
It says "can return an error to indicate code doesn't exist". Well, does it, or not?
There was a problem hiding this comment.
Sorry clarified. It will return an error if code is not found.
core/state/reader.go
Outdated
| for _, reader := range r.readers { | ||
| // Skip state reader as it doesn't provide contract code and | ||
| // always returns an error. | ||
| if _, ok := reader.(*trieReader); ok { |
There was a problem hiding this comment.
This is a hack: snapshot reader can never return code. So we skip it here and only focus on trie reader.
- If the state reader returns empty result and no error the multi reader can confuse it for empty code.
- If the state reader returns error it can cause multi reader to always return an error and confuse consumer of the Reader.
core/state/reader.go
Outdated
| // - It returns an error to indicate code doesn't exist | ||
| // - The returned code is safe to modify after the call |
There was a problem hiding this comment.
I want the docs to be really clear on semantics here. Here, you say that an error is returned if code doesn't exist. So, does it return errors if you ask for the contract code of anything which has no code? Or only if the account does not exist in the trie?
I mean, there are more points that could be clarified too:
How about asking for the code of a contract which is currently marked as selfdestructed? How about during initcode execution, or just after creation?
Also, the implementations (snapshot) seem to return error just because they are unable to provide the code. That seems like a spec-flaw. The API doesn't allow the implementor to signal "I'm unable to do this", thus forcing it to signal "the code doesn't exist", which is not necessarily the case. An API-wart, IMO
There was a problem hiding this comment.
It's not beautiful, but you could e.g. add another error, so the underlying code can signal NotSupportedErr or something
There was a problem hiding this comment.
I want the docs to be really clear on semantics here. Here, you say that an error is returned if code doesn't exist. So, does it return errors if you ask for the contract code of anything which has no code? Or only if the account does not exist in the trie?
I'm not sure how to formulate this better. What happens is we query disk by code hash. Address is not used at all. I believe it is kept in the (cachingDB) interface because of verkle.
So the semantic is return value only if code with that hash is found and it has size > 0.
How about asking for the code of a contract which is currently marked as selfdestructed? How about during initcode execution, or just after creation?
The reader doesn't care about evm semantics as it consults disk directly.
There was a problem hiding this comment.
Then we should not have address in there... ?
There was a problem hiding this comment.
Also, the implementations (snapshot) seem to return error just because they are unable to provide the code. That seems like a spec-flaw. The API doesn't allow the implementor to signal "I'm unable to do this", thus forcing it to signal "the code doesn't exist", which is not necessarily the case. An API-wart, IMO
Right. I have now added a method "SupportsCodeQuery() bool" to the interface which should return true if a reader allows you to fetch code. I'm not particularly fond of the name
There was a problem hiding this comment.
Then we should not have address in there... ?
Summoning @rjl493456442 as I tried to stay consistent with state.Database
There was a problem hiding this comment.
The address was added for verkle. In verkle, the contract code is tight with the contract.
|
One thing I am not happy with this implementation is: for all reader implementations, now they are tight with code reader. Theoretically, we can divide reader into several different parts: state reader (account reader, storage reader) and code reader. The existing readers (state reader and trie reader) are actually the implementations of account reader and storage reader. Please give me a bit more time to figure out the proper interface definition here. Ideally, it would be: type Reader interface {
AccountReader
StorageReader
CodeReader
}And we can somehow combine different types of reader together and assemble them as a unified Reader. |
|
Closing in favor of #30816 |
Contract code is technically also a part of the state. This PR adds code to the state reader interface. We also add code size because code size is separately cached in the cachingDB (although I think the overhead of computing the code size on the consumer end should not be too bad).
The motivation is #30441 where we are overriding the state reader interface to hook into state read events (account load, storage load and code load). You can see how that is done here: s1na@a22b30b#diff-abb7c3ba4a7ea95d325d17a5c93b0ebde2c5b43e67f2bc3eb0f76a0c5fc7a8cfR335-R337