-
Notifications
You must be signed in to change notification settings - Fork 753
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
RPC state manager has a few inconsistent behaviors #3301
Comments
Interesting the eth_getProof behavior comes from a geth bug. Not sure if it's worth patching it on the ethjs end ethereum/go-ethereum#28357 . One issue with this bug is it falsely returns |
Do you mean with (1) that NormalStateManager ( Did you check (2) for the same accounts? Because there is a weird quirk in the |
I'm not entirely sure what that Geth PR fixes. For the returned proofs, in case that an account with code hash with 0x00.. is returned (this should never happen, if we do this, this is incorrect I believe) then for semantics we might want |
Oh yea you are right I'll update description for this. I'm currently using my own fork of the state managers (because the rpc state manager wasn't released at the time I built them) but I want to move back to just using the ethereumjs ones soon. When I do that I'm happy to make these more consistent. I believe this issue might cause issues in gas estimation (since there is a surcharge for uninitialized accounts) but I haven't tested yet.
So this isn't a bug in ethereumjs when I dug into it it's actually a bug in Geth. They fixed the bug but most rpc providers including alchemy and quicknode are still falsely returning I've reached out to many rpc providers about this though and I'm trying to get it fixed on their end. Thus I don't think we need to implement a workaround. |
You are right regarding gas issues for initialized accounts if we would use the output for ethereumjs-monorepo/packages/statemanager/src/rpcStateManager.ts Lines 225 to 243 in 98ee94e
accountExists ) for this in our EVM.
I nevertheless agree that we should ensure the outputs for RPC / Default StateManager should be consistent though 🤔 |
The first point should be at least partially addressed, although it's hard to perfectly handle all possible cases with RPCSM. For the second point, I agree that the best approach would be to fix the issue on the provider's side since the fix already exists. We could however note that the issue exists somewhere in our documentation and/or code and generally provide a caveat that some inconsistency in behavior is possible since a third-party provider is being used. |
Thanks @scorbajio! I have notified all RPC providers that I know of about this issue and I believe at least alchemy has fixed it on their end. I think we can close this issue but feel free to reopen if you disagree |
I'm happy to make these fixes but opening up issues since it will be a couple of weeks til I can and I don't want to forget.
Found 2 issues
1. Inconsistent behavior compared to NormalStateManager regarding uninitialized accounts with
getAccount
NormalStateManager will throw an error when no account exists. Meanwhile, the Rpc state manager will always return an empty account rather than throwing.
2. Inconsistent behaviors for different rpcs.
The rpc provider uses
eth_getProof
which has inconsistent behaviors for different rpc providers. Alchemy will return a code hash of0x0000000000000000000000000000000000000000000000000000000000000000
while other providers will return a code hash of0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470
. I think alchemy behavior is a bug but this also causes inconsisten behavior on our end from not handling itThe text was updated successfully, but these errors were encountered: