Seidb restructure #2653
Seidb restructure #2653
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2653 +/- ##
==========================================
- Coverage 42.01% 42.00% -0.02%
==========================================
Files 1018 1018
Lines 84809 84809
==========================================
- Hits 35636 35624 -12
- Misses 45834 45843 +9
- Partials 3339 3342 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| // Get returns value by key. | ||
| func (db *Database) Get(key []byte) ([]byte, error) { |
There was a problem hiding this comment.
from ai: Pebble Get() returns a value that is only valid until the returned closer is closed.
value is returned while closer.Close() is also called, which means the caller may observe invalid / corrupted data? We should copy value before closing the closer. we can do:
func (db *Database) Get(key []byte) ([]byte, error) {
value, closer, err := db.storage.Get(key)
if err != nil {
if errors.Is(err, pebble.ErrNotFound) {
return nil, nil
}
return nil, errors.WithStack(err)
}
defer closer.Close()
out := append([]byte(nil), value...) // copy
return out, nil
}
There was a problem hiding this comment.
That's interesting, I think this is copied from v3 and we didn't really see this issue at all in v3. Will still fix it though
| // OpenDB opens an existing or create a new database. | ||
| func OpenDB(dbPath string) *Database { | ||
| cache := pebble.NewCache(1024 * 1024 * 512) | ||
| defer cache.Unref() |
There was a problem hiding this comment.
defer cache.Unref() will run when OpenDB() returns, potentially releasing the cache while the DB is still using it?
There was a problem hiding this comment.
We have to keep the defer cache.Unref() - it's the correct pattern recommended by PebbleDB. The idea is to keep reference counting down to 0 when closing the DB.
| } | ||
|
|
||
| func getStorePrefix(storeKey string) []byte { | ||
| return []byte(fmt.Sprintf("s/k:%s/", storeKey)) |
There was a problem hiding this comment.
nitpick perf improve:
func getStorePrefix(storeKey string) []byte {
// "s/k:" + storeKey + "/"
b := make([]byte, 0, len("s/k:/")+len(storeKey))
b = append(b, "s/k:"...)
b = append(b, storeKey...)
b = append(b, '/' )
return b
}
|
|
||
| GetProof(key []byte) *ics23.CommitmentProof | ||
|
|
||
| io.Closer |
There was a problem hiding this comment.
It seems ModuleStore inherits io.Closer, but commitment.NewStore wraps it and never calls Close. Is ModuleStore.Close a no-op? or we should also have it in commitment.Store and call close() on rootmulti
There was a problem hiding this comment.
It's called by CMS I think? But this piece of code need more refactory in the future, I would not really touch it in this PR yet
Describe your changes and provide context
Refactor and restructure seidb to prepare for v3. The new structure will look like this:
This PR made changes of:
Testing performed to validate your change