Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gracefully handle dropped UPnP support #843

Open
Sjors opened this issue Oct 30, 2024 · 6 comments · Fixed by bitcoin/bitcoin#31198
Open

Gracefully handle dropped UPnP support #843

Sjors opened this issue Oct 30, 2024 · 6 comments · Fixed by bitcoin/bitcoin#31198
Milestone

Comments

@Sjors
Copy link
Member

Sjors commented Oct 30, 2024

bitcoin/bitcoin#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
Copy link
Member

laanwj commented Oct 31, 2024

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?

@laanwj laanwj added this to the 29.0 milestone Oct 31, 2024
@maflcko
Copy link
Contributor

maflcko commented Oct 31, 2024

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
Copy link
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.

@Sjors
Copy link
Member Author

Sjors commented Oct 31, 2024

@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
Copy link
Member

laanwj commented Oct 31, 2024

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
Copy link
Contributor

maflcko commented Nov 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants