From 81ea66c30e2953dee24d5b127c28daa0d9452a28 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Fri, 27 Sep 2019 07:31:44 -0400 Subject: [PATCH 001/183] Drop signal CClientUIInterface::LoadWallet --- src/dummywallet.cpp | 10 ++++++++-- src/interfaces/chain.cpp | 1 - src/interfaces/chain.h | 5 +---- src/interfaces/handler.cpp | 14 ++++++++++++++ src/interfaces/handler.h | 4 ++++ src/interfaces/node.cpp | 5 ++--- src/ui_interface.cpp | 3 --- src/ui_interface.h | 7 ------- src/wallet/wallet.cpp | 15 ++++++++++++++- src/wallet/wallet.h | 3 +++ 10 files changed, 46 insertions(+), 21 deletions(-) diff --git a/src/dummywallet.cpp b/src/dummywallet.cpp index 0edcb0286dca..100f2d5dde91 100644 --- a/src/dummywallet.cpp +++ b/src/dummywallet.cpp @@ -11,6 +11,8 @@ enum class WalletCreationStatus; namespace interfaces { class Chain; +class Handler; +class Wallet; } class DummyWalletInit : public WalletInitInterface { @@ -80,9 +82,13 @@ WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& throw std::logic_error("Wallet function called in non-wallet build."); } -namespace interfaces { +using LoadWalletFn = std::function wallet)>; +std::unique_ptr HandleLoadWallet(LoadWalletFn load_wallet) +{ + throw std::logic_error("Wallet function called in non-wallet build."); +} -class Wallet; +namespace interfaces { std::unique_ptr MakeWallet(const std::shared_ptr& wallet) { diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index b2c20573fb66..9d54e64ec26b 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -342,7 +342,6 @@ class ChainImpl : public Chain void initMessage(const std::string& message) override { ::uiInterface.InitMessage(message); } void initWarning(const std::string& message) override { InitWarning(message); } void initError(const std::string& message) override { InitError(message); } - void loadWallet(std::unique_ptr wallet) override { ::uiInterface.LoadWallet(wallet); } void showProgress(const std::string& title, int progress, bool resume_possible) override { ::uiInterface.ShowProgress(title, progress, resume_possible); diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 73a78e21fb31..894a7c433ec5 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -43,7 +43,7 @@ class Wallet; //! asynchronously //! (https://github.com/bitcoin/bitcoin/pull/10973#issuecomment-380101269). //! -//! * The initMessages() and loadWallet() methods which the wallet uses to send +//! * The initMessage() and showProgress() methods which the wallet uses to send //! notifications to the GUI should go away when GUI and wallet can directly //! communicate with each other without going through the node //! (https://github.com/bitcoin/bitcoin/pull/15288#discussion_r253321096). @@ -213,9 +213,6 @@ class Chain //! Send init error. virtual void initError(const std::string& message) = 0; - //! Send wallet load notification to the GUI. - virtual void loadWallet(std::unique_ptr wallet) = 0; - //! Send progress indicator. virtual void showProgress(const std::string& title, int progress, bool resume_possible) = 0; diff --git a/src/interfaces/handler.cpp b/src/interfaces/handler.cpp index 92601fc4e99c..4e235688fee6 100644 --- a/src/interfaces/handler.cpp +++ b/src/interfaces/handler.cpp @@ -22,6 +22,15 @@ class HandlerImpl : public Handler boost::signals2::scoped_connection m_connection; }; +class CleanupHandler : public Handler +{ +public: + explicit CleanupHandler(std::function cleanup) : m_cleanup(std::move(cleanup)) {} + ~CleanupHandler() override { if (!m_cleanup) return; m_cleanup(); m_cleanup = nullptr; } + void disconnect() override { if (!m_cleanup) return; m_cleanup(); m_cleanup = nullptr; } + std::function m_cleanup; +}; + } // namespace std::unique_ptr MakeHandler(boost::signals2::connection connection) @@ -29,4 +38,9 @@ std::unique_ptr MakeHandler(boost::signals2::connection connection) return MakeUnique(std::move(connection)); } +std::unique_ptr MakeHandler(std::function cleanup) +{ + return MakeUnique(std::move(cleanup)); +} + } // namespace interfaces diff --git a/src/interfaces/handler.h b/src/interfaces/handler.h index c4c674cac5ae..46918bc22e79 100644 --- a/src/interfaces/handler.h +++ b/src/interfaces/handler.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_INTERFACES_HANDLER_H #define BITCOIN_INTERFACES_HANDLER_H +#include #include namespace boost { @@ -30,6 +31,9 @@ class Handler //! Return handler wrapping a boost signal connection. std::unique_ptr MakeHandler(boost::signals2::connection connection); +//! Return handler wrapping a cleanup function. +std::unique_ptr MakeHandler(std::function cleanup); + } // namespace interfaces #endif // BITCOIN_INTERFACES_HANDLER_H diff --git a/src/interfaces/node.cpp b/src/interfaces/node.cpp index 227ac9f7b914..2c24b02f2362 100644 --- a/src/interfaces/node.cpp +++ b/src/interfaces/node.cpp @@ -42,11 +42,10 @@ std::vector ListWalletDir(); std::vector> GetWallets(); std::shared_ptr LoadWallet(interfaces::Chain& chain, const std::string& name, std::string& error, std::vector& warnings); WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::vector& warnings, std::shared_ptr& result); +std::unique_ptr HandleLoadWallet(interfaces::Node::LoadWalletFn load_wallet); namespace interfaces { -class Wallet; - namespace { class NodeImpl : public Node @@ -282,7 +281,7 @@ class NodeImpl : public Node } std::unique_ptr handleLoadWallet(LoadWalletFn fn) override { - return MakeHandler(::uiInterface.LoadWallet_connect([fn](std::unique_ptr& wallet) { fn(std::move(wallet)); })); + return HandleLoadWallet(std::move(fn)); } std::unique_ptr handleNotifyNumConnectionsChanged(NotifyNumConnectionsChangedFn fn) override { diff --git a/src/ui_interface.cpp b/src/ui_interface.cpp index d310637145b3..3968464efc5e 100644 --- a/src/ui_interface.cpp +++ b/src/ui_interface.cpp @@ -16,7 +16,6 @@ struct UISignals { boost::signals2::signal NotifyNumConnectionsChanged; boost::signals2::signal NotifyNetworkActiveChanged; boost::signals2::signal NotifyAlertChanged; - boost::signals2::signal LoadWallet; boost::signals2::signal ShowProgress; boost::signals2::signal NotifyBlockTip; boost::signals2::signal NotifyHeaderTip; @@ -36,7 +35,6 @@ ADD_SIGNALS_IMPL_WRAPPER(InitMessage); ADD_SIGNALS_IMPL_WRAPPER(NotifyNumConnectionsChanged); ADD_SIGNALS_IMPL_WRAPPER(NotifyNetworkActiveChanged); ADD_SIGNALS_IMPL_WRAPPER(NotifyAlertChanged); -ADD_SIGNALS_IMPL_WRAPPER(LoadWallet); ADD_SIGNALS_IMPL_WRAPPER(ShowProgress); ADD_SIGNALS_IMPL_WRAPPER(NotifyBlockTip); ADD_SIGNALS_IMPL_WRAPPER(NotifyHeaderTip); @@ -48,7 +46,6 @@ void CClientUIInterface::InitMessage(const std::string& message) { return g_ui_s void CClientUIInterface::NotifyNumConnectionsChanged(int newNumConnections) { return g_ui_signals.NotifyNumConnectionsChanged(newNumConnections); } void CClientUIInterface::NotifyNetworkActiveChanged(bool networkActive) { return g_ui_signals.NotifyNetworkActiveChanged(networkActive); } void CClientUIInterface::NotifyAlertChanged() { return g_ui_signals.NotifyAlertChanged(); } -void CClientUIInterface::LoadWallet(std::unique_ptr& wallet) { return g_ui_signals.LoadWallet(wallet); } void CClientUIInterface::ShowProgress(const std::string& title, int nProgress, bool resume_possible) { return g_ui_signals.ShowProgress(title, nProgress, resume_possible); } void CClientUIInterface::NotifyBlockTip(bool b, const CBlockIndex* i) { return g_ui_signals.NotifyBlockTip(b, i); } void CClientUIInterface::NotifyHeaderTip(bool b, const CBlockIndex* i) { return g_ui_signals.NotifyHeaderTip(b, i); } diff --git a/src/ui_interface.h b/src/ui_interface.h index 9efc2db391f8..f47b56c281d2 100644 --- a/src/ui_interface.h +++ b/src/ui_interface.h @@ -17,10 +17,6 @@ class connection; } } // namespace boost -namespace interfaces { -class Wallet; -} // namespace interfaces - /** General change type (added, updated, removed). */ enum ChangeType { @@ -105,9 +101,6 @@ class CClientUIInterface */ ADD_SIGNALS_DECL_WRAPPER(NotifyAlertChanged, void, ); - /** A wallet has been loaded. */ - ADD_SIGNALS_DECL_WRAPPER(LoadWallet, void, std::unique_ptr& wallet); - /** * Show progress e.g. for verifychain. * resume_possible indicates shutting down now will result in the current progress action resuming upon restart. diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 159d4f78c6db..e19ed95b9da7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -47,6 +47,7 @@ static const size_t OUTPUT_GROUP_MAX_ENTRIES = 10; static CCriticalSection cs_wallets; static std::vector> vpwallets GUARDED_BY(cs_wallets); +static std::list g_load_wallet_fns GUARDED_BY(cs_wallets); bool AddWallet(const std::shared_ptr& wallet) { @@ -89,6 +90,13 @@ std::shared_ptr GetWallet(const std::string& name) return nullptr; } +std::unique_ptr HandleLoadWallet(LoadWalletFn load_wallet) +{ + LOCK(cs_wallets); + auto it = g_load_wallet_fns.emplace(g_load_wallet_fns.end(), std::move(load_wallet)); + return interfaces::MakeHandler([it] { LOCK(cs_wallets); g_load_wallet_fns.erase(it); }); +} + static Mutex g_wallet_release_mutex; static std::condition_variable g_wallet_release_cv; static std::set g_unloading_wallet_set; @@ -4562,7 +4570,12 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, } } - chain.loadWallet(interfaces::MakeWallet(walletInstance)); + { + LOCK(cs_wallets); + for (auto& load_wallet : g_load_wallet_fns) { + load_wallet(interfaces::MakeWallet(walletInstance)); + } + } // Register with the validation interface. It's ok to do this after rescan since we're still holding locked_chain. walletInstance->handleNotifications(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 85c277ff506e..954e74df05e3 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -35,6 +35,8 @@ #include +using LoadWalletFn = std::function wallet)>; + //! Explicitly unload and delete the wallet. //! Blocks the current thread after signaling the unload intent so that all //! wallet clients release the wallet. @@ -48,6 +50,7 @@ bool HasWallets(); std::vector> GetWallets(); std::shared_ptr GetWallet(const std::string& name); std::shared_ptr LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::vector& warnings); +std::unique_ptr HandleLoadWallet(LoadWalletFn load_wallet); enum class WalletCreationStatus { SUCCESS, From 6d6a7a8403ae923f189812edebdd95761de0e7f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Thu, 26 Sep 2019 21:43:44 +0100 Subject: [PATCH 002/183] gui: Fix duplicate wallet showing up The slot BitcoinGUI::addWallet can be invoked twice for the same WalletModel due to a concurrent wallet being loaded after the first `connect()`: ```cpp connect(wallet_controller, &WalletController::walletAdded, this, &BitcoinGUI::addWallet); connect(wallet_controller, &WalletController::walletRemoved, this, &BitcoinGUI::removeWallet); for (WalletModel* wallet_model : m_wallet_controller->getOpenWallets()) { addWallet(wallet_model); ``` --- src/qt/bitcoingui.cpp | 2 +- src/qt/walletframe.cpp | 8 +++++--- src/qt/walletframe.h | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index 2aeba6d82c33..bbf1faa13cf9 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -632,10 +632,10 @@ void BitcoinGUI::setWalletController(WalletController* wallet_controller) void BitcoinGUI::addWallet(WalletModel* walletModel) { if (!walletFrame) return; + if (!walletFrame->addWallet(walletModel)) return; const QString display_name = walletModel->getDisplayName(); setWalletActionsEnabled(true); rpcConsole->addWallet(walletModel); - walletFrame->addWallet(walletModel); m_wallet_selector->addItem(display_name, QVariant::fromValue(walletModel)); if (m_wallet_selector->count() == 2) { m_wallet_selector_label_action->setVisible(true); diff --git a/src/qt/walletframe.cpp b/src/qt/walletframe.cpp index d7f0617315ad..704a85ccc7a9 100644 --- a/src/qt/walletframe.cpp +++ b/src/qt/walletframe.cpp @@ -39,11 +39,11 @@ void WalletFrame::setClientModel(ClientModel *_clientModel) this->clientModel = _clientModel; } -void WalletFrame::addWallet(WalletModel *walletModel) +bool WalletFrame::addWallet(WalletModel *walletModel) { - if (!gui || !clientModel || !walletModel) return; + if (!gui || !clientModel || !walletModel) return false; - if (mapWalletViews.count(walletModel) > 0) return; + if (mapWalletViews.count(walletModel) > 0) return false; WalletView *walletView = new WalletView(platformStyle, this); walletView->setBitcoinGUI(gui); @@ -67,6 +67,8 @@ void WalletFrame::addWallet(WalletModel *walletModel) }); connect(walletView, &WalletView::outOfSyncWarningClicked, this, &WalletFrame::outOfSyncWarningClicked); + + return true; } void WalletFrame::setCurrentWallet(WalletModel* wallet_model) diff --git a/src/qt/walletframe.h b/src/qt/walletframe.h index 156653f47dfa..20fad08b0ef8 100644 --- a/src/qt/walletframe.h +++ b/src/qt/walletframe.h @@ -36,7 +36,7 @@ class WalletFrame : public QFrame void setClientModel(ClientModel *clientModel); - void addWallet(WalletModel *walletModel); + bool addWallet(WalletModel *walletModel); void setCurrentWallet(WalletModel* wallet_model); void removeWallet(WalletModel* wallet_model); void removeAllWallets(); From 582e66b6e75d58033987a7b0474226cfdd724ce0 Mon Sep 17 00:00:00 2001 From: Gr0kchain <7654306+gr0kchain@users.noreply.github.com> Date: Wed, 6 Nov 2019 11:57:27 +0200 Subject: [PATCH 003/183] doc: Added regtest config for linearize script Updated the example-linearize.cfg file to include support for the regtest chain network config which is used by the ./linearize-data.py Problem: Without the regtest magic, genesis hash and path config, the `linearize-data.py` script cannot generate a bootstrap.dat file. Example of error: ./linearize-data.py ./linearize.cfg Read 102 hashes Genesis block not found in hashlist Solution: Added netmagic, genesis and input example parameters to file. Resolution 1. Starting bitcoind in regtest mode 2. bitcoin-cli generatetoaddress 101 $(bitcoin-cli getnewaddress) 3. ./linearize-hashes.py ./linearize.cfg > ./hashlist.txt 4. ./linearize-data.py ./linearize.cfg Example after fix: $ ./linearize-data.py ./linearize.cfg Read 102 hashes Input file /Users/gr0kchain/.bitcoin/regtest/blocks/blk00000.dat Output file /Users/gr0kchain/Downloads/bootstrap.dat Done (102 blocks written) --- contrib/linearize/example-linearize.cfg | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/contrib/linearize/example-linearize.cfg b/contrib/linearize/example-linearize.cfg index 2315898bf1d7..5990b9307ab4 100644 --- a/contrib/linearize/example-linearize.cfg +++ b/contrib/linearize/example-linearize.cfg @@ -28,6 +28,11 @@ input=/home/example/.bitcoin/blocks #genesis=000000000933ea01ad0ee984209779baaec3ced90fa3f408719526f8d77f4943 #input=/home/example/.bitcoin/testnet3/blocks +# regtest +#netmagic=fabfb5da +#genesis=0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 +#input=/home/example/.bitcoin/regtest/blocks + # "output" option causes blockchain files to be written to the given location, # with "output_file" ignored. If not used, "output_file" is used instead. # output=/home/example/blockchain_directory From 6f6465cefcd599c89c00f7b51f42a4b87a5ffb0b Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 19 Aug 2019 11:55:38 -0400 Subject: [PATCH 004/183] scripted-diff: [validation] Rename CheckInputs to CheckInputScripts CheckInputs() used to check no double spends, scripts & sigs and amounts. Since 832e0744cb8b1e1625cdb19b257f97316ac16a90, the double spend and amount checks have been moved to CheckTxInputs(), and CheckInputs() now just validates input scripts. Rename the function to CheckInputScripts(). -BEGIN VERIFY SCRIPT- sed -i -E -e 's/CheckInputs\b/CheckInputScripts/g' $(git grep -l CheckInputs | grep -v doc/) -END VERIFY SCRIPT- --- src/script/standard.h | 2 +- src/test/txvalidationcache_tests.cpp | 32 ++++++++++++++-------------- src/validation.cpp | 28 ++++++++++++------------ test/functional/feature_cltv.py | 2 +- test/functional/feature_dersig.py | 2 +- 5 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/script/standard.h b/src/script/standard.h index 6db28dbc2d29..5931e82b9d3a 100644 --- a/src/script/standard.h +++ b/src/script/standard.h @@ -47,7 +47,7 @@ extern unsigned nMaxDatacarrierBytes; * but in the future other flags may be added, such as a soft-fork to enforce * strict DER encoding. * - * Failing one of these tests may trigger a DoS ban - see CheckInputs() for + * Failing one of these tests may trigger a DoS ban - see CheckInputScripts() for * details. */ static const unsigned int MANDATORY_SCRIPT_VERIFY_FLAGS = SCRIPT_VERIFY_P2SH; diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index a5bc15bb9f6d..61539637c139 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -13,7 +13,7 @@ #include -bool CheckInputs(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks); +bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks); BOOST_AUTO_TEST_SUITE(tx_validationcache_tests) @@ -97,8 +97,8 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup) BOOST_CHECK_EQUAL(mempool.size(), 0U); } -// Run CheckInputs (using CoinsTip()) on the given transaction, for all script -// flags. Test that CheckInputs passes for all flags that don't overlap with +// Run CheckInputScripts (using CoinsTip()) on the given transaction, for all script +// flags. Test that CheckInputScripts passes for all flags that don't overlap with // the failing_flags argument, but otherwise fails. // CHECKLOCKTIMEVERIFY and CHECKSEQUENCEVERIFY (and future NOP codes that may // get reassigned) have an interaction with DISCOURAGE_UPGRADABLE_NOPS: if @@ -125,8 +125,8 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t fail // WITNESS requires P2SH test_flags |= SCRIPT_VERIFY_P2SH; } - bool ret = CheckInputs(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, nullptr); - // CheckInputs should succeed iff test_flags doesn't intersect with + bool ret = CheckInputScripts(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, nullptr); + // CheckInputScripts should succeed iff test_flags doesn't intersect with // failing_flags bool expected_return_value = !(test_flags & failing_flags); BOOST_CHECK_EQUAL(ret, expected_return_value); @@ -135,13 +135,13 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t fail if (ret && add_to_cache) { // Check that we get a cache hit if the tx was valid std::vector scriptchecks; - BOOST_CHECK(CheckInputs(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, &scriptchecks)); + BOOST_CHECK(CheckInputScripts(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, &scriptchecks)); BOOST_CHECK(scriptchecks.empty()); } else { // Check that we get script executions to check, if the transaction // was invalid, or we didn't add to cache. std::vector scriptchecks; - BOOST_CHECK(CheckInputs(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, &scriptchecks)); + BOOST_CHECK(CheckInputScripts(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, &scriptchecks)); BOOST_CHECK_EQUAL(scriptchecks.size(), tx.vin.size()); } } @@ -149,7 +149,7 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t fail BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup) { - // Test that passing CheckInputs with one set of script flags doesn't imply + // Test that passing CheckInputScripts with one set of script flags doesn't imply // that we would pass again with a different set of flags. { LOCK(cs_main); @@ -204,16 +204,16 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup) TxValidationState state; PrecomputedTransactionData ptd_spend_tx(spend_tx); - BOOST_CHECK(!CheckInputs(CTransaction(spend_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, nullptr)); + BOOST_CHECK(!CheckInputScripts(CTransaction(spend_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, nullptr)); // If we call again asking for scriptchecks (as happens in // ConnectBlock), we should add a script check object for this -- we're // not caching invalidity (if that changes, delete this test case). std::vector scriptchecks; - BOOST_CHECK(CheckInputs(CTransaction(spend_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, &scriptchecks)); + BOOST_CHECK(CheckInputScripts(CTransaction(spend_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, &scriptchecks)); BOOST_CHECK_EQUAL(scriptchecks.size(), 1U); - // Test that CheckInputs returns true iff DERSIG-enforcing flags are + // Test that CheckInputScripts returns true iff DERSIG-enforcing flags are // not present. Don't add these checks to the cache, so that we can // test later that block validation works fine in the absence of cached // successes. @@ -272,7 +272,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup) invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100; TxValidationState state; PrecomputedTransactionData txdata(invalid_with_cltv_tx); - BOOST_CHECK(CheckInputs(CTransaction(invalid_with_cltv_tx), state, ::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, nullptr)); + BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_cltv_tx), state, ::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, nullptr)); } // TEST CHECKSEQUENCEVERIFY @@ -300,12 +300,12 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup) invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100; TxValidationState state; PrecomputedTransactionData txdata(invalid_with_csv_tx); - BOOST_CHECK(CheckInputs(CTransaction(invalid_with_csv_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, nullptr)); + BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_csv_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, nullptr)); } // TODO: add tests for remaining script flags - // Test that passing CheckInputs with a valid witness doesn't imply success + // Test that passing CheckInputScripts with a valid witness doesn't imply success // for the same tx with a different witness. { CMutableTransaction valid_with_witness_tx; @@ -362,12 +362,12 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup) TxValidationState state; PrecomputedTransactionData txdata(tx); // This transaction is now invalid under segwit, because of the second input. - BOOST_CHECK(!CheckInputs(CTransaction(tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, nullptr)); + BOOST_CHECK(!CheckInputScripts(CTransaction(tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, nullptr)); std::vector scriptchecks; // Make sure this transaction was not cached (ie because the first // input was valid) - BOOST_CHECK(CheckInputs(CTransaction(tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, &scriptchecks)); + BOOST_CHECK(CheckInputScripts(CTransaction(tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, &scriptchecks)); // Should get 2 script checks back -- caching is on a whole-transaction basis. BOOST_CHECK_EQUAL(scriptchecks.size(), 2U); } diff --git a/src/validation.cpp b/src/validation.cpp index 5a67493b0dd2..f142e683c575 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -183,7 +183,7 @@ std::unique_ptr pblocktree; // See definition for documentation static void FindFilesToPruneManual(std::set& setFilesToPrune, int nManualPruneHeight); static void FindFilesToPrune(std::set& setFilesToPrune, uint64_t nPruneAfterHeight); -bool CheckInputs(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks = nullptr); +bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks = nullptr); static FILE* OpenUndoFile(const FlatFilePos &pos, bool fReadOnly = false); static FlatFileSeq BlockFileSeq(); static FlatFileSeq UndoFileSeq(); @@ -399,7 +399,7 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS // pool.cs should be locked already, but go ahead and re-take the lock here // to enforce that mempool doesn't change between when we check the view - // and when we actually call through to CheckInputs + // and when we actually call through to CheckInputScripts LOCK(pool.cs); assert(!tx.IsCoinBase()); @@ -407,7 +407,7 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS const Coin& coin = view.AccessCoin(txin.prevout); // At this point we haven't actually checked if the coins are all - // available (or shouldn't assume we have, since CheckInputs does). + // available (or shouldn't assume we have, since CheckInputScripts does). // So we just return failure if the inputs are not available here, // and then only have to check equivalence for available inputs. if (coin.IsSpent()) return false; @@ -424,8 +424,8 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS } } - // Call CheckInputs() to cache signature and script validity against current tip consensus rules. - return CheckInputs(tx, state, view, flags, /* cacheSigStore = */ true, /* cacheFullSciptStore = */ true, txdata); + // Call CheckInputScripts() to cache signature and script validity against current tip consensus rules. + return CheckInputScripts(tx, state, view, flags, /* cacheSigStore = */ true, /* cacheFullSciptStore = */ true, txdata); } namespace { @@ -911,18 +911,18 @@ bool MemPoolAccept::PolicyScriptChecks(ATMPArgs& args, Workspace& ws, Precompute // Check against previous transactions // This is done last to help prevent CPU exhaustion denial-of-service attacks. - if (!CheckInputs(tx, state, m_view, scriptVerifyFlags, true, false, txdata)) { + if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, true, false, txdata)) { // SCRIPT_VERIFY_CLEANSTACK requires SCRIPT_VERIFY_WITNESS, so we // need to turn both off, and compare against just turning off CLEANSTACK // to see if the failure is specifically due to witness validation. - TxValidationState state_dummy; // Want reported failures to be from first CheckInputs - if (!tx.HasWitness() && CheckInputs(tx, state_dummy, m_view, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, txdata) && - !CheckInputs(tx, state_dummy, m_view, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, txdata)) { + TxValidationState state_dummy; // Want reported failures to be from first CheckInputScripts + if (!tx.HasWitness() && CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, txdata) && + !CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, txdata)) { // Only the witness is missing, so the transaction itself may be fine. state.Invalid(TxValidationResult::TX_WITNESS_MUTATED, state.GetRejectReason(), state.GetDebugMessage()); } - return false; // state filled in by CheckInputs + return false; // state filled in by CheckInputScripts } return true; @@ -953,7 +953,7 @@ bool MemPoolAccept::ConsensusScriptChecks(ATMPArgs& args, Workspace& ws, Precomp // transactions into the mempool can be exploited as a DoS attack. unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(::ChainActive().Tip(), chainparams.GetConsensus()); if (!CheckInputsFromMempoolAndCache(tx, state, m_view, m_pool, currentBlockScriptVerifyFlags, txdata)) { - return error("%s: BUG! PLEASE REPORT THIS! CheckInputs failed against latest-block but not STANDARD flags %s, %s", + return error("%s: BUG! PLEASE REPORT THIS! CheckInputScripts failed against latest-block but not STANDARD flags %s, %s", __func__, hash.ToString(), FormatStateMessage(state)); } @@ -1485,7 +1485,7 @@ void InitScriptExecutionCache() { * * Non-static (and re-declared) in src/test/txvalidationcache_tests.cpp */ -bool CheckInputs(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { if (tx.IsCoinBase()) return true; @@ -2132,11 +2132,11 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, std::vector vChecks; bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */ TxValidationState tx_state; - if (fScriptChecks && !CheckInputs(tx, tx_state, view, flags, fCacheResults, fCacheResults, txdata[i], g_parallel_script_checks ? &vChecks : nullptr)) { + if (fScriptChecks && !CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txdata[i], g_parallel_script_checks ? &vChecks : nullptr)) { // Any transaction validation failure in ConnectBlock is a block consensus failure state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), tx_state.GetDebugMessage()); - return error("ConnectBlock(): CheckInputs on %s failed with %s", + return error("ConnectBlock(): CheckInputScripts on %s failed with %s", tx.GetHash().ToString(), FormatStateMessage(state)); } control.Add(vChecks); diff --git a/test/functional/feature_cltv.py b/test/functional/feature_cltv.py index 7d131e604537..2c6f2e733b21 100755 --- a/test/functional/feature_cltv.py +++ b/test/functional/feature_cltv.py @@ -135,7 +135,7 @@ def run_test(self): block.hashMerkleRoot = block.calc_merkle_root() block.solve() - with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputs on {} failed with non-mandatory-script-verify-flag (Negative locktime)'.format(block.vtx[-1].hash)]): + with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputScripts on {} failed with non-mandatory-script-verify-flag (Negative locktime)'.format(block.vtx[-1].hash)]): self.nodes[0].p2p.send_and_ping(msg_block(block)) assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip) self.nodes[0].p2p.sync_with_ping() diff --git a/test/functional/feature_dersig.py b/test/functional/feature_dersig.py index 2ace96fef405..27da49cf243e 100755 --- a/test/functional/feature_dersig.py +++ b/test/functional/feature_dersig.py @@ -120,7 +120,7 @@ def run_test(self): block.rehash() block.solve() - with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputs on {} failed with non-mandatory-script-verify-flag (Non-canonical DER signature)'.format(block.vtx[-1].hash)]): + with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputScripts on {} failed with non-mandatory-script-verify-flag (Non-canonical DER signature)'.format(block.vtx[-1].hash)]): self.nodes[0].p2p.send_and_ping(msg_block(block)) assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip) self.nodes[0].p2p.sync_with_ping() From 3bd8db80d8d335ab63ece4f110b0fadd562e80b7 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 19 Aug 2019 12:10:45 -0400 Subject: [PATCH 005/183] [validation] fix comments in CheckInputScripts() --- src/validation.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index f142e683c575..7fb683f7765a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -406,12 +406,12 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS for (const CTxIn& txin : tx.vin) { const Coin& coin = view.AccessCoin(txin.prevout); - // At this point we haven't actually checked if the coins are all - // available (or shouldn't assume we have, since CheckInputScripts does). - // So we just return failure if the inputs are not available here, - // and then only have to check equivalence for available inputs. + // AcceptToMemoryPoolWorker has already checked that the coins are + // available, so this shouldn't fail. If the inputs are not available + // here then return false. if (coin.IsSpent()) return false; + // Check equivalence for available inputs. const CTransactionRef& txFrom = pool.get(txin.prevout.hash); if (txFrom) { assert(txFrom->GetHash() == txin.prevout.hash); @@ -909,7 +909,7 @@ bool MemPoolAccept::PolicyScriptChecks(ATMPArgs& args, Workspace& ws, Precompute constexpr unsigned int scriptVerifyFlags = STANDARD_SCRIPT_VERIFY_FLAGS; - // Check against previous transactions + // Check input scripts and signatures. // This is done last to help prevent CPU exhaustion denial-of-service attacks. if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, true, false, txdata)) { // SCRIPT_VERIFY_CLEANSTACK requires SCRIPT_VERIFY_WITNESS, so we @@ -1469,8 +1469,10 @@ void InitScriptExecutionCache() { } /** - * Check whether all inputs of this transaction are valid (no double spends, scripts & sigs, amounts) - * This does not modify the UTXO set. + * Check whether all of this transaction's input scripts succeed. + * + * This involves ECDSA signature checks so can be computationally intensive. This function should + * only be called after the cheap sanity checks in CheckTxInputs passed. * * If pvChecks is not nullptr, script checks are pushed onto it instead of being performed inline. Any * script checks which are not necessary (eg due to script execution cache hits) are, obviously, From 3e730bf90aaf53c41ff3a778f6aac15d163d1c0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Mon, 11 Nov 2019 22:21:43 +0000 Subject: [PATCH 006/183] zmq: Fix due to invalid argument and multiple notifiers --- src/zmq/zmqpublishnotifier.cpp | 3 ++- test/functional/interface_zmq.py | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/zmq/zmqpublishnotifier.cpp b/src/zmq/zmqpublishnotifier.cpp index ba89d1401d10..233a45d19fa1 100644 --- a/src/zmq/zmqpublishnotifier.cpp +++ b/src/zmq/zmqpublishnotifier.cpp @@ -112,7 +112,8 @@ bool CZMQAbstractPublishNotifier::Initialize(void *pcontext) void CZMQAbstractPublishNotifier::Shutdown() { - assert(psocket); + // Early return if Initialize was not called + if (!psocket) return; int count = mapPublishNotifiers.count(address); diff --git a/test/functional/interface_zmq.py b/test/functional/interface_zmq.py index 5aea10fbce53..89c55f31f3b1 100755 --- a/test/functional/interface_zmq.py +++ b/test/functional/interface_zmq.py @@ -59,6 +59,10 @@ def test_basic(self): # Note that the publishing order is not defined in the documentation and # is subject to change. import zmq + + # Invalid zmq arguments don't take down the node, see #17185. + self.restart_node(0, ["-zmqpubrawtx=foo", "-zmqpubhashtx=bar"]) + address = 'tcp://127.0.0.1:28332' socket = self.ctx.socket(zmq.SUB) socket.set(zmq.RCVTIMEO, 60000) From 3f7dc9b808316c1e5d677af8d9a99112568c8ccb Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Mon, 11 Nov 2019 18:32:49 -0500 Subject: [PATCH 007/183] refactor: Clean up long lines in settings code Suggested by James O'Beirne https://github.com/bitcoin/bitcoin/pull/15934#discussion_r344366743 This commit does not change behavior. --- src/util/settings.cpp | 13 ++++++++++--- src/util/settings.h | 11 +++++++++-- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/util/settings.cpp b/src/util/settings.cpp index af75fef31057..d20f509e24a7 100644 --- a/src/util/settings.cpp +++ b/src/util/settings.cpp @@ -68,7 +68,9 @@ SettingsValue GetSetting(const Settings& settings, // precedence over early settings, but for backwards compatibility in // the config file the precedence is reversed for all settings except // chain name settings. - const bool reverse_precedence = (source == Source::CONFIG_FILE_NETWORK_SECTION || source == Source::CONFIG_FILE_DEFAULT_SECTION) && !get_chain_name; + const bool reverse_precedence = + (source == Source::CONFIG_FILE_NETWORK_SECTION || source == Source::CONFIG_FILE_DEFAULT_SECTION) && + !get_chain_name; // Weird behavior preserved for backwards compatibility: Negated // -regtest and -testnet arguments which you would expect to override @@ -78,7 +80,10 @@ SettingsValue GetSetting(const Settings& settings, const bool skip_negated_command_line = get_chain_name; // Ignore settings in default config section if requested. - if (ignore_default_section_config && source == Source::CONFIG_FILE_DEFAULT_SECTION && !never_ignore_negated_setting) return; + if (ignore_default_section_config && source == Source::CONFIG_FILE_DEFAULT_SECTION && + !never_ignore_negated_setting) { + return; + } // Skip negated command line settings. if (skip_negated_command_line && span.last_negated()) return; @@ -111,7 +116,9 @@ std::vector GetSettingsList(const Settings& settings, // value is followed by non-negated value, in which case config file // settings will be brought back from the dead (but earlier command // line settings will still be ignored). - const bool add_zombie_config_values = (source == Source::CONFIG_FILE_NETWORK_SECTION || source == Source::CONFIG_FILE_DEFAULT_SECTION) && !prev_negated_empty; + const bool add_zombie_config_values = + (source == Source::CONFIG_FILE_NETWORK_SECTION || source == Source::CONFIG_FILE_DEFAULT_SECTION) && + !prev_negated_empty; // Ignore settings in default config section if requested. if (ignore_default_section_config && source == Source::CONFIG_FILE_DEFAULT_SECTION) return; diff --git a/src/util/settings.h b/src/util/settings.h index 17832e4d2ccf..9ca581109df7 100644 --- a/src/util/settings.h +++ b/src/util/settings.h @@ -43,11 +43,18 @@ struct Settings { //! [section] keywords) //! @param get_chain_name - enable special backwards compatible behavior //! for GetChainName -SettingsValue GetSetting(const Settings& settings, const std::string& section, const std::string& name, bool ignore_default_section_config, bool get_chain_name); +SettingsValue GetSetting(const Settings& settings, + const std::string& section, + const std::string& name, + bool ignore_default_section_config, + bool get_chain_name); //! Get combined setting value similar to GetSetting(), except if setting was //! specified multiple times, return a list of all the values specified. -std::vector GetSettingsList(const Settings& settings, const std::string& section, const std::string& name, bool ignore_default_section_config); +std::vector GetSettingsList(const Settings& settings, + const std::string& section, + const std::string& name, + bool ignore_default_section_config); //! Return true if a setting is set in the default config file section, and not //! overridden by a higher priority command-line or network section value. From 57e8b7a7273567aa4a4aee87cce18e9bff8f3196 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Mon, 11 Nov 2019 18:40:52 -0500 Subject: [PATCH 008/183] refactor: Clean up includeconf comments Suggested by Antoine Riard https://github.com/bitcoin/bitcoin/pull/15934#discussion_r344291875 and John Newbery https://github.com/bitcoin/bitcoin/pull/15934#discussion_r344271224 This commit does not change behavior. --- src/util/system.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/system.cpp b/src/util/system.cpp index 2a2ae6fdf532..9043d02f0a5b 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -344,7 +344,7 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin } } - // we do not allow -includeconf from command line, so we clear it here + // we do not allow -includeconf from command line bool success = true; if (auto* includes = util::FindKey(m_settings.command_line_options, "includeconf")) { for (const auto& include : util::SettingsSpan(*includes)) { @@ -780,7 +780,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) return false; } // `-includeconf` cannot be included in the command line arguments except - // as `-noincludeconf` (which indicates that no conf file should be used). + // as `-noincludeconf` (which indicates that no included conf file should be used). bool use_conf_file{true}; { LOCK(cs_args); From dc0f1480746b34aa3ca2d9c0f1ec764083026b40 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Mon, 11 Nov 2019 19:05:12 -0500 Subject: [PATCH 009/183] refactor: Replace FlagsOfKnownArg with GetArgFlags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename suggested by João Barbosa https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-519048000 This also gets rid of ArgsManager::NONE constant, which was an implementation detail not meant to be used by ArgsManager callers. Finally this reverts a change from 7f40528cd50fc43ac0bd3e785de24d661adddb7a https://github.com/bitcoin/bitcoin/pull/15934 adding "-" characters to argument names. Better for GetArgFlags to require "-" prefixes for consistency with other ArgsManager methods, and to be more efficient later when GetArg functions need to call GetArgFlags (https://github.com/bitcoin/bitcoin/pull/16545) This commit does not change behavior. --- src/util/system.cpp | 14 +++++++------- src/util/system.h | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/util/system.cpp b/src/util/system.cpp index 9043d02f0a5b..9601777b3188 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -326,9 +326,9 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin key.erase(0, 1); std::string section; util::SettingsValue value = InterpretOption(section, key, val); - const unsigned int flags = FlagsOfKnownArg(key); + Optional flags = GetArgFlags('-' + key); if (flags) { - if (!CheckValid(key, value, flags, error)) { + if (!CheckValid(key, value, *flags, error)) { return false; } // Weird behavior preserved for backwards compatibility: command @@ -355,16 +355,16 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin return success; } -unsigned int ArgsManager::FlagsOfKnownArg(const std::string& key) const +Optional ArgsManager::GetArgFlags(const std::string& name) const { LOCK(cs_args); for (const auto& arg_map : m_available_args) { - const auto search = arg_map.second.find('-' + key); + const auto search = arg_map.second.find(name); if (search != arg_map.second.end()) { return search->second.m_flags; } } - return ArgsManager::NONE; + return nullopt; } std::vector ArgsManager::GetArgs(const std::string& strArg) const @@ -745,9 +745,9 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& file std::string section; std::string key = option.first; util::SettingsValue value = InterpretOption(section, key, option.second); - const unsigned int flags = FlagsOfKnownArg(key); + Optional flags = GetArgFlags('-' + key); if (flags) { - if (!CheckValid(key, value, flags, error)) { + if (!CheckValid(key, value, *flags, error)) { return false; } m_settings.ro_config[section][key].push_back(value); diff --git a/src/util/system.h b/src/util/system.h index e0b6371dc998..d02d3f274ac1 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -132,7 +133,6 @@ class ArgsManager { public: enum Flags { - NONE = 0x00, // Boolean options can accept negation syntax -noOPTION or -noOPTION=1 ALLOW_BOOL = 0x01, ALLOW_INT = 0x02, @@ -296,9 +296,9 @@ class ArgsManager /** * Return Flags for known arg. - * Return ArgsManager::NONE for unknown arg. + * Return nullopt for unknown arg. */ - unsigned int FlagsOfKnownArg(const std::string& key) const; + Optional GetArgFlags(const std::string& name) const; }; extern ArgsManager gArgs; From 3e185522ace1678e0a25b9cf8a5553a4bc279bea Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 12 Nov 2019 13:47:19 -0500 Subject: [PATCH 010/183] refactor: Get rid of ArgsManagerHelper class Suggested by John Newbery https://github.com/bitcoin/bitcoin/pull/15934#issuecomment-551969778 This commit does not change behavior. --- src/util/system.cpp | 47 ++++++++++++++++++++------------------------- src/util/system.h | 18 +++++++++++++++-- 2 files changed, 37 insertions(+), 28 deletions(-) diff --git a/src/util/system.cpp b/src/util/system.cpp index 9601777b3188..8ea82342254e 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -167,23 +167,6 @@ static std::string SettingName(const std::string& arg) return arg.size() > 0 && arg[0] == '-' ? arg.substr(1) : arg; } -/** Internal helper functions for ArgsManager */ -class ArgsManagerHelper { -public: - /** Determine whether to use config settings in the default section, - * See also comments around ArgsManager::ArgsManager() below. */ - static inline bool UseDefaultSection(const ArgsManager& am, const std::string& arg) EXCLUSIVE_LOCKS_REQUIRED(am.cs_args) - { - return (am.m_network == CBaseChainParams::MAIN || am.m_network_only_args.count(arg) == 0); - } - - static util::SettingsValue Get(const ArgsManager& am, const std::string& arg) - { - LOCK(am.cs_args); - return GetSetting(am.m_settings, am.m_network, SettingName(arg), !UseDefaultSection(am, arg), /* get_chain_name= */ false); - } -}; - /** * Interpret -nofoo as if the user supplied -foo=0. * @@ -370,7 +353,7 @@ Optional ArgsManager::GetArgFlags(const std::string& name) const std::vector ArgsManager::GetArgs(const std::string& strArg) const { LOCK(cs_args); - bool ignore_default_section_config = !ArgsManagerHelper::UseDefaultSection(*this, strArg); + bool ignore_default_section_config = !UseDefaultSection(strArg); std::vector result; for (const util::SettingsValue& value : util::GetSettingsList(m_settings, m_network, SettingName(strArg), ignore_default_section_config)) { @@ -381,29 +364,29 @@ std::vector ArgsManager::GetArgs(const std::string& strArg) const bool ArgsManager::IsArgSet(const std::string& strArg) const { - return !ArgsManagerHelper::Get(*this, strArg).isNull(); + return !GetSetting(strArg).isNull(); } bool ArgsManager::IsArgNegated(const std::string& strArg) const { - return ArgsManagerHelper::Get(*this, strArg).isFalse(); + return GetSetting(strArg).isFalse(); } std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const { - const util::SettingsValue value = ArgsManagerHelper::Get(*this, strArg); + const util::SettingsValue value = GetSetting(strArg); return value.isNull() ? strDefault : value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str(); } int64_t ArgsManager::GetArg(const std::string& strArg, int64_t nDefault) const { - const util::SettingsValue value = ArgsManagerHelper::Get(*this, strArg); + const util::SettingsValue value = GetSetting(strArg); return value.isNull() ? nDefault : value.isFalse() ? 0 : value.isTrue() ? 1 : value.isNum() ? value.get_int64() : atoi64(value.get_str()); } bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const { - const util::SettingsValue value = ArgsManagerHelper::Get(*this, strArg); + const util::SettingsValue value = GetSetting(strArg); return value.isNull() ? fDefault : value.isBool() ? value.get_bool() : InterpretBool(value.get_str()); } @@ -854,9 +837,9 @@ std::string ArgsManager::GetChainName() const { auto get_net = [&](const std::string& arg) { LOCK(cs_args); - util::SettingsValue value = GetSetting(m_settings, /* section= */ "", SettingName(arg), - /* ignore_default_section_config= */ false, - /* get_chain_name= */ true); + util::SettingsValue value = util::GetSetting(m_settings, /* section= */ "", SettingName(arg), + /* ignore_default_section_config= */ false, + /* get_chain_name= */ true); return value.isNull() ? false : value.isBool() ? value.get_bool() : InterpretBool(value.get_str()); }; @@ -874,6 +857,18 @@ std::string ArgsManager::GetChainName() const return GetArg("-chain", CBaseChainParams::MAIN); } +bool ArgsManager::UseDefaultSection(const std::string& arg) const +{ + return m_network == CBaseChainParams::MAIN || m_network_only_args.count(arg) == 0; +} + +util::SettingsValue ArgsManager::GetSetting(const std::string& arg) const +{ + LOCK(cs_args); + return util::GetSetting( + m_settings, m_network, SettingName(arg), !UseDefaultSection(arg), /* get_chain_name= */ false); +} + bool RenameOver(fs::path src, fs::path dest) { #ifdef WIN32 diff --git a/src/util/system.h b/src/util/system.h index d02d3f274ac1..4db30281968f 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -148,8 +148,6 @@ class ArgsManager }; protected: - friend class ArgsManagerHelper; - struct Arg { std::string m_help_param; @@ -166,6 +164,22 @@ class ArgsManager NODISCARD bool ReadConfigStream(std::istream& stream, const std::string& filepath, std::string& error, bool ignore_invalid_keys = false); + /** + * Returns true if settings values from the default section should be used, + * depending on the current network and whether the setting is + * network-specific. + */ + bool UseDefaultSection(const std::string& arg) const EXCLUSIVE_LOCKS_REQUIRED(cs_args); + + /** + * Get setting value. + * + * Result will be null if setting was unset, true if "-setting" argument was passed + * false if "-nosetting" argument was passed, and a string if a "-setting=value" + * argument was passed. + */ + util::SettingsValue GetSetting(const std::string& arg) const; + public: ArgsManager(); From 0fa54358b06b58f4d17073bcc8a959eb9498aadc Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 12 Nov 2019 13:56:18 -0500 Subject: [PATCH 011/183] refactor: Add ArgsManager::GetSettingsList method Add for consistency with ArgsManager::GetSetting method and to make setting types accessible to ArgsManager callers and tests (test added next commit). This commit does not change behavior. --- src/util/system.cpp | 11 +++++++---- src/util/system.h | 5 +++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/util/system.cpp b/src/util/system.cpp index 8ea82342254e..b6a026b02a7d 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -352,11 +352,8 @@ Optional ArgsManager::GetArgFlags(const std::string& name) const std::vector ArgsManager::GetArgs(const std::string& strArg) const { - LOCK(cs_args); - bool ignore_default_section_config = !UseDefaultSection(strArg); std::vector result; - for (const util::SettingsValue& value : - util::GetSettingsList(m_settings, m_network, SettingName(strArg), ignore_default_section_config)) { + for (const util::SettingsValue& value : GetSettingsList(strArg)) { result.push_back(value.isFalse() ? "0" : value.isTrue() ? "1" : value.get_str()); } return result; @@ -869,6 +866,12 @@ util::SettingsValue ArgsManager::GetSetting(const std::string& arg) const m_settings, m_network, SettingName(arg), !UseDefaultSection(arg), /* get_chain_name= */ false); } +std::vector ArgsManager::GetSettingsList(const std::string& arg) const +{ + LOCK(cs_args); + return util::GetSettingsList(m_settings, m_network, SettingName(arg), !UseDefaultSection(arg)); +} + bool RenameOver(fs::path src, fs::path dest) { #ifdef WIN32 diff --git a/src/util/system.h b/src/util/system.h index 4db30281968f..633560d70ced 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -180,6 +180,11 @@ class ArgsManager */ util::SettingsValue GetSetting(const std::string& arg) const; + /** + * Get list of setting values. + */ + std::vector GetSettingsList(const std::string& arg) const; + public: ArgsManager(); From 425bb307252cf4dec9b3ef6426e6548b2be7a303 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Sun, 4 Aug 2019 09:21:54 -0400 Subject: [PATCH 012/183] refactor: Add util_CheckValue test Test GetSetting and GetArg type coercion, negation, and default value handling. Test is expanded later to cover other flags besides ALLOW_ANY when they are implemented in https://github.com/bitcoin/bitcoin/pull/16545 This commit does not change behavior. --- src/test/util_tests.cpp | 108 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index b9fcd97a8f45..f079b2bae36a 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -189,12 +190,119 @@ struct TestArgsManager : public ArgsManager AddArg(arg.first, "", arg.second, OptionsCategory::OPTIONS); } } + using ArgsManager::GetSetting; + using ArgsManager::GetSettingsList; using ArgsManager::ReadConfigStream; using ArgsManager::cs_args; using ArgsManager::m_network; using ArgsManager::m_settings; }; +//! Test GetSetting and GetArg type coercion, negation, and default value handling. +class CheckValueTest : public TestChain100Setup +{ +public: + struct Expect { + util::SettingsValue setting; + bool default_string = false; + bool default_int = false; + bool default_bool = false; + const char* string_value = nullptr; + Optional int_value; + Optional bool_value; + Optional> list_value; + const char* error = nullptr; + + Expect(util::SettingsValue s) : setting(std::move(s)) {} + Expect& DefaultString() { default_string = true; return *this; } + Expect& DefaultInt() { default_int = true; return *this; } + Expect& DefaultBool() { default_bool = true; return *this; } + Expect& String(const char* s) { string_value = s; return *this; } + Expect& Int(int64_t i) { int_value = i; return *this; } + Expect& Bool(bool b) { bool_value = b; return *this; } + Expect& List(std::vector m) { list_value = std::move(m); return *this; } + Expect& Error(const char* e) { error = e; return *this; } + }; + + void CheckValue(unsigned int flags, const char* arg, const Expect& expect) + { + TestArgsManager test; + test.SetupArgs({{"-value", flags}}); + const char* argv[] = {"ignored", arg}; + std::string error; + bool success = test.ParseParameters(arg ? 2 : 1, (char**)argv, error); + + BOOST_CHECK_EQUAL(test.GetSetting("-value").write(), expect.setting.write()); + auto settings_list = test.GetSettingsList("-value"); + if (expect.setting.isNull() || expect.setting.isFalse()) { + BOOST_CHECK_EQUAL(settings_list.size(), 0); + } else { + BOOST_CHECK_EQUAL(settings_list.size(), 1); + BOOST_CHECK_EQUAL(settings_list[0].write(), expect.setting.write()); + } + + if (expect.error) { + BOOST_CHECK(!success); + BOOST_CHECK_NE(error.find(expect.error), std::string::npos); + } else { + BOOST_CHECK(success); + BOOST_CHECK_EQUAL(error, ""); + } + + if (expect.default_string) { + BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), "zzzzz"); + } else if (expect.string_value) { + BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), expect.string_value); + } else { + BOOST_CHECK(!success); + } + + if (expect.default_int) { + BOOST_CHECK_EQUAL(test.GetArg("-value", 99999), 99999); + } else if (expect.int_value) { + BOOST_CHECK_EQUAL(test.GetArg("-value", 99999), *expect.int_value); + } else { + BOOST_CHECK(!success); + } + + if (expect.default_bool) { + BOOST_CHECK_EQUAL(test.GetBoolArg("-value", false), false); + BOOST_CHECK_EQUAL(test.GetBoolArg("-value", true), true); + } else if (expect.bool_value) { + BOOST_CHECK_EQUAL(test.GetBoolArg("-value", false), *expect.bool_value); + BOOST_CHECK_EQUAL(test.GetBoolArg("-value", true), *expect.bool_value); + } else { + BOOST_CHECK(!success); + } + + if (expect.list_value) { + auto l = test.GetArgs("-value"); + BOOST_CHECK_EQUAL_COLLECTIONS(l.begin(), l.end(), expect.list_value->begin(), expect.list_value->end()); + } else { + BOOST_CHECK(!success); + } + } +}; + +BOOST_FIXTURE_TEST_CASE(util_CheckValue, CheckValueTest) +{ + using M = ArgsManager; + + CheckValue(M::ALLOW_ANY, nullptr, Expect{{}}.DefaultString().DefaultInt().DefaultBool().List({})); + CheckValue(M::ALLOW_ANY, "-novalue", Expect{false}.String("0").Int(0).Bool(false).List({})); + CheckValue(M::ALLOW_ANY, "-novalue=", Expect{false}.String("0").Int(0).Bool(false).List({})); + CheckValue(M::ALLOW_ANY, "-novalue=0", Expect{true}.String("1").Int(1).Bool(true).List({"1"})); + CheckValue(M::ALLOW_ANY, "-novalue=1", Expect{false}.String("0").Int(0).Bool(false).List({})); + CheckValue(M::ALLOW_ANY, "-novalue=2", Expect{false}.String("0").Int(0).Bool(false).List({})); + CheckValue(M::ALLOW_ANY, "-novalue=abc", Expect{true}.String("1").Int(1).Bool(true).List({"1"})); + CheckValue(M::ALLOW_ANY, "-value", Expect{""}.String("").Int(0).Bool(true).List({""})); + CheckValue(M::ALLOW_ANY, "-value=", Expect{""}.String("").Int(0).Bool(true).List({""})); + CheckValue(M::ALLOW_ANY, "-value=0", Expect{"0"}.String("0").Int(0).Bool(false).List({"0"})); + CheckValue(M::ALLOW_ANY, "-value=1", Expect{"1"}.String("1").Int(1).Bool(true).List({"1"})); + CheckValue(M::ALLOW_ANY, "-value=2", Expect{"2"}.String("2").Int(2).Bool(true).List({"2"})); + CheckValue(M::ALLOW_ANY, "-value=abc", Expect{"abc"}.String("abc").Int(0).Bool(false).List({"abc"})); +} + BOOST_AUTO_TEST_CASE(util_ParseParameters) { TestArgsManager testArgs; From cba2710220d76bbe790b04088839cbbd410436de Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 12 Nov 2019 17:01:00 -0500 Subject: [PATCH 013/183] scripted-diff: Remove unused ArgsManager type flags in tests The bool/int/string flags were added speculatively in #16097 and trigger errors when type checking is actually implemented in https://github.com/bitcoin/bitcoin/pull/16545 -BEGIN VERIFY SCRIPT- sed -i 's/ALLOW_\(BOOL\|INT\|STRING\)/ALLOW_ANY/g' src/test/util_tests.cpp src/test/getarg_tests.cpp -END VERIFY SCRIPT- This commit does not change behavior. --- src/test/getarg_tests.cpp | 14 +++++++------- src/test/util_tests.cpp | 38 +++++++++++++++++++------------------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/test/getarg_tests.cpp b/src/test/getarg_tests.cpp index 5abd1087ec02..4c64d8c8334f 100644 --- a/src/test/getarg_tests.cpp +++ b/src/test/getarg_tests.cpp @@ -43,7 +43,7 @@ static void SetupArgs(const std::vector>& a BOOST_AUTO_TEST_CASE(boolarg) { - const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_BOOL); + const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY); SetupArgs({foo}); ResetArgs("-foo"); BOOST_CHECK(gArgs.GetBoolArg("-foo", false)); @@ -97,8 +97,8 @@ BOOST_AUTO_TEST_CASE(boolarg) BOOST_AUTO_TEST_CASE(stringarg) { - const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_STRING); - const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_STRING); + const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY); + const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY); SetupArgs({foo, bar}); ResetArgs(""); BOOST_CHECK_EQUAL(gArgs.GetArg("-foo", ""), ""); @@ -124,8 +124,8 @@ BOOST_AUTO_TEST_CASE(stringarg) BOOST_AUTO_TEST_CASE(intarg) { - const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_INT); - const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_INT); + const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY); + const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY); SetupArgs({foo, bar}); ResetArgs(""); BOOST_CHECK_EQUAL(gArgs.GetArg("-foo", 11), 11); @@ -159,8 +159,8 @@ BOOST_AUTO_TEST_CASE(doubledash) BOOST_AUTO_TEST_CASE(boolargno) { - const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_BOOL); - const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_BOOL); + const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY); + const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY); SetupArgs({foo, bar}); ResetArgs("-nofoo"); BOOST_CHECK(!gArgs.GetBoolArg("-foo", true)); diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index f079b2bae36a..9b8f5254de99 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -397,12 +397,12 @@ BOOST_AUTO_TEST_CASE(util_ArgParsing) BOOST_AUTO_TEST_CASE(util_GetBoolArg) { TestArgsManager testArgs; - const auto a = std::make_pair("-a", ArgsManager::ALLOW_BOOL); - const auto b = std::make_pair("-b", ArgsManager::ALLOW_BOOL); - const auto c = std::make_pair("-c", ArgsManager::ALLOW_BOOL); - const auto d = std::make_pair("-d", ArgsManager::ALLOW_BOOL); - const auto e = std::make_pair("-e", ArgsManager::ALLOW_BOOL); - const auto f = std::make_pair("-f", ArgsManager::ALLOW_BOOL); + const auto a = std::make_pair("-a", ArgsManager::ALLOW_ANY); + const auto b = std::make_pair("-b", ArgsManager::ALLOW_ANY); + const auto c = std::make_pair("-c", ArgsManager::ALLOW_ANY); + const auto d = std::make_pair("-d", ArgsManager::ALLOW_ANY); + const auto e = std::make_pair("-e", ArgsManager::ALLOW_ANY); + const auto f = std::make_pair("-f", ArgsManager::ALLOW_ANY); const char *argv_test[] = { "ignored", "-a", "-nob", "-c=0", "-d=1", "-e=false", "-f=true"}; @@ -441,8 +441,8 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArgEdgeCases) TestArgsManager testArgs; // Params test - const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_BOOL); - const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_BOOL); + const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY); + const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY); const char *argv_test[] = {"ignored", "-nofoo", "-foo", "-nobar=0"}; testArgs.SetupArgs({foo, bar}); std::string error; @@ -514,16 +514,16 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream) TestArgsManager test_args; LOCK(test_args.cs_args); - const auto a = std::make_pair("-a", ArgsManager::ALLOW_BOOL); - const auto b = std::make_pair("-b", ArgsManager::ALLOW_BOOL); - const auto ccc = std::make_pair("-ccc", ArgsManager::ALLOW_STRING); - const auto d = std::make_pair("-d", ArgsManager::ALLOW_STRING); + const auto a = std::make_pair("-a", ArgsManager::ALLOW_ANY); + const auto b = std::make_pair("-b", ArgsManager::ALLOW_ANY); + const auto ccc = std::make_pair("-ccc", ArgsManager::ALLOW_ANY); + const auto d = std::make_pair("-d", ArgsManager::ALLOW_ANY); const auto e = std::make_pair("-e", ArgsManager::ALLOW_ANY); - const auto fff = std::make_pair("-fff", ArgsManager::ALLOW_BOOL); - const auto ggg = std::make_pair("-ggg", ArgsManager::ALLOW_BOOL); - const auto h = std::make_pair("-h", ArgsManager::ALLOW_BOOL); - const auto i = std::make_pair("-i", ArgsManager::ALLOW_BOOL); - const auto iii = std::make_pair("-iii", ArgsManager::ALLOW_INT); + const auto fff = std::make_pair("-fff", ArgsManager::ALLOW_ANY); + const auto ggg = std::make_pair("-ggg", ArgsManager::ALLOW_ANY); + const auto h = std::make_pair("-h", ArgsManager::ALLOW_ANY); + const auto i = std::make_pair("-i", ArgsManager::ALLOW_ANY); + const auto iii = std::make_pair("-iii", ArgsManager::ALLOW_ANY); test_args.SetupArgs({a, b, ccc, d, e, fff, ggg, h, i, iii}); test_args.ReadConfigString(str_config); @@ -726,8 +726,8 @@ BOOST_AUTO_TEST_CASE(util_GetArg) BOOST_AUTO_TEST_CASE(util_GetChainName) { TestArgsManager test_args; - const auto testnet = std::make_pair("-testnet", ArgsManager::ALLOW_BOOL); - const auto regtest = std::make_pair("-regtest", ArgsManager::ALLOW_BOOL); + const auto testnet = std::make_pair("-testnet", ArgsManager::ALLOW_ANY); + const auto regtest = std::make_pair("-regtest", ArgsManager::ALLOW_ANY); test_args.SetupArgs({testnet, regtest}); const char* argv_testnet[] = {"cmd", "-testnet"}; From e9fd366044e271632dc0e4f96e1c14f8e87213ae Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 13 Nov 2019 15:23:06 -0500 Subject: [PATCH 014/183] refactor: Remove null setting check in GetSetting() Also rename the "result_complete" variable in GetSettingsList() to "done" to be more consistent with GetSetting(). This change doesn't affect current behavior but could be useful in the future to support dynamically changing settings at runtime and adding new settings sources, because it lets high priority sources reset settings back to default (see test). By removing a special case for null, this change also helps merge code treat settings values more like black boxes, and interfere less with settings parsing and retrieval. --- src/test/settings_tests.cpp | 13 +++++++++++++ src/util/settings.cpp | 16 +++++++++------- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/test/settings_tests.cpp b/src/test/settings_tests.cpp index b0ee76ea6b13..7e30fbcf68f0 100644 --- a/src/test/settings_tests.cpp +++ b/src/test/settings_tests.cpp @@ -45,6 +45,19 @@ BOOST_AUTO_TEST_CASE(Simple) CheckValues(settings2, R"("val2")", R"(["val2","val3"])"); } +// Confirm that a high priority setting overrides a lower priority setting even +// if the high priority setting is null. This behavior is useful for a high +// priority setting source to be able to effectively reset any setting back to +// its default value. +BOOST_AUTO_TEST_CASE(NullOverride) +{ + util::Settings settings; + settings.command_line_options["name"].push_back("value"); + BOOST_CHECK_EQUAL(R"("value")", GetSetting(settings, "section", "name", false, false).write().c_str()); + settings.forced_settings["name"] = {}; + BOOST_CHECK_EQUAL(R"(null)", GetSetting(settings, "section", "name", false, false).write().c_str()); +} + // Test different ways settings can be merged, and verify results. This test can // be used to confirm that updates to settings code don't change behavior // unintentionally. diff --git a/src/util/settings.cpp b/src/util/settings.cpp index d20f509e24a7..e4979df75522 100644 --- a/src/util/settings.cpp +++ b/src/util/settings.cpp @@ -56,6 +56,7 @@ SettingsValue GetSetting(const Settings& settings, bool get_chain_name) { SettingsValue result; + bool done = false; // Done merging any more settings sources. MergeSettings(settings, section, name, [&](SettingsSpan span, Source source) { // Weird behavior preserved for backwards compatibility: Apply negated // setting even if non-negated setting would be ignored. A negated @@ -79,6 +80,8 @@ SettingsValue GetSetting(const Settings& settings, // negated values, or at least warn they are ignored. const bool skip_negated_command_line = get_chain_name; + if (done) return; + // Ignore settings in default config section if requested. if (ignore_default_section_config && source == Source::CONFIG_FILE_DEFAULT_SECTION && !never_ignore_negated_setting) { @@ -88,13 +91,12 @@ SettingsValue GetSetting(const Settings& settings, // Skip negated command line settings. if (skip_negated_command_line && span.last_negated()) return; - // Stick with highest priority value, keeping result if already set. - if (!result.isNull()) return; - if (!span.empty()) { result = reverse_precedence ? span.begin()[0] : span.end()[-1]; + done = true; } else if (span.last_negated()) { result = false; + done = true; } }); return result; @@ -106,7 +108,7 @@ std::vector GetSettingsList(const Settings& settings, bool ignore_default_section_config) { std::vector result; - bool result_complete = false; + bool done = false; // Done merging any more settings sources. bool prev_negated_empty = false; MergeSettings(settings, section, name, [&](SettingsSpan span, Source source) { // Weird behavior preserved for backwards compatibility: Apply config @@ -125,7 +127,7 @@ std::vector GetSettingsList(const Settings& settings, // Add new settings to the result if isn't already complete, or if the // values are zombies. - if (!result_complete || add_zombie_config_values) { + if (!done || add_zombie_config_values) { for (const auto& value : span) { if (value.isArray()) { result.insert(result.end(), value.getValues().begin(), value.getValues().end()); @@ -136,8 +138,8 @@ std::vector GetSettingsList(const Settings& settings, } // If a setting was negated, or if a setting was forced, set - // result_complete to true to ignore any later lower priority settings. - result_complete |= span.negated() > 0 || source == Source::FORCED; + // done to true to ignore any later lower priority settings. + done |= span.negated() > 0 || source == Source::FORCED; // Update the negated and empty state used for the zombie values check. prev_negated_empty |= span.last_negated() && result.empty(); From 1be0b1fb2adcf95d76f879195564c0bf84162e31 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Wed, 20 Nov 2019 21:40:53 +0100 Subject: [PATCH 015/183] test: add functional test for non-standard bare multisig txs A transaction is rejected by the mempool with reason "bare-multisig" if any of the outputs' scriptPubKey has bare multisig format (M ... N OP_CHECKSIG) and bitcoind is started with "-permitbaremultisig=0". --- test/functional/mempool_accept.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index 0eef1d3ddfa3..95d8053dc260 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -8,6 +8,7 @@ import math from test_framework.test_framework import BitcoinTestFramework +from test_framework.key import ECKey from test_framework.messages import ( BIP125_SEQUENCE_NUMBER, COIN, @@ -20,6 +21,9 @@ hash160, CScript, OP_0, + OP_2, + OP_3, + OP_CHECKMULTISIG, OP_EQUAL, OP_HASH160, OP_RETURN, @@ -35,7 +39,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 self.extra_args = [[ - '-txindex', + '-txindex','-permitbaremultisig=0', ]] * self.num_nodes def skip_test_if_missing_module(self): @@ -261,6 +265,15 @@ def run_test(self): rawtxs=[tx.serialize().hex()], ) tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_reference))) + key = ECKey() + key.generate() + pubkey = key.get_pubkey().get_bytes() + tx.vout[0].scriptPubKey = CScript([OP_2, pubkey, pubkey, pubkey, OP_3, OP_CHECKMULTISIG]) # Some bare multisig script (2-of-3) + self.check_mempool_result( + result_expected=[{'txid': tx.rehash(), 'allowed': False, 'reject-reason': 'bare-multisig'}], + rawtxs=[tx.serialize().hex()], + ) + tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_reference))) tx.vin[0].scriptSig = CScript([OP_HASH160]) # Some not-pushonly scriptSig self.check_mempool_result( result_expected=[{'txid': tx.rehash(), 'allowed': False, 'reject-reason': 'scriptsig-not-pushonly'}], From bb2c8ce23c9d7ba8d0e5538243e07218443c85b4 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 20 Nov 2019 12:38:44 -0500 Subject: [PATCH 016/183] keypool: Remove superfluous topup from CWallet::GetNewChangeDestination This does not change behavior. This TopUp() is unnecessary as currently m_spk_man calls TopUp further down the call stack inside LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination) By removing this here, we also prepare for future changes where CWallet has multiple ScriptPubKeyMans instead of m_spk_man. --- src/wallet/wallet.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b1e1385ca36c..65ac67982530 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3122,8 +3122,6 @@ bool CWallet::GetNewChangeDestination(const OutputType type, CTxDestination& des { error.clear(); - m_spk_man->TopUp(); - ReserveDestination reservedest(this, type); if (!reservedest.GetReservedDestination(dest, true)) { error = "Error: Keypool ran out, please call keypoolrefill first"; From ea50e34b287e0da0806c1116bb55ade730e8ff6c Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 20 Nov 2019 12:42:10 -0500 Subject: [PATCH 017/183] keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination An opportunistic TopUp is moved from LegacyScriptPubKeyMan::GetNewDestination to CWallet::GetNewDestination. Another opportunistic TopUp is moved from LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination) to ReserveDestination::GetReservedDestination. Moving opportunistic TopUps ensures that ScriptPubKeyMans will always be topped up before requesting Destinations from them as we cannot always rely on future ScriptPubKeyMan implementaions topping up internally. As such, it is also unnecessary to keep the TopUp calls in the LegacyScriptPubKeyMan functions so they are moved. This does not change behavior as TopUp calls are moved up the call stack. --- src/wallet/scriptpubkeyman.cpp | 3 --- src/wallet/wallet.cpp | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 3eaaf3786c38..6ed0a6a6ad45 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -14,7 +14,6 @@ bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error) { error.clear(); - TopUp(); // Generate a new key that is added to wallet CPubKey new_key; @@ -1147,8 +1146,6 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key { LOCK(cs_wallet); - TopUp(); - bool fReturningInternal = fRequestedInternal; fReturningInternal &= (IsHDEnabled() && m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) || m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS); bool use_split_keypool = set_pre_split_keypool.empty(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 65ac67982530..e8abb437e9ab 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3109,6 +3109,7 @@ bool CWallet::GetNewDestination(const OutputType type, const std::string label, bool result = false; auto spk_man = m_spk_man.get(); if (spk_man) { + spk_man->TopUp(); result = spk_man->GetNewDestination(type, dest, error); } if (result) { @@ -3302,6 +3303,8 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool inter if (nIndex == -1) { + m_spk_man->TopUp(); + CKeyPool keypool; if (!m_spk_man->GetReservedDestination(type, internal, nIndex, keypool)) { return false; From 2cb4e8bdc7ef75ae8d95c246af1e8e1f9c7045bd Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 25 Nov 2019 01:33:17 +0100 Subject: [PATCH 018/183] [test] move string helper functions into test library --- src/test/settings_tests.cpp | 3 ++- src/test/util.h | 33 --------------------------------- src/test/util/str.h | 33 +++++++++++++++++++++++++++++++++ src/test/util_tests.cpp | 2 +- 4 files changed, 36 insertions(+), 35 deletions(-) diff --git a/src/test/settings_tests.cpp b/src/test/settings_tests.cpp index b0ee76ea6b13..235420e6ac89 100644 --- a/src/test/settings_tests.cpp +++ b/src/test/settings_tests.cpp @@ -4,8 +4,9 @@ #include -#include #include +#include + #include #include diff --git a/src/test/util.h b/src/test/util.h index f90cb0d623db..3cf830dd3860 100644 --- a/src/test/util.h +++ b/src/test/util.h @@ -34,37 +34,4 @@ std::string getnewaddress(CWallet& w); /** Returns the generated coin */ CTxIn generatetoaddress(const std::string& address); -/** - * Increment a string. Useful to enumerate all fixed length strings with - * characters in [min_char, max_char]. - */ -template -bool NextString(CharType (&string)[StringLength], CharType min_char, CharType max_char) -{ - for (CharType& elem : string) { - bool has_next = elem != max_char; - elem = elem < min_char || elem >= max_char ? min_char : CharType(elem + 1); - if (has_next) return true; - } - return false; -} - -/** - * Iterate over string values and call function for each string without - * successive duplicate characters. - */ -template -void ForEachNoDup(CharType (&string)[StringLength], CharType min_char, CharType max_char, Fn&& fn) { - for (bool has_next = true; has_next; has_next = NextString(string, min_char, max_char)) { - int prev = -1; - bool skip_string = false; - for (CharType c : string) { - if (c == prev) skip_string = true; - if (skip_string || c < min_char || c > max_char) break; - prev = c; - } - if (!skip_string) fn(); - } -} - #endif // BITCOIN_TEST_UTIL_H diff --git a/src/test/util/str.h b/src/test/util/str.h index 63629501e85e..ef94692df096 100644 --- a/src/test/util/str.h +++ b/src/test/util/str.h @@ -9,4 +9,37 @@ bool CaseInsensitiveEqual(const std::string& s1, const std::string& s2); +/** + * Increment a string. Useful to enumerate all fixed length strings with + * characters in [min_char, max_char]. + */ +template +bool NextString(CharType (&string)[StringLength], CharType min_char, CharType max_char) +{ + for (CharType& elem : string) { + bool has_next = elem != max_char; + elem = elem < min_char || elem >= max_char ? min_char : CharType(elem + 1); + if (has_next) return true; + } + return false; +} + +/** + * Iterate over string values and call function for each string without + * successive duplicate characters. + */ +template +void ForEachNoDup(CharType (&string)[StringLength], CharType min_char, CharType max_char, Fn&& fn) { + for (bool has_next = true; has_next; has_next = NextString(string, min_char, max_char)) { + int prev = -1; + bool skip_string = false; + for (CharType c : string) { + if (c == prev) skip_string = true; + if (skip_string || c < min_char || c > max_char) break; + prev = c; + } + if (!skip_string) fn(); + } +} + #endif // BITCOIN_TEST_UTIL_STR_H diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index b9fcd97a8f45..fbd2bd5651b5 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -7,7 +7,7 @@ #include #include #include -#include +#include #include #include #include From f613e5dfdafe708f63ebb5193c44e2bc770c6651 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 25 Nov 2019 02:44:40 +0100 Subject: [PATCH 019/183] [test] move mining helper functions into test library --- src/Makefile.test_util.include | 3 +- src/bench/block_assemble.cpp | 1 + src/bench/wallet_balance.cpp | 1 + src/test/util.cpp | 44 ----------------------------- src/test/util.h | 14 ---------- src/test/util/mining.cpp | 51 ++++++++++++++++++++++++++++++++++ src/test/util/mining.h | 24 ++++++++++++++++ 7 files changed, 79 insertions(+), 59 deletions(-) create mode 100644 src/test/util/mining.cpp create mode 100644 src/test/util/mining.h diff --git a/src/Makefile.test_util.include b/src/Makefile.test_util.include index cf55d141b0f8..139b70956923 100644 --- a/src/Makefile.test_util.include +++ b/src/Makefile.test_util.include @@ -10,6 +10,7 @@ EXTRA_LIBRARIES += \ TEST_UTIL_H = \ test/util/blockfilter.h \ test/util/logging.h \ + test/util/mining.h \ test/util/setup_common.h \ test/util/str.h \ test/util/transaction_utils.h @@ -19,6 +20,7 @@ libtest_util_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) libtest_util_a_SOURCES = \ test/util/blockfilter.cpp \ test/util/logging.cpp \ + test/util/mining.cpp \ test/util/setup_common.cpp \ test/util/str.cpp \ test/util/transaction_utils.cpp \ @@ -28,4 +30,3 @@ LIBTEST_UTIL += $(LIBBITCOIN_SERVER) LIBTEST_UTIL += $(LIBBITCOIN_COMMON) LIBTEST_UTIL += $(LIBBITCOIN_UTIL) LIBTEST_UTIL += $(LIBBITCOIN_CRYPTO_BASE) - diff --git a/src/bench/block_assemble.cpp b/src/bench/block_assemble.cpp index 2f47398d99d1..2ccae0c66756 100644 --- a/src/bench/block_assemble.cpp +++ b/src/bench/block_assemble.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp index 0e660d6bcd65..bd79ddfdd6d4 100644 --- a/src/bench/wallet_balance.cpp +++ b/src/bench/wallet_balance.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include diff --git a/src/test/util.cpp b/src/test/util.cpp index ed031270f2da..e775ad323ba9 100644 --- a/src/test/util.cpp +++ b/src/test/util.cpp @@ -4,15 +4,9 @@ #include -#include -#include #include -#include #include -#include #include