builder gas limit & some refactoring #6583
Conversation
| @@ -1423,6 +1442,10 @@ const fn default_max_per_epoch_activation_churn_limit() -> u64 { | |||
| 8 | |||
| } | |||
|
|
|||
| const fn default_gas_limit_adjustment_factor() -> u64 { | |||
| 1024 | |||
There was a problem hiding this comment.
| @@ -1296,6 +1311,10 @@ pub struct Config { | |||
| #[serde(with = "serde_utils::address_hex")] | |||
| deposit_contract_address: Address, | |||
|
|
|||
| #[serde(default = "default_gas_limit_adjustment_factor")] | |||
| #[serde(with = "serde_utils::quoted_u64")] | |||
| gas_limit_adjustment_factor: u64, | |||
There was a problem hiding this comment.
I wonder if we should leave this out of Config so that users can't set it? It's not defined as part of the upstream config.yaml that the EF uses, so might make sense not to add it here as a non-standard config.
Then again, for most configs it just won't be present so this doesn't pose an issue.
| // Update `gas_limit` if `updated.gas_limit` is `Some` and: | ||
| // - `self.gas_limit` is `None`, or | ||
| // - both are `Some` but the values differ. |
There was a problem hiding this comment.
Just to confirm, the idea here is to make any previous gas limit "sticky"? So if we receive a validator registration that has a gas limit, followed by a duplicate registration without a gas limit, we remember the gas limit after processing the 2nd registration rather than overwriting it with None?
| let max_gas_limit_difference = parent_gas_limit | ||
| .checked_div(spec.gas_limit_adjustment_factor) | ||
| .and_then(|result| result.checked_sub(1)) | ||
| .unwrap_or(0); | ||
|
|
||
| // Adjust the gas limit safely | ||
| if target_gas_limit > parent_gas_limit { | ||
| let gas_diff = target_gas_limit.saturating_sub(parent_gas_limit); | ||
| parent_gas_limit.checked_add(std::cmp::min(gas_diff, max_gas_limit_difference)) | ||
| } else { | ||
| let gas_diff = parent_gas_limit.saturating_sub(target_gas_limit); | ||
| parent_gas_limit.checked_sub(std::cmp::min(gas_diff, max_gas_limit_difference)) | ||
| } | ||
| } |
There was a problem hiding this comment.
I've checked this against the spec for check_gas_limit and I am convinced this is correct.
It's unfortunate that the spec uses max_adjustment_delta for an inclusive (>=) check, meaning that the actual max is 1 less than the "max adjustment delta". Oh well.
|
@mergify queue |
🛑 The pull request has been removed from the queue
|
|
@mergify dequeue |
|
This pull request has been removed from the queue for the following reason: Pull request #6583 has been dequeued by a You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
✅ The pull request has been removed from the queue
|
|
@mergify requeue |
✅ This pull request will be re-embarked automaticallyThe followup |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 86891e6 |
I had forgotten to finish this one when I got distracted by other stuff but I think it's good to go now.
Lighthouse now enforces validator sovereignty over the gas limit. The overwhelming majority of builders already respect this and I've notified the relays for the few that didn't and they told me they've fixed the bug. I also did some refactoring for clarity and convenience.