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

Define eth_account #329

Closed
wants to merge 6 commits into from
Closed

Define eth_account #329

wants to merge 6 commits into from

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Nov 24, 2022

This PR adds a new method to the eth namespace: eth_getAccount.
The new method returns either

  • The account, if it exists in the state trie, or
  • null if the account does not exist in the state trie at the given block/state root.

The definition of account is the trie-leaf-definition: balance, nonce, root storageRoot (storage root), and codeHash. It does not contain address, since that is not necessarily available in a node which has the state but not out-of-state metadata (such as preimages).

Consideration: maybe root should be renamed storageRoot for clarity. Doone

Original discussion
ethereum/go-ethereum#26231 (comment)

@lightclient
Copy link
Member

Overall, LGTM. I requested we discuss this on ACD 151.

@MicahZoltu
Copy link
Contributor

MicahZoltu commented Nov 30, 2022

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.

@MicahZoltu
Copy link
Contributor

If the intent is for debugging purposes, can we add it to a debug_ namespace rather than eth_ namespace, to make it clear that dapps shouldn't rely on the content being consistent long term?

@holiman
Copy link
Contributor Author

holiman commented Nov 30, 2022

If the intent is for debugging purposes, can we add it to a debug_ namespace rather than eth_ namespace

I'd be fine with that

@shemnon
Copy link
Contributor

shemnon commented Dec 7, 2022

I would prefer to see storageRoot rather than just root.

Also, I see this as useful for more than just debugging, so I'm ok with the eth_ namespace.

@MicahZoltu
Copy link
Contributor

Also, I see this as useful for more than just debugging, so I'm ok with the eth_ namespace.

Do you have some example use cases other than debugging?

@tynes
Copy link

tynes commented Dec 8, 2022

Also, I see this as useful for more than just debugging, so I'm ok with the eth_ namespace.

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 eth_getProof.

@holiman
Copy link
Contributor Author

holiman commented Dec 8, 2022

Also, I see this as useful for more than just debugging,

I can live with either, but I do think this is information should be a first-class citizen of eth. It's a very basic unit of information, and can be useful for a variety of purposes.

We currently get the storage root with eth_getProof.

However, getting things using eth_getProof sounds like a safer solution though

@holiman
Copy link
Contributor Author

holiman commented Dec 8, 2022

I would prefer to see storageRoot rather than just root.

Makes sense to me

@MicahZoltu
Copy link
Contributor

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 eth_getProof.

What problem are you having with eth_getProof that would be solved by eth_account?

@holiman
Copy link
Contributor Author

holiman commented Dec 28, 2022

What problem are you having with eth_getProof

eth_getProof solves a different class of problems. It also requires a lot more 'dedication' from the server, which needs to traverse the trie and look up proofs to serve the request. It is the same role a legacy light server serves, and it cannot be served by a node which is trie-less, does aggressive pruning or generally has a fast direct lookup and slow trie-lookup.

@holiman
Copy link
Contributor Author

holiman commented Jan 20, 2023

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?

@lightclient
Copy link
Member

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:
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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": { ... }
}

Copy link

Choose a reason for hiding this comment

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

yeah, much better. agreed.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree with @lightclient

src/schemas/state.yaml Outdated Show resolved Hide resolved
src/schemas/state.yaml Outdated Show resolved Hide resolved
tests/eth_getAccount/get-account.io Outdated Show resolved Hide resolved
@yperbasis
Copy link
Member

Erigon doesn't store historical MPT roots, so we can return storageRoot only for the current/latest state.

@gballet
Copy link
Member

gballet commented Feb 2, 2023

the question has just been asked on ACD: verkle trees won't be able to return storageRoot as this will no longer exist.

@lightclient
Copy link
Member

Seems like 2 main pieces of feedback from ACD:

  • many clients doesn't have storageRoot very far behind head, including erigon, so I'm not sure how useful that value will be
  • how will this play with verkle trees?

@holiman
Copy link
Contributor Author

holiman commented Feb 2, 2023 via email

@holiman
Copy link
Contributor Author

holiman commented Feb 3, 2023

many clients doesn't have storageRoot very far behind head, including erigon, so I'm not sure how useful that value will be

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.

@juanfranblanco
Copy link

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?

@holiman
Copy link
Contributor Author

holiman commented Feb 3, 2023

, 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

@juanfranblanco
Copy link

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.

@lightclient
Copy link
Member

Coming back to this, I think given the fact that i) all this information is available via other means, such as eth_getProof and ii) verkle does not have the concept of storageRoot and we would need update this method for that, I am inclined to simply close this.

I guess last call- is there a reason to continue pursuing this versus using the existing methods given the tradeoffs?

@holiman
Copy link
Contributor Author

holiman commented Jul 6, 2023

Fair enough.. Closing

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.

9 participants