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.

Sign up for GitHub

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

fix: enhanced reliability of eth RPC methods with null checks and retry mechanisms #3349

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix: enhanced reliability of eth RPC methods with null checks and retry mechanisms #3349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
152b18c
fix: fixed flaky precheck test
quiet-node Dec 22, 2024
bc26b45
fix: handle log null entities more gracefully in getBlock()
quiet-node Dec 20, 2024
548cdeb
fix: added null check to root hash builder
quiet-node Dec 20, 2024
d97250d
fix: modified getContractResultWithRetry() to accept more general get…
quiet-node Dec 22, 2024
ae23d06
fix: reused getContractResultWithRetry for getContractResults
quiet-node Dec 22, 2024
a99288f
fix: added getContractResultsLogsWithRetry()
quiet-node Dec 23, 2024
a410b15
fix: reverted licenses
quiet-node Dec 23, 2024
8687322
fix: checked empty blockHash for getHistoricalBlockResponse()
quiet-node Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 85 additions & 18 deletions packages/relay/src/lib/clients/mirrorNodeClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* -
/*-
*
* Hedera JSON RPC Relay
*
Expand Down Expand Up @@ -620,7 +620,10 @@
requestDetails,
);

await this.cacheService.set(cachedLabel, block, MirrorNodeClient.GET_BLOCK_ENDPOINT, requestDetails);
if (block) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if I understand correctly, but the errors described in the issue were because of caching null blocks?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an issue I noticed and an attempt securing the code. While it may or may not be directly related to the problem, we should ensure the block is not null before storing it in the cache. Otherwise, we risk repeatedly retrieving null from the cache, which likely has a TTL.

await this.cacheService.set(cachedLabel, block, MirrorNodeClient.GET_BLOCK_ENDPOINT, requestDetails);
}

return block;
}

Expand Down Expand Up @@ -754,21 +757,42 @@
* In some very rare cases the /contracts/results api is called before all the data is saved in
* the mirror node DB and `transaction_index` or `block_number` is returned as `undefined` or `block_hash` as `0x`.
* A single re-fetch is sufficient to resolve this problem.
* @param {string} transactionIdOrHash - The transaction ID or hash
* @param {RequestDetails} requestDetails - The request details for logging and tracking.
*
* @param {string} methodName - The name of the method used to fetch contract results.
* @param {any[]} args - The arguments to be passed to the specified method for fetching contract results.
* @param {RequestDetails} requestDetails - Details used for logging and tracking the request.
* @returns {Promise<any>} - A promise resolving to the fetched contract result, either on the first attempt or after a retry.
*/
public async getContractResultWithRetry(transactionIdOrHash: string, requestDetails: RequestDetails) {
const contractResult = await this.getContractResult(transactionIdOrHash, requestDetails);
if (
contractResult &&
!(
contractResult.transaction_index &&
contractResult.block_number &&
contractResult.block_hash != EthImpl.emptyHex
)
) {
return this.getContractResult(transactionIdOrHash, requestDetails);
public async getContractResultWithRetry(
methodName: string,
args: any[],
requestDetails: RequestDetails,
): Promise<any> {
const shortDelay = 500;
const contractResult = await this[methodName](...args);

if (contractResult) {
const contractObjects = Array.isArray(contractResult) ? contractResult : [contractResult];
for (const contractObject of contractObjects) {
if (
contractObject &&
(contractObject.transaction_index == null ||
contractObject.block_number == null ||
contractObject.block_hash == EthImpl.emptyHex)
) {
if (this.logger.isLevelEnabled('debug')) {
this.logger.debug(

Check warning on line 784 in packages/relay/src/lib/clients/mirrorNodeClient.ts

View check run for this annotation

Codecov / codecov/patch

packages/relay/src/lib/clients/mirrorNodeClient.ts#L784 

Added line #L784 was not covered by tests
`${requestDetails.formattedRequestId} Contract result contains undefined transaction_index, block_number, or block_hash is an empty hex (0x): transaction_hash:${contractObject.hash}, transaction_index:${contractObject.transaction_index}, block_number=${contractObject.block_number}, block_hash=${contractObject.block_hash}. Retrying after a delay of ${shortDelay} ms `,
);
}

// Backoff before repeating request
await new Promise((r) => setTimeout(r, shortDelay));
return await this[methodName](...args);
}
}
}

return contractResult;
}

Expand Down Expand Up @@ -870,14 +894,25 @@
return this.getQueryParams(queryParamObject);
}

public async getContractResultsLogs(
/**
* In some very rare cases the /contracts/results/logs api is called before all the data is saved in
* the mirror node DB and `transaction_index`, `block_number`, `index` is returned as `undefined`, or block_hash is an empty hex (0x).
* A single re-fetch is sufficient to resolve this problem.
*
* @param {RequestDetails} requestDetails - Details used for logging and tracking the request.
* @param {IContractLogsResultsParams} [contractLogsResultsParams] - Parameters for querying contract logs results.
* @param {ILimitOrderParams} [limitOrderParams] - Parameters for limit and order when fetching the logs.
* @returns {Promise<any[]>} - A promise resolving to the paginated contract logs results.
*/
public async getContractResultsLogsWithRetry(
requestDetails: RequestDetails,
contractLogsResultsParams?: IContractLogsResultsParams,
limitOrderParams?: ILimitOrderParams,
) {
): Promise<any[]> {
const shortDelay = 500;
const queryParams = this.prepareLogsParams(contractLogsResultsParams, limitOrderParams);

return this.getPaginatedResults(
const logResults = await this.getPaginatedResults(
`${MirrorNodeClient.GET_CONTRACT_RESULT_LOGS_ENDPOINT}${queryParams}`,
MirrorNodeClient.GET_CONTRACT_RESULT_LOGS_ENDPOINT,
MirrorNodeClient.CONTRACT_RESULT_LOGS_PROPERTY,
Expand All @@ -886,6 +921,38 @@
1,
MirrorNodeClient.mirrorNodeContractResultsLogsPageMax,
);

if (logResults) {
for (const log of logResults) {
if (
log &&
(log.transaction_index == null ||
log.block_number == null ||
log.index == null ||
log.block_hash === EthImpl.emptyHex)
) {
if (this.logger.isLevelEnabled('debug')) {
this.logger.debug(

Check warning on line 935 in packages/relay/src/lib/clients/mirrorNodeClient.ts

View check run for this annotation

Codecov / codecov/patch

packages/relay/src/lib/clients/mirrorNodeClient.ts#L935 

Added line #L935 was not covered by tests
`${requestDetails.formattedRequestId} Contract result log contains undefined transaction_index, block_number, index, or block_hash is an empty hex (0x): transaction_hash:${log.transaction_hash}, transaction_index:${log.transaction_index}, block_number=${log.block_number}, log_index=${log.index}, block_hash=${log.block_hash}. Retrying after a delay of ${shortDelay} ms.`,
);
}

// Backoff before repeating request
await new Promise((r) => setTimeout(r, shortDelay));
return await this.getPaginatedResults(
`${MirrorNodeClient.GET_CONTRACT_RESULT_LOGS_ENDPOINT}${queryParams}`,
MirrorNodeClient.GET_CONTRACT_RESULT_LOGS_ENDPOINT,
MirrorNodeClient.CONTRACT_RESULT_LOGS_PROPERTY,
requestDetails,
[],
1,
MirrorNodeClient.mirrorNodeContractResultsLogsPageMax,
);
}
}
}

return logResults;
}

public async getContractResultsLogsByAddress(
Expand Down
5 changes: 5 additions & 0 deletions packages/relay/src/lib/clients/sdkClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,6 +724,11 @@ export class SDKClient {
);
return transactionResponse;
} catch (e: any) {
this.logger.warn(
e,
`${requestDetails.formattedRequestId} Transaction failed while executing transaction via the SDK: transactionId=${transaction.transactionId}, callerName=${callerName}, txConstructorName=${txConstructorName}`,
);

Comment on lines +727 to +731
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for Rereviewers:
This is an effort to enhance debugging.

if (e instanceof JsonRpcError) {
throw e;
}
Expand Down
38 changes: 26 additions & 12 deletions packages/relay/src/lib/eth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* -
/*-
*
* Hedera JSON RPC Relay
*
Expand Down Expand Up @@ -1922,13 +1922,17 @@ export class EthImpl implements Eth {
transactionIndex: string,
requestDetails: RequestDetails,
): Promise<Transaction | null> {
const contractResults = await this.mirrorNodeClient.getContractResults(
const contractResults = await this.mirrorNodeClient.getContractResultWithRetry(
this.mirrorNodeClient.getContractResults.name,
[
requestDetails,
{
[blockParam.title]: blockParam.value,
transactionIndex: Number(transactionIndex),
},
undefined,
],
requestDetails,
{
[blockParam.title]: blockParam.value,
transactionIndex: Number(transactionIndex),
},
undefined,
);

if (!contractResults[0]) return null;
Expand Down Expand Up @@ -2201,7 +2205,11 @@ export class EthImpl implements Eth {
this.logger.trace(`${requestIdPrefix} getTransactionByHash(hash=${hash})`, hash);
}

const contractResult = await this.mirrorNodeClient.getContractResultWithRetry(hash, requestDetails);
const contractResult = await this.mirrorNodeClient.getContractResultWithRetry(
this.mirrorNodeClient.getContractResult.name,
[hash, requestDetails],
requestDetails,
);
if (contractResult === null || contractResult.hash === undefined) {
// handle synthetic transactions
const syntheticLogs = await this.common.getLogsWithParams(
Expand Down Expand Up @@ -2265,7 +2273,12 @@ export class EthImpl implements Eth {
return cachedResponse;
}

const receiptResponse = await this.mirrorNodeClient.getContractResultWithRetry(hash, requestDetails);
const receiptResponse = await this.mirrorNodeClient.getContractResultWithRetry(
this.mirrorNodeClient.getContractResult.name,
[hash, requestDetails],
requestDetails,
);

if (receiptResponse === null || receiptResponse.hash === undefined) {
// handle synthetic transactions
const syntheticLogs = await this.common.getLogsWithParams(
Expand Down Expand Up @@ -2531,10 +2544,11 @@ export class EthImpl implements Eth {
if (blockResponse == null) return null;
const timestampRange = blockResponse.timestamp;
const timestampRangeParams = [`gte:${timestampRange.from}`, `lte:${timestampRange.to}`];
const contractResults = await this.mirrorNodeClient.getContractResults(

const contractResults = await this.mirrorNodeClient.getContractResultWithRetry(
this.mirrorNodeClient.getContractResults.name,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we just being cautious or does this case actually need to have retry support?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contractResults here was a contributing factor to the 500 errors. In the rare cases mentioned, contractResults contains null fields. When passed to buildReceiptRootHashes, the function does not handle these null values correctly, resulting in 500 errors. Therefore, this scenario requires a retry if such a rare case occurs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these cases occur only on the mainnet and at the time of the transaction, I plan to release these updates that include additional debugging information for improved analysis. If the issues persist, it will be easier to pinpoint the root causes and escalate them to the mainnet team more effectively.

[requestDetails, { timestamp: timestampRangeParams }, undefined],
requestDetails,
{ timestamp: timestampRangeParams },
undefined,
);
const gasUsed = blockResponse.gas_used;
const params = { timestamp: timestampRangeParams };
Expand Down
19 changes: 12 additions & 7 deletions packages/relay/src/lib/services/debugService/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,19 @@
*
*/

import { ConfigService } from '@hashgraph/json-rpc-config-service/dist/services';
import type { Logger } from 'pino';
import type { MirrorNodeClient } from '../../clients';
import type { IDebugService } from './IDebugService';
import type { CommonService } from '../ethService';

import { decodeErrorMessage, mapKeysAndValues, numberTo0x, strip0x } from '../../../formatters';
import type { MirrorNodeClient } from '../../clients';
import { IOpcode } from '../../clients/models/IOpcode';
import { IOpcodesResponse } from '../../clients/models/IOpcodesResponse';
import constants, { CallType, TracerType } from '../../constants';
import { predefined } from '../../errors/JsonRpcError';
import { EthImpl } from '../../eth';
import { IOpcodesResponse } from '../../clients/models/IOpcodesResponse';
import { IOpcode } from '../../clients/models/IOpcode';
import { ICallTracerConfig, IOpcodeLoggerConfig, ITracerConfig, RequestDetails } from '../../types';
import { ConfigService } from '@hashgraph/json-rpc-config-service/dist/services';
import type { CommonService } from '../ethService';
import type { IDebugService } from './IDebugService';

/**
* Represents a DebugService for tracing and debugging transactions and debugging
Expand Down Expand Up @@ -300,7 +301,11 @@ export class DebugService implements IDebugService {
try {
const [actionsResponse, transactionsResponse] = await Promise.all([
this.mirrorNodeClient.getContractsResultsActions(transactionHash, requestDetails),
this.mirrorNodeClient.getContractResultWithRetry(transactionHash, requestDetails),
this.mirrorNodeClient.getContractResultWithRetry(
this.mirrorNodeClient.getContractResult.name,
[transactionHash, requestDetails],
requestDetails,
),
]);
if (!actionsResponse || !transactionsResponse) {
throw predefined.RESOURCE_NOT_FOUND(`Failed to retrieve contract results for transaction ${transactionHash}`);
Expand Down
36 changes: 26 additions & 10 deletions packages/relay/src/lib/services/ethService/ethCommonService/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,21 @@
*
*/

import constants from '../../../constants';
import { JsonRpcError, predefined } from '../../../errors/JsonRpcError';
import { ICommonService } from './ICommonService';
import { ConfigService } from '@hashgraph/json-rpc-config-service/dist/services';
import * as _ from 'lodash';
import { Logger } from 'pino';
import { MirrorNodeClient } from '../../../clients';

import { nullableNumberTo0x, numberTo0x, parseNumericEnvVar, toHash32 } from '../../../../formatters';
import { SDKClientError } from '../../../errors/SDKClientError';
import { MirrorNodeClient } from '../../../clients';
import constants from '../../../constants';
import { JsonRpcError, predefined } from '../../../errors/JsonRpcError';
import { MirrorNodeClientError } from '../../../errors/MirrorNodeClientError';
import { SDKClientError } from '../../../errors/SDKClientError';
import { EthImpl } from '../../../eth';
import { Log } from '../../../model';
import * as _ from 'lodash';
import { CacheService } from '../../cacheService/cacheService';
import { ConfigService } from '@hashgraph/json-rpc-config-service/dist/services';
import { RequestDetails } from '../../../types';
import { CacheService } from '../../cacheService/cacheService';
import { ICommonService } from './ICommonService';

/**
* Create a new Common Service implementation.
Expand Down Expand Up @@ -175,6 +177,20 @@
returnLatest?: boolean,
): Promise<any> {
if (!returnLatest && this.blockTagIsLatestOrPending(blockNumberOrTagOrHash)) {
if (this.logger.isLevelEnabled('debug')) {
this.logger.debug(
`${requestDetails.formattedRequestId} Detected a contradiction between blockNumberOrTagOrHash and returnLatest. The request does not target the latest block, yet blockNumberOrTagOrHash representing latest or pending: returnLatest=${returnLatest}, blockNumberOrTagOrHash=${blockNumberOrTagOrHash}`,
);
}
return null;
}

if (blockNumberOrTagOrHash === EthImpl.emptyHex) {
if (this.logger.isLevelEnabled('debug')) {
this.logger.debug(

Check warning on line 190 in packages/relay/src/lib/services/ethService/ethCommonService/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/relay/src/lib/services/ethService/ethCommonService/index.ts#L190 

Added line #L190 was not covered by tests
`${requestDetails.formattedRequestId} Invalid input detected in getHistoricalBlockResponse(): blockNumberOrTagOrHash=${blockNumberOrTagOrHash}.`,
);
}
return null;
}

Expand Down Expand Up @@ -321,7 +337,7 @@
if (address) {
logResults = await this.getLogsByAddress(address, params, requestDetails);
} else {
logResults = await this.mirrorNodeClient.getContractResultsLogs(requestDetails, params);
logResults = await this.mirrorNodeClient.getContractResultsLogsWithRetry(requestDetails, params);
}

if (!logResults) {
Expand All @@ -334,7 +350,7 @@
new Log({
address: log.address,
blockHash: toHash32(log.block_hash),
blockNumber: numberTo0x(log.block_number),
blockNumber: nullableNumberTo0x(log.block_number),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Why would a block number be null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is one of the strange cases where block_number, transaction_index, or blockHash are falsy, leading to 500 errors. The retry mechanism should address the issue. Should I revert this change and throw a more informative error message even if those fields remain falsy after the retry? Hmm maybe that would be better so the clients can resubmit the requests themselves other than having null in the response. Wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, if a retry is ensured to resolve the issue then maybe we can manage one iteration in the short term.
We should open up an issue in the mirror node repo to investigate this issue so this can be resolved.

data: log.data,
logIndex: nullableNumberTo0x(log.index),
removed: false,
Expand Down
22 changes: 18 additions & 4 deletions packages/relay/src/receiptsRootUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@
import { RLP } from '@ethereumjs/rlp';
import { Trie } from '@ethereumjs/trie';
import { bytesToInt, concatBytes, hexToBytes, intToBytes, intToHex } from '@ethereumjs/util';
import { EthImpl } from './lib/eth';

import { prepend0x } from './formatters';
import { EthImpl } from './lib/eth';
import { Log } from './lib/model';
import { LogsBloomUtils } from './logsBloomUtils';

Expand Down Expand Up @@ -93,16 +94,29 @@ export class ReceiptsRootUtils {
public static buildReceiptRootHashes(txHashes: string[], contractResults: any[], logs: Log[]): IReceiptRootHash[] {
const receipts: IReceiptRootHash[] = [];

for (let i in txHashes) {
for (const i in txHashes) {
const txHash: string = txHashes[i];
const logsPerTx: Log[] = logs.filter((log) => log.transactionHash == txHash);
const crPerTx: any[] = contractResults.filter((cr) => cr.hash == txHash);

// Determine the transaction index for the current transaction hash:
// - Prefer the `transaction_index` from the contract results (`crPerTx`) if available.
// - Fallback to the `transactionIndex` from logs (`logsPerTx`) if no valid `transaction_index` is found in `crPerTx`.
// - If neither source provides a valid value, `transactionIndex` remains `null`.
let transactionIndex: any = null;
quiet-node marked this conversation as resolved.
Show resolved Hide resolved
if (crPerTx.length && crPerTx[0].transaction_index != null) {
transactionIndex = intToHex(crPerTx[0].transaction_index);
} else if (logsPerTx.length) {
transactionIndex = logsPerTx[0].transactionIndex;
}

receipts.push({
transactionIndex: crPerTx.length ? intToHex(crPerTx[0].transaction_index) : logsPerTx[0].transactionIndex,
transactionIndex,
type: crPerTx.length && crPerTx[0].type ? intToHex(crPerTx[0].type) : null,
root: crPerTx.length ? crPerTx[0].root : EthImpl.zeroHex32Byte,
status: crPerTx.length ? crPerTx[0].status : EthImpl.oneHex,
cumulativeGasUsed: crPerTx.length ? intToHex(crPerTx[0].block_gas_used) : EthImpl.zeroHex,
cumulativeGasUsed:
crPerTx.length && crPerTx[0].block_gas_used ? intToHex(crPerTx[0].block_gas_used) : EthImpl.zeroHex,
logsBloom: crPerTx.length
? crPerTx[0].bloom
: LogsBloomUtils.buildLogsBloom(logs[0].address, logsPerTx[0].topics),
Expand Down
Loading
Loading