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 43.78% 43.75% -0.03%
==========================================
Files 1904 1906 +2
Lines 158932 159030 +98
==========================================
+ Hits 69585 69589 +4
- Misses 82944 83041 +97
+ Partials 6403 6400 -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.
There was a problem hiding this comment.
i double checked this issue. the defer executes immediately when OpenDB returns, not when the DB is closed though
| } | ||
|
|
||
| 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
| @@ -0,0 +1 @@ | |||
| package parquet | |||
| @@ -0,0 +1 @@ | |||
| package event | |||
## Describe your changes and provide context Refactor and restructure seidb to prepare for v3. The new structure will look like this: <img width="736" height="349" alt="image" src="https://github.com/user-attachments/assets/de643253-8ffc-4631-9152-ec2a6b5f2676" /> This PR made changes of: 1. Move files around to different folder names 2. Update all dependency references based on the new structure 3. Add new placeholders for future preparation 4. Add a new basic pebbledb implementation ## Testing performed to validate your change
## Describe your changes and provide context Refactor and restructure seidb to prepare for v3. The new structure will look like this: <img width="736" height="349" alt="image" src="https://github.com/user-attachments/assets/de643253-8ffc-4631-9152-ec2a6b5f2676" /> This PR made changes of: 1. Move files around to different folder names 2. Update all dependency references based on the new structure 3. Add new placeholders for future preparation 4. Add a new basic pebbledb implementation ## Testing performed to validate your change
Refactor and restructure seidb to prepare for v3. The new structure will look like this: <img width="736" height="349" alt="image" src="https://github.com/user-attachments/assets/de643253-8ffc-4631-9152-ec2a6b5f2676" /> This PR made changes of: 1. Move files around to different folder names 2. Update all dependency references based on the new structure 3. Add new placeholders for future preparation 4. Add a new basic pebbledb implementation
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