gui: Fix issue: "default port not shown correctly in settings dialog"

l2a5b1 commented at 11:47 pm on March 8, 2018: contributor

In f05d349 the value of the addrProxy and addrSeparateProxyTor settings is set to an illegal default value, because the value of DEFAULT_GUI_PROXY_PORT is passed to the fieldWidth parameter of the QString QString::arg(const QString &a, int fieldWidth = 0, QChar fillChar = QLatin1Char( ' ' )) const method:

https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L129

https://github.com/bitcoin/bitcoin/blob/29fad97c320c892ab6a480c81e2078ec22ab354b/src/qt/optionsmodel.cpp#L139

This will create a default proxy setting that consists of 9053 characters and ends with the string 127.0.0.1:%2.

This PR attempts to resolve #12623 by setting the correct value for the addrProxy and addrSeparateProxyTor settings (i) if the proxy setting does not exist; or (ii) if the proxy setting has an illegal value caused by to the aforementioned bug.

The second condition is only relevant if we don’t want Bitcoin Core 0.16.0 users to explicitly reset their settings to see the correct default proxy port value.

meshcollider added the label GUI on Mar 9, 2018

laanwj commented at 12:48 pm on March 10, 2018: member

Concept ACK, soryr for making such a stupid mistake.

The second condition is only relevant if we don’t want Bitcoin Core 0.16.0 users to explicitly reset their settings to see the correct default proxy port value.

If we add that workaround, let’s at least add a comment that explains that workaround, so that future maintainers know why it’s there and when it can be removed.

l2a5b1 force-pushed on Mar 10, 2018

l2a5b1 commented at 5:18 pm on March 10, 2018: contributor

Thanks @laanwj! I have added a comment to both conditional statements to clarify the purpose of the second boolean condition.

randolf approved

MarcoFalke added this to the milestone 0.16.1 on Mar 11, 2018

fanquake deleted a comment on Mar 15, 2018

in src/qt/optionsmodel.cpp:129 in 9ff17ee9ef outdated

124@@ -125,8 +125,10 @@ void OptionsModel::Init(bool resetSettings)
125 
126     if (!settings.contains("fUseProxy"))
127         settings.setValue("fUseProxy", false);
128-    if (!settings.contains("addrProxy"))
129-        settings.setValue("addrProxy", QString("%1:%2").arg(DEFAULT_GUI_PROXY_HOST, DEFAULT_GUI_PROXY_PORT));
130+    // The second condition in the boolean expression is a workaround to overwrite the 'addrProxy' setting
131+    // in case it has been set to an illegal default value by release 0.16.0 (see issue #12623; PR #12650).

Sjors commented at 4:52 pm on April 3, 2018:

Move to OptionsModel::checkAndMigrate()?

Sjors commented at 4:53 pm on April 3, 2018: member

Concept ACK

l2a5b1 force-pushed on Apr 6, 2018

l2a5b1 commented at 10:45 pm on April 6, 2018: contributor

He @Sjors, thanks! I took a stab at addressing your suggestion in 6bb2adb and rebased this feature branch onto master. I committed on top of 87bd26b in case you prefer that version and want me to revert the changes.

l2a5b1 force-pushed on Apr 7, 2018

in src/qt/optionsmodel.cpp:497 in 0592609eb6 outdated

492@@ -485,4 +493,16 @@ void OptionsModel::checkAndMigrate()
493 
494         settings.setValue(strSettingsVersionKey, CLIENT_VERSION);
495     }
496+
497+    // Overwrite the 'addrProxy' setting in case it has been set to an illegal

MarcoFalke commented at 2:43 pm on April 7, 2018:

Trailing whitespace in this line

l2a5b1 force-pushed on Apr 7, 2018

l2a5b1 commented at 9:07 pm on April 7, 2018: contributor

Thanks @MarcoFalke! 1cd545e is rebased onto the master (048ac8326) and addresses the trailing whitespace character.

l2a5b1 force-pushed on Apr 7, 2018

l2a5b1 commented at 10:13 pm on April 7, 2018: contributor

squashed previous work in 2f3ce02, which is based on @Sjors’ suggestion:

Move to OptionsModel::checkAndMigrate()?

Sjors approved

Sjors commented at 9:51 am on April 9, 2018: member

tACK 2f3ce02

Nit: maybe change “Addresses” to “Fix” in your commit message, if only so it fits in 72 characters.

jonasschnelli commented at 6:30 pm on April 10, 2018: contributor

utACK 2f3ce024eb36f93f82f4fc644d57c45639b9f3de

Nods for new migration/auto-fix code without EOL strategy.

Fix illegal default `addProxy` and `addrSeparateProxyTor` settings. 40c58866c7

l2a5b1 force-pushed on Apr 10, 2018

l2a5b1 commented at 8:31 pm on April 10, 2018: contributor

Thanks @jonasschnelli, @sjors! @sjors: I have addressed your feedback and changed the commit message in 40c5886 to make it fit in 72 characters.

Sjors commented at 10:37 am on April 11, 2018: member

reACK 40c58866c7e7af3582cefb9b1810a20902e44167

laanwj merged this on Apr 11, 2018

laanwj closed this on Apr 11, 2018

laanwj referenced this in commit f15b72f482 on Apr 11, 2018

MarcoFalke deleted a comment on Apr 11, 2018

MarcoFalke added the label Needs backport on Apr 11, 2018

MarcoFalke commented at 11:30 pm on April 11, 2018: member

Tagged for backport in case there is a 16.1 release

fanquake referenced this in commit 882f520642 on Apr 12, 2018

fanquake referenced this in commit 298b92596e on Apr 16, 2018

MarcoFalke referenced this in commit 4b9eb00936 on Apr 20, 2018

fanquake referenced this in commit f118a7a35b on Apr 26, 2018

laanwj referenced this in commit feba12fe85 on May 16, 2018

fanquake removed the label Needs backport on May 16, 2018

fanquake commented at 2:28 pm on May 16, 2018: member

Backported in #12967

HashUnlimited referenced this in commit afcfda04cd on Jun 29, 2018

ccebrecos referenced this in commit 74f25659ba on Sep 14, 2018

Fabcien referenced this in commit 97ff16fa0f on Aug 30, 2019

jonspock referenced this in commit 5932fd780f on Dec 8, 2019

jonspock referenced this in commit 102093c190 on Dec 8, 2019

proteanx referenced this in commit 47bc951e08 on Dec 12, 2019

PastaPastaPasta referenced this in commit 547aef1b0b on Apr 3, 2020

ckti referenced this in commit 82290b48bf on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021

gui: Fix issue: “default port not shown correctly in settings dialog” #12650