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

Jump to bottom

chore(cometBFTService): simplified CometBFTService #2026

Merged
merged 20 commits into from
Oct 1, 2024
Merged

chore(cometBFTService): simplified CometBFTService #2026

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
445b8b7
finalized state cleanup
abi87 Sep 28, 2024
7e66eed
nit
abi87 Sep 28, 2024
affdafa
consolidated height error
abi87 Sep 28, 2024
cfe9f30
nit
abi87 Sep 28, 2024
7189e64
fixed initChainer
abi87 Sep 28, 2024
69af02c
run make generate-check
abi87 Sep 28, 2024
1edcce6
Merge branch 'appease-generate-check' into cometBFT-some-more-cleanup-2
abi87 Sep 28, 2024
f54e688
nit
abi87 Sep 28, 2024
e14ac8c
Merge branch 'main' into cometBFT-some-more-cleanup-2
abi87 Sep 28, 2024
a2d8d4e
appeased nilaway
abi87 Sep 28, 2024
d0834cc
Merge branch 'cometBFT-some-more-cleanup-2' of https://github.com/ber…
abi87 Sep 28, 2024
f4b97a5
nit
abi87 Sep 28, 2024
f7e82fd
fix errors content
abi87 Sep 30, 2024
f943e95
fixed errors formatting
abi87 Sep 30, 2024
b679d11
reverted validator check
abi87 Sep 30, 2024
cfec782
fixed refactoring regressions
abi87 Sep 30, 2024
7621133
Merge branch 'main' into cometBFT-some-more-cleanup-2
abi87 Sep 30, 2024
4f9dd82
nit
abi87 Sep 30, 2024
57e8eab
improved comments and dropped useless waits on ctx Done
abi87 Sep 30, 2024
6e5b289
made panics uniform
abi87 Sep 30, 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
157 changes: 75 additions & 82 deletions mod/consensus/pkg/cometbft/service/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ import (
"github.com/sourcegraph/conc/iter"
)

//nolint:gocognit // todo fix.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

side-effect of InitChain changes

var (
errInvalidHeight = errors.New("invalid height")
errNilFinalizeBlockState = errors.New("finalizeBlockState is nil")
)

func (s *Service[LoggerT]) InitChain(
_ context.Context,
req *cmtabci.InitChainRequest,
Expand All @@ -57,7 +61,6 @@ func (s *Service[LoggerT]) InitChain(

// On a new chain, we consider the init chain block height as 0, even though
// req.InitialHeight is 1 by default.
initHeader := cmtproto.Header{ChainID: req.ChainId, Time: req.Time}
Copy link
Collaborator Author

@abi87 abi87 Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

initHeader is really used only in defer call, to prepare context for next block. Moved it to defer to simplify logic and reduce its scope. This is different from cosmos SDK BaseApp where the header is used to setup the state

s.logger.Info(
"InitChain",
"initialHeight",
Expand All @@ -67,98 +70,95 @@ func (s *Service[LoggerT]) InitChain(
)

// Set the initial height, which will be used to determine if we are
// proposing
// or processing the first block or not.
// proposing or processing the first block or not.
nidhi-singh02 marked this conversation as resolved.
Show resolved Hide resolved
s.initialHeight = req.InitialHeight
if s.initialHeight == 0 { // If initial height is 0, set it to 1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

non informative comment, dropped

if s.initialHeight == 0 {
s.initialHeight = 1
}

// if req.InitialHeight is > 1, then we set the initial version on all
// stores
if req.InitialHeight > 1 {
initHeader.Height = req.InitialHeight
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is not really needed since Height will be overwritten in defer and initHeader is only read in defer.

if err := s.sm.CommitMultiStore().
SetInitialVersion(req.InitialHeight); err != nil {
return nil, err
}
}

s.setState(execModeFinalize)
s.finalizeBlockState = s.resetState()

defer func() {
// InitChain represents the state of the application BEFORE the first
// block, i.e. the genesis block. This means that when processing the
// app's InitChain handler, the block height is zero by default.
// However, after Commit is called
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nit: Commit is not really called for setting initial state

// the height needs to reflect the true block height.
initHeader.Height = req.InitialHeight
// However, after genesis is handled the height needs to reflect
// the true block height.
initHeader := cmtproto.Header{
nidhi-singh02 marked this conversation as resolved.
Show resolved Hide resolved
ChainID: req.ChainId,
Time: req.Time,
Height: req.InitialHeight,
}
s.finalizeBlockState.SetContext(
s.finalizeBlockState.Context().WithBlockHeader(initHeader),
)
}()

if s.finalizeBlockState == nil {
return nil, errors.New("finalizeBlockState is nil")
}
Comment on lines -102 to -104
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

redundant check since s.setState(execModeFinalize) is called right before. We don't do these checks after setState is called for proposals.


res, err := s.initChainer(s.finalizeBlockState.Context(), req)
resValidators, err := s.initChainer(
s.finalizeBlockState.Context(),
req.AppStateBytes,
)
if err != nil {
return nil, err
}

if res == nil {
// check validators
if len(resValidators) == 0 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is an hollow check, since we generate res in initChainer (it's not the result of s.Middleware.InitGenesis).
Replaced with what I understand it's the check we want

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

another point is that we may have a stricter check here and enforce that we always have at least a validator?

return nil, errors.New(
"application init chain handler returned a nil response",
"application init chain handler returned no validators",
)
}
if len(req.Validators) != len(resValidators) {
return nil, fmt.Errorf(
"len(RequestInitChain.Validators) != len(GenesisValidators) (%d != %d)",
len(req.Validators),
len(resValidators),
)
}

if len(req.Validators) > 0 {
if len(req.Validators) != len(res.Validators) {
return nil, fmt.Errorf(
"len(RequestInitChain.Validators) != len(GenesisValidators) (%d != %d)",
len(req.Validators),
len(res.Validators),
)
sort.Sort(cmtabci.ValidatorUpdates(req.Validators))

for i := range resValidators {
if req.Validators[i].Power != resValidators[i].Power {
return nil, errors.New("mismatched power")
}
if !bytes.Equal(
req.Validators[i].PubKeyBytes, resValidators[i].
PubKeyBytes) {
return nil, errors.New("mismatched pubkey bytes")
}

sort.Sort(cmtabci.ValidatorUpdates(req.Validators))

for i := range res.Validators {
if req.Validators[i].Power != res.Validators[i].Power {
return nil, errors.New("mismatched power")
}
if !bytes.Equal(
req.Validators[i].PubKeyBytes, res.Validators[i].
PubKeyBytes) {
return nil, errors.New("mismatched pubkey bytes")
}

if req.
Validators[i].PubKeyType != res.
Validators[i].PubKeyType {
return nil, errors.New("mismatched pubkey types")
}
if req.Validators[i].PubKeyType !=
resValidators[i].PubKeyType {
return nil, errors.New("mismatched pubkey types")
abi87 marked this conversation as resolved.
Show resolved Hide resolved
}
}

// NOTE: We don't commit, but FinalizeBlock for block InitialHeight starts
// from
// this FinalizeBlockState.
// from this FinalizeBlockState.
return &cmtabci.InitChainResponse{
ConsensusParams: res.ConsensusParams,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ConsensusParams is not set by the response generated in initChainer, so this effectively assigning default value of ConsensusParams.
Replaced with ConsensusParams from request, which I believe is what we need to do

Validators: res.Validators,
ConsensusParams: req.ConsensusParams,
Validators: resValidators,
AppHash: s.sm.CommitMultiStore().LastCommitID().Hash,
}, nil
}

// InitChainer initializes the chain.
func (s *Service[LoggerT]) initChainer(
ctx sdk.Context,
req *cmtabci.InitChainRequest,
) (*cmtabci.InitChainResponse, error) {
appStateBytes []byte,
) ([]cmtabci.ValidatorUpdate, error) {
var genesisState map[string]json.RawMessage
if err := json.Unmarshal(req.AppStateBytes, &genesisState); err != nil {
if err := json.Unmarshal(appStateBytes, &genesisState); err != nil {
return nil, err
}
valUpdates, err := s.Middleware.InitGenesis(
Expand All @@ -169,17 +169,10 @@ func (s *Service[LoggerT]) initChainer(
return nil, err
}

convertedValUpdates, err := iter.MapErr(
return iter.MapErr(
valUpdates,
convertValidatorUpdate[cmtabci.ValidatorUpdate],
)
if err != nil {
return nil, err
}

return &cmtabci.InitChainResponse{
Validators: convertedValUpdates,
}, nil
}

func (s *Service[LoggerT]) Info(
Expand Down Expand Up @@ -214,13 +207,16 @@ func (s *Service[LoggerT]) PrepareProposal(
_ context.Context,
req *cmtabci.PrepareProposalRequest,
) (*cmtabci.PrepareProposalResponse, error) {
s.setState(execModePrepareProposal)

// CometBFT must never call PrepareProposal with a height of 0.
if req.Height < 1 {
return nil, errors.New("PrepareProposal called with invalid height")
return nil, fmt.Errorf(
"prepareProposal, height %v: %w",
req.Height,
errInvalidHeight,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct Error Formatting in PrepareProposal

When using fmt.Errorf with %w for error wrapping, %w should not be combined with other verbs. Modify the error message to ensure proper error unwrapping.

Suggested Change:

-return nil, fmt.Errorf(
-    "prepareProposal, height %v: %w",
-    req.Height,
-    errInvalidHeight,
-)
+return nil, fmt.Errorf(
+    "prepareProposal at height %v: %w",
+    req.Height,
+    errInvalidHeight,
+)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return nil, fmt.Errorf(
"prepareProposal, height %v: %w",
req.Height,
errInvalidHeight,
)
return nil, fmt.Errorf(
"prepareProposal at height %v: %w",
req.Height,
errInvalidHeight,
)

}

s.prepareProposalState = s.resetState()
s.prepareProposalState.SetContext(
s.getContextForProposal(
s.prepareProposalState.Context(),
Expand Down Expand Up @@ -262,20 +258,21 @@ func (s *Service[LoggerT]) ProcessProposal(
) (*cmtabci.ProcessProposalResponse, error) {
// CometBFT must never call ProcessProposal with a height of 0.
if req.Height < 1 {
return nil, errors.New("ProcessProposal called with invalid height")
return nil, fmt.Errorf(
"processProposal, height %v: %w",
req.Height,
errInvalidHeight,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct Error Formatting in ProcessProposal

As in PrepareProposal, ensure that error messages in ProcessProposal use %w correctly for wrapping errors.

Suggested Change:

-return nil, fmt.Errorf(
-    "processProposal, height %v: %w",
-    req.Height,
-    errInvalidHeight,
-)
+return nil, fmt.Errorf(
+    "processProposal at height %v: %w",
+    req.Height,
+    errInvalidHeight,
+)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return nil, fmt.Errorf(
"processProposal, height %v: %w",
req.Height,
errInvalidHeight,
)
return nil, fmt.Errorf(
"processProposal at height %v: %w",
req.Height,
errInvalidHeight,
)

}

s.setState(execModeProcessProposal)

// Since the application can get access to FinalizeBlock state and write to
// it, we must be sure to reset it in case ProcessProposal timeouts and is
// called
// again in a subsequent round. However, we only want to do this after we've
// processed the first block, as we want to avoid overwriting the
// finalizeState
// after state changes during InitChain.
// called again in a subsequent round. However, we only want to do this
// after we've processed the first block, as we want to avoid overwriting
// the finalizeState after state changes during InitChain.
s.processProposalState = s.resetState()
if req.Height > s.initialHeight {
s.setState(execModeFinalize)
s.finalizeBlockState = s.resetState()
}

s.processProposalState.SetContext(
Expand Down Expand Up @@ -317,12 +314,7 @@ func (s *Service[LoggerT]) internalFinalizeBlock(
return nil, err
}

if s.finalizeBlockState == nil {
s.setState(execModeFinalize)
}
if s.finalizeBlockState == nil {
return nil, errors.New("finalizeBlockState is nil")
}
Comment on lines -321 to -323
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

redundant check since s.setState(execModeFinalize) is called right before. We don't do these checks after setState is called for proposals.

s.finalizeBlockState = s.resetState()

// First check for an abort signal after beginBlock, as it's the first place
// we spend any significant amount of time.
Expand Down Expand Up @@ -394,7 +386,11 @@ func (s *Service[_]) validateFinalizeBlockHeight(
req *cmtabci.FinalizeBlockRequest,
) error {
if req.Height < 1 {
return fmt.Errorf("invalid height: %d", req.Height)
return fmt.Errorf(
"finalizeBlock, height %v: %w",
req.Height,
errInvalidHeight,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct Error Formatting in validateFinalizeBlockHeight

Ensure proper error wrapping by adjusting the use of %w in fmt.Errorf.

Suggested Change:

-return fmt.Errorf(
-    "finalizeBlock, height %v: %w",
-    req.Height,
-    errInvalidHeight,
-)
+return fmt.Errorf(
+    "finalizeBlock at height %v: %w",
+    req.Height,
+    errInvalidHeight,
+)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return fmt.Errorf(
"finalizeBlock, height %v: %w",
req.Height,
errInvalidHeight,
)
return fmt.Errorf(
"finalizeBlock at height %v: %w",
req.Height,
errInvalidHeight,
)

}

lastBlockHeight := s.LastBlockHeight()
Expand Down Expand Up @@ -449,7 +445,7 @@ func (s *Service[LoggerT]) Commit(
context.Context, *cmtabci.CommitRequest,
) (*cmtabci.CommitResponse, error) {
if s.finalizeBlockState == nil {
return nil, errors.New("finalizeBlockState is nil")
return nil, fmt.Errorf("commit: %w", errNilFinalizeBlockState)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve Error Message in Commit

Provide a more descriptive error message when s.finalizeBlockState is nil during commit to aid in debugging.

Suggested Change:

-return nil, fmt.Errorf("commit: %w", errNilFinalizeBlockState)
+return nil, fmt.Errorf("commit failed: %w", errNilFinalizeBlockState)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return nil, fmt.Errorf("commit: %w", errNilFinalizeBlockState)
return nil, fmt.Errorf("commit failed: %w", errNilFinalizeBlockState)

}
header := s.finalizeBlockState.Context().BlockHeader()
retainHeight := s.GetBlockRetentionHeight(header.Height)
Expand Down Expand Up @@ -484,7 +480,7 @@ func (s *Service[LoggerT]) workingHash() []byte {
// MultiStore (s.sm.CommitMultiStore()) so when Commit() is called it
// persists those values.
if s.finalizeBlockState == nil {
panic("workingHash() called before FinalizeBlock()")
panic(fmt.Errorf("workingHash: %w", errNilFinalizeBlockState))
abi87 marked this conversation as resolved.
Show resolved Hide resolved
}
s.finalizeBlockState.ms.Write()

Expand All @@ -507,14 +503,11 @@ func (s *Service[LoggerT]) getContextForProposal(
ctx sdk.Context,
height int64,
) sdk.Context {
if height == s.initialHeight {
if s.finalizeBlockState == nil {
return ctx
}
ctx, _ = s.finalizeBlockState.Context().CacheContext()
if height != s.initialHeight || s.finalizeBlockState == nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nit: reduced indentation. This is really a personal preference, I am happy to revert it if we like the current version better

return ctx
}

ctx, _ = s.finalizeBlockState.Context().CacheContext()
abi87 marked this conversation as resolved.
Show resolved Hide resolved
return ctx
}

Expand Down
40 changes: 11 additions & 29 deletions mod/consensus/pkg/cometbft/service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ package cometbft
import (
"context"
"errors"
"fmt"

storetypes "cosmossdk.io/store/types"
servercmtlog "github.com/berachain/beacon-kit/mod/consensus/pkg/cometbft/service/log"
Expand All @@ -44,16 +43,6 @@ import (
"github.com/cosmos/cosmos-sdk/version"
)

type (
execMode uint8
)

const (
execModePrepareProposal execMode = iota
execModeProcessProposal
execModeFinalize
)

const InitialAppVersion uint64 = 0

type Service[
Expand All @@ -72,8 +61,11 @@ type Service[
finalizeBlockState *state
interBlockCache storetypes.MultiStorePersistentCache
paramStore *params.ConsensusParamsStore
initialHeight int64
minRetainBlocks uint64

// initialHeight is used to determine if we are
// proposing or processing the first block or not.
initialHeight int64
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like an explanation of attributes where we declare them. Happy to revert if we don't agree on this (or it breaks codebase conventions)

minRetainBlocks uint64

// application's version string
version string
Expand Down Expand Up @@ -212,26 +204,16 @@ func (s *Service[_]) setInterBlockCache(
s.interBlockCache = cache
}

func (s *Service[LoggerT]) setState(mode execMode) {
// resetState provides a fresh state which can be used to reset
// prepareProposal/processProposal/finalizeBlock State.
// A state is explicitly returned to avoid false positives from
// nilaway tool.
Comment on lines +219 to +220
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Clarify the comment regarding static analysis tool.

The comment references the "nilaway tool," which might be unclear or incorrectly named. For better clarity, consider specifying the correct tool name or generalizing the statement.

Suggested revision:

-// A state is explicitly returned to avoid false positives from
-// nilaway tool.
+// Explicitly returning the state helps avoid false positives from
+// static analysis tools that check for nil references.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// A state is explicitly returned to avoid false positives from
// nilaway tool.
// Explicitly returning the state helps avoid false positives from
// static analysis tools that check for nil references.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like being specific (a different tool may give different results) but @itsdevbear and @ocnc may have a different opinion

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

func (s *Service[LoggerT]) resetState() *state {
ms := s.sm.CommitMultiStore().CacheMultiStore()
baseState := &state{
return &state{
ms: ms,
ctx: sdk.NewContext(ms, false, servercmtlog.WrapSDKLogger(s.logger)),
}
Comment on lines +217 to 226
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Consider clarifying the comment about static analysis.

The resetState method is a good addition that simplifies state management for different stages of block processing. It provides a consistent way to create a fresh state with a cached multi-store and a new context.

The comment explaining the purpose of explicitly returning the state is helpful. However, it could be more specific about which static analysis tool it's referring to.

Consider updating the comment to be more specific:

// resetState provides a fresh state which can be used to reset
// prepareProposal/processProposal/finalizeBlock State.
// A state is explicitly returned to avoid false positives from
// static analysis tools that check for nil references (e.g., nilaway).


switch mode {
case execModePrepareProposal:
s.prepareProposalState = baseState

case execModeProcessProposal:
s.processProposalState = baseState

case execModeFinalize:
s.finalizeBlockState = baseState

default:
panic(fmt.Sprintf("invalid runTxMode for setState: %d", mode))
}
}

// convertValidatorUpdate abstracts the conversion of a
Expand Down
Loading