fix: fixed cumulativeGasUsed calculations in eth_getBlockReceipts and eth_getTransactionReceipt (#4921)#4936
Conversation
… eth_getTransactionReceipt (hiero-ledger#4921) Signed-off-by: Bartosz Solka <bartosz.solka@blockydevs.com>
jasuwienas
left a comment
jasuwienas
left a comment
There was a problem hiding this comment.
Looks great overall, but I have a few questions.
| for (const item of items) { | ||
| const { transactionIndex, logsPerTx, crPerTx } = item; | ||
|
|
||
| const gasUsed = crPerTx.length && crPerTx[0].gas_used != null ? crPerTx[0].gas_used : 0; |
There was a problem hiding this comment.
| const gasUsed = crPerTx.length && crPerTx[0].gas_used != null ? crPerTx[0].gas_used : 0; | |
| const gasUsed = crPerTx[0]?.gas_used ?? 0; |
|
|
||
| const gasUsed = crPerTx.length && crPerTx[0].gas_used != null ? crPerTx[0].gas_used : 0; | ||
| cumulativeGas += gasUsed; | ||
| const transactionIndexHex = intToHex(transactionIndex); |
There was a problem hiding this comment.
We still might have -1 here...
There was a problem hiding this comment.
I will use 0 as a fallback value. Do you see any risks in doing that?
| const receiptPromises = contractResults.map(async (contractResult) => { | ||
| // Ensure contract results are processed in transaction_index (block) order | ||
| const sortedContractResults = [...contractResults].sort((a, b) => { | ||
| const aIdx = a.transaction_index ?? Number.MAX_SAFE_INTEGER; |
There was a problem hiding this comment.
What should we sort by eventually? In previous method we used:
crPerTx[0].transaction_index
but when it was missing we checked:
logsPerTx[0].transactionIndex
now we are taking only cr, right?
Since we are fetching them all with a single request woulnd't it be posisble to fget them fetched from mirrorndoe right away, instead of sorting them on our own?
Aren;'t they, by any chance already sorted?
| const logsPerTx: Log[] = logs.filter((log) => log.transactionHash === txHash); | ||
| const crPerTx: any[] = contractResults.filter((cr) => cr.hash === txHash); |
There was a problem hiding this comment.
nit: Don't we really know what is the type of crPerTx?
There was a problem hiding this comment.
I think we should be able to type it. Maybe we can create another issue to fix it everywhere in the codebase?
| const params: IContractResultsParams = { | ||
| blockNumber: receiptResponse.block_number, | ||
| }; | ||
|
|
||
| const blockContractResults = await this.mirrorNodeClient.getContractResults(requestDetails, params); |
There was a problem hiding this comment.
OK, I really don’t like the idea of fetching heavy block data for every transaction just to retrieve this single value :/ . But I don't know what to do about that, I really think this should be just calcualted in the mirrornode.
q:
Is there any correlation between the transaction index and the timestamp? Since this is the Hashgraph overall, consensus timestamps should matter for ordering, right? Maybe it applies to the on-block orders and we can fetch only the transactions with a timestamp lower than or equal to the one we’re querying for? (or would there be a problem with internal transactions for examlple?)
Also, we’re ignoring the fallback tx index from logs and only using the one from contract results. Wont that create differences for a single transaction when comparing it across these different endpoints?
…iero-ledger#4921) Signed-off-by: Bartosz Solka <bartosz.solka@blockydevs.com>
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (71.59%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.
@@ Coverage Diff @@
## main #4936 +/- ##
===========================================
- Coverage 95.94% 70.34% -25.60%
===========================================
Files 143 143
Lines 23701 23839 +138
Branches 1877 632 -1245
===========================================
- Hits 22740 16770 -5970
- Misses 937 7052 +6115
+ Partials 24 17 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 85 files with indirect coverage changes 🚀 New features to boost your workflow:
|
2 participants
Description
This PR fixes an issue where cumulativeGasUsed in transaction receipts was incorrectly set to the block’s total gas used (block_gas_used) for every transaction in the block.
It updates the receipt generation logic so that cumulativeGasUsed reflects the cumulative gas consumed in the block up to and including each transaction, matching the Ethereum Yellow Paper semantics.
This change has been implemented for both eth_getBlockReceipts and eth_getTransactionReceipt.
It also updates the receipts root hash computation (and its expected test value) to use the corrected cumulative gas and adds targeted tests to validate multi‑transaction block behavior and OpenRPC contract for these endpoints.
Related issue(s)
Fixes #4921
Testing Guide
Changes from original design (optional)
N/A
Additional work needed (optional)
N/A
Checklist