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

fix(state-transition): skip execution payload timestamp validation on Bartio #2147

Merged
merged 10 commits into from
Nov 13, 2024
2 changes: 1 addition & 1 deletion beacond/cmd/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ func DefaultComponents() []any {
*BeaconBlock, *BeaconBlockBody, *BeaconBlockHeader,
],
components.ProvideStateProcessor[
*BeaconBlock, *BeaconBlockBody, *BeaconBlockHeader,
*Logger, *BeaconBlock, *BeaconBlockBody, *BeaconBlockHeader,
*BeaconState, *BeaconStateMarshallable, *Deposit, *ExecutionPayload,
*ExecutionPayloadHeader, *KVStore,
],
Expand Down
1 change: 1 addition & 0 deletions mod/beacon/blockchain/process.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ func (s *Service[

ProposerAddress: blk.GetProposerAddress(),
NextPayloadTimestamp: blk.GetNextPayloadTimestamp(),
ConsensusBlockHeight: blk.GetConsensusBlockHeight(),
},
st,
blk.GetBeaconBlock(),
Expand Down
1 change: 1 addition & 0 deletions mod/beacon/blockchain/receive.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ func (s *Service[
SkipValidateRandao: false,
ProposerAddress: blk.GetProposerAddress(),
NextPayloadTimestamp: blk.GetNextPayloadTimestamp(),
ConsensusBlockHeight: blk.GetConsensusBlockHeight(),
},
st, blk.GetBeaconBlock(),
)
Expand Down
5 changes: 5 additions & 0 deletions mod/beacon/blockchain/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ type ConsensusBlock[BeaconBlockT any] interface {
// consensus for the next payload to be proposed. It is also
// used to bound current payload upon validation
GetNextPayloadTimestamp() math.U64

// GetConsensusBlockHeight returns the height of consensus block,
// which may be different from execution payload height
// in some networks. Currently only used for logging
GetConsensusBlockHeight() math.U64
abi87 marked this conversation as resolved.
Show resolved Hide resolved
}

// BeaconBlock represents a beacon block interface.
Expand Down
5 changes: 5 additions & 0 deletions mod/beacon/validator/block_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ func (s *Service[
ctx,
slotData.GetProposerAddress(),
slotData.GetNextPayloadTimestamp(),
slotData.GetConsensusBlockHeight(),
st,
blk,
)
Expand Down Expand Up @@ -339,13 +340,15 @@ func (s *Service[
ctx context.Context,
proposerAddress []byte,
nextPayloadTimestamp math.U64,
consensusBlockHeight math.U64,
st BeaconStateT,
blk BeaconBlockT,
) error {
stateRoot, err := s.computeStateRoot(
ctx,
proposerAddress,
abi87 marked this conversation as resolved.
Show resolved Hide resolved
nextPayloadTimestamp,
consensusBlockHeight,
st,
blk,
)
Expand All @@ -368,6 +371,7 @@ func (s *Service[
ctx context.Context,
proposerAddress []byte,
nextPayloadTimestamp math.U64,
consensusBlockHeight math.U64,
st BeaconStateT,
blk BeaconBlockT,
) (common.Root, error) {
Expand All @@ -385,6 +389,7 @@ func (s *Service[
SkipValidateRandao: true,
ProposerAddress: proposerAddress,
NextPayloadTimestamp: nextPayloadTimestamp,
ConsensusBlockHeight: consensusBlockHeight,
},
st, blk,
); err != nil {
Expand Down
5 changes: 5 additions & 0 deletions mod/beacon/validator/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,11 @@ type SlotData[AttestationDataT, SlashingInfoT any] interface {
// consensus for the next payload to be proposed. It is also
// used to bound current payload upon validation
GetNextPayloadTimestamp() math.U64

// GetConsensusBlockHeight returns the height of consensus block,
// which may be different from execution payload height
// in some networks. Currently only used for logging.
GetConsensusBlockHeight() math.U64
abi87 marked this conversation as resolved.
Show resolved Hide resolved
}

// StateProcessor defines the interface for processing the state.
Expand Down
30 changes: 30 additions & 0 deletions mod/config/pkg/spec/special_cases.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// SPDX-License-Identifier: BUSL-1.1
//
// Copyright (C) 2024, Berachain Foundation. All rights reserved.
// Use of this software is governed by the Business Source License included
// in the LICENSE file of this repository and at www.mariadb.com/bsl11.
//
// ANY USE OF THE LICENSED WORK IN VIOLATION OF THIS LICENSE WILL AUTOMATICALLY
// TERMINATE YOUR RIGHTS UNDER THIS LICENSE FOR THE CURRENT AND ALL OTHER
// VERSIONS OF THE LICENSED WORK.
//
// THIS LICENSE DOES NOT GRANT YOU ANY RIGHT IN ANY TRADEMARK OR LOGO OF
// LICENSOR OR ITS AFFILIATES (PROVIDED THAT YOU MAY USE A TRADEMARK OR LOGO OF
// LICENSOR AS EXPRESSLY REQUIRED BY THIS LICENSE).
//
// TO THE EXTENT PERMITTED BY APPLICABLE LAW, THE LICENSED WORK IS PROVIDED ON
// AN “AS IS” BASIS. LICENSOR HEREBY DISCLAIMS ALL WARRANTIES AND CONDITIONS,
// EXPRESS OR IMPLIED, INCLUDING (WITHOUT LIMITATION) WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE, NON-INFRINGEMENT, AND
// TITLE.

package spec

// Special cased Bartio for some ad-hoc handling due to the way
// some bugs were handled on Bartio. To be removed.
const (
abi87 marked this conversation as resolved.
Show resolved Hide resolved
BartioChainID uint64 = 80084
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just consolidated variables here


//nolint:lll // temporary.
BArtioValRoot = "0x9147586693b6e8faa837715c0f3071c2000045b54233901c2e7871b15872bc43"
)
abi87 marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 2 additions & 2 deletions mod/consensus-types/pkg/types/payload.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
package types

import (
"github.com/berachain/beacon-kit/mod/config/pkg/spec"
engineprimitives "github.com/berachain/beacon-kit/mod/engine-primitives/pkg/engine-primitives"
"github.com/berachain/beacon-kit/mod/errors"
"github.com/berachain/beacon-kit/mod/primitives/pkg/bytes"
Expand Down Expand Up @@ -569,8 +570,7 @@ func (p *ExecutionPayload) ToHeader(

// TODO: This is live on bArtio with a bug and needs to be hardforked
// off of. This is a temporary solution to avoid breaking changes.
//nolint:mnd // don't want the circular dep.
if eth1ChainID == 80084 {
if eth1ChainID == spec.BartioChainID {
txsRoot = engineprimitives.BartioTransactions(
p.GetTransactions(),
).HashTreeRoot()
Expand Down
12 changes: 12 additions & 0 deletions mod/consensus/pkg/types/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,14 @@ package types
import "github.com/berachain/beacon-kit/mod/primitives/pkg/math"

type commonConsensusData struct {
// use to verify block builder
proposerAddress []byte

// used to build next block and validate current payload timestamp
nextPayloadTimestamp math.U64
abi87 marked this conversation as resolved.
Show resolved Hide resolved

// useful for logging
consensusBlkHeight math.U64
abi87 marked this conversation as resolved.
Show resolved Hide resolved
}

// GetProposerAddress returns the address of the validator
Expand All @@ -40,3 +45,10 @@ func (c *commonConsensusData) GetProposerAddress() []byte {
func (c *commonConsensusData) GetNextPayloadTimestamp() math.U64 {
return c.nextPayloadTimestamp
}

// GetConsensusBlockHeight returns the height of consensus block,
// which may be different from execution payload height
// in some networks. Currently only used for logging.
func (c *commonConsensusData) GetConsensusBlockHeight() math.U64 {
return c.consensusBlkHeight
}
5 changes: 5 additions & 0 deletions mod/node-core/pkg/components/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,11 @@ type (
// consensus for the next payload to be proposed. It is also
// used to bound current payload upon validation
GetNextPayloadTimestamp() math.U64

// GetConsensusBlockHeight returns the height of consensus block,
// which may be different from execution payload height
// in some networks. Currently only used for logging
GetConsensusBlockHeight() math.U64
}

// BeaconBlock represents a generic interface for a beacon block.
Expand Down
9 changes: 8 additions & 1 deletion mod/node-core/pkg/components/state_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"cosmossdk.io/depinject"
engineprimitives "github.com/berachain/beacon-kit/mod/engine-primitives/pkg/engine-primitives"
"github.com/berachain/beacon-kit/mod/execution/pkg/engine"
"github.com/berachain/beacon-kit/mod/log"
"github.com/berachain/beacon-kit/mod/primitives/pkg/common"
"github.com/berachain/beacon-kit/mod/primitives/pkg/crypto"
"github.com/berachain/beacon-kit/mod/state-transition/pkg/core"
Expand All @@ -32,6 +33,7 @@ import (
// StateProcessorInput is the input for the state processor for the depinject
// framework.
type StateProcessorInput[
LoggerT log.AdvancedLogger[LoggerT],
calbera marked this conversation as resolved.
Show resolved Hide resolved
ExecutionPayloadT ExecutionPayload[
ExecutionPayloadT, ExecutionPayloadHeaderT, WithdrawalsT,
],
Expand All @@ -40,6 +42,7 @@ type StateProcessorInput[
WithdrawalsT Withdrawals[WithdrawalT],
] struct {
depinject.In
Logger LoggerT
ChainSpec common.ChainSpec
ExecutionEngine *engine.Engine[
ExecutionPayloadT,
Expand All @@ -53,6 +56,7 @@ type StateProcessorInput[
// ProvideStateProcessor provides the state processor to the depinject
// framework.
func ProvideStateProcessor[
LoggerT log.AdvancedLogger[LoggerT],
BeaconBlockT BeaconBlock[BeaconBlockT, BeaconBlockBodyT, BeaconBlockHeaderT],
BeaconBlockBodyT BeaconBlockBody[
BeaconBlockBodyT, *AttestationData, DepositT,
Expand All @@ -78,7 +82,9 @@ func ProvideStateProcessor[
WithdrawalT Withdrawal[WithdrawalT],
](
in StateProcessorInput[
ExecutionPayloadT, ExecutionPayloadHeaderT, WithdrawalT, WithdrawalsT,
LoggerT,
ExecutionPayloadT, ExecutionPayloadHeaderT,
WithdrawalT, WithdrawalsT,
],
) *core.StateProcessor[
BeaconBlockT, BeaconBlockBodyT, BeaconBlockHeaderT,
Expand All @@ -105,6 +111,7 @@ func ProvideStateProcessor[
WithdrawalsT,
WithdrawalCredentials,
](
in.Logger.With("service", "state-processor"),
in.ChainSpec,
in.ExecutionEngine,
in.Signer,
Expand Down
12 changes: 12 additions & 0 deletions mod/primitives/pkg/transition/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ type Context struct {
// consensus for the next payload to be proposed. It is also
// used to bound current payload upon validation
NextPayloadTimestamp math.U64

// ConsensusBlockHeight is the height of consensus block,
// which may be different from execution payload height
// in some networks. Currently only used for logging
ConsensusBlockHeight math.U64
}

// GetOptimisticEngine returns whether to optimistically assume the execution
Expand Down Expand Up @@ -88,6 +93,13 @@ func (c *Context) GetNextPayloadTimestamp() math.U64 {
return c.NextPayloadTimestamp
}

// GetConsensusBlockHeight returns the height of consensus block,
// which may be different from execution payload height
// in some networks. Currently only used for logging.
func (c *Context) GetConsensusBlockHeight() math.U64 {
return c.ConsensusBlockHeight
}

// Unwrap returns the underlying standard context.
func (c *Context) Unwrap() context.Context {
return c.Context
Expand Down
2 changes: 2 additions & 0 deletions mod/state-transition/pkg/core/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
storetypes "cosmossdk.io/store/types"
"github.com/berachain/beacon-kit/mod/consensus-types/pkg/types"
engineprimitives "github.com/berachain/beacon-kit/mod/engine-primitives/pkg/engine-primitives"
"github.com/berachain/beacon-kit/mod/log/pkg/noop"
"github.com/berachain/beacon-kit/mod/node-core/pkg/components"
"github.com/berachain/beacon-kit/mod/primitives/pkg/common"
"github.com/berachain/beacon-kit/mod/primitives/pkg/crypto"
Expand Down Expand Up @@ -130,6 +131,7 @@ func createStateProcessor(
engineprimitives.Withdrawals,
types.WithdrawalCredentials,
](
noop.NewLogger[any](),
cs,
execEngine,
signer,
Expand Down
5 changes: 5 additions & 0 deletions mod/state-transition/pkg/core/state_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"bytes"

"github.com/berachain/beacon-kit/mod/errors"
"github.com/berachain/beacon-kit/mod/log"
"github.com/berachain/beacon-kit/mod/primitives/pkg/common"
"github.com/berachain/beacon-kit/mod/primitives/pkg/constants"
"github.com/berachain/beacon-kit/mod/primitives/pkg/crypto"
Expand Down Expand Up @@ -77,6 +78,8 @@ type StateProcessor[
},
WithdrawalCredentialsT ~[32]byte,
] struct {
// logger is used for logging information and errors.
logger log.Logger
// cs is the chain specification for the beacon chain.
cs common.ChainSpec
// signer is the BLS signer used for cryptographic operations.
Expand Down Expand Up @@ -140,6 +143,7 @@ func NewStateProcessor[
},
WithdrawalCredentialsT ~[32]byte,
](
logger log.Logger,
cs common.ChainSpec,
executionEngine ExecutionEngine[
ExecutionPayloadT, ExecutionPayloadHeaderT, WithdrawalsT,
Expand All @@ -158,6 +162,7 @@ func NewStateProcessor[
ExecutionPayloadHeaderT, ForkT, ForkDataT, KVStoreT, ValidatorT,
ValidatorsT, WithdrawalT, WithdrawalsT, WithdrawalCredentialsT,
]{
logger: logger,
cs: cs,
executionEngine: executionEngine,
signer: signer,
Expand Down
11 changes: 3 additions & 8 deletions mod/state-transition/pkg/core/state_processor_genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
package core

import (
"github.com/berachain/beacon-kit/mod/config/pkg/spec"
"github.com/berachain/beacon-kit/mod/primitives/pkg/common"
"github.com/berachain/beacon-kit/mod/primitives/pkg/constants"
"github.com/berachain/beacon-kit/mod/primitives/pkg/encoding/hex"
Expand All @@ -29,12 +30,6 @@ import (
"github.com/berachain/beacon-kit/mod/primitives/pkg/version"
)

//nolint:lll // temporary.
const (
bArtioValRoot = "0x9147586693b6e8faa837715c0f3071c2000045b54233901c2e7871b15872bc43"
bArtioChainID = 80084
)

// InitializePreminedBeaconStateFromEth1 initializes the beacon state.
//
//nolint:gocognit,funlen // todo fix.
Expand Down Expand Up @@ -119,9 +114,9 @@ func (sp *StateProcessor[
}

// Handle special case bartio genesis.
if sp.cs.DepositEth1ChainID() == bArtioChainID {
if sp.cs.DepositEth1ChainID() == spec.BartioChainID {
if err = st.SetGenesisValidatorsRoot(
common.Root(hex.MustToBytes(bArtioValRoot))); err != nil {
common.Root(hex.MustToBytes(spec.BArtioValRoot))); err != nil {
abi87 marked this conversation as resolved.
Show resolved Hide resolved
return nil, err
}
} else if err = st.
Expand Down
30 changes: 23 additions & 7 deletions mod/state-transition/pkg/core/state_processor_payload.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ package core
import (
"context"

"github.com/berachain/beacon-kit/mod/config/pkg/spec"
engineprimitives "github.com/berachain/beacon-kit/mod/engine-primitives/pkg/engine-primitives"
"github.com/berachain/beacon-kit/mod/errors"
"github.com/berachain/beacon-kit/mod/primitives/pkg/math"
Expand All @@ -46,6 +47,14 @@ func (sp *StateProcessor[
g, gCtx = errgroup.WithContext(context.Background())
)

sp.logger.Info("processExecutionPayload",
"consensus height", ctx.GetConsensusBlockHeight().Unwrap(),
"payload height", payload.GetNumber().Unwrap(),
"payload timestamp", payload.GetTimestamp().Unwrap(),
"bound timestamp", ctx.GetNextPayloadTimestamp().Unwrap(),
"skip payload verification", ctx.GetSkipPayloadVerification(),
)
calbera marked this conversation as resolved.
Show resolved Hide resolved

// Skip payload verification if the context is configured as such.
if !ctx.GetSkipPayloadVerification() {
g.Go(func() error {
Expand Down Expand Up @@ -90,7 +99,10 @@ func (sp *StateProcessor[
nextPayloadTimestamp math.U64,
optimisticEngine bool,
) error {
if err := sp.validateStatelessPayload(blk, nextPayloadTimestamp); err != nil {
if err := sp.validateStatelessPayload(
blk,
nextPayloadTimestamp,
); err != nil {
return err
}
return sp.validateStatefulPayload(ctx, st, blk, optimisticEngine)
Expand All @@ -107,12 +119,16 @@ func (sp *StateProcessor[
body := blk.GetBody()
payload := body.GetExecutionPayload()

if pt := payload.GetTimestamp(); pt >= nextPayloadTimestamp {
return errors.Wrapf(
ErrTooFarInTheFuture,
"payload timestamp, max: %d, got: %d",
nextPayloadTimestamp, pt,
)
// We skip timestamp check on Bartio for backward compatibility reasons
// TODO: enforce the check when we drop other Bartio special cases.
if sp.cs.DepositEth1ChainID() != spec.BartioChainID {
if pt := payload.GetTimestamp(); pt >= nextPayloadTimestamp {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be just >? Why are we failing on the == case?

Copy link
Collaborator Author

@abi87 abi87 Nov 13, 2024

Choose a reason for hiding this comment

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

This is done to guarantee strict monotonicity of payload timestamp and it works for the following reason.
NextPayloadTimestamp is set to consensus_block.Time + 1 sec, while payload time is equal to:

  • parent_consensus_block.Time + 1 sec if payload is built optimistically (i.e. built while parent_consensus_block was being verified)
  • consensus_block.Time is payload is built non-optimistically (i.e. built just in time when PrepareBlock request arrived for consensus_block.

Note that consensus_block.Time-parent_consensus_block.Time > 1 sec.
So in both cases the bound is strict because

payload_time <= max(parent_consensus_block.Time + 1 sec, consensus_block.Time) < consensus_block.Time + 1 sec == nextPayloadTime

LMK if this is clear and if you see any error. I really should draw some documentation around this, as soon as I have a second

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What we should do that don't do currently is validate consensus configs and making sure that no block can be finalized before 1 second.
This is in line with current specs and it is reasonable I think, but not enforce in code.

return errors.Wrapf(
ErrTooFarInTheFuture,
"payload timestamp, max: %d, got: %d",
nextPayloadTimestamp, pt,
)
}
}

// Verify the number of withdrawals.
Expand Down
Loading
Loading