api3dao / airseeker

A service powering the dAPIs
MIT License
2 stars 5 forks source link

Support setting the sanitization window to 0 #306

Open Siegrift opened 4 months ago

Siegrift commented 4 months ago

NOTE: The issue started as a task to use base fee from the RPC as the lower bound after the sanitization. The decision was later changed, and now the issue support sanitization window set to 0.

PREVIOUS DESCRIPTION: We discussed on call, that it's possible that the RPC node returns gas price which is lower than the base fee. That said, so far all occurrences of the "gas price lower than base fee" were seen after we've sanitised the gas price. The disadvantage of this could be that we submit the tx with a high gas price (e.g. in case there is an issue with RPC and it reports wrong base fee).

What we could do is get the gas price and base fee from the RPC, sanitize it and then use max(sanitized_gas_price, base_fee).

Siegrift commented 3 months ago

Agreed on the call to implement this, but log the whenever this sanitization happens. There should be an alert created for this per chain. There is a possibility that the base fee reported by the RPC will be too large, causing us to drain the wallet. This is quite unprobable, but we will see how this works in practice.

bdrhn9 commented 3 months ago

We also discussed this issue with @bbenligiray . This problem arises specifically on arbitrum, where transactions fail to be submitted due to continuously increasing gas prices as we attempt to sanitize gas prices.

The main issue is that unlike other chains, arbitrum's base fee is equivalent to the legacy gas price from eth_gasPrice, meaning the priority fee is always zero. Although priority fee is zero, the chain utilizes eip-1559 and rejects the transaction if tx gas price under the base fee. On other chains, during gas price spikes (e.g., 100x), most of the increased cost comes from the priority fee, while the base fee can only increase by 12.5% per block. This allows us to continue submitting transactions, even if the txes aren't immediately mined.

Even if we implement a solution for this issue, our transactions may still not be submitted on arbitrum. This is because the base fee, even if used as a bottom limit, has the potential to increase before we submit the transaction. So implementing this feature redundant to complicate our gas oracle.

Fwiw, some chains don't include the base fee attribute in their eth_getBlockByNumber call. So for some chains, our eth_getBlockByNumber will be made for nothing.

To solve this problem arbitrum like chains, it's better to disable sanitization by setting sanitizationWindow to 0.

Siegrift commented 3 months ago

On other chains, during gas price spikes (e.g., 100x), most of the increased cost comes from the priority fee, while the base fee can only increase by 12.5% per block.

While this is a good point, I don't think we will be able to submit a transaction during these heavy gas spikes because of the sanitization. This shouldn't matter that much though, because we can expect the OEV update to be done by the searcher if the update is important.

This is because the base fee, even if used as a bottom limit, has the potential to increase before we submit the transaction. So implementing this feature redundant to complicate our gas oracle.

Yeah, good point. I guess we would want to have some factor of the base fee (e.g. 150% of the provider returned base fee) applied. But that is another complexity increase.

To solve this problem arbitrum like chains, it's better to disable sanitization by setting sanitizationWindow to 0.

Yeah, makes sense to me. And I am happy that we avoid the complexity 💪 .

That said, I checked the implementation and we are not really supporting sanitization window set to 0 because of this. Also here we may purge the currently fetched gas price (pretty rare case as the check works with seconds) and we'll also do this warning.

bdrhn9 commented 3 months ago

I don't think we will be able to submit a transaction during these heavy gas spikes because of the sanitization.

We talked about to update sanitization parameters, I forgot to mention.

{
 sanitizationMultiplier: 5 -> 10,
 sanitizationSamplingWindow: 600 -> 300,
}

Both update on the parameters will help to submit transactions on congested situations.

That said, I checked the implementation and we are not really supporting sanitization window set to 0 because of this. Also here we may purge the currently fetched gas price (pretty rare case as the check works with seconds) and we'll also do this warning.

How about setting sanitizationPercentile to 100? If you see here, if we set sanitizationPercentile to 100 and sanitizationMultiplier more than or equal to 1, sanitization will never happen. What do you think about this work around?

Siegrift commented 3 months ago

We talked about to update sanitization parameters, I forgot to mention.

All chains, Including testnets?

What do you think about this work around?

Yeah, setting sanitizationPercentile to 100 should work. But set the sanitizationMultiplier to something larger, e.g. 100 to avoid edge cases due to integer division.

I'd probably still keep the issue open, because the workaround moves the complexity to the configuration. But we don't need to implement it now.

bdrhn9 commented 3 months ago

All chains, Including testnets?

All chains that we want to utilize sanitization.

Yeah, setting sanitizationPercentile to 100 should work. But set the sanitizationMultiplier to something larger, e.g. 100 to avoid edge cases due to integer division.

👌🏼

Siegrift commented 3 months ago

Moved to backlog and unassigned @bdrhn9 based on the discussion above.

Also, changed the issue title.

bdrhn9 commented 3 months ago

https://github.com/api3dao/airseeker-v2/blob/adf258517695ebc76161668fd84c5717689cfae6/src/config/schema.ts#L49

Also setting sanitizationPercentile to 100 isn't allowed. But I just used 99.999 which is okay because we are ceiling the value here.