Rework Software Expiry #134
Conversation
|
Approach ACK |
| QMessageBox::critical(nullptr, QObject::tr("Software expired"), QObject::tr("This software is expired, and may be out of consensus. You must choose to upgrade or override this expiration.")); | ||
| return EXIT_FAILURE; | ||
| if (IsThisSoftwareExpiringSoon(GetTime())) { | ||
| QMessageBox::warning(nullptr, QObject::tr("Software expires soon"), QObject::tr("This software expires soon, and may fall out of consensus. You must choose to upgrade, or override this expiration with the 'softwareexpiry' option (Unix datetime, or 0 for no expiry).")); |
There was a problem hiding this comment.
Probably shouldn't pause startup waiting for a response
| return EXIT_FAILURE; | ||
| } | ||
|
|
||
| gArgs.ModifyRWConfigFile("softwareexpiry", "0"); |
There was a problem hiding this comment.
rwconf is kind of deprecated at this point (superceded by settings.json)
There was a problem hiding this comment.
The ModifyRWConfigFile function has a default-true arg for also_settings_json. I've confirmed that this actually updates the settings.json file (I've been launching with -softwareexpiry=X for testing purposes).
Line 196 in 5f82566
Line 1099 in 5f82566
Line 1090 in 5f82566
Line 1060 in 5f82566
Is there some other approach we want to take, or all good?
| return EXIT_FAILURE; | ||
| } | ||
|
|
||
| gArgs.ModifyRWConfigFile("softwareexpiry", "0"); |
There was a problem hiding this comment.
Probably shouldn't persist forever, and should reset to default if the user upgrades?
There was a problem hiding this comment.
How would we want to approach that?
I think if we don't introduce an update detection mechanism, we have to go with 0. If we set eg 2 years in the future, then in a year the user updates to a brand new version, that version would end up erroneously expiring in 1 year.
The approach which springs to mind is adding an additional config value, like uiexpiryoverrideversion, and setting it to the current version when overriding software expiry. Then on launch we could check whether the uiexpiryoverrideversion is the currently running version (and not an empty string) and, if not, clear uiexpiryoverrideversion and softwareexpiry in the rwconf/settings.json. If the user manually configured softwareexpiry in their bitcoin.conf, it would be untouched.
We could also generalize this with just a lastexecutedversion value instead, always set to the current version, and have some handler for when we detect a change.
| @@ -4458,12 +4458,13 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio | |||
| if (IsThisSoftwareExpired(block.nTime)) { | |||
| // Wait an extra day before we start rejecting blocks | |||
| CBlockIndex const *blockindex_old = pindexPrev; | |||
| for (int i = 0; i < 144; ++i) { | |||
| for (int i = 0; i < std::min(144, pindexPrev->nHeight); ++i) { | |||
There was a problem hiding this comment.
I suppose it should be
| for (int i = 0; i < std::min(144, pindexPrev->nHeight); ++i) { | |
| for (int i = 0; i < std::min(144, pindexPrev->nHeight - 1); ++i) { |
?
But to avoid re-calculating it every iteration, better to offset the starting index.
| assert(blockindex_old); | ||
| blockindex_old = blockindex_old->pprev; | ||
| } | ||
| assert(blockindex_old); | ||
| if (IsThisSoftwareExpired(blockindex_old->GetMedianTimePast())) { | ||
| chainman.GetNotifications().fatalError(_("This software is expired, and may be out of consensus. You must choose to upgrade, or override this expiration with the 'softwareexpiry' option (Unix datetime, or 0 for no expiry).")); |
There was a problem hiding this comment.
Maybe use warningSet so it shows up in RPC, -alertnotify, etc?
|
Been away from the computer for a few days, but ack the comments; will take a look at cleanup this coming week. |
|
Approach ACK |
1 similar comment
|
Approach ACK |
|
I believe I've addressed the concepts presented herein in the upcoming Knots 29 |
As an alternative to #124.
bitcoindandbitcoin-qt.bitcoindstartup failure message for expired software.bitcoin-qtstartup error message for expired software with an input box requiring the user to typeI accept this software may be unsafe.to continue (overriding expiry this way updates configuration).bitcoindorbitcoin-qtexpire at runtime, rather than silently rejecting blocks.The rationale for these changes is largely explained by my comments on the above-linked PR. I considered using the same prompt at runtime as at startup for
bitcoin-qt, but the churn required was much higher with minimal benefit (as it stands, the software pops up a dialog telling the user the software is expired, on closing,bitcoin-qtexits, and if the user tries to start it again, they get the override prompt).As a brief summary:
bitcoind, shutting down with an error message telling the user how to override is effectively identical to pushing a prompt for continued operation when running attached to a terminal and, when not running attached to a terminal (eg disowned, or under a service manager), a prompt cannot be displayed anyway.bitcoin-qtnow gives the user the opportunity to override without going and manually editing the configuration files, without simply showing them a dialog they can hit yes on to continue without thinking about anything.