-
Notifications
You must be signed in to change notification settings - Fork 4.1k
fix(grpc): return actual earliest_store_height in node.Status endpoint #25647
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
base: main
Are you sure you want to change the base?
Conversation
|
Would love to get this backported to 0.50.x. It would enable fixing manifest-network/yaci#28 |
|
This seems like a nice feature but will need more extensive testing such as using |
e7384c5 to
eaa696a
Compare
|
Added systemtest for the
Also updated the service to use Run locally: make build && cp build/simd tests/systemtests/binaries/
cd tests/systemtests && go test -tags system_test -v -run TestNodeStatus |
|
@aljo242 is there anything blocking this PR? |
|
Bump. Would like to be sure there is nothing else needed from my end here. |
|
@aljo242 is there anything I can do to help this one along? |
995a18e to
1172c68
Compare
|
@technicallyty to review this |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25647 +/- ##
==========================================
+ Coverage 68.23% 70.70% +2.46%
==========================================
Files 894 895 +1
Lines 58150 58217 +67
==========================================
+ Hits 39680 41161 +1481
+ Misses 18470 17056 -1414
🚀 New features to boost your workflow:
|
i'm fine with merging this with systemtests, but i would like to use systemtest more sparingly in the future. systest runs a local 4 node network, which is not necessary for queries like this. an e2e integration test is more than enough for these types of PRs |
| // TODO: Get earliest version from store. | ||
| // | ||
| // Ref: ... | ||
| // EarliestStoreHeight: sdkCtx.MultiStore(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason why we can't use the sdkCtx.MultiStore() to achieve this functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I know -- I can change it if that seems like a better way to go.
Edit: we would have to add a ctx.EarliestHeight() to it first, which is more consistent but I guess more code as well. Which do you think is the better path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain why it would have to be a method on ctx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't — my bad. I moved EarliestVersion() from MultiStore to CommitMultiStore instead.
The query context's MultiStore() is an in-memory CacheMultiStore — created in CreateQueryContext via CacheMultiStoreWithVersion(height) with no reference to the root DB or its metadata.
Placing it on CommitMultiStore means CacheMultiStore doesn't need to implement it at all and avoids a stub that just panics (like LatestVersion() currently does on CacheMultiStore, which is the bad example I was following originally). This way the query/service receives CommitMultiStore at registration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok so if we go that route, we could just get EarliestHeight from ctx and then we don't have to pass around the cms? i'd prefer that
4f40612 to
5c4a042
Compare
Add EarliestVersion() to the CommitMultiStore interface to enable querying the earliest available state height. Placed on CommitMultiStore rather than MultiStore since it requires root DB access that CacheMultiStore does not have. - Implement in rootmulti.Store with persistent DB storage - Track earliest version after pruning in PruneStores - Add unit tests for EarliestVersion functionality Ref: cosmos#15463
Pass CommitMultiStore to the node query service to populate EarliestStoreHeight in the Status response.
5c4a042 to
609ddf6
Compare
|
I edited the original description and cleaned it up a bit. This makes more sense for sure. Sorry for the mess. |
Description
Closes: #15463
Add
EarliestVersion()to theCommitMultiStoreinterface to enable querying the earliest available state height.This addresses the longstanding TODO in the Status gRPC endpoint that was returning
0forEarliestStoreHeight.Problem
Indexers and tooling need to know which heights a pruned node can serve queries for. Currently:
earliest_block_heightin CometBFT's STATUS RPC returns the earliest block, not the earliest stateEarliestStoreHeightfield incosmos.base.node.v1beta1.Statuswas hardcoded to0Solution
EarliestVersion() int64to theCommitMultiStoreinterface (notMultiStore, sinceCacheMultiStorelacks root DB access)rootmulti.Store:s/earliestkey1for unpruned chainsPruneStores()CommitMultiStoreto the node gRPC service to populateEarliestStoreHeightChanges
store/types/store.goEarliestVersion()to `client/grpc/node/service.goCommitMultiStore, useEarliestVersion()in Status responsestore/rootmulti/store.goEarliestVersion(),GetEarliestVersion(),flushEarliestVersion(), updatePruneStores()store/rootmulti/store_test.goruntime/app.goCommitMultiStoreto service registrationsimapp/app.goCommitMultiStoreto service registrationTesting
TestEarliestVersion- basic functionality, default valueTestEarliestVersionWithPruning- tracks updates after pruningTestEarliestVersionPersistence- persists across restartsTestNodeStatus- gRPC endpoint returns valid heightsAPI Changes
Breaking change for any code implementing
CommitMultiStore— must now implementEarliestVersion() int64.No impact on
MultiStoreorCacheMultiStoreimplementations.