From cb1f17aa35cfe7ba56a329f84947de7d586b89fc Mon Sep 17 00:00:00 2001 From: shakezula Date: Tue, 11 Jul 2023 14:52:57 -0600 Subject: [PATCH 01/20] [chore] submodule pattern updates to TreeStore --- persistence/module.go | 11 ++- persistence/sql/sql.go | 11 --- persistence/trees/module.go | 56 +++++++++---- persistence/trees/module_test.go | 121 +++++++++++++++++++++++++++ persistence/trees/trees.go | 93 ++++++++++++-------- runtime/bus.go | 4 + shared/modules/persistence_module.go | 2 +- 7 files changed, 227 insertions(+), 71 deletions(-) create mode 100644 persistence/trees/module_test.go diff --git a/persistence/module.go b/persistence/module.go index dec15112b..192380fbb 100644 --- a/persistence/module.go +++ b/persistence/module.go @@ -103,21 +103,24 @@ func (*persistenceModule) Create(bus modules.Bus, options ...modules.ModuleOptio return nil, err } - treeModule, err := trees.Create( + _, err = trees.Create( bus, trees.WithTreeStoreDirectory(persistenceCfg.TreesStoreDir), - trees.WithLogger(m.logger)) + trees.WithLogger(m.logger), + ) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create TreeStoreModule: %w", err) } m.config = persistenceCfg m.genesisState = genesisState m.networkId = runtimeMgr.GetConfig().NetworkId + // TECHDEBT: fetch blockstore from bus once it's a proper submodule m.blockStore = blockStore + // TECHDEBT: fetch txIndexer from bus m.txIndexer = txIndexer - m.stateTrees = treeModule + m.stateTrees = m.GetBus().GetTreeStore() // TECHDEBT: reconsider if this is the best place to call `populateGenesisState`. Note that // this forces the genesis state to be reloaded on every node startup until state diff --git a/persistence/sql/sql.go b/persistence/sql/sql.go index 80a1c8320..9e99189bb 100644 --- a/persistence/sql/sql.go +++ b/persistence/sql/sql.go @@ -6,7 +6,6 @@ import ( "fmt" "github.com/jackc/pgx/v5" - "github.com/pokt-network/pocket/persistence/indexer" ptypes "github.com/pokt-network/pocket/persistence/types" coreTypes "github.com/pokt-network/pocket/shared/core/types" ) @@ -91,16 +90,6 @@ func GetAccountsUpdated( return accounts, nil } -// GetTransactions takes a transaction indexer and returns the transactions for the current height -func GetTransactions(txi indexer.TxIndexer, height uint64) ([]*coreTypes.IndexedTransaction, error) { - // TECHDEBT(#813): Avoid this cast to int64 - indexedTxs, err := txi.GetByHeight(int64(height), false) - if err != nil { - return nil, fmt.Errorf("failed to get transactions by height: %w", err) - } - return indexedTxs, nil -} - // GetPools returns the pools updated at the given height func GetPools(pgtx pgx.Tx, height uint64) ([]*coreTypes.Account, error) { pools, err := GetAccountsUpdated(pgtx, ptypes.Pool, height) diff --git a/persistence/trees/module.go b/persistence/trees/module.go index 942ab0423..f85e0c54d 100644 --- a/persistence/trees/module.go +++ b/persistence/trees/module.go @@ -3,20 +3,23 @@ package trees import ( "fmt" + "github.com/pokt-network/pocket/persistence/indexer" "github.com/pokt-network/pocket/persistence/kvstore" "github.com/pokt-network/pocket/shared/modules" "github.com/pokt-network/smt" ) -func (*treeStore) Create(bus modules.Bus, options ...modules.TreeStoreOption) (modules.TreeStoreModule, error) { - m := &treeStore{} +var _ modules.Module = &TreeStore{} + +func (*TreeStore) Create(bus modules.Bus, options ...modules.ModuleOption) (modules.Module, error) { + m := &TreeStore{} + + bus.RegisterModule(m) for _, option := range options { option(m) } - bus.RegisterModule(m) - if err := m.setupTrees(); err != nil { return nil, err } @@ -24,14 +27,14 @@ func (*treeStore) Create(bus modules.Bus, options ...modules.TreeStoreOption) (m return m, nil } -func Create(bus modules.Bus, options ...modules.TreeStoreOption) (modules.TreeStoreModule, error) { - return new(treeStore).Create(bus, options...) +func Create(bus modules.Bus, options ...modules.ModuleOption) (modules.Module, error) { + return new(TreeStore).Create(bus, options...) } // WithLogger assigns a logger for the tree store -func WithLogger(logger *modules.Logger) modules.TreeStoreOption { - return func(m modules.TreeStoreModule) { - if mod, ok := m.(*treeStore); ok { +func WithLogger(logger *modules.Logger) modules.ModuleOption { + return func(m modules.InjectableModule) { + if mod, ok := m.(*TreeStore); ok { mod.logger = logger } } @@ -39,20 +42,37 @@ func WithLogger(logger *modules.Logger) modules.TreeStoreOption { // WithTreeStoreDirectory assigns the path where the tree store // saves its data. -func WithTreeStoreDirectory(path string) modules.TreeStoreOption { - return func(m modules.TreeStoreModule) { - if mod, ok := m.(*treeStore); ok { - mod.treeStoreDir = path +func WithTreeStoreDirectory(path string) modules.ModuleOption { + return func(m modules.InjectableModule) { + mod, ok := m.(*TreeStore) + if ok { + mod.TreeStoreDir = path } } } -func (t *treeStore) setupTrees() error { - if t.treeStoreDir == ":memory:" { +// WithTxIndexer assigns a TxIndexer for use during operation. +func WithTxIndexer(txi indexer.TxIndexer) modules.ModuleOption { + return func(m modules.InjectableModule) { + mod, ok := m.(*TreeStore) + if ok { + mod.TXI = txi + } + } +} + +func (t *TreeStore) GetModuleName() string { return modules.TreeStoreModuleName } +func (t *TreeStore) Start() error { return nil } +func (t *TreeStore) Stop() error { return nil } +func (t *TreeStore) GetBus() modules.Bus { return t.Bus } +func (t *TreeStore) SetBus(bus modules.Bus) { t.Bus = bus } + +func (t *TreeStore) setupTrees() error { + if t.TreeStoreDir == ":memory:" { return t.setupInMemory() } - nodeStore, err := kvstore.NewKVStore(fmt.Sprintf("%s/%s_nodes", t.treeStoreDir, RootTreeName)) + nodeStore, err := kvstore.NewKVStore(fmt.Sprintf("%s/%s_nodes", t.TreeStoreDir, RootTreeName)) if err != nil { return err } @@ -64,7 +84,7 @@ func (t *treeStore) setupTrees() error { t.merkleTrees = make(map[string]*stateTree, len(stateTreeNames)) for i := 0; i < len(stateTreeNames); i++ { - nodeStore, err := kvstore.NewKVStore(fmt.Sprintf("%s/%s_nodes", t.treeStoreDir, stateTreeNames[i])) + nodeStore, err := kvstore.NewKVStore(fmt.Sprintf("%s/%s_nodes", t.TreeStoreDir, stateTreeNames[i])) if err != nil { return err } @@ -78,7 +98,7 @@ func (t *treeStore) setupTrees() error { return nil } -func (t *treeStore) setupInMemory() error { +func (t *TreeStore) setupInMemory() error { nodeStore := kvstore.NewMemKVStore() t.rootTree = &stateTree{ name: RootTreeName, diff --git a/persistence/trees/module_test.go b/persistence/trees/module_test.go new file mode 100644 index 000000000..8172ead94 --- /dev/null +++ b/persistence/trees/module_test.go @@ -0,0 +1,121 @@ +package trees_test + +import ( + "fmt" + "testing" + + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" + + "github.com/pokt-network/pocket/internal/testutil" + "github.com/pokt-network/pocket/p2p/providers/current_height_provider" + "github.com/pokt-network/pocket/p2p/providers/peerstore_provider" + "github.com/pokt-network/pocket/persistence/trees" + "github.com/pokt-network/pocket/runtime" + "github.com/pokt-network/pocket/runtime/genesis" + "github.com/pokt-network/pocket/runtime/test_artifacts" + coreTypes "github.com/pokt-network/pocket/shared/core/types" + cryptoPocket "github.com/pokt-network/pocket/shared/crypto" + "github.com/pokt-network/pocket/shared/modules" + mockModules "github.com/pokt-network/pocket/shared/modules/mocks" +) + +const ( + serviceURLFormat = "node%d.consensus:42069" +) + +func TestTreeStore_Create(t *testing.T) { + ctrl := gomock.NewController(t) + mockRuntimeMgr := mockModules.NewMockRuntimeMgr(ctrl) + mockBus := createMockBus(t, mockRuntimeMgr) + + genesisStateMock := createMockGenesisState(nil) + persistenceMock := preparePersistenceMock(t, mockBus, genesisStateMock) + + mockBus.EXPECT().GetPersistenceModule().Return(persistenceMock).AnyTimes() + persistenceMock.EXPECT().GetBus().AnyTimes().Return(mockBus) + persistenceMock.EXPECT().NewRWContext(int64(0)).AnyTimes() + persistenceMock.EXPECT().GetTxIndexer().AnyTimes() + + treemod, err := trees.Create(mockBus, + trees.WithTreeStoreDirectory(":memory:")) + assert.NoError(t, err) + got := treemod.GetBus() + assert.Equal(t, got, mockBus) +} + +func TestTreeStore_DebugClearAll(t *testing.T) { + // TODO: Write test case for the DebugClearAll method + t.Skip("TODO: Write test case for DebugClearAll method") +} + +// createMockGenesisState configures and returns a mocked GenesisState +func createMockGenesisState(valKeys []cryptoPocket.PrivateKey) *genesis.GenesisState { + genesisState := new(genesis.GenesisState) + validators := make([]*coreTypes.Actor, len(valKeys)) + for i, valKey := range valKeys { + addr := valKey.Address().String() + mockActor := &coreTypes.Actor{ + ActorType: coreTypes.ActorType_ACTOR_TYPE_VAL, + Address: addr, + PublicKey: valKey.PublicKey().String(), + ServiceUrl: validatorId(i + 1), + StakedAmount: test_artifacts.DefaultStakeAmountString, + PausedHeight: int64(0), + UnstakingHeight: int64(0), + Output: addr, + } + validators[i] = mockActor + } + genesisState.Validators = validators + + return genesisState +} + +// Persistence mock - only needed for validatorMap access +func preparePersistenceMock(t *testing.T, busMock *mockModules.MockBus, genesisState *genesis.GenesisState) *mockModules.MockPersistenceModule { + ctrl := gomock.NewController(t) + + persistenceModuleMock := mockModules.NewMockPersistenceModule(ctrl) + readCtxMock := mockModules.NewMockPersistenceReadContext(ctrl) + + readCtxMock.EXPECT().GetAllValidators(gomock.Any()).Return(genesisState.GetValidators(), nil).AnyTimes() + readCtxMock.EXPECT().GetAllStakedActors(gomock.Any()).DoAndReturn(func(height int64) ([]*coreTypes.Actor, error) { + return testutil.Concatenate[*coreTypes.Actor]( + genesisState.GetValidators(), + genesisState.GetServicers(), + genesisState.GetFishermen(), + genesisState.GetApplications(), + ), nil + }).AnyTimes() + persistenceModuleMock.EXPECT().NewReadContext(gomock.Any()).Return(readCtxMock, nil).AnyTimes() + readCtxMock.EXPECT().Release().AnyTimes() + + persistenceModuleMock.EXPECT().GetBus().Return(busMock).AnyTimes() + persistenceModuleMock.EXPECT().SetBus(busMock).AnyTimes() + persistenceModuleMock.EXPECT().GetModuleName().Return(modules.PersistenceModuleName).AnyTimes() + busMock.RegisterModule(persistenceModuleMock) + + return persistenceModuleMock +} + +func validatorId(i int) string { + return fmt.Sprintf(serviceURLFormat, i) +} + +// createMockBus returns a mock bus with stubbed out functions for bus registration +func createMockBus(t *testing.T, runtimeMgr modules.RuntimeMgr) *mockModules.MockBus { + t.Helper() + ctrl := gomock.NewController(t) + mockBus := mockModules.NewMockBus(ctrl) + mockBus.EXPECT().GetRuntimeMgr().Return(runtimeMgr).AnyTimes() + mockBus.EXPECT().RegisterModule(gomock.Any()).DoAndReturn(func(m modules.Module) { + m.SetBus(mockBus) + }).AnyTimes() + mockModulesRegistry := mockModules.NewMockModulesRegistry(ctrl) + mockModulesRegistry.EXPECT().GetModule(peerstore_provider.PeerstoreProviderSubmoduleName).Return(nil, runtime.ErrModuleNotRegistered(peerstore_provider.PeerstoreProviderSubmoduleName)).AnyTimes() + mockModulesRegistry.EXPECT().GetModule(current_height_provider.ModuleName).Return(nil, runtime.ErrModuleNotRegistered(current_height_provider.ModuleName)).AnyTimes() + mockBus.EXPECT().GetModulesRegistry().Return(mockModulesRegistry).AnyTimes() + mockBus.EXPECT().PublishEventToBus(gomock.Any()).AnyTimes() + return mockBus +} diff --git a/persistence/trees/trees.go b/persistence/trees/trees.go index 64f210122..380032eb1 100644 --- a/persistence/trees/trees.go +++ b/persistence/trees/trees.go @@ -72,26 +72,29 @@ type stateTree struct { nodeStore kvstore.KVStore } -var _ modules.TreeStoreModule = &treeStore{} +var _ modules.TreeStoreModule = &TreeStore{} -// treeStore stores a set of merkle trees that +// TreeStore stores a set of merkle trees that // it manages. It fulfills the modules.TreeStore interface. // * It is responsible for atomic commit or rollback behavior // of the underlying trees by utilizing the lazy loading // functionality provided by the underlying smt library. -type treeStore struct { +type TreeStore struct { base_modules.IntegrableModule - logger *modules.Logger - treeStoreDir string + logger *modules.Logger + Bus modules.Bus + TXI indexer.TxIndexer + + TreeStoreDir string rootTree *stateTree merkleTrees map[string]*stateTree } -// GetTree returns the name, root hash, and nodeStore for the matching tree tree -// stored in the TreeStore. This enables the caller to import the smt and not -// change the one stored -func (t *treeStore) GetTree(name string) ([]byte, kvstore.KVStore) { +// GetTree returns the root hash and nodeStore for the matching tree +// stored in the TreeStore. This enables the caller to import the smt +// and not change the one stored. +func (t *TreeStore) GetTree(name string) ([]byte, kvstore.KVStore) { if name == RootTreeName { return t.rootTree.tree.Root(), t.rootTree.nodeStore } @@ -101,18 +104,21 @@ func (t *treeStore) GetTree(name string) ([]byte, kvstore.KVStore) { return nil, nil } -// Update takes a transaction and a height and updates -// all of the trees in the treeStore for that height. -func (t *treeStore) Update(pgtx pgx.Tx, height uint64) (string, error) { - txi := t.GetBus().GetPersistenceModule().GetTxIndexer() +// Update takes a pgx transaction and a height and updates all of the trees in the TreeStore for that height. +// It is atomic and handles its own savepoint and rollback creation. +func (t *TreeStore) Update(pgtx pgx.Tx, height uint64) (string, error) { t.logger.Info().Msgf("🌴 updating state trees at height %d", height) - return t.updateMerkleTrees(pgtx, txi, height) + stateHash, err := t.updateMerkleTrees(pgtx, t.TXI, height) + if err != nil { + return "", fmt.Errorf("failed to update merkle trees: %w", err) + } + return stateHash, nil } // DebugClearAll is used by the debug cli to completely reset all merkle trees. // This should only be called by the debug CLI. // TECHDEBT: Move this into a separate file with a debug build flag to avoid accidental usage in prod -func (t *treeStore) DebugClearAll() error { +func (t *TreeStore) DebugClearAll() error { if err := t.rootTree.nodeStore.ClearAll(); err != nil { return fmt.Errorf("failed to clear root node store: %w", err) } @@ -127,14 +133,11 @@ func (t *treeStore) DebugClearAll() error { return nil } -// GetModuleName implements the respective `TreeStoreModule` interface method. -func (t *treeStore) GetModuleName() string { - return modules.TreeStoreModuleName -} - // updateMerkleTrees updates all of the merkle trees in order defined by `numMerkleTrees` -// * it returns the new state hash capturing the state of all the trees or an error if one occurred -func (t *treeStore) updateMerkleTrees(pgtx pgx.Tx, txi indexer.TxIndexer, height uint64) (string, error) { +// * it returns the new state hash capturing the state of all the trees or an error if one occurred. +// * This function does not commit state to disk. The caller must manually invoke `Commit` to persist +// changes to disk. +func (t *TreeStore) updateMerkleTrees(pgtx pgx.Tx, txi indexer.TxIndexer, height uint64) (string, error) { for treeName := range t.merkleTrees { switch treeName { // Actor Merkle Trees @@ -173,7 +176,7 @@ func (t *treeStore) updateMerkleTrees(pgtx pgx.Tx, txi indexer.TxIndexer, height // Data Merkle Trees case TransactionsTreeName: - indexedTxs, err := sql.GetTransactions(txi, height) + indexedTxs, err := getTransactions(txi, height) if err != nil { return "", fmt.Errorf("failed to get transactions: %w", err) } @@ -210,24 +213,30 @@ func (t *treeStore) updateMerkleTrees(pgtx pgx.Tx, txi indexer.TxIndexer, height } } - if err := t.commit(); err != nil { + if err := t.Commit(); err != nil { return "", fmt.Errorf("failed to commit: %w", err) } return t.getStateHash(), nil } -func (t *treeStore) commit() error { - for treeName, stateTree := range t.merkleTrees { - if err := stateTree.tree.Commit(); err != nil { - return fmt.Errorf("failed to commit %s: %w", treeName, err) +func (t *TreeStore) Commit() error { + if err := t.rootTree.tree.Commit(); err != nil { + return fmt.Errorf("failed to commit root tree: %w", err) + } + + for name, treeStore := range t.merkleTrees { + if err := treeStore.tree.Commit(); err != nil { + return fmt.Errorf("failed to commit %s: %w", name, err) } } return nil } -func (t *treeStore) getStateHash() string { +func (t *TreeStore) getStateHash() string { for _, stateTree := range t.merkleTrees { - if err := t.rootTree.tree.Update([]byte(stateTree.name), stateTree.tree.Root()); err != nil { + key := []byte(stateTree.name) + val := stateTree.tree.Root() + if err := t.rootTree.tree.Update(key, val); err != nil { log.Fatalf("failed to update root tree with %s tree's hash: %v", stateTree.name, err) } } @@ -243,7 +252,7 @@ func (t *treeStore) getStateHash() string { //////////////////////// // NB: I think this needs to be done manually for all 4 types. -func (t *treeStore) updateActorsTree(actorType coreTypes.ActorType, actors []*coreTypes.Actor) error { +func (t *TreeStore) updateActorsTree(actorType coreTypes.ActorType, actors []*coreTypes.Actor) error { for _, actor := range actors { bzAddr, err := hex.DecodeString(actor.GetAddress()) if err != nil { @@ -271,7 +280,7 @@ func (t *treeStore) updateActorsTree(actorType coreTypes.ActorType, actors []*co // Account Tree Helpers // ////////////////////////// -func (t *treeStore) updateAccountTrees(accounts []*coreTypes.Account) error { +func (t *TreeStore) updateAccountTrees(accounts []*coreTypes.Account) error { for _, account := range accounts { bzAddr, err := hex.DecodeString(account.GetAddress()) if err != nil { @@ -291,7 +300,7 @@ func (t *treeStore) updateAccountTrees(accounts []*coreTypes.Account) error { return nil } -func (t *treeStore) updatePoolTrees(pools []*coreTypes.Account) error { +func (t *TreeStore) updatePoolTrees(pools []*coreTypes.Account) error { for _, pool := range pools { bzAddr, err := hex.DecodeString(pool.GetAddress()) if err != nil { @@ -315,7 +324,7 @@ func (t *treeStore) updatePoolTrees(pools []*coreTypes.Account) error { // Data Tree Helpers // /////////////////////// -func (t *treeStore) updateTransactionsTree(indexedTxs []*coreTypes.IndexedTransaction) error { +func (t *TreeStore) updateTransactionsTree(indexedTxs []*coreTypes.IndexedTransaction) error { for _, idxTx := range indexedTxs { txBz := idxTx.GetTx() txHash := crypto.SHA3Hash(txBz) @@ -326,7 +335,7 @@ func (t *treeStore) updateTransactionsTree(indexedTxs []*coreTypes.IndexedTransa return nil } -func (t *treeStore) updateParamsTree(params []*coreTypes.Param) error { +func (t *TreeStore) updateParamsTree(params []*coreTypes.Param) error { for _, param := range params { paramBz, err := codec.GetCodec().Marshal(param) paramKey := crypto.SHA3Hash([]byte(param.Name)) @@ -341,7 +350,7 @@ func (t *treeStore) updateParamsTree(params []*coreTypes.Param) error { return nil } -func (t *treeStore) updateFlagsTree(flags []*coreTypes.Flag) error { +func (t *TreeStore) updateFlagsTree(flags []*coreTypes.Flag) error { for _, flag := range flags { flagBz, err := codec.GetCodec().Marshal(flag) flagKey := crypto.SHA3Hash([]byte(flag.Name)) @@ -356,7 +365,7 @@ func (t *treeStore) updateFlagsTree(flags []*coreTypes.Flag) error { return nil } -func (t *treeStore) updateIBCTree(keys, values [][]byte) error { +func (t *TreeStore) updateIBCTree(keys, values [][]byte) error { if len(keys) != len(values) { return fmt.Errorf("keys and values must be the same length") } @@ -374,3 +383,13 @@ func (t *treeStore) updateIBCTree(keys, values [][]byte) error { } return nil } + +// getTransactions takes a transaction indexer and returns the transactions for the current height +func getTransactions(txi indexer.TxIndexer, height uint64) ([]*coreTypes.IndexedTransaction, error) { + // TECHDEBT(#813): Avoid this cast to int64 + indexedTxs, err := txi.GetByHeight(int64(height), false) + if err != nil { + return nil, fmt.Errorf("failed to get transactions by height: %w", err) + } + return indexedTxs, nil +} diff --git a/runtime/bus.go b/runtime/bus.go index 2b607ac8e..f5d3aa19d 100644 --- a/runtime/bus.go +++ b/runtime/bus.go @@ -71,6 +71,10 @@ func (m *bus) GetPersistenceModule() modules.PersistenceModule { return getModuleFromRegistry[modules.PersistenceModule](m, modules.PersistenceModuleName) } +func (m *bus) GetTreeStoreModule() modules.TreeStoreModule { + return getModuleFromRegistry[modules.TreeStoreModule](m, modules.TreeStoreModuleName) +} + func (m *bus) GetP2PModule() modules.P2PModule { return getModuleFromRegistry[modules.P2PModule](m, modules.P2PModuleName) } diff --git a/shared/modules/persistence_module.go b/shared/modules/persistence_module.go index b5d17d65a..923d12593 100644 --- a/shared/modules/persistence_module.go +++ b/shared/modules/persistence_module.go @@ -1,6 +1,6 @@ package modules -//go:generate mockgen -destination=./mocks/persistence_module_mock.go github.com/pokt-network/pocket/shared/modules PersistenceModule,PersistenceRWContext,PersistenceReadContext,PersistenceWriteContext,PersistenceLocalContext +//go:generate mockgen -destination=./mocks/persistence_module_mock.go github.com/pokt-network/pocket/shared/modules PersistenceModule,PersistenceRWContext,PersistenceReadContext,PersistenceWriteContext,PersistenceLocalContext,TreeStoreModule import ( "math/big" From 34d4e8242e00aada4d30abc042154925979d2d71 Mon Sep 17 00:00:00 2001 From: shakezula Date: Wed, 12 Jul 2023 09:07:21 -0600 Subject: [PATCH 02/20] updates TreeStore to use Submodule type --- persistence/module.go | 3 +-- persistence/trees/module.go | 12 ++++-------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/persistence/module.go b/persistence/module.go index 192380fbb..cc0920929 100644 --- a/persistence/module.go +++ b/persistence/module.go @@ -106,8 +106,7 @@ func (*persistenceModule) Create(bus modules.Bus, options ...modules.ModuleOptio _, err = trees.Create( bus, trees.WithTreeStoreDirectory(persistenceCfg.TreesStoreDir), - trees.WithLogger(m.logger), - ) + trees.WithLogger(m.logger)) if err != nil { return nil, fmt.Errorf("failed to create TreeStoreModule: %w", err) } diff --git a/persistence/trees/module.go b/persistence/trees/module.go index f85e0c54d..58756b61a 100644 --- a/persistence/trees/module.go +++ b/persistence/trees/module.go @@ -9,9 +9,9 @@ import ( "github.com/pokt-network/smt" ) -var _ modules.Module = &TreeStore{} +var _ modules.Submodule = &TreeStore{} -func (*TreeStore) Create(bus modules.Bus, options ...modules.ModuleOption) (modules.Module, error) { +func (*TreeStore) Create(bus modules.Bus, options ...modules.ModuleOption) (modules.TreeStoreModule, error) { m := &TreeStore{} bus.RegisterModule(m) @@ -27,7 +27,7 @@ func (*TreeStore) Create(bus modules.Bus, options ...modules.ModuleOption) (modu return m, nil } -func Create(bus modules.Bus, options ...modules.ModuleOption) (modules.Module, error) { +func Create(bus modules.Bus, options ...modules.ModuleOption) (modules.TreeStoreModule, error) { return new(TreeStore).Create(bus, options...) } @@ -61,11 +61,7 @@ func WithTxIndexer(txi indexer.TxIndexer) modules.ModuleOption { } } -func (t *TreeStore) GetModuleName() string { return modules.TreeStoreModuleName } -func (t *TreeStore) Start() error { return nil } -func (t *TreeStore) Stop() error { return nil } -func (t *TreeStore) GetBus() modules.Bus { return t.Bus } -func (t *TreeStore) SetBus(bus modules.Bus) { t.Bus = bus } +func (t *TreeStore) GetModuleName() string { return modules.TreeStoreModuleName } func (t *TreeStore) setupTrees() error { if t.TreeStoreDir == ":memory:" { From 8fdebb8b639d40c584d8ed79d2a84f62fbceb83e Mon Sep 17 00:00:00 2001 From: shakezula Date: Wed, 12 Jul 2023 13:43:40 -0600 Subject: [PATCH 03/20] [fixup] fixes mocks for treestore module --- persistence/trees/module_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/persistence/trees/module_test.go b/persistence/trees/module_test.go index 8172ead94..c1a6b2ff3 100644 --- a/persistence/trees/module_test.go +++ b/persistence/trees/module_test.go @@ -109,7 +109,7 @@ func createMockBus(t *testing.T, runtimeMgr modules.RuntimeMgr) *mockModules.Moc ctrl := gomock.NewController(t) mockBus := mockModules.NewMockBus(ctrl) mockBus.EXPECT().GetRuntimeMgr().Return(runtimeMgr).AnyTimes() - mockBus.EXPECT().RegisterModule(gomock.Any()).DoAndReturn(func(m modules.Module) { + mockBus.EXPECT().RegisterModule(gomock.Any()).DoAndReturn(func(m modules.Submodule) { m.SetBus(mockBus) }).AnyTimes() mockModulesRegistry := mockModules.NewMockModulesRegistry(ctrl) From aba1bb6ea66b727c1b40098308ab07129314293d Mon Sep 17 00:00:00 2001 From: shakezula Date: Wed, 12 Jul 2023 14:14:20 -0600 Subject: [PATCH 04/20] [fixup] embeds TreeStoreFactory on to TreeStoreModule --- persistence/trees/module_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/persistence/trees/module_test.go b/persistence/trees/module_test.go index c1a6b2ff3..f5745d1c6 100644 --- a/persistence/trees/module_test.go +++ b/persistence/trees/module_test.go @@ -28,7 +28,6 @@ func TestTreeStore_Create(t *testing.T) { ctrl := gomock.NewController(t) mockRuntimeMgr := mockModules.NewMockRuntimeMgr(ctrl) mockBus := createMockBus(t, mockRuntimeMgr) - genesisStateMock := createMockGenesisState(nil) persistenceMock := preparePersistenceMock(t, mockBus, genesisStateMock) @@ -40,6 +39,7 @@ func TestTreeStore_Create(t *testing.T) { treemod, err := trees.Create(mockBus, trees.WithTreeStoreDirectory(":memory:")) assert.NoError(t, err) + treemod.GetModuleName() got := treemod.GetBus() assert.Equal(t, got, mockBus) } From 342c611e232705302fad7d3894029fc0e5d3bb7c Mon Sep 17 00:00:00 2001 From: shakezula Date: Wed, 12 Jul 2023 14:14:50 -0600 Subject: [PATCH 05/20] [fixup] TreeStoreOption and TreeStoreModule updates --- persistence/trees/module.go | 18 +++++++++--------- shared/modules/treestore_module.go | 1 + 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/persistence/trees/module.go b/persistence/trees/module.go index 58756b61a..f84bd0a5f 100644 --- a/persistence/trees/module.go +++ b/persistence/trees/module.go @@ -9,9 +9,9 @@ import ( "github.com/pokt-network/smt" ) -var _ modules.Submodule = &TreeStore{} +var _ modules.TreeStoreModule = &TreeStore{} -func (*TreeStore) Create(bus modules.Bus, options ...modules.ModuleOption) (modules.TreeStoreModule, error) { +func (*TreeStore) Create(bus modules.Bus, options ...modules.TreeStoreOption) (modules.TreeStoreModule, error) { m := &TreeStore{} bus.RegisterModule(m) @@ -27,13 +27,13 @@ func (*TreeStore) Create(bus modules.Bus, options ...modules.ModuleOption) (modu return m, nil } -func Create(bus modules.Bus, options ...modules.ModuleOption) (modules.TreeStoreModule, error) { +func Create(bus modules.Bus, options ...modules.TreeStoreOption) (modules.TreeStoreModule, error) { return new(TreeStore).Create(bus, options...) } // WithLogger assigns a logger for the tree store -func WithLogger(logger *modules.Logger) modules.ModuleOption { - return func(m modules.InjectableModule) { +func WithLogger(logger *modules.Logger) modules.TreeStoreOption { + return func(m modules.TreeStoreModule) { if mod, ok := m.(*TreeStore); ok { mod.logger = logger } @@ -42,8 +42,8 @@ func WithLogger(logger *modules.Logger) modules.ModuleOption { // WithTreeStoreDirectory assigns the path where the tree store // saves its data. -func WithTreeStoreDirectory(path string) modules.ModuleOption { - return func(m modules.InjectableModule) { +func WithTreeStoreDirectory(path string) modules.TreeStoreOption { + return func(m modules.TreeStoreModule) { mod, ok := m.(*TreeStore) if ok { mod.TreeStoreDir = path @@ -52,8 +52,8 @@ func WithTreeStoreDirectory(path string) modules.ModuleOption { } // WithTxIndexer assigns a TxIndexer for use during operation. -func WithTxIndexer(txi indexer.TxIndexer) modules.ModuleOption { - return func(m modules.InjectableModule) { +func WithTxIndexer(txi indexer.TxIndexer) modules.TreeStoreOption { + return func(m modules.TreeStoreModule) { mod, ok := m.(*TreeStore) if ok { mod.TXI = txi diff --git a/shared/modules/treestore_module.go b/shared/modules/treestore_module.go index a2aedfb8e..8a0bc95ad 100644 --- a/shared/modules/treestore_module.go +++ b/shared/modules/treestore_module.go @@ -19,6 +19,7 @@ type TreeStoreFactory = FactoryWithOptions[TreeStoreModule, TreeStoreOption] // merkle trees that compose the state hash of pocket. type TreeStoreModule interface { Submodule + TreeStoreFactory // Update returns the new state hash for a given height. // * Height is passed through to the Update function and is used to query the TxIndexer for transactions From a3b6d34792180b4f871a0675261ccbff27a84e6d Mon Sep 17 00:00:00 2001 From: shakezula Date: Wed, 12 Jul 2023 14:15:18 -0600 Subject: [PATCH 06/20] [docs] updates the Modules readme to reflect some best practices --- shared/modules/doc/README.md | 74 ++++++++++++++++++++++-------------- 1 file changed, 46 insertions(+), 28 deletions(-) diff --git a/shared/modules/doc/README.md b/shared/modules/doc/README.md index 28a2c249b..f48ce3b00 100644 --- a/shared/modules/doc/README.md +++ b/shared/modules/doc/README.md @@ -6,32 +6,34 @@ This document outlines how we structured the code by splitting it into modules, - [tl;dr Just show me an example](#tldr-just-show-me-an-example) - [Definitions](#definitions) - - [Requirement Level Keywords](#requirement-level-keywords) - - [Module](#module) - - [Module mock](#module-mock) - - [Shared module interfaces](#shared-module-interfaces) - - [Base module](#base-module) - - [Submodule](#submodule) - - [Shared submodule](#shared-submodule) - - [Class Diagram Legend](#class-diagram-legend) - - [Module, Submodule \& Shared Interfaces](#module-submodule--shared-interfaces) - - [Factory interfaces](#factory-interfaces) + - [Requirement Level Keywords](#requirement-level-keywords) + - [Module](#module) + - [Module mock](#module-mock) + - [Shared module interfaces](#shared-module-interfaces) + - [Base module](#base-module) + - [Submodule](#submodule) + - [Shared submodule](#shared-submodule) + - [Class Diagram Legend](#class-diagram-legend) + - [Module, Submodule \& Shared Interfaces](#module-submodule--shared-interfaces) + - [Factory interfaces](#factory-interfaces) - [Code Organization](#code-organization) -- [(Sub)Modules in detail](#submodules-in-detail) - [Shared (sub)module interfaces](#shared-submodule-interfaces) - [Construction parameters \& non-(sub)module dependencies](#construction-parameters--non-submodule-dependencies) - - [Module creation](#module-creation) - - [Module configs \& options](#module-configs--options) - - [Submodule creation](#submodule-creation) - - [Submodule configs \& options](#submodule-configs--options) - - [Configs](#configs) - - [Options](#options) - - [Comprehensive Submodule Example:](#comprehensive-submodule-example) - - [Interacting \& Registering with the `bus`](#interacting--registering-with-the-bus) - - [Modules Registry](#modules-registry) - - [Modules Registry Example](#modules-registry-example) - - [Start the module](#start-the-module) - - [Add a logger to the module](#add-a-logger-to-the-module) - - [Get the module `bus`](#get-the-module-bus) - - [Stop the module](#stop-the-module) +- [(Sub)Modules in detail](#submodules-in-detail) + - [Shared (sub)module interfaces](#shared-submodule-interfaces) + - [Construction parameters \& non-(sub)module dependencies](#construction-parameters--non-submodule-dependencies) + - [Module creation](#module-creation) + - [Module configs \& options](#module-configs--options) + - [Submodule creation](#submodule-creation) + - [Submodule configs \& options](#submodule-configs--options) + - [Configs](#configs) + - [Options](#options) + - [Comprehensive Submodule Example:](#comprehensive-submodule-example) + - [Interacting \& Registering with the `bus`](#interacting--registering-with-the-bus) + - [Modules Registry](#modules-registry) + - [Modules Registry Example](#modules-registry-example) + - [Start the module](#start-the-module) + - [Add a logger to the module](#add-a-logger-to-the-module) + - [Get the module `bus`](#get-the-module-bus) + - [Stop the module](#stop-the-module) ## tl;dr Just show me an example @@ -420,8 +422,8 @@ What the `bus` does is setting its reference to the module instance and delegati ```golang func (m *bus) RegisterModule(module modules.Module) { - module.SetBus(m) - m.modulesRegistry.RegisterModule(module) + module.SetBus(m) + m.modulesRegistry.RegisterModule(module) } ``` @@ -432,7 +434,23 @@ This is quite **important** because it unlocks a powerful concept **Dependency I This enables the developer to define different implementations of a module and to register the one that is needed at runtime. This is because we can only have one module registered with a unique name and also because, by convention, we keep module names defined as constants. This is useful not only for prototyping but also for different use cases such as the `p1` CLI and the `pocket` binary where different implementations of the same module are necessary due to the fact that the `p1` CLI doesn't have a persistence module but still needs to know what's going on in the network. -Submodules can be registered the same way full Modules can be, by passing the Submodule to the RegisterModule function. Submodules should typically be registered to the bus for dependency injection reasons. +Submodules can be registered the same way full Modules can be, by passing the Submodule to the RegisterModule function. Submodules should typically be registered to the bus for dependency injection reasons. Modules and submodules are all responsible for registering themselves with the Bus. This pattern emerged organically during development and is now considered best practice. Additionally, modules should not maintain pointer references to modules and should instead call the Bus to get a new reference to a module whenever they need to call that module. + +Submodule interfaces are typically defined in the `shared/modules` package with the rest of the module interfaces in a file named `XXX_module.go`, where XXX denotes the name of the submodule. That same file should contain the factory function definition for a submodule which should be embedded by the Submodule interface type. Modules should follow the same pattern but embed the Module interface instead of the Submodule interface in the module's interface declaration. + +For example, in the TreeStore code below, you can see that the TreeStoreFactory is embedded in the TreeStoreModule, which also embeds the Submodule interface. + +```go +type TreeStoreFactory = FactoryWithOptions[TreeStoreModule, TreeStoreOption] + +// TreeStoreModules defines the interface for atomic updates and rollbacks to the internal +// merkle trees that compose the state hash of pocket. +type TreeStoreModule interface { + Submodule + TreeStoreFactory + // ... +} +``` ##### Modules Registry Example From 3beb52864e4427aed21b7f5e2947495ac4f416b7 Mon Sep 17 00:00:00 2001 From: shakezula Date: Wed, 12 Jul 2023 15:25:03 -0600 Subject: [PATCH 07/20] [fixup] removes bus and txindexer from TreeStore struct and only fetches them at runtime --- persistence/module.go | 12 ++++++------ persistence/trees/module.go | 11 ----------- persistence/trees/trees.go | 5 ++--- 3 files changed, 8 insertions(+), 20 deletions(-) diff --git a/persistence/module.go b/persistence/module.go index cc0920929..ab5ed9d50 100644 --- a/persistence/module.go +++ b/persistence/module.go @@ -41,9 +41,9 @@ type persistenceModule struct { // IMPORTANT: It doubles as the data store for the transaction tree in state tree set. txIndexer indexer.TxIndexer - // stateTrees manages all of the merkle trees maintained by the + // treeStore manages all of the merkle trees maintained by the // persistence module that roll up into the state commitment. - stateTrees modules.TreeStoreModule + treeStore modules.TreeStoreModule // Only one write context is allowed at a time writeContext *PostgresContext @@ -119,7 +119,7 @@ func (*persistenceModule) Create(bus modules.Bus, options ...modules.ModuleOptio m.blockStore = blockStore // TECHDEBT: fetch txIndexer from bus m.txIndexer = txIndexer - m.stateTrees = m.GetBus().GetTreeStore() + m.treeStore = m.GetBus().GetTreeStore() // TECHDEBT: reconsider if this is the best place to call `populateGenesisState`. Note that // this forces the genesis state to be reloaded on every node startup until state @@ -182,7 +182,7 @@ func (m *persistenceModule) NewRWContext(height int64) (modules.PersistenceRWCon stateHash: "", blockStore: m.blockStore, txIndexer: m.txIndexer, - stateTrees: m.stateTrees, + stateTrees: m.treeStore, networkId: m.networkId, } @@ -214,7 +214,7 @@ func (m *persistenceModule) NewReadContext(height int64) (modules.PersistenceRea stateHash: "", blockStore: m.blockStore, txIndexer: m.txIndexer, - stateTrees: m.stateTrees, + stateTrees: m.treeStore, networkId: m.networkId, }, nil } @@ -241,7 +241,7 @@ func (m *persistenceModule) GetTxIndexer() indexer.TxIndexer { } func (m *persistenceModule) GetTreeStore() modules.TreeStoreModule { - return m.stateTrees + return m.treeStore } func (m *persistenceModule) GetNetworkID() string { diff --git a/persistence/trees/module.go b/persistence/trees/module.go index f84bd0a5f..00e8f6879 100644 --- a/persistence/trees/module.go +++ b/persistence/trees/module.go @@ -3,7 +3,6 @@ package trees import ( "fmt" - "github.com/pokt-network/pocket/persistence/indexer" "github.com/pokt-network/pocket/persistence/kvstore" "github.com/pokt-network/pocket/shared/modules" "github.com/pokt-network/smt" @@ -51,16 +50,6 @@ func WithTreeStoreDirectory(path string) modules.TreeStoreOption { } } -// WithTxIndexer assigns a TxIndexer for use during operation. -func WithTxIndexer(txi indexer.TxIndexer) modules.TreeStoreOption { - return func(m modules.TreeStoreModule) { - mod, ok := m.(*TreeStore) - if ok { - mod.TXI = txi - } - } -} - func (t *TreeStore) GetModuleName() string { return modules.TreeStoreModuleName } func (t *TreeStore) setupTrees() error { diff --git a/persistence/trees/trees.go b/persistence/trees/trees.go index 380032eb1..dd5c01357 100644 --- a/persistence/trees/trees.go +++ b/persistence/trees/trees.go @@ -83,8 +83,6 @@ type TreeStore struct { base_modules.IntegrableModule logger *modules.Logger - Bus modules.Bus - TXI indexer.TxIndexer TreeStoreDir string rootTree *stateTree @@ -108,7 +106,8 @@ func (t *TreeStore) GetTree(name string) ([]byte, kvstore.KVStore) { // It is atomic and handles its own savepoint and rollback creation. func (t *TreeStore) Update(pgtx pgx.Tx, height uint64) (string, error) { t.logger.Info().Msgf("🌴 updating state trees at height %d", height) - stateHash, err := t.updateMerkleTrees(pgtx, t.TXI, height) + txi := t.GetBus().GetPersistenceModule().GetTxIndexer() + stateHash, err := t.updateMerkleTrees(pgtx, txi, height) if err != nil { return "", fmt.Errorf("failed to update merkle trees: %w", err) } From b98ddec00426ad228d7b0440824acf0d041da6d0 Mon Sep 17 00:00:00 2001 From: shakezula Date: Mon, 17 Jul 2023 15:54:31 -0600 Subject: [PATCH 08/20] [fixup] get height provider from bus in tree tests * fixes treestore module tests to use the bus for fetching the height provider instead of accessing it from the package --- persistence/trees/module_test.go | 3 +-- shared/modules/persistence_module.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/persistence/trees/module_test.go b/persistence/trees/module_test.go index f5745d1c6..f5bc8d491 100644 --- a/persistence/trees/module_test.go +++ b/persistence/trees/module_test.go @@ -8,7 +8,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/pokt-network/pocket/internal/testutil" - "github.com/pokt-network/pocket/p2p/providers/current_height_provider" "github.com/pokt-network/pocket/p2p/providers/peerstore_provider" "github.com/pokt-network/pocket/persistence/trees" "github.com/pokt-network/pocket/runtime" @@ -114,7 +113,7 @@ func createMockBus(t *testing.T, runtimeMgr modules.RuntimeMgr) *mockModules.Moc }).AnyTimes() mockModulesRegistry := mockModules.NewMockModulesRegistry(ctrl) mockModulesRegistry.EXPECT().GetModule(peerstore_provider.PeerstoreProviderSubmoduleName).Return(nil, runtime.ErrModuleNotRegistered(peerstore_provider.PeerstoreProviderSubmoduleName)).AnyTimes() - mockModulesRegistry.EXPECT().GetModule(current_height_provider.ModuleName).Return(nil, runtime.ErrModuleNotRegistered(current_height_provider.ModuleName)).AnyTimes() + mockModulesRegistry.EXPECT().GetModule(modules.CurrentHeightProviderSubmoduleName).Return(nil, runtime.ErrModuleNotRegistered(modules.CurrentHeightProviderSubmoduleName)).AnyTimes() mockBus.EXPECT().GetModulesRegistry().Return(mockModulesRegistry).AnyTimes() mockBus.EXPECT().PublishEventToBus(gomock.Any()).AnyTimes() return mockBus diff --git a/shared/modules/persistence_module.go b/shared/modules/persistence_module.go index 923d12593..b5d17d65a 100644 --- a/shared/modules/persistence_module.go +++ b/shared/modules/persistence_module.go @@ -1,6 +1,6 @@ package modules -//go:generate mockgen -destination=./mocks/persistence_module_mock.go github.com/pokt-network/pocket/shared/modules PersistenceModule,PersistenceRWContext,PersistenceReadContext,PersistenceWriteContext,PersistenceLocalContext,TreeStoreModule +//go:generate mockgen -destination=./mocks/persistence_module_mock.go github.com/pokt-network/pocket/shared/modules PersistenceModule,PersistenceRWContext,PersistenceReadContext,PersistenceWriteContext,PersistenceLocalContext import ( "math/big" From c00445667a3317e59769c52872757441f2b892b3 Mon Sep 17 00:00:00 2001 From: shakezula Date: Mon, 17 Jul 2023 16:08:45 -0600 Subject: [PATCH 09/20] [fixup] only use bus to fetch TreeStore * this commit updates persistence module to only fetch the TreeStore from the bus instead of maintaining a reference to it at Create time. --- persistence/module.go | 13 ++----------- shared/modules/persistence_module.go | 3 --- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/persistence/module.go b/persistence/module.go index ab5ed9d50..0d6acd17f 100644 --- a/persistence/module.go +++ b/persistence/module.go @@ -41,10 +41,6 @@ type persistenceModule struct { // IMPORTANT: It doubles as the data store for the transaction tree in state tree set. txIndexer indexer.TxIndexer - // treeStore manages all of the merkle trees maintained by the - // persistence module that roll up into the state commitment. - treeStore modules.TreeStoreModule - // Only one write context is allowed at a time writeContext *PostgresContext @@ -119,7 +115,6 @@ func (*persistenceModule) Create(bus modules.Bus, options ...modules.ModuleOptio m.blockStore = blockStore // TECHDEBT: fetch txIndexer from bus m.txIndexer = txIndexer - m.treeStore = m.GetBus().GetTreeStore() // TECHDEBT: reconsider if this is the best place to call `populateGenesisState`. Note that // this forces the genesis state to be reloaded on every node startup until state @@ -182,7 +177,7 @@ func (m *persistenceModule) NewRWContext(height int64) (modules.PersistenceRWCon stateHash: "", blockStore: m.blockStore, txIndexer: m.txIndexer, - stateTrees: m.treeStore, + stateTrees: m.GetBus().GetTreeStore(), networkId: m.networkId, } @@ -214,7 +209,7 @@ func (m *persistenceModule) NewReadContext(height int64) (modules.PersistenceRea stateHash: "", blockStore: m.blockStore, txIndexer: m.txIndexer, - stateTrees: m.treeStore, + stateTrees: m.GetBus().GetTreeStore(), networkId: m.networkId, }, nil } @@ -240,10 +235,6 @@ func (m *persistenceModule) GetTxIndexer() indexer.TxIndexer { return m.txIndexer } -func (m *persistenceModule) GetTreeStore() modules.TreeStoreModule { - return m.treeStore -} - func (m *persistenceModule) GetNetworkID() string { return m.networkId } diff --git a/shared/modules/persistence_module.go b/shared/modules/persistence_module.go index b5d17d65a..072ae03b5 100644 --- a/shared/modules/persistence_module.go +++ b/shared/modules/persistence_module.go @@ -35,9 +35,6 @@ type PersistenceModule interface { GetTxIndexer() indexer.TxIndexer TransactionExists(transactionHash string) (bool, error) - // TreeStore operations - GetTreeStore() TreeStoreModule - // Debugging / development only HandleDebugMessage(*messaging.DebugMessage) error From 614597b2ff10d32c264cf099addda40fa68c1101 Mon Sep 17 00:00:00 2001 From: shakezula Date: Mon, 17 Jul 2023 16:11:05 -0600 Subject: [PATCH 10/20] [fixup] renames TreeStoreModuleName * renames TreeStoreModuleName to TreeStoreSubmoduleName --- ibc/store/provable_store_test.go | 2 +- persistence/trees/module.go | 2 +- runtime/bus.go | 4 ++-- shared/modules/treestore_module.go | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ibc/store/provable_store_test.go b/ibc/store/provable_store_test.go index f5739e359..10fdaf3c0 100644 --- a/ibc/store/provable_store_test.go +++ b/ibc/store/provable_store_test.go @@ -465,7 +465,7 @@ func newTreeStoreMock(t *testing.T, ctrl := gomock.NewController(t) treeStoreMock := mockModules.NewMockTreeStoreModule(ctrl) - treeStoreMock.EXPECT().GetModuleName().Return(modules.TreeStoreModuleName).AnyTimes() + treeStoreMock.EXPECT().GetModuleName().Return(modules.TreeStoreSubmoduleName).AnyTimes() treeStoreMock.EXPECT().SetBus(gomock.Any()).Return().AnyTimes() treeStoreMock.EXPECT().GetBus().Return(bus).AnyTimes() diff --git a/persistence/trees/module.go b/persistence/trees/module.go index 00e8f6879..2a798f497 100644 --- a/persistence/trees/module.go +++ b/persistence/trees/module.go @@ -50,7 +50,7 @@ func WithTreeStoreDirectory(path string) modules.TreeStoreOption { } } -func (t *TreeStore) GetModuleName() string { return modules.TreeStoreModuleName } +func (t *TreeStore) GetModuleName() string { return modules.TreeStoreSubmoduleName } func (t *TreeStore) setupTrees() error { if t.TreeStoreDir == ":memory:" { diff --git a/runtime/bus.go b/runtime/bus.go index f5d3aa19d..9d7a6e05f 100644 --- a/runtime/bus.go +++ b/runtime/bus.go @@ -72,7 +72,7 @@ func (m *bus) GetPersistenceModule() modules.PersistenceModule { } func (m *bus) GetTreeStoreModule() modules.TreeStoreModule { - return getModuleFromRegistry[modules.TreeStoreModule](m, modules.TreeStoreModuleName) + return getModuleFromRegistry[modules.TreeStoreModule](m, modules.TreeStoreSubmoduleName) } func (m *bus) GetP2PModule() modules.P2PModule { @@ -128,7 +128,7 @@ func (m *bus) GetIBCModule() modules.IBCModule { } func (m *bus) GetTreeStore() modules.TreeStoreModule { - return getModuleFromRegistry[modules.TreeStoreModule](m, modules.TreeStoreModuleName) + return getModuleFromRegistry[modules.TreeStoreModule](m, modules.TreeStoreSubmoduleName) } func (m *bus) GetIBCHost() modules.IBCHostSubmodule { diff --git a/shared/modules/treestore_module.go b/shared/modules/treestore_module.go index 8a0bc95ad..e1f93050b 100644 --- a/shared/modules/treestore_module.go +++ b/shared/modules/treestore_module.go @@ -8,7 +8,7 @@ import ( //go:generate mockgen -destination=./mocks/treestore_module_mock.go github.com/pokt-network/pocket/shared/modules TreeStoreModule const ( - TreeStoreModuleName = "tree_store" + TreeStoreSubmoduleName = "tree_store" ) type TreeStoreOption func(TreeStoreModule) From a6f8b27f7f832eda483e6ddf633b7978df9b701a Mon Sep 17 00:00:00 2001 From: shakezula Date: Mon, 17 Jul 2023 16:59:38 -0600 Subject: [PATCH 11/20] [fixup] renames tree store factory function * renames the tree store's factory function to treeStoreFactory instead of TreeStoreFactory to keep it private. --- shared/modules/doc/README.md | 8 ++++---- shared/modules/treestore_module.go | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/shared/modules/doc/README.md b/shared/modules/doc/README.md index f48ce3b00..6c187aae8 100644 --- a/shared/modules/doc/README.md +++ b/shared/modules/doc/README.md @@ -436,18 +436,18 @@ This is useful not only for prototyping but also for different use cases such as Submodules can be registered the same way full Modules can be, by passing the Submodule to the RegisterModule function. Submodules should typically be registered to the bus for dependency injection reasons. Modules and submodules are all responsible for registering themselves with the Bus. This pattern emerged organically during development and is now considered best practice. Additionally, modules should not maintain pointer references to modules and should instead call the Bus to get a new reference to a module whenever they need to call that module. -Submodule interfaces are typically defined in the `shared/modules` package with the rest of the module interfaces in a file named `XXX_module.go`, where XXX denotes the name of the submodule. That same file should contain the factory function definition for a submodule which should be embedded by the Submodule interface type. Modules should follow the same pattern but embed the Module interface instead of the Submodule interface in the module's interface declaration. +Submodule interfaces are typically defined in the `shared/modules` package with the rest of the module interfaces in a file named `XXX_submodule.go`, where XXX denotes the name of the submodule. That same file should contain the factory function definition for a submodule which should be embedded by the `Submodule` interface type. Factory function definitions should typically not be exported. -For example, in the TreeStore code below, you can see that the TreeStoreFactory is embedded in the TreeStoreModule, which also embeds the Submodule interface. +For example, in the TreeStore code below, the `treeStoreFactory` is defined and then embedded in the `TreeStoreModule`, which also embeds the Submodule interface. Typically these factory functions should be kept private at the package level. ```go -type TreeStoreFactory = FactoryWithOptions[TreeStoreModule, TreeStoreOption] +type treeStoreFactory = FactoryWithOptions[TreeStoreModule, TreeStoreOption] // TreeStoreModules defines the interface for atomic updates and rollbacks to the internal // merkle trees that compose the state hash of pocket. type TreeStoreModule interface { Submodule - TreeStoreFactory + treeStoreFactory // ... } ``` diff --git a/shared/modules/treestore_module.go b/shared/modules/treestore_module.go index e1f93050b..908749bed 100644 --- a/shared/modules/treestore_module.go +++ b/shared/modules/treestore_module.go @@ -13,13 +13,13 @@ const ( type TreeStoreOption func(TreeStoreModule) -type TreeStoreFactory = FactoryWithOptions[TreeStoreModule, TreeStoreOption] +type treeStoreFactory = FactoryWithOptions[TreeStoreModule, TreeStoreOption] // TreeStoreModules defines the interface for atomic updates and rollbacks to the internal // merkle trees that compose the state hash of pocket. type TreeStoreModule interface { Submodule - TreeStoreFactory + treeStoreFactory // Update returns the new state hash for a given height. // * Height is passed through to the Update function and is used to query the TxIndexer for transactions From 4b3ce442e3b983811a7bee36c0869d589642be18 Mon Sep 17 00:00:00 2001 From: shakezula Date: Tue, 18 Jul 2023 12:33:26 -0600 Subject: [PATCH 12/20] [docs] adds techdebt comment about privatization to TreeStore struct --- persistence/trees/trees.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/persistence/trees/trees.go b/persistence/trees/trees.go index dd5c01357..0a1e6ea43 100644 --- a/persistence/trees/trees.go +++ b/persistence/trees/trees.go @@ -74,24 +74,25 @@ type stateTree struct { var _ modules.TreeStoreModule = &TreeStore{} -// TreeStore stores a set of merkle trees that -// it manages. It fulfills the modules.TreeStore interface. -// * It is responsible for atomic commit or rollback behavior -// of the underlying trees by utilizing the lazy loading -// functionality provided by the underlying smt library. +// TreeStore stores a set of merkle trees that it manages. +// It fulfills the modules.TreeStore interface +// * It is responsible for atomic commit or rollback behavior of the underlying +// trees by utilizing the lazy loading functionality of the smt library. +// TECHDEBT(#880): TreeStore is exported for testing purposes to avoid import cycle errors. +// Make it private and export a custom struct with a test build tag when necessary. type TreeStore struct { base_modules.IntegrableModule logger *modules.Logger - TreeStoreDir string + treeStoreDir string rootTree *stateTree merkleTrees map[string]*stateTree } -// GetTree returns the root hash and nodeStore for the matching tree -// stored in the TreeStore. This enables the caller to import the smt -// and not change the one stored. +// GetTree returns the root hash and nodeStore for the matching tree stored in the TreeStore. +// This enables the caller to import the SMT without changing the one stored unless they call +// `Commit()` to write to the nodestore. func (t *TreeStore) GetTree(name string) ([]byte, kvstore.KVStore) { if name == RootTreeName { return t.rootTree.tree.Root(), t.rootTree.nodeStore From 7ce02c0f4ffc14d5966f5ca8b7172e25fb2c26cb Mon Sep 17 00:00:00 2001 From: shakezula Date: Tue, 18 Jul 2023 12:34:33 -0600 Subject: [PATCH 13/20] [fixup] privatize treeStoreDir field on TreeStore struct --- persistence/trees/module.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/persistence/trees/module.go b/persistence/trees/module.go index 2a798f497..14aaa9eff 100644 --- a/persistence/trees/module.go +++ b/persistence/trees/module.go @@ -45,7 +45,7 @@ func WithTreeStoreDirectory(path string) modules.TreeStoreOption { return func(m modules.TreeStoreModule) { mod, ok := m.(*TreeStore) if ok { - mod.TreeStoreDir = path + mod.treeStoreDir = path } } } @@ -53,11 +53,11 @@ func WithTreeStoreDirectory(path string) modules.TreeStoreOption { func (t *TreeStore) GetModuleName() string { return modules.TreeStoreSubmoduleName } func (t *TreeStore) setupTrees() error { - if t.TreeStoreDir == ":memory:" { + if t.treeStoreDir == ":memory:" { return t.setupInMemory() } - nodeStore, err := kvstore.NewKVStore(fmt.Sprintf("%s/%s_nodes", t.TreeStoreDir, RootTreeName)) + nodeStore, err := kvstore.NewKVStore(fmt.Sprintf("%s/%s_nodes", t.treeStoreDir, RootTreeName)) if err != nil { return err } @@ -69,7 +69,7 @@ func (t *TreeStore) setupTrees() error { t.merkleTrees = make(map[string]*stateTree, len(stateTreeNames)) for i := 0; i < len(stateTreeNames); i++ { - nodeStore, err := kvstore.NewKVStore(fmt.Sprintf("%s/%s_nodes", t.TreeStoreDir, stateTreeNames[i])) + nodeStore, err := kvstore.NewKVStore(fmt.Sprintf("%s/%s_nodes", t.treeStoreDir, stateTreeNames[i])) if err != nil { return err } From ad0cdcbe50861e7090b7db3c91d9308ad1291c18 Mon Sep 17 00:00:00 2001 From: d7t Date: Wed, 19 Jul 2023 17:18:54 -0600 Subject: [PATCH 14/20] [docs] Apply documentation suggestions from code review Co-authored-by: Daniel Olshansky --- persistence/trees/trees.go | 2 +- shared/modules/doc/README.md | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/persistence/trees/trees.go b/persistence/trees/trees.go index 0a1e6ea43..7b134536c 100644 --- a/persistence/trees/trees.go +++ b/persistence/trees/trees.go @@ -134,7 +134,7 @@ func (t *TreeStore) DebugClearAll() error { } // updateMerkleTrees updates all of the merkle trees in order defined by `numMerkleTrees` -// * it returns the new state hash capturing the state of all the trees or an error if one occurred. +// * It returns the new state hash capturing the state of all the trees or an error if one occurred. // * This function does not commit state to disk. The caller must manually invoke `Commit` to persist // changes to disk. func (t *TreeStore) updateMerkleTrees(pgtx pgx.Tx, txi indexer.TxIndexer, height uint64) (string, error) { diff --git a/shared/modules/doc/README.md b/shared/modules/doc/README.md index 6c187aae8..b5d4d2bec 100644 --- a/shared/modules/doc/README.md +++ b/shared/modules/doc/README.md @@ -434,9 +434,13 @@ This is quite **important** because it unlocks a powerful concept **Dependency I This enables the developer to define different implementations of a module and to register the one that is needed at runtime. This is because we can only have one module registered with a unique name and also because, by convention, we keep module names defined as constants. This is useful not only for prototyping but also for different use cases such as the `p1` CLI and the `pocket` binary where different implementations of the same module are necessary due to the fact that the `p1` CLI doesn't have a persistence module but still needs to know what's going on in the network. -Submodules can be registered the same way full Modules can be, by passing the Submodule to the RegisterModule function. Submodules should typically be registered to the bus for dependency injection reasons. Modules and submodules are all responsible for registering themselves with the Bus. This pattern emerged organically during development and is now considered best practice. Additionally, modules should not maintain pointer references to modules and should instead call the Bus to get a new reference to a module whenever they need to call that module. +**Registration**: Submodules can be registered the same way full Modules by passing the Submodule to the `RegisterModule` function. Submodules should typically be registered to the bus for dependency injection reasons. -Submodule interfaces are typically defined in the `shared/modules` package with the rest of the module interfaces in a file named `XXX_submodule.go`, where XXX denotes the name of the submodule. That same file should contain the factory function definition for a submodule which should be embedded by the `Submodule` interface type. Factory function definitions should typically not be exported. +**IMPORTANT**: Modules and Submodules are all responsible for registering themselves with the Bus. This pattern emerged organically during development and is now considered best practice. + +**Module Access**: Full Modules should not maintain pointer references to other full Modules. Instead, they should call the Bus to get a new module reference whenever needed. + +Submodule interfaces are typically defined in the `shared/modules` package with the rest of the module interfaces in a file named `XXX_submodule.go`, where XXX denotes the name of the Submodule. That same file SHOULD contain the factory function definition for a Submodule where the `Submodule` interface type MUST be embedded. Factory function definitions SHOULD NOT be exported. For example, in the TreeStore code below, the `treeStoreFactory` is defined and then embedded in the `TreeStoreModule`, which also embeds the Submodule interface. Typically these factory functions should be kept private at the package level. From 4a9d866d7ffb073a16c777dec77b9c914a168109 Mon Sep 17 00:00:00 2001 From: Daniel Olshansky Date: Wed, 19 Jul 2023 16:35:17 -0700 Subject: [PATCH 15/20] [Temp] s/TreeStore/treeStore (#920) Small commit to reduce visibility of a type for in progress PR --- persistence/trees/module.go | 18 +++--- persistence/trees/module_test.go | 107 +++++++++++++++++++++++-------- persistence/trees/trees.go | 36 +++++------ 3 files changed, 107 insertions(+), 54 deletions(-) diff --git a/persistence/trees/module.go b/persistence/trees/module.go index 14aaa9eff..090546d0a 100644 --- a/persistence/trees/module.go +++ b/persistence/trees/module.go @@ -8,10 +8,10 @@ import ( "github.com/pokt-network/smt" ) -var _ modules.TreeStoreModule = &TreeStore{} +var _ modules.TreeStoreModule = &treeStore{} -func (*TreeStore) Create(bus modules.Bus, options ...modules.TreeStoreOption) (modules.TreeStoreModule, error) { - m := &TreeStore{} +func (*treeStore) Create(bus modules.Bus, options ...modules.TreeStoreOption) (modules.TreeStoreModule, error) { + m := &treeStore{} bus.RegisterModule(m) @@ -27,13 +27,13 @@ func (*TreeStore) Create(bus modules.Bus, options ...modules.TreeStoreOption) (m } func Create(bus modules.Bus, options ...modules.TreeStoreOption) (modules.TreeStoreModule, error) { - return new(TreeStore).Create(bus, options...) + return new(treeStore).Create(bus, options...) } // WithLogger assigns a logger for the tree store func WithLogger(logger *modules.Logger) modules.TreeStoreOption { return func(m modules.TreeStoreModule) { - if mod, ok := m.(*TreeStore); ok { + if mod, ok := m.(*treeStore); ok { mod.logger = logger } } @@ -43,16 +43,16 @@ func WithLogger(logger *modules.Logger) modules.TreeStoreOption { // saves its data. func WithTreeStoreDirectory(path string) modules.TreeStoreOption { return func(m modules.TreeStoreModule) { - mod, ok := m.(*TreeStore) + mod, ok := m.(*treeStore) if ok { mod.treeStoreDir = path } } } -func (t *TreeStore) GetModuleName() string { return modules.TreeStoreSubmoduleName } +func (t *treeStore) GetModuleName() string { return modules.TreeStoreSubmoduleName } -func (t *TreeStore) setupTrees() error { +func (t *treeStore) setupTrees() error { if t.treeStoreDir == ":memory:" { return t.setupInMemory() } @@ -83,7 +83,7 @@ func (t *TreeStore) setupTrees() error { return nil } -func (t *TreeStore) setupInMemory() error { +func (t *treeStore) setupInMemory() error { nodeStore := kvstore.NewMemKVStore() t.rootTree = &stateTree{ name: RootTreeName, diff --git a/persistence/trees/module_test.go b/persistence/trees/module_test.go index f5bc8d491..452305838 100644 --- a/persistence/trees/module_test.go +++ b/persistence/trees/module_test.go @@ -6,6 +6,7 @@ import ( "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/pokt-network/pocket/internal/testutil" "github.com/pokt-network/pocket/p2p/providers/peerstore_provider" @@ -35,12 +36,21 @@ func TestTreeStore_Create(t *testing.T) { persistenceMock.EXPECT().NewRWContext(int64(0)).AnyTimes() persistenceMock.EXPECT().GetTxIndexer().AnyTimes() - treemod, err := trees.Create(mockBus, - trees.WithTreeStoreDirectory(":memory:")) + treemod, err := trees.Create(mockBus, trees.WithTreeStoreDirectory(":memory:")) assert.NoError(t, err) - treemod.GetModuleName() + got := treemod.GetBus() assert.Equal(t, got, mockBus) + + // root hash should be empty for empty tree + root, ns := treemod.GetTree(trees.TransactionsTreeName) + require.Equal(t, root, []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}) + + // nodestore should have no values in it + keys, vals, err := ns.GetAll(nil, false) + require.NoError(t, err) + require.Empty(t, keys, vals) + } func TestTreeStore_DebugClearAll(t *testing.T) { @@ -78,22 +88,45 @@ func preparePersistenceMock(t *testing.T, busMock *mockModules.MockBus, genesisS persistenceModuleMock := mockModules.NewMockPersistenceModule(ctrl) readCtxMock := mockModules.NewMockPersistenceReadContext(ctrl) - readCtxMock.EXPECT().GetAllValidators(gomock.Any()).Return(genesisState.GetValidators(), nil).AnyTimes() - readCtxMock.EXPECT().GetAllStakedActors(gomock.Any()).DoAndReturn(func(height int64) ([]*coreTypes.Actor, error) { - return testutil.Concatenate[*coreTypes.Actor]( - genesisState.GetValidators(), - genesisState.GetServicers(), - genesisState.GetFishermen(), - genesisState.GetApplications(), - ), nil - }).AnyTimes() - persistenceModuleMock.EXPECT().NewReadContext(gomock.Any()).Return(readCtxMock, nil).AnyTimes() - readCtxMock.EXPECT().Release().AnyTimes() - - persistenceModuleMock.EXPECT().GetBus().Return(busMock).AnyTimes() - persistenceModuleMock.EXPECT().SetBus(busMock).AnyTimes() - persistenceModuleMock.EXPECT().GetModuleName().Return(modules.PersistenceModuleName).AnyTimes() - busMock.RegisterModule(persistenceModuleMock) + readCtxMock.EXPECT(). + GetAllValidators(gomock.Any()). + Return(genesisState.GetValidators(), nil).AnyTimes() + + readCtxMock.EXPECT(). + GetAllStakedActors(gomock.Any()). + DoAndReturn(func(height int64) ([]*coreTypes.Actor, error) { + return testutil.Concatenate[*coreTypes.Actor]( + genesisState.GetValidators(), + genesisState.GetServicers(), + genesisState.GetFishermen(), + genesisState.GetApplications(), + ), nil + }). + AnyTimes() + + persistenceModuleMock.EXPECT(). + NewReadContext(gomock.Any()). + Return(readCtxMock, nil). + AnyTimes() + + readCtxMock.EXPECT(). + Release(). + AnyTimes() + + persistenceModuleMock.EXPECT(). + GetBus(). + Return(busMock). + AnyTimes() + persistenceModuleMock.EXPECT(). + SetBus(busMock). + AnyTimes() + persistenceModuleMock.EXPECT(). + GetModuleName(). + Return(modules.PersistenceModuleName). + AnyTimes() + + busMock. + RegisterModule(persistenceModuleMock) return persistenceModuleMock } @@ -106,15 +139,35 @@ func validatorId(i int) string { func createMockBus(t *testing.T, runtimeMgr modules.RuntimeMgr) *mockModules.MockBus { t.Helper() ctrl := gomock.NewController(t) + mockBus := mockModules.NewMockBus(ctrl) - mockBus.EXPECT().GetRuntimeMgr().Return(runtimeMgr).AnyTimes() - mockBus.EXPECT().RegisterModule(gomock.Any()).DoAndReturn(func(m modules.Submodule) { - m.SetBus(mockBus) - }).AnyTimes() + mockBus.EXPECT(). + GetRuntimeMgr(). + Return(runtimeMgr). + AnyTimes() + + mockBus.EXPECT(). + RegisterModule(gomock.Any()). + DoAndReturn(func(m modules.Submodule) { + m.SetBus(mockBus) + }). + AnyTimes() + mockModulesRegistry := mockModules.NewMockModulesRegistry(ctrl) - mockModulesRegistry.EXPECT().GetModule(peerstore_provider.PeerstoreProviderSubmoduleName).Return(nil, runtime.ErrModuleNotRegistered(peerstore_provider.PeerstoreProviderSubmoduleName)).AnyTimes() - mockModulesRegistry.EXPECT().GetModule(modules.CurrentHeightProviderSubmoduleName).Return(nil, runtime.ErrModuleNotRegistered(modules.CurrentHeightProviderSubmoduleName)).AnyTimes() - mockBus.EXPECT().GetModulesRegistry().Return(mockModulesRegistry).AnyTimes() - mockBus.EXPECT().PublishEventToBus(gomock.Any()).AnyTimes() + mockModulesRegistry.EXPECT(). + GetModule(peerstore_provider.PeerstoreProviderSubmoduleName). + Return(nil, runtime.ErrModuleNotRegistered(peerstore_provider.PeerstoreProviderSubmoduleName)). + AnyTimes() + mockModulesRegistry.EXPECT(). + GetModule(modules.CurrentHeightProviderSubmoduleName). + Return(nil, runtime.ErrModuleNotRegistered(modules.CurrentHeightProviderSubmoduleName)). + AnyTimes() + mockBus.EXPECT(). + GetModulesRegistry(). + Return(mockModulesRegistry). + AnyTimes() + mockBus.EXPECT(). + PublishEventToBus(gomock.Any()). + AnyTimes() return mockBus } diff --git a/persistence/trees/trees.go b/persistence/trees/trees.go index 7b134536c..c952f4d9d 100644 --- a/persistence/trees/trees.go +++ b/persistence/trees/trees.go @@ -72,15 +72,15 @@ type stateTree struct { nodeStore kvstore.KVStore } -var _ modules.TreeStoreModule = &TreeStore{} +var _ modules.TreeStoreModule = &treeStore{} -// TreeStore stores a set of merkle trees that it manages. -// It fulfills the modules.TreeStore interface +// treeStore stores a set of merkle trees that it manages. +// It fulfills the modules.treeStore interface // * It is responsible for atomic commit or rollback behavior of the underlying // trees by utilizing the lazy loading functionality of the smt library. -// TECHDEBT(#880): TreeStore is exported for testing purposes to avoid import cycle errors. +// TECHDEBT(#880): treeStore is exported for testing purposes to avoid import cycle errors. // Make it private and export a custom struct with a test build tag when necessary. -type TreeStore struct { +type treeStore struct { base_modules.IntegrableModule logger *modules.Logger @@ -93,7 +93,7 @@ type TreeStore struct { // GetTree returns the root hash and nodeStore for the matching tree stored in the TreeStore. // This enables the caller to import the SMT without changing the one stored unless they call // `Commit()` to write to the nodestore. -func (t *TreeStore) GetTree(name string) ([]byte, kvstore.KVStore) { +func (t *treeStore) GetTree(name string) ([]byte, kvstore.KVStore) { if name == RootTreeName { return t.rootTree.tree.Root(), t.rootTree.nodeStore } @@ -105,7 +105,7 @@ func (t *TreeStore) GetTree(name string) ([]byte, kvstore.KVStore) { // Update takes a pgx transaction and a height and updates all of the trees in the TreeStore for that height. // It is atomic and handles its own savepoint and rollback creation. -func (t *TreeStore) Update(pgtx pgx.Tx, height uint64) (string, error) { +func (t *treeStore) Update(pgtx pgx.Tx, height uint64) (string, error) { t.logger.Info().Msgf("🌴 updating state trees at height %d", height) txi := t.GetBus().GetPersistenceModule().GetTxIndexer() stateHash, err := t.updateMerkleTrees(pgtx, txi, height) @@ -118,7 +118,7 @@ func (t *TreeStore) Update(pgtx pgx.Tx, height uint64) (string, error) { // DebugClearAll is used by the debug cli to completely reset all merkle trees. // This should only be called by the debug CLI. // TECHDEBT: Move this into a separate file with a debug build flag to avoid accidental usage in prod -func (t *TreeStore) DebugClearAll() error { +func (t *treeStore) DebugClearAll() error { if err := t.rootTree.nodeStore.ClearAll(); err != nil { return fmt.Errorf("failed to clear root node store: %w", err) } @@ -137,7 +137,7 @@ func (t *TreeStore) DebugClearAll() error { // * It returns the new state hash capturing the state of all the trees or an error if one occurred. // * This function does not commit state to disk. The caller must manually invoke `Commit` to persist // changes to disk. -func (t *TreeStore) updateMerkleTrees(pgtx pgx.Tx, txi indexer.TxIndexer, height uint64) (string, error) { +func (t *treeStore) updateMerkleTrees(pgtx pgx.Tx, txi indexer.TxIndexer, height uint64) (string, error) { for treeName := range t.merkleTrees { switch treeName { // Actor Merkle Trees @@ -219,7 +219,7 @@ func (t *TreeStore) updateMerkleTrees(pgtx pgx.Tx, txi indexer.TxIndexer, height return t.getStateHash(), nil } -func (t *TreeStore) Commit() error { +func (t *treeStore) Commit() error { if err := t.rootTree.tree.Commit(); err != nil { return fmt.Errorf("failed to commit root tree: %w", err) } @@ -232,7 +232,7 @@ func (t *TreeStore) Commit() error { return nil } -func (t *TreeStore) getStateHash() string { +func (t *treeStore) getStateHash() string { for _, stateTree := range t.merkleTrees { key := []byte(stateTree.name) val := stateTree.tree.Root() @@ -252,7 +252,7 @@ func (t *TreeStore) getStateHash() string { //////////////////////// // NB: I think this needs to be done manually for all 4 types. -func (t *TreeStore) updateActorsTree(actorType coreTypes.ActorType, actors []*coreTypes.Actor) error { +func (t *treeStore) updateActorsTree(actorType coreTypes.ActorType, actors []*coreTypes.Actor) error { for _, actor := range actors { bzAddr, err := hex.DecodeString(actor.GetAddress()) if err != nil { @@ -280,7 +280,7 @@ func (t *TreeStore) updateActorsTree(actorType coreTypes.ActorType, actors []*co // Account Tree Helpers // ////////////////////////// -func (t *TreeStore) updateAccountTrees(accounts []*coreTypes.Account) error { +func (t *treeStore) updateAccountTrees(accounts []*coreTypes.Account) error { for _, account := range accounts { bzAddr, err := hex.DecodeString(account.GetAddress()) if err != nil { @@ -300,7 +300,7 @@ func (t *TreeStore) updateAccountTrees(accounts []*coreTypes.Account) error { return nil } -func (t *TreeStore) updatePoolTrees(pools []*coreTypes.Account) error { +func (t *treeStore) updatePoolTrees(pools []*coreTypes.Account) error { for _, pool := range pools { bzAddr, err := hex.DecodeString(pool.GetAddress()) if err != nil { @@ -324,7 +324,7 @@ func (t *TreeStore) updatePoolTrees(pools []*coreTypes.Account) error { // Data Tree Helpers // /////////////////////// -func (t *TreeStore) updateTransactionsTree(indexedTxs []*coreTypes.IndexedTransaction) error { +func (t *treeStore) updateTransactionsTree(indexedTxs []*coreTypes.IndexedTransaction) error { for _, idxTx := range indexedTxs { txBz := idxTx.GetTx() txHash := crypto.SHA3Hash(txBz) @@ -335,7 +335,7 @@ func (t *TreeStore) updateTransactionsTree(indexedTxs []*coreTypes.IndexedTransa return nil } -func (t *TreeStore) updateParamsTree(params []*coreTypes.Param) error { +func (t *treeStore) updateParamsTree(params []*coreTypes.Param) error { for _, param := range params { paramBz, err := codec.GetCodec().Marshal(param) paramKey := crypto.SHA3Hash([]byte(param.Name)) @@ -350,7 +350,7 @@ func (t *TreeStore) updateParamsTree(params []*coreTypes.Param) error { return nil } -func (t *TreeStore) updateFlagsTree(flags []*coreTypes.Flag) error { +func (t *treeStore) updateFlagsTree(flags []*coreTypes.Flag) error { for _, flag := range flags { flagBz, err := codec.GetCodec().Marshal(flag) flagKey := crypto.SHA3Hash([]byte(flag.Name)) @@ -365,7 +365,7 @@ func (t *TreeStore) updateFlagsTree(flags []*coreTypes.Flag) error { return nil } -func (t *TreeStore) updateIBCTree(keys, values [][]byte) error { +func (t *treeStore) updateIBCTree(keys, values [][]byte) error { if len(keys) != len(values) { return fmt.Errorf("keys and values must be the same length") } From 61e2f610eb95fcf877df6691947d24799d25cc8d Mon Sep 17 00:00:00 2001 From: shakezula Date: Wed, 19 Jul 2023 18:20:03 -0600 Subject: [PATCH 16/20] [fixup] adds error logs to treeStore#Commit error paths --- persistence/trees/trees.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/persistence/trees/trees.go b/persistence/trees/trees.go index c952f4d9d..c8ef2f645 100644 --- a/persistence/trees/trees.go +++ b/persistence/trees/trees.go @@ -219,16 +219,20 @@ func (t *treeStore) updateMerkleTrees(pgtx pgx.Tx, txi indexer.TxIndexer, height return t.getStateHash(), nil } +// Commit commits changes in the sub-trees to the root tree and then commits updates for each sub-tree. func (t *treeStore) Commit() error { if err := t.rootTree.tree.Commit(); err != nil { + t.logger.Err(err).Msg("failed to commit root tree: changes to sub-trees will not be committed - this should be investigated") return fmt.Errorf("failed to commit root tree: %w", err) } for name, treeStore := range t.merkleTrees { if err := treeStore.tree.Commit(); err != nil { + t.logger.Err(err).Msgf("failed to commit to %s tree: changes will not be saved - this should be investigated", name) return fmt.Errorf("failed to commit %s: %w", name, err) } } + return nil } From 6aaff894c8297876479d3e54e302cb0f9af292af Mon Sep 17 00:00:00 2001 From: shakezula Date: Wed, 19 Jul 2023 19:31:47 -0600 Subject: [PATCH 17/20] [fixup] formatting long mock definitions --- persistence/trees/module_test.go | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/persistence/trees/module_test.go b/persistence/trees/module_test.go index 452305838..4c21776fd 100644 --- a/persistence/trees/module_test.go +++ b/persistence/trees/module_test.go @@ -31,10 +31,20 @@ func TestTreeStore_Create(t *testing.T) { genesisStateMock := createMockGenesisState(nil) persistenceMock := preparePersistenceMock(t, mockBus, genesisStateMock) - mockBus.EXPECT().GetPersistenceModule().Return(persistenceMock).AnyTimes() - persistenceMock.EXPECT().GetBus().AnyTimes().Return(mockBus) - persistenceMock.EXPECT().NewRWContext(int64(0)).AnyTimes() - persistenceMock.EXPECT().GetTxIndexer().AnyTimes() + mockBus.EXPECT(). + GetPersistenceModule(). + Return(persistenceMock). + AnyTimes() + persistenceMock.EXPECT(). + GetBus(). + AnyTimes(). + Return(mockBus) + persistenceMock.EXPECT(). + NewRWContext(int64(0)). + AnyTimes() + persistenceMock.EXPECT(). + GetTxIndexer(). + AnyTimes() treemod, err := trees.Create(mockBus, trees.WithTreeStoreDirectory(":memory:")) assert.NoError(t, err) @@ -50,7 +60,6 @@ func TestTreeStore_Create(t *testing.T) { keys, vals, err := ns.GetAll(nil, false) require.NoError(t, err) require.Empty(t, keys, vals) - } func TestTreeStore_DebugClearAll(t *testing.T) { @@ -91,7 +100,6 @@ func preparePersistenceMock(t *testing.T, busMock *mockModules.MockBus, genesisS readCtxMock.EXPECT(). GetAllValidators(gomock.Any()). Return(genesisState.GetValidators(), nil).AnyTimes() - readCtxMock.EXPECT(). GetAllStakedActors(gomock.Any()). DoAndReturn(func(height int64) ([]*coreTypes.Actor, error) { @@ -103,16 +111,13 @@ func preparePersistenceMock(t *testing.T, busMock *mockModules.MockBus, genesisS ), nil }). AnyTimes() - persistenceModuleMock.EXPECT(). NewReadContext(gomock.Any()). Return(readCtxMock, nil). AnyTimes() - readCtxMock.EXPECT(). Release(). AnyTimes() - persistenceModuleMock.EXPECT(). GetBus(). Return(busMock). @@ -124,7 +129,6 @@ func preparePersistenceMock(t *testing.T, busMock *mockModules.MockBus, genesisS GetModuleName(). Return(modules.PersistenceModuleName). AnyTimes() - busMock. RegisterModule(persistenceModuleMock) @@ -139,21 +143,19 @@ func validatorId(i int) string { func createMockBus(t *testing.T, runtimeMgr modules.RuntimeMgr) *mockModules.MockBus { t.Helper() ctrl := gomock.NewController(t) - mockBus := mockModules.NewMockBus(ctrl) + mockModulesRegistry := mockModules.NewMockModulesRegistry(ctrl) + mockBus.EXPECT(). GetRuntimeMgr(). Return(runtimeMgr). AnyTimes() - mockBus.EXPECT(). RegisterModule(gomock.Any()). DoAndReturn(func(m modules.Submodule) { m.SetBus(mockBus) }). AnyTimes() - - mockModulesRegistry := mockModules.NewMockModulesRegistry(ctrl) mockModulesRegistry.EXPECT(). GetModule(peerstore_provider.PeerstoreProviderSubmoduleName). Return(nil, runtime.ErrModuleNotRegistered(peerstore_provider.PeerstoreProviderSubmoduleName)). @@ -169,5 +171,6 @@ func createMockBus(t *testing.T, runtimeMgr modules.RuntimeMgr) *mockModules.Moc mockBus.EXPECT(). PublishEventToBus(gomock.Any()). AnyTimes() + return mockBus } From c9dfe009f6626e6a88ca72fe28d077d65350958c Mon Sep 17 00:00:00 2001 From: shakezula Date: Thu, 20 Jul 2023 10:57:32 -0600 Subject: [PATCH 18/20] [fixup] change assertion on empty byte slice --- persistence/trees/module_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/persistence/trees/module_test.go b/persistence/trees/module_test.go index 4c21776fd..7c7bc660c 100644 --- a/persistence/trees/module_test.go +++ b/persistence/trees/module_test.go @@ -54,7 +54,7 @@ func TestTreeStore_Create(t *testing.T) { // root hash should be empty for empty tree root, ns := treemod.GetTree(trees.TransactionsTreeName) - require.Equal(t, root, []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}) + require.Equal(t, root, make([]byte, 32)) // nodestore should have no values in it keys, vals, err := ns.GetAll(nil, false) From 33a42dabadd953d521611f078c2255f289006a56 Mon Sep 17 00:00:00 2001 From: shakezula Date: Thu, 20 Jul 2023 15:55:43 -0600 Subject: [PATCH 19/20] [docs] adds documentation on order of module registration --- shared/modules/doc/README.md | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/shared/modules/doc/README.md b/shared/modules/doc/README.md index b5d4d2bec..201856453 100644 --- a/shared/modules/doc/README.md +++ b/shared/modules/doc/README.md @@ -434,7 +434,20 @@ This is quite **important** because it unlocks a powerful concept **Dependency I This enables the developer to define different implementations of a module and to register the one that is needed at runtime. This is because we can only have one module registered with a unique name and also because, by convention, we keep module names defined as constants. This is useful not only for prototyping but also for different use cases such as the `p1` CLI and the `pocket` binary where different implementations of the same module are necessary due to the fact that the `p1` CLI doesn't have a persistence module but still needs to know what's going on in the network. -**Registration**: Submodules can be registered the same way full Modules by passing the Submodule to the `RegisterModule` function. Submodules should typically be registered to the bus for dependency injection reasons. +**Registration**: Submodules can be registered the same way full Modules by passing the Submodule to the `RegisterModule` function. Submodules should typically be registered to the bus for dependency injection reasons. Registration should occur _after_ processing the module's options. + +```go +func (*treeStore) Create(bus modules.Bus, options ...modules.TreeStoreOption) (modules.TreeStoreModule, error) { + m := &treeStore{} + + for _, option := range options { + option(m) + } + + bus.RegisterModule(m) + // ... +} +``` **IMPORTANT**: Modules and Submodules are all responsible for registering themselves with the Bus. This pattern emerged organically during development and is now considered best practice. From 9b0ab2c9dec88be4966d7847b99884b03f310265 Mon Sep 17 00:00:00 2001 From: shakezula Date: Thu, 20 Jul 2023 15:56:06 -0600 Subject: [PATCH 20/20] [fixup] register treestore submodule after processing options --- persistence/trees/module.go | 4 ++-- persistence/trees/trees.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/persistence/trees/module.go b/persistence/trees/module.go index 090546d0a..7da8bf20f 100644 --- a/persistence/trees/module.go +++ b/persistence/trees/module.go @@ -13,12 +13,12 @@ var _ modules.TreeStoreModule = &treeStore{} func (*treeStore) Create(bus modules.Bus, options ...modules.TreeStoreOption) (modules.TreeStoreModule, error) { m := &treeStore{} - bus.RegisterModule(m) - for _, option := range options { option(m) } + bus.RegisterModule(m) + if err := m.setupTrees(); err != nil { return nil, err } diff --git a/persistence/trees/trees.go b/persistence/trees/trees.go index c8ef2f645..39c5c1ece 100644 --- a/persistence/trees/trees.go +++ b/persistence/trees/trees.go @@ -222,13 +222,13 @@ func (t *treeStore) updateMerkleTrees(pgtx pgx.Tx, txi indexer.TxIndexer, height // Commit commits changes in the sub-trees to the root tree and then commits updates for each sub-tree. func (t *treeStore) Commit() error { if err := t.rootTree.tree.Commit(); err != nil { - t.logger.Err(err).Msg("failed to commit root tree: changes to sub-trees will not be committed - this should be investigated") + t.logger.Err(err).Msg("TECHDEBT: failed to commit root tree: changes to sub-trees will not be committed - this should be investigated") return fmt.Errorf("failed to commit root tree: %w", err) } for name, treeStore := range t.merkleTrees { if err := treeStore.tree.Commit(); err != nil { - t.logger.Err(err).Msgf("failed to commit to %s tree: changes will not be saved - this should be investigated", name) + t.logger.Err(err).Msgf("TECHDEBT: failed to commit to %s tree: changes will not be saved - this should be investigated", name) return fmt.Errorf("failed to commit %s: %w", name, err) } }