Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 7 additions & 5 deletions consensus/cometbft/service/prepare_proposal.go

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think payload builders should still be able to send an empty block if their EL went fatal. Something to discuss further

@shotes shotes Mar 19, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

An empty proposed block would get broadcasted, rejected by validators, and then prevoted nil. Whereas a missed proposed would timeout and get prevoted nil**. The behavior at consensus level is the same, but we're adding extra network congestion by broadcasting empty proposal. Why would we do that if we don't have to?

**From comet spec:

Else, if the proposal is invalid or wasn’t received on time, it prevotes <nil>.

Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"time"

"github.com/berachain/beacon-kit/consensus/types"
"github.com/berachain/beacon-kit/errors"
"github.com/berachain/beacon-kit/primitives/math"
cmtabci "github.com/cometbft/cometbft/abci/types"
)
Expand Down Expand Up @@ -71,17 +72,18 @@ func (s *Service) prepareProposal(
s.prepareProposalState.Context(),
slotData,
)
if err != nil {
switch {
case errors.IsFatal(err):
return nil, err
case err != nil:
s.logger.Error(
"failed to prepare proposal",
"height", req.Height,
"time", req.Time,
"err", err,
)
return &cmtabci.PrepareProposalResponse{Txs: [][]byte{}}, nil
default:
return &cmtabci.PrepareProposalResponse{Txs: [][]byte{blkBz, sidecarsBz}}, nil
}

return &cmtabci.PrepareProposalResponse{
Txs: [][]byte{blkBz, sidecarsBz},
}, nil
}
14 changes: 9 additions & 5 deletions consensus/cometbft/service/process_proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"fmt"
"time"

"github.com/berachain/beacon-kit/errors"
cmtabci "github.com/cometbft/cometbft/abci/types"
)

Expand Down Expand Up @@ -68,20 +69,23 @@ func (s *Service) processProposal(
// errors to consensus indicate that the node was not able to understand
// whether the block was valid or not. Viceversa, we signal that a block
// is invalid by its status, but we do return nil error in such a case.
status := cmtabci.PROCESS_PROPOSAL_STATUS_ACCEPT
err := s.Blockchain.ProcessProposal(
s.processProposalState.Context(),
req,
)
if err != nil {
status = cmtabci.PROCESS_PROPOSAL_STATUS_REJECT
switch {
case errors.IsFatal(err):
return nil, err
case err != nil:
s.logger.Error(
"failed to process proposal",
"proposal rejected",
"height", req.Height,
"time", req.Time,
"hash", fmt.Sprintf("%X", req.Hash),
"err", err,
)
return &cmtabci.ProcessProposalResponse{Status: cmtabci.PROCESS_PROPOSAL_STATUS_REJECT}, nil
default:
return &cmtabci.ProcessProposalResponse{Status: cmtabci.PROCESS_PROPOSAL_STATUS_ACCEPT}, nil
}
return &cmtabci.ProcessProposalResponse{Status: status}, nil
}
37 changes: 11 additions & 26 deletions errors/mod.go → errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,14 @@ type DetailedError struct {
fatal bool
}

// WrapNonFatal returns the error message.
func WrapNonFatal(err error) error {
return &DetailedError{
error: err,
fatal: false,
}
// Error returns a string representation.
func (e *DetailedError) Error() string {
return e.error.Error()
}

// Unwrap returns the wrapped error.
func (e *DetailedError) Unwrap() error {
return e.error
}

// WrapFatal creates a new DetailedError with the
Expand All @@ -78,7 +80,7 @@ func WrapFatal(err error) error {
// IsFatal checks if the provided error is a
// DetailedError and if it is fatal.
func IsFatal(err error) bool {
// If the error is nil, obviouisly it is not fatal.
// If the error is nil, obviously it is not fatal.
if err == nil {
return false
}
Expand All @@ -100,23 +102,6 @@ func IsFatal(err error) bool {
return customErr.fatal
}

// All other errors are fatal.
return true
}

// JoinFatal checks if any of the provided errors is a
// DetailedError and if it is fatal.
func JoinFatal(errs ...error) error {
fatal := false
for _, err := range errs {
if IsFatal(err) {
fatal = true
break
}
}
retErr := stderrors.Join(errs...)
if fatal {
return WrapFatal(retErr)
}
return WrapNonFatal(retErr)
// Only declared fatal errors are fatal.
return false
}
4 changes: 2 additions & 2 deletions execution/engine/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func (ee *Engine) NotifyForkchoiceUpdate(
"err", err,
)
ee.metrics.markForkchoiceUpdateFatalError(err)
return nil, backoff.Permanent(err)
return nil, backoff.Permanent(errors.WrapFatal(err))

default:
ee.logger.Info(
Expand Down Expand Up @@ -245,7 +245,7 @@ func (ee *Engine) NotifyNewPayload(
lastValidHash = &common.ExecutionHash{}
}
ee.metrics.markNewPayloadFatalError(payloadHash, *lastValidHash, err)
return nil, backoff.Permanent(err)
return nil, backoff.Permanent(errors.WrapFatal(err))
default:
ee.logger.Error(
"NotifyNewPayload: EL returns unknown error.",
Expand Down
4 changes: 2 additions & 2 deletions testing/simulated/chaos_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ func (s *SimulatedSuite) TestProcessProposal_CrashedExecutionClient_Errors() {
ProposerAddress: pubkey.Address(),
Time: proposalTime,
})
s.Require().NoError(err)
s.Require().Equal(types.PROCESS_PROPOSAL_STATUS_REJECT, processResp.Status)
s.Require().Error(err)
s.Require().Nil(processResp)
s.Require().Contains(s.LogBuffer.String(), client.ErrBadConnection.Error())
}

Expand Down