implement uacomment config parameter which can add comments to user agent as per BIP-0014 #6462

Merged
merged 2 commits into from Aug 5, 2015

Conversation

Projects
None yet
8 participants
@prusnak
Contributor

prusnak commented Jul 22, 2015

User can add the following into theirs bitcoin.conf:

uacomment=Prague
uacomment=BeagleBone

which will result in the following user agent:

/Satoshi:0.11.99(Prague; BeagleBone)/
@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jul 23, 2015

Contributor

Concept ACK. Needs some sort of size limiting.

Contributor

jgarzik commented Jul 23, 2015

Concept ACK. Needs some sort of size limiting.

@laanwj

View changes

src/clientversion.cpp
@@ -99,11 +100,20 @@ std::string FormatSubVersion(const std::string& name, int nClientVersion, const
std::ostringstream ss;
ss << "/";
ss << name << ":" << FormatVersion(nClientVersion);
- if (!comments.empty())
+
+ const std::string key = "-uacomment";

This comment has been minimized.

@laanwj

laanwj Jul 24, 2015

Member

Right now, FormatSubVersion is a pure utility function that doesn't use any global state. I think it should stay that way. E.g. by adding an const std::vector<std::string> &commentList argument. Then pass in this data at the caller side.

Also makes it easier to test this in util_tests.

@laanwj

laanwj Jul 24, 2015

Member

Right now, FormatSubVersion is a pure utility function that doesn't use any global state. I think it should stay that way. E.g. by adding an const std::vector<std::string> &commentList argument. Then pass in this data at the caller side.

Also makes it easier to test this in util_tests.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 24, 2015

Member

Concept ACK.

Member

laanwj commented Jul 24, 2015

Concept ACK.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 24, 2015

Member

Concept ACK, agree with @laanwj.

Member

sipa commented Jul 24, 2015

Concept ACK, agree with @laanwj.

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak Jul 27, 2015

Contributor

Updated commit to reflect @laanwj request (to not have FormatSubVersion function dependent on global object).

(Actually, this was my original code, but I changed it, because I did not want to duplicate the logic in two files).

Contributor

prusnak commented Jul 27, 2015

Updated commit to reflect @laanwj request (to not have FormatSubVersion function dependent on global object).

(Actually, this was my original code, but I changed it, because I did not want to duplicate the logic in two files).

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak Jul 29, 2015

Contributor

@jgarzik What is the maximum desired length of user agent string? Or should we just limit the number of comments and their length (e.g. max 5 comments of max 40 characters)?

Contributor

prusnak commented Jul 29, 2015

@jgarzik What is the maximum desired length of user agent string? Or should we just limit the number of comments and their length (e.g. max 5 comments of max 40 characters)?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 29, 2015

Member

Bitcoin Core will reject strings longer than 256 characters for the subser string in "version" messages.

Member

sipa commented Jul 29, 2015

Bitcoin Core will reject strings longer than 256 characters for the subser string in "version" messages.

@laanwj laanwj added the P2P label Jul 31, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 31, 2015

Member

Tested ACK. Named a few nodes.

  "subversion": "/Satoshi:0.11.99(Inanna)/",
...
  "subversion": "/Satoshi:0.11.99(Ishtar)/",
...
  "subversion": "/Satoshi:0.11.99(Ereshkigal)/",

Names are returned both by getnetworkinfo and the version P2P message, which I verified with bitcoin-submittx:

recv msg_version(nVersion=70002 nServices=1 nTime=Fri Jul 31 15:41:00 2015 addrTo=CAddress(nTime=0 nServices=1 ip=0.0.0.0 port=0) addrFrom=CAddress(nTime=0 nServices=1 ip=0.0.0.0 port=18444) nNonce=0x95FDC41E438E8122 strSubVer=/Satoshi:0.11.99(Ishtar)/ nStartingHeight=0)

It will happily send a subversion longer than 256 bytes, though:

recv msg_version(nVersion=70002 nServices=1 nTime=Fri Jul 31 15:53:46 2015 addrTo=CAddress(nTime=0 nServices=1 ip=0.0.0.0 port=0) addrFrom=CAddress(nTime=0 nServices=1 ip=0.0.0.0 port=18333) nNonce=0x8782C2C672C750F3 strSubVer=/Satoshi:0.11.99(Ishtar; 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef; 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef; 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef; 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef)/ nStartingHeight=5178)

... which, as expected, gets me only connections to old versions. The result is quite interesting. Some nodes send me a REJECT message, which, as it arrives before full version negotiation is seen as misbehavior:

2015-07-31 13:52:46 Misbehaving: 52.1.41.179:18333 (0 -> 1)
2015-07-31 13:52:46 ProcessMessages(reject, 31 bytes) FAILED peer=1

Although not an easy to trigger issue, we should avoid sending a string longer than we accept ourselves.

I share your concern about duplicating code: but you could still define a function that calls FormatSubVersion(CLIENT_NAME, CLIENT_VERSION, mapMultiArgs.count("-uacomment") ? mapMultiArgs["-uacomment"] : std::vector<string>())));

Member

laanwj commented Jul 31, 2015

Tested ACK. Named a few nodes.

  "subversion": "/Satoshi:0.11.99(Inanna)/",
...
  "subversion": "/Satoshi:0.11.99(Ishtar)/",
...
  "subversion": "/Satoshi:0.11.99(Ereshkigal)/",

Names are returned both by getnetworkinfo and the version P2P message, which I verified with bitcoin-submittx:

recv msg_version(nVersion=70002 nServices=1 nTime=Fri Jul 31 15:41:00 2015 addrTo=CAddress(nTime=0 nServices=1 ip=0.0.0.0 port=0) addrFrom=CAddress(nTime=0 nServices=1 ip=0.0.0.0 port=18444) nNonce=0x95FDC41E438E8122 strSubVer=/Satoshi:0.11.99(Ishtar)/ nStartingHeight=0)

It will happily send a subversion longer than 256 bytes, though:

recv msg_version(nVersion=70002 nServices=1 nTime=Fri Jul 31 15:53:46 2015 addrTo=CAddress(nTime=0 nServices=1 ip=0.0.0.0 port=0) addrFrom=CAddress(nTime=0 nServices=1 ip=0.0.0.0 port=18333) nNonce=0x8782C2C672C750F3 strSubVer=/Satoshi:0.11.99(Ishtar; 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef; 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef; 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef; 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef)/ nStartingHeight=5178)

... which, as expected, gets me only connections to old versions. The result is quite interesting. Some nodes send me a REJECT message, which, as it arrives before full version negotiation is seen as misbehavior:

2015-07-31 13:52:46 Misbehaving: 52.1.41.179:18333 (0 -> 1)
2015-07-31 13:52:46 ProcessMessages(reject, 31 bytes) FAILED peer=1

Although not an easy to trigger issue, we should avoid sending a string longer than we accept ourselves.

I share your concern about duplicating code: but you could still define a function that calls FormatSubVersion(CLIENT_NAME, CLIENT_VERSION, mapMultiArgs.count("-uacomment") ? mapMultiArgs["-uacomment"] : std::vector<string>())));

implement uacomment config parameter
which can add comments to user agent as per BIP-0014
@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak Jul 31, 2015

Contributor

Easiest logic would be to discard all of the comments if strlen > 256.

Contributor

prusnak commented Jul 31, 2015

Easiest logic would be to discard all of the comments if strlen > 256.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 31, 2015

Member

It's a workable solution but failing silently is unexpected behavior. I'd prefer to check this in AppInit2 and fail if the subversion string is too long.

Member

laanwj commented Jul 31, 2015

It's a workable solution but failing silently is unexpected behavior. I'd prefer to check this in AppInit2 and fail if the subversion string is too long.

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak Jul 31, 2015

Contributor

Added check to AppInit2 function. (No more than 4 comments and no longer than 40 characters allowed).

Contributor

prusnak commented Jul 31, 2015

Added check to AppInit2 function. (No more than 4 comments and no longer than 40 characters allowed).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Aug 1, 2015

Member

utACK.

Nit: now you can provide 4string x 40bytes. What if one likes to provide 1x50 bytes?
Would it not make more sense to limit the combined string to 240 (or 255) bytes?

Member

jonasschnelli commented Aug 1, 2015

utACK.

Nit: now you can provide 4string x 40bytes. What if one likes to provide 1x50 bytes?
Would it not make more sense to limit the combined string to 240 (or 255) bytes?

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak Aug 1, 2015

Contributor

Reworked check to check total length of all comments altogether. If it is more than 200 characters the error is issued.

Contributor

prusnak commented Aug 1, 2015

Reworked check to check total length of all comments altogether. If it is more than 200 characters the error is issued.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Aug 1, 2015

Contributor

ut ACK

Contributor

jgarzik commented Aug 1, 2015

ut ACK

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 1, 2015

Member

utACK

Member

sipa commented Aug 1, 2015

utACK

limit total length of user agent comments
Reworked-By: Wladimir J. van der Laan <laanwj@gmail.com>
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 5, 2015

Member

@prusnak I slightly reworked the last commit:

  • Add a constant MAX_SUBVERSION_LENGTH in net.h and use where appropriate
  • Compute strSubVersion only once, during initialization, and check the size immediately
  • Use stored strSubVersion in PushVersion and GetNetworkInfo
  • Remove translation for the error.

This reduces the amount of code, and cuts down on magic values.
https://github.com/laanwj/bitcoin/tree/2015_08_prusnak_uacomment
Can you please take it over?

Member

laanwj commented Aug 5, 2015

@prusnak I slightly reworked the last commit:

  • Add a constant MAX_SUBVERSION_LENGTH in net.h and use where appropriate
  • Compute strSubVersion only once, during initialization, and check the size immediately
  • Use stored strSubVersion in PushVersion and GetNetworkInfo
  • Remove translation for the error.

This reduces the amount of code, and cuts down on magic values.
https://github.com/laanwj/bitcoin/tree/2015_08_prusnak_uacomment
Can you please take it over?

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak Aug 5, 2015

Contributor

@laanwj Updated PR to your code.

Contributor

prusnak commented Aug 5, 2015

@laanwj Updated PR to your code.

@laanwj laanwj merged commit 7b79cbd into bitcoin:master Aug 5, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 5, 2015

Merge pull request #6462
7b79cbd limit total length of user agent comments (Pavol Rusnak)
557f8ea implement uacomment config parameter which can add comments to user agent as per BIP-0014 (Pavol Rusnak)
@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak Aug 5, 2015

Contributor

Thanks!

Contributor

prusnak commented Aug 5, 2015

Thanks!

@ABISprotocol

This comment has been minimized.

Show comment
Hide comment
@ABISprotocol

ABISprotocol Aug 6, 2015

I can't help but wonder if this conflicts somehow with #253 and the effort there which seemed to be to not identify or associate users... what might be the long-term effects of this effort which is to associate a bitcoin address with a node? No need to answer me here.

I can't help but wonder if this conflicts somehow with #253 and the effort there which seemed to be to not identify or associate users... what might be the long-term effects of this effort which is to associate a bitcoin address with a node? No need to answer me here.

+ // format user agent, check total size
+ strSubVersion = FormatSubVersion(CLIENT_NAME, CLIENT_VERSION, mapMultiArgs.count("-uacomment") ? mapMultiArgs["-uacomment"] : std::vector<string>());
+ if (strSubVersion.size() > MAX_SUBVERSION_LENGTH) {
+ return InitError(strprintf("Total length of network version string %i exceeds maximum of %i characters. Reduce the number and/or size of uacomments.",

This comment has been minimized.

@Diapolo

Diapolo Aug 6, 2015

Shouldn't that one be translatable?

@Diapolo

Diapolo Aug 6, 2015

Shouldn't that one be translatable?

This comment has been minimized.

@laanwj

laanwj Aug 6, 2015

Member

No, I'd say. Apart from being a specific technical error (which we don't translate for Googlability, see translation_strings_policy.md) it's an extremely rare condition caused by almost-abuse of the configuration system. No need to waste translator time on that.

@laanwj

laanwj Aug 6, 2015

Member

No, I'd say. Apart from being a specific technical error (which we don't translate for Googlability, see translation_strings_policy.md) it's an extremely rare condition caused by almost-abuse of the configuration system. No need to waste translator time on that.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 6, 2015

Member

@ABISprotocol How is having this option a problem? It's not always a good idea to use it, but many people run essentially public nodes without wallet, and for those there is no need to 'hide in the crowd'.

Member

laanwj commented Aug 6, 2015

@ABISprotocol How is having this option a problem? It's not always a good idea to use it, but many people run essentially public nodes without wallet, and for those there is no need to 'hide in the crowd'.

@ABISprotocol

This comment has been minimized.

Show comment
Hide comment
@ABISprotocol

ABISprotocol Aug 10, 2015

@laanwj Since you asked, my thinking was that there might be ancillary effects on other users, a butterfly effect if you will in the privacy context. Though different functionally (and thus not comparable in the way one might consider it to be), the address reuse problem comes to mind (again, this is perhaps not relevant here because it is addressed in other ways, such as implementing stealth or confidential transactions); but at the core of this, the concern is that when you have an option which encourages or motivates users to associate a node with an address, then that will have privacy implications not only for the user who makes that choice, but for other users in the network as well.

@laanwj Since you asked, my thinking was that there might be ancillary effects on other users, a butterfly effect if you will in the privacy context. Though different functionally (and thus not comparable in the way one might consider it to be), the address reuse problem comes to mind (again, this is perhaps not relevant here because it is addressed in other ways, such as implementing stealth or confidential transactions); but at the core of this, the concern is that when you have an option which encourages or motivates users to associate a node with an address, then that will have privacy implications not only for the user who makes that choice, but for other users in the network as well.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 10, 2015

Member
Member

sipa commented Aug 10, 2015

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Sep 1, 2015

Member

I might be missing something but how does this implement BIP-0014? Current master happily accepts comments containing any of the "reserved symbols" mentioned in BIP-0014 and silently drops other characters when pushing to the network.
E.g. -:

My local node:

getnetworkinfo
[...]
"subversion": "/Satoshi:0.11.99(node-bob_12);:/:)()/",
[...]

Some remote node:

getpeerinfo
[...]
"subver": "/Satoshi:0.11.99(nodebob_12);:/:)()/",
[...]
Member

MarcoFalke commented Sep 1, 2015

I might be missing something but how does this implement BIP-0014? Current master happily accepts comments containing any of the "reserved symbols" mentioned in BIP-0014 and silently drops other characters when pushing to the network.
E.g. -:

My local node:

getnetworkinfo
[...]
"subversion": "/Satoshi:0.11.99(node-bob_12);:/:)()/",
[...]

Some remote node:

getpeerinfo
[...]
"subver": "/Satoshi:0.11.99(nodebob_12);:/:)()/",
[...]

@jonasschnelli jonasschnelli referenced this pull request in bitcoinxt/bitcoinxt Sep 12, 2015

Closed

implement uacomment config parameter #69

@str4d str4d referenced this pull request in zcash/zcash Jul 14, 2017

Open

Bitcoin 0.12 P2P/Net PRs 1 #2534

@str4d str4d referenced this pull request in zcash/zcash Dec 19, 2017

Merged

Implement uacomment config parameter #2813

zkbot added a commit to zcash/zcash that referenced this pull request Mar 21, 2018

Auto merge of #2390 - str4d:2132-mapargs-prep, r=<try>
Misc upstream PRs

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6077
  - Second commit only (first was already applied to 0.11.X and then reverted)
- bitcoin/bitcoin#6284
- bitcoin/bitcoin#6489
- bitcoin/bitcoin#6462
- bitcoin/bitcoin#6647
- bitcoin/bitcoin#6235
- bitcoin/bitcoin#6905
- bitcoin/bitcoin#6780
  - Excluding second commit (QT) and third commit (requires bitcoin/bitcoin#6993)
- bitcoin/bitcoin#6961
  - Excluding QT parts, and a small `src/policy/policy.cpp` change which depends on a bunch of other PRs, which we'll have to remember to come back to.
- bitcoin/bitcoin#7044
- bitcoin/bitcoin#8856
- bitcoin/bitcoin#9002

Part of #2074 and #2132.

zkbot added a commit to zcash/zcash that referenced this pull request Apr 13, 2018

Auto merge of #2813 - str4d:2074-uacomment, r=<try>
Implement uacomment config parameter

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6462
- bitcoin/bitcoin#6647

Part of #2074.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment