Skip to content

IPC-506: Fix GetCode and GetStorageAt for ethaccount#511

Merged
raulk merged 3 commits into
mainfrom
ipc-506-fix-getcode-ethaccount
Jan 8, 2024
Merged

IPC-506: Fix GetCode and GetStorageAt for ethaccount#511
raulk merged 3 commits into
mainfrom
ipc-506-fix-getcode-ethaccount

Conversation

@aakoshh

@aakoshh aakoshh commented Jan 4, 2024

Copy link
Copy Markdown
Contributor

Fixes #506

This is an alternative for #507 that makes 22 exit code work like a PLACEHOLDER account by returning None from read_evm_actor.

I think it's sufficient indication that we're dealing with ETHACCOUNT, because:

  • read_evm_actor is an internal method to the Ethereum API facade, it's only ever called with H160 addresses and two well known method numbers (GetCode and GetStorageAt), never with user input
  • therefore the Message will go to an f410 address constructed from the 20 byte address hash, and if that exists, it's some sort of Ethereum account or contract (if it didn't exist we wouldn't get code 22)
  • if the actor doesn't handle the two well known method numbers, it's not an EVM actor

@raulk raulk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is rather brittle and not very future-proof, e.g. what if this Eth address is a masked ID address for a Wasm or built-in actor that does support the method number? IMO the correct solution is to obtain the type of the actor from the state tree, and act differently. This is what Lotus does for these two methods.

There is also a third edge case in the Eth API that requires the actor type:

https://github.com/filecoin-project/lotus/blob/a3417e7488356d97ada733ffa27a7df22b6d1d16/node/impl/full/eth.go#L374

Let me submit a draft PR with the solution I've been working on.

@aakoshh

aakoshh commented Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

You are right that if someone uses a masked ID then they can get some strange results. Most likely they will get an error, because either the input or the output will not parse correctly, resulting in some kind of error code which is not 22, so they will correctly see the error message in the response.

@aakoshh
aakoshh force-pushed the ipc-506-fix-getcode-ethaccount branch from 4ee1ab6 to b98a561 Compare January 4, 2024 21:26
@aakoshh

aakoshh commented Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

Thank you for the link to the EthGetTransactionCount implementation. Please forgive my ignorance, but why does it need special handling for EVM actors? Aren't those contracts for which the nonce will always be 0? Why would anyone even query "the number of transactions sent from this address", what does it mean for them, and why does it matter if the contract is alive or not?

@aakoshh

aakoshh commented Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

Ah, okay, I see there is a separate nonce inside the EVM state with a different meaning from the sequence of the actor itself, something about CREATE and CREATE2. Never saw this mentioned in relation to eth_getTransactionCount before. I am guessing that because geth returns the nonce, we are faithfully replicating this even though if we read the definition of the JSON-RPC method 0 (ie. sequence) would make more sense?

@aakoshh

aakoshh commented Jan 8, 2024

Copy link
Copy Markdown
Contributor Author

@raulk I suggest we merge this and its tests, so that at least the bug is squashed, and then you have time to finish your version with the manifest without having to point at different branches.

@raulk raulk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good enough for an intermediate fix.

@raulk
raulk merged commit 003ac8f into main Jan 8, 2024
@raulk
raulk deleted the ipc-506-fix-getcode-ethaccount branch January 8, 2024 22:07
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.

eth_getCode and eth_getStorageAt unable to deal with non-EVM actors

2 participants