New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement uacomment config parameter #2813

Merged
merged 3 commits into from Apr 30, 2018

Conversation

@str4d
Contributor

str4d commented Dec 19, 2017

Cherry-picked from the following upstream PRs:

Part of #2074.

@str4d str4d requested review from daira and arcalinea Dec 19, 2017

@str4d str4d added this to 1.0.15: P2P / Net in Release planning Dec 20, 2017

@str4d str4d modified the milestone: 1.0.15 Jan 2, 2018

@str4d str4d moved this from 1.0.15: P2P / Net to 1.1.1: P2P / Net in Release planning Jan 4, 2018

@zkbot

This comment has been minimized.

Contributor

zkbot commented Feb 7, 2018

☔️ The latest upstream changes (presumably #2898) made this pull request unmergeable. Please resolve the merge conflicts.

@str4d str4d added this to the v1.1.1 milestone Mar 30, 2018

@str4d str4d requested review from Eirik0 and removed request for arcalinea Apr 5, 2018

@Eirik0

Eirik0 requested changes Apr 9, 2018 edited

Went through each file and looks good. There was an extra test in test_FormatSubVersion in util_tests.cpp which has been updated which looks good.

Assuming the merge conflicts are resolved, I would say this is good to go.

prusnak and others added some commits Jul 27, 2015

implement uacomment config parameter
which can add comments to user agent as per BIP-0014
limit total length of user agent comments
Reworked-By: Wladimir J. van der Laan <laanwj@gmail.com>
[uacomment] Sanitize per BIP-0014
* SanitizeString() can be requested to be more strict
* Throw error when SanitizeString() changes uacomments
* Fix tests
@str4d

This comment has been minimized.

Contributor

str4d commented Apr 13, 2018

Rebased on master to fix merge conflict.

@zkbot try

@zkbot

This comment has been minimized.

Contributor

zkbot commented Apr 13, 2018

⌛️ Trying commit 3c1db17 with merge c97906e...

zkbot added a commit 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.

@str4d str4d requested a review from Eirik0 Apr 13, 2018

@str4d str4d added this to Implemented — Waiting for ACKs in Zcashd Team Apr 13, 2018

@zkbot

This comment has been minimized.

Contributor

zkbot commented Apr 13, 2018

☀️ Test successful - pr-try
State: approved= try=True

@Eirik0

Eirik0 approved these changes Apr 13, 2018

@Eirik0

This comment has been minimized.

Contributor

Eirik0 commented Apr 16, 2018

utACK

@str4d str4d requested a review from bitcartel Apr 18, 2018

@str4d str4d requested review from arcalinea and removed request for daira Apr 26, 2018

@mdr0id mdr0id self-requested a review Apr 30, 2018

@mdr0id

mdr0id approved these changes Apr 30, 2018

Tested ACK:

zcash.conf

uacomment=BIRD1
uacomment=BIRD2

➜ src git:(2074-uacomment) ./zcash-cli getnetworkinfo

"subversion": "/MagicBean:1.1.0(BIRD1; BIRD2)/",

string strResult;
for (std::string::size_type i = 0; i < str.size(); i++)
{
if (safeChars.find(str[i]) != std::string::npos)
if (SAFE_CHARS[rule].find(str[i]) != std::string::npos)

This comment has been minimized.

@mdr0id

mdr0id Apr 30, 2018

Contributor

It seems like there should be an additional sanity check on the rule parameter passed in since it is used to index SAFE_CHARS. The outer loop does not set this, and it doesn't seem safe to assume this will always be passed a valid index via the rule param.

@mdr0id mdr0id merged commit b300118 into zcash:master Apr 30, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Review to Released (Merged in Master) Apr 30, 2018

@str4d str4d deleted the str4d:2074-uacomment branch Apr 30, 2018

@str4d str4d removed request for bitcartel and arcalinea May 1, 2018

@str4d str4d referenced this pull request May 1, 2018

Open

Misc upstream PRs #2390

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