op-node: Simplify Receipts Fetching API

[trianglesphere]

Description

The previous api would return a receipts fetcher that the caller was responsible for iterating through and then calling Result on. This moves this inside the Fetch function. This simplifies usage of the API and does not result in performance regression because the receipts hash check requires all receipts to be fetched before

Tests

No added tests, but all pass.

[changeset-bot]

⚠️ No Changeset found

Latest commit: c54de7b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[mergify]

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

[tynes]

Generally seems like a sane change that makes using the L1ReceiptsFetcher easier, can't really think of a good reason to defer fetching the receipts like how the old code was architected

[protolambda]

Do we really want this? Sure it simplifies the API (yay, good thinking), but I expect it makes things quite unreliable when syncing from a L1 source: receipts are not cached individually, and the progress of the fetching is lost, since the receipt fetcher itself isn't cached anymore either.

Note that there's 1 receipt request per L1 tx. And on average Mainnet L1 has 10x as many txs per block as Goerli. And our op-e2e tx throughput is both lower and unrealistically stable (pretty much never a failed receipt retrieval).

Maybe we should do a mainnet test: without deploying any contracts, we can create a ghost rollup, that essentially functions the same like a rollup without active sequencer. Verifiers will run through all L1 blocks and receipts, and hit the bottlenecks that I think are fragile enough to warrant per-request caching/continuation on errors. If that works fine we can merge this, but I highly doubt it (especially when syncing from a rate-limited L1 endpoint, like an average user might...)

[trianglesphere]

Do we really want this?

It's not an easy API to use and doesn't document that it relies on future calls to (receiptsFetcher).Fetch to work after a previous one didn't because Fetch hits a catch

receipts are not cached individually

That's not that hard to do.

[trianglesphere]

@protolambda thanks for pointing out that the receipts fetcher cache is important. I've added a commit to this PR which now caches the receipts fetcher instead of the set of receipts. It attempts to used a cached receipts fetcher instead of making a new one. This means that this new code is the essentially the same as the old code except for the fact that the iteration is done inside of the Fetch call rather than outside.

How do you feel about this change? I think the performance should be the same and the API is a bit easier to use.

[protolambda]

LGTM!

Hopefully the debug_getRawReceipts RPC method becomes more accessible than debug namespace some day, then the receipts fetcher stuff would be easier for us both. (And might still even be worth adding as option for those that sync from their own node now that I think of it)

[mergify]

This PR has been added to the merge queue, and will be merged soon.

[mergify]

This PR is next in line to be merged, and will be merged as soon as checks pass.

[mergify]

Merge failed. Please see automated check logs for more details.

[trianglesphere]

gonna merge by hand b/c mergify messed up the update merge.

[mergify]

Hey @trianglesphere! This PR has merge conflicts. Please fix them before continuing review.

[trianglesphere]

Needed a force update b/c it conflicted with #3647 (b/c there are several interfaces that say the exact same thing).