Skip to content

Conversation

@darosior
Copy link
Member

@darosior darosior commented Mar 1, 2019

The commands getpeerinfo and getnetworkinfo returns the services a node provides in the form of a flag (in the field services). This PR adds a field servicesnames (resp. localservicesnames) which contains the names of these services so that a user does not have to parse the flag to know which services his node provides.
EDIT: see #15511 (comment)

@gmaxwell
Copy link
Contributor

gmaxwell commented Mar 1, 2019

What's the motivation for this? Generally we've regarded the flags as rather esoteric troubleshooting stuff. The example output doesn't make it clear how unknown values would be handled.

@darosior
Copy link
Member Author

darosior commented Mar 1, 2019

The motivation is that it is more readable for an user which services the node provides, thus making it more convenient to use : no need to parse the flags himself (which needs to read the code to check what is the meaning of each flag).
Unknown values just won't pass the conditions and would not be returned, is it an issue ?

@kristapsk
Copy link
Contributor

NACK, there could be some software that expects output of getnetworkinfo RPC as it is now. This will break compatibility. User-friendly services field could be added, but then as a new field, not replacing existing one.

@darosior
Copy link
Member Author

darosior commented Mar 1, 2019

Indeed.

@darosior darosior force-pushed the services_for_humans branch from 59ca4df to c6ec631 Compare March 1, 2019 16:05
@darosior darosior force-pushed the services_for_humans branch from c6ec631 to 309102f Compare March 1, 2019 16:06
@darosior
Copy link
Member Author

darosior commented Mar 1, 2019

Changed the behavior to add a new field instead of modifying the existent one. Edited the description.

@gmaxwell
Copy link
Contributor

gmaxwell commented Mar 1, 2019

I'm sorry, I don't see how "more readable" is actually helpful in and of itself. Why does anyone need to be reading these things?

Unknown values just won't pass the conditions and would not be returned, is it an issue ?

Yes, field is a lot more useful for unknown values-- e.g. reasoning about new or incompatible software-- than it is for known ones.

@darosior
Copy link
Member Author

darosior commented Mar 2, 2019

I'm sorry, I don't see how "more readable" is actually helpful in and of itself. Why does anyone need to be reading these things?

Actually, I made this after helping the guy asking about NODE_GETUTXO on the thread your responded to on bitcointalk. I thought that other node owners would wonder which services their node supports.
I understand that I may be wrong and this may just be useless, I close it for now.

@darosior darosior closed this Mar 2, 2019
@sdaftuar
Copy link
Member

sdaftuar commented Mar 2, 2019

For what it's worth, I would concept ACK something like this -- the times I use getpeerinfo tend to be times I'm debugging something, and having the service bits be more easy to decode would be helpful to me.

@Sjors
Copy link
Member

Sjors commented Sep 1, 2019

Concept ACK. I didn't know about this PR when I opened #16780. Don't override the existing field though. You could make the output more compact by presenting it as a string: NODE_NETWORK | NODE_BLOOM.

If people still find that too much visual clutter, especially in the already quite long output of getpeerinfo, you could add a debug param. If you go that route, then I suggest returning a dictionary with all possible flags and a true / false bool.

@darosior
Copy link
Member Author

darosior commented Sep 2, 2019

Reopening since @sdaftuar and @Sjors concept ACKed.
Rebased and modified, as proposed in #15511 (comment), the array to a string (this introduces some temporary variables but the output is much nicer) : since it only adds one line I think it doesnt worth a command argument addition.

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:

"protocolversion": 70015,
"localservices": "0000000000000409",
"localservicesnames": "NODE_NETWORK | NODE_WITNESS | NODE_NETWORK_LIMITED",
"localrelay": true,

@darosior
Copy link
Member Author

darosior commented Sep 2, 2019

Reopened in a new PR (#16787) since I force pushed before clicking reopen....

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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants