diff options
author | Andrew Chow <achow101-github@achow101.com> | 2022-03-09 10:54:42 -0500 |
---|---|---|
committer | Andrew Chow <achow101-github@achow101.com> | 2022-03-09 10:54:48 -0500 |
commit | 47bbd3ff4f5e0c04da4731c5d26d23d97cfa0bf1 (patch) | |
tree | 37763c2d997bd5d74e63126fa584d6f264c6aadc /src | |
parent | 7003b6ab24f6adfffd71d7b7d4182afde52ff859 (diff) | |
parent | 5b1aae12ca4a99c6b09349981a4902717a6a5d3e (diff) | |
download | bitcoin-47bbd3ff4f5e0c04da4731c5d26d23d97cfa0bf1.tar.xz |
Merge bitcoin/bitcoin#24498: qt: Avoid crash on startup if int specified in settings.json
5b1aae12ca4a99c6b09349981a4902717a6a5d3e qt: Avoid crash on startup if int specified in settings.json (Ryan Ofsky)
84b0973e35dae63cd1b60199b481e24d54e58c97 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 https://github.com/bitcoin/bitcoin/issues/24457#issuecomment-1059825678. 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 https://github.com/bitcoin/bitcoin/issues/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 5b1aae12ca4a99c6b09349981a4902717a6a5d3e
achow101:
ACK 5b1aae12ca4a99c6b09349981a4902717a6a5d3e
jonatack:
Code review ACK 5b1aae12ca4a99c6b09349981a4902717a6a5d3e
Tree-SHA512: 958991b4bead9b82a3879fdca0f8d6405e2a212b7c46cf356f078843a4f156e27fd75fc46e2013aa5159582ead06d343c1ed248d678b3e5bbd312f247e37894c
Diffstat (limited to 'src')
-rw-r--r-- | src/Makefile.qttest.include | 3 | ||||
-rw-r--r-- | src/qt/test/optiontests.cpp | 31 | ||||
-rw-r--r-- | src/qt/test/optiontests.h | 25 | ||||
-rw-r--r-- | src/qt/test/test_main.cpp | 5 | ||||
-rw-r--r-- | src/test/getarg_tests.cpp | 112 | ||||
-rw-r--r-- | src/util/system.cpp | 2 |
6 files changed, 177 insertions, 1 deletions
diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include index 8e6fa2eb0d..29c322fbc2 100644 --- a/src/Makefile.qttest.include +++ b/src/Makefile.qttest.include @@ -7,6 +7,7 @@ TESTS += qt/test/test_bitcoin-qt TEST_QT_MOC_CPP = \ qt/test/moc_apptests.cpp \ + qt/test/moc_optiontests.cpp \ qt/test/moc_rpcnestedtests.cpp \ qt/test/moc_uritests.cpp @@ -19,6 +20,7 @@ endif # ENABLE_WALLET TEST_QT_H = \ qt/test/addressbooktests.h \ qt/test/apptests.h \ + qt/test/optiontests.h \ qt/test/rpcnestedtests.h \ qt/test/uritests.h \ qt/test/util.h \ @@ -30,6 +32,7 @@ qt_test_test_bitcoin_qt_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BITCOIN_ qt_test_test_bitcoin_qt_SOURCES = \ init/bitcoin-qt.cpp \ qt/test/apptests.cpp \ + qt/test/optiontests.cpp \ qt/test/rpcnestedtests.cpp \ qt/test/test_main.cpp \ qt/test/uritests.cpp \ diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp new file mode 100644 index 0000000000..51894e1915 --- /dev/null +++ b/src/qt/test/optiontests.cpp @@ -0,0 +1,31 @@ +// Copyright (c) 2018 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include <qt/bitcoin.h> +#include <qt/test/optiontests.h> +#include <test/util/setup_common.h> +#include <util/system.h> + +#include <QSettings> +#include <QTest> + +#include <univalue.h> + +//! Entry point for BitcoinApplication tests. +void OptionTests::optionTests() +{ + // Test regression https://github.com/bitcoin/bitcoin/issues/24457. Ensure + // that setting integer prune value doesn't cause an exception to be thrown + // in the OptionsModel constructor + gArgs.LockSettings([&](util::Settings& settings) { + settings.forced_settings.erase("prune"); + settings.rw_settings["prune"] = 3814; + }); + gArgs.WriteSettingsFile(); + OptionsModel{}; + gArgs.LockSettings([&](util::Settings& settings) { + settings.rw_settings.erase("prune"); + }); + gArgs.WriteSettingsFile(); +} diff --git a/src/qt/test/optiontests.h b/src/qt/test/optiontests.h new file mode 100644 index 0000000000..779d4cc209 --- /dev/null +++ b/src/qt/test/optiontests.h @@ -0,0 +1,25 @@ +// Copyright (c) 2019 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_QT_TEST_OPTIONTESTS_H +#define BITCOIN_QT_TEST_OPTIONTESTS_H + +#include <qt/optionsmodel.h> + +#include <QObject> + +class OptionTests : public QObject +{ + Q_OBJECT +public: + explicit OptionTests(interfaces::Node& node) : m_node(node) {} + +private Q_SLOTS: + void optionTests(); + +private: + interfaces::Node& m_node; +}; + +#endif // BITCOIN_QT_TEST_OPTIONTESTS_H diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp index 10b7e2ffe7..07d256f05a 100644 --- a/src/qt/test/test_main.cpp +++ b/src/qt/test/test_main.cpp @@ -10,6 +10,7 @@ #include <interfaces/node.h> #include <qt/bitcoin.h> #include <qt/test/apptests.h> +#include <qt/test/optiontests.h> #include <qt/test/rpcnestedtests.h> #include <qt/test/uritests.h> #include <test/util/setup_common.h> @@ -89,6 +90,10 @@ int main(int argc, char* argv[]) if (QTest::qExec(&app_tests) != 0) { fInvalid = true; } + OptionTests options_tests(app.node()); + if (QTest::qExec(&options_tests) != 0) { + fInvalid = true; + } URITests test1; if (QTest::qExec(&test1) != 0) { fInvalid = true; diff --git a/src/test/getarg_tests.cpp b/src/test/getarg_tests.cpp index 76a65ec528..c877105fe7 100644 --- a/src/test/getarg_tests.cpp +++ b/src/test/getarg_tests.cpp @@ -3,6 +3,8 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <test/util/setup_common.h> +#include <univalue.h> +#include <util/settings.h> #include <util/strencodings.h> #include <util/system.h> @@ -41,6 +43,116 @@ void SetupArgs(ArgsManager& local_args, const std::vector<std::pair<std::string, } } +// Test behavior of GetArg functions when string, integer, and boolean types +// are specified in the settings.json file. GetArg functions are convenience +// functions. The GetSetting method can always be used instead of GetArg +// methods to retrieve original values, and there's not always an objective +// answer to what GetArg behavior is best in every case. This test makes sure +// there's test coverage for whatever the current behavior is, so it's not +// broken or changed unintentionally. +BOOST_AUTO_TEST_CASE(setting_args) +{ + ArgsManager args; + SetupArgs(args, {{"-foo", ArgsManager::ALLOW_ANY}}); + + auto set_foo = [&](const util::SettingsValue& value) { + args.LockSettings([&](util::Settings& settings) { + settings.rw_settings["foo"] = value; + }); + }; + + set_foo("str"); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"str\""); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "str"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), false); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), false); + + set_foo("99"); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"99\""); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "99"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 99); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), true); + + set_foo("3.25"); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"3.25\""); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "3.25"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 3); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), true); + + set_foo("0"); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"0\""); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "0"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), false); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), false); + + set_foo(""); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "\"\""); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), ""); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), true); + + set_foo(99); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "99"); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "99"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 99); + BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error); + + set_foo(3.25); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "3.25"); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "3.25"); + BOOST_CHECK_THROW(args.GetIntArg("foo", 100), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error); + + set_foo(0); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "0"); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "0"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0); + BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error); + + set_foo(true); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "true"); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "1"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 1); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), true); + + set_foo(false); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "false"); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "0"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 0); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), false); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), false); + + set_foo(UniValue::VOBJ); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "{}"); + BOOST_CHECK_THROW(args.GetArg("foo", "default"), std::runtime_error); + BOOST_CHECK_THROW(args.GetIntArg("foo", 100), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error); + + set_foo(UniValue::VARR); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "[]"); + BOOST_CHECK_THROW(args.GetArg("foo", "default"), std::runtime_error); + BOOST_CHECK_THROW(args.GetIntArg("foo", 100), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", true), std::runtime_error); + BOOST_CHECK_THROW(args.GetBoolArg("foo", false), std::runtime_error); + + set_foo(UniValue::VNULL); + BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "null"); + BOOST_CHECK_EQUAL(args.GetArg("foo", "default"), "default"); + BOOST_CHECK_EQUAL(args.GetIntArg("foo", 100), 100); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", true), true); + BOOST_CHECK_EQUAL(args.GetBoolArg("foo", false), false); +} + BOOST_AUTO_TEST_CASE(boolarg) { ArgsManager local_args; diff --git a/src/util/system.cpp b/src/util/system.cpp index 1ad701c56d..8e45453d31 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -591,7 +591,7 @@ bool ArgsManager::IsArgNegated(const std::string& strArg) const std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const { const util::SettingsValue value = GetSetting(strArg); - return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str(); + return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.isNum() ? value.getValStr() : value.get_str(); } int64_t ArgsManager::GetIntArg(const std::string& strArg, int64_t nDefault) const |