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

Jump to bottom

fix: Inconsistent handling of failed EVM transactions #3158

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+142 −8
Open

fix: Inconsistent handling of failed EVM transactions #3158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 17 additions & 5 deletions packages/relay/src/lib/eth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,15 @@ import {
formatContractResult,
formatTransactionIdWithoutQueryParams,
getFunctionSelector,
hexToASCII,
isHex,
isValidEthereumAddress,
nanOrNumberTo0x,
nullableNumberTo0x,
numberTo0x,
parseNumericEnvVar,
prepend0x,
strip0x,
toHash32,
trimPrecedingZeros,
weibarHexToTinyBarInt,
Expand Down Expand Up @@ -326,10 +328,9 @@ export class EthImpl implements Eth {
requestDetails: RequestDetails,
): Promise<IFeeHistory | JsonRpcError> {
const requestIdPrefix = requestDetails.formattedRequestId;
const maxResults =
ConfigService.get('TEST')
? constants.DEFAULT_FEE_HISTORY_MAX_RESULTS
: Number(ConfigService.get('FEE_HISTORY_MAX_RESULTS'));
const maxResults = ConfigService.get('TEST')
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: for a different PR this feels like something the ConfigService should handle internally.
Is that possible?

? constants.DEFAULT_FEE_HISTORY_MAX_RESULTS
: Number(ConfigService.get('FEE_HISTORY_MAX_RESULTS'));

this.logger.trace(
`${requestIdPrefix} feeHistory(blockCount=${blockCount}, newestBlock=${newestBlock}, rewardPercentiles=${rewardPercentiles})`,
Expand Down Expand Up @@ -2297,9 +2298,20 @@ export class EthImpl implements Eth {
throw predefined.MAX_BLOCK_SIZE(blockResponse.count);
}

const isWrongNonce = (contractResult: { result: string; error_message: any }) => {
return (
contractResult.result === constants.TRANSACTION_RESULT_STATUS.WRONG_NONCE ||
hexToASCII(strip0x(contractResult.error_message ?? '')) === constants.TRANSACTION_RESULT_STATUS.WRONG_NONCE
);
};

// prepare transactionArray
let transactionArray: any[] = [];
for (const contractResult of contractResults) {
if (isWrongNonce(contractResult)) {
// skip wrong nonce transactions as they are not valid transactions which should be included in the block
continue;
}
contractResult.from = await this.resolveEvmAddress(contractResult.from, requestDetails, [constants.TYPE_ACCOUNT]);
contractResult.to = await this.resolveEvmAddress(contractResult.to, requestDetails);
contractResult.chain_id = contractResult.chain_id || this.chain;
Expand All @@ -2308,7 +2320,7 @@ export class EthImpl implements Eth {
}

transactionArray = this.populateSyntheticTransactions(showDetails, logs, transactionArray, requestDetails);
transactionArray = showDetails ? _.uniqBy(transactionArray, 'hash') : _.uniq(transactionArray);
transactionArray = showDetails ? transactionArray : _.uniq(transactionArray);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure showDetails should change the length of the array, it concerns only with returning either transaction hashes or the transaction objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we never want to show duplicated transaction hashes in the response when showDetails is false, so that's why I left this one there just in case.

Filtering the WRONG_NONCE transactions should be sufficient to remove duplicate hashes, but I am not sure if this is the only pre-check where this scenario could happen, that's why I left it just in case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I agree with Luis here. The size should be the same regardless of showDetails.
I'm not sure why this logic exists, shouldn't they always be unique transactions based on hash?
Maybe something to follow up on, why should we ever have duplicates when showDetails is true?


const formattedReceipts: IReceiptRootHash[] = ReceiptsRootUtils.buildReceiptRootHashes(
transactionArray.map((tx) => (showDetails ? tx.hash : tx)),
Expand Down
62 changes: 61 additions & 1 deletion packages/relay/tests/lib/eth/eth_getBlockByHash.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
} from '../../helpers';
import { SDKClient } from '../../../src/lib/clients';
import RelayAssertions from '../../assertions';
import { numberTo0x } from '../../../dist/formatters';
import { ASCIIToHex, numberTo0x, prepend0x } from '../../../dist/formatters';
import {
ACCOUNT_WITHOUT_TRANSACTIONS,
BLOCK_HASH,
Expand Down Expand Up @@ -334,4 +334,64 @@ describe('@ethGetBlockByHash using MirrorNode', async function () {
args,
);
});

it('eth_getBlockByHash should skip transactions with wrong nonces when showDetails = false', async function () {
// mirror node request mocks
restMock.onGet(`blocks/${BLOCK_HASH}`).reply(200, DEFAULT_BLOCK);
restMock.onGet(CONTRACT_RESULTS_WITH_FILTER_URL).reply(200, {
results: [
...defaultContractResults.results,
{ ...defaultContractResults.results[0], result: 'WRONG_NONCE' },
{ ...defaultContractResults.results[0], error_message: prepend0x(ASCIIToHex('WRONG_NONCE')) },
],
});
restMock.onGet(CONTRACT_RESULTS_LOGS_WITH_FILTER_URL).reply(200, DEFAULT_ETH_GET_BLOCK_BY_LOGS);

const showDetails = false;
const result = await ethImpl.getBlockByHash(BLOCK_HASH, showDetails, requestDetails);

RelayAssertions.assertBlock(
result,
{
hash: BLOCK_HASH_TRIMMED,
gasUsed: TOTAL_GAS_USED,
number: BLOCK_NUMBER_HEX,
parentHash: BLOCK_HASH_PREV_TRIMMED,
timestamp: BLOCK_TIMESTAMP_HEX,
transactions: [CONTRACT_HASH_1, CONTRACT_HASH_2], // should not include the transaction with wrong nonce
receiptsRoot: DEFAULT_BLOCK_RECEIPTS_ROOT_HASH,
},
showDetails,
);
});

it('eth_getBlockByHash should skip transactions with wrong nonces when showDetails = true', async function () {
// mirror node request mocks
restMock.onGet(`blocks/${BLOCK_HASH}`).reply(200, DEFAULT_BLOCK);
restMock.onGet(CONTRACT_RESULTS_WITH_FILTER_URL).reply(200, {
results: [
...defaultContractResults.results,
{ ...defaultContractResults.results[1], result: 'WRONG_NONCE' },
{ ...defaultContractResults.results[1], error_message: prepend0x(ASCIIToHex('WRONG_NONCE')) },
],
});
restMock.onGet(CONTRACT_RESULTS_LOGS_WITH_FILTER_URL).reply(200, DEFAULT_ETH_GET_BLOCK_BY_LOGS);

const showDetails = true;
const result = await ethImpl.getBlockByHash(BLOCK_HASH, showDetails, requestDetails);

RelayAssertions.assertBlock(
result,
{
hash: BLOCK_HASH_TRIMMED,
gasUsed: TOTAL_GAS_USED,
number: BLOCK_NUMBER_HEX,
parentHash: BLOCK_HASH_PREV_TRIMMED,
timestamp: BLOCK_TIMESTAMP_HEX,
transactions: [CONTRACT_HASH_1, CONTRACT_HASH_2], // should not include the transaction with wrong nonce
receiptsRoot: DEFAULT_BLOCK_RECEIPTS_ROOT_HASH,
},
showDetails,
);
});
});
66 changes: 64 additions & 2 deletions packages/relay/tests/lib/eth/eth_getBlockByNumber.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { expect, use } from 'chai';
import sinon from 'sinon';
import chaiAsPromised from 'chai-as-promised';
import { Logger } from 'pino';
import { predefined } from '../../../src/lib/errors/JsonRpcError';
import { predefined } from '../../../src';
import { EthImpl } from '../../../src/lib/eth';
import {
blockLogsBloom,
Expand All @@ -34,7 +34,7 @@ import { Block, Transaction } from '../../../src/lib/model';
import { MirrorNodeClient, SDKClient } from '../../../src/lib/clients';
import RelayAssertions from '../../assertions';
import constants from '../../../src/lib/constants';
import { hashNumber, numberTo0x } from '../../../dist/formatters';
import { ASCIIToHex, hashNumber, numberTo0x, prepend0x } from '../../../dist/formatters';
import {
BLOCK_HASH,
BLOCK_HASH_PREV_TRIMMED,
Expand Down Expand Up @@ -566,4 +566,66 @@ describe('@ethGetBlockByNumber using MirrorNode', async function () {
args,
);
});

it('eth_getBlockByNumber should skip transactions with wrong nonces when showDetails = false', async function () {
Copy link
Contributor

@acuarica acuarica Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Aren't these two test cases basically the same module showDetails=true|false? If that's the case, maybe it's worth to merge them into a single test case, something like

[false, true].forEach(showDetails => {
  it(`eth_getBlockByNumber should skip transactions with wrong nonces when showDetails = ${showDetails}`, async function () {
  });
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, that would simplify things a bit

// mirror node request mocks
restMock.onGet(`blocks/${BLOCK_NUMBER}`).reply(200, DEFAULT_BLOCK);
restMock.onGet(BLOCKS_LIMIT_ORDER_URL).reply(200, MOST_RECENT_BLOCK);
restMock.onGet(CONTRACT_RESULTS_WITH_FILTER_URL).reply(200, {
results: [
...defaultContractResults.results,
{ ...defaultContractResults.results[0], result: 'WRONG_NONCE' },
{ ...defaultContractResults.results[0], error_message: prepend0x(ASCIIToHex('WRONG_NONCE')) },
],
});
restMock.onGet(CONTRACT_RESULTS_LOGS_WITH_FILTER_URL).reply(200, DEFAULT_ETH_GET_BLOCK_BY_LOGS);

const showDetails = false;
const result = await ethImpl.getBlockByNumber(numberTo0x(BLOCK_NUMBER), showDetails, requestDetails);

RelayAssertions.assertBlock(
result,
{
hash: BLOCK_HASH_TRIMMED,
gasUsed: TOTAL_GAS_USED,
number: BLOCK_NUMBER_HEX,
parentHash: BLOCK_HASH_PREV_TRIMMED,
timestamp: BLOCK_TIMESTAMP_HEX,
transactions: [CONTRACT_HASH_1, CONTRACT_HASH_2], // should not include the transaction with wrong nonce
receiptsRoot: DEFAULT_BLOCK_RECEIPTS_ROOT_HASH,
},
showDetails,
);
});

it('eth_getBlockByNumber should skip transactions with wrong nonces when showDetails = true', async function () {
// mirror node request mocks
restMock.onGet(`blocks/${BLOCK_NUMBER}`).reply(200, DEFAULT_BLOCK);
restMock.onGet(BLOCKS_LIMIT_ORDER_URL).reply(200, MOST_RECENT_BLOCK);
restMock.onGet(CONTRACT_RESULTS_WITH_FILTER_URL).reply(200, {
results: [
...defaultContractResults.results,
{ ...defaultContractResults.results[1], result: 'WRONG_NONCE' },
{ ...defaultContractResults.results[1], error_message: prepend0x(ASCIIToHex('WRONG_NONCE')) },
],
});
restMock.onGet(CONTRACT_RESULTS_LOGS_WITH_FILTER_URL).reply(200, DEFAULT_ETH_GET_BLOCK_BY_LOGS);

const showDetails = true;
const result = await ethImpl.getBlockByHash(numberTo0x(BLOCK_NUMBER), showDetails, requestDetails);

RelayAssertions.assertBlock(
result,
{
hash: BLOCK_HASH_TRIMMED,
gasUsed: TOTAL_GAS_USED,
number: BLOCK_NUMBER_HEX,
parentHash: BLOCK_HASH_PREV_TRIMMED,
timestamp: BLOCK_TIMESTAMP_HEX,
transactions: [CONTRACT_HASH_1, CONTRACT_HASH_2], // should not include the transaction with wrong nonce
receiptsRoot: DEFAULT_BLOCK_RECEIPTS_ROOT_HASH,
},
showDetails,
);
});
});
Loading