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

About use of eth_getBlockReceipts JSON RPC for cryo txs #93

Closed
cool-mestorf opened this issue Oct 26, 2023 · 4 comments · Fixed by #94
Closed

About use of eth_getBlockReceipts JSON RPC for cryo txs #93

cool-mestorf opened this issue Oct 26, 2023 · 4 comments · Fixed by #94

Comments

@cool-mestorf
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Use of new JSON RPC API - eth_getBlockRecipts broke some scripts that worked before, especially for other chains that forked ethereum's execution clients (apparently geth) complying with most of JSON RPC specs but not very up-to-date. Tested on optimism & arbitrum, for official mainnet sequencer rpc endpoints.

$ cast rpc eth_getBlockReceipts 0x6A31748 --rpc-url https://mainnet.optimism.io
Error:
(code: -32001, message: rpc method is not whitelisted, data: None)

$ cast rpc eth_getBlockReceipts 0x896C1CB --rpc-url https://arb1.arbitrum.io/rpc
Error:
(code: -32601, message: the method eth_getBlockReceipts does not exist/is not available, data: None)

This problem occured after merge of #89 which was introduced to filter out reverted txs (#46). While this feature will be indeed crucial and very useful for majority of cryo users, I have some doubts about its current specification and implementation.

First, eth_getBlockReceipts was standarized recently. While almost every execution clients implement the method on ethereum, I found most of other evm-like chains and its execution clients do not have the method. The only chain I found implementing this method aside from ethereum was bnb.

Second, in terms of clarifying which data the user wants to extract, the status, gas_used and transaction_index etc. are actually properties of receipts, not txs. If only raw transaction data is needed for certain analysis, fetching receipts causes unnecessary overhead. (Still, I think there are very few analysis can be done with tx data without receipts) This is important because while there were receipt related properties on transaction schema before, it was considered optional, but current implementation for --exclude-failed seems to force fetching receipts and fails if fetching receipt errors whether the option is supplied or not.

Describe the solution you'd like

  1. implement workaround when eth_getBlockReceipts errors - iterating over transaction hashes and call eth_getTransactionReceipt. This will emit much more RPC calls than the original solution, but is considerable if it is better than failing.
  2. When eth_getBlockReceipts errors leave receipt field None, and if --exclude-failed option was supplied, panic on transform step

Also, this RPC method will have to be mentioned on README.md as used method for fetching txs data.

Describe alternatives you've considered

  • separating cryo txs from cryo receipts - but this would not help much for users to overcome this issue but increase product's complexity.

Additional context

If specs are fixed, I would be happy if I can submit a PR to implement the fix.

@sslivkoff
Copy link
Member

sslivkoff commented Oct 28, 2023

part of this is a documentation issue, part of this is a bug

documentation: when cryo fetches transaction data, it only grabs the transaction receipts if either of the gas_used or success columns are in the set of collected columns. these columns are in the default set of columns, so are collected by default when a user types cryo txs .... but a user can add the argument --exclude-columns succes gas_used and then receipts will no longer be collected. I dont think these should be separated into two tables because they have a 1:1 relationship, but I would be open to adding new syntax to make this easier to type, something like cryo txs_no_receipts ... instead of cryo txs ... to mean that all receipt columns should be excluded

bug: when eth_getBlockReceipts is not supported by an endpoint, the code is supposed to fallback to eth_getTransactionReceipts. if it is not doing that, it is a bug. there is also an opportunity for smarter logic here, like using knowledge of the underlying network or client metadata to determine whether eth_getBlockReceipts is supported

@cool-mestorf
Copy link
Contributor Author

Thanks for your kind reply. I also agree with all the points you mentioned: introducing a different data type for txs without receipts (except the example name was a bit verbose), bug about fallback logic, and smarter rpc selection with metadata. Any actionable item that I may contribute? I think the bug you mentioned about fallback is most urgent one to work on, and I had no clue of the fallback when I read through the logic for fetching receipts. Is it really present but not applied, or is it omitted?

@sslivkoff
Copy link
Member

the logic is here and here

the fallback logic got a bit mangled in the refactor

there should be a get_block_receipts function that first tries to get bulk block receipts and if that fails it falls back to individual tx receipt calls

@cool-mestorf
Copy link
Contributor Author

I implemented the originally intended fallback logic in #94. Could you please take a look and let me know your opinions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants