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(stf): port simappv2 changes #20587

Merged
merged 1 commit into from
Jun 10, 2024
Merged

feat(stf): port simappv2 changes #20587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
40 changes: 33 additions & 7 deletions server/v2/stf/branch/writer_map.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,21 +53,47 @@ func (b WriterMap) ApplyStateChanges(stateChanges []store.StateChanges) error {
return nil
}

// GetStateChanges returns the state changes for all actors in the WriterMap, including all direct
// ancesotors from which this WriterMap was derived.
// See WriterMap.recurseStateChanges for more details.
// Subject to possible renaming to ensure a developer can retrieve only changes in *this* branch
// context (not ancestors) if that is desired.
// see: https://github.com/cosmos/cosmos-sdk/pull/20412#discussion_r1618771230
func (b WriterMap) GetStateChanges() ([]store.StateChanges, error) {
sc := make([]store.StateChanges, len(b.branchedWriterState))
for account, stateChange := range b.branchedWriterState {
kvChanges, err := stateChange.ChangeSets()
if err != nil {
return nil, err
}
var (
changes = make(map[string][]store.KVPair)
sc []store.StateChanges
)
if err := b.recurseStateChanges(changes); err != nil {
return nil, err
}

for account, kvPairs := range changes {
Comment on lines +56 to +71
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming GetStateChanges to reflect its functionality more accurately.

The method name GetStateChanges might imply that it only retrieves changes from the current branch, but it also includes ancestor changes. Consider renaming it to GetAllStateChanges or similar to avoid confusion.

sc = append(sc, store.StateChanges{
Actor: []byte(account),
StateChanges: kvChanges,
StateChanges: kvPairs,
})
}
return sc, nil
}

func (b WriterMap) recurseStateChanges(changes map[string][]store.KVPair) error {
// depth first
if wr, ok := b.state.(WriterMap); ok {
if err := wr.recurseStateChanges(changes); err != nil {
return err
}
}
for account, stateChange := range b.branchedWriterState {
kvChanges, err := stateChange.ChangeSets()
if err != nil {
return err
}
changes[account] = append(changes[account], kvChanges...)
}
return nil
}
Comment on lines +80 to +95
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimize the recursion in recurseStateChanges.

The recursive approach in recurseStateChanges could lead to performance issues with deep recursion. Consider using an iterative approach with a stack to manage state changes, which can be more efficient and easier to understand.


func (b WriterMap) applyStateChange(sc store.StateChanges) error {
writableState, err := b.GetWriter(sc.Actor)
if err != nil {
Expand Down
24 changes: 16 additions & 8 deletions server/v2/stf/core_router_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"context"
"errors"
"fmt"
"reflect"
Dismissed Show dismissed Hide dismissed
"strings"

"google.golang.org/protobuf/runtime/protoiface"
Expand Down Expand Up @@ -61,14 +62,8 @@

// NewQueryRouterService implements router.Service.
func NewQueryRouterService(queryRouterBuilder *MsgRouterBuilder) router.Service {
queryRouter, err := queryRouterBuilder.Build()
if err != nil {
panic(fmt.Errorf("cannot create queryRouter: %w", err))
}

return &queryRouterService{
builder: queryRouterBuilder,
handler: queryRouter,
}
}

Expand Down Expand Up @@ -100,8 +95,21 @@
ctx context.Context,
req, resp protoiface.MessageV1,
) error {
// see https://github.com/cosmos/cosmos-sdk/pull/20349
panic("not implemented")
// TODO lazy initialization is ugly and not thread safe. we don't want to check a mutex on every InvokeTyped either.
if m.handler == nil {
var err error
m.handler, err = m.builder.Build()
if err != nil {
return fmt.Errorf("cannot create queryRouter: %w", err)
}
}
// reflection is required, see https://github.com/cosmos/cosmos-sdk/pull/20349
res, err := m.handler(ctx, req)
if err != nil {
return err
}
reflect.Indirect(reflect.ValueOf(resp)).Set(reflect.Indirect(reflect.ValueOf(res)))
return nil
Comment on lines +98 to +112
Copy link
Contributor

Choose a reason for hiding this comment

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

Address thread safety and performance concerns in InvokeTyped.

The lazy initialization of m.handler in InvokeTyped is not thread-safe. This could lead to race conditions in a multi-threaded environment. Consider initializing m.handler in the constructor or using synchronization mechanisms like mutexes to ensure thread safety.

}

// InvokeUntyped execute a message and returns a response.
Expand Down
26 changes: 19 additions & 7 deletions server/v2/stf/stf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

appmanager "cosmossdk.io/core/app"
appmodulev2 "cosmossdk.io/core/appmodule/v2"
corecontext "cosmossdk.io/core/context"
"cosmossdk.io/core/event"
"cosmossdk.io/core/gas"
"cosmossdk.io/core/header"
Expand All @@ -17,6 +18,9 @@ import (
"cosmossdk.io/server/v2/stf/internal"
)

// Identity defines STF's bytes identity and it's used by STF to store things in its own state.
var Identity = []byte("stf")

// STF is a struct that manages the state transition component of the app.
type STF[T transaction.Tx] struct {
logger log.Logger
Expand Down Expand Up @@ -108,10 +112,15 @@ func (s STF[T]) DeliverBlock(

// reset events
exCtx.events = make([]event.Event, 0)

// begin block
beginBlockEvents, err := s.beginBlock(exCtx)
if err != nil {
return nil, nil, err
var beginBlockEvents []event.Event
if !block.IsGenesis {
// begin block
beginBlockEvents, err = s.beginBlock(exCtx)
if err != nil {
return nil, nil, err
}
}

// check if we need to return early
Expand Down Expand Up @@ -401,11 +410,13 @@ func (s STF[T]) validatorUpdates(
return ctx.events, valSetUpdates, nil
}

const headerInfoPrefix = 0x0
const headerInfoPrefix = 0x37

// setHeaderInfo sets the header info in the state to be used by queries in the future.
func (s STF[T]) setHeaderInfo(state store.WriterMap, headerInfo header.Info) error {
runtimeStore, err := state.GetWriter(appmanager.RuntimeIdentity)
// TODO storing header info is too low level here, stf should be stateless.
// We should have a keeper that does this.
runtimeStore, err := state.GetWriter(Identity)
Comment on lines +413 to +419
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor setHeaderInfo to separate concerns.

The method setHeaderInfo is performing low-level operations that might be better handled by a dedicated component. Consider introducing a HeaderKeeper that encapsulates the logic for storing and retrieving header information, which would make STF more focused on its primary responsibilities.

Copy link
Member

Choose a reason for hiding this comment

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

Like we talked about, this GetWriter("stf") will err unless an "stf" storekey is provided, probably by runtime/v2 if we don't want depinject infra code in stf.

if err != nil {
return err
}
Expand All @@ -422,7 +433,7 @@ func (s STF[T]) setHeaderInfo(state store.WriterMap, headerInfo header.Info) err

// getHeaderInfo gets the header info from the state. It should only be used for queries
func (s STF[T]) getHeaderInfo(state store.WriterMap) (i header.Info, err error) {
runtimeStore, err := state.GetWriter(appmanager.RuntimeIdentity)
runtimeStore, err := state.GetWriter(Identity)
if err != nil {
return header.Info{}, err
}
Expand Down Expand Up @@ -579,11 +590,12 @@ func (s STF[T]) makeContext(
store store.WriterMap,
execMode transaction.ExecMode,
) *executionContext {
valuedCtx := context.WithValue(ctx, corecontext.ExecModeKey, execMode)
return newExecutionContext(
s.makeGasMeter,
s.makeGasMeteredState,
s.branchFn,
ctx,
valuedCtx,
sender,
store,
execMode,
Expand Down
Loading