search

bitcoin-core / gui

Bitcoin Core GUI staging repository
https://github.com/bitcoin/bitcoin
MIT License
604 stars 266 forks source link

Gracefully handle dropped UPnP support #843

Open Sjors opened 3 weeks ago

Sjors commented 3 weeks ago

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:

error

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.

laanwj commented 3 weeks ago

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?

maflcko commented 3 weeks ago

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)

darosior commented 3 weeks ago

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.

Sjors commented 3 weeks ago

@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.

laanwj commented 3 weeks ago

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?

maflcko commented 3 weeks ago

I don't think this was fixed fully for the gui