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.

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

[WIP] IAVL store wrapper #916

Closed
wants to merge 2 commits into from
Closed

[WIP] IAVL store wrapper #916

wants to merge 2 commits into from

Conversation

kfangw
Copy link
Contributor

@kfangw kfangw commented Jan 21, 2019

Github Issue

Background

Solution

Possible Drawbacks

@codecov-io
Copy link

codecov-io commented Jan 22, 2019

Codecov Report

Merging #916 into master will decrease coverage by 0.59%.
The diff coverage is 42.97%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #916     +/-   ##
=========================================
- Coverage   60.47%   59.88%   -0.6%     
=========================================
  Files         159      162      +3     
  Lines       11203    11452    +249     
=========================================
+ Hits         6775     6858     +83     
- Misses       3629     3792    +163     
- Partials      799      802      +3
Flag Coverage Δ
#integration_tests_long_term 43.9% <ø> (ø) ⬆️
#integration_tests_node 39.89% <ø> (-0.68%) ⬇️
#integration_tests_retry 37.21% <ø> (+0.05%) ⬆️
#unittests 47.57% <42.97%> (-0.11%) ⬇️
Impacted Files Coverage Δ
lib/statedb/store/iavlstore.go 37.5% <37.5%> (ø)
lib/statedb/stateobject.go 39.6% <39.6%> (ø)
lib/statedb/statedb.go 47.41% <47.41%> (ø)
lib/network/http2_client.go 54.63% <0%> (-10.31%) ⬇️
lib/node/runner/node_api/api_node_item.go 53.84% <0%> (-10.26%) ⬇️
lib/block/transaction_pool.go 52.83% <0%> (-3.78%) ⬇️
lib/node/runner/util.go 71.42% <0%> (-3.58%) ⬇️
lib/node/runner/finish_ballot.go 43.86% <0%> (-1.42%) ⬇️
lib/node/runner/checker.go 73.36% <0%> (-0.23%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c71cd9b...5b2077b. Read the comment docs.

@anarcher
Copy link
Contributor

How about that we make a branch for it? and make a pr of this branch (for master rebasing) :->

@kfangw
Copy link
Contributor Author

kfangw commented Jan 23, 2019

@anarcher good idea. However, as you can see, this is in WIP.
When I finalize this feature (especially test cases) than I will make a branch what you want.

@anarcher
Copy link
Contributor

anarcher commented Jan 23, 2019

IMHO, state.DB and StateDB are confusing by same naming DB. I would like have layering naming. e.g. State -(use)-> Store -(use)-> DB(leveldb,rocksdb).

@anarcher
Copy link
Contributor

anarcher commented Jan 23, 2019

I am not clear that we have account (only) based states?
As you know, Cosmos style is that multi-substores. With this approaches, we can have accounts and contracts and storages each other. (not having account.Code and account.Storages).

Block.StateRoot = MultiStore's Roothash and each of substores can be accounts(one of StoreKVKey) and contracts, storages... (staking or gov...)

@kfangw
Copy link
Contributor Author

kfangw commented Jan 23, 2019

I am not clear that we have account (only) based states?
As you know, Cosmos style is that multi-substores. With this approaches, we can have accounts and contracts and storages each other. (not having account.Code and account.Storages).

Block.StateRoot = MultiStore's Roothash and each of substores can be accounts(one of StoreKVKey) and contracts, storages... (staking or gov...)

@anarcher
Good idea.
Balance, Code, and Storage can be divided into substores.
But, for the Storage, each account can hold multiple (k,v) pairs.
The key may duplicated
To solve this, IMO, PrefixDB, PrefixStore may be adaptable.

/balance/(address, balance) pair for a data item
/code/(address, code) pair
/storage/(addressprefixed data key, value) pair

Is it makes sense?

This is a example code snippet


func TestMulti(t *testing.T) {
	db := dbm.NewMemDB()

	st := store.NewCommitMultiStore(db)

	key1 := sdk.NewKVStoreKey("balance")
	key2 := sdk.NewKVStoreKey("code")
	key3 := sdk.NewKVStoreKey("storage")

	st.MountStoreWithDB(key1, sdk.StoreTypeIAVL, nil)
	st.MountStoreWithDB(key2, sdk.StoreTypeIAVL, nil)
	st.MountStoreWithDB(key3, sdk.StoreTypeIAVL, nil)

	err := st.LoadLatestVersion()
	require.Nil(t, err)
	
	addrExample := []byte{0x00, 0x00, 0x01, 0x02, 0xFF}

	balanceStore := st.GetCommitKVStore(key1)
	codeStore := st.GetCommitKVStore(key2)
	storageStore := st.GetCommitKVStore(key3)
	theAccountStorageStore := storageStore.Prefix(addrExample)
	
	
	balanceStore.Set(addrExample, []byte("1000"))
	codeStore.Set(addrExample, []byte("print hello world"))
	theAccountStorageStore.Set([]byte("variableKey"), []byte("variableValue"))
	
	commitID := st.Commit()
	
	t.Log(commitID.Hash) //Hash for root Store
	t.Log(balanceStore.LastCommitID().Hash)
	t.Log(codeStore.LastCommitID().Hash)
	t.Log(storageStore.LastCommitID().Hash)
}

@anarcher
Copy link
Contributor

anarcher commented Jan 23, 2019

My first about storage is that storage treated as []byte from storage type. ( accAddress -> storage bytes )

It means that we storage items (k/v) saved one byte array in store (with amino) So we don’t need to prefix if it cares like this. But l am not sure about it. We need to have times to consider it :-)

@kfangw kfangw closed this Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants