-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
eth_call
incorrectly reconciling deferred (Filecoin) and immediate (Ethereum) execution
#11205
Comments
Yeah, this logic is definitely wrong. |
Ah, ok, I remember why we did this. The old logic was "as correct as we could make it", at least without significant work. The issue is that:
So, we went with the option of applying all messages in the specified epoch, then applying the user's message at the specified epoch. The alternative would be to make some form of "fake" tipset and/or hack the lookback logic to make everything work even when we're executing at the wrong tipset. Which.. it sounds like we need to do. One remaining question is: does |
Ok, this shouldn't be difficult to fix in the ethereum case. |
Thanks for the extra context!
I see. Yes, this could lead to inconsistency, e.g. if the
Are we able to achieve something like this without increasing the computational and IO cost of
In Ethereum, |
So, an issue here is:
We currently "support" pending so we'll need to be careful not to break users.
Are you sure because my research indicates otherwise (although I haven't actually tested it). |
Here's what I currently have: #10462 But see the question at the end. |
Yep. Here's a reference: https://github.com/ethereum/go-ethereum/blob/1a2135044cd498039be46e1b611627665ff6c4bc/internal/ethapi/api.go#L1109. This loads the state with the root specified in the block header at the requested height, which in Ethereum is the post root state. And the height fed into the EVM is the supplied height. |
Great. That's what I get for trusting stack overflow... So.... we have two options:
3 is, disgustingly, the most correct option at least as far as ethereum is concerned, but I really don't want to do that as it'll be an ETH JSON-RPC API specific feature. |
We should discuss further in slack, but we are currently working on a filecoin grant to add the indexing required to support this sort of usage. |
So here's what I think the problem is:
Here's what the solution can be
|
Hey @aarshkshah1992 -- this is definitely a rough edge, and it's currently coming back to haunt us at IPC 👻 (cc @aakoshh @snissn @cryptoAtwill). We're not necessarily concerned about the cron actor state per-se. In fact, the cron actor state only contains system-defined cron entries, and therefore can be deemed static from a practical viewpoint. The problem is with all the remaining built-in actors state objects that are modified as a result of cron ticks either directly or indirectly (e.g. miner actor, storage power actor, etc.) Earlier in this discussion I suggested to call it a day and slap a warning on eth_call, but since then I have acquired more understanding on how certain Filecoin apps like lending pools are designed, and I now suspect this can be dangerous. To illustrate the scenario, imagine you are lending pool operator with a backend that monitors the chain for accounting or decision-making via a combination of events and eth_calls (e.g. something very common when using The Graph). This case is pretty extreme, but it exposes the correctness flaw:
Instead, I propose the following solution:
|
So... I think there's a much bigger problem: invisible reorgs. I can call
This is an issue because:
No solution involving post-cron state will fix this, only "apply all the messages" is correct. I think we may need a lot of caching (and ideally an on-disk cache). |
Ah, nvm. We're all making some incorrect assumptions about cron. The
So:
This isn't an issue because
We'll blame epoch 1051 which is, IMO, correct. I think the answer is to always use the next block's |
Fendermint was pulling parent effects only until committed_finality-1. This choice assumed that chains like Filecoin leaked deferred execution mechanics through the query mechanism we use (Eth JSON-RPC API). Therefore it was deemed that the safe choice was to lag behind by one epoch. However, we can safely assume that Eth JSON-RPC APIs will only expose blocks that have been executed and whose side-effects are available (unless "pending" is requested). Filecoin reconciles its deferred execution model with Eth client expectations of immediate execution, alas with some known rough edges [1]. Furthermore, this choice assumed that our chain_head_delay can be 0, i.e. we can consume the exposed HEAD of the parent in real time. In practice, that's not a good idea, ever. All in all, this is problematic because it's a hidden. Users inspecting the committed parent finality and seeing N now need to understand that events emitted in N are (counterintuitively) not imported yet, and they need to wait longer. This PR: - Rectifies the queried range so that we import up to, and including, the committed parent finality. - Disallows a zero chain_head_delay. Note: I'm clarifying with @aakoshh the mechanics of Fendermint's Eth JSON-RPC API; concretely, if it renconciles deferred execution or not. There are some off-by-one shifts assumed in a number of places, so unclear. However, by disallowing a chain_head_delay of 0, we are able to work with APIs that expose heights before their results are available. Note 2: when we adopt light clients to watch the parent, some assumptions may change. [1] filecoin-project/lotus#11205
Context: cerc-io/stack-orchestrator#509 (comment)
Ethereum clients and libraries expect that calling
eth_call
with a given epoch (e.g. 1234) will apply the call on the output state of the supplied epoch. In Ethereum, the output is computed immediately and the root hash is part of the block header of height 1234. In Filecoin, deferred execution causes the output of epoch 1234 to be calculated on the next non-null round, normally 1235 (but not necessarily).The logic here is flawed.
LOTUS_SKIP_APPLY_TS_MESSAGE_CALL_WITH_GAS
env var introduced in perf: eth: gas estimate set applyTsMessages false #10546 must go.eth_call
.The text was updated successfully, but these errors were encountered: