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
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
32 | @@ -33,6 +33,17 @@ string SanitizeString(const string& str, int rule) 33 | return strResult; 34 | } 35 | 36 | +/// Formats the network peer user agent text (or subversion)
Please put the comment in the h file. And use the formatting according to the guideline (CONTRIBUTING.md)
I will move my comments to the header file and squash the commits into one if the concept is ACK.
37 | +/// by removing the begining and ending charactors(/). 38 | +/// example: /Satoshi:0.11.2/ --> Satoshi:0.11.2 39 | +string SanitizeSubVersionString(const string& str) 40 | +{ 41 | + string strResult = SanitizeString(str); 42 | + if ((strResult.length() > 3) && (strResult.substr(0,1) == "/") && (strResult.substr((strResult.length()-1),1) == "/"))
Why is the length at least four?
It should be two and I can change, then squash my commits. Thanks!
You can simplify the condition as (strResult.front() == '/' && strResult.back() == '/').
I like making the GUI cleaner but isn't the slash part of the string? https://github.com/bitcoin/bips/blob/master/bip-0014.mediawiki#proposal
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.
It's a debug window... it's full of information which is irrelevant and rarely needed.
NACK on the concept
This also affects the RPC getpeerinfo output. So this would be a tiny API change. Not sure if we want this.
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.
@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:20160225As 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.
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?
https://bitnodes.21.co/nodes/
There are hundreds of superfluous forward slashes that got propagated because of a sanitation issue:
I think BIP 14 pertains to the wire format, NOT the UI.
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.
This is adding code which is for all intents doing nothing.
NACK on the simple principle that less code is better.
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: https://github.com/bitcoin/bips/pull/352
I'm closing this, there doesn't seem to be agreement to do this.