Skip to content

Conversation

@txhsl
Copy link
Contributor

@txhsl txhsl commented Jul 15, 2024

Close #262.

@txhsl txhsl force-pushed the avoid-rpc-estimate-error branch from e0dbd07 to 7949ec0 Compare July 15, 2024 09:51
@txhsl txhsl requested a review from AnnaShaleva July 15, 2024 09:52
@txhsl
Copy link
Contributor Author

txhsl commented Jul 15, 2024

By filling up the MaxPriorityFeePerGas field, we can let third-party tools to pass minGasTipCap check if they don't provide it.

But, let me take Remix as an example, it only provides a MaxFeePerGas which equals 4/3 of the latest base fee. Since we are going to burn 50% or less of the transaction fee, the MaxFeePerGas should be larger than 2 * baseFee, otherwise the sanity check will fail. So the issue is not fully solved yet.

Is it correct to overwrite the MaxFeePerGas in estimation, or any other solutions?

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Is it correct to overwrite the MaxFeePerGas in estimation, or any other solutions?

We may try to modify args.setFeeDefaults in order to make special conditions for NeoXBurn hardfork. But I'm not sure if MaxFeePerGas is safe to be modified, even only in case if it's insufficient. Because other tools may rely on their value, whereas backend silently modifies it for its own check, and if it's not a problem with Remix, then it may be a problem for some other tool.

@txhsl
Copy link
Contributor Author

txhsl commented Jul 16, 2024

But I'm not sure if MaxFeePerGas is safe to be modified, even only in case if it's insufficient. Because other tools may rely on their value, whereas backend silently modifies it for its own check, and if it's not a problem with Remix, then it may be a problem for some other tool.

Well, let's have a look at documents.

The code comment says.

EstimateGas returns the lowest possible gas limit that allows the transaction to run successfully at block blockNrOrHash, or the latest block if blockNrOrHash is unspecified. It returns error if the transaction would revert or if there are unexpected failures. The returned value is capped by both args.Gas (if non-nil & non-zero) and the backend's RPCGasCap configuration (if non-zero).

The official document says.

EstimateGas generates and returns an estimate of how much gas is necessary to allow the transaction to complete. The transaction will not be added to the blockchain.

And Alchemy notes.

  • eth_estimateGas will check the balance of the sender (to make sure that the sender has enough gas to complete the request). This means that even though the call doesn't consume any gas, the from address must have enough gas to execute the transaction.
  • If no gas is specified geth uses the block gas limit from the pending block as an upper bound. As a result, the returned estimate might not be enough to execute the call/transaction when the amount of actual gas needed is higher than the pending block gas limit.

So, there are several points.

  1. EstimateGas returns a gas limit value if the requested tx can succeed;
  2. EstimateGas returns an error if the requested tx failed;
  3. EstimateGas checks the balance of the sender and returns an error if the balance is not enough to pay for the requested tx.

I think, set a minimum price for empty request is compatible based on this description. Ideally, we should return a ErrUnderpriced error to Remix, but we have to make it successful.

setFeeDefaults() works good on filling in default fee values for unspecified tx fields, but we are going to do something different, which is filling in minimum allowed fee values for underpriced tx. So I'm going to add a new one.

@txhsl txhsl force-pushed the avoid-rpc-estimate-error branch from 7949ec0 to c4df340 Compare July 16, 2024 06:55
@txhsl txhsl requested a review from chenquanyu July 16, 2024 07:03
@txhsl txhsl merged commit 9428d77 into bane-main Jul 16, 2024
@txhsl txhsl deleted the avoid-rpc-estimate-error branch July 16, 2024 09:46
@txhsl txhsl changed the title internal/ethapi: set default fees before gas estimation internal/ethapi: set necessary fee price before gas estimation Jul 16, 2024
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

This way looks better.

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.

Avoid gas estimate errors caused by the fee policies of Neo X

4 participants