Gracefully handle dropped UPnP support #843

issue Sjors openend this issue on October 30, 2024
  1. Sjors commented at 9:57 pm on October 30, 2024: member

    https://github.com/bitcoin/bitcoin/pull/31130 dropped UPnP support. It’s now recommended that users use PCP (with NATPMP fallback).

    But this is not explained in a friendly manner:

    A user would have to figure out that they need to manually edit settings.json or delete it and redo all their settings.

    We should probably automatically delete it from settings.json. And then display a warning / info (instead of error) that tells users to consider the PCP setting in the GUI.

  2. laanwj commented at 7:41 am on October 31, 2024: member

    Even ignoring the spurious setting would be better in the GUI case. No need to rub this in the users face. i didn’t expect this consequence when i suggested adding a message 😅

    Though it’s still better than creashing with generic “-upnp: Unknown setting”. i guess…

    But yes this needs to be informational level at most, it should just continue.

    Even worse: i guess it even happens if the settings json specifies upnp=0 to explicitly disable it?

  3. laanwj added this to the milestone 29.0 on Oct 31, 2024
  4. maflcko commented at 10:11 am on October 31, 2024: contributor
    An easy first fix would be to downgrade InitError to InitWarning, as the alert shouldn’t be fatal, as it is possible to continue without it. (This applies to bitcoind as well)
  5. darosior commented at 1:35 pm on October 31, 2024: member

    Thanks Maflcko, i think it’s a good short term solution to fix what i just broke. I’ll open a PR shortly.

    That said, it does seem unknown entries in settings.json should be ignored by the GUI as it’s a file the user isn’t supposed to edit? Right now it’s assuming we will never remove a startup option, which isn’t very robust.

  6. Sjors commented at 4:32 pm on October 31, 2024: member
    @darosior I could imagine a scenario in which we remove a Tor related setting and it would be unsafe to simply ignore it. But that’s not the case here.
  7. laanwj commented at 7:21 pm on October 31, 2024: member

    An easy first fix would be to downgrade InitError to InitWarning, as the alert shouldn’t be fatal, as it is possible to continue without it. (This applies to bitcoind as well)

    Do we also need to explicitly remote the entry from settings.json as well, to make sure the message doesn’t appear at every start after that. Or does this happen automatically in re-saving it (at shutdown) somehow?

  8. achow101 closed this on Nov 1, 2024

  9. achow101 referenced this in commit 975b115e1a on Nov 1, 2024
  10. maflcko commented at 11:35 am on November 2, 2024: contributor
    I don’t think this was fixed fully for the gui
  11. maflcko reopened this on Nov 2, 2024


Sjors laanwj maflcko darosior

Milestone
29.0


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-26 10:20 UTC

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