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 eth call gas estimation discrepancy #2440

Merged
merged 6 commits into from
Aug 24, 2023
Merged

Fix eth call gas estimation discrepancy #2440

merged 6 commits into from
Aug 24, 2023

Conversation

librelois
Copy link
Collaborator

@librelois librelois commented Aug 16, 2023 •
edited
Loading

What does it do?

Fix a discrepancy bug between eth_call and on-chain tx execution.
The bug is caused by the fact that the PoV consumed by the ethereum transaction is counted differently in the estimation context than in the on-chain context.
The discrepancy is due to the fact that estimation queries generally not provide values for fields max_fee_per_gas and max_priority_fee_per_gas whereas the on-chain transaction provides values for these fields.

The solution is to consider that these fields are always supplied to estimate the size of the on-chain transaction, this implies over-estimation of 45 gas (negligible for fees) only for legacy transactions and only if the "pov gas" is greater than the used gas.

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@librelois librelois added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes labels Aug 16, 2023
@librelois librelois marked this pull request as ready for review August 16, 2023 12:24
@github-actions
Copy link
Contributor

github-actions bot commented Aug 16, 2023 •
edited
Loading

Coverage generated "Thu Aug 24 18:06:40 UTC 2023":
https://d3ifz9vhxc2wtb.cloudfront.net/pulls/2440/html/index.html

Master coverage: 87.39%
Pull coverage:

Agusrodri
Agusrodri approved these changes Aug 17, 2023
View reviewed changes
Copy link
Contributor

@Agusrodri Agusrodri left a comment

Choose a reason for hiding this comment

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

Some small comments regarding code cleanup mostly. LGTM.

tests/tests/test-gas/test-gas-foreign-asset.ts Outdated
Comment on lines 60 to 70
// const { result } = await context.createBlock(
// await createTransaction(context, {
// ...CHARLETH_TRANSACTION_TEMPLATE,
// to: ADDRESS_ERC20,
// data: ERC20_INTERFACE.encodeFunctionData("approve", [alith.address, 100000000]),
// })
// );

// const receipt = await context.web3.eth.getTransactionReceipt(result.hash);
// console.log("Zero approve gas used", receipt.gasUsed);
// expect(receipt.status).to.equal(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// const { result } = await context.createBlock(
// await createTransaction(context, {
// ...CHARLETH_TRANSACTION_TEMPLATE,
// to: ADDRESS_ERC20,
// data: ERC20_INTERFACE.encodeFunctionData("approve", [alith.address, 100000000]),
// })
// );
// const receipt = await context.web3.eth.getTransactionReceipt(result.hash);
// console.log("Zero approve gas used", receipt.gasUsed);
// expect(receipt.status).to.equal(true);

tests/tests/test-gas/test-gas-foreign-asset.ts Outdated
let gasEst = await context.web3.eth.estimateGas({
from: alith.address,
data: ERC20_INTERFACE.encodeFunctionData("approve", [baltathar.address, 0]),
// gasPrice: "0x0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// gasPrice: "0x0",

tests/tests/test-gas/test-gas-foreign-asset.ts Outdated
Comment on lines 3 to 4
import { KeyringPair } from "@polkadot/keyring/types";
import { ParaId } from "@polkadot/types/interfaces";
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems that these imports are not used

Suggested change
import { KeyringPair } from "@polkadot/keyring/types";
import { ParaId } from "@polkadot/types/interfaces";

tests/tests/test-gas/test-gas-foreign-asset.ts Outdated

import { KeyringPair } from "@polkadot/keyring/types";
import { ParaId } from "@polkadot/types/interfaces";
import { BN, hexToU8a, numberToHex, stringToHex, u8aToHex } from "@polkadot/util";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { BN, hexToU8a, numberToHex, stringToHex, u8aToHex } from "@polkadot/util";
import { BN, hexToU8a, u8aToHex } from "@polkadot/util";

tests/tests/test-gas/test-gas-foreign-asset.ts Outdated
import { blake2AsU8a, xxhashAsU8a } from "@polkadot/util-crypto";
import { expect } from "chai";

import { ALITH_ADDRESS, alith, baltathar, generateKeyringPair } from "../../util/accounts";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { ALITH_ADDRESS, alith, baltathar, generateKeyringPair } from "../../util/accounts";
import { ALITH_ADDRESS, alith, baltathar } from "../../util/accounts";

tests/tests/test-gas/test-gas-foreign-asset.ts Outdated
import { expect } from "chai";

import { ALITH_ADDRESS, alith, baltathar, generateKeyringPair } from "../../util/accounts";
import { PARA_2000_SOURCE_LOCATION, RELAY_SOURCE_LOCATION } from "../../util/assets";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { PARA_2000_SOURCE_LOCATION, RELAY_SOURCE_LOCATION } from "../../util/assets";
import { RELAY_SOURCE_LOCATION } from "../../util/assets";

tests/tests/test-gas/test-gas-foreign-asset.ts Outdated
Comment on lines 11 to 17
import {
registerForeignAsset,
injectHrmpMessageAndSeal,
RawXcmMessage,
XcmFragment,
weightMessage,
} from "../../util/xcm";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import {
registerForeignAsset,
injectHrmpMessageAndSeal,
RawXcmMessage,
XcmFragment,
weightMessage,
} from "../../util/xcm";
import { registerForeignAsset } from "../../util/xcm";

tests/tests/test-gas/test-gas-foreign-asset.ts Outdated
XcmFragment,
weightMessage,
} from "../../util/xcm";
import { customWeb3Request, web3EthCall } from "../../util/providers";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { customWeb3Request, web3EthCall } from "../../util/providers";

tests/tests/test-gas/test-gas-foreign-asset.ts Outdated

import { describeDevMoonbeam } from "../../util/setup-dev-tests";

import { expectOk } from "../../util/expect";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { expectOk } from "../../util/expect";

Agusrodri
Agusrodri reviewed Aug 17, 2023
View reviewed changes
tests/tests/test-gas/test-gas-foreign-asset.ts Outdated
);

const receipt = await context.web3.eth.getTransactionReceipt(result.hash);
console.log("First approve gas used", receipt.gasUsed);
Copy link
Contributor

@Agusrodri Agusrodri Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't we avoid console.logging inside TS tests? I think we had console.log() related issues in our CI in the past (correct me if I'm wrong).

Suggested change
console.log("First approve gas used", receipt.gasUsed);

Agusrodri
Agusrodri reviewed Aug 17, 2023
View reviewed changes
tests/tests/test-gas/test-gas-foreign-asset.ts Outdated
to: ADDRESS_ERC20,
});

console.log("Revoke gas estimate", gasEst);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same case here

Suggested change
console.log("Revoke gas estimate", gasEst);

Agusrodri
Agusrodri reviewed Aug 17, 2023
View reviewed changes
tests/tests/test-gas/test-gas-foreign-asset.ts Outdated
);

const receipt2 = await context.web3.eth.getTransactionReceipt(result2.hash);
console.log("Revoke gas used", receipt2.gasUsed);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.log("Revoke gas used", receipt2.gasUsed);

notlesh
notlesh reviewed Aug 17, 2023
View reviewed changes
runtime/common/src/apis.rs Outdated
// 65 bytes signature
210;
255;
Copy link
Contributor

Choose a reason for hiding this comment

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

How did we get 210 -> 255? If it's 2 * 32 from max_fee_per_gas and max_prority_fee_per_gas, shouldn't it be 274?

The numbers from the comments do add up correctly to 255 at least...

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it seems like we lost from. Is this because there is no such thing in an actual transaction (it's recovered from its signature)...?

Copy link
Contributor

@notlesh notlesh Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

from is 20, so 210 + 64 - 20 => 254, still off by one 😆

Copy link
Collaborator Author

@librelois librelois Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

it seems like we lost from. Is this because there is no such thing in an actual transaction

Yes exactly

still off by one

The one come from access_list, even if the access list is empty, the scale encoded format take 1 byte for the empty Vec.

@librelois librelois merged commit 85031d9 into master Aug 24, 2023
26 checks passed
@librelois librelois deleted the elois-fix-eth-call-diff branch August 24, 2023 23:07
grw-ms pushed a commit that referenced this pull request Aug 28, 2023
@librelois @fgamundi @grw-ms
Fix eth call diff (#2440)
282ee7f 
* fix tx encoded size estimation

* Working test

* test cleanup

* test cleanup 2

* accont for pallet index + call index+ transaction type enum variant

* pretiier

---------

Co-authored-by: Francisco Gamundi <francisco@moonsonglabs.com>
@noandrea noandrea changed the title Fix eth call diff Fix eth call gas estimation discrepancy Aug 31, 2023
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Agusrodri Agusrodri Agusrodri approved these changes

@notlesh notlesh notlesh approved these changes

@crystalin crystalin Awaiting requested review from crystalin

Assignees
No one assigned
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@librelois @notlesh @Agusrodri @fgamundi