fix: fixed cumulativeGasUsed calculations in eth_getBlockReceipts and eth_getTransactionReceipt (#4921) #4936
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -132,27 +132,50 @@ function populateSyntheticTransactions( | |
| } | ||
|
|
||
| function buildReceiptRootHashes(txHashes: string[], contractResults: any[], logs: Log[]): IReceiptRootHash[] { | ||
| const items: { | ||
| transactionIndex: number; | ||
| logsPerTx: Log[]; | ||
| crPerTx: any[]; | ||
| }[] = []; | ||
|
|
||
| for (const txHash of txHashes) { | ||
| const logsPerTx: Log[] = logs.filter((log) => log.transactionHash === txHash); | ||
| const crPerTx: any[] = contractResults.filter((cr) => cr.hash === txHash); | ||
|
|
||
| // Derive numeric transaction index (for ordering) | ||
| let txIndexNum: number = 0; | ||
| if (crPerTx.length && crPerTx[0].transaction_index != null) { | ||
| txIndexNum = crPerTx[0].transaction_index; | ||
| } else if (logsPerTx.length) { | ||
| txIndexNum = parseInt(logsPerTx[0].transactionIndex, 16); | ||
| } | ||
|
|
||
| items.push({ | ||
| transactionIndex: txIndexNum, | ||
| logsPerTx, | ||
| crPerTx, | ||
| }); | ||
| } | ||
|
|
||
| // Sort by transaction index = block order | ||
| items.sort((a, b) => a.transactionIndex - b.transactionIndex); | ||
|
|
||
| const receipts: IReceiptRootHash[] = []; | ||
| let cumulativeGas = 0; | ||
|
|
||
| for (const item of items) { | ||
| const { transactionIndex, logsPerTx, crPerTx } = item; | ||
|
|
||
| const gasUsed = crPerTx[0]?.gas_used ?? 0; | ||
| cumulativeGas += gasUsed; | ||
| const transactionIndexHex = intToHex(transactionIndex); | ||
|
Contributor
There was a problem hiding this comment. We still might have -1 here...
Contributor
Author
There was a problem hiding this comment. I will use |
||
|
|
||
| receipts.push({ | ||
| transactionIndex: transactionIndexHex, | ||
| type: crPerTx.length && crPerTx[0].type ? intToHex(crPerTx[0].type) : null, | ||
| root: crPerTx.length ? crPerTx[0].root : constants.ZERO_HEX_32_BYTE, | ||
| status: crPerTx.length ? crPerTx[0].status : constants.ONE_HEX, | ||
| cumulativeGasUsed: intToHex(cumulativeGas), | ||
BartoszSolkaBD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| logsBloom: crPerTx.length | ||
| ? crPerTx[0].bloom | ||
| : LogsBloomUtils.buildLogsBloom(logs[0].address, logsPerTx[0].topics), | ||
|
|
@@ -372,7 +395,15 @@ export async function getBlockReceipts( | |
| logsByHash.set(log.transactionHash, existingLogs); | ||
| } | ||
|
|
||
| // 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; | ||
|
Contributor
There was a problem hiding this comment. What should we sort by eventually? In previous method we used: but when it was missing we checked: 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?
Contributor
Author
There was a problem hiding this comment. I think in this case, we should first create all receipts, both regular and synthetic, and then do another loop to calculate cumulativeGasUsed for each. Synthetic receipts have
Contributor
There was a problem hiding this comment. Ok, I get it now, synthetics dont ever influnce this value since they always use 0 gas.In this case I don't think we should take them into consideration here. BUT we need to set this cumulativeGasPrice for the synthetic receipts generated here: as well. Which we aren't doing now, right? |
||
| const bIdx = b.transaction_index ?? Number.MAX_SAFE_INTEGER; | ||
| return aIdx - bIdx; | ||
| }); | ||
|
|
||
| let blockGasUsedBeforeTransaction = 0; | ||
| const receiptPromises = sortedContractResults.map(async (contractResult) => { | ||
| if (Utils.isRejectedDueToHederaSpecificValidation(contractResult)) { | ||
| logger.debug( | ||
| `Transaction with hash %s is skipped due to hedera-specific validation failure (%s)`, | ||
|
|
@@ -394,12 +425,16 @@ export async function getBlockReceipts( | |
| logs: contractResult.logs, | ||
| receiptResponse: contractResult, | ||
| to, | ||
| blockGasUsedBeforeTransaction, | ||
| }; | ||
|
|
||
| const receipt = TransactionReceiptFactory.createRegularReceipt(transactionReceiptParams) as ITransactionReceipt; | ||
| blockGasUsedBeforeTransaction += contractResult.gas_used; | ||
| return receipt; | ||
| }); | ||
|
|
||
| const resolvedReceipts = await Promise.all(receiptPromises); | ||
| receipts.push(...resolvedReceipts.filter((r): r is ITransactionReceipt => r !== null)); | ||
|
|
||
| const regularTxHashes = new Set(contractResults.map((result) => result.hash)); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,13 @@ import { | |
| } from '../../../factories/transactionReceiptFactory'; | ||
| import { Log, Transaction } from '../../../model'; | ||
| import { Precheck } from '../../../precheck'; | ||
| import { | ||
| IContractResultsParams, | ||
| ITransactionReceipt, | ||
| LockAcquisitionResult, | ||
| RequestDetails, | ||
| TypedEvents, | ||
| } from '../../../types'; | ||
| import HAPIService from '../../hapiService/hapiService'; | ||
| import { ICommonService, LockService, TransactionPoolService } from '../../index'; | ||
| import { ITransactionService } from './ITransactionService'; | ||
|
|
@@ -487,12 +493,39 @@ export class TransactionService implements ITransactionService { | |
| this.common.resolveEvmAddress(receiptResponse.to, requestDetails), | ||
| ]); | ||
|
|
||
| let blockGasUsedBeforeTransaction = 0; | ||
| if (receiptResponse.transaction_index > 0) { | ||
| const params: IContractResultsParams = { | ||
| blockNumber: receiptResponse.block_number, | ||
| }; | ||
|
|
||
| const blockContractResults = await this.mirrorNodeClient.getContractResults(requestDetails, params); | ||
|
Comment on lines
+498
to
+502
Contributor
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: 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?
Contributor
Author
There was a problem hiding this comment. https://docs.hedera.com/api-reference/contracts/list-contract-results-from-all-contracts-on-the-network#parameter-transaction-index - there is an option to even query by transaction index
Contributor
There was a problem hiding this comment. Honestly, I didn’t anticipate all of these implications when we were preparing the initial issue. What I do know is that fetching the block can be a very heavy operation (and I think it happens far less frequently than fetching a transaction receipt).
|
||
|
|
||
| if (Array.isArray(blockContractResults)) { | ||
| for (const cr of blockContractResults) { | ||
| if (Utils.isRejectedDueToHederaSpecificValidation(cr)) { | ||
| continue; | ||
| } | ||
|
|
||
| if (cr.transaction_index == null || cr.gas_used == null) { | ||
| continue; | ||
| } | ||
|
|
||
| // Only sum gas for transactions that come before this one in the block | ||
| if (cr.transaction_index < receiptResponse.transaction_index) { | ||
| blockGasUsedBeforeTransaction += cr.gas_used; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return TransactionReceiptFactory.createRegularReceipt({ | ||
| effectiveGas, | ||
| from, | ||
| logs, | ||
| receiptResponse, | ||
| to, | ||
| blockGasUsedBeforeTransaction, | ||
| }); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Don't we really know what is the type of crPerTx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be able to type it. Maybe we can create another issue to fix it everywhere in the codebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it’s not that important, it just crossed my mind while I was checking the PR. We can address it in a separate issue, we already have this problem in the code.