Conversation
2abc735 to
42d33ca
Compare
42d33ca to
9a33c6a
Compare
|
Overall, LGTM. I requested we discuss this on ACD 151. |
|
What is the use case for this? These days I'm hesitant to proactively add things that don't have incredibly compelling use cases because once added, changing/removing become hard. When we switch to verkle trees or make other changes the account structure may change so I'm particularly hesitant to expose all of this as it feels like a leaky abstraction. |
|
If the intent is for debugging purposes, can we add it to a |
I'd be fine with that |
|
I would prefer to see Also, I see this as useful for more than just debugging, so I'm ok with the |
Do you have some example use cases other than debugging? |
Optimism commits to the storage root of a particular L2 contract on L1 for withdrawals. A user must provide a storage proof to finalize the withdrawal. We currently get the storage root with |
I can live with either, but I do think this is information should be a first-class citizen of
However, getting things using eth_getProof sounds like a safer solution though |
Makes sense to me |
What problem are you having with |
|
|
SO... Geth (as represented by me and @lightclient ) is on board behind this, so is Besu (via @shemnon ) -- what do we need to do to move forward? |
|
I'd like to get a 👍 from nethermind and erigon. I requested we mention it on the next ACD. If they thumb up on this PR out-of-band I think we can just merge. |
| Account: | ||
| title: Account | ||
| type: object | ||
| required: |
There was a problem hiding this comment.
Propose including the blockHash this was resolved from to the return value to support the case that a tag was the block param passed in. If the EL is providing the account state as of the "latest" block, the caller likely has no idea what block that is. This api is not in the engine API so we can't assume the caller is aware of current chain state.
There was a problem hiding this comment.
Myeah, that makes sense... but IMO it doesn't belong in the Account object, but rather as a metadata along with the account.
so maybe something like this?
{
"blockHash": "0x...",
"stateRoot": "...",
"account": { ... }
}There was a problem hiding this comment.
I think we already have this issue with all the other state reading methods, such as eth_getBalance, eth_getTransactionCount, etc. I think it is better to retain consistency with the other methods and in the future possible move them all to this format above.
|
Erigon doesn't store historical MPT roots, so we can return |
|
the question has just been asked on ACD: verkle trees won't be able to return |
|
Seems like 2 main pieces of feedback from ACD:
|
|
A lot of things will change with verkle, since it redefines the tree leaves. Let's see what makes sense when we get to that point, imo.
Re storage root, I don't understand what the problem is.
|
This does not really compute for me. Please elaborate. Do you mean that in order to produce the consensus-definition of an account, they have to iterate the (flat) storage? If so, I'm curious which clients, apart from Erigon. I really do not think (hope) this is a common way to handle account storage. |
On that note, any thoughts now or when Verkle to retrieve the whole account storage using the same method? |
Well, leaving verkle aside for now, retrieving the whole storage is not "generally" doable. For clients which have only a trie-based represenation, returning the whole storage might mean iterating over a trie containing hundreds of thousands of leaves. For clients with a flat storage representation, it is "simpler" but still, for some accounts, the storage portions are huge. And returning those over RPC is a huge job. Anyway, it's not really related to this feature |
@holiman Thank you, yes you are right if you store only the trie representation it is pretty impossible, hence the Verkle comment as potential improvement / thought / hope :). But you are right some contracts will have a huge state too, so not great for an rpc call. My thought was for users not needing to constantly request data if they could verify it has not changed locally (cached), but bringing down the whole state will negate the benefit on big accounts / contracts as opposed to retrieving a specific value using account_proofs, and yes not related to this either. |
|
Coming back to this, I think given the fact that i) all this information is available via other means, such as I guess last call- is there a reason to continue pursuing this versus using the existing methods given the tradeoffs? |
|
Fair enough.. Closing |
This PR adds a new method to the
ethnamespace:eth_getAccount.The new method returns either
nullif the account does not exist in the state trie at the given block/state root.The definition of
accountis the trie-leaf-definition:balance,nonce,rootstorageRoot(storage root), andcodeHash. It does not containaddress, since that is not necessarily available in a node which has the state but not out-of-state metadata (such as preimages).Consideration: maybeDoonerootshould be renamedstorageRootfor clarity.Original discussion
ethereum/go-ethereum#26231 (comment)