Propagate the user agent from node to pool #664

Falci · 2021-11-26T11:53:06Z

No description provided.

coveralls · 2021-11-26T11:57:23Z

Pull Request Test Coverage Report for Build 1584633147

1 of 7 (14.29%) changed or added relevant lines in 1 file are covered.
906 unchanged lines in 6 files lost coverage.
Overall coverage increased (+0.4%) to 62.647%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/net/pool.js	1	7	14.29%

Files with Coverage Reduction	New Missed Lines	%
lib/net/pool.js	2	30.29%
lib/mining/template.js	7	87.2%
lib/net/packets.js	13	93.16%
lib/wallet/walletdb.js	34	78.96%
lib/blockchain/chain.js	253	68.09%
lib/node/rpc.js	597	30.59%

Totals
Change from base Build 1502820251:	0.4%
Covered Lines:	21298
Relevant Lines:	31800

💛 - Coveralls

nodech · 2021-11-30T05:53:32Z

I think it's okay for users to rewrite Agent, but I was wondering if for general cases we should add special option, so they can just annotate hsd version. E.g.
./hsd --agent-comment other-variation - giving us version - /hsd:3.0.1(other-variation)/.

This way we don't lose information about original version. Ofc someone really wants to, they could use agent to totally rewrite and that's okay as well, but in general this should give better alternative ?

p.s. This is similar to bitcoins uacomment.

nodech · 2021-11-30T05:58:05Z

@Falci could you also use that as an alternative in bob-wallet ?

It could be: /hsd:3.0.1 (bob:version)/. I see you are also preserving hsd version there, so I guess it will be ok.

Falci · 2021-11-30T07:41:13Z

@nodech it sounds good. I'll work on that.

pinheadmz · 2021-12-15T15:07:08Z

I agree with @nodech I use this commit for easyhandshake: pinheadmz@8c4f397

pinheadmz · 2021-12-17T17:34:12Z

lib/node/fullnode.js

+      agent: this.config.str('agent'),
+      agentComment: this.config.str('agentComment'),


Can we just add one?

The PR was about the first. You guys asked for this extra.
Why both?

Ofc someone really wants to, they could use agent to totally rewrite and that's okay as well, but in general this should give better alternative ?

pinheadmz · 2021-12-17T17:35:11Z

lib/net/pool.js

+    if (options.agentComment) {
+      assert(typeof options.agentComment === 'string');
+
+      let agentComment;
+      if (this.agent.endsWith('/')) {
+        agentComment = `${this.agent.slice(0, -1)}(${options.agentComment})/`;
+      } else {
+        agentComment = `${this.agent}(${options.agentComment})`;
+      }
+;
+      assert(agentComment.length <= 255);
+      this.agent = agentComment;
+    }
+


Many thoughts:

assertion errors should report why they throw ("...must be string")

comment should be appended to current (default) agent: /hsd:v3.0.1/sinpapeles

total string should be checked for length and asserted with explanation if error

why do we care if the ua comment ends with a slash>

This implementation follows @nodech's comment:

./hsd --agent-comment other-variation - giving us version - /hsd:3.0.1(other-variation)/.

pinheadmz · 2021-12-17T17:53:37Z

@nodech maybe we need to agree on how this should work. Here's what I'm thinking:

only one new option, --agent-comment or --agent I don't care
whatever that string is, gets appended to default hsd version: /hsd:v3.0.1/<<<WHATEVER>>>
if null or empty sting, default agent continues to be: /hsd:v3.0.1/

Users that want to replace the entire agent string can fork hsd, but IMO this is the best way to add extra strings like bob wallet version and still get stats about hsd version deployment

nodech · 2021-12-18T15:13:52Z

@nodech maybe we need to agree on how this should work. Here's what I'm thinking:
* only one new option, `--agent-comment` or `--agent` I don't care

* whatever that string is, gets appended to default hsd version: `/hsd:v3.0.1/<<<WHATEVER>>>`

* if `null` or empty sting, default agent continues to be: `/hsd:v3.0.1/`
Users that want to replace the entire agent string can fork hsd, but IMO this is the best way to add extra strings like bob wallet version and still get stats about hsd version deployment

That sounds good with one modification: /hsd:v3.0.1/something-else:version/ and this will be in line with BIP 14

As for user agent comments, they are meant for something else.
-- EDIT

Currently, Let's add --agent only that adds that something-else bit and don't bother with user agent.
User agent could be used by specific builds for specific devices, but I don't think that's a good idea tbh.

pinheadmz · 2021-12-18T15:30:57Z

Ok SGTM. So you want to remove any trailing slashes from the input string and always add one back in the final output? What about the other special characters mentioned in the BIP?

nodech · 2021-12-18T15:33:23Z

Because we are accepting whole version in the latter part of /hsd../passed-agent/, we can't actually disallow (): - they can be passed. Only thing we should disallow there is /, because user may want to pass something like: mymodifications:v0.1.1 (Windows)

rithvikvibhu · 2022-04-01T07:07:06Z

Hey, just checking in if it's ready to merge. I think it's fine to have both agent and agentComment - most would prefer agentComment and when integrating with other apps (like fingertip) they can change agent to that app's name.

pinheadmz · 2022-04-11T18:09:41Z

Closing this and moving discussion to updated PR #710
@nodech @rithvikvibhu @Falci would appreciate your review there.

Propagate the user agent from node to pool

d508964

Falci mentioned this pull request Nov 26, 2021

Broadcast Bob's name and version via hsd kyokan/bob-wallet#431

Merged

nodech added net/p2p part of the codebase quick review difficulty - easy labels Nov 30, 2021

nodech added the waiting for the author modifier label Dec 2, 2021

nodech added the patch Release version label Dec 9, 2021

feat(pool): Adds --agent-comment option

27041b4

pinheadmz requested changes Dec 17, 2021

View reviewed changes

pinheadmz mentioned this pull request Apr 11, 2022

net: propagate the user agent from node to pool #710

Merged

1 task

pinheadmz closed this Apr 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate the user agent from node to pool #664

Propagate the user agent from node to pool #664

Falci commented Nov 26, 2021

coveralls commented Nov 26, 2021 •

edited

Loading

nodech commented Nov 30, 2021 •

edited

Loading

nodech commented Nov 30, 2021 •

edited

Loading

Falci commented Nov 30, 2021

pinheadmz commented Dec 15, 2021

pinheadmz Dec 17, 2021

Falci Dec 17, 2021

pinheadmz Dec 17, 2021

Falci Dec 17, 2021

pinheadmz commented Dec 17, 2021 •

edited

Loading

nodech commented Dec 18, 2021 •

edited

Loading

pinheadmz commented Dec 18, 2021

nodech commented Dec 18, 2021

rithvikvibhu commented Apr 1, 2022

pinheadmz commented Apr 11, 2022

		agent: this.config.str('agent'),
		agentComment: this.config.str('agentComment'),

Propagate the user agent from node to pool #664

Propagate the user agent from node to pool #664

Conversation

Falci commented Nov 26, 2021

coveralls commented Nov 26, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1584633147

💛 - Coveralls

nodech commented Nov 30, 2021 • edited Loading

nodech commented Nov 30, 2021 • edited Loading

Falci commented Nov 30, 2021

pinheadmz commented Dec 15, 2021

pinheadmz Dec 17, 2021

Choose a reason for hiding this comment

Falci Dec 17, 2021

Choose a reason for hiding this comment

pinheadmz Dec 17, 2021

Choose a reason for hiding this comment

Falci Dec 17, 2021

Choose a reason for hiding this comment

pinheadmz commented Dec 17, 2021 • edited Loading

nodech commented Dec 18, 2021 • edited Loading

pinheadmz commented Dec 18, 2021

nodech commented Dec 18, 2021

rithvikvibhu commented Apr 1, 2022

pinheadmz commented Apr 11, 2022

coveralls commented Nov 26, 2021 •

edited

Loading

nodech commented Nov 30, 2021 •

edited

Loading

nodech commented Nov 30, 2021 •

edited

Loading

pinheadmz commented Dec 17, 2021 •

edited

Loading

nodech commented Dec 18, 2021 •

edited

Loading