It prevented the GUI from starting when its settings.json had the -upnp option set. This also doesn’t prevent the node from running, so this error didn’t need to be fatal.
Thanks to Sjors for bringing attention to what i broke and to Maflcko for suggesting a simple short term fix.
It prevented the GUI from starting when its settings.json had the -upnp option set. This also doesn't
prevent the node from running, so this error didn't need to be fatal.
Thanks to Sjors for bringing attention to what i broke and to Maflcko for suggestion a simple short
term fix.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31198.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, tdb3, achow101 |
| Concept ACK | kevkevinpal, laanwj |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
lgtm ACK a1b3ccae4be82297fd20f5be15a03eeb477507d0
I think the GUI will still need to be fixed either way. A warning is tolerable, but horrible UX, if the user can’t do anything about it other than manually finding and editing the settings json file.
ACK a1b3ccae4be82297fd20f5be15a03eeb477507d0
Sanity check below.
On master (f07a533dfcb172321972e5afb3b38a4bd24edb87), prevents start:
PR branch (a1b3ccae4be82297fd20f5be15a03eeb477507d0), doesn’t:
Concept ACK a1b3cca
makes sense to give a warning instead of giving an error which is bad ux for a user who has been using -upnp
One thing i wonder though: do we have to explicitly remove the setting from settings.json after this warning, to prevent it happening every startup? Warning the user once is enough.
I agree we should still fix the GUI. However i’m not sure how best to achieve that. Also if we do it it would be nice to make it extensible so next time we remove a startup option we don’t have to go through this again.
One hack would be to make the ArgsManager passed to AppInitParameterInteraction non-const and after showing the warning dropping it from the settings and overwriting the settings.json file there:
https://github.com/bitcoin/bitcoin/blob/f1bcf3edc5027b501616670db33d8be1f2cb5a11/src/init.cpp#L866-L877
Another way could be to never persist hidden args in the settings, and we always keep startup options we remove as hidden args for a couple versions?
Another way could be to never persist hidden args in the settings, and we always keep startup options we remove as hidden args for a couple versions?
Agree that a consistent way would be better.
i’m not sure about re-using hidden. Could we maybe have a “REMOVED” flag on arguments specifically for this “don’t complain on load, but don’t persist” behavior? Could keep this around for a long time after removal, potentially, even after removing the warning code, to make sure users never run into this issue when upgrading.
(this would only affect settings.json–providing it on the command line, or explicltly in bitcoin.conf would then cause a normal “unknown arg” warning)
