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: eth_estimateGas error in zero balance address #537

Merged
merged 7 commits into from
Oct 25, 2023

Conversation

colinlyguo
Copy link
Member

@colinlyguo colinlyguo commented Oct 18, 2023

1. Purpose or design rationale of this PR

Correct response (Ethereum, account with 0 balance):

curl https://mainnet.infura.io/v3/9baa7a0388c9440a85534617d4d862a9 -X POST -H "Content-Type: application/json" --data '{"method":"eth_estimateGas","params":[{"from":"0x8888889C9818892B700e27F316cc3E41e17fBeb9","to":"0x8888889C9818892B700e27F316cc3E41e17fBeb9"}],"id":1,"jsonrpc":"2.0"}'

{"jsonrpc":"2.0","id":1,"result":"0x5208"}

Incorrect response (Scroll, account with 0 balance):

curl https://rpc.scroll.io/ -X POST -H "Content-Type: application/json" --data '{"method":"eth_estimateGas","params":[{"from":"0x8888889C9818892B700e27F316cc3E41e17fBeb9","to":"0x8888889C9818892B700e27F316cc3E41e17fBeb9"}],"id":1,"jsonrpc":"2.0"}'

{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"err: insufficient funds for gas * price + value: address 0x8888889C9818892b700E27f316CC3E41e17fbeb9 have 0 want 75453596795115 (supplied gas 5005617)"}}

Root cause:

  1. Error msg return path:
    a. https://github.com/scroll-tech/go-ethereum/blob/scroll-v5.0.1/core/state_transition.go#L243
    b. https://github.com/scroll-tech/go-ethereum/blob/scroll-v5.0.1/core/state_transition.go#L312
    c. https://github.com/scroll-tech/go-ethereum/blob/scroll-v5.0.1/core/state_transition.go#L341
    d. https://github.com/scroll-tech/go-ethereum/blob/scroll-v5.0.1/core/state_transition.go#L205
    e. https://github.com/scroll-tech/go-ethereum/blob/scroll-v5.0.1/internal/ethapi/api.go#L1003
    f. https://github.com/scroll-tech/go-ethereum/blob/scroll-v5.0.1/internal/ethapi/api.go#L1176
  2. Caused by adding l1 data fee here: https://github.com/scroll-tech/go-ethereum/blob/scroll-v5.0.1/core/state_transition.go#L238

No need to check L1 data fee in DoCall, because it only checks if the tx can be successfully executed within a specified gas limit.

2. PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • fix: A bug fix

3. Deployment tag versioning

Has the version in params/version.go been updated?

  • This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
  • Yes

4. Breaking change label

Does this PR have the breaking-change label?

  • This PR is not a breaking change
  • Yes

@colinlyguo colinlyguo added the bug Something isn't working label Oct 18, 2023
@colinlyguo colinlyguo self-assigned this Oct 18, 2023
@colinlyguo colinlyguo changed the title fix: estimate gas zero balance error fix: eth_estimateGas error in zero balance address Oct 18, 2023
@HAOYUatHZ
Copy link
Member

HAOYUatHZ commented Oct 19, 2023

I am not sure whether it's what we want, if a user estimates gas and then uses this gas to call, the call will still fail?

i feel like we can handle this types of error specially (if it's this error type, don't return error in DoEstimateGas, but provide the l1fee&l2fee details)

@mask-pp
Copy link

mask-pp commented Oct 19, 2023

It works at my local env.

mask-pp
mask-pp previously approved these changes Oct 19, 2023
@colinlyguo
Copy link
Member Author

colinlyguo commented Oct 19, 2023

I am not sure whether it's what we want, if a user estimates gas and then uses this gas to call, the call will still fail?

i feel like we can handle this types of error specially (if it's this error type, don't return error in DoEstimateGas, but provide the l1fee&l2fee details)

L1 data fee is subtracted here when tx has fees: https://github.com/scroll-tech/go-ethereum/blob/scroll-v5.0.1/internal/ethapi/api.go#L1137-L1145
After subtraction, DoCall only cares about the L2 fees, which is compatible with semantics in Ethereum. So preCheck can ignore L1 data fees.

@colinlyguo
Copy link
Member Author

colinlyguo commented Oct 19, 2023

Local test looks good (eth_call & eth_estimateGas):

curl http://localhost:8545 -X POST -H "Content-Type: application/json" --data '{"method":"eth_call","params":[{"from":"0x8888889C9818892B700e27F316cc3E41e17fBeb9","to":"0x8888889C9818892B700e27F316cc3E41e17fBeb9"}, "latest"],"id":1,"jsonrpc":"2.0"}' | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   208  100    39  100   169  12400  53736 --:--:-- --:--:-- --:--:--  203k
{
  "jsonrpc": "2.0",
  "id": 1,
  "result": "0x"
}
curl http://localhost:8545 -X POST -H "Content-Type: application/json" --data '{"method":"eth_estimateGas","params":[{"from":"0x8888889C9818892B700e27F316cc3E41e17fBeb9","to":"0x8888889C9818892B700e27F316cc3E41e17fBeb9"}],"id":1,"jsonrpc":"2.0"}' | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   209  100    43  100   166   7742  29888 --:--:-- --:--:-- --:--:-- 69666
{
  "jsonrpc": "2.0",
  "id": 1,
  "result": "0x5208"
}

@mask-pp
Copy link

mask-pp commented Oct 20, 2023

One situation that should be considered is that l1DataFee is used when running but eth_estimateGas doesn't include l1DataFee.

@mask-pp
Copy link

mask-pp commented Oct 20, 2023

It works at my local env.

More complete verification is needed.

HAOYUatHZ
HAOYUatHZ previously approved these changes Oct 22, 2023
mask-pp
mask-pp previously approved these changes Oct 25, 2023
@colinlyguo colinlyguo dismissed stale reviews from mask-pp and HAOYUatHZ via 11bf5a9 October 25, 2023 08:18
@Thegaram
Copy link

An additional note: eth_estimateGas will fail if the account has zero balance but the user provides a gas price:

$ curl http://localhost:8545 -X POST -H "Content-Type: application/json" --data '{"method":"eth_estimateGas","params":[{"from":"0x8888889C9818892B700e27F316cc3E41e17fBeb9","to":"0x8888889C9818892B700e27F316cc3E41e17fBeb9", "gasPrice": "0x1"}],"id":1,"jsonrpc":"2.0"}'
{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"insufficient funds for l1 fee"}}

l1DataFee, err := EstimateL1MsgFee(ctx, b, args, blockNrOrHash, nil, 0, gasCap, b.ChainConfig())
if err != nil {
return 0, err
}
if l1DataFee.Cmp(available) >= 0 {
return 0, errors.New("insufficient funds for l1 fee")
}

This is consistent with the way that geth handles value in this case.

@colinlyguo
Copy link
Member Author

An additional note: eth_estimateGas will fail if the account has zero balance but the user provides a gas price:

$ curl http://localhost:8545 -X POST -H "Content-Type: application/json" --data '{"method":"eth_estimateGas","params":[{"from":"0x8888889C9818892B700e27F316cc3E41e17fBeb9","to":"0x8888889C9818892B700e27F316cc3E41e17fBeb9", "gasPrice": "0x1"}],"id":1,"jsonrpc":"2.0"}'
{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"insufficient funds for l1 fee"}}

l1DataFee, err := EstimateL1MsgFee(ctx, b, args, blockNrOrHash, nil, 0, gasCap, b.ChainConfig())
if err != nil {
return 0, err
}
if l1DataFee.Cmp(available) >= 0 {
return 0, errors.New("insufficient funds for l1 fee")
}

This is consistent with the way that geth handles value in this case.

Yeah. Let's consider how to handle this systematically after receiving more feedback. Potential inconsistency with SDKs.

@Thegaram Thegaram merged commit bbf4204 into develop Oct 25, 2023
5 checks passed
@Thegaram Thegaram deleted the fix-estimateGas-zero-balance-error branch October 25, 2023 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants