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

feat(server/v2/cometbft): Implement necessary handlers and helpers for vote extensions #22830

Merged
merged 18 commits into from
Dec 19, 2024
Merged
25 changes: 24 additions & 1 deletion runtime/v2/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ type AppBuilder[T transaction.Tx] struct {
branch func(state store.ReaderMap) store.WriterMap
txValidator func(ctx context.Context, tx T) error
postTxExec func(ctx context.Context, tx T, success bool) error
preblocker func(ctx context.Context, txs []T) error
}

// RegisterModules registers the provided modules with the module manager.
Expand Down Expand Up @@ -95,11 +96,23 @@ func (a *AppBuilder[T]) Build(opts ...AppBuilderOption[T]) (*App[T], error) {

endBlocker, valUpdate := a.app.moduleManager.EndBlock()

preblockerFn := func(ctx context.Context, txs []T) error {
if err := a.app.moduleManager.PreBlocker()(ctx, txs); err != nil {
return err
}

if a.preblocker != nil {
return a.preblocker(ctx, txs)
}

return nil
}

stf, err := stf.New[T](
a.app.logger.With("module", "stf"),
a.app.msgRouterBuilder,
a.app.queryRouterBuilder,
a.app.moduleManager.PreBlocker(),
preblockerFn,
a.app.moduleManager.BeginBlock(),
endBlocker,
a.txValidator,
Expand Down Expand Up @@ -219,3 +232,13 @@ func AppBuilderWithPostTxExec[T transaction.Tx](
a.postTxExec = postTxExec
}
}

func AppBuilderWithPreblocker[T transaction.Tx](
Copy link
Member

Choose a reason for hiding this comment

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

Can we add go docs here?

Copy link
Member

@julienrbrt julienrbrt Dec 18, 2024

Choose a reason for hiding this comment

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

We should precise as well that the module preblocker are ran before any logic there.
Which makes me think, do you think we should always run pre blocker before their logic?
What if we give them a callback to call preblocker themselves if needed.

Like

preblocker func(
		ctx context.Context, txs []T, mmPreblocker func()
	) error,

And then above we do smth like

	preblockerFn := func(ctx context.Context, txs []T, mmPreblocker func()) error {
		if a.preblocker != nil {
			return a.preblocker(ctx, txs, func() { if err := a.app.moduleManager.PreBlocker()(ctx, txs); err != nil {
			panic(err)
		} })
		}

                 return .app.moduleManager.PreBlocker()(ctx, txs)
	}

Not sure if needed however.

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 like this, adding it

preblocker func(
ctx context.Context, txs []T,
) error,
) AppBuilderOption[T] {
return func(a *AppBuilder[T]) {
a.preblocker = preblocker
}
}
18 changes: 10 additions & 8 deletions server/v2/cometbft/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ type consensus[T transaction.Tx] struct {

prepareProposalHandler handlers.PrepareHandler[T]
processProposalHandler handlers.ProcessHandler[T]
verifyVoteExt handlers.VerifyVoteExtensionhandler
verifyVoteExt handlers.VerifyVoteExtensionHandler
extendVote handlers.ExtendVoteHandler
checkTxHandler handlers.CheckTxHandler[T]

Expand Down Expand Up @@ -142,7 +142,7 @@ func (c *consensus[T]) Info(ctx context.Context, _ *abciproto.InfoRequest) (*abc
// if height is 0, we dont know the consensus params
var appVersion uint64 = 0
if version > 0 {
cp, err := c.GetConsensusParams(ctx)
cp, err := GetConsensusParams(ctx, c.app)
// if the consensus params are not found, we set the app version to 0
// in the case that the start version is > 0
if cp == nil || errors.Is(err, collections.ErrNotFound) {
Expand Down Expand Up @@ -387,7 +387,7 @@ func (c *consensus[T]) PrepareProposal(
LastCommit: toCoreExtendedCommitInfo(req.LocalLastCommit),
})

txs, err := c.prepareProposalHandler(ciCtx, c.app, c.appCodecs.TxCodec, req)
txs, err := c.prepareProposalHandler(ciCtx, c.app, c.appCodecs.TxCodec, req, c.chainID)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -433,7 +433,7 @@ func (c *consensus[T]) ProcessProposal(
LastCommit: toCoreCommitInfo(req.ProposedLastCommit),
})

err := c.processProposalHandler(ciCtx, c.app, c.appCodecs.TxCodec, req)
err := c.processProposalHandler(ciCtx, c.app, c.appCodecs.TxCodec, req, c.chainID)
if err != nil {
c.logger.Error("failed to process proposal", "height", req.Height, "time", req.Time, "hash", fmt.Sprintf("%X", req.Hash), "err", err)
return &abciproto.ProcessProposalResponse{
Expand Down Expand Up @@ -533,7 +533,7 @@ func (c *consensus[T]) FinalizeBlock(

c.lastCommittedHeight.Store(req.Height)

cp, err := c.GetConsensusParams(ctx) // we get the consensus params from the latest state because we committed state above
cp, err := GetConsensusParams(ctx, c.app) // we get the consensus params from the latest state because we committed state above
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -600,7 +600,7 @@ func (c *consensus[T]) Commit(ctx context.Context, _ *abciproto.CommitRequest) (

c.snapshotManager.SnapshotIfApplicable(lastCommittedHeight)

cp, err := c.GetConsensusParams(ctx)
cp, err := GetConsensusParams(ctx, c.app)
if err != nil {
return nil, err
}
Expand All @@ -619,7 +619,7 @@ func (c *consensus[T]) VerifyVoteExtension(
) (*abciproto.VerifyVoteExtensionResponse, error) {
// If vote extensions are not enabled, as a safety precaution, we return an
// error.
cp, err := c.GetConsensusParams(ctx)
cp, err := GetConsensusParams(ctx, c.app)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -658,7 +658,7 @@ func (c *consensus[T]) VerifyVoteExtension(
func (c *consensus[T]) ExtendVote(ctx context.Context, req *abciproto.ExtendVoteRequest) (*abciproto.ExtendVoteResponse, error) {
// If vote extensions are not enabled, as a safety precaution, we return an
// error.
cp, err := c.GetConsensusParams(ctx)
cp, err := GetConsensusParams(ctx, c.app)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -702,6 +702,8 @@ func decodeTxs[T transaction.Tx](logger log.Logger, rawTxs [][]byte, codec trans
if err != nil {
// do not return an error here, as we want to deliver the block even if some txs are invalid
logger.Debug("failed to decode tx", "err", err)
txs[i] = RawTx(rawTx).(T) // allows getting the raw bytes down the line
continue
}
txs[i] = tx
}
Expand Down
10 changes: 5 additions & 5 deletions server/v2/cometbft/handlers/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func NewDefaultProposalHandler[T transaction.Tx](mp mempool.Mempool[T]) *Default
}

func (h *DefaultProposalHandler[T]) PrepareHandler() PrepareHandler[T] {
return func(ctx context.Context, app AppManager[T], codec transaction.Codec[T], req *abci.PrepareProposalRequest) ([]T, error) {
return func(ctx context.Context, app AppManager[T], codec transaction.Codec[T], req *abci.PrepareProposalRequest, chainID string) ([]T, error) {
var maxBlockGas uint64

res, err := app.Query(ctx, 0, &consensustypes.QueryParamsRequest{})
Expand Down Expand Up @@ -98,7 +98,7 @@ func (h *DefaultProposalHandler[T]) PrepareHandler() PrepareHandler[T] {
}

func (h *DefaultProposalHandler[T]) ProcessHandler() ProcessHandler[T] {
return func(ctx context.Context, app AppManager[T], codec transaction.Codec[T], req *abci.ProcessProposalRequest) error {
return func(ctx context.Context, app AppManager[T], codec transaction.Codec[T], req *abci.ProcessProposalRequest, chainID string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Remove unused chainID parameter from ProcessHandler

The chainID parameter is indeed not used within the ProcessHandler implementation. The function body only deals with mempool operations and doesn't utilize the chainID parameter anywhere.

  • Remove the chainID parameter from the ProcessHandler function signature in server/v2/cometbft/handlers/defaults.go
🔗 Analysis chain

Verify the usage of chainID parameter

Similar to PrepareHandler, the chainID parameter has been added but is not utilized within the function body.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for chainID usage in process proposal handlers
rg -A 5 "chainID.*ProcessProposal"

Length of output: 36


Script:

#!/bin/bash
# Let's search for the ProcessHandler function implementation and check chainID usage
ast-grep --pattern 'func ProcessHandler[T any]($$$) func($$$) error {
  $$$
}'

# Also search for any chainID usage in the file
rg -A 5 "chainID" server/v2/cometbft/handlers/defaults.go

Length of output: 1064

// If the mempool is nil we simply return ACCEPT,
// because PrepareProposal may have included txs that could fail verification.
_, isNoOp := h.mempool.(mempool.NoOpMempool[T])
Expand Down Expand Up @@ -174,15 +174,15 @@ func decodeTxs[T transaction.Tx](codec transaction.Codec[T], txsBz [][]byte) []T
// NoOpPrepareProposal defines a no-op PrepareProposal handler. It will always
// return the transactions sent by the client's request.
func NoOpPrepareProposal[T transaction.Tx]() PrepareHandler[T] {
return func(ctx context.Context, app AppManager[T], codec transaction.Codec[T], req *abci.PrepareProposalRequest) ([]T, error) {
return func(ctx context.Context, app AppManager[T], codec transaction.Codec[T], req *abci.PrepareProposalRequest, chainID string) ([]T, error) {
return decodeTxs(codec, req.Txs), nil
}
}

// NoOpProcessProposal defines a no-op ProcessProposal Handler. It will always
// return ACCEPT.
func NoOpProcessProposal[T transaction.Tx]() ProcessHandler[T] {
return func(context.Context, AppManager[T], transaction.Codec[T], *abci.ProcessProposalRequest) error {
return func(context.Context, AppManager[T], transaction.Codec[T], *abci.ProcessProposalRequest, string) error {
return nil
}
}
Expand All @@ -197,7 +197,7 @@ func NoOpExtendVote() ExtendVoteHandler {

// NoOpVerifyVoteExtensionHandler defines a no-op VerifyVoteExtension handler. It
// will always return an ACCEPT status with no error.
func NoOpVerifyVoteExtensionHandler() VerifyVoteExtensionhandler {
func NoOpVerifyVoteExtensionHandler() VerifyVoteExtensionHandler {
return func(context.Context, store.ReaderMap, *abci.VerifyVoteExtensionRequest) (*abci.VerifyVoteExtensionResponse, error) {
return &abci.VerifyVoteExtensionResponse{Status: abci.VERIFY_VOTE_EXTENSION_STATUS_ACCEPT}, nil
}
Expand Down
8 changes: 4 additions & 4 deletions server/v2/cometbft/handlers/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,16 @@ import (
type (
// PrepareHandler passes in the list of Txs that are being proposed. The app can then do stateful operations
// over the list of proposed transactions. It can return a modified list of txs to include in the proposal.
PrepareHandler[T transaction.Tx] func(context.Context, AppManager[T], transaction.Codec[T], *abci.PrepareProposalRequest) ([]T, error)
PrepareHandler[T transaction.Tx] func(context.Context, AppManager[T], transaction.Codec[T], *abci.PrepareProposalRequest, string) ([]T, error)
facundomedica marked this conversation as resolved.
Show resolved Hide resolved

// ProcessHandler is a function that takes a list of transactions and returns a boolean and an error.
// If the verification of a transaction fails, the boolean is false and the error is non-nil.
ProcessHandler[T transaction.Tx] func(context.Context, AppManager[T], transaction.Codec[T], *abci.ProcessProposalRequest) error
ProcessHandler[T transaction.Tx] func(context.Context, AppManager[T], transaction.Codec[T], *abci.ProcessProposalRequest, string) error

// VerifyVoteExtensionhandler is a function type that handles the verification of a vote extension request.
// VerifyVoteExtensionHandler is a function type that handles the verification of a vote extension request.
// It takes a context, a store reader map, and a request to verify a vote extension.
// It returns a response to verify the vote extension and an error if any.
VerifyVoteExtensionhandler func(context.Context, store.ReaderMap, *abci.VerifyVoteExtensionRequest) (*abci.VerifyVoteExtensionResponse, error)
VerifyVoteExtensionHandler func(context.Context, store.ReaderMap, *abci.VerifyVoteExtensionRequest) (*abci.VerifyVoteExtensionResponse, error)

// ExtendVoteHandler is a function type that handles the extension of a vote.
// It takes a context, a store reader map, and a request to extend a vote.
Expand Down
2 changes: 1 addition & 1 deletion server/v2/cometbft/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ type ServerOptions[T transaction.Tx] struct {
PrepareProposalHandler handlers.PrepareHandler[T]
ProcessProposalHandler handlers.ProcessHandler[T]
CheckTxHandler handlers.CheckTxHandler[T]
VerifyVoteExtensionHandler handlers.VerifyVoteExtensionhandler
VerifyVoteExtensionHandler handlers.VerifyVoteExtensionHandler
ExtendVoteHandler handlers.ExtendVoteHandler
KeygenF keyGenF

Expand Down
Loading
Loading