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

Fix evm gas estimate bug #2833

Merged
merged 1 commit into from
Jun 11, 2024
Merged

Fix evm gas estimate bug #2833

merged 1 commit into from
Jun 11, 2024

Conversation

dastansam
Copy link
Member

@dastansam dastansam commented Jun 10, 2024

  • makes rpc-binary-search-estimate default and switches it on
  • uses pallet-ethereum provided weight converter function to derive weight limit and proof size base cost

partially fixes #2773

cc @abhi3700 could you please try it yourself as well, compile the node/domain with this fix and using --skip-simulation

Code contributor checklist:

vedhavyas
vedhavyas previously approved these changes Jun 11, 2024
Copy link
Contributor

@vedhavyas vedhavyas left a comment

Choose a reason for hiding this comment

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

Changes looks good to me.
Though I'm not sure if deriving weight limit here makes any difference. Lets try anyway

@@ -40,3 +40,7 @@ sp-core = { git = "https://github.com/subspace/polkadot-sdk", rev = "6da3c45e1d5
sp-inherents = { git = "https://github.com/subspace/polkadot-sdk", rev = "6da3c45e1d5b3c1f09b5e54152b8848149f9d5e6" }
sp-runtime = { git = "https://github.com/subspace/polkadot-sdk", rev = "6da3c45e1d5b3c1f09b5e54152b8848149f9d5e6" }
substrate-frame-rpc-system = { git = "https://github.com/subspace/polkadot-sdk", rev = "6da3c45e1d5b3c1f09b5e54152b8848149f9d5e6" }

[features]
default = ["rpc-binary-search-estimate"]
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 sure I have enabled this for evm specific crates.

Copy link
Member Author

Choose a reason for hiding this comment

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

reverted

@abhi3700
Copy link
Contributor

abhi3700 commented Jun 11, 2024

It works fine with forge .....--skip-simulation flag. But without using the flag, I can see the txs failing like before with OutOfGas, Reverted errors.

I used the script without setting gas manually.

@dastansam
Copy link
Member Author

It works fine with forge .....--skip-simulation flag. But without using the flag, I can see the txs failing like before with OutOfGas error.

yeah that was the idea. Without this PR it fails with out of gas even with skip simulation. You can try that as well, if you have time of course

@dastansam dastansam requested a review from vedhavyas June 11, 2024 12:28
@dastansam dastansam added this pull request to the merge queue Jun 11, 2024
Merged via the queue into main with commit c4c1a63 Jun 11, 2024
9 checks passed
@dastansam dastansam deleted the evm-domain-gas-estimate-fix branch June 11, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure of Solidity script on Nova while pass on other EVM chains
3 participants