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

Support setting the sanitization window to 0 #306

Open
Siegrift opened this issue May 14, 2024 · 8 comments
Open

Support setting the sanitization window to 0 #306

Siegrift opened this issue May 14, 2024 · 8 comments

Comments

@Siegrift
Copy link
Collaborator

Siegrift commented May 14, 2024

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 Siegrift added the on hold We do not plan to address this at the moment label May 14, 2024
@Siegrift Siegrift removed the on hold We do not plan to address this at the moment label May 22, 2024
@Siegrift
Copy link
Collaborator Author

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.

@Siegrift Siegrift self-assigned this May 22, 2024
@bdrhn9 bdrhn9 assigned bdrhn9 and unassigned Siegrift May 23, 2024
@bdrhn9
Copy link
Contributor

bdrhn9 commented May 28, 2024

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.

@bdrhn9 bdrhn9 closed this as completed May 28, 2024
@Siegrift
Copy link
Collaborator Author

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.

@Siegrift Siegrift reopened this May 28, 2024
@bdrhn9
Copy link
Contributor

bdrhn9 commented May 28, 2024

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
Copy link
Collaborator Author

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
Copy link
Contributor

bdrhn9 commented May 28, 2024

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
Copy link
Collaborator Author

Siegrift commented May 28, 2024

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

Also, changed the issue title.

@Siegrift Siegrift changed the title Use base fee as override after sanitization Support setting the sanitization window to 0 May 28, 2024
@bdrhn9
Copy link
Contributor

bdrhn9 commented May 28, 2024

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.

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

No branches or pull requests

2 participants