feat: implement object stores #24159
Conversation
generic interface generic btree generic cachekv generic transient store support ObjStore changelog Update CHANGELOG.md Signed-off-by: yihuang <[email protected]> object store key Apply review suggestions fix merge conflict fix snapshot revert dependers prefix store support object store (cosmos#236) Problem: snapshot for object store is not skipped (cosmos#585) resolve
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| gs.gasMeter.ConsumeGas(gs.gasConfig.ReadCostFlat, types.GasReadCostFlatDesc) | ||
| value = gs.parent.Get(key) | ||
|
|
||
| // TODO overflow-safe math? | ||
| gs.gasMeter.ConsumeGas(gs.gasConfig.ReadCostPerByte*types.Gas(len(key)), types.GasReadPerByteDesc) | ||
| gs.gasMeter.ConsumeGas(gs.gasConfig.ReadCostPerByte*types.Gas(len(value)), types.GasReadPerByteDesc) | ||
| gs.gasMeter.ConsumeGas(gs.gasConfig.ReadCostPerByte*types.Gas(gs.valueLen(value)), types.GasReadPerByteDesc) |
Check failure
Code scanning / gosec
integer overflow conversion uint64 -> uint32 Error
| gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostFlat, types.GasWriteCostFlatDesc) | ||
| // TODO overflow-safe math? | ||
| gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostPerByte*types.Gas(len(key)), types.GasWritePerByteDesc) | ||
| gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostPerByte*types.Gas(len(value)), types.GasWritePerByteDesc) | ||
| gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostPerByte*types.Gas(gs.valueLen(value)), types.GasWritePerByteDesc) |
Check failure
Code scanning / gosec
integer overflow conversion uint64 -> uint32 Error
| if gi.Valid() { | ||
| key := gi.Key() | ||
| value := gi.Value() | ||
|
|
||
| gi.gasMeter.ConsumeGas(gi.gasConfig.ReadCostPerByte*types.Gas(len(key)), types.GasValuePerByteDesc) | ||
| gi.gasMeter.ConsumeGas(gi.gasConfig.ReadCostPerByte*types.Gas(len(value)), types.GasValuePerByteDesc) | ||
| gi.gasMeter.ConsumeGas(gi.gasConfig.ReadCostPerByte*types.Gas(gi.valueLen(value)), types.GasValuePerByteDesc) |
Check failure
Code scanning / gosec
integer overflow conversion uint64 -> uint32 Error
| store, ok := cms.getCacheWrap(key).(types.KVStore) | ||
| if !ok { | ||
| panic(fmt.Sprintf("store with key %v is not KVStore", key)) | ||
| } |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| // AssertValidValueGeneric checks if the value is valid(value is not nil and within length limit) | ||
| func AssertValidValueGeneric[V any](value V, isZero func(V) bool, valueLen func(V) int) { | ||
| if isZero(value) { | ||
| panic("value is nil") |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| panic("value is nil") | ||
| } | ||
| AssertValidValueLength(valueLen(value)) | ||
| } |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
github-actions
bot
added
C:x/evidence
C:x/upgrade
C:x/feegrant
C:x/nft
C:x/circuit
and removed
C:Simulations
labels
| panic(fmt.Sprintf("store with key %v is not ObjKVStore", key)) | ||
| } | ||
|
|
||
| return store |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
📝 WalkthroughWalkthroughThis pull request introduces extensive changes across the storage and runtime components. It adds support for mounting object stores in the base application along with a corresponding ObjectStore API in the context. In addition, the codebase is refactored to adopt generics in various store implementations (cache KV, gas tracking, prefix, B-tree, transient, and multi-store), updating iterator interfaces, cache wrappers, and store type declarations. The modifications also extend to testing and validation functions, ensuring a consistent and type-safe design throughout the system. Changes
Sequence Diagram(s)sequenceDiagram
participant App as BaseApp
participant RT as Runtime
participant Ctx as Context
participant OS as ObjectStore
App->>App: MountObjectStores(keys)
App->>RT: Register object store via ProvideObjectStoreKey
RT-->>App: Object store key registered
Ctx->>OS: Call ObjectStore(key)
OS-->>Ctx: Return ObjKVStore instance
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
store/types/validity.go (1)
30-36:⚠️ Potential issueNew generic function for value validation
This new generic function enhances flexibility by allowing validation of any type, not just byte slices. However, there's a potential issue with panic in consensus methods.
The
panicat line 33 could cause a chain halt if called during BeginBlock or EndBlock consensus methods, as flagged in previous reviews. Consider using error returns instead of panics for safer error handling in consensus-critical code paths.
🧹 Nitpick comments (32)
store/internal/btree/btree.go (5)
25-26: Adopt generic type constraints or constraints to refine usage if needed.Declaring
BTree[V any]is flexible. However, consider whether you need further constraints onV(e.g., comparable) to avoid misusages of the B-tree with unsupported data types in future expansions.
39-39: Naming clarity.
Setis concise, but consider documenting in-code how collisions or overwrites are handled for new developers.
73-77: Shallow vs. deep copy semantics.
bt.tree.Copy()creates a shadow copy of the underlying structure. Document that subsequent modifications in one copy do not bleed into the other, clarifying concurrency usage.
84-86: Generic item struct.
type item[V any]is clear and consistent with the rest of the generic design. Avoid storing excessively largeVvalues in the tree if performance or memory usage is critical.
90-92: Comparison logic.
byKeys[V any]focuses purely on the key. As you expand usage, ensure the ordering remains correct for all valid keys (including empty ones).store/cachemulti/store.go (3)
27-27: Type change for db field.Switching to
types.CacheWrapgives more flexibility for store layering. Ensure consumers handle the new type assumption properly where an underlying KV or Obj store is expected.
53-64: Tracing logic confined to KVStore check.Ensuring only
KVStoreis traced is sensible. If in future a new store type also needs tracing, consider a more extensible approach or a trait-like interface.
181-188: New method for ObjKVStore.
GetObjKVStoreparallelsGetKVStore. The naming is clear, although consider expanding doc comments to clarify difference from a plain KV store.store/internal/btree/memiterator.go (6)
1-1: Package rename to “btree”.Renaming from
internaltobtreecan improve clarity, but guard against exposing this package beyond its intended scope.
17-18: Generic memIterator definition.Parametric polymorphism is a clean approach here. Keep an eye on any constraints needed on
V(e.g., deep copying vs. references) if iterating over complex objects.
26-30: newMemIterator with generics.
startandendchecks, along with building anitem[V]based on an emptyV, fit the rest of the B-tree usage. Document the meaning ofemptyif it's semantically special.
52-58: Iterator validity on creation.Storing the initial
validstate inmi.validensures consistent behavior. Validate if start > end in ascending mode or end < start in descending mode is always a valid scenario.
101-109: Range checking.
keyInRangeenforces ascending vs. descending constraints. Maintain consistency with the rest of the code (checking empty vs. nil).
115-117: Return generic value.Provides direct access to
V. May want to note any effect on memory usage or copying overhead for large typesV.store/types/store.go (1)
473-495: Correct the comment to reflect object store usage.The docstring references "transient stores," which seems to be borrowed from the
TransientStoreKeycomment. Suggest aligning it with object store semantics. For example:-// ObjectStoreKey is used for indexing transient stores in a MultiStore +// ObjectStoreKey is used for indexing object stores in a MultiStoreCHANGELOG.md (1)
58-58: Consistent naming suggestion.In some sections, the feature is referred to as “ObjectStore” or “object store.” Consider maintaining a consistent style (e.g., “ObjectStore API”) to align with other references in the repository.
baseapp/baseapp.go (1)
349-357: Added MountObjectStores methodA new
MountObjectStoresmethod has been implemented to mount object stores with the BaseApp's internal commit multi-store. The implementation follows the same pattern as other store mounting methods, sorting keys alphabetically and mounting each store with theStoreTypeObjecttype.One detail to note is that this method uses
memKeyas the variable name when iterating through object store keys, which might be confusing since we're dealing with object stores, not memory stores. Consider renaming for clarity:- memKey := keys[key] + objKey := keys[key]server/mock/store.go (2)
117-119: Consider adding an implementation or a TODO.
This newGetObjKVStoremethod currently panics. If a full implementation is planned, consider adding a TODO comment or partial logic.Would you like me to open an issue to track this method's implementation or provide a draft?
189-189: Parameters intentionally ignored.
Using underscores to ignore parameters inCacheWrapWithTraceis acceptable, but a brief code comment explaining why these parameters are unnecessary could help future maintainers.store/internal/btreeadaptor.go (4)
25-28: Performance consideration for Has.
Invokingts.Get(key)might have overhead ifGetis expensive. However, this direct check is acceptable for simplicity.Could cache existences or provide a dedicated approach if performance is critical.
30-36: Panic on iteration error.
Panicking inIteratormight be acceptable for internal usage. In production contexts, consider returning the error without crashing the app.func (ts *BTreeStore[V]) Iterator(start, end []byte) types.GIterator[V] { it, err := ts.BTree.Iterator(start, end) if err != nil { - panic(err) + // handle or return error instead of panic } return it }
38-44: Reverse iteration also panics on error.
Same concern as the forward iterator. Consider more graceful error handling.
56-59: Ignoring trace parameters.
Similar to other unimplemented methods, ignoring parameters is acceptable here. If tracing is needed later, ensure a proper pass-through.store/rootmulti/store.go (2)
69-72: Refactor clarifies multi-store support.Switching from
CommitKVStoreto more generalCommitStorebroadens store compatibility. Ensure all usage sites expecting a KV-based interface handle cases where the store might not be a KV store (e.g., object stores).
570-576: Ensure correct KV re-wrapping logic.When re-wrapping the cache store with
listenkv.NewStore, confirm that no double-wrapping occurs elsewhere, which might lead to duplicate events or unintended overhead.store/prefix/store.go (2)
39-39: Typo check in doc comment.The method doc references "for convinience or safety." Consider correcting "convenience" for clarity.
-// for convinience or safety +// for convenience or safety
88-94: Trace integration in CacheWrapWithTrace.Conditional logic to handle only the byte-slice store is typical. This avoids complexity for other types, though it’s a bit specialized. Acceptable approach.
store/gaskv/store.go (4)
9-10: Set object length for gas tracking.
ObjectValueLength = 16is a baseline overhead for object store values. Ensure the chosen length aligns with typical object usage.
23-30: Generic object-based store.
ObjStore = GStore[any]plusNewObjStorehandles gas for arbitrary value types. Consider returning different gas usage for varying object sizes if needed.
67-74: Gas usage on Get.Charging read cost for key length plus value length is standard. Watch for potential integer overflow if
len(key)orvalueLen(value)becomes large.
200-209: Consume gas for iteration.
consumeSeekGascharges read costs for both key and value. Ensure that large values do not cause integer multiplication overflow. Typically not a big concern in practice.store/cachekv/internal/mergeiterator.go (1)
29-30: Constructor improvements with generics.The
NewCacheMergeIteratorfunction properly enforces type safety for differentVtypes. Consider validating thatisZerois not nil to prevent unexpected runtime errors.func NewCacheMergeIterator[V any](parent, cache types.GIterator[V], ascending bool, isZero func(V) bool) types.GIterator[V] { + if isZero == nil { + panic("isZero function must not be nil") + } iter := &cacheMergeIterator[V]{ ... }Also applies to: 34-34
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (18)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumsimapp/go.modis excluded by!**/*.modsimapp/go.sumis excluded by!**/*.sum,!**/*.sumsystemtests/go.modis excluded by!**/*.modsystemtests/go.sumis excluded by!**/*.sum,!**/*.sumtests/systemtests/go.modis excluded by!**/*.modtests/systemtests/go.sumis excluded by!**/*.sum,!**/*.sumx/circuit/go.modis excluded by!**/*.modx/circuit/go.sumis excluded by!**/*.sum,!**/*.sumx/evidence/go.modis excluded by!**/*.modx/evidence/go.sumis excluded by!**/*.sum,!**/*.sumx/feegrant/go.modis excluded by!**/*.modx/feegrant/go.sumis excluded by!**/*.sum,!**/*.sumx/nft/go.modis excluded by!**/*.modx/nft/go.sumis excluded by!**/*.sum,!**/*.sumx/upgrade/go.modis excluded by!**/*.modx/upgrade/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (27)
CHANGELOG.md(1 hunks)baseapp/baseapp.go(2 hunks)runtime/module.go(2 hunks)runtime/store.go(1 hunks)server/mock/store.go(2 hunks)store/CHANGELOG.md(1 hunks)store/cachekv/internal/mergeiterator.go(11 hunks)store/cachekv/search_benchmark_test.go(2 hunks)store/cachekv/store.go(11 hunks)store/cachemulti/store.go(4 hunks)store/gaskv/store.go(2 hunks)store/internal/btree/btree.go(3 hunks)store/internal/btree/btree_test.go(4 hunks)store/internal/btree/memiterator.go(6 hunks)store/internal/btreeadaptor.go(1 hunks)store/listenkv/store.go(2 hunks)store/listenkv/store_test.go(1 hunks)store/prefix/store.go(9 hunks)store/rootmulti/store.go(17 hunks)store/rootmulti/store_test.go(4 hunks)store/tracekv/store.go(1 hunks)store/tracekv/store_test.go(1 hunks)store/transient/store.go(1 hunks)store/types/store.go(7 hunks)store/types/validity.go(2 hunks)types/context.go(1 hunks)x/group/internal/orm/testsupport.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (17)
types/context.go (4)
store/types/store.go (1)
ObjKVStore(303-303)store/gaskv/store.go (1)
NewObjStore(25-30)store/prefix/store.go (1)
NewObjStore(31-37)store/transient/store.go (1)
NewObjStore(49-54)
store/listenkv/store_test.go (1)
store/types/store.go (1)
CacheWrap(329-334)
baseapp/baseapp.go (1)
store/types/store.go (2)
ObjectStoreKey(474-476)StoreTypeObject(366-366)
server/mock/store.go (1)
store/types/store.go (4)
StoreKey(403-406)ObjKVStore(303-303)TraceContext(519-519)CacheWrap(329-334)
runtime/store.go (1)
store/types/store.go (1)
Iterator(297-297)
x/group/internal/orm/testsupport.go (1)
store/types/store.go (1)
KVStore(299-299)
store/internal/btree/btree_test.go (1)
store/internal/btree/btree.go (1)
NewBTree(30-37)
store/tracekv/store_test.go (1)
store/types/store.go (1)
CacheWrap(329-334)
store/internal/btreeadaptor.go (4)
store/types/store.go (5)
KVStore(299-299)Iterator(297-297)GIterator(268-294)StoreType(356-356)StoreTypeDB(360-360)store/internal/btree/btree.go (1)
BTree(25-27)store/cachekv/store.go (1)
NewGStore(60-69)store/transient/store.go (1)
NewGStore(24-26)
store/rootmulti/store.go (6)
store/types/store.go (6)
StoreKey(403-406)CommitStore(35-38)Store(17-20)CommitKVStore(317-320)KVStore(299-299)StoreTypeObject(366-366)store/iavl/store.go (1)
Store(37-41)store/gaskv/store.go (2)
Store(14-14)ObjStore(23-23)store/cachekv/store.go (1)
Store(31-31)store/transient/store.go (4)
Store(29-31)Store(40-42)ObjStore(45-47)ObjStore(56-58)store/listenkv/store.go (1)
Store(15-19)
store/cachekv/search_benchmark_test.go (5)
store/gaskv/store.go (1)
GStore(34-41)store/cachekv/store.go (1)
GStore(44-57)store/prefix/store.go (1)
GStore(42-48)store/transient/store.go (1)
GStore(19-21)store/internal/btree/btree.go (1)
NewBTree(30-37)
store/transient/store.go (3)
store/types/store.go (6)
Committer(23-32)Store(17-20)KVStore(299-299)StoreType(356-356)StoreTypeTransient(362-362)StoreTypeObject(366-366)store/internal/btreeadaptor.go (2)
BTreeStore(14-18)NewBTreeStore(21-23)store/internal/btree/btree.go (1)
NewBTree(30-37)
store/internal/btree/memiterator.go (2)
store/types/store.go (1)
Iterator(297-297)store/internal/btree/btree.go (1)
BTree(25-27)
store/internal/btree/btree.go (1)
store/types/store.go (2)
Iterator(297-297)GIterator(268-294)
store/prefix/store.go (5)
store/gaskv/store.go (4)
GStore(34-41)Store(14-14)ObjStore(23-23)NewGStore(44-59)store/cachekv/store.go (3)
GStore(44-57)Store(31-31)NewGStore(60-69)store/transient/store.go (6)
GStore(19-21)Store(29-31)Store(40-42)ObjStore(45-47)ObjStore(56-58)NewGStore(24-26)store/types/store.go (8)
KVStore(299-299)Store(17-20)ObjKVStore(303-303)GKVStore(247-265)StoreType(356-356)CacheWrap(329-334)Iterator(297-297)GIterator(268-294)store/types/validity.go (1)
AssertValidValueGeneric(31-36)
store/cachekv/store.go (5)
store/types/store.go (8)
CacheKVStore(309-314)Store(17-20)KVStore(299-299)GKVStore(247-265)CacheWrap(329-334)TraceContext(519-519)Iterator(297-297)GIterator(268-294)store/cachemulti/store.go (1)
Store(26-33)store/internal/btree/btree.go (2)
BTree(25-27)NewBTree(30-37)store/types/validity.go (2)
AssertValidKey(13-20)AssertValidValueGeneric(31-36)store/cachekv/internal/mergeiterator.go (1)
NewCacheMergeIterator(29-39)
store/gaskv/store.go (4)
store/types/store.go (6)
KVStore(299-299)Store(17-20)GKVStore(247-265)StoreType(356-356)Iterator(297-297)GIterator(268-294)store/prefix/store.go (2)
Store(14-14)GStore(42-48)store/transient/store.go (3)
Store(29-31)Store(40-42)GStore(19-21)store/types/validity.go (1)
AssertValidValueGeneric(31-36)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: test-system-legacy
- GitHub Check: test-system
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-x-upgrade
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-integration
- GitHub Check: test-e2e
- GitHub Check: build (arm64)
- GitHub Check: Analyze
- GitHub Check: build (amd64)
- GitHub Check: Gosec
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (138)
runtime/store.go (2)
151-157: Return type updated from dbm.Iterator to storetypes.IteratorThis change aligns with the broader update to use generics in store implementations. The
storetypes.Iteratoris now a type alias forGIterator[[]byte], providing better type-safety while maintaining the same functionality.
159-165: Return type updated from dbm.Iterator to storetypes.IteratorThe return type change for the reverse iterator matches the forward iterator change, providing consistency in the API.
store/types/validity.go (3)
3-4: Added import for errors packageThe
errorspackage is now needed for creating error messages in theAssertValidValueLengthfunction.
27-28: Modularized length validation logicExtracting the length validation to a separate function improves code maintainability and reusability.
38-42: Added separate function for value length validationThis encapsulation creates a clear separation of concerns and allows the length validation logic to be reused in both the original and generic validation functions.
However, the same panic issue exists here. Consider returning an error instead:
-func AssertValidValueLength(l int) { +func AssertValidValueLength(l int) error { if l > MaxValueLength { - panic(errors.New("value is too large")) + return errors.New("value is too large") } + return nil }store/internal/btree/btree_test.go (4)
1-2: Package name updated from 'internal' to 'btree'The package name change reflects the move from an internal implementation detail to a more explicitly named btree package.
12-13: Added explicit type parameter to NewBTree function callUpdated the function call to use the new generic API by specifying the type parameter
[]byte.
43-44: Added explicit type parameter to NewBTree function callConsistent with the earlier change, the function call now includes the type parameter
[]byte.
174-175: Added explicit type parameter to NewBTree function callFinal instance of updating the NewBTree function call with the type parameter
[]byte, maintaining consistency throughout the test file.store/rootmulti/store_test.go (5)
803-804: Added new test store keyAdded
testStoreKey4for testing the new object store functionality.
876-886: Updated hashStores to use CommitStore instead of CommitKVStoreThe function signature has been updated to use the more general
types.CommitStoreinterface instead of the specifictypes.CommitKVStore. This change accommodates the new object store functionality.
931-933: Renamed commitKVStoreStub to commitStoreStubThe stub is now more generally named to reflect that it can represent any type of commit store, not just KV stores.
936-940: Updated Commit method signatureThe Commit method now works with the more general
CommitStoretype, maintaining backward compatibility while supporting the new object store functionality.
942-966: Updated prepareStoreMap to include object storeThe function now returns a map that includes the new object store type and prepares all necessary store types for testing. This change is well-structured and provides proper test coverage for the new functionality.
store/internal/btree/btree.go (7)
30-37: Initialization looks good.The
NewBTree[V any]function properly configures the B-tree with balanced defaults. The usage ofbyKeys[V]is consistent with the new generic approach.
43-47: Handle potential nil key.The function gracefully returns an empty value if the key is not found. However, confirm upstream if users might mistakenly pass
nilor empty slices. Possibly add a check to return early with an error if there's an unintendednilusage.
52-54: Confirm correctness for delete logic.Deleting via a new
itemwith an empty value works, since only the key is used for matching in the underlyingbtree.Delete. Ensure there's no confusion about empty vs. nil values forV.
57-62: Boundary checks for nil vs. empty keys.The code correctly distinguishes between
nilchecks and zero-length checks. Confirm that callingIterator([]byte{}, []byte{}, ...)is valid or if an error should be thrown.
64-69: Reverse range iteration logic.The approach for seeking the end boundary, then stepping back aligns with a reverse iteration pattern. Keep an eye on performance if reversed iteration occurs frequently with large data sets.
79-81: Use caution with large data sets.
Clear()quickly empties the B-tree, which is great for performance. Just be mindful of memory usage if large sets are cleared repeatedly within short intervals.
95-97: Helper function is straightforward.
newItem[V any]is a useful constructor for clarity, reinforcing the key-value pairing.store/cachemulti/store.go (3)
41-45: Wrap vs. double wrap.The direct call to
store.CacheWrap()ensures consistent usage. Double-check that repeated wrapping won't degrade performance if done frequently.
164-170: AddedgetCacheWrap.Centralizing store retrieval with proper nil checks helps maintain consistency. The
panicon a missing or invalid store is consistent with existing behavior, though it might be harsher than returning an error.
172-179: Strict KVStore retrieval.Casting to
types.KVStoreand panicking if invalid is standard for this codebase. Just confirm at a higher level that the store is always guaranteed to be typed as KV before usage.store/internal/btree/memiterator.go (9)
12-12: Validation with interface compliance.
var _ types.Iterator = (*memIterator[[]byte])(nil)is a helpful compile-time check to ensurememIteratormeets the required interface.
34-46: Ascending vs. descending logic.Using
Seekfor ascending or descending positioning is straightforward. Confirm that transitioning fromendtoPrev()does not skip any valid items in edge cases.
67-69: Implementing Domain.The domain boundaries reflect the original iteration limits. For clarity, consider docstrings on whether these are inclusive or exclusive.
71-74: Resource release.Releasing the underlying btree iterator is crucial. The
Close()method is minimal and correct.
76-81: Error check for invalid iterator usage.Returning an error if the iterator is invalid is fine, though sometimes code uses
nilerror to indicate iteration is done. Confirm usage in external code.
83-85: Valid method returns the correct flag.No issues here—just ensure no concurrency modifies
mi.valid.
87-99: Next for iteration.
Next()logic is consistent with ascending or descending. The second range check inmi.keyInRange(...)is a good safety measure.
111-113: Key retrieval.Returning the raw byte slice is typical in the Cosmos SDK store design. Confirm that externally, no modification of the returned slice is performed.
119-123: Enforce iterator validity.
assertValidpanics if invalid. This is consistent with store error handling, but do confirm that panic is desired over a simple no-op or error return.store/types/store.go (3)
131-131: Consider documenting store type-check behavior.While this new method aligns with the existing pattern (e.g.,
GetKVStore), if aStoreKeyof the wrong type is passed, the method may panic. Consider documenting the panic condition or returning an error to handle these cases more gracefully.
231-294: Validate usage of generics and verify test coverage.The introduction of
GBasicKVStore[V],GKVStore[V], andGIterator[V]increases type safety and flexibility, but thoroughly verify calls and test coverage to ensure existing consumers handle the new generic APIs correctly (e.g., potential nil values, type mismatches, or serialization quirks).
590-602: Good addition for creating multiple object store keys.This helper function aligns well with
NewKVStoreKeysandNewTransientStoreKeys. No concerns here aside from ensuring any naming collisions are properly handled (theassertNoCommonPrefixcall addresses this).store/CHANGELOG.md (1)
32-35: Entry looks good.Adding a dedicated "Features" section and listing support for object stores helps keep the changelog transparent.
store/listenkv/store_test.go (1)
273-276: Updated test to match new CacheWrap implementation behaviorThe test has been updated to no longer expect a panic when calling
store.CacheWrap(). This aligns with the implementation change instore/listenkv/store.gowhere theCacheWrapmethod now returns a wrapped cache store instead of panicking.types/context.go (1)
352-355: New ObjectStore API added to ContextThis addition introduces object store support to the Context, complementing the existing KVStore and TransientStore methods. The implementation correctly wraps the object store with gas metering capabilities.
The implementation follows the same pattern as the existing store access methods, maintaining consistency in the API design. This is part of a larger change to support object stores throughout the codebase.
store/listenkv/store.go (2)
6-6: New import added for cachekv packageAdded the cachekv import to support the updated CacheWrap implementation.
136-136: CacheWrap now creates a cache wrapper instead of panickingThe
CacheWrapmethod implementation has been updated to return a cache-wrapped store instead of panicking. This is a significant behavioral change that improves the functionality of the ListenKVStore by allowing it to be cache wrapped.This change aligns with the overall architectural improvement of making store types more consistent in their support for caching operations. The corresponding test in
store_test.gohas been updated to reflect this behavior change.store/tracekv/store_test.go (1)
284-287: Updated test to match new CacheWrap implementation behaviorThe test has been updated to no longer expect a panic when calling
store.CacheWrap(). This indicates that theCacheWrapmethod in the corresponding implementation file has been modified to properly return a cache-wrapped store instead of panicking.This change is consistent with similar updates made to the
ListenKVStoreimplementation, showing a systematic approach to improving cache wrapping support across different store types.x/group/internal/orm/testsupport.go (1)
29-30: Updated KVStore method to use type assertionThe method has been changed to retrieve a generic store with
GetCommitStoreand then cast it tostoretypes.KVStore. This modification aligns with the broader changes in the PR to support multiple store types, including the new object stores.store/tracekv/store.go (1)
164-166: Fixed CacheWrap implementation to delegate to parent instead of panickingThe
CacheWrapmethod has been updated to properly delegate to the parent store's implementation rather than panicking. This is a significant improvement as it allows trace KV stores to be used in contexts where cache-wrapping occurs, making the API more consistent with other store implementations.runtime/module.go (2)
70-70: Added ObjectStoreKey provider to module registrationThe
ProvideObjectStoreKeyfunction has been added to the list of providers in theinit()method, enabling modules to request object store keys via dependency injection.
229-233: Implemented ProvideObjectStoreKey functionThe
ProvideObjectStoreKeyfunction creates a new object store key with a namespaced format (object:<module_name>) and registers it with the application builder. This implementation follows the same pattern as the other store key providers, ensuring consistency in how different store types are created and registered.baseapp/baseapp.go (1)
308-310: Added ObjectStoreKey support to MountStores methodThe
MountStoresmethod has been extended to handle*storetypes.ObjectStoreKeytypes, routing them to the appropriate store type. This enables the application to mount object stores alongside other store types.store/cachekv/search_benchmark_test.go (3)
7-7: New import for B-tree usage.
Importing"cosmossdk.io/store/internal/btree"aligns with the move to generic B-tree implementations.
25-30: Using generics for cValue.
Definingmap[string]*cValue[[]byte]{}and storing&cValue[[]byte]{}improves type safety for byte slice values. This approach looks correct and consistent with the rest of the generics refactoring.
39-44: Returning a generic GStore.
Returning&GStore[[]byte]{...}is an appropriate way to integrate the new generics-based store. Ensure existing benchmark logic is still valid under this generic approach.Could you confirm that the benchmark results remain consistent (or improved) with these generic changes?
store/internal/btreeadaptor.go (5)
11-12: Interface compliance check.
Declaring_ types.KVStore = (*BTreeStore[[]byte])(nil)confirmsBTreeStore[[]byte]fulfills theKVStoreinterface, which is good practice.
13-19: Struct embedding BTree.
Embeddingbtree.BTree[V]and storingisZero,valueLencallbacks is a clean design for a generic store.
20-23: Factory method for BTreeStore.
NewBTreeStoreis straightforward. No issues noted.
46-49: Store type set to DB.
ReturningStoreTypeDBis correct if this store truly behaves like a persistent database. Confirm this type aligns with planned usage (versus transient or object store).Do you intend for this to appear as
StoreTypeDBin the multi-store?
51-54: CacheWrap bridging to GStore.
CacheWrapreturningcachekv.NewGStoreis consistent with the dynamic layering approach.store/transient/store.go (14)
14-16: Additional interface checks for ObjStore.
These lines confirmObjStorefulfills bothCommitterandObjKVStore. Good for clarity.
19-21: Generic GStore type.
Embeddinginternal.BTreeStore[V]underGStore[V]is consistent with the new generics strategy.
23-26: NewGStore constructor.
Neatly builds oninternal.NewBTreeStore. Straightforward approach to constructing a transient store.
28-31: Store struct specialized for []byte.
WrappingGStore[[]byte]underStoreis a clean composition approach.
34-38: NewStore method.
Initializes the store with defaultisZeroandvalueLenlambdas for[]byte. Looks good.
40-42: GetStoreType = StoreTypeTransient.
This is consistent with an in-memory ephemeral store.
44-47: ObjStore struct specialized for any.
WrappingGStore[any]underObjStoreis suitable for generic object usage.
49-54: NewObjStore method.
Defaults forisZeroandvalueLenare placeholders, but likely correct for flexible object usage.
56-57: ObjStore → StoreTypeObject.
Appropriate for a specialized object store.
62-64: Commit clears transient data.
Invokingts.Clear()upon commit is appropriate for this ephemeral store.
67-67: No-op pruning.
Transient stores do not require standard pruning. This is consistent with the approach.
71-72: GetPruning returns undefined.
This effectively signals that pruning is not handled within transient stores.
76-76: LastCommitID returns empty.
Transient store commits do not persist, so returning an empty commit ID is logical.
80-80: WorkingHash returns empty.
Empty or zeroed hash is appropriate for a store that does not persist data.store/rootmulti/store.go (9)
180-190: Confirm panic strategy for type mismatch.
GetCommitKVStorepanics if the underlying store isn't aCommitKVStore. This is consistent with typical Cosmos SDK patterns, but consider if returning an error would benefit graceful error handling.
239-239: Use consistent map initialization.
newStores := make(map[types.StoreKey]types.CommitStore)aligns with the updated store interface. Looks good for storing variousCommitStoreimplementations.
631-637: Parallel structure to ascending iteration logic.Similar re-wrapping for reverse iterators is correctly handled here. No immediate concerns.
652-652: Retrieve only once.
store := rs.GetCommitStore(key)inGetStoreis succinct. No further issues.
671-674: Maintain consistent error handling.Panicking on a non-KV store is consistent with other store retrieval methods. Ensure unexpected store types are never passed here to avoid chain halts.
686-699: Introduce new ObjKVStore retrieval.
GetObjKVStoreparallelsGetKVStorewith a panic if type assertion fails. This unblocks mounting different store types. Looks fine.
1027-1080: Load approach for object stores.Support for
StoreTypeObjectinloadCommitStoreFromParamsextends multi-store functionality. Validate usage of the newNewObjStore()or analogous constructor to avoid mismatched store types.
1092-1092: Transient/memory/object store skip logic.Skipping commit logic for ephemeral or object-based stores prevents extraneous overhead. This approach appears consistent with how non-persistent data is handled.
1230-1230: Conditional commit filter.Excluding transient, memory, and object stores from commit steps ensures correct bounding of persistent data. No issues spotted.
store/prefix/store.go (14)
13-16: Define generic store aliases.Introducing
StoreasGStore[[]byte]andObjStoreasGStore[any]expands usage beyond byte arrays. Looks good.
18-21: Interfaces implemented.Declaring that
Storeimplementstypes.KVStoreandObjStoreimplementstypes.ObjKVStoreclarifies type coverage. No concerns.
23-29: New prefix store constructor.
NewStoresets up a customisZeroandvalueLenfor byte slices, ensuring correct validations. Looks solid.
31-37: Initialize object store with generic checks.
NewObjStoreleveragesanyfor the value type with a fixed length in gas calculations. This is consistent and flexible.
42-43: Embed type-aware behavior in GStore.Functions
isZeroandvalueLenlet the sameGStorehandle different type definitions across prefix sub-stores. No immediate issues.Also applies to: 46-47
50-54: Generic constructor is consistent with store types.
NewGStoreproperly assigns the parent and prefix, and sets theisZero/valueLencallbacks. This design fosters reuse.
70-70: Key function enforces non-nil.
key()method panics ifkeyis nil. This is in line with the rest of the cosmos-sdk store logic. Looks good.
79-85: Override of GetStoreType.Maintaining the underlying parent's store type is standard. No issues found.
97-99: Delete usage is straightforward.Nothing unusual. The prefix is appended to identify the correct subrange for deletion.
120-134: Iterator range construction.
cloneAppendusage ensures correct prefixing. The logic fornewendis consistent with prefix store patterns.
138-150: Reverse iterator parallels forward iteration.Uses the same prefixing approach but in reverse. No issues spotted.
153-153: Ensure no data race.
var _ types.Iterator = (*prefixIterator[[]byte])(nil)is a compile-time check. Harmless but helpful.
155-160: Generic prefixIterator logic.Properly strips prefixes, maintains iteration validity, and returns typed values. The approach is consistent with existing prefix store iteration patterns.
Also applies to: 163-170, 179-211
221-229: Error method ensures uniqueness.Returning an error if the iterator is invalid matches typical usage. No concerns about partial iteration states.
store/gaskv/store.go (15)
14-15: New type alias for Gas store.
Store = GStore[[]byte]fosters consistency with other store definitions. Good addition.
16-21: Gas-wrapped store constructor.Initializes
GStorewith specializedisZeroandvalueLenfor[]byte, ensuring correct gas usage for standard KV ops.
32-41: Generic structure for gas usage.Embedding gasMeter, gasConfig, parent, and type functions is consistent with a typed store pattern. No problems.
43-59: Parametric constructor for GStore.Combines parent store, meter, config, and callback functions. Straightforward approach.
62-64: Respect parent's store type.
GetStoreType()defers to the underlying store. Aligns with how other composite stores are implemented.
79-86: Gas usage on Set.Similar to Get, we have both a flat cost and a cost per byte. The callbacks
isZeroandvalueLenensure correctness for the stored type.
90-93: Gas usage on Has.
Hasincurring a fixed cost is consistent with existing gas accounting. This is typical.
96-100: Delete cost.Even though space is freed, some cost is consumed to deter malicious patterns. Looks appropriate.
102-115: Iterators with gas usage.
iterator()method returns a specialized gasIterator that charges appropriately. Straightforward solution.
117-125: Disable cache wrapping.Panicking on
CacheWraporCacheWrapWithTraceis expected in a gas store setting. This is consistent with the approach to not double-wrap gas logic.
127-139: Construct gasIterator.The nested logic for ascending/descending iterators is standard, ensuring
gasIteratorintercepts read calls. No issues.
141-146: gasIterator structure & constructor.
gasConfigplusvalueLencallback preserve typed usage. Clean design continuing the pattern.Also applies to: 148-154
158-165: Next usage for iteration cost.Consuming gas for each seek step is consistent with the distributed ledger's cost model.
Also applies to: 167-173
184-186: Value accessor.No additional gas cost for returning the current value is consistent with typical store designs.
189-191: Close & Error forwarding.Simply delegates to the parent, as expected. No concerns.
Also applies to: 193-196
store/cachekv/internal/mergeiterator.go (13)
17-19: Fully generic struct enhancements.Defining
cacheMergeIterator[V any]and introducing theisZerofunction nicely generalizes zero-value detection. Ensure that implementers remember to handle any potential panics ifisZerois inadvertently set tonil.Also applies to: 24-24
27-27: Compile-time interface check.This assignment to
_enforces a compile-time check ensuringcacheMergeIterator[[]byte]implementstypes.Iterator. This pattern is correct and commonly used in Go for interface compliance.
43-43: Domain method remains consistent.No issues found with this method’s updated signature; it smoothly forwards to the parent’s domain, preserving existing domain semantics.
48-48: Valid method consistency.The
Valid()method retains expected behavior; no concerns.
53-53: Next method clarity.The
Next()logic correctly merges parent and cache iterators. No performance pitfalls detected.
80-80: Key method correctness.Appropriately handles priority in the merge logic. No issues found.
110-110: Value method correctness.The method merges the value logic consistently with the parent or cache. Looks good.
140-140: Close method concurrency safety.Closing both iterators is straightforward and error handling is consolidated. This is correct and safe as no concurrency is involved here by design.
151-151: Error method alignment.Method returns an error if invalid—consistent with the
Valid()check. No concerns.
161-161: Assertion handles invalid state properly.Panicking on an invalid iterator is consistent with the store’s approach. Proceed.
168-168: compare function inversion logic.Flips sign for descending order; correct use of
bytes.Compare. No issues.
181-183: skipCacheDeletes uses zero-value logic.Replacing nil-check with
isZeroaccommodates any type. Implementation is coherent. EnsureisZeroremains consistent with how values are stored.
192-192: skipUntilExistsOrInvalid zero-value handling.Handling deletes by checking
isZero(valueC)is intuitive. The iteration logic properly skips invalid or deleted items. This is essential for correct cache-parent merging.Also applies to: 217-217, 229-229
store/cachekv/store.go (18)
13-13: New btree import.Importing
"cosmossdk.io/store/internal/btree"is expected for the new generic B-tree usage. Looks good.
21-28: Generic cValue and kvPair definitions.Introducing
cValue[V any]andkvPair[V any]provides flexibility for arbitrary value types. The approach is clean and aligns with the overall generics refactor.
31-32: Type alias for backward compatibility.
type Store = GStore[[]byte]preserves existing references toStorewhile adopting generics under the hood. This is a good bridging strategy.
36-42: Refactored NewStore constructor.
NewStorenow delegates toNewGStore, providing specializedisZeroandvalueLenfor[]byte. Implementation is straightforward.
43-58: Thread-safe GStore struct with zero-value checks.Storing
isZeroandvalueLenfunction pointers is a flexible design. No immediate concurrency issues are visible, given thesync.Mutex.
59-69: NewGStore generic constructor.Constructs the store with typed parent and B-tree. Make sure
isZeroandvalueLenremain consistent with any custom logic in advanced usage.
71-72: Preserved store type.
GetStoreType()defers to the parent’s store type, preserving existing semantics. No concerns.
77-77: Get method generification.Properly locks the store, fetches values from cache or parent, and caches them. Basic logic remains unchanged except for the generic type. Good approach.
95-97: Set method with generic validation.Using
AssertValidValueGenericis an excellent approach to ensure correctness across multiple value types. Implementation is clear and consistent.
105-107: Has method uses isZero.Replacing nil-check with
!isZero(value)is consistent with the new generic design. No issues.
111-117: Delete method sets zeroValue.Replaces the entry with a zero value rather than removing from the map, aiding iteration logic. Implementation looks consistent with the new design.
120-120: resetCaches method.Large cache resets are a known optimization technique. Re-initializing
sortedCache = btree.NewBTree[V]()also makes sense to handle big sets. LGTM.Also applies to: 126-126, 139-139
142-142: Write method improvements.• Locks for thread safety.
• Rebuilds a local slice from dirty entries, sorts, and writes them to the parent.
• Usesstore.resetCaches()to free memory.Everything aligns with the new generics approach.
Also applies to: 143-143, 148-148, 155-155, 180-180
189-190: CacheWrap refactor.
CacheWrapWithTracegracefully wraps the trace store only when working with[]byte. For other generic types, it falls back to a standard cache wrap. Clever approach.Also applies to: 193-198
205-206: Iterator methods.Delegates domain-specific iteration to the new generic
iteratorfunction. No issues.Also applies to: 210-211
214-215: iterator function hooking into NewCacheMergeIterator.Combines an isolated copy of sortedCache with the parent’s iterator. The usage of
internal.NewCacheMergeIteratorensures consistent detection of zero-value deletes. Nicely done.Also applies to: 218-219, 223-224, 237-238
335-335: dirtyItems domain filtering.Builds a slice of only the unsorted keys within [start, end), then sorts them if needed. The logic looks correct and is consistent with existing boundaries.
Also applies to: 339-339, 343-344
396-398: clearUnsortedCacheSubset & setCacheValue.Clearing or removing from
unsortedCacheby referencingkvPair[V]keys, then inserting intosortedCache, is consistent with the multi-level caching approach.
The newsetCacheValuedesign storing zero-value for deletes is also coherent.Also applies to: 424-424, 427-428
Description
replace #22893 for integrating with block stm, the series of backports branch was tested under crypto-org-chain/ethermint#564
Module parameters are loaded multiple times for each tx, although the raw bytes are cached, the decoding process is not.
Support an object store that is transient stores for interface{}, applications can cache objects directly, without doing unnecessary encoding/decoding.
Caching module parameters is an important use case for this, there are also many use cases out there.
Add prefix store support object store.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Improvements