Implement automatic shutdown of deprecated Zcash versions #2297

str4d · 2017-04-25T06:30:20Z

Closes #2274.

str4d · 2017-04-25T06:35:58Z

Started after warning threshold, deprecation threshold reached:

Started after deprecation, -disabledeprecation=1.0.8 (assertion is I think related to issue #2214):

Started after deprecation, -disabledeprecation=1.0.8-1:

daira · 2017-04-25T11:07:38Z

Can the messages be wrapped using FormatParagraph, please?

daira · 2017-04-25T11:13:35Z

Also, "deprecation" to me means that the use of something is discouraged but it still works. We need a different word.

str4d · 2017-05-11T03:37:23Z

With wrapping:

ebfull · 2017-05-11T05:23:49Z

@daira In a sense it still works because you can bypass the shutdown, you just have to acknowledge the deprecation and explicitly ignore it. Like selectively disabling a compiler warning. (With warnings set to error. 😄)

arielgabizon · 2017-05-11T19:13:10Z

src/init.cpp

@@ -344,6 +344,7 @@ std::string HelpMessage(HelpMessageMode mode)
 #endif
    }
    strUsage += HelpMessageOpt("-datadir=<dir>", _("Specify data directory"));
+    strUsage += HelpMessageOpt("-disabledeprecation=<version>", _("Disable block-height node deprecation"));


It's hard to understand what exactly <version> is here

I'll add documentation for <version> in the help string.

str4d · 2017-05-11T22:08:21Z

@daira @ebfull I went with -disabledeprecation because the automatic shutdown arises from the deprecation policy, and it is the deprecation policy for that version we are disabling. That is, because we call the policy deprecation, I figured the flag should be -disabledeprecation (so the two are clearly associated in the user's mind). But perhaps that isn't necessary. Or maybe we can add clarification of the effects of -disabledeprecation to the help string?

nathan-at-least

NACK: There's no automated test coverage of this EnforceNodeDeprecation.

nathan-at-least · 2017-05-11T22:10:28Z

src/deprecation.h

+
+// Deprecation policy is 4th third-Tuesday after a release
+static const int APPROX_RELEASE_HEIGHT = 115000;
+static const int WEEKS_UNTIL_DEPRECATION = 18;


Add a comment like: // We use 18 weeks to ensure auto-deprecation is *always* > 4 months, barring block time skew issues.

See above; this value is to be updated during the release process such that it is the correct number of weeks for that release.

nathan-at-least · 2017-05-11T22:11:28Z

src/deprecation.h

+#define ZCASH_DEPRECATION_H
+
+// Deprecation policy is 4th third-Tuesday after a release
+static const int APPROX_RELEASE_HEIGHT = 115000;


It's important to me that this has a very high chance of occuring after the release day. Add a comment with release date and back-of-the-envelope calculations showing some grace period.

Did you review the whole PR? I updated the release process to require that these values are updated, so it will always be the case that this value occurs after the release day.

I did read the whole PR. My rationale was someone reading this code who doesn't know our release process would know how we calculated the number. It's not important enough to block the PR, and the comment on the preceding line should make it clear enough.

Closes zcash#2274.

str4d · 2017-05-12T02:43:59Z

Force-pushed to squash the following changes in:

Clarify help text, add example.
Expand notes in release process doc
Add unit tests.

nathan-at-least

Excellent unit tests of the boundary conditions, shutdown, logging, and disabling behaviors!

nathan-at-least · 2017-05-12T03:57:00Z

src/deprecation.h

+#define ZCASH_DEPRECATION_H
+
+// Deprecation policy is 4th third-Tuesday after a release
+static const int APPROX_RELEASE_HEIGHT = 115000;


I did read the whole PR. My rationale was someone reading this code who doesn't know our release process would know how we calculated the number. It's not important enough to block the PR, and the comment on the preceding line should make it clear enough.

arielgabizon · 2017-05-12T11:33:28Z

I'm not sure I understand the point of the version parameter.
From what I understood in the code, you're supposed to give as this parameter the version you are currently running.
But if that's the case, then why not just interpret --disabledeprecation as
"Whatever version I'm running, even if I don't remember my version number, I don't want to deprecate right now"
and not require this parameter?

You could still remind the user what version he is running in the message here
5b3bc97#diff-f8d7df41b8550c564a6295b0952fa1c3R49

str4d · 2017-05-12T20:39:21Z

I'm not sure I understand the point of the version parameter.
From what I understood in the code, you're supposed to give as this parameter the version you are currently running.
But if that's the case, then why not just interpret --disabledeprecation as
"Whatever version I'm running, even if I don't remember my version number, I don't want to deprecate right now"
and not require this parameter?

The reasoning IMHO is that we don't want that parameter to be "whatever version I'm running, even if I don't remember my version number", because that is exactly equivalent to "ignore deprecation always", and that is a much easier flag to have added in third-party default configs and then be forgotten about, which removes the benefit of this feature almost entirely (because it is effectively no longer by default). If the current version is necessary, then either the user is required to think about that flag on every upgrade (which is additional friction as @bitcartel noted here), or the third party wallet etc. is required to consider it when designing their UI. Either case is I think much closer to the desired goals of this feature. Of course, this assumes we do go for this feature (auto-senescence) instead of one of the other approaches being discussed in #2274.

arielgabizon · 2017-05-13T03:49:24Z

OK that makes sense

daira

ut(ACK+cov), modulo one nonblocking comment.

daira · 2017-05-14T14:32:25Z

src/deprecation.cpp

+                            DEPRECATION_HEIGHT) + " " +
+                  _("You should upgrade to the latest version of Zcash.") + " " +
+                  strprintf(_("To disable deprecation for this version, set %s%s."),
+                            "-disabledeprecation=", CLIENT_VERSION_STR);


I think this should say "To disable deprecation shutdown..." and (less important) that the option name should be "-disabledeprecationshutdown=".

nathan-at-least · 2017-05-14T18:48:27Z

@zkbot r+

zkbot · 2017-05-14T18:48:28Z

📌 Commit b4f861d has been approved by nathan-at-least

zkbot · 2017-05-14T18:48:32Z

⌛ Testing commit b4f861d with merge 301db3a0a548d2e6900564aecf284117d6efc8d8...

zkbot · 2017-05-14T18:50:43Z

💔 Test failed - pr-merge

nathan-at-least · 2017-05-15T01:38:44Z

@zkbot retry

zkbot · 2017-05-15T01:38:49Z

⌛ Testing commit b4f861d with merge 3a98e3b...

Implement automatic shutdown of deprecated Zcash versions Closes #2274.

bitcartel · 2017-05-15T03:07:58Z

Will this merge close ticket #2274 partially or fully? I was under the impression we were still discussing this PR, based on what @str4d wrote above:

Of course, this assumes we do go for this feature (auto-senescence) instead of one of the other approaches being discussed in #2274.

I just posted a comment yesterday on the ticket: #2274 (comment)

I was hoping we would have more discussion about how this feature might be deployed as in its current form its a big change for end users.

zkbot · 2017-05-15T03:21:36Z

☀️ Test successful - pr-merge
Approved by: nathan-at-least
Pushing 3a98e3b to master...

str4d added D-bitcoin-divergence Design issue: Divergence from Bitcoin (upstream code and/or architecture). network upgrade management Z-NU0 wishlist I-SECURITY Problems and improvements related to security. labels Apr 25, 2017

str4d added this to the 1.0.9 milestone Apr 25, 2017

str4d requested a review from nathan-at-least April 25, 2017 06:30

daira added this to In Progress in Security and Stability Apr 26, 2017

daira added this to Work Queue in Network Upgrade 0 Apr 26, 2017

daira moved this from Work Queue to Awaiting Review in Network Upgrade 0 Apr 26, 2017

arielgabizon reviewed May 11, 2017

View reviewed changes

nathan-at-least suggested changes May 11, 2017

View reviewed changes

str4d added 2 commits May 12, 2017 14:41

Implement automatic shutdown of deprecated Zcash versions

5b3bc97

Closes zcash#2274.

Wrap messages nicely on metrics screen

b4f861d

str4d force-pushed the 2274-apoptosis branch from fe7404c to b4f861d Compare May 12, 2017 02:42

nathan-at-least approved these changes May 12, 2017

View reviewed changes

nathan-at-least requested review from daira and ageis May 12, 2017 04:03

arielgabizon approved these changes May 13, 2017

View reviewed changes

daira approved these changes May 14, 2017

View reviewed changes

zkbot added a commit that referenced this pull request May 15, 2017

Auto merge of #2297 - str4d:2274-apoptosis, r=nathan-at-least

3a98e3b

Implement automatic shutdown of deprecated Zcash versions Closes #2274.

zkbot merged commit b4f861d into zcash:master May 15, 2017

This was referenced May 15, 2017

Switch to libsecp256k1-based validation for ECDSA #2335

Merged

Remove unneeded lax ECDSA signature verification #2360

Merged

nathan-at-least mentioned this pull request May 15, 2017

Design hard fork activation mechanism #2286

Closed

nathan-at-least moved this from In Progress to Awaiting Review in Security and Stability May 30, 2017

nathan-at-least moved this from Awaiting Review to Complete in Security and Stability May 30, 2017

WaveringAna mentioned this pull request Jun 12, 2017

24 hour ban peers with out of date client versions. HorizenOfficial/zen_archived#4

Closed

nathan-at-least moved this from Awaiting Review to Complete in Network Upgrade 0 Jun 19, 2017

radix42 mentioned this pull request Sep 16, 2017

Adjust auto-senescence parameters MyHush/hush#34

Open

WaveringAna mentioned this pull request Sep 23, 2017

Pull PRs from upstream #1 HorizenOfficial/zen_archived#69

Merged

daira removed the Z-NU0 wishlist label Nov 9, 2017

WaveringAna mentioned this pull request Nov 18, 2017

Development HorizenOfficial/zen_archived#90

Merged

defuse mentioned this pull request Jan 30, 2019

Boost Build Error With --enable-proton #3816

Closed

str4d deleted the 2274-apoptosis branch January 27, 2023 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement automatic shutdown of deprecated Zcash versions #2297

Implement automatic shutdown of deprecated Zcash versions #2297

str4d commented Apr 25, 2017

str4d commented Apr 25, 2017

daira commented Apr 25, 2017

daira commented Apr 25, 2017 •

edited

Loading

str4d commented May 11, 2017

ebfull commented May 11, 2017 •

edited

Loading

arielgabizon May 11, 2017 •

edited

Loading

str4d May 11, 2017

str4d commented May 11, 2017

nathan-at-least left a comment

nathan-at-least May 11, 2017

str4d May 12, 2017

nathan-at-least May 11, 2017

str4d May 12, 2017

nathan-at-least May 12, 2017

str4d commented May 12, 2017

nathan-at-least left a comment

nathan-at-least May 12, 2017

arielgabizon commented May 12, 2017

str4d commented May 12, 2017

arielgabizon commented May 13, 2017

daira left a comment

daira May 14, 2017

nathan-at-least commented May 14, 2017

zkbot commented May 14, 2017

zkbot commented May 14, 2017

zkbot commented May 14, 2017

nathan-at-least commented May 15, 2017

zkbot commented May 15, 2017

bitcartel commented May 15, 2017

zkbot commented May 15, 2017

Implement automatic shutdown of deprecated Zcash versions #2297

Implement automatic shutdown of deprecated Zcash versions #2297

Conversation

str4d commented Apr 25, 2017

str4d commented Apr 25, 2017

daira commented Apr 25, 2017

daira commented Apr 25, 2017 • edited Loading

str4d commented May 11, 2017

ebfull commented May 11, 2017 • edited Loading

arielgabizon May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d commented May 11, 2017

nathan-at-least left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d commented May 12, 2017

nathan-at-least left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielgabizon commented May 12, 2017

str4d commented May 12, 2017

arielgabizon commented May 13, 2017

daira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathan-at-least commented May 14, 2017

zkbot commented May 14, 2017

zkbot commented May 14, 2017

zkbot commented May 14, 2017

nathan-at-least commented May 15, 2017

zkbot commented May 15, 2017

bitcartel commented May 15, 2017

zkbot commented May 15, 2017

daira commented Apr 25, 2017 •

edited

Loading

ebfull commented May 11, 2017 •

edited

Loading

arielgabizon May 11, 2017 •

edited

Loading