Skip to content
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

rpc: Human readable network services #16787

Merged
merged 2 commits into from Sep 10, 2019

Conversation

@darosior
Copy link
Contributor

commented Sep 2, 2019

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:

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

And one of getnetworkinfo:

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

Fixes #16780.

@darosior darosior force-pushed the darosior:services_for_humans branch from 42fd0ca to 6e47bf2 Sep 2, 2019
@fanquake fanquake changed the title Human readable network services rpc: Human readable network services Sep 2, 2019
@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

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.

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

or leave out the NODE_ as it's redundant,

"localservicesnames": ["NETWORK", "WITNESS", "NETWORK_LIMITED"]
@sipa

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

I prefer @laanwj's version as well.

@darosior darosior force-pushed the darosior:services_for_humans branch from 6e47bf2 to fcbbb7e Sep 2, 2019
@darosior

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

Updated to use a list instead of a -separated string and to remove the redundant NODE_ prefix. Will add CHANGELOG too.

Copy link
Member

left a comment

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

master (33f9750):

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

This PR (0774c03):

src/bitcoin-cli getpeerinfo | grep -i 'services' -A 7
    "services": "000000000000040d",
    "servicesnames": [
      "NETWORK",
      "BLOOM",
      "WITNESS",
      "NETWORK_LIMITED"
    ],
    "relaytxes": true,
--
    "services": "000000000000000d",
    "servicesnames": [
      "NETWORK",
      "BLOOM",
      "WITNESS"
    ],
    "relaytxes": true,
    "lastsend": 1567474969,
--
    "services": "000000000000040d",
    "servicesnames": [
      "NETWORK",
      "BLOOM",
      "WITNESS",
      "NETWORK_LIMITED"
    ],
    "relaytxes": true,
@fanquake fanquake requested review from sdaftuar and Sjors Sep 3, 2019
@practicalswift

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Concept ACK -- nice usability improvement!

Copy link
Member

left a comment

a documentation nit

src/rpc/net.cpp Show resolved Hide resolved
src/rpc/net.cpp Outdated Show resolved Hide resolved
@darosior darosior force-pushed the darosior:services_for_humans branch from 0774c03 to 1e29f04 Sep 3, 2019
@darosior

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

Rebased to point in the help that only known services are decoded, and to group the decoding into one function as suggested by @MarcoFalke

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

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.

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Code review ACK 1e29f04

src/rpc/net.cpp Outdated Show resolved Hide resolved
src/rpc/net.cpp Outdated Show resolved Hide resolved
@darosior darosior force-pushed the darosior:services_for_humans branch from 1e29f04 to 10adfa5 Sep 4, 2019
@darosior

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

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

Copy link
Contributor

left a comment

ACK 10adfa5. 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.

doc/release-notes-16787.md Outdated Show resolved Hide resolved
src/rpc/net.cpp Outdated Show resolved Hide resolved
src/rpc/net.cpp Outdated Show resolved Hide resolved
@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

(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 …

darosior added 2 commits Mar 1, 2019
…tworkinfo'
@darosior darosior force-pushed the darosior:services_for_humans branch from 10adfa5 to 66740f4 Sep 5, 2019
@darosior

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

Rebased to fix the last doc nits (and hopefully make Travis happy).

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

unsigned ACK 66740f4

Copy link
Member

left a comment

Should have tests for these new fields?

src/rpc/util.cpp Show resolved Hide resolved
src/rpc/util.cpp Show resolved Hide resolved
src/rpc/util.cpp Show resolved Hide resolved
src/rpc/net.cpp Show resolved Hide resolved
@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Works for me
ACK 66740f4
Please add a (functional) tests for this feature in a next PR

laanwj added a commit that referenced this pull request Sep 10, 2019
66740f4 doc: add a release note for the new field in 'getpeerinfo' and 'getnetworkinfo' (darosior)
6564f58 rpc/net: decode the services flags in a new entry (darosior)

Pull request description:

  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`:
  ```
  "services": "000000000000040d",
  "servicesnames": "NODE_NETWORK | NODE_BLOOM | NODE_WITNESS | NODE_NETWORK_LIMITED",
  "relaytxes": true,
  ```
  And one of `getnetworkinfo`:
  ```
  "localservices": "0000000000000409",
  "localservicesnames": "NODE_NETWORK | NODE_WITNESS | NODE_NETWORK_LIMITED",
  "localrelay": true,
  ```

  Fixes #16780.

ACKs for top commit:
  MarcoFalke:
    unsigned ACK 66740f4
  laanwj:
    ACK 66740f4

Tree-SHA512: 0acc37134b283f56004a41243903d7790cb01591ddf0342489bd05f3a2c780563075373ba5fd55180fa15632e8968ffa11a979b8afece75a6a2e891342601440
@laanwj laanwj merged commit 66740f4 into bitcoin:master Sep 10, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@darosior darosior deleted the darosior:services_for_humans branch Sep 10, 2019
@darosior

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

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

Will do

laanwj added a commit that referenced this pull request Sep 12, 2019
…tworkinfo`

1d524c6 tests: rename 'test_getnetworkinginfo' in 'test_getnetworkinfo' (darosior)
07a8f65 tests: add a test for the 'servicesnames' RPC field (darosior)

Pull request description:

  As per #16787 (comment), fixes #16844.

  This adds a test for both commands in the first commit and renames the test for `getnetworkinfo` in the second commit.

ACKs for top commit:
  laanwj:
    ACK 1d524c6

Tree-SHA512: 8267dce4d54356debab75014e6f9ba885b892da605ed32f26a5446c232992fcae761861bb678adbdb942815d4706f3768c70deee6afec68f219b23605475be01
@tynes tynes referenced this pull request Sep 20, 2019
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 21, 2019
@tynes tynes referenced this pull request Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.