-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Bug: Call handler improperly invoked when call was reverted on-chain #3701
Comments
Thank you very much for filing this @ryley-o 🙏 🤝 |
Great write up. We'll dig into this. |
(updated description with a link I accidentally left out to our decentralized subgraph!) |
Hi! Thanks for the detailed description. I discussed with @leoyvens. We had previously fixed a similar issue #2409 where callHandlers were triggered for failing transactions. This is a more nuanced case in that the transaction succeeded, it's just that one of the underlying calls failed, which reverted the execution. Fixing this definitively under the current architecture would require fetching all the call traces for every transaction which triggers a callHandler, which would have a non-trivial performance impact. We are currently testing the Firehose as an alternative data source for Ethereum indexing (vs. RPC), which would allow us to fix this bug in a performant fashion (as the firehose provides all the traces). That work is ongoing and will hopefully be available soon, so that is probably the happiest path from where we are. Again apologies for this, as this is certainly a tricky bug. |
I dug into this particular transaction from how we see them in the firehose block (see json format below) Indeed, the DELEGATE call at index 5 calls method "multiSend()", which makes two children calls (index 6 and 7), both to AddProject(). The CALL 7 fails and reverts ( This is probably why the RPC implementation cannot catch this particular case, there is no error on this call. By using the "stateReverted" from the firehose, we can easily fix the behavior and ignore the "successful but reverted" calls the same way that we do with "failed and reverted" calls. (I'm dumping more data mostly for documentation purpose, but also for all the curious minds...) Simplified map of the calls
Content of firehose transaction (some fields have been removed for readability)
|
* fix firehose eth CallHandler triggering on reverted calls * call handlers in firehose: use state_reverted, not status_reverted, to also catch issue #3701 * align firehose with RPC behavior using status_reverted and status_failed Co-authored-by: Stéphane Duchesneau <stephane.duchesneau@streamingfast.io>
Do you want to request a feature or report a bug?
bug
What is the current behavior?
Currently, we expect call handlers to be invoked only when they are not reverted and associated state changes are applied to the blockchain. However, we observe a case where our subgraph call handler was invoked, but the execution of the call was reverted on-chain, and no associated state changes were not applied to the blockchain.
If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.
We believe that this transaction would currently always show an example of the bug:
https://bloxy.info/tx/0xc1f4610a49f7b6e5672a31fd4df6c0fa8bf63b54ade7a2cb6d7424df1822ceb5
For context, the transaction is intended to be a Gnosis Safe multiSend transaction where two projects are added to Art Blocks. The first
addProject
succeeded, but the secondaddProject
reverted (out of gas). The result of the transaction is that bothaddProject
calls were reverted (blockchain state does not see either project added), but the transaction as a whole succeeded. Thus, we would expect noaddProject
call handler to be invoked in our subgraph. However, we see that the firstaddProject
was invoked in our subgraph, leading to a mismatch of blockchain state vs. subgraph state.We can prove that our call handler was triggered via the following query. Comparing blocks
14676509
vs.14676510
shows that project 308 was added in our subgraph.reference subgraph: https://thegraph.com/explorer/subgraph?id=5So3nipgHT3ks7pEPDQ6YgSFhfEmADrh481P9z1ZtcMA&view=Playground
What is the expected behavior?
The expected behavior is as described above - we would not expect a call handler to be invoked when it was actually reverted during execution.
Other
The following issue may be related to the best solution for this bug. The bug described in this issue is slightly different because in this case, we think the call was improperly interpreted to not have been reverted (whereas the linked issue below would just give visibility to all calls that were properly interpreted to have reverted).
#1055
The text was updated successfully, but these errors were encountered: