rpc: Human readable network services #16787

pull darosior wants to merge 2 commits into bitcoin:master from darosior:services_for_humans changing 4 files +39 −2
  1. darosior commented at 10:11 am on September 2, 2019: member

    This is a reopen of #15511 (comment) since there have been concept ACKs from sdaftuar and Sjors.

    This adds a new entry to getpeerinfo and getnetworkinfo which decodes the network services flags.

    Here is a truncated output of getpeerinfo:

    0"services": "000000000000040d",
1"servicesnames": "NETWORK | BLOOM | WITNESS | NETWORK_LIMITED",
2"relaytxes": true,

    And one of getnetworkinfo:

    0"localservices": "0000000000000409",
1"localservicesnames": "NETWORK | WITNESS | NETWORK_LIMITED",
2"localrelay": true,

    Fixes #16780.

  2. darosior force-pushed on Sep 2, 2019
  3. fanquake added the label RPC/REST/ZMQ on Sep 2, 2019
  4. fanquake renamed this:
    Human readable network services
    rpc: Human readable network services
    on Sep 2, 2019
  5. laanwj commented at 11:58 am on September 2, 2019: member

    Concept ACK, though to avoid clients having to do additional string parsing, i’d slightly prefer a list instead of one |-separated string. E.g.

    0"localservicesnames": ["NODE_NETWORK", "NODE_WITNESS", "NODE_NETWORK_LIMITED"]

    or leave out the NODE_ as it’s redundant,

    0"localservicesnames": ["NETWORK", "WITNESS", "NETWORK_LIMITED"]
  6. sipa commented at 4:15 pm on September 2, 2019: member
    I prefer @laanwj’s version as well.
  7. darosior force-pushed on Sep 2, 2019
  8. darosior commented at 7:39 pm on September 2, 2019: member
    Updated to use a list instead of a | -separated string and to remove the redundant NODE_ prefix. Will add CHANGELOG too.
  9. fanquake commented at 1:51 am on September 3, 2019: member

    Concept ACK - looks like multiple developers have said that this would be a worthwhile inclusion. Thanks for writing release notes as well.

    master (33f9750b1b86a705d092b0e1314ed15287c45239):

    0    "services": "000000000000040d",
1    "relaytxes": true,

    This PR (0774c03056a1722fe15b8b0926bf040c8932c7c3):

     0src/bitcoin-cli getpeerinfo | grep -i 'services' -A 7
 1    "services": "000000000000040d",
 2    "servicesnames": [
 3      "NETWORK",
 4      "BLOOM",
 5      "WITNESS",
 6      "NETWORK_LIMITED"
 7    ],
 8    "relaytxes": true,
 9--
10    "services": "000000000000000d",
11    "servicesnames": [
12      "NETWORK",
13      "BLOOM",
14      "WITNESS"
15    ],
16    "relaytxes": true,
17    "lastsend": 1567474969,
18--
19    "services": "000000000000040d",
20    "servicesnames": [
21      "NETWORK",
22      "BLOOM",
23      "WITNESS",
24      "NETWORK_LIMITED"
25    ],
26    "relaytxes": true,
  10. fanquake requested review from sdaftuar on Sep 3, 2019
  11. fanquake requested review from Sjors on Sep 3, 2019
  12. practicalswift commented at 12:02 pm on September 3, 2019: contributor
    Concept ACK – nice usability improvement!
  13. in src/rpc/net.cpp:85 in 0774c03056 outdated 
    81@@ -82,6 +82,10 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
82             "    \"addrbind\":\"ip:port\",    (string) Bind address of the connection to the peer\n"
83             "    \"addrlocal\":\"ip:port\",   (string) Local address as reported by the peer\n"
84             "    \"services\":\"xxxxxxxxxxxxxxxx\",   (string) The services offered\n"
85+            "    \"servicesnames\":[              (array) the services offered, in human-readable form\n"
    MarcoFalke commented at 12:59 pm on September 3, 2019:
    Should mention that unrecognised services are not shown
    darosior commented at 1:36 pm on September 3, 2019:
    Fixed
  14. in src/rpc/net.cpp:165 in 0774c03056 outdated 
    160+            servicesNames.push_back("BLOOM");
161+        if (stats.nServices & NODE_WITNESS)
162+            servicesNames.push_back("WITNESS");
163+        if (stats.nServices & NODE_NETWORK_LIMITED)
164+            servicesNames.push_back("NETWORK_LIMITED");
165+        obj.pushKV("servicesnames", servicesNames);
    MarcoFalke commented at 1:01 pm on September 3, 2019:

    Couldn’t this be in a function, so that only one place has to be updated in the future?

    0        obj.pushKV("servicesnames", GetServicesNames(stats.nServices));
    darosior commented at 1:36 pm on September 3, 2019:
    Fixed
  15. MarcoFalke commented at 1:01 pm on September 3, 2019: member
    a documentation nit
  16. darosior force-pushed on Sep 3, 2019
  17. darosior commented at 1:38 pm on September 3, 2019: member
    Rebased to point in the help that only known services are decoded, and to group the decoding into one function as suggested by @MarcoFalke
  18. DrahtBot commented at 1:57 pm on September 3, 2019: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16839 (Replace Connman and BanMan globals with Node local by ryanofsky)
    • #16411 (Signet support by kallewoof)

    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.

  19. laanwj commented at 8:08 pm on September 3, 2019: member
    Code review ACK 1e29f044dd3580f53d44b8293893c287100aa48d
  20. in src/rpc/net.cpp:454 in 1e29f044dd outdated 
    450@@ -446,6 +451,10 @@ static UniValue getnetworkinfo(const JSONRPCRequest& request)
451             "  \"subversion\": \"/Satoshi:x.x.x/\",     (string) the server subversion string\n"
452             "  \"protocolversion\": xxxxx,              (numeric) the protocol version\n"
453             "  \"localservices\": \"xxxxxxxxxxxxxxxx\", (string) the services we offer to the network\n"
454+            "  \"localservicesnames\": [              (array) the known services we offer to the network, in human-readable form\n"
    MarcoFalke commented at 11:36 pm on September 3, 2019:
    unknown services can not be offered, so mentioning it here will cause confusion
  21. in src/rpc/net.cpp:496 in 1e29f044dd outdated 
    492@@ -484,8 +493,11 @@ static UniValue getnetworkinfo(const JSONRPCRequest& request)
493     obj.pushKV("version",       CLIENT_VERSION);
494     obj.pushKV("subversion",    strSubVersion);
495     obj.pushKV("protocolversion",PROTOCOL_VERSION);
496-    if(g_connman)
497-        obj.pushKV("localservices", strprintf("%016x", g_connman->GetLocalServices()));
498+    if(g_connman) {
    MarcoFalke commented at 11:37 pm on September 3, 2019: 
    0    if (g_connman) {
  22. darosior force-pushed on Sep 4, 2019
  23. darosior commented at 2:58 pm on September 4, 2019: member
    (Travis is failing for mysterious reasons but the build is passing on https://bitcoinbuilds.org/?build=1484 fwiw :-) )
  24. in doc/release-notes-16787.md:3 in 10adfa5a76 outdated 
    0@@ -0,0 +1,3 @@
1+RPC changes
2+-----------
3+The `getnetworkinfo` and `getpeerinfo` commands now contains a new field with decoded network service flags.
    mzumsande commented at 11:03 pm on September 4, 2019:
    nit: contain
  25. in src/rpc/net.cpp:455 in 2443d32650 outdated 
    450@@ -446,6 +451,10 @@ static UniValue getnetworkinfo(const JSONRPCRequest& request)
451             "  \"subversion\": \"/Satoshi:x.x.x/\",     (string) the server subversion string\n"
452             "  \"protocolversion\": xxxxx,              (numeric) the protocol version\n"
453             "  \"localservices\": \"xxxxxxxxxxxxxxxx\", (string) the services we offer to the network\n"
454+            "  \"localservicesnames\": [              (array) the services we offer to the network, in human-readable form\n"
455+            "      \"SERVICE_NAME\",         (string) the service name if it is recognised\n"
    mzumsande commented at 11:07 pm on September 4, 2019:
    nit: could put some more whitespace before “(string)” for optics
    MarcoFalke commented at 11:50 pm on September 4, 2019:

    If you need to change this for other reasons, might as well remove the “if it is recognised”. If we offer a service, we better recognize it.

    0            "      \"SERVICE_NAME\",         (string) the service name\n"
    darosior commented at 11:16 am on September 5, 2019:
    Ah, I copied it from getpeerinfo …
    darosior commented at 1:12 pm on September 5, 2019:
    fixed
    darosior commented at 1:13 pm on September 5, 2019:
    Fixed
  26. mzumsande commented at 11:33 pm on September 4, 2019: member
    ACK 10adfa5a769b0f47858bc77f2641c691e9b80c52. Reviewed code, ran tests and tried it out on mainnet - works as expected. Feel free to ignore nits if you don’t change something else.
  27. laanwj commented at 11:42 am on September 5, 2019: member

    (Travis is failing for mysterious reasons but the build is passing on https://bitcoinbuilds.org/?build=1484 fwiw :-) )

    yeahh Travis is having infrastructure issues again (I think), restarted the failing jobs …

  28. rpc/net: decode the services flags in a new entry 6564f58c87
  29. doc: add a release note for the new field in 'getpeerinfo' and 'getnetworkinfo' 66740f460a
  30. darosior force-pushed on Sep 5, 2019
  31. darosior commented at 1:26 pm on September 5, 2019: member
    Rebased to fix the last doc nits (and hopefully make Travis happy).
  32. MarcoFalke commented at 5:48 pm on September 5, 2019: member
    unsigned ACK 66740f460af5f9d8c61eb5b154863bffb20d94b5
  33. in src/rpc/util.cpp:737 in 66740f460a 
    732@@ -733,3 +733,21 @@ std::vector<CScript> EvalDescriptorStringOrObject(const UniValue& scanobject, Fl
733     }
734     return ret;
735 }
736+
737+UniValue GetServicesNames(ServiceFlags services)
    promag commented at 11:23 pm on September 5, 2019:
    Should also #include <protocol.h>
    darosior commented at 8:07 am on September 6, 2019:
    It is included. https://github.com/bitcoin/bitcoin/pull/16787/files
  34. in src/rpc/util.cpp:739 in 66740f460a 
    732@@ -733,3 +733,21 @@ std::vector<CScript> EvalDescriptorStringOrObject(const UniValue& scanobject, Fl
733     }
734     return ret;
735 }
736+
737+UniValue GetServicesNames(ServiceFlags services)
738+{
739+    UniValue servicesNames(UniValue::VARR);
    promag commented at 11:24 pm on September 5, 2019:
    nit, s/servicesNames/names or s/servicesNames/res.
  35. in src/rpc/util.cpp:742 in 66740f460a 
    737+UniValue GetServicesNames(ServiceFlags services)
738+{
739+    UniValue servicesNames(UniValue::VARR);
740+
741+    if (services & NODE_NETWORK)
742+        servicesNames.push_back("NETWORK");
    promag commented at 11:26 pm on September 5, 2019:
    nit, same line as if?
  36. in src/rpc/net.cpp:455 in 66740f460a 
    450@@ -446,6 +451,10 @@ static UniValue getnetworkinfo(const JSONRPCRequest& request)
451             "  \"subversion\": \"/Satoshi:x.x.x/\",     (string) the server subversion string\n"
452             "  \"protocolversion\": xxxxx,              (numeric) the protocol version\n"
453             "  \"localservices\": \"xxxxxxxxxxxxxxxx\", (string) the services we offer to the network\n"
454+            "  \"localservicesnames\": [                (array) the services we offer to the network, in human-readable form\n"
455+            "      \"SERVICE_NAME\",                    (string) the service name\n"
    promag commented at 11:27 pm on September 5, 2019:
    if it is recognised?
    darosior commented at 8:08 am on September 6, 2019:
    As mentioned above, getnetworkinfo indicates service we offer : we’d better recognise it
  37. promag commented at 11:28 pm on September 5, 2019: member
    Should have tests for these new fields?
  38. laanwj commented at 7:02 am on September 10, 2019: member
    Works for me ACK 66740f460af5f9d8c61eb5b154863bffb20d94b5 Please add a (functional) tests for this feature in a next PR
  39. laanwj referenced this in commit 33c466a642 on Sep 10, 2019
  40. laanwj merged this on Sep 10, 2019
  41. laanwj closed this on Sep 10, 2019
  42. darosior deleted the branch on Sep 10, 2019
  43. darosior commented at 7:13 pm on September 10, 2019: member

    Please add a (functional) tests for this feature in a next PR

    Will do

  44. laanwj referenced this in commit 04d9939f46 on Sep 12, 2019
  45. luke-jr referenced this in commit 8971be7dc8 on Sep 21, 2019
  46. sickpig referenced this in commit 922cab0e99 on Jan 20, 2020
  47. sickpig referenced this in commit 91d9bf998d on Jan 22, 2020
  48. sickpig referenced this in commit 716432bba5 on Jan 31, 2020
  49. sickpig referenced this in commit ff34445b4f on Feb 6, 2020
  50. sickpig referenced this in commit 38ade11f9a on Feb 7, 2020
  51. sickpig referenced this in commit c787d89dec on Feb 10, 2020
  52. sickpig referenced this in commit e7f15db5fc on Feb 11, 2020
  53. gandrewstone referenced this in commit 6403cd77c4 on Feb 12, 2020
  54. jasonbcox referenced this in commit e5b0e658a2 on Oct 16, 2020
  55. kittywhiskers referenced this in commit 1dbf0257e7 on Aug 22, 2021
  56. kittywhiskers referenced this in commit 3c2b35368d on Aug 22, 2021
  57. kittywhiskers referenced this in commit c3552eb57e on Sep 16, 2021
  58. kittywhiskers referenced this in commit 58340cef63 on Sep 18, 2021
  59. UdjinM6 referenced this in commit 4ffd42de63 on Sep 21, 2021
  60. thelazier referenced this in commit 948a9e12a1 on Sep 25, 2021
  61. kittywhiskers referenced this in commit 7c0326161a on Oct 12, 2021
  62. DrahtBot locked this on Dec 16, 2021


darosior laanwj sipa fanquake practicalswift MarcoFalke DrahtBot mzumsande promag


Sjors sdaftuar

Labels
RPC/REST/ZMQ

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: 2024-07-06 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me