Skip to content

Commit 47bbd3f

Browse files
achow101achow101
committed
Merge bitcoin/bitcoin#24498: qt: Avoid crash on startup if int specified in settings.json
5b1aae1 qt: Avoid crash on startup if int specified in settings.json (Ryan Ofsky) 84b0973 test: Add tests for GetArg methods / settings.json type coercion (Ryan Ofsky) Pull request description: Should probably add this change to 23.x as suggested by Luke bitcoin/bitcoin#24457 (comment). If settings like `prune` are added to `settings.json` in the future, it would be preferable for 23.x releases to respect the setting instead of crash. --- Fix GUI startup crash reported by Rspigler in bitcoin/bitcoin#24457 that happens if `settings.json` contains an integer value for any of the configuration options which GUI settings can currently clash with (-dbcache, -par, -spendzeroconfchange, -signer, -upnp, -natpmp, -listen, -server, -proxy, -proxy, -onion, -onion, -lang, and -prune). The fix is a one-line change in `ArgsManager::GetArg`. The rest of the PR just adds a regression test for the GUI and unit tests for ArgsManager::GetArg methods. ACKs for top commit: laanwj: Code review ACK 5b1aae1 achow101: ACK 5b1aae1 jonatack: Code review ACK 5b1aae1 Tree-SHA512: 958991b4bead9b82a3879fdca0f8d6405e2a212b7c46cf356f078843a4f156e27fd75fc46e2013aa5159582ead06d343c1ed248d678b3e5bbd312f247e37894c
2 parents 7003b6a + 5b1aae1 commit 47bbd3f

File tree

7 files changed

+180
-1
lines changed

7 files changed

+180
-1
lines changed

‎build_msvc/test_bitcoin-qt/test_bitcoin-qt.vcxproj

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
<ClCompile Include="..\..\src\test\util\setup_common.cpp" />
1313
<ClCompile Include="..\..\src\qt\test\addressbooktests.cpp" />
1414
<ClCompile Include="..\..\src\qt\test\apptests.cpp" />
15+
<ClCompile Include="..\..\src\qt\test\optiontests.cpp" />
1516
<ClCompile Include="..\..\src\qt\test\rpcnestedtests.cpp" />
1617
<ClCompile Include="..\..\src\qt\test\test_main.cpp" />
1718
<ClCompile Include="..\..\src\qt\test\uritests.cpp" />
@@ -20,6 +21,7 @@
2021
<ClCompile Include="..\..\src\wallet\test\wallet_test_fixture.cpp" />
2122
<ClCompile Include="$(GeneratedFilesOutDir)\moc\moc_addressbooktests.cpp" />
2223
<ClCompile Include="$(GeneratedFilesOutDir)\moc\moc_apptests.cpp" />
24+
<ClCompile Include="$(GeneratedFilesOutDir)\moc\moc_optiontests.cpp" />
2325
<ClCompile Include="$(GeneratedFilesOutDir)\moc\moc_rpcnestedtests.cpp" />
2426
<ClCompile Include="$(GeneratedFilesOutDir)\moc\moc_uritests.cpp" />
2527
<ClCompile Include="$(GeneratedFilesOutDir)\moc\moc_wallettests.cpp" />
@@ -88,6 +90,7 @@
8890
<ItemGroup>
8991
<MocTestFiles Include="..\..\src\qt\test\addressbooktests.h" />
9092
<MocTestFiles Include="..\..\src\qt\test\apptests.h" />
93+
<MocTestFiles Include="..\..\src\qt\test\optiontests.h" />
9194
<MocTestFiles Include="..\..\src\qt\test\rpcnestedtests.h" />
9295
<MocTestFiles Include="..\..\src\qt\test\uritests.h" />
9396
<MocTestFiles Include="..\..\src\qt\test\wallettests.h" />

‎src/Makefile.qttest.include

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ TESTS += qt/test/test_bitcoin-qt
77

88
TEST_QT_MOC_CPP = \
99
qt/test/moc_apptests.cpp \
10+
qt/test/moc_optiontests.cpp \
1011
qt/test/moc_rpcnestedtests.cpp \
1112
qt/test/moc_uritests.cpp
1213

@@ -19,6 +20,7 @@ endif # ENABLE_WALLET
1920
TEST_QT_H = \
2021
qt/test/addressbooktests.h \
2122
qt/test/apptests.h \
23+
qt/test/optiontests.h \
2224
qt/test/rpcnestedtests.h \
2325
qt/test/uritests.h \
2426
qt/test/util.h \
@@ -30,6 +32,7 @@ qt_test_test_bitcoin_qt_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BITCOIN_
3032
qt_test_test_bitcoin_qt_SOURCES = \
3133
init/bitcoin-qt.cpp \
3234
qt/test/apptests.cpp \
35+
qt/test/optiontests.cpp \
3336
qt/test/rpcnestedtests.cpp \
3437
qt/test/test_main.cpp \
3538
qt/test/uritests.cpp \

‎src/qt/test/optiontests.cpp

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
// Copyright (c) 2018 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <qt/bitcoin.h>
6+
#include <qt/test/optiontests.h>
7+
#include <test/util/setup_common.h>
8+
#include <util/system.h>
9+
10+
#include <QSettings>
11+
#include <QTest>
12+
13+
#include <univalue.h>
14+
15+
//! Entry point for BitcoinApplication tests.
16+
void OptionTests::optionTests()
17+
{
18+
// Test regression https://github.com/bitcoin/bitcoin/issues/24457. Ensure
19+
// that setting integer prune value doesn't cause an exception to be thrown
20+
// in the OptionsModel constructor
21+
gArgs.LockSettings([&](util::Settings& settings) {
22+
settings.forced_settings.erase("prune");
23+
settings.rw_settings["prune"] = 3814;
24+
});
25+
gArgs.WriteSettingsFile();
26+
OptionsModel{};
27+
gArgs.LockSettings([&](util::Settings& settings) {
28+
settings.rw_settings.erase("prune");
29+
});
30+
gArgs.WriteSettingsFile();
31+
}

‎src/qt/test/optiontests.h

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// Copyright (c) 2019 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#ifndef BITCOIN_QT_TEST_OPTIONTESTS_H
6+
#define BITCOIN_QT_TEST_OPTIONTESTS_H
7+
8+
#include <qt/optionsmodel.h>
9+
10+
#include <QObject>
11+
12+
class OptionTests : public QObject
13+
{
14+
Q_OBJECT
15+
public:
16+
explicit OptionTests(interfaces::Node& node) : m_node(node) {}
17+
18+
private Q_SLOTS:
19+
void optionTests();
20+
21+
private:
22+
interfaces::Node& m_node;
23+
};
24+
25+
#endif // BITCOIN_QT_TEST_OPTIONTESTS_H

‎src/qt/test/test_main.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include <interfaces/node.h>
1111
#include <qt/bitcoin.h>
1212
#include <qt/test/apptests.h>
13+
#include <qt/test/optiontests.h>
1314
#include <qt/test/rpcnestedtests.h>
1415
#include <qt/test/uritests.h>
1516
#include <test/util/setup_common.h>
@@ -89,6 +90,10 @@ int main(int argc, char* argv[])
8990
if (QTest::qExec(&app_tests) != 0) {
9091
fInvalid = true;
9192
}
93+
OptionTests options_tests(app.node());
94+
if (QTest::qExec(&options_tests) != 0) {
95+
fInvalid = true;
96+
}
9297
URITests test1;
9398
if (QTest::qExec(&test1) != 0) {
9499
fInvalid = true;

‎src/test/getarg_tests.cpp

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
44

55
#include <test/util/setup_common.h>
6+
#include <univalue.h>
7+
#include <util/settings.h>
68
#include <util/strencodings.h>
79
#include <util/system.h>
810

@@ -41,6 +43,116 @@ void SetupArgs(ArgsManager& local_args, const std::vector<std::pair<std::string,
4143
}
4244
}
4345

46+
// Test behavior of GetArg functions when string, integer, and boolean types
47+
// are specified in the settings.json file. GetArg functions are convenience
48+
// functions. The GetSetting method can always be used instead of GetArg
49+
// methods to retrieve original values, and there's not always an objective
50+
// answer to what GetArg behavior is best in every case. This test makes sure
51+
// there's test coverage for whatever the current behavior is, so it's not
52+
// broken or changed unintentionally.
53+
BOOST_AUTO_TEST_CASE(setting_args)
54+
{
55+
ArgsManager args;
56+
SetupArgs(args, {{"-foo", ArgsManager::ALLOW_ANY}});
57+
58+
auto set_foo = [&](const util::SettingsValue& value) {
59+
args.LockSettings([&](util::Settings& settings) {
60+
settings.rw_settings["foo"] = value;
61+
});
62+
};
63+
64+
set_foo("str");
65+
BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"str\"");
66+
BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "str");
67+
BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0);
68+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), false);
69+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), false);
70+
71+
set_foo("99");
72+
BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"99\"");
73+
BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "99");
74+
BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 99);
75+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true);
76+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), true);
77+
78+
set_foo("3.25");
79+
BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"3.25\"");
80+
BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "3.25");
81+
BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 3);
82+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true);
83+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), true);
84+
85+
set_foo("0");
86+
BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"0\"");
87+
BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "0");
88+
BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0);
89+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), false);
90+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), false);
91+
92+
set_foo("");
93+
BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"\"");
94+
BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "");
95+
BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0);
96+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true);
97+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), true);
98+
99+
set_foo(99);
100+
BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "99");
101+
BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "99");
102+
BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 99);
103+
BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error);
104+
BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error);
105+
106+
set_foo(3.25);
107+
BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "3.25");
108+
BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "3.25");
109+
BOOST_CHECK_THROW(args.GetIntArg("foo", 100), std::runtime_error);
110+
BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error);
111+
BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error);
112+
113+
set_foo(0);
114+
BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "0");
115+
BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "0");
116+
BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0);
117+
BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error);
118+
BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error);
119+
120+
set_foo(true);
121+
BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "true");
122+
BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "1");
123+
BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 1);
124+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true);
125+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), true);
126+
127+
set_foo(false);
128+
BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "false");
129+
BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "0");
130+
BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0);
131+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), false);
132+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), false);
133+
134+
set_foo(UniValue::VOBJ);
135+
BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "{}");
136+
BOOST_CHECK_THROW(args.GetArg("foo", "default"), std::runtime_error);
137+
BOOST_CHECK_THROW(args.GetIntArg("foo", 100), std::runtime_error);
138+
BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error);
139+
BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error);
140+
141+
set_foo(UniValue::VARR);
142+
BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "[]");
143+
BOOST_CHECK_THROW(args.GetArg("foo", "default"), std::runtime_error);
144+
BOOST_CHECK_THROW(args.GetIntArg("foo", 100), std::runtime_error);
145+
BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error);
146+
BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error);
147+
148+
set_foo(UniValue::VNULL);
149+
BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "null");
150+
BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "default");
151+
BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 100);
152+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true);
153+
BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), false);
154+
}
155+
44156
BOOST_AUTO_TEST_CASE(boolarg)
45157
{
46158
ArgsManager local_args;

‎src/util/system.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -591,7 +591,7 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const
591591
std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const
592592
{
593593
const util::SettingsValue value = GetSetting(strArg);
594-
return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str();
594+
return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.isNum() ? value.getValStr() : value.get_str();
595595
}
596596

597597
int64_t ArgsManager::GetIntArg(const std::string& strArg, int64_t nDefault) const

0 commit comments

Comments
 (0)