init: warn, don’t error, when ‘-upnp’ is set #31198

pull darosior wants to merge 1 commits into bitcoin:master from darosior:2024_upnp_unbreak_gui changing 1 files +2 −2
  1. darosior commented at 6:10 pm on October 31, 2024: member

    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.

    Fixes https://github.com/bitcoin-core/gui/issues/843.

  2. init: warn, don't error, when '-upnp' is set
    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.
    a1b3ccae4b
  3. DrahtBot commented at 6:10 pm on October 31, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31198.

    Reviews

    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.

  4. maflcko commented at 6:18 pm on October 31, 2024: member

    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.

  5. tdb3 approved
  6. tdb3 commented at 10:34 pm on October 31, 2024: contributor

    ACK a1b3ccae4be82297fd20f5be15a03eeb477507d0

    Sanity check below.

    On master (f07a533dfcb172321972e5afb3b38a4bd24edb87), prevents start: image

    PR branch (a1b3ccae4be82297fd20f5be15a03eeb477507d0), doesn’t: image

  7. kevkevinpal commented at 11:38 pm on October 31, 2024: contributor

    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

  8. laanwj commented at 1:50 pm on November 1, 2024: member
    Concept ACK. A big improvement. 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.
  9. laanwj added the label P2P on Nov 1, 2024
  10. achow101 commented at 9:09 pm on November 1, 2024: member
    ACK a1b3ccae4be82297fd20f5be15a03eeb477507d0
  11. DrahtBot requested review from laanwj on Nov 1, 2024
  12. achow101 merged this on Nov 1, 2024
  13. achow101 closed this on Nov 1, 2024

  14. darosior deleted the branch on Nov 1, 2024
  15. darosior commented at 11:01 pm on November 1, 2024: member

    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?

  16. laanwj commented at 7:52 am on November 2, 2024: member

    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)

  17. Sjors commented at 3:01 pm on November 8, 2024: member
    cc @ryanofsky who recently refactored a lot of the settings code.
  18. laanwj commented at 1:22 pm on November 9, 2024: member
    Maybe unknown settings in settings.json should never be a fatal startup error. But it also shouldn’t be silently ignored (in case the setting changed important behavior). It could warn once at startup that they are there then remove them. This is probably the most user friendly way, as we don’t really ever want to force GUI users to manually edit a json file?

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me