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

fix(eth): apply limit in EthGetBlockReceiptsLimited #12883

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ameeetgaikwad
Copy link

@ameeetgaikwad ameeetgaikwad commented Feb 9, 2025

Related Issues

#12838

Proposed Changes

Additional Info

Checklist

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

github-actions[bot]

This comment was marked as duplicate.

github-actions[bot]

This comment was marked as duplicate.

github-actions[bot]

This comment was marked as duplicate.

@ameeetgaikwad ameeetgaikwad changed the title refactor:prevent fetching receipts for old blocks refactor: prevent fetching receipts for old blocks Feb 9, 2025
@github-actions github-actions bot dismissed their stale review February 9, 2025 09:40

PR title now matches the required format.

@rjan90
Copy link
Contributor

rjan90 commented Feb 10, 2025

OK, that was a bit excessive from the PR title GH-actions workflow, I'll investigate how to make it less spammy and improve its behavior.

@rjan90 rjan90 added the skip/changelog This change does not require CHANGELOG.md update label Feb 10, 2025
@BigLep
Copy link
Member

BigLep commented Feb 10, 2025

@ameeetgaikwad : I don't know the codebase to be able to speak to the feasibility, but did you look at adding a unit test?

@rvagg
Copy link
Member

rvagg commented Feb 11, 2025

@ameeetgaikwad check out the lint error from CI, your editor should be able to pick this one up too if you have it configured correctly.

errLookback: fmt.Errorf("lookbacks of more than %s are disallowed", options.maxLookbackDuration),

This is the error we're kind of aiming to replicate, but it's a slightly different since we're not doing this directly in the gateway. Also, let's use the epoch (ts.Height()) number rather than tipset key because this is an Ethereum API where tipsets make less sense, so the error can tell them that the number is too far back. Let's make it:

"the requested block number %d is older than the allowed limit of %d and cannot be retrieved"

@rvagg
Copy link
Member

rvagg commented Feb 11, 2025

Regarding testing, you should be able to piggy-back an existing itest to run this. Try this:

diff --git a/itests/fevm_test.go b/itests/fevm_test.go
index 7bbab0c64..babae9eb4 100644
--- a/itests/fevm_test.go
+++ b/itests/fevm_test.go
@@ -1185,6 +1185,17 @@ func TestEthGetBlockReceipts(t *testing.T) {
        gethBlockReceipts, err := client.EthGetBlockReceipts(ctx, req)
        require.NoError(t, err)
        require.Len(t, gethBlockReceipts, 3)
+
+       t.Run("EthGetBlockReceiptsLimited", func(t *testing.T) {
+               // just to be sure we're far enough in the chain for the limit to work
+               client.WaitTillChain(ctx, kit.HeightAtLeast(10))
+               // request epoch 2
+               bn := ethtypes.EthUint64(5)
+               // limit to 5 epochs lookback
+               blockReceipts, err := client.EthGetBlockReceiptsLimited(ctx, ethtypes.EthBlockNumberOrHash{BlockNumber: &bn}, 5)
+               require.ErrorContains(t, err, "older than the allowed")
+               require.Nil(t, blockReceipts, "should not return any receipts")
+       })
 }
 
 func deployContractWithEth(ctx context.Context, t *testing.T, client *kit.TestFullNode, ethAddr ethtypes.EthAddress,

Then you can run it with go test ./itests/fevm_test.go -v -run TestEthGetBlockReceipts to see it work 🤞

@ameeetgaikwad
Copy link
Author

@ameeetgaikwad : I don't know the codebase to be able to speak to the feasibility, but did you look at adding a unit test?

yes i tried adding tests for EthGetBlockReceiptsLimited  function. But when I run the tests, it gives this error: EthGetBlockReceiptsLimited is not in the openrpc spec ⁠

@ameeetgaikwad
Copy link
Author

@ameeetgaikwad check out the lint error from CI, your editor should be able to pick this one up too if you have it configured correctly.

errLookback: fmt.Errorf("lookbacks of more than %s are disallowed", options.maxLookbackDuration),

This is the error we're kind of aiming to replicate, but it's a slightly different since we're not doing this directly in the gateway. Also, let's use the epoch (ts.Height()) number rather than tipset key because this is an Ethereum API where tipsets make less sense, so the error can tell them that the number is too far back. Let's make it:

"the requested block number %d is older than the allowed limit of %d and cannot be retrieved"

ok ser! will look into this

@rvagg rvagg changed the title refactor: prevent fetching receipts for old blocks fix(eth): apply limit in EthGetBlockReceiptsLimited Feb 18, 2025
@github-actions github-actions bot dismissed their stale review February 18, 2025 04:32

PR title now matches the required format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does not require CHANGELOG.md update
Projects
Status: ⌨️ In Progress
Development

Successfully merging this pull request may close these issues.

4 participants