-
Notifications
You must be signed in to change notification settings - Fork 83
Disable software expiry by default #124
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
base: 28.x-knots
Are you sure you want to change the base?
Disable software expiry by default #124
Conversation
It should only be disabled by power users, would be accurate. The current default behaviour is best for ordinary users. |
utAck b408201 Updated the default value to 0, ensuring the checker always returns false, effectively bypassing the comparative check, preventing shutting down bool IsThisSoftwareExpired(int64_t nTime)
{
if (g_software_expiry <= 0) {
return false;
}
return (nTime > g_software_expiry);
} The current approach carries potential issues, particularly for non-admin users. Without proper handling, it increases the risk of unexpected behaviour. This can be especially problematic if a Lightning Network (LN) node relies on this behaviour. |
I think having a way of yelling at users running very old software (who haven't explicitly indicated that they want to, eg with the config option) is a fair idea, given upstream's disclosure policies, but agree shutting down isn't the best solution. Maybe a warning prompt which requires typing out a confirmation (eg: "I accept that this software may be insecure"), so users can't just ignore it by hitting space/enter.
|
NACK. I'm for your node forcing you to acknowledge changing security contexts and actively manage it by default. I don't think the default behavior should be to ignore a lack of node management and pretend everything is fine without being explicitly told to. That said, I think the proposed idea of setting the override setting via GUI with explicit error message and consent is a good idea. I appreciate that this may cause passively managed daemon only nodes some troubleshooting as they are forced to actively manage their node. Good. That's the point. For RPC users such as lightning use cases, I presume that these users will be alerted by the fact they aren't in sync anymore. If more than that is necessary it could be reasonable to throw RPC errors regarding the invalid node state so users are forced to investigate the error and make some intentional management choices. What this 2 year default really highlights for me however is that the existing security disclosure policies are entirely inappropriate. How can a user make an informed choice about the code they run and the safety of an older client if they don't have the disclosures? |
For an alternative, here's what I'm thinking(, have partially implemented), and am seeking opinions on: Startup
At runtimeThe
Rough ImplsTwo branches for implementing (roughly) the above: https://github.com/pithosian/bitcoin/pull/1/files achieves the above at the cost of a much larger change. A middleground could be to use a |
Most ordinary users are unaware of this option and expiration followed by rejection of blocks. |
Just a general comment here. I would also avoid too much "calculations" in the constants, such as DEFAULT_SOFTWARE_EXPIRY. Set it to zero as suggested and do the calculation/check in function/method instead. This will make it more readable and easier to understand, in addition to make tests on it. Or even better IMO implement all time and date handling into it's own library. |
Concept ACK, the current knots creates a fork like behavior between other client implementations on the Bitcoin networks. This could also lead to a miner losing revenue or wasting energy if their node stops running after the expiry. |
No fork like behavior. The node drops off the network. It doesn't follow a fork of the chain or get tricked into joining a fork of the network. If you mean a Git fork (Knots is a Git fork of Core) Luke has been able to maintain this code diff from Bitcoin Core without a problem for years. One can certainly argue for and against this default but it isn't "fork like behavior". edit: Interesting. Some people are claiming that the node doesn't drop off the network (the Bitcoin daemon is killed) and it just stops accepting blocks on the most proof-of-work consensus valid chain. edit edit: From a quick perusal I think the claims are rubbish. The daemon would need to stay running, disconnect from all peers or be restarted with different consensus rules to when it shutdown to "accept backdated blocks on a lower PoW chain". I would like to test this though to check this is definitely FUD. |
This is exactly what is happening 24 hours after the MTP passses expiration, so at that point could someone not start building blocks (n) blocks back. Then end up with a timestamp just higher than the last valid MTP and get a reorg off that? edit: sauce Line 4458 in 5f82566
|
@portlandhodl: If that's the case and it isn't a complete shutdown in all cases then yeah the code isn't doing how the feature has been described ("software expiry"). It is one thing to completely shutdown after x years, it is a whole other thing entirely to stay up in some kind of zombie state without the user realizing. |
Rationale: This config option should only be used by power users as it could reject blocks. Some information about its usage could be added in docs or release notes.
Alternative solution: Warning or error after expiry.
I couldn't find a way to exploit this feature however it could lead to bugs, bad user experience etc. Related discussion: https://njump.me/nevent1qvzqqqqqqypzqt4s8g0nzmpul8yspel4xmhz3e2gvdysv7lqrz5ktf78efd57leuqqs9ll49daxc2w2qsn8xckr54acr2xcj076x2hfufyswtmgyx4kuqqqcgf23c