Add proper gas limit check through local computation

[terencechain]

This PR adds gas limit check through local computation and compare expected gas limit against builder's header. The expected gas limit is the following with adjustment_factor set to 1024

	def expected_gas_limit(parent_gas_limit, target_gas_limit, adjustment_factor):
	 max_gas_limit_difference = (parent_gas_limit // adjustment_factor) - 1
	 if target_gas_limit > parent_gas_limit:
	     gas_diff = target_gas_limit - parent_gas_limit
	     return parent_gas_limit + min(gas_diff, max_gas_limit_difference)
	 else:
	     gas_diff = parent_gas_limit - target_gas_limit
	     return parent_gas_limit - min(gas_diff, max_gas_limit_difference)

[potuz]

targetGasLimit may be a bit confusing given that we have "gas limit" and "gas target" already mean something.

[terencechain]

Renamed it to proposerGasLimit. Same as lighthouse

[potuz]

This has an underflow if parentGasLimit < 1024, can't happen in real life though

[terencechain]

7065b2d

[potuz]

No need for an else branch here.

[terencechain]

7065b2d

[saolyn]

You don't seem to need this else either:

Suggested change

	reg, err := vs.BlockBuilder.RegistrationByValidatorID(ctx, idx)
	if err != nil {
	log.WithError(err).Warn("Proposer: failed to get registration by validator ID, could not check gas limit")
	} else {
	gasLimit := expectedGasLimit(parentGasLimit, reg.GasLimit)
	if gasLimit != header.GasLimit() {
	return nil, fmt.Errorf("incorrect header gas limit %d != %d", gasLimit, header.GasLimit())
	}
	}
	reg, err := vs.BlockBuilder.RegistrationByValidatorID(ctx, idx)
	if err != nil {
	log.WithError(err).Warn("Proposer: failed to get registration by validator ID, could not check gas limit")
	}
	gasLimit := expectedGasLimit(parentGasLimit, reg.GasLimit)
	if gasLimit != header.GasLimit() {
	return nil, fmt.Errorf("incorrect header gas limit %d != %d", gasLimit, header.GasLimit())
	}

[terencechain]

I think you need the else, we dont want to call expectedGasLimit if error

[potuz]

reg is used so if there' s an error the call to expectedGasLimit can' t be carried

[potuz]

Does the registration cache handle the case where the validator does not include a value?

[terencechain]

I think so. I went through the code as my understanding: the DefaultBuilderGasLimit is initially set in config.go:

It is then defined as 30M in mainnet_config.go:

The beacon node does not provide a flag to set this gas limit, and it is not part of the consensus spec configuration. Instead, it is hard-coded in the client. The DefaultBuilderGasLimit is also defined as a default in the validator's flag.go. If the validator does not specify the --suggested-gas-limit flag, the value in mainnet_config.go overrides it:

The DefaultBuilderGasLimit is used in proposer_setting.go's getProposerSettings function. This function displays or recreates the currently applied proposer settings under the validator's custom flags:

In loader.go, the DefaultBuilderGasLimit ensures that the default gas limit applies if the --suggested-gas-limit flag is not set. It is then saved to the proposer settings at startup when registering for the validator node service:

The SaveProposerSettings function is called within the Load method of settingsLoader in loader.go. This step saves the default gas limit to the proposer settings file:

When registering the validator client, the system ensures that the proposer settings with the correct default gas limit are stored in the database:

[aimxhaisse]

Not sure I get it right but it seems there is something confusing here, when using --proposer-settings-file at the validator level, if in the file if there is:

{
  "default_config": {
    "builder": {
      "enabled": false,
      "gas_limit": "42"
    },
    "fee_recipient": "XXXX"
  },
  "my_key": {
    "YYYY": {
      "builder": {
        "enabled": true
       "gas_limit": "43",
      },
      "fee_recipient": "YYYYY"
    },

When using --suggested-fee-recipient=ZZZZ, my_key will still propose on YYYY (proposer settings file has priority over flag).

However, when using --suggested-gas-limit=10, my_key will use the flag override instead of 43 (flag has priority for gas limit over proposer settings).

In processBuilderConfig, gas limit is overridden by the flag while the fee recipient is not:

Also, if the user specifies for my_key a gas limit but no fee-recipient, it seems no fee-recipient is used? (or maybe later in the hot-path it defaults to the flag?).

It could make sense to align both behavior (to always have the file override the flags).

[terencechain]

yes there's a bug there, similar bug: #14721
unrelated to this PR though so will merge this PR and fix that today