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.
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?
InitError to InitWarning, as the alert shouldn’t be fatal, as it is possible to continue without it. (This applies to bitcoind as well)
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.
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?
This is tagged for 29.x, but it’s not exactly clear what needs doing, or if anyone is working on it.
Anyone migrating from 28.x to 29.x, who previously had the upnp/natpmp options selected, will currently see this at every startup, with no way to fix from the UI, as far as I understand:
with no way to fix from the UI
Can’t they just unset it in the UI settings?
Can’t they just unset it in the UI settings?
No, because the checkbox for upnp no longer exists.
Looks like this is still up for grabs, waiting on a pull request for 29.x.
Can’t they just unset it in the UI settings?
No, because the checkbox for upnp no longer exists.
Also not with a previous release as a workaround?
@laanwj did you get a chance to look into this? Else i guess it falls on me to take care of this before deadline..
Whoops. No i didn’t, i lost track of the deadline (which, assuming this is considered a bugfix is 2025-03-06, according to #31029), sorry. i will have a look at it today and tomorrow, if i don’t manage to figure out this week, it would be great if you can take it over.
Sounds good, thanks for looking into it.
——– Original Message ——– On 2/20/25 8:17 AM, laanwj wrote:
@.***(https://github.com/laanwj) did you get a chance to look into this? Else i guess it falls on me to take care of this before deadline..
Whoops. No i didn’t, i lost track of the deadline (which, assuming this is considered a bugfix is 2025-03-06, according to #31029), sorry. i will have a look at it today and tomorrow, if i don’t manage to figure out this week, it would be great if you can take it over.
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
[laanwj]laanwj left a comment (bitcoin-core/gui#843)
@.***(https://github.com/laanwj) did you get a chance to look into this? Else i guess it falls on me to take care of this before deadline..
Whoops. No i didn’t, i lost track of the deadline (which, assuming this is considered a bugfix is 2025-03-06, according to #31029), sorry. i will have a look at it today and tomorrow, if i don’t manage to figure out this week, it would be great if you can take it over.
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
I’m not sure there is a strong justification for the -upnp option to trigger an interactive warning now, if it was always just a best-effort setting that wouldn’t previously fail if it couldn’t register with the upnp server., or the server wouldn’t provide a port or external ip. Since anyone relying on this would need external monitoring or testing to know if it was working anyway, just using LogWarning here seems like it should be ok.
But if I’m underestimating the importance of having an interactive warning, it would be possible to trigger it only when the upnp setting is enabled on the command line or config file, not when the setting is specified in the settings.json file by adding an argsmanager method to check if a setting is present in Settings::command_line_options or Settings::ro_config maps.
@ryanofsky For this specific setting it isn’t too important imo. It would be critical for a disappearing privacy setting, but not for a setting that opens up more network surface.
But i do think it would be nice to warn once before dropping it from settings.json. i mean our goal with this change is to encourage people to enable port forwarding to have more connectable nodes. So a reminder to use NAT-PMP instead instead is good.
Relatedly, I think QSetttings migration code could be also be improved: https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1963647337
And I think the warning message could be much more descriptive because it’s possible someone enabled this setting a long time ago without a full understanding of what it does, and the message doesn’t provide any context. Might suggest a warning like:
But i do think it would be nice to warn once before dropping it from
settings.json. i mean our goal with this change is to encourage people to enable port forwarding to have more connectable nodes. So a reminder to use NAT-PMP instead instead is good.
This also does sound like a good idea, and should not be too hard to implement. though not quite as easy as replacing InitWarning with LogWarning. It could also be done at any point in the future after changing InitWarning to LogWarning.
-upnp to -natpmp . With a LogWarning but not an UI warning. i don’t think most users care about what method of automatic port forwarding method is used?