rpc: Add getindexinfo RPC #19550
rpc: Add getindexinfo RPC #19550
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK |
|
Thanks for the reviews, I should have addressed all the comments:
|
|
Tested ACK f301c86 |
|
I think this is a helpful addition for connecting applications. |
|
The first commit is already merged in master. The change was embedded into commit 12410b1. Reviewing the main commit now. |
|
Added const usage and research notes. Dropped first commit and rebased on master. |
| return results; | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Indentation, could clang-format listindices() if you retouch -- see details for an idea.
clang-format -i src/rpc/misc.cpp
static RPCHelpMan listindices()
{
- return RPCHelpMan{"listindices",
- "\nReturns the available indices and their status.\n",
- {},
- RPCResult{
- RPCResult::Type::OBJ, "", "", {
- {
- RPCResult::Type::OBJ, "name", "The name of the index",
- {
- {RPCResult::Type::BOOL, "synced", "Whether the index is synced or not"},
- {RPCResult::Type::NUM, "best_block_height", "The block height to which the index is synced"},
- }
- },
- },
- },
- RPCExamples{
- HelpExampleCli("listindices", "")
- },
- [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
-{
- UniValue results(UniValue::VOBJ);
+ return RPCHelpMan{
+ "listindices",
+ "\nReturns the available indices and their status.\n",
+ {},
+ RPCResult{
+ RPCResult::Type::OBJ,
+ "",
+ "",
+ {
+ {RPCResult::Type::OBJ, "name", "The name of the index", {
.../...
+ },
+ },
+ RPCExamples{HelpExampleCli("listindices", "")},
+ [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
+ UniValue results(UniValue::VOBJ);
- if (g_txindex) {
- results.pushKVs(SummaryToJSON(g_txindex->GetSummary()));
- }
+ if (g_txindex) {
+ results.pushKVs(SummaryToJSON(g_txindex->GetSummary()));
+ }
- ForEachBlockFilterIndex([&results](BlockFilterIndex& index) {
- results.pushKVs(SummaryToJSON(index.GetSummary()));
- });
+ ForEachBlockFilterIndex([&results](BlockFilterIndex& index) {
+ results.pushKVs(SummaryToJSON(index.GetSummary()));
+ });
- return results;
-},
+ return results;
+ },
};
}There was a problem hiding this comment.
I have chosen this indentation to keep it consistent with https://github.com/bitcoin/bitcoin/pull/19528/files
|
Code review re-ACK 28b4cbf per Thanks, and sorry about that; I'm not sure the default initialization is useful in this case and in non-critical code, though I guess it can't hurt. When I saw this catch today that I had missed in my own review, I resolved to reinforce my thinking about this. |
Or Per RPCs are hard to change once released, so it seems a good idea to get them right. |
|
Thanks for the suggestions! Honestly, I was a bit on the fence on the naming of this RPC anyway and since there were two reviewers in favor of this renaming and none against, I have implemented it now as Please note this trade-off in the code currently: Even when an index name (even an incorrect one) is given, it will still get the status for all/both indices internally. That could be easily fixed by matching the index name param against a set of index names allowed. While less efficient, the upside of the current approach is it uses fewer LOC and that it has more flexibility in the sense that it will automatically work if a new (custom?) block filter index is introduced. I think this is ok since an internal optimization can still be done later. I am thinking of something like an I also went back on passing the result around, it seemed cleaner this way now. And squashed into one commit. |
|
You may want to update PR title. |
This was my first thought as well: if an index is specified, ISTM the output should give feedback on the index that it understood (by displaying it, the same as when no index is specified). I'll review properly tomorrow, so don't give this comment too much weight yet. |
There was a problem hiding this comment.
ACK in general, with some suggestions.
Perhaps the param would best be named index, for what it is, rather than name. See the param names of the other MiscRPCCommands.
Perhaps it would be better to return the same output for one index, as for all indexes when only one index is active, thereby providing feedback that the RPC correctly understood the passed index.
Based on what most of the other PRs being merged are doing (see just-merged https://github.com/bitcoin/bitcoin/pull/19473/commits), I don't think you are required to squash this into one single commit as long as they are hygienic. The more frequent pattern for this change would seem to be one commit for the change, one for the tests, and one for the release note.
|
Pushed the changes as requested. I understand now the name parameter was more thought of like a filter so I documented it as such. I also renamed it to |
|
Addressed review comments by @jonatack |
Github-Pull: bitcoin#19550 Rebased-From: e733410
Github-Pull: bitcoin#19550 Rebased-From: 84ec6f1
|
Re-ACK 124e1ee |
124e1ee doc: Add release notes for getindexinfo RPC (Fabian Jahr) c447b09 test: Add tests for getindexinfo RPC (Fabian Jahr) 667bc7a rpc: Add getindexinfo RPC (Fabian Jahr) Pull request description: As I was playing with indices a I was missing an RPC that gives information about the active indices in the node. I think this can be helpful for many users, especially since there are some new index candidates coming up (bitcoin#14053, bitcoin#18000) that can give a quick overview without the user having to parse the logs. Feature summary: - Adds new RPC `listindices` (placed in Util section) - That RPC only lists the actively running indices - For each index it gives the name, whether it is synced and up to which block height it is synced ACKs for top commit: laanwj: Re-ACK 124e1ee jonatack: Code review re-ACK 124e1ee per `git range-diff a57af89 47a5372 124e1ee` no change since my last re-ACK, rebase only Tree-SHA512: 3b7174c87951e6457fef099f530337803906baf32fb64261410b8def2c0917853d6a1bf3059cd590b1cc1523608f8916dafb327a431d27ecbf8d7454406b5b35
Extend upstream bitcoin/bitcoin#19550 to support also the namehash index.
Summary: This is a backport of [[bitcoin/bitcoin#19550 | core#19550]] Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10118
As I was playing with indices a I was missing an RPC that gives information about the active indices in the node. I think this can be helpful for many users, especially since there are some new index candidates coming up (#14053, #18000) that can give a quick overview without the user having to parse the logs.
Feature summary:
getindexinfo(placed in Util section)