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

refactor(server/v2): Update prepare & process proposal #21237

Merged
merged 11 commits into from
Sep 4, 2024
Merged

refactor(server/v2): Update prepare & process proposal #21237

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
faf5335
check result err
hieuvubk Aug 9, 2024
f6b5f43
check handler & right slice
hieuvubk Aug 9, 2024
532edc8
fix type
hieuvubk Aug 9, 2024
96171df
move tx decode to handler
hieuvubk Aug 13, 2024
c734809
fix interface types
hieuvubk Aug 13, 2024
53fafa8
Merge branch 'main' into hieu/refactor-proposal
hieuvubk Aug 13, 2024
9d725d4
Merge branch 'main' into hieu/refactor-proposal
hieuvubk Aug 14, 2024
5841d3b
update comment
hieuvubk Aug 14, 2024
5262b84
Merge branch 'hieu/refactor-proposal' of https://github.com/cosmos/co…
hieuvubk Aug 14, 2024
6856241
update proposal handler
hieuvubk Sep 4, 2024
1075d9d
merge main
hieuvubk Sep 4, 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
3 changes: 2 additions & 1 deletion server/v2/appmanager/appmanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ func (a AppManager[T]) ValidateTx(ctx context.Context, tx T) (appmanager.TxResul
if err != nil {
return appmanager.TxResult{}, err
}
return a.stf.ValidateTx(ctx, latestState, a.config.ValidateTxGasLimit, tx), nil
res := a.stf.ValidateTx(ctx, latestState, a.config.ValidateTxGasLimit, tx)
return res, res.Error
}

// Simulate runs validation and execution flow of a Tx.
Expand Down
20 changes: 16 additions & 4 deletions server/v2/cometbft/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,8 +315,12 @@ func (c *Consensus[T]) PrepareProposal(
return nil, errors.New("PrepareProposal called with invalid height")
}

decodedTxs := make([]T, len(req.Txs))
for i, tx := range req.Txs {
if c.prepareProposalHandler == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this fail at app build time?

Copy link
Member

@julienrbrt julienrbrt Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

if you don't pass anything to the comet server, we use the default options (no op handlers).
so to get this you would have to purposely pass nil. as we don't want to make those mandatory, making it fail at compile time isn't best.

return nil, errors.New("no prepare proposal function was set")
}

var decodedTxs []T
Copy link
Member

Choose a reason for hiding this comment

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

we should preallocate here as we know the length

Copy link
Member

Choose a reason for hiding this comment

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

Thought the same but we skip undecodable txs, so the index would then be nil

Copy link
Member

Choose a reason for hiding this comment

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

The only case we can't decode is if it's a vote exetension or some other data type of the state machine right?

Copy link
Member

@julienrbrt julienrbrt Aug 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should be yeah

Copy link
Member

Choose a reason for hiding this comment

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

need to remove the decoding part and make it part of the handler. it wont work this way, and requiring users to modify the decoder feels weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense to move it to handler.

for _, tx := range req.Txs {
decTx, err := c.txCodec.Decode(tx)
if err != nil {
// TODO: vote extension meta data as a custom type to avoid possibly accepting invalid txs
Expand All @@ -325,7 +329,7 @@ func (c *Consensus[T]) PrepareProposal(
continue
}

decodedTxs[i] = decTx
decodedTxs = append(decodedTxs, decTx)
Copy link
Member

Choose a reason for hiding this comment

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

i dont believe this is correct. in the case of vote extensions we will not be able to decode the txs meaning we will always remove it from the block. This should happen in the handler instead of here. This is how its handled in abci.

}

ciCtx := contextWithCometInfo(ctx, comet.Info{
Expand Down Expand Up @@ -356,7 +360,15 @@ func (c *Consensus[T]) ProcessProposal(
ctx context.Context,
req *abciproto.ProcessProposalRequest,
) (*abciproto.ProcessProposalResponse, error) {
decodedTxs := make([]T, len(req.Txs))
if req.Height < 1 {
return nil, errors.New("ProcessProposal called with invalid height")
}

if c.processProposalHandler == nil {
return nil, errors.New("no process proposal function was set")
}

var decodedTxs []T
for _, tx := range req.Txs {
decTx, err := c.txCodec.Decode(tx)
if err != nil {
Expand Down
16 changes: 8 additions & 8 deletions server/v2/cometbft/handlers/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import (
abci "github.com/cometbft/cometbft/api/cometbft/abci/v1"
"github.com/cosmos/gogoproto/proto"

consensusv1 "cosmossdk.io/api/cosmos/consensus/v1"
appmanager "cosmossdk.io/core/app"
"cosmossdk.io/core/store"
"cosmossdk.io/core/transaction"
"cosmossdk.io/server/v2/cometbft/mempool"
consensustypes "cosmossdk.io/x/consensus/types"
)

type AppManager[T transaction.Tx] interface {
Expand Down Expand Up @@ -41,14 +41,14 @@ func (h *DefaultProposalHandler[T]) PrepareHandler() PrepareHandler[T] {

var maxBlockGas uint64

res, err := app.Query(ctx, 0, &consensusv1.QueryParamsRequest{})
res, err := app.Query(ctx, 0, &consensustypes.QueryParamsRequest{})
if err != nil {
return nil, err
}

paramsResp, ok := res.(*consensusv1.QueryParamsResponse)
paramsResp, ok := res.(*consensustypes.QueryParamsResponse)
if !ok {
return nil, fmt.Errorf("unexpected consensus params response type; expected: %T, got: %T", &consensusv1.QueryParamsResponse{}, res)
return nil, fmt.Errorf("unexpected consensus params response type; expected: %T, got: %T", &consensustypes.QueryParamsResponse{}, res)
}

if b := paramsResp.GetParams().Block; b != nil {
Expand Down Expand Up @@ -110,19 +110,19 @@ func (h *DefaultProposalHandler[T]) ProcessHandler() ProcessHandler[T] {
return nil
}

_, ok := req.(*abci.PrepareProposalRequest)
_, ok := req.(*abci.ProcessProposalRequest)
if !ok {
return fmt.Errorf("invalid request type: %T", req)
}

res, err := app.Query(ctx, 0, &consensusv1.QueryParamsRequest{})
res, err := app.Query(ctx, 0, &consensustypes.QueryParamsRequest{})
if err != nil {
return err
}

paramsResp, ok := res.(*consensusv1.QueryParamsResponse)
paramsResp, ok := res.(*consensustypes.QueryParamsResponse)
if !ok {
return fmt.Errorf("unexpected consensus params response type; expected: %T, got: %T", &consensusv1.QueryParamsResponse{}, res)
return fmt.Errorf("unexpected consensus params response type; expected: %T, got: %T", &consensustypes.QueryParamsResponse{}, res)
}

var maxBlockGas uint64
Expand Down
Loading