Improve gas estimation logic #163
Conversation
| // TODO: Instead of assigning a default value here, we should analyze historical | ||
| // blocks from the current height to estimate an accurate value for this premium. | ||
| // To achieve this we would need to perform a set of ABCI queries. | ||
| // In the meantime, this value should be good enough to make sure that the | ||
| // message is included in a block. | ||
| out.gas_premium = TokenAmount::from_nano(BigInt::from(Self::DEFAULT_GAS_PREMIUM)); |
There was a problem hiding this comment.
ethers-rs does this, do we really have to do it here? This is forcing rewards onto users via estimates. Shouldn't it be up to them and the miners?
There was a problem hiding this comment.
In Lotus they have a local estimation of fees also in case the user didn't set the gas_premium. The reason for that is that you can use Lotus to send non-EVM messages directly and the user may not set the premium. I guess having a constant premium here as a sanity-check should be OK for us because we will rely on ethers and other tooling to compute this for us, so we should expect almost all messages to have gas_premium != 0.
Weak opinion weakly held. I can also bypass gas_premium here even if it is zero if you think this is the correct way of doing it.
There was a problem hiding this comment.
I don't know, I didn't think there was a correct way because the FVM happily executes with 0 fees and 0 premiums, and it should be up to the validator to make sure they only include transactions which are profitable, see #29
Personally I would not set anything that isn't necessary to be set, especially since this value is not returned from the API, so even now I'm not sure what effect we have by setting it.
There was a problem hiding this comment.
After reading the spec and tinkering with it, I think you are right, is better to just estimate the gas_limit for now, and then compute the gas_premium and gas_fee_cap optimally once we figure out the base_fee story. Unless you are against, let's defer for the future.
There was a problem hiding this comment.
Great stuff, finally we can work out what these damn gas fields are for!
I still don't fully understand why we need this.
If we set gas_fee_cap to 0, then the FVM does not do a balance check (miner pays the penalty) and one can execute a message to its fullest, and just take the gas_used as the limit. Maybe do 10% over estimation.
You said that the estimated limits weren't enough for you, so I assume you must have set the gas_fee_cap to some non-zero value, in which case you can run out of balance as well as the gas limit (but that we set to the BLOCK_GAS_LIMIT during estimation).
Couldn't we just set the fees to 0 and return the gas, and let the client worry about coming up with a fee they are willing to pay?
Signed-off-by: Alfonso de la Rocha <[email protected]>
…ard/fendermint into adlrocha/testing-tooling
| return Ok(Some(GasEstimate { | ||
| exit_code: ret.msg_receipt.exit_code, | ||
| info: ret.failure_info.map(|x| x.to_string()).unwrap_or_default(), | ||
| gas_limit: 0, |
There was a problem hiding this comment.
Should this be gas_used? In the bottom it is, even in the error case.
There was a problem hiding this comment.
I used this to make explicit that the message errored and we shouldn't use the gas estimation, but no strong opinion, we can return the gas_used even if it failed.
| if msg.sequence.is_zero() { | ||
| if let Some((_, state)) = self.actor_state(&msg.from)? { | ||
| msg.sequence = state.sequence; | ||
| } | ||
| } |
There was a problem hiding this comment.
For correctness this should be inside the with_exec_state, otherwise the state root can be different once use_cache: true has been used to execute some prior transactions. It's my bug and we can fix it in another PR.
There was a problem hiding this comment.
There was a problem hiding this comment.
You are right! Up to you I can fix it here if needed (although it may be cleaner to just start a new PR and get this over with :) )
There was a problem hiding this comment.
Thanks, created a ticket for it, let's do it there at some point.
* add eth API launch doc * update eth api doc * temp fix * fix types and cargo * support legacy transaction for estimate gas * wip: support for legacy tx in estimate gas * FM-161: add accept_legacy to to_fvm_message * Improve gas estimation logic (#163) * add some traces * improve gas estimation logic * clean comments * fix linter * remove unused println * FM-163: address review and prevent panic errors * FIX: make linter happy * FM-163: clean code and finish review * FM-163: rm prints * FM: add transaction compat to call query (#170) * FM-163: modify cache semantics in call * FM-163: fix gas_fee_cap and add fvm settings to config * FM-163: fix use_cache and config --------- Signed-off-by: Alfonso de la Rocha <[email protected]> * FM 161: minor fix * FM 161: minor fix --------- Signed-off-by: Alfonso de la Rocha <[email protected]> Co-authored-by: Alfonso de la Rocha <[email protected]> Co-authored-by: adlrocha <[email protected]>
Depends on: #161
This PR improves the gas estimation logic. Instead of directly pre-executing the message to get the estimated gas of the call we:
Without this, tools like Metamask and Remix didn't estimate the gas correctly and message never went through. This approach is not as accurate as the fine-grained one used in Lotus, but for our use case is good enough to ensure that the transaction is included in the next block.
Additionally, the gas estimation also populates the gas premium and gas limit of the transaction when not set.
For the gas estimation logic, I had to include a new
call_with_cachemethod to allow enabling and disabling the cache to determine if in the subsequent execution of message the results should be conveniently buffered or not.