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

feat: add support for eth_getBlockReceipts #12447

Closed
wants to merge 9 commits into from

Conversation

virajbhartiya
Copy link
Member

@virajbhartiya virajbhartiya commented Sep 11, 2024

Related Issues

Fixes #12429

Proposed Changes

Implimented eth_getBlockReceipts method which accepts a block number and returns all the transaction receipts for it

Additional Info

QuickNode
Infura

Checklist

Before you mark the PR ready for review, please make sure that:

@rjan90
Copy link
Contributor

rjan90 commented Sep 12, 2024

Hey! For the gen-check and docs-check to pass, you need to run make gen and make docsgen - then those should pass.

I will take a look at the other failing tests tomorrow morning

CHANGELOG.md Outdated Show resolved Hide resolved
// Verify that the receipt is correct.
receipts, err := client.EthGetBlockReceipts(ctx, ethtypes.EthBlockNumberOrHash{BlockHash: &receipt.BlockHash})
require.NoError(t, err)
require.Len(t, receipts, 1)
Copy link
Member

Choose a reason for hiding this comment

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

I would love to be able to get a version of this with >1 transactions in a block, we have some other tests that do a multi-message submission and wait for them to land, it would be good if we could replicate that behaviour here in a non-flaky way, have a fish around and see if you can find something, otherwise let me know if you have trouble and I might be able to either find something similar or cook something up

node/impl/full/eth.go Outdated Show resolved Hide resolved
node/impl/full/eth.go Outdated Show resolved Hide resolved
@jennijuju jennijuju linked an issue Sep 13, 2024 that may be closed by this pull request
@virajbhartiya
Copy link
Member Author

I have made the changes mentioned above, I just couldn't figure out get more than 1 transaction block, I am researching a bit more on that.

CHANGELOG.md Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Sep 16, 2024

@virajbhartiya check out #12269 - I think that what we want is already there, TestMultipleEvents has a loop to try and get 3 transactions into the same tipset. I think you should just be able to add something on the end of that test with eth_getBlockReceipts.

You also need to go mod tidy and possibly update your local submodules (make clean deps - although we currently don't have filecoin-ffi builds published [reason unknown yet] so if you do that then you're in for a manual Rust compile which is a bit tedious - alternatively back out the merge-main that you did so you don't pull in the commits on master that have the filecoin-ffi update).

node/impl/full/eth.go Outdated Show resolved Hide resolved
@rjan90 rjan90 mentioned this pull request Sep 16, 2024
51 tasks
@BigLep
Copy link
Member

BigLep commented Sep 17, 2024

@rvagg
Copy link
Member

rvagg commented Sep 17, 2024

Some feedback:

  • We still have some CHANGELOG.md differences beyond just the addition of the new line, you now have some deletions. Perhaps try checking out the version from master to your own branch and re-inserting the single change manually (git checkout origin/master -- CHANGELOG.md).
  • Same thing with go.sum, you've got some additional changes in there that are causing problems with dependencies for running the tests. Just check it out from master to overwrite your changes.
  • As per the discussion on Slack, we're going to need to avoid the Height()+1 thing: https://filecoinproject.slack.com/archives/CP50PPW2X/p1726457493521879

This is quite a diversion from the original task, that's my bad for not looking far enough into the future at the fact that we're being asked for the tipset with the messages but what we need is on the next tipset where those messages are executed. This is getting complicated, and I'm happy to guide you through the process but please let me know if this gets bigger in scope than what you're happy to work on and I can either hop in and contribute or take over.

The main problem we have is that this API can be asked for an arbitrary tipset. If it just took block "numbers" (epochs) then we might be able to get away with it, but it can also be a block hash, which is a tipset CID. Unfortunately, that tipset may not be on the canonical chain, it could be part of a reorg that shunts it off to a fork that's discarded, but it's still valid as long as we have it. So Height()+1 is going to get us the canonical chain, but not necessarily the next tipset in the chain past the specific tipset that was requested.

So, what we need to do is re-execute the current tipset to get the next so that we have receipts and for execution of the messages we care about, and therefore the logs too. In most cases, it will already have been executed, and there is an execution cache, so it's going to be cheap most of the time. If you look in EthGetBlockByHash it does a similar thing. Trace the call trough newEthBlockFromFilecoinTipSet and you'll see an executeTipset which does what we need - essentially take a given tipset (which we have), execute its messages, get its receipts and then we have what we need to get the events.

Unfortunately it gets trickier from there because we need to go from the raw events to the eth logs. This might take some refactoring to expose the pieces we need and it's also going to be impacted a little by #12421, but I think we can ignore that for now. In EventFilterManager we have a function loadExecutedMessages that can extract the raw events for us from a tipset. Back in eth.go we have ethFilterLogsFromEvents which can convert them to what we need for the output in eth form.

@virajbhartiya
Copy link
Member Author

I have reverted the changes made in CHANGELOG.md and go.sum, as per the discussion on Slack made the necessary changes to avoid using Height()+1. Please check it out.

I'm trying to implement loadExecutedMessages in EthGetBlockReceipts. I got some ideas on how to implement it from your explanation. I'll give it a try. Please help out with it.

@Kubuxu Kubuxu deleted the branch filecoin-project:main September 17, 2024 14:38
@Kubuxu Kubuxu closed this Sep 17, 2024
@aarshkshah1992
Copy link
Contributor

Hey @Kubuxu Why did we close this issue ?

@Kubuxu
Copy link
Contributor

Kubuxu commented Sep 17, 2024

I deleted the main branch, which closed this. The author will reopen.

@rvagg
Copy link
Member

rvagg commented Sep 17, 2024

@virajbhartiya just retarget your PR to master and reopen the PR, we don't use a main branch in lotus but that's what your PR originally targeted.

@rvagg
Copy link
Member

rvagg commented Sep 18, 2024

nevermind, I see we are continuing in #12478

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

Support eth_getBlockReceipts
6 participants