Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

feat: add support for eth_getBlockReceipts #365

Merged
merged 4 commits into from
Aug 8, 2021
Merged

feat: add support for eth_getBlockReceipts #365

merged 4 commits into from
Aug 8, 2021

Conversation

ekzhang
Copy link
Contributor

@ekzhang ekzhang commented Aug 4, 2021

Thanks for the great library, here's a small PR for a bump that I ran into while trying to modify mev-inspect-rs to work on an Erigon archive node.

Motivation

The Erigon node does not support parity_getBlockReceipts, but it does support eth_getBlockReceipts. This is listed on the RPC interface at https://github.com/ledgerwatch/erigon/blob/devel/cmd/rpcdaemon/README.md.

Also worth mentioning that this is an open PR at ethereum/go-ethereum#19721 as well.

Solution

Add a few function to the Middleware trait that invokes eth_getBlockReceipts.

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Hey thanks for the contribution!! Do you think there might be a way to have the client check whether it's connected to an Erigon or Parity node and abstract away the 2 methods and only have one? That'd be a nice upgrade. Otherwise I'm OK with merging this as-is, as long as you add one small comment that this method is not available on Geth (the PR looks stale, given that there has been no EIP for the RPC call AFAICT)

@ekzhang
Copy link
Contributor Author

ekzhang commented Aug 6, 2021

Sounds good! There is a way to check this directly using the web_clientVersion RPC, which is standard. Maybe I could do the following:

  • Add another .client_version() function to the Provider / Middleware.
  • Have .get_block_receipts() call .client_version() before determining which RPC to send, parity_getBlockReceipts or eth_getBlockReceipts.
  • Add #[deprecated(since="0.2.3")] to the .parity_block_receipts() function.

Does this sound reasonable?

@gakonst
Copy link
Owner

gakonst commented Aug 7, 2021

Yes that SGTM. I think the overhead of having an additional RPC request per each of these calls might be fine. Alternatively, I think we could also just cache the node-type inside the Provider struct (behind an enum ideally). The first time the call is made, it'd set that enum to the variant returned by web3_clientVersion, and then it'd just reuse it

@ekzhang
Copy link
Contributor Author

ekzhang commented Aug 8, 2021

Hey @gakonst, I tried to add a cached client_version result to the Provider struct, but realized this won't work directly because:

  • To cache the return value onto an self.client_version: Option<String> field, the client_version() or get_block_receipts() function would need to be changed to take a &mut self receiver.

I can think of a couple options:

  • If I used a futures_util::lock::Mutex<Option<String>> instead to keep the cached client version, and locked it every time.
  • Same as above, but use a Arc<Mutex<_>> instead, which gives an auto-derive for Clone and also shares the client version cache between the two objects.
  • Just call client_version() every time without caching.

None of these are super clean, what would be best? Or is it even just better to stay simple and keep the two separate functions for eth_getBlockReceipts and parity_getBlockReceipts?

@gakonst
Copy link
Owner

gakonst commented Aug 8, 2021

@ekzhang Another approach:

  1. Make the Provider constructor async, call client_version inside it and set it to a String var (the async constructor will be a breaking change prob mean that you'll need to update it in a few places - hope that's not a big pain..)
  2. If the node type is Erigon -> call the method you added, if Parity -> call parity, otherwise return error (e.g. Unsupported node type)

If you think these are a bit out of scope, I'm happy to merge the PR as-is and we can consider it for another PR.

@ekzhang
Copy link
Contributor Author

ekzhang commented Aug 8, 2021

Amazing, thanks for the help! I'm not sure I understand the implications of such a change fn new() -> async fn new() well enough and would prefer to merge as-is. :)

@gakonst gakonst merged commit f165c13 into gakonst:master Aug 8, 2021
@gakonst
Copy link
Owner

gakonst commented Aug 8, 2021

Sounds good! Merged, and we'll track separately.

@ekzhang ekzhang deleted the feat/block-receipts branch August 8, 2021 21:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants