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

internal/ethapi: correctly generate access list when no gas limit is provided #25328

Closed
wants to merge 1 commit into from

Conversation

lightclient
Copy link
Member

Currently, geth returns an incorrect access list or fails when calling eth_createAccessList without a gas limit. This is because during each generation of the access list generation, it does a gas limit estimate. However, the estimate runs before the updated access list is set. This eventually causes the call to execute with less gas than required by the intrinsic gas formula.

Before patch:

> a = personal.listAccounts[0]
"0x9ca58ca9c146b4910075875c23c006ba0b3fe5ad"
> eth.sendTransaction({"from": a, "data": "0x6009600c60003960096000f3585854585854585854"})
"0x67F8AB20E1a548F447b1b3c3D61f18793E42f6dD"
> eth.createAccessList({"to": "0x67F8AB20E1a548F447b1b3c3D61f18793E42f6dD", "from": a}, "latest")
WARN [07-18|13:39:25.020] Served eth_createAccessList              reqid=13 duration=7.551625ms err="failed to apply transaction: 0xebe6fc97a9e69824f6a1f6c21cc8d44a59ffc65781fbf453b6b62d9325fbff9c err: intrinsic gas too low: have 27312, want 29100"
Error: failed to apply transaction: 0xebe6fc97a9e69824f6a1f6c21cc8d44a59ffc65781fbf453b6b62d9325fbff9c err: intrinsic gas too low: have 27312, want 29100
        at web3.js:6365:9(45)
        at send (web3.js:5099:62(34))
        at <eval>:1:21(9)
> eth.createAccessList({"to": "0x67F8AB20E1a548F447b1b3c3D61f18793E42f6dD", "from": a, "gas": "0xffff"}, "latest")
{
  accessList: [{
      address: "0x67F8AB20E1a548F447b1b3c3D61f18793E42f6dD",
      storageKeys: ["0x0000000000000000000000000000000000000000000000000000000000000004", "0x0000000000000000000000000000000000000000000000000000000000000007", "0x0000000000000000000000000000000000000000000000000000000000000001"]
  }],
  gasUsed: "0x72e4"
}

After patch:

> a = personal.listAccounts[0]
"0x3f9bd0812b70643544bfe736dd37d59b4896663b"
> eth.sendTransaction({"from": a, "data": "0x6009600c60003960096000f3585854585854585854"})
"0x1F9949697a37ED29a0e5B5B44daDD87927e96d9A"
> eth.createAccessList({"to": "0x1F9949697a37ED29a0e5B5B44daDD87927e96d9A", "from": a}, "latest")
{
  accessList: [{
      address: "0x1f9949697a37ed29a0e5b5b44dadd87927e96d9a",
      storageKeys: ["0x0000000000000000000000000000000000000000000000000000000000000001", "0x0000000000000000000000000000000000000000000000000000000000000004", "0x0000000000000000000000000000000000000000000000000000000000000007"]
  }],
  gasUsed: "0x72e4"
}

@rjl493456442
Copy link
Member

But in your example, the gas usage and accesslist are identical?

@lightclient
Copy link
Member Author

Yes, but I have to feed it a gas limit large enough to simulate the call and calculate the access list. The returned gas limit is just the gas used on the final call of the simulation.

@lightclient lightclient changed the title internal/ethapi: correctly estimate gas during access list generation internal/ethapi: correctly generate access list when no gas limit is provided Jul 19, 2022
@lightclient
Copy link
Member Author

I updated the title so it is a bit clearer!

@rjl493456442
Copy link
Member

@MariusVanDerWijden Any particular reason for re-estimating gas usage every time?

  • If gas amount is provided, we use this amount for executing and bubble up error if it's not enough
  • If gas amount is not provided, we can use a large amount(e.g. the gaslimit of current block) for executing

@ligi ligi added the pr:merge label Aug 2, 2022
@holiman
Copy link
Contributor

holiman commented Aug 2, 2022

IMO this PR is fine (fixes a flaw), and it would be fine to rewrite it to be more clever in a different PR.

@fjl
Copy link
Contributor

fjl commented Aug 2, 2022

It would be better to remove the gas estimation from this loop entirely. eth_createAccessList is about finding the most complete access list, so we should just run the VM with the RPC gas cap instead of estimating gas here.

@lightclient
Copy link
Member Author

Closing in favor of #25467.

@lightclient lightclient closed this Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants