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

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

Merged
merged 18 commits into from
Dec 19, 2024
Merged

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

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
adebbc0
progress
facundomedica Dec 9, 2024
1c54265
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu…
facundomedica Dec 10, 2024
79efd34
kind of working
facundomedica Dec 11, 2024
8944e10
Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu…
facundomedica Dec 11, 2024
ed2e353
working finally
facundomedica Dec 13, 2024
aaff4f5
cleanup
facundomedica Dec 13, 2024
fc20251
merge main
facundomedica Dec 16, 2024
741cdeb
fix named params
facundomedica Dec 16, 2024
ee42697
lint
facundomedica Dec 16, 2024
97ec24b
lint
facundomedica Dec 16, 2024
0a13d59
fixing
facundomedica Dec 17, 2024
0807cb3
merge
facundomedica Dec 17, 2024
7db5e2a
attempt to debug test
facundomedica Dec 18, 2024
6ee24df
fix dumb error
facundomedica Dec 18, 2024
ad2801e
remove implementation
facundomedica Dec 18, 2024
28beb94
fix
facundomedica Dec 18, 2024
f620313
suggestions
facundomedica Dec 19, 2024
ca5d8f4
fixes
facundomedica Dec 19, 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
34 changes: 33 additions & 1 deletion runtime/v2/builder.go
View file
Open in desktop
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, mmPreblocker func()) error
}

// RegisterModules registers the provided modules with the module manager.
Expand Down Expand Up @@ -95,11 +96,28 @@ 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 a.preblocker != nil {
return a.preblocker(ctx, txs, func() {
if err := a.app.moduleManager.PreBlocker()(ctx, txs); err != nil {
panic(err)
}
})
} else {
// if there is no preblocker set, call the module manager's preblocker directly
if err := a.app.moduleManager.PreBlocker()(ctx, txs); err != nil {
return err
}
}

return nil
}
facundomedica marked this conversation as resolved.
Show resolved Hide resolved

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 +237,17 @@ func AppBuilderWithPostTxExec[T transaction.Tx](
a.postTxExec = postTxExec
}
}

// AppBuilderWithPreblocker sets logic that will be executed before each block.
// mmPreblocker can be used to call module manager's preblocker, so that it can be
// called before or after depending on the app's logic.
// This is especially useful when implementing vote extensions.
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 •
edited
Loading

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, mmPreblocker func(),
) error,
) AppBuilderOption[T] {
return func(a *AppBuilder[T]) {
a.preblocker = preblocker
}
}
18 changes: 10 additions & 8 deletions server/v2/cometbft/abci.go
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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(ctx context.Context, app AppManager[T], cdc transaction.Codec[T], req *abci.PrepareProposalRequest, chainID string) ([]T, error)

// 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(ctx context.Context, app AppManager[T], cdc transaction.Codec[T], req *abci.ProcessProposalRequest, chainID 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
View file
Open in desktop
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