From 84b0973e35dae63cd1b60199b481e24d54e58c97 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 7 Mar 2022 13:29:46 -0500 Subject: [PATCH 1/2] test: Add tests for GetArg methods / settings.json type coercion Just add tests. No changes to application behavior. Tests will be updated in the next commit changing & improving current behavior. Include a Qt test for GUI startup crash reported by Rspigler in https://github.com/bitcoin/bitcoin/issues/24457 caused by GetArg behavior 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). --- .../test_bitcoin-qt/test_bitcoin-qt.vcxproj | 3 + src/Makefile.qttest.include | 3 + src/qt/test/optiontests.cpp | 31 +++++ src/qt/test/optiontests.h | 25 ++++ src/qt/test/test_main.cpp | 5 + src/test/getarg_tests.cpp | 112 ++++++++++++++++++ 6 files changed, 179 insertions(+) create mode 100644 src/qt/test/optiontests.cpp create mode 100644 src/qt/test/optiontests.h diff --git a/build_msvc/test_bitcoin-qt/test_bitcoin-qt.vcxproj b/build_msvc/test_bitcoin-qt/test_bitcoin-qt.vcxproj index d3be693e994..ec572b4f2ed 100644 --- a/build_msvc/test_bitcoin-qt/test_bitcoin-qt.vcxproj +++ b/build_msvc/test_bitcoin-qt/test_bitcoin-qt.vcxproj @@ -12,6 +12,7 @@ + @@ -20,6 +21,7 @@ + @@ -88,6 +90,7 @@ + diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include index 8e6fa2eb0d7..29c322fbc2e 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 00000000000..80d8d77984f --- /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 +#include +#include +#include + +#include +#include + +#include + +//! Entry point for BitcoinApplication tests. +void OptionTests::optionTests() +{ + // Test regression https://github.com/bitcoin/bitcoin/issues/24457. Check + // if setting an integer prune value causes 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(); + QVERIFY_EXCEPTION_THROWN(OptionsModel{}, std::runtime_error); + 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 00000000000..779d4cc209a --- /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 + +#include + +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 10b7e2ffe78..07d256f05ae 100644 --- a/src/qt/test/test_main.cpp +++ b/src/qt/test/test_main.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -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 76a65ec528b..4dc12ec7d74 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 +#include +#include #include #include @@ -41,6 +43,116 @@ void SetupArgs(ArgsManager& local_args, const std::vector Date: Mon, 7 Mar 2022 13:29:46 -0500 Subject: [PATCH 2/2] qt: Avoid crash on startup if int specified in settings.json 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). Fix is a one-line change in ArgsManager::GetArg. --- src/qt/test/optiontests.cpp | 8 ++++---- src/test/getarg_tests.cpp | 6 +++--- src/util/system.cpp | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qt/test/optiontests.cpp b/src/qt/test/optiontests.cpp index 80d8d77984f..51894e1915f 100644 --- a/src/qt/test/optiontests.cpp +++ b/src/qt/test/optiontests.cpp @@ -15,15 +15,15 @@ //! Entry point for BitcoinApplication tests. void OptionTests::optionTests() { - // Test regression https://github.com/bitcoin/bitcoin/issues/24457. Check - // if setting an integer prune value causes an exception to be thrown in - // the OptionsModel constructor. + // 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(); - QVERIFY_EXCEPTION_THROWN(OptionsModel{}, std::runtime_error); + OptionsModel{}; gArgs.LockSettings([&](util::Settings& settings) { settings.rw_settings.erase("prune"); }); diff --git a/src/test/getarg_tests.cpp b/src/test/getarg_tests.cpp index 4dc12ec7d74..c877105fe78 100644 --- a/src/test/getarg_tests.cpp +++ b/src/test/getarg_tests.cpp @@ -98,21 +98,21 @@ BOOST_AUTO_TEST_CASE(setting_args) set_foo(99); BOOST_CHECK_EQUAL(args.GetSetting("foo").write(), "99"); - BOOST_CHECK_THROW(args.GetArg("foo", "default"), std::runtime_error); + 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_THROW(args.GetArg("foo", "default"), std::runtime_error); + 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_THROW(args.GetArg("foo", "default"), std::runtime_error); + 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); diff --git a/src/util/system.cpp b/src/util/system.cpp index 1ad701c56d9..8e45453d310 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