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.

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

Implement gasLimit Precheck #304

Merged
merged 32 commits into from
Jul 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
2cfb8ad
gasLimit precheck
Jun 15, 2022
330060c
Merge branch 'main' into 128-gasLimit-precheck
Jun 15, 2022
38dbb4f
fix bignumber comparison issue
Jun 16, 2022
b12bfc7
Merge branch 'main' into 128-gasLimit-precheck
Jun 16, 2022
56bebbf
Merge branch 'main' into 128-gasLimit-precheck
Jun 17, 2022
b3d5a15
Merge branch 'main' into 128-gasLimit-precheck
Ivo-Yankov Jul 4, 2022
c8ffccc
test: add acceptance tests
Ivo-Yankov Jul 4, 2022
f53ffe8
test: add unit tests
Ivo-Yankov Jul 4, 2022
69b3305
Merge branch 'main' into 128-gasLimit-precheck
Ivo-Yankov Jul 4, 2022
121f46b
chore: resolve merge conflicts
Ivo-Yankov Jul 4, 2022
d837526
chore: code smells
Ivo-Yankov Jul 4, 2022
9f80bac
fix: simplify test logic
Ivo-Yankov Jul 5, 2022
d29e963
Merge branch 'main' into 128-gasLimit-precheck
Ivo-Yankov Jul 6, 2022
926ace9
chore: resolve conflicts
Ivo-Yankov Jul 6, 2022
cbd43e3
nit: resolving comments
Ivo-Yankov Jul 6, 2022
7d54430
Merge branch 'main' into 128-gasLimit-precheck
Ivo-Yankov Jul 6, 2022
877f7ae
gasLimit precheck
Jun 15, 2022
f523ed5
test: add acceptance tests
Ivo-Yankov Jul 4, 2022
779a789
test: add unit tests
Ivo-Yankov Jul 4, 2022
18bbc84
chore: resolve merge conflicts
Ivo-Yankov Jul 4, 2022
e423acb
fix: simplify test logic
Ivo-Yankov Jul 5, 2022
db2a476
chore: resolve conflicts
Ivo-Yankov Jul 6, 2022
2c95d4a
nit: resolving comments
Ivo-Yankov Jul 6, 2022
c5d20bd
Rebased and addressed own feedback
Nana-EC Jul 6, 2022
5a08c82
Fix merge conflict
Nana-EC Jul 6, 2022
a79c0a3
Merge remote-tracking branch 'origin/128-gasLimit-precheck' into 128-…
Ivo-Yankov Jul 8, 2022
df46ede
chore: resolve conflicts
Ivo-Yankov Jul 8, 2022
b91cf8c
Merge branch 'main' into 128-gasLimit-precheck
Ivo-Yankov Jul 8, 2022
2998412
test: disable precheck to check if that fixes dapp ci
Ivo-Yankov Jul 8, 2022
8f4c303
test: enable nonce and chainId prechecks
Ivo-Yankov Jul 8, 2022
fe35006
test: enable gasLimit precheck
Ivo-Yankov Jul 8, 2022
116672d
fix: wrong intrinsic gas cost for tx with empty data
Ivo-Yankov Jul 8, 2022
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
9 changes: 8 additions & 1 deletion packages/relay/src/lib/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,5 +45,12 @@ export default {
TYPE_ACCOUNT: 'account',

FEE_HISTORY_MAX_RESULTS: Number(process.env.FEE_HISTORY_MAX_RESULTS || 10),
ORDER
ORDER,

BLOCK_GAS_LIMIT: 15_000_000,
ISTANBUL_TX_DATA_NON_ZERO_COST: 16,
TX_BASE_COST: 21_000,
TX_DEFAULT_GAS: 400_000,
TX_CREATE_EXTRA: 32_000,
TX_DATA_ZERO_COST: 4,
};
12 changes: 11 additions & 1 deletion packages/relay/src/lib/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,16 @@ export const predefined = {
code: -32006,
message: 'Incorrect nonce'
}),
'GAS_LIMIT_TOO_HIGH': new JsonRpcError({
name: 'gasLimit too high',
code: -32005,
message: 'Transaction gas limit exceeds block gas limit'
}),
'GAS_LIMIT_TOO_LOW': new JsonRpcError({
name: 'gasLimit too low',
code: -32003,
message: 'Intrinsic gas exceeds gas limit'
}),
'REQUEST_BEYOND_HEAD_BLOCK': (requested: number, latest: number) => new JsonRpcError({
name: 'Incorrect block',
code: -32000,
Expand All @@ -91,4 +101,4 @@ export const predefined = {
code: -32000,
message: 'Insufficient funds for transfer'
}),
};
};
15 changes: 9 additions & 6 deletions packages/relay/src/lib/eth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ export class EthImpl implements Eth {
static emptyArrayHex = '0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347';
static zeroAddressHex = '0x0000000000000000000000000000000000000000';
static emptyBloom = "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000";
static defaultGas = EthImpl.numberTo0x(400_000);
static gasTxBaseCost = EthImpl.numberTo0x(21_000);
static defaultGas = EthImpl.numberTo0x(constants.TX_DEFAULT_GAS);
static gasTxBaseCost = EthImpl.numberTo0x(constants.TX_BASE_COST);
static ethTxType = 'EthereumTransaction';
static ethEmptyTrie = '0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421';
static defaultGasUsedRatio = EthImpl.numberTo0x(0.5);
Expand Down Expand Up @@ -629,11 +629,14 @@ export class EthImpl implements Eth {
*/
async sendRawTransaction(transaction: string): Promise<string | JsonRpcError> {
this.logger.trace('sendRawTransaction(transaction=%s)', transaction);
await this.precheck.nonce(transaction);

const chainIdPrecheckRes = this.precheck.chainId(transaction);
if (!chainIdPrecheckRes.passes) {
return chainIdPrecheckRes.error;
try {
await this.precheck.gasLimit(transaction);
await this.precheck.nonce(transaction);
this.precheck.chainId(transaction);
}
catch(e: any) {
return e;
}

const gasPrice = await this.getFeeWeibars(EthImpl.ethSendRawTransaction);
Expand Down
58 changes: 50 additions & 8 deletions packages/relay/src/lib/precheck.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@
import * as ethers from 'ethers';
import { predefined } from './errors';
import { MirrorNodeClient, SDKClient } from './clients';
import {EthImpl} from "./eth";
import {Logger} from "pino";
import { EthImpl } from "./eth";
import { Logger } from "pino";
import constants from './constants';

export class Precheck {
private mirrorNodeClient: MirrorNodeClient;
Expand Down Expand Up @@ -65,18 +66,15 @@ export class Precheck {
}
}


chainId(transaction: string) {
const tx = ethers.utils.parseTransaction(transaction);
const txChainId = EthImpl.prepend0x(Number(tx.chainId).toString(16));
const passes = txChainId === this.chain;
if (!passes) {
this.logger.trace('Failed chainId precheck for sendRawTransaction(transaction=%s, chainId=%s)', transaction, txChainId);
throw predefined.UNSUPPORTED_CHAIN_ID(txChainId, this.chain);
}

return {
passes,
error: predefined.UNSUPPORTED_CHAIN_ID(txChainId, this.chain)
};
}

gasPrice(transaction: string, gasPrice: number) {
Expand Down Expand Up @@ -116,11 +114,55 @@ export class Precheck {
}
} catch (error: any) {
this.logger.trace('Error on balance precheck for sendRawTransaction(transaction=%s, totalValue=%s, error=%s)', transaction, txTotalValue, error.message);

result.passes = false;
result.error = predefined.INTERNAL_ERROR;
}

return result;
}

/**
* @param transaction
*/
async gasLimit(transaction: string) {
const tx = ethers.utils.parseTransaction(transaction);
const gasLimit = tx.gasLimit.toNumber();
const failBaseLog = 'Failed gasLimit precheck for sendRawTransaction(transaction=%s).';

const intrinsicGasCost = Precheck.transactionIntrinsicGasCost(tx.data, tx.to);


if (gasLimit > constants.BLOCK_GAS_LIMIT) {
this.logger.trace(`${failBaseLog} Gas Limit was too high: %s, block gas limit: %s`, transaction, gasLimit, constants.BLOCK_GAS_LIMIT);
throw predefined.GAS_LIMIT_TOO_HIGH;
} else if (gasLimit < intrinsicGasCost) {
this.logger.trace(`${failBaseLog} Gas Limit was too low: %s, intrinsic gas cost: %s`, transaction, gasLimit, intrinsicGasCost);
throw predefined.GAS_LIMIT_TOO_LOW;
}
}

/**
* Calculates the intrinsic gas cost based on the number of empty bytes and whether the transaction is CONTRACT_CREATE
* @param data
* @param to
* @private
*/
private static transactionIntrinsicGasCost(data: string, to: string | undefined) {
const isCreate = (to == undefined) || (to.length == 0);

let zeros = 0;

const dataBytes = Buffer.from(data, "hex");

for (const c of dataBytes) {
if (c == 0) {
zeros++;
}
}

const nonZeros = data.replace('0x', '').length - zeros;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was causing a wrong calculation of the gas cost. If 0x is not removed from data, that counts as 2 nonZero bytes, and the calculated gas cost is TX_BASE_COST + 2 * ISTANBUL_TX_DATA_NON_ZERO_COST, which meant that transactions with empty data (hbar transfer transactions) could not pass the precheck if the default gasLimit (21000) was used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch

const cost = constants.TX_BASE_COST + constants.TX_DATA_ZERO_COST * zeros + constants.ISTANBUL_TX_DATA_NON_ZERO_COST * nonZeros;
return isCreate ? cost + constants.TX_CREATE_EXTRA : cost;
}
}
15 changes: 14 additions & 1 deletion packages/relay/tests/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@
*/

import {expect} from "chai";
import {ethers} from 'ethers';

// Randomly generated key
const defaultPrivateKey = '8841e004c6f47af679c91d9282adc62aeb9fabd19cdff6a9da5a358d0613c30a';

const expectUnsupportedMethod = (result) => {
expect(result).to.have.property('code');
Expand All @@ -29,6 +33,15 @@ const expectUnsupportedMethod = (result) => {
expect(result.message).to.be.equal('Unsupported JSON-RPC method');
};

const expectedError = () => {
expect(true).to.eq(false);
};

const signTransaction = async (transaction, key = defaultPrivateKey) => {
const wallet = new ethers.Wallet(key);
return wallet.signTransaction(transaction);
};

const mockData = {
accountEvmAddress: '0x00000000000000000000000000000000000003f6',
account: {
Expand Down Expand Up @@ -80,4 +93,4 @@ const mockData = {
}
};

export {expectUnsupportedMethod, mockData};
export {expectUnsupportedMethod, expectedError, signTransaction, mockData};
118 changes: 111 additions & 7 deletions packages/relay/tests/lib/precheck.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,22 @@ const registry = new Registry();

import sinon from 'sinon';
import pino from 'pino';
import {Precheck} from "../../src/lib/precheck";
import {MirrorNodeClient, SDKClient} from "../../src/lib/clients";
import { Precheck } from "../../src/lib/precheck";
import { expectedError, signTransaction } from "../helpers";
import { MirrorNodeClient, SDKClient } from "../../src/lib/clients";
import axios from "axios";
import MockAdapter from "axios-mock-adapter";
import { ethers } from "ethers";
import constants from '../../src/lib/constants';
const logger = pino();

describe('Precheck', async function() {

const txWithMatchingChainId = '0x02f87482012a0485a7a358200085a7a3582000832dc6c09400000000000000000000000000000000000003f78502540be40080c001a006f4cd8e6f84b76a05a5c1542a08682c928108ef7163d9c1bf1f3b636b1cd1fba032097cbf2dda17a2dcc40f62c97964d9d930cdce2e8a9df9a8ba023cda28e4ad';
const txWithNonMatchingChainId = '0xf86a0385a7a3582000832dc6c09400000000000000000000000000000000000003f78502540be400801ca06750e92db52fa708e27f94f27e0cfb7f5800f9b657180bb2e94c1520cfb1fb6da01bec6045068b6db38b55017bb8b50166699384bc1791fd8331febab0cf629a2a';
const oneTinyBar = ethers.utils.parseUnits('1', 10);
const defaultGasPrice = 720_000_000_000;
const defaultChainId = Number('0x12a');

let precheck: Precheck;
let mock: MockAdapter;
Expand All @@ -61,19 +65,119 @@ describe('Precheck', async function() {
});

describe('chainId', async function() {
it('should return true for matching chainId', async function() {
const result = precheck.chainId(txWithMatchingChainId);
it('should pass for matching chainId', async function() {
try {
precheck.chainId(txWithMatchingChainId);
}
catch(e) {
expect(e).to.not.exist;
}
});

it('should not pass for non-matching chainId', async function() {
try {
precheck.chainId(txWithNonMatchingChainId);
expectedError();
}
catch(e: any) {
expect(e).to.exist;
expect(e.code).to.eq(-32000);
expect(e.message).to.eq('ChainId (0x0) not supported. The correct chainId is 0x12a');
}
});
});

describe('gas price', async function() {
it('should return true for gas price gt to required gas price', async function() {
const result = precheck.gasPrice(txWithMatchingChainId, 10);
expect(result).to.exist;
expect(result.error).to.exist;
expect(result.passes).to.eq(true);
});

it('should return false for non-matching chainId', async function() {
const result = precheck.chainId(txWithNonMatchingChainId);
it('should return true for gas price equal to required gas price', async function() {
const result = precheck.gasPrice(txWithMatchingChainId, defaultGasPrice);
expect(result).to.exist;
expect(result.error).to.exist;
expect(result.passes).to.eq(true);
});

it('should not pass for non-matching chainId', async function() {
try {
precheck.chainId(txWithNonMatchingChainId);
expectedError();
}
catch(e: any) {
expect(e).to.exist;
expect(e.code).to.eq(-32000);
expect(e.message).to.eq('ChainId (0x0) not supported. The correct chainId is 0x12a');
}
});

it('should return false for gas price not enough', async function() {
const minGasPrice = 1000 * constants.TINYBAR_TO_WEIBAR_COEF;
const result = precheck.gasPrice(txWithMatchingChainId, minGasPrice);
expect(result).to.exist;
expect(result.error).to.exist;
expect(result.passes).to.eq(false);
});
});

describe('gasLimit', async function() {
const defaultTx = {
value: oneTinyBar,
gasPrice: defaultGasPrice,
chainId: defaultChainId
};

function testFailingGasLimitPrecheck(gasLimits, errorCode, errorMessage) {
for (const gasLimit of gasLimits) {
it(`should fail for gasLimit: ${gasLimit}`, async function () {
const tx = {
...defaultTx,
gasLimit: gasLimit
};
const signed = await signTransaction(tx);

try {
await precheck.gasLimit(signed);
expectedError();
} catch (e: any) {
expect(e).to.exist;
expect(e.code).to.eq(errorCode);
expect(e.message).to.eq(errorMessage);
}
});
}
}

function testPassingGasLimitPrecheck(gasLimits) {
for (const gasLimit of gasLimits) {
it(`should pass for gasLimit: ${gasLimit}`, async function () {
const tx = {
...defaultTx,
gasLimit: gasLimit
};
const signed = await signTransaction(tx);

try {
await precheck.gasLimit(signed);
} catch (e: any) {
expect(e).to.not.exist;
}
});
}
}

const validGasLimits = [60000, 100000, 500000, 1000000, 5000000, 10000000];
const lowGasLimits = [1, 10, 100, 1000, 10000, 30000, 50000];
const highGasLimits = [20000000, 100000000, 999999999999];

testPassingGasLimitPrecheck(validGasLimits);
testFailingGasLimitPrecheck(lowGasLimits, -32003, 'Intrinsic gas exceeds gas limit');
testFailingGasLimitPrecheck(highGasLimits, -32005, 'Transaction gas limit exceeds block gas limit');
});

describe('gas price', async function() {
it('should return true for gas price gt to required gas price', async function() {
const result = precheck.gasPrice(txWithMatchingChainId, 10);
Expand All @@ -96,5 +200,5 @@ describe('Precheck', async function() {
expect(result.error).to.exist;
expect(result.passes).to.eq(false);
});
})
});
});
Loading