ethapi: reduce some of the wasted effort in GetTransactionReceipt #32021
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
omerfirmak
requested review from
fjl,
s1na,
lightclient and
rjl493456442
as code owners
omerfirmak
commented
Jun 12, 2025
omerfirmak
force-pushed
the
get-receipt-by-hash
branch
from
a25ac66 to
92ac972
Compare
omerfirmak
force-pushed
the
get-receipt-by-hash
branch
from
92ac972 to
b6270a5
Compare
fjl
requested changes
Jun 13, 2025
omerfirmak
force-pushed
the
get-receipt-by-hash
branch
3 times, most recently
from
e83328c to
5970887
Compare
fjl
reviewed
Jun 16, 2025
fjl
reviewed
Jun 16, 2025
fjl
reviewed
Jun 16, 2025
|
all done @fjl |
|
@rjl493456442, here are the benchmark numbers you asked for |
omerfirmak
commented
Jun 18, 2025
omerfirmak
force-pushed
the
get-receipt-by-hash
branch
2 times, most recently
from
9cab99c to
977bcf4
Compare
|
squashed everything so I can rebase another PR on top of this. |
this is an attempt at reducing wasted effort in GetReceiptByHash by not decoding or deriving unrelated receipts
omerfirmak
force-pushed
the
get-receipt-by-hash
branch
from
977bcf4 to
9a8da8a
Compare
|
Btw, I just realized that our RPC (e.g. eth_getTransactionReceipt) doesn't serve receipt on side chain. It's aligned with the old implementation though, not sure if we should fix it or now. |
| for indx, receipt := range receipts { | ||
| receiptByLookup, err := chain.GetCanonicalReceipt(body.Transactions[indx], receipt.BlockHash, | ||
| receipt.BlockNumber.Uint64(), uint64(indx)) | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
removing this would get rid of the new assert dependency
There was a problem hiding this comment.
It is not an entirely new dependency since it is being used by other packages, so I thought it would be fine. But we could remove if its not something we want.
|
Compared to the latest release, this PR achieves around 1.5x speed up. |
fjl
approved these changes
Jun 24, 2025
33 tasks
howjmay
pushed a commit
to iotaledger/go-ethereum
that referenced
this pull request
Aug 27, 2025
…hereum#32021) Towards ethereum#26974 --------- Co-authored-by: Gary Rong <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Towards #26974
Best reviewed commit by commit.
From my benchmarks, it seems that the dominant cost is still the DB reads. Sender derivation seems to be negligible.
Currently for a single receipt to be served we need to do multiple DB reads. If we are so concerned about the performance of this RPC, we should make the receipts self-contained so they don't need data from multiple queries to be stitched together to make a full receipt. This comes at the expense of slight increase in storage requirements but it might be acceptable given that EIP-4444 is in the works.