Remove stale pending upgrades

[MonsieurNicolas]

Right now it's possible to have stale upgrades queued in the "upgrade queue" if the node was not online and in sync when the upgrade happened.

This can have potentially bad effects such as causing unwanted votes for changing network settings when they eventually change.

This is caused by the current implementation that only Upgrades::removeUpgrades to remove upgrades.

We should instead simply make createUpgradesFor clear out upgrades that are no op and remove the (then redundant) Upgrades::removeUpgrades code.

[MonsieurNicolas]

actually we should just remove invalid upgrades altogether and consider no-op invalid

[MonsieurNicolas]

I think that removing very old upgrades could be the best option (very simple): basically if an upgrade is passed its trigger time by more than X hours, just remove it. What do you think @sisuresh ?

[sisuresh]

Yeah that sounds like a good idea. Does a hardcoded threshold of 12 hours make sense to you?

[MonsieurNicolas]

yes sounds good to me @sisuresh

[sisuresh]

@MonsieurNicolas What do you think about adding the logic to remove old upgrades to removeUpgrade? There might be an issue with moving the removeUpgrade logic to createUpgradesFor.

Right now, the execution path looks like valueExternalized -> removeUpgrades
-> setUpgrades (this persists the upgrades) -> async_wait( triggerNextLedger -> createUpgradesFor). I'm concerned about moving the remove and persist upgrades logic into the asyncWait.

[OwenJacob]

I would just like to add a small note regarding this behavior:

I'm using the commands section in the stellar-core.cfg to send upgrades for nodes in a private network. When I spin up a node the upgrades are armed so that once the network gains consensus the upgrades are triggered. The upgrades are set to be triggered for Unix epoch - following this change the upgrades need to be triggered using a time within 12 hours of now or in the future meaning the configuration needs to be constantly updated for nodes that require upgrades.

What I have now:
COMMANDS=[ "upgrades?mode=set&upgradetime=1970-01-01T00:00:00Z&basefee=10000000&basereserve=1000000&maxtxsize=500&protocolversion=12", "ll?level=${LOG_LEVEL}" ]

What I should have in future:
COMMANDS=[ "upgrades?mode=set&upgradetime={A time within 12 hours of now}&basefee=10000000&basereserve=1000000&maxtxsize=500&protocolversion=12", "ll?level=${LOG_LEVEL}" ]

My use case is definitely a corner case but I note for the Googler of the future...