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.

Sign up for GitHub

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

Add proper gas limit check through local computation #14707

Merged
merged 4 commits into from
Dec 13, 2024
Merged

Add proper gas limit check through local computation #14707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c178784
Add proper gas limit check through local computation
terencechain Dec 10, 2024
77abdfd
Potuz's feedback
terencechain Dec 10, 2024
824b21c
Fix new line
terencechain Dec 10, 2024
ac6bb3d
Merge branch 'develop' into check-header-gas-limit
terencechain Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve
- Added a Prometheus error counter metric for SSE requests.
- Save light client updates and bootstraps in DB.
- Added more comprehensive tests for `BlockToLightClientHeader`. [PR](https://github.com/prysmaticlabs/prysm/pull/14699)
- Added proper gas limit check for header from the builder.
- Added an error field to log `Finished building block`.

### Changed
Expand Down
7 changes: 6 additions & 1 deletion beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,12 @@ func (vs *Server) BuildBlockParallel(ctx context.Context, sBlk interfaces.Signed
// There's no reason to try to get a builder bid if local override is true.
var builderBid builderapi.Bid
if !(local.OverrideBuilder || skipMevBoost) {
builderBid, err = vs.getBuilderPayloadAndBlobs(ctx, sBlk.Block().Slot(), sBlk.Block().ProposerIndex())
latestHeader, err := head.LatestExecutionPayloadHeader()
if err != nil {
return nil, status.Errorf(codes.Internal, "Could not get latest execution payload header: %v", err)
}
parentGasLimit := latestHeader.GasLimit()
builderBid, err = vs.getBuilderPayloadAndBlobs(ctx, sBlk.Block().Slot(), sBlk.Block().ProposerIndex(), parentGasLimit)
if err != nil {
builderGetPayloadMissCount.Inc()
log.WithError(err).Error("Could not get builder payload")
Expand Down
46 changes: 45 additions & 1 deletion beacon-chain/rpc/prysm/v1alpha1/validator/proposer_bellatrix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ var emptyTransactionsRoot = [32]byte{127, 254, 36, 30, 166, 1, 135, 253, 176, 24
// blockBuilderTimeout is the maximum amount of time allowed for a block builder to respond to a
// block request. This value is known as `BUILDER_PROPOSAL_DELAY_TOLERANCE` in builder spec.
const blockBuilderTimeout = 1 * time.Second
const gasLimitAdjustmentFactor = 1024

// Sets the execution data for the block. Execution data can come from local EL client or remote builder depends on validator registration and circuit breaker conditions.
func setExecutionData(ctx context.Context, blk interfaces.SignedBeaconBlock, local *blocks.GetPayloadResponse, bid builder.Bid, builderBoostFactor primitives.Gwei) (primitives.Wei, *enginev1.BlobsBundle, error) {
Expand Down Expand Up @@ -170,7 +171,11 @@ func setExecutionData(ctx context.Context, blk interfaces.SignedBeaconBlock, loc

// This function retrieves the payload header and kzg commitments given the slot number and the validator index.
// It's a no-op if the latest head block is not versioned bellatrix.
func (vs *Server) getPayloadHeaderFromBuilder(ctx context.Context, slot primitives.Slot, idx primitives.ValidatorIndex) (builder.Bid, error) {
func (vs *Server) getPayloadHeaderFromBuilder(
ctx context.Context,
slot primitives.Slot,
idx primitives.ValidatorIndex,
parentGasLimit uint64) (builder.Bid, error) {
ctx, span := trace.StartSpan(ctx, "ProposerServer.getPayloadHeaderFromBuilder")
defer span.End()

Expand Down Expand Up @@ -243,6 +248,16 @@ func (vs *Server) getPayloadHeaderFromBuilder(ctx context.Context, slot primitiv
return nil, fmt.Errorf("incorrect parent hash %#x != %#x", header.ParentHash(), h.BlockHash())
}

reg, err := vs.BlockBuilder.RegistrationByValidatorID(ctx, idx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. I went through the code as my understanding: the DefaultBuilderGasLimit is initially set in config.go:
Screenshot 2024-12-12 at 10 50 14 AM

It is then defined as 30M in mainnet_config.go:
Screenshot 2024-12-12 at 10 49 55 AM

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:
Screenshot 2024-12-12 at 10 52 27 AM

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:
Screenshot 2024-12-12 at 10 54 43 AM

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:
Screenshot 2024-12-12 at 11 00 58 AM

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:
Screenshot 2024-12-12 at 11 04 13 AM
Screenshot 2024-12-12 at 11 04 41 AM

When registering the validator client, the system ensures that the proposer settings with the correct default gas limit are stored in the database:
Screenshot 2024-12-12 at 11 05 43 AM

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Screenshot 2024-12-13 at 10 53 22

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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())
}
}
Comment on lines +251 to +259
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


t, err := slots.ToTime(uint64(vs.TimeFetcher.GenesisTime().Unix()), slot)
if err != nil {
return nil, err
Expand Down Expand Up @@ -393,3 +408,32 @@ func setExecution(blk interfaces.SignedBeaconBlock, execution interfaces.Executi

return nil
}

// Calculates expected gas limit based on parent gas limit and target gas limit.
// Spec code:
//
// 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)
func expectedGasLimit(parentGasLimit, proposerGasLimit uint64) uint64 {
maxGasLimitDiff := uint64(0)
if parentGasLimit > gasLimitAdjustmentFactor {
maxGasLimitDiff = parentGasLimit/gasLimitAdjustmentFactor - 1
}
if proposerGasLimit > parentGasLimit {
if proposerGasLimit-parentGasLimit > maxGasLimitDiff {
return parentGasLimit + maxGasLimitDiff
}
return proposerGasLimit
}

if parentGasLimit-proposerGasLimit > maxGasLimitDiff {
return parentGasLimit - maxGasLimitDiff
}
return proposerGasLimit
}
Loading
Loading