Cleanup storage #185

ethanfrey · 2017-07-26T19:23:41Z

Lots of improvements:

Middleware to control what parts are committed and what rolledback upon error in DeliverTx (by default, nonce update and fee deduction happens).
Expose more power of IAVLTree, remove, lists and first/last in range
Improve caching mechanism to be adapted to the storage layer

odeke-em · 2017-07-26T23:29:30Z

app/store.go

+	// Expand the path fully
+	dbPath, err := filepath.Abs(dbName)
+	if err != nil {
+		panic(fmt.Sprintf("Invalid Database Name: %s", dbName))


So for a package function, wouldn't it be better if instead we returned an error instead of surprisingly panicking in the code so

func NewStore(dbName string, cacheSize int, logger log.Logger) (*Store, error)

otherwise the function itself has no mention that it'll panic if we can't expand the path. IMHO if we want to panic, maybe make another function say

func MustMakeNewStore(dbName string, cacheSize int, logger log.Logger) *Store

and then from there one can panic.

Totally agree...
This code was adapted from another package. Basically all the parsing code copied.
There are way too many panics all over our codebase... and after fighting against them for a while I get lazy. Thanks for pushing me to do this properly.

Gotcha, I've encountered this quite often too when adapting code from different packages. For sure, you've extra hands here and we'll fight these panics 🙏 #panicBusters

odeke-em · 2017-07-26T23:29:35Z

app/store.go

+}
+
+// CloseDB closes the database
+// func (s *Store) CloseDB() {


Did you mean to leave this commented out?

It was never really called anywhere, so I guess it can be removed. I'll delete it fully.

odeke-em · 2017-07-26T23:32:00Z

state/chainstate.go

+// SetChainID stores the chain id in the store
+func (s *ChainState) SetChainID(store KVStore, chainID string) {
+	s.chainID = chainID
+	store.Set([]byte("base/chain_id"), []byte(chainID))


Let's lift this out of here and instead make it a variable

var baseChainIDKey = []byte("base/chain_id") ... store.Set(baseChainIDKey, []byte(chainID))

Yes, good point

odeke-em · 2017-07-26T23:37:21Z

state/kvcache.go

+	e *list.Element // The KVCache.keys element
+}
+
+// NOTE: If store is nil, creates a new MemKVStore


// NewKVCache creates a new KVCache from store.
// If store is nil, it creates a new MemKVStore.

odeke-em · 2017-07-26T23:39:02Z

state/kvstore.go

 type MemKVStore struct {
 	m map[string][]byte
 }

+var _ SimpleDB = NewMemKVStore()


var _ SimpleDB = (*MemKVStore)(nil)

odeke-em · 2017-07-26T23:41:19Z

state/kvstore.go

-	keys     *list.List
-	logging  bool
-	logLines []string
+func (mkv *MemKVStore) Has(key []byte) (has bool) {


Just a random question: whenever I see a key-value store immediately I imagine concurrent usage, would that apply for this one?

You are correct, but since the whole blockchain is about serializing transactions, this doesn't apply. You can have two caches over one store (check and deliver), but in the end one will be discarded and there are never concurrent writes while you are reading.

Aye aye, thanks for letting me know, TIL.

odeke-em · 2017-07-26T23:46:40Z

state/merkle.go

+	merkle.Tree
+}
+
+var _ SimpleDB = &Bonsai{}


var _ SimpleDB = (*Bonsai)(nil)

odeke-em · 2017-07-26T23:47:14Z

state/merkle.go

+	stopAtCount := func(key []byte, value []byte) (stop bool) {
+		m := Model{key, value}
+		res = append(res, m)
+		// return false


Let's remove this, please.

odeke-em · 2017-07-26T23:47:50Z

state/merkle.go

+func (b *Bonsai) Commit(sub SimpleDB) error {
+	bb, ok := sub.(*Bonsai)
+	if !ok || (b.id != bb.id) {
+		return errors.New("Not a sub-transaction")


can be lifted out and made a var so

return errNotASubTransaction

odeke-em · 2017-07-27T15:25:51Z

app/store.go

+		err = wire.ReadBinaryBytes(eyesStateBytes, &chainState)
+		if err != nil {
+			logger.Error("error reading MerkleEyesState", "err", err)
+			panic(err)


We forgot to remove this panic here :)

odeke-em · 2017-07-27T15:30:09Z

Thank you for the PR and for addressing my comments @ethanfrey! Just one nit left otherwise LGTM from a code standpoint.

odeke-em

LGTM after removing the last panic mentioned previously.

… safe...

fix: prevent panic in Coins.IsEqual

Export now only exports to file

ethanfrey force-pushed the feature/cleanup-state branch from 9088c5a to cf472e6 Compare July 26, 2017 20:13

odeke-em reviewed Jul 26, 2017

View reviewed changes

ethanfrey changed the title ~~WIP: Cleanup storage~~ Cleanup storage Jul 27, 2017

odeke-em reviewed Jul 27, 2017

View reviewed changes

odeke-em approved these changes Jul 27, 2017

View reviewed changes

ethanfrey added 18 commits July 27, 2017 15:31

Add checkpoint to middleware not app

5272ca5

Pull in logic from merkleeyes, get it all working with trees

f6e7d4b

Fixed benchmark and compared unstable to this branch

243d767

Add SimpleDB interface for merkle tree

caff0ad

Started implementing SimpleDB for MemKVStore

28ebfd6

Add basic kv tests

37b5d16

Test list queries and iavl implementation

744d035

Make kvtests legible

d0d1a96

Reorg state package and fix all imports

d607b76

Revert all on failed CheckTx, fee+nonce increment on failed DeliverTx

199ee81

Cache test for Bonsai

84e2fa6

SimpleDB works except for persisted

f1785e3

Ported everything to SimpleDB interface

51a29e4

Add tests for checkpointer

6b96fa4

Fix app state to commit properly

ea8bdd3

Code cleanup from emmanuel

2b79aa0

Last cleanup

27e7fbe

Use KVMemCache for checkpointing IAVL tree as they are not go-routine…

dfdbfa0

… safe...

ethanfrey force-pushed the feature/cleanup-state branch from 6d8faec to dfdbfa0 Compare July 27, 2017 19:32

ethanfrey merged commit 761fb0d into unstable Jul 27, 2017

ethanfrey deleted the feature/cleanup-state branch July 27, 2017 19:38

mattverse referenced this pull request in mattverse/cosmos-sdk Apr 20, 2022

Merge pull request #185 from agoric-labs/11223-coins-equal

8e6b3cf

fix: prevent panic in Coins.IsEqual

alexanderbez pushed a commit that referenced this pull request May 26, 2022

chore(deps): bump actions/cache from 3.0.1 to 3.0.2 (#185)

71bb5bd

sbellem pushed a commit to initc3/cosmos-sdk that referenced this pull request Jan 27, 2023

Merge pull request cosmos#185 from scrtlabs/lior-export-to-file

45cac6c

Export now only exports to file

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup storage #185

Cleanup storage #185

ethanfrey commented Jul 26, 2017

odeke-em Jul 26, 2017

ethanfrey Jul 27, 2017

odeke-em Jul 27, 2017

odeke-em Jul 26, 2017

ethanfrey Jul 27, 2017

odeke-em Jul 26, 2017

ethanfrey Jul 27, 2017

odeke-em Jul 26, 2017

odeke-em Jul 26, 2017

odeke-em Jul 26, 2017

ethanfrey Jul 27, 2017

odeke-em Jul 27, 2017

odeke-em Jul 26, 2017

odeke-em Jul 26, 2017

odeke-em Jul 26, 2017

odeke-em Jul 27, 2017

odeke-em commented Jul 27, 2017

odeke-em left a comment

Cleanup storage #185

Cleanup storage #185

Conversation

ethanfrey commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odeke-em commented Jul 27, 2017

odeke-em left a comment

Choose a reason for hiding this comment