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

EthGetBlockReceiptsLimited does not use limit #12838

Open
5 of 11 tasks
elmattic opened this issue Jan 21, 2025 · 8 comments
Open
5 of 11 tasks

EthGetBlockReceiptsLimited does not use limit #12838

elmattic opened this issue Jan 21, 2025 · 8 comments
Assignees
Labels

Comments

@elmattic
Copy link

Checklist

  • This is not a security-related bug/issue. If it is, please follow please follow the security policy.
  • I have searched on the issue tracker and the lotus forum, and there is no existing related issue or discussion.
  • I am running the Latest release, the most recent RC(release canadiate) for the upcoming release or the dev branch(master), or have an issue updating to any of these.
  • I did not make any code changes to lotus.

Lotus component

  • lotus daemon - chain sync
  • lotus fvm/fevm - Lotus FVM and FEVM interactions
  • lotus miner/worker - sealing
  • lotus miner - proving(WindowPoSt/WinningPoSt)
  • lotus JSON-RPC API
  • lotus message management (mpool)
  • Other

Lotus Version

./lotus version
Daemon:  1.31.0-rc1+calibnet+git.6cd798259.dirty+api1.5.0
Local: lotus version 1.31.0-rc1+calibnet+git.6cd798259.dirty

Repro Steps

  1. Run '...'
  2. Do '...'
  3. See error '...'
    ...

Describe the Bug

https://github.com/filecoin-project/lotus/blob/master/node/impl/eth/transaction.go#L448

Upon reviewing this function's implementation, I noticed that the limit parameter is not being used anywhere.

This is not necessarily a problem, but rather a potential question about what the specification should be.

Logging Information

n/a
@elmattic elmattic added the kind/bug Kind: Bug label Jan 21, 2025
@github-project-automation github-project-automation bot moved this to 📌 Triage in FilOz Jan 21, 2025
@rvagg rvagg moved this from 📌 Triage to 🐱 Todo in FilOz Jan 22, 2025
@rvagg rvagg self-assigned this Jan 22, 2025
@rvagg
Copy link
Member

rvagg commented Jan 22, 2025

Whoops! Thanks for picking this up @elmattic; we'll get this done.

@BigLep
Copy link
Member

BigLep commented Jan 23, 2025

Agreed this should get fixed. We expect to get this in 2025Q1, but aren't prioritizing it yet until lad things for nv25. We assume that if any RPC operator was impacted by this they could block the calling client or disable the API if necessary while we work on a fix. (Certainly if this becomes a fire for a team, we'll prioritize getting it done quicker.)

@rvagg
Copy link
Member

rvagg commented Jan 23, 2025

I'm going to mark this as good-first-issue because it really should be simple, it'd basically be something like this:

	if limit > api.LookbackNoLimit && ts.Height() < e.chainStore.GetHeaviestTipSet().Height()-abi.ChainEpoch(limit) {
		return nil, xerrors.Errorf("tipset %s is too old to fetch receipts for", ts.Key())
	}

along with at least one test.

@rvagg rvagg added the good first issue Good for newcomers label Jan 23, 2025
@rvagg rvagg removed their assignment Jan 23, 2025
@rvagg
Copy link
Member

rvagg commented Jan 23, 2025

(unassigning myself to leave the door open but in reality I'll probably end up doing it)

@ameeetgaikwad
Copy link

would like to take this up @rvagg

@BigLep
Copy link
Member

BigLep commented Jan 27, 2025

Go for it @ameeetgaikwad. Assigned!

@BigLep
Copy link
Member

BigLep commented Feb 3, 2025

@ameeetgaikwad : just checking in on how this is going.

@ameeetgaikwad
Copy link

@BigLep yes! Pushing my EOD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🐱 Todo
Development

No branches or pull requests

4 participants