Skip to content

Add workaround for logic bug in 'canPayFee()'.#1310

Closed
oneeman wants to merge 1 commit intooneeman/merge-upstream-1.9.13from
oneeman/workaround-for-canPayFees-issue
Closed

Add workaround for logic bug in 'canPayFee()'.#1310
oneeman wants to merge 1 commit intooneeman/merge-upstream-1.9.13from
oneeman/workaround-for-canPayFees-issue

Conversation

@oneeman
Copy link
Copy Markdown
Contributor

@oneeman oneeman commented Jan 21, 2021

Description

This PR implements a non-hard-fork fix for the issue brought up in #1292. It does not modify the behavior during mining and block processing, but rather only for state transitions done as part of eth_call and eth_estimateGas. Though the the error during gas estimation is not a regression from the 1.9.13 merge (it already happens with prior versions of celo-blockchain), I am opening this PR against the 1.9.13 merge branch because it includes some changes the code for eth_call and eth_estimateGas, and doing it this way will avoid gratuitous conflicts.

Note that we should still do a hard fork to fix the behavior during mining and block processing.

The correct behavior is to allow a transaction if it has enough funds to pay the fee. This was how it worked if the fee was paid in Celo. However, if the fee was in another currency, the check currently uses '>' rather than '>='. Changing that requires a hard fork, since it would allow transactions that are currently being rejected. Until such a hard fork, however, this '>' is causing eth_call and eth_estimateGas to fail when the fee currency isn't Celo (and the account has no funds), even though the gas price is zero. This commit fixes that by adding special handling to use '>=' if the state transition is part of eth_call or eth_estimateGas, while leaving the '>' as is otherwise (e.g. as part of mining or processing a block).

Alternatives

I considered using the minimum gas price being 0 as the condition instead of adding a new field, since in actual transactions that are mined we have a non-zero minimum gas price. However, the approach taken in this commit seems safer and more explicit.

Tested

Trying to send a transaction with fees in cUSD without having any cUSD in the account.

Before the fix, gas estimation fails:

    Error: Gas estimation failed: Could not decode transaction failure reason

After the fix, gas estimation succeeds (since estimation uses gas price zero), and then sending the transaction fails, as it should, since for that the gas price isn't zero:

    Error: insufficient funds for gas * price + value + gatewayFee

Backwards compatibility

This will make eth_call and eth_estimateGas succeed in certain cases where they were currently failing.

The correct behavior is to allow a transaction if it has enough funds to pay the fee. This was how it worked if the fee was paid in Celo. However, if the fee was in another currency, the check uses '>' rather than '>='. Changing that requires a hard fork, since it would allow transactions that are currently being rejected. Until such a hard fork, however, this '>' is causing eth_call and eth_estimateGas to fail when the fee currency isn't Celo (and the account has no funds), even though the gas price is zero.  This commit fixes that by adding special handling to use '>=' if the state transition is part of eth_call or eth_estimateGas, while leaving the '>' as is otherwise (e.g. as part of mining or processing a block).
Copy link
Copy Markdown
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

LGTM

@oneeman
Copy link
Copy Markdown
Contributor Author

oneeman commented Jan 22, 2021

Closing this until we have a better idea of plans for the hard fork change.

@oneeman oneeman closed this Jan 22, 2021
@oneeman oneeman deleted the oneeman/workaround-for-canPayFees-issue branch February 17, 2021 19:38
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.

2 participants