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

Duplicated calculation in eth_estimateGas. #1584

Closed
Tracked by #1596
yangby-cryptape opened this issue Nov 22, 2023 · 0 comments · Fixed by #1591, #1599 or #1609
Closed
Tracked by #1596

Duplicated calculation in eth_estimateGas. #1584

yangby-cryptape opened this issue Nov 22, 2023 · 0 comments · Fixed by #1591, #1599 or #1609
Assignees
Labels
t:bug Something isn't working

Comments

@yangby-cryptape
Copy link
Collaborator

yangby-cryptape commented Nov 22, 2023

Description

When call eth_estimateGas to estimate gas used, the gas calculation for create-tx, call-tx and tx.data are incorrect and duplicated.

let base_gas = if to.is_some() {
GAS_CALL_TRANSACTION + data_gas_cost(&data)
} else {
GAS_CREATE_TRANSACTION + GAS_CALL_TRANSACTION + data_gas_cost(&data)
};

let used_gas = executor.used_gas() + base_gas;


For Example

  • Do simplest transfer transaction.

    Data Size1 Estimated in Axon Actual in Axon Estimated in Geth Actual in Geth
    0 42000 21000 21000 21000
    1 42084 21016 21016 21016
    2 42168 21032 21032 21032

Appendix: More Unit Tests are Required

  • Call eth_estimateGas for a transaction whose gas is set to a value that greater than u64::MAX, will cause Axon panic.

    .map(|gas| gas.as_u64())

    There is already an in-process PR.

    I just record it here, and request a unit test for it, after that PR merged.

Footnotes

  1. Set the transaction.data to be N non-zero bytes.

@yangby-cryptape yangby-cryptape added the t:bug Something isn't working label Nov 22, 2023
@Flouse Flouse linked a pull request Nov 23, 2023 that will close this issue
6 tasks
@yangby-cryptape yangby-cryptape self-assigned this Nov 24, 2023
@Flouse Flouse linked a pull request Nov 27, 2023 that will close this issue
7 tasks
@Flouse Flouse mentioned this issue Nov 27, 2023
6 tasks
@Flouse Flouse reopened this Nov 30, 2023
@Flouse Flouse linked a pull request Dec 1, 2023 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment