Skip to content
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

Closes #2092. Maintain record of alerts sent onto mainnet. #2093

Merged
merged 3 commits into from
Feb 20, 2017

Conversation

bitcartel
Copy link
Contributor

No description provided.

@bitcartel bitcartel added A-networking Area: Networking code I-SECURITY Problems and improvements related to security. labels Feb 12, 2017
@bitcartel bitcartel added this to the 1.0.7 milestone Feb 12, 2017
Copy link

@Maroc-OS Maroc-OS left a comment

Choose a reason for hiding this comment

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

Some things have to be changed and/or defined globally. anyone agree?

alert.strComment = "";
alert.strStatusBar = "URGENT: Upgrade required: see https://z.cash";
alert.strRPCError = "URGENT: Upgrade required: see https://z.cash";
alert.strStatusBar = "Your client is out of date and potentially vulnerable to denial of service. Please update to the most recent version of Zcash (1.0.5). More info at: https://z.cash/support/security.html";

Choose a reason for hiding this comment

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

should be "1.0.6", maybe "1.0.7", why not using zcash VERSION already defined to keep it dynamic!!

Copy link
Contributor

Choose a reason for hiding this comment

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

Because this is documenting the message that was actually sent. There would be no point in making it dependent on the version of zcash that is sending the message, because that is only evaluated once, and then the alert message is just a string.


// Set specific client version/versions here. If setSubVer is empty, no filtering on subver is done:
// alert.setSubVer.insert(std::string("/MagicBean:0.7.2/"));
alert.setSubVer.insert(std::string("/MagicBean:1.0.3/"));
Copy link

@Maroc-OS Maroc-OS Feb 13, 2017

Choose a reason for hiding this comment

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

This also needs to be refactored "/MagicBean:1.0.3/", not in this PR but in whole tree, this have to get defined somewhere, to keep code clean and can be changed in a single file

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case the alert was intended to only apply to 1.0.3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Maroc-OS This PR is more about keeping a historical record of alerts that have been sent out.

Choose a reason for hiding this comment

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

I C am still reading the codes, am getting it bit by bit :)

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK; minor nit about indentation.

// Sanity check
assert(alert.strComment.length() <= 65536); // max length in alert.h
assert(alert.strStatusBar.length() <= 256);
assert(alert.strRPCError.length() <= 256);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there's an indentation mismatch (tabs?) in this block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Tabs changed to spaces.

@daira
Copy link
Contributor

daira commented Feb 19, 2017

utReACK. @zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Feb 19, 2017

📌 Commit f47269c has been approved by daira

@zkbot
Copy link
Contributor

zkbot commented Feb 19, 2017

⌛ Testing commit f47269c with merge 55ac53a...

@zkbot
Copy link
Contributor

zkbot commented Feb 19, 2017

💔 Test failed - zcash

@str4d
Copy link
Contributor

str4d commented Feb 20, 2017

Test failures caused by #2112.

@str4d
Copy link
Contributor

str4d commented Feb 20, 2017

@zkbot retry

@zkbot
Copy link
Contributor

zkbot commented Feb 20, 2017

⌛ Testing commit f47269c with merge 4e338b0...

zkbot added a commit that referenced this pull request Feb 20, 2017
Closes #2092. Maintain record of alerts sent onto mainnet.
@zkbot
Copy link
Contributor

zkbot commented Feb 20, 2017

☀️ Test successful - zcash

@zkbot zkbot merged commit f47269c into zcash:master Feb 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Area: Networking code I-SECURITY Problems and improvements related to security.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants