rpc: Add getindexinfo RPC #19550

pull fjahr wants to merge 3 commits into bitcoin:master from fjahr:index_rpc changing 5 files +107 −0
  1. fjahr commented at 10:21 PM on July 18, 2020: member

    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:

    • Adds new RPC getindexinfo (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
  2. promag commented at 10:30 PM on July 18, 2020: member

    Concept ACK, why not? This can help the client determine whether the node is capable for it's needs.

    Instead of returning an array, it could return an object { name: summary }, or duplicate index names are allowed?

  3. DrahtBot added the label RPC/REST/ZMQ on Jul 19, 2020
  4. DrahtBot added the label Tests on Jul 19, 2020
  5. DrahtBot added the label UTXO Db and Indexes on Jul 19, 2020
  6. DrahtBot commented at 3:50 AM on July 19, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19528 (rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc) by MarcoFalke)
    • #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)

    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.

  7. in src/rpc/misc.cpp:7 in 6b7d72dc72 outdated 
       3 | @@ -4,6 +4,8 @@
   4 |  // file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 |  
   6 |  #include <httpserver.h>
   7 | +#include <index/txindex.h>
   8 | +#include <index/blockfilterindex.h>
    jonatack commented at 4:48 AM on July 19, 2020:

    nit: sort

  8. in src/rpc/misc.cpp:617 in 6b7d72dc72 outdated 
     612 | +{
 613 | +            RPCHelpMan{"listindices",
 614 | +                "\nReturns the available indices and their status.\n",
 615 | +                {},
 616 | +                RPCResult{
 617 | +                    RPCResult::Type::ARR, "", "",
    jonatack commented at 4:51 AM on July 19, 2020:

    Not sure it makes sense to do so here, but ISTM returning a JSON object is generally the preferred RPC result.

    MarcoFalke commented at 6:03 AM on July 19, 2020:

    The name should be unique, so it could be the key of the json object?

  9. jonatack commented at 4:58 AM on July 19, 2020: member

    Concept ACK, thanks for adding the test.

  10. in test/functional/rpc_misc.py:14 in 6b7d72dc72 outdated 
      10 | @@ -11,6 +11,7 @@
  11 |      assert_equal,
  12 |      assert_greater_than,
  13 |      assert_greater_than_or_equal,
  14 | +    wait_until,
    MarcoFalke commented at 6:04 AM on July 19, 2020:

    unrelated style nit: If you want you can cherry-pick the change to the test framework from https://github.com/bitcoin/bitcoin/pull/19134/files#diff-64721c5ee64d44f7114d6d0d2226db4d and then call self.wait_until below instead of the import.

  11. in src/rpc/misc.cpp:664 in 6b7d72dc72 outdated 
     655 | @@ -611,6 +656,7 @@ static const CRPCCommand commands[] =
 656 |      { "util",               "getdescriptorinfo",      &getdescriptorinfo,      {"descriptor"} },
 657 |      { "util",               "verifymessage",          &verifymessage,          {"address","signature","message"} },
 658 |      { "util",               "signmessagewithprivkey", &signmessagewithprivkey, {"privkey","message"} },
 659 | +    { "util",               "listindices",            &listindices,            {} },
    MarcoFalke commented at 6:07 AM on July 19, 2020:

    unrelated style-nit: Maybe use the "new" constructor that takes an RpcMethodFnType . See https://doxygen.bitcoincore.org/class_c_r_p_c_command.html#afa39f5bda9319f3b91c6b38bbc7c7434 and #19528

  12. in src/rpc/misc.cpp:635 in 6b7d72dc72 outdated 
     630 | +            }.Check(request);
 631 | +
 632 | +    UniValue results(UniValue::VARR);
 633 | +
 634 | +    if (g_txindex) {
 635 | +        AddSummary(g_txindex->GetSummary(), results);
    MarcoFalke commented at 6:11 AM on July 19, 2020:

    nit: Could avoid passing results around

            results.push_back(SummaryToJson(g_txindex->GetSummary()));
  13. MarcoFalke approved
  14. MarcoFalke commented at 6:12 AM on July 19, 2020: member

    review ACK, only style nits

  15. MarcoFalke deleted a comment on Jul 19, 2020
  16. MarcoFalke removed the label Tests on Jul 19, 2020
  17. theStack commented at 3:42 PM on July 19, 2020: member

    Concept ACK

  18. fjahr force-pushed on Jul 19, 2020
  19. fjahr commented at 7:19 PM on July 19, 2020: member

    Thanks for the reviews, I should have addressed all the comments:

    • Now returning an object with the index names as keys (names are not enforced to be unique by the code at the moment I think but the names are used to distinguish the indices in the logs so they should be unique).
    • Cherry-picked the commit that makes wait_until part of the TestFramework and used it
    • Used new RPC constructor
    • Not passing around results object anymore
    • Fixed includes sorting
  20. in src/index/base.h:117 in f301c86b77 outdated 
     111 | @@ -106,6 +112,9 @@ class BaseIndex : public CValidationInterface
 112 |  
 113 |      /// Stops the instance from staying in sync with blockchain updates.
 114 |      void Stop();
 115 | +
 116 | +    /// Get a summary of the index and its state.
 117 | +    IndexSummary GetSummary();
    promag commented at 9:38 PM on July 19, 2020:

    Could be const?

    fjahr commented at 7:23 PM on July 25, 2020:

    done

  21. laanwj commented at 8:38 PM on July 22, 2020: member

    Tested ACK f301c86b77f027f350e646e19afe1d95a46bc24c (tested with and without -txindex on regtest)

  22. promag commented at 8:30 AM on July 23, 2020: member

    Code review ACK f301c86b77f027f350e646e19afe1d95a46bc24c, needs release notes.

  23. jonasschnelli commented at 1:53 PM on July 25, 2020: contributor

    I think this is a helpful addition for connecting applications. Code Review utACK f301c86b77f027f350e646e19afe1d95a46bc24c.

  24. jonatack commented at 4:19 PM on July 25, 2020: member

    The first commit is already merged in master. The change was embedded into commit 12410b1feb80189061eb4a2b43421e53cbb758a8. Reviewing the main commit now.

  25. in src/index/base.h:19 in f301c86b77 outdated 
      12 | @@ -13,6 +13,12 @@
  13 |  
  14 |  class CBlockIndex;
  15 |  
  16 | +struct IndexSummary {
  17 | +    std::string name;
  18 | +    bool synced;
  19 | +    int best_block_height;
    jonatack commented at 5:51 PM on July 25, 2020:

    Would non-static member initialization here provide potential extra safety?

    -    bool synced;
-    int best_block_height;
+    bool synced{false};
+    int best_block_height{1};
    fjahr commented at 7:18 PM on July 25, 2020:

    done

  26. fjahr force-pushed on Jul 25, 2020
  27. fjahr commented at 5:52 PM on July 25, 2020: member

    Added const usage and research notes. Dropped first commit and rebased on master.

  28. in src/rpc/misc.cpp:693 in 3785cfd3c3 outdated 
     643 | +    });
 644 | +
 645 | +    return results;
 646 | +},
 647 | +    };
 648 | +}
    jonatack commented at 6:00 PM on July 25, 2020:

    Indentation, could clang-format listindices() if you retouch -- see details for an idea.

    <details><summary><code>clang-format -i src/rpc/misc.cpp</code></summary><p>

     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;
+        },
     };
 }

    </p></details>

    fjahr commented at 6:29 PM on July 25, 2020:

    I have chosen this indentation to keep it consistent with https://github.com/bitcoin/bitcoin/pull/19528/files

  29. in src/index/base.cpp:325 in 3785cfd3c3 outdated 
     318 | @@ -319,3 +319,12 @@ void BaseIndex::Stop()
 319 |          m_thread_sync.join();
 320 |      }
 321 |  }
 322 | +
 323 | +const IndexSummary BaseIndex::GetSummary()
 324 | +{
 325 | +    IndexSummary summary;
    jonatack commented at 6:09 PM on July 25, 2020:

    Would IndexSummary summary{}; provide further safety?

    fjahr commented at 7:18 PM on July 25, 2020:

    done

  30. jonatack commented at 6:24 PM on July 25, 2020: member

    ACK 3785cfd3c3 code review, debug build/tests, tested the RPC and its help.

  31. fjahr force-pushed on Jul 25, 2020
  32. fjahr commented at 7:23 PM on July 25, 2020: member

    Also fixed initializations are proposed by @jonatack . I am leaving the indentation as is since it is consistent with #19528 and the suggested change from clang-format seem pretty different from our general formatting for RPCs so far.

  33. jonatack commented at 7:52 PM on July 25, 2020: member

    Code review re-ACK 28b4cbfa5a5 per git diff 3785cfd 28b4cbf

    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.

  34. in src/index/base.h:117 in 28b4cbfa5a outdated 
     111 | @@ -106,6 +112,9 @@ class BaseIndex : public CValidationInterface
 112 |  
 113 |      /// Stops the instance from staying in sync with blockchain updates.
 114 |      void Stop();
 115 | +
 116 | +    /// Get a summary of the index and its state.
 117 | +    const IndexSummary GetSummary();
    MarcoFalke commented at 5:58 AM on July 26, 2020: 
        IndexSummary GetSummary() const;

    The const needs to be in the end, otherwise it has no effect

    fjahr commented at 9:55 AM on July 26, 2020:

    duh, done

  35. in src/rpc/misc.cpp:641 in 28b4cbfa5a outdated 
     636 | +
 637 | +    if (g_txindex) {
 638 | +        results.pushKVs(SummaryToJSON(g_txindex->GetSummary()));
 639 | +    }
 640 | +
 641 | +    ForEachBlockFilterIndex([&results](BlockFilterIndex& index) {
    MarcoFalke commented at 5:58 AM on July 26, 2020: 
        ForEachBlockFilterIndex([&results](const BlockFilterIndex& index) {

    You might be able to test whether the const works by applying this patch

    fjahr commented at 9:55 AM on July 26, 2020:

    yep, done

  36. fjahr force-pushed on Jul 26, 2020
  37. fjahr commented at 9:56 AM on July 26, 2020: member

    Addressed review comments by @MarcoFalke

  38. promag commented at 10:12 AM on July 26, 2020: member

    Code review ACK.

    Feel free to squash commits.

    I'd suggest to rename this RPC, but its probably too late. I was thinking getindexstatus [index], with optional index parameter - if null/omitted returns all indices.

  39. MarcoFalke commented at 10:26 AM on July 26, 2020: member

    ACK 8094e211d9af2bbe16d4de865aca5268bbc0f0f1

  40. jonatack commented at 10:54 AM on July 26, 2020: member

    I was thinking getindexstatus [index], with optional index parameter - if null/omitted returns all indices.

    Or getindexinfo.

    Per bitcoin-cli help | grep "info\|list", "list" RPCs seem to deal with user-side financial data, and "info" RPCs are more for technical-side settings and might make more sense here.

    RPCs are hard to change once released, so it seems a good idea to get them right.

  41. fjahr commented at 12:32 AM on July 28, 2020: member

    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 getindexinfo. It also takes an index name now as suggested. Sorry reviewers, most of the code has changed now.

    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 IndexMan or so to encapsulate that logic better for when we have more indices in the future. But of course, I can still change this now if that is preferred.

    I also went back on passing the result around, it seemed cleaner this way now. And squashed into one commit.

  42. fjahr force-pushed on Jul 28, 2020
  43. promag commented at 1:03 AM on July 28, 2020: member

    You may want to update PR title.

  44. fjahr renamed this:
    rpc: Add listindices RPC
    rpc: Add getindexinfo RPC
    on Jul 28, 2020
  45. promag commented at 7:25 PM on July 28, 2020: member

    Not sure if the format should be different if an index is specified. And if the specified index is unknown (either invalid or just because it's not available) then the resulting object would be {}. No strong opinion though.

  46. jonatack commented at 7:31 PM on July 28, 2020: member

    Not sure if the format should be different if an index is specified. And if the specified index is unknown (either invalid or just because it's not available) then the resulting object would be {}. No strong opinion though.

    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.

  47. in doc/release-notes.md:118 in 4771dcc35d outdated 
     114 | @@ -115,6 +115,10 @@ Changes to Wallet or GUI related RPCs can be found in the GUI or Wallet section
 115 |  New RPCs
 116 |  --------
 117 |  
 118 | +- The `getindexinfo` RPC by default lists the actively running indices of the
    jonatack commented at 8:54 AM on July 29, 2020:

    I think it would be clearer to omit "by default", and maybe s/lists/returns/

    fjahr commented at 1:06 PM on July 31, 2020:

    done

  48. in doc/release-notes.md:120 in 4771dcc35d outdated 
     114 | @@ -115,6 +115,10 @@ Changes to Wallet or GUI related RPCs can be found in the GUI or Wallet section
 115 |  New RPCs
 116 |  --------
 117 |  
 118 | +- The `getindexinfo` RPC by default lists the actively running indices of the
 119 | +  node, including their current sync status and height. It also accepts an index
 120 | +  name to only return the status of that index. (#19550)
    jonatack commented at 8:55 AM on July 29, 2020: 
      `index` param to specify returning only the status of that index. (#19550)
  49. jonatack commented at 9:03 AM on July 29, 2020: member

    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.

  50. fjahr commented at 1:06 PM on July 31, 2020: member

    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 index_name which is consistent with wallet_name which seemed the most comparable.

  51. fjahr force-pushed on Jul 31, 2020
  52. in src/rpc/misc.cpp:621 in dc888cdb07 outdated 
     616 | +static RPCHelpMan getindexinfo()
 617 | +{
 618 | +    return RPCHelpMan{"getindexinfo",
 619 | +                "\nReturns the status of one or all available indices currently running in the node.\n",
 620 | +                {
 621 | +                    {"index_name", RPCArg::Type::STR, /* default */ "", "Filter results for an index with a specific name."},
    jonatack commented at 1:36 PM on July 31, 2020:

    Would (something like) this be better?

                        {"index_name", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Filter results for an index with a specific name."},

    It would change the help from

    Arguments:
1. index_name    (string, optional, default=) Filter results for an index with a specific name.

    to

    Arguments:
1. index_name    (string, optional) Filter results for an index with a specific name.
    jonatack commented at 1:42 PM on July 31, 2020:

    (I should have given feedback about the "default=" earlier, but somehow kept forgetting to add it to the review)

  53. in src/rpc/misc.cpp:607 in dc888cdb07 outdated 
     598 | @@ -597,6 +599,62 @@ static UniValue echo(const JSONRPCRequest& request)
 599 |      return request.params;
 600 |  }
 601 |  
 602 | +static UniValue SummaryToJSON(const IndexSummary&& summary, std::string index_name)
 603 | +{
 604 | +    UniValue ret_summary(UniValue::VOBJ);
 605 | +    if (index_name != "" && index_name != summary.name) {
 606 | +        return ret_summary;
 607 | +    }
    jonatack commented at 1:36 PM on July 31, 2020:

    nit: not sure if empty() is more idiomatic

    -    if (index_name != "" && index_name != summary.name) {
-        return ret_summary;
-    }
+    if (!index_name.empty() && index_name != summary.name) return ret_summary;
  54. jonatack commented at 1:40 PM on July 31, 2020: member

    ACK dc888cdb07 with a question for the help

  55. fjahr force-pushed on Aug 1, 2020
  56. fjahr commented at 1:34 PM on August 1, 2020: member

    Addressed review comments by @jonatack

  57. jonatack approved
  58. jonatack commented at 4:02 PM on August 1, 2020: member

    Code review re-ACK 47a5372 per git diff dc888cd 47a5372 :racehorse:

  59. DrahtBot added the label Needs rebase on Aug 14, 2020
  60. luke-jr referenced this in commit 1766a868e1 on Aug 15, 2020
  61. luke-jr referenced this in commit be0453e2b5 on Aug 15, 2020
  62. rpc: Add getindexinfo RPC 667bc7a7f7
  63. test: Add tests for getindexinfo RPC c447b09458
  64. doc: Add release notes for getindexinfo RPC 124e1ee134
  65. fjahr force-pushed on Aug 16, 2020
  66. jonatack commented at 10:26 AM on August 16, 2020: member

    Code review re-ACK 124e1ee per git range-diff a57af89 47a5372 124e1ee no change since my last re-ACK, rebase only

  67. DrahtBot removed the label Needs rebase on Aug 16, 2020
  68. laanwj commented at 1:57 PM on August 20, 2020: member

    Re-ACK 124e1ee1343f8bfb3748393ced9debdbdee60d3b

  69. laanwj merged this on Aug 20, 2020
  70. laanwj closed this on Aug 20, 2020
  71. sidhujag referenced this in commit defff93348 on Aug 20, 2020
  72. domob1812 referenced this in commit 2f2bfb323e on Aug 24, 2020
  73. deadalnix referenced this in commit 2a36ff1840 on Sep 15, 2021
  74. DrahtBot locked this on Feb 15, 2022
Contributors
fjahrpromagDrahtBotjonatackMarcoFalketheStacklaanwjjonasschnelli
Labels
UTXO Db and IndexesRPC/REST/ZMQ
Linked (view graph)
#14053 Add address-based index (attempt 4?)#18000 [WIP] Index for UTXO Set Statistics
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 18:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me