Skip to content

Conversation

@CryptoDJ
Copy link
Contributor

@CryptoDJ CryptoDJ commented Mar 4, 2016

Formats the network peer user agent text (or clean subversion) by removing the forward slash prefix and suffix. Example: /Satoshi:0.11.2/ --> Satoshi:0.11.2

bitcoin_useragent_before_better

The forward slashes in the User Agent text are occupying two spaces and seem unnecessary for the Qt wallet UI.

This code is forked from my original pull for Dark Silk: https://github.com/SCDeveloper/DarkSilk-Release-Candidate/pull/138

@CryptoDJ CryptoDJ changed the title Removes backslashes from user agent in peers UI. Removes backslashes from user agent in Peers tab UI. Mar 4, 2016
@CryptoDJ CryptoDJ changed the title Removes backslashes from user agent in Peers tab UI. Removes forwardslash from user agent in Peers tab UI. Mar 4, 2016
@CryptoDJ CryptoDJ changed the title Removes forwardslash from user agent in Peers tab UI. Removes forward slash (/) from user agent in peers tab UI. Mar 4, 2016
return strResult;
}

/// Formats the network peer user agent text (or subversion)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put the comment in the h file. And use the formatting according to the guideline (CONTRIBUTING.md)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will move my comments to the header file and squash the commits into one if the concept is ACK.

@maflcko
Copy link
Member

maflcko commented Mar 4, 2016

@CryptoDJ
Copy link
Contributor Author

CryptoDJ commented Mar 4, 2016

It looks like BIP0014 uses the forward slashes as delimiters or separator. One node doesn't have many user agents, so the delimiter is not needed in this case.

They should not be misused beyond what is specified in this section.

  • / separates the code-stack
  • : specifies the implementation version of the particular stack
  • ( and ) delimits a comment which optionally separates data using ;

Displaying a separator or delimiter in the UI is unnecessary and goes against Jakob Nielsen heuristics: https://www.nngroup.com/articles/ten-usability-heuristics/:

  • Aesthetic and minimalist design
    Dialogues should not contain information which is irrelevant or rarely needed. Every extra unit of information in a dialogue competes with the relevant units of information and diminishes their relative visibility.

@pstratem
Copy link
Contributor

pstratem commented Mar 5, 2016

It's a debug window... it's full of information which is irrelevant and rarely needed.

NACK on the concept

@CryptoDJ
Copy link
Contributor Author

CryptoDJ commented Mar 5, 2016

@pstratem, I don't think your NACK would be considered sound technical justification. The UI is displaying a delimiter for no reason.

@jonasschnelli
Copy link
Contributor

This also affects the RPC getpeerinfo output. So this would be a tiny API change. Not sure if we want this.

@laanwj laanwj added the GUI label Mar 6, 2016
@laanwj
Copy link
Member

laanwj commented Mar 6, 2016

I'm neutral on this change itself.
However, if you do this please add the filtering as a postprocessign step to the GUI code only, not to the core P2P code. RPC should stay the same.

On second thought: no, I don't like this. I like the debug window to report exactly the data that goes over the network. This may make the string prettier but makes it less useful too.

@maflcko
Copy link
Member

maflcko commented Mar 6, 2016

@CryptoDJ I think it's a matter of taste which looks better:

  • /btcwire:0.4.0/btcd:0.12.0/ or btcwire:0.4.0/btcd:0.12.0
  • /Satoshi:0.11.2/ljr:20151118/ or Satoshi:0.11.2/ljr:20151118
  • /Satoshi:0.12.0/Knots:20160225/ or Satoshi:0.12.0/Knots:20160225

As the current way is exactly the way how BIP 14 defines it, you can't really say it is "irrelevant or rarely needed". Personallly, I'd suggest we just stick with BIP 14 for simplicity and consistency.

@CryptoDJ
Copy link
Contributor Author

CryptoDJ commented Mar 6, 2016

The member variable is called cleanSubVer and it is put through a sanitize function. See the comment in net.h:

// strSubVer is whatever byte array we read from the wire. However, this field is intended
// to be printed out, displayed to humans in various forms and so on. So we sanitize it and
// store the sanitized version in cleanSubVer. The original should be used when dealing with
// the network or wire types and the cleaned string used when displayed or logged.

When consuming the API message, the / in cleanSubVer is kept and displayed to end users. Web sites display it b/c who want to further sanitize the string? Also, why have strSubVer and cleanSubVer?

@CryptoDJ
Copy link
Contributor Author

CryptoDJ commented Mar 6, 2016

https://bitnodes.21.co/nodes/
There are hundreds of superfluous forward slashes that got propagated because of a sanitation issue:
cleansubver

I think BIP 14 pertains to the wire format, NOT the UI.

@laanwj
Copy link
Member

laanwj commented Mar 7, 2016

I think BIP 14 pertains to the wire format, NOT the UI.

Correct.

However, the reason for the debug window is troubleshooting. For technical diagnostics, it is important to have accurate information. Granted, removing a slash is not a big deal, but we shouldn't make a habit out of presenting things differently than they are on the wire.

In any case: if the goal of this is to affect the UI only, this pull request should touch only UI code.

@pstratem
Copy link
Contributor

pstratem commented Mar 7, 2016

This is adding code which is for all intents doing nothing.

NACK on the simple principle that less code is better.

@CryptoDJ
Copy link
Contributor Author

My problem is with BIP 14 and I will pull request to change it. The user-agent string example begins and ends with delimiters (the forward slash or /) that have no use to humans or computers and therefore should be eliminated. Eliminating these superfluous delimiters will slightly decreased the data size transmitted to peers, stored when consuming API messages and make the UI look better.

So far, no one has been able to explain the benefit of the leading and trailing forwards slashes except that it is specified in BIP 14.

Edit: BIP 14 pull request: bitcoin/bips#352

CryptoDJ added a commit to CryptoDJ/bips that referenced this pull request Mar 11, 2016
The user-agent string example begins and ends with delimiters (the forward slash or /) that have no use to humans or computers and therefore should be eliminated. Eliminating these superfluous delimiters will slightly decreased the data size transmitted to peers, stored when consuming API messages and make the UI look better.

So far, no one has been able to explain the benefit of the leading and trailing forwards slashes except that it is specified in this BIP.  Please see this pull request:  bitcoin/bitcoin#7640
@laanwj
Copy link
Member

laanwj commented Mar 11, 2016

I'm closing this, there doesn't seem to be agreement to do this.

@laanwj laanwj closed this Mar 11, 2016
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants