From 673111c448d8fd9473d13747767eb048a5b3827a Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 10 Jun 2020 09:50:50 -0400 Subject: [PATCH 01/12] Merge #19233: Make SetMiscWarning() accept bilingual_str argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit d49612f98add29066817b7c808b76c2d728948e5 Make SetMiscWarning() accept bilingual_str argument (Hennadii Stepanov) d1ae7c0355662481a7d181a0a458284936d53eb1 Make GetWarnings() return bilingual_str (Hennadii Stepanov) 38e33aa481cefbe12c50f344bae190c0d95fb489 refactor: Make GetWarnings() bilingual_str aware internally (Hennadii Stepanov) Pull request description: This is one more step for consistent usage of `bilingual_str`. No new translation messages are defined. ACKs for top commit: laanwj: Code review ACK d49612f98add29066817b7c808b76c2d728948e5 MarcoFalke: ACK d49612f98add29066817b7c808b76c2d728948e5 ๐ŸŒ‚ Tree-SHA512: 7413cb94a85291209c182845f6873350bb9e9ce940647d416c462a136603832fec8a63d792341bf634f07629767c78bc206d3a318cf10c7e87241c114c2496e9 --- src/index/base.cpp | 2 +- src/interfaces/node.h | 3 ++- src/node/interfaces.cpp | 2 +- src/qt/bitcoin.cpp | 4 ++-- src/qt/clientmodel.cpp | 2 +- src/rpc/blockchain.cpp | 3 ++- src/rpc/mining.cpp | 3 ++- src/rpc/net.cpp | 3 ++- src/shutdown.cpp | 2 +- src/test/timedata_tests.cpp | 3 ++- src/timedata.cpp | 2 +- src/validation.cpp | 20 ++++++++++---------- src/wallet/wallet.cpp | 2 +- src/warnings.cpp | 37 +++++++++++++++++++++---------------- src/warnings.h | 8 +++++--- 15 files changed, 54 insertions(+), 42 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index cb860f544d4a..5a55e0bbfd4b 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -22,7 +22,7 @@ template static void FatalError(const char* fmt, const Args&... args) { std::string strMessage = tfm::format(fmt, args...); - SetMiscWarning(strMessage); + SetMiscWarning(Untranslated(strMessage)); LogPrintf("*** %s\n", strMessage); AbortError(_("A fatal internal error occurred, see debug.log for details")); StartShutdown(); diff --git a/src/interfaces/node.h b/src/interfaces/node.h index a0ed8c7bfa30..fb3982670581 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -11,6 +11,7 @@ #include // For Network #include // For SecureString #include +#include #include #include @@ -171,7 +172,7 @@ class Node virtual void initParameterInteraction() = 0; //! Get warnings. - virtual std::string getWarnings() = 0; + virtual bilingual_str getWarnings() = 0; // Get log flags. virtual uint64_t getLogCategories() = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index dad926ed0a97..abbf8fa14565 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -246,7 +246,7 @@ class NodeImpl : public Node std::string getNetwork() override { return Params().NetworkIDString(); } void initLogging() override { InitLogging(gArgs); } void initParameterInteraction() override { InitParameterInteraction(gArgs); } - std::string getWarnings() override { return GetWarnings(true); } + bilingual_str getWarnings() override { return GetWarnings(true); } uint64_t getLogCategories() override { return LogInstance().GetCategoryMask(); } bool baseInitialize() override { diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 3a3697f13f47..b85b72cf4d8b 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -143,7 +143,7 @@ BitcoinCore::BitcoinCore(interfaces::Node& node) : void BitcoinCore::handleRunawayException(const std::exception_ptr e) { PrintExceptionContinue(e, "Runaway exception"); - Q_EMIT runawayException(QString::fromStdString(m_node.getWarnings())); + Q_EMIT runawayException(QString::fromStdString(m_node.getWarnings().translated)); } void BitcoinCore::initialize() @@ -747,7 +747,7 @@ int GuiMain(int argc, char* argv[]) } } catch (...) { PrintExceptionContinue(std::current_exception(), "Runaway exception"); - app.handleRunawayException(QString::fromStdString(node->getWarnings())); + app.handleRunawayException(QString::fromStdString(node->getWarnings().translated)); } return rv; } diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp index 23f15b4b236a..10e4b7125a05 100644 --- a/src/qt/clientmodel.cpp +++ b/src/qt/clientmodel.cpp @@ -202,7 +202,7 @@ enum BlockSource ClientModel::getBlockSource() const QString ClientModel::getStatusBarWarnings() const { - return QString::fromStdString(m_node.getWarnings()); + return QString::fromStdString(m_node.getWarnings().translated); } OptionsModel *ClientModel::getOptionsModel() diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index d39985e0303d..d09184abadcf 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -1766,7 +1767,7 @@ UniValue getblockchaininfo(const JSONRPCRequest& request) obj.pushKV("softforks", softforks); - obj.pushKV("warnings", GetWarnings(false)); + obj.pushKV("warnings", GetWarnings(false).original); return obj; } diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 7e584ea74f9e..d6ee5490c381 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -39,6 +39,7 @@ #include #include #include +#include #include #include #include @@ -459,7 +460,7 @@ static UniValue getmininginfo(const JSONRPCRequest& request) obj.pushKV("networkhashps", getnetworkhashps(request)); obj.pushKV("pooledtx", (uint64_t)mempool.size()); obj.pushKV("chain", Params().NetworkIDString()); - obj.pushKV("warnings", GetWarnings(false)); + obj.pushKV("warnings", GetWarnings(false).original); return obj; } diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 7a7ba6be8aa6..14876c148832 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -594,7 +595,7 @@ static UniValue getnetworkinfo(const JSONRPCRequest& request) } } obj.pushKV("localaddresses", localAddresses); - obj.pushKV("warnings", GetWarnings(false)); + obj.pushKV("warnings", GetWarnings(false).original); return obj; } diff --git a/src/shutdown.cpp b/src/shutdown.cpp index 8421f4f81bf6..33c02106fa66 100644 --- a/src/shutdown.cpp +++ b/src/shutdown.cpp @@ -23,7 +23,7 @@ bool AbortNode(const std::string& strMessage, bilingual_str user_message) { - SetMiscWarning(strMessage); + SetMiscWarning(Untranslated(strMessage)); LogPrintf("*** %s\n", strMessage); if (user_message.empty()) { user_message = _("A fatal internal error occurred, see debug.log for details"); diff --git a/src/test/timedata_tests.cpp b/src/test/timedata_tests.cpp index 8c880babd12a..1dcee23bbbc6 100644 --- a/src/test/timedata_tests.cpp +++ b/src/test/timedata_tests.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -66,7 +67,7 @@ BOOST_AUTO_TEST_CASE(addtimedata) MultiAddTimeData(1, DEFAULT_MAX_TIME_ADJUSTMENT + 1); //filter size 5 } - BOOST_CHECK(GetWarnings(true).find("clock is wrong") != std::string::npos); + BOOST_CHECK(GetWarnings(true).original.find("clock is wrong") != std::string::npos); // nTimeOffset is not changed if the median of offsets exceeds DEFAULT_MAX_TIME_ADJUSTMENT BOOST_CHECK_EQUAL(GetTimeOffset(), 0); diff --git a/src/timedata.cpp b/src/timedata.cpp index 51dc62a6458a..f53fbe231be6 100644 --- a/src/timedata.cpp +++ b/src/timedata.cpp @@ -92,7 +92,7 @@ void AddTimeData(const CNetAddr& ip, int64_t nOffsetSample) if (!fMatch) { fDone = true; bilingual_str strMessage = strprintf(_("Please check that your computer's date and time are correct! If your clock is wrong, %s will not work properly."), PACKAGE_NAME); - SetMiscWarning(strMessage.translated); + SetMiscWarning(strMessage); uiInterface.ThreadSafeMessageBox(strMessage, "", CClientUIInterface::MSG_WARNING); } } diff --git a/src/validation.cpp b/src/validation.cpp index 4210ae99e715..33b78d5e1abf 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2655,20 +2655,20 @@ void CChainState::PruneAndFlush() } } -static void DoWarning(const std::string& strWarning) +static void DoWarning(const bilingual_str& warning) { static bool fWarned = false; - SetMiscWarning(strWarning); + SetMiscWarning(warning); if (!fWarned) { - AlertNotify(strWarning); + AlertNotify(warning.original); fWarned = true; } } /** Private helper function that concatenates warning messages. */ -static void AppendWarning(std::string& res, const std::string& warn) +static void AppendWarning(bilingual_str& res, const bilingual_str& warn) { - if (!res.empty()) res += ", "; + if (!res.empty()) res += Untranslated(", "); res += warn; } @@ -2685,7 +2685,7 @@ void CChainState::UpdateTip(const CBlockIndex* pindexNew) g_best_block_cv.notify_all(); } - std::string warningMessages; + bilingual_str warning_messages; assert(std::addressof(::ChainstateActive()) == std::addressof(*this)); if (!this->IsInitialBlockDownload()) { @@ -2694,11 +2694,11 @@ void CChainState::UpdateTip(const CBlockIndex* pindexNew) WarningBitsConditionChecker checker(bit); ThresholdState state = checker.GetStateFor(pindex, m_params.GetConsensus(), warningcache[bit]); if (state == ThresholdState::ACTIVE || state == ThresholdState::LOCKED_IN) { - const std::string strWarning = strprintf(_("Warning: unknown new rules activated (versionbit %i)").translated, bit); + const bilingual_str warning = strprintf(_("Warning: unknown new rules activated (versionbit %i)"), bit); if (state == ThresholdState::ACTIVE) { - DoWarning(strWarning); + DoWarning(warning); } else { - AppendWarning(warningMessages, strWarning); + AppendWarning(warning_messages, warning); } } } @@ -2710,7 +2710,7 @@ void CChainState::UpdateTip(const CBlockIndex* pindexNew) FormatISO8601DateTime(pindexNew->GetBlockTime()), GuessVerificationProgress(m_params.TxData(), pindexNew), this->CoinsTip().DynamicMemoryUsage() * (1.0 / (1<<20)), this->CoinsTip().GetCacheSize(), m_evoDb.GetMemoryUsage() * (1.0 / (1<<20)), - !warningMessages.empty() ? strprintf(" warning='%s'", warningMessages) : ""); + !warning_messages.empty() ? strprintf(" warning='%s'", warning_messages.original) : ""); } /** Disconnect m_chain's tip. diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c23fcea7c672..3df7610e80ac 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4689,7 +4689,7 @@ std::shared_ptr CWallet::Create(interfaces::Chain& chain, interfaces::C // Warn user every time a non-encrypted HD wallet is started if (walletInstance->IsHDEnabled() && !walletInstance->IsLocked()) { - SetMiscWarning(_("Make sure to encrypt your wallet and delete all non-encrypted backups after you have verified that the wallet works!").translated); + SetMiscWarning(_("Make sure to encrypt your wallet and delete all non-encrypted backups after you have verified that the wallet works!")); } if (gArgs.IsArgSet("-mintxfee")) { diff --git a/src/warnings.cpp b/src/warnings.cpp index c718712162fe..71c8ad5fb489 100644 --- a/src/warnings.cpp +++ b/src/warnings.cpp @@ -6,18 +6,21 @@ #include #include +#include #include #include #include +#include + static Mutex g_warnings_mutex; -static std::string strMiscWarning GUARDED_BY(g_warnings_mutex); +static bilingual_str g_misc_warnings GUARDED_BY(g_warnings_mutex); static bool fLargeWorkInvalidChainFound GUARDED_BY(g_warnings_mutex) = false; -void SetMiscWarning(const std::string& strWarning) +void SetMiscWarning(const bilingual_str& warning) { LOCK(g_warnings_mutex); - strMiscWarning = strWarning; + g_misc_warnings = warning; } void SetfLargeWorkInvalidChainFound(bool flag) @@ -26,31 +29,33 @@ void SetfLargeWorkInvalidChainFound(bool flag) fLargeWorkInvalidChainFound = flag; } -std::string GetWarnings(bool verbose) +bilingual_str GetWarnings(bool verbose) { - std::string warnings_concise; - std::string warnings_verbose; - const std::string warning_separator = "
"; + bilingual_str warnings_concise; + std::vector warnings_verbose; LOCK(g_warnings_mutex); // Pre-release build warning if (!CLIENT_VERSION_IS_RELEASE) { - warnings_concise = "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"; - warnings_verbose = _("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications").translated; + warnings_concise = _("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"); + warnings_verbose.emplace_back(warnings_concise); } // Misc warnings like out of disk space and clock is wrong - if (strMiscWarning != "") { - warnings_concise = strMiscWarning; - warnings_verbose += (warnings_verbose.empty() ? "" : warning_separator) + strMiscWarning; + if (!g_misc_warnings.empty()) { + warnings_concise = g_misc_warnings; + warnings_verbose.emplace_back(warnings_concise); } if (fLargeWorkInvalidChainFound) { - warnings_concise = "Warning: We do not appear to fully agree with our peers! You may need to upgrade, or other nodes may need to upgrade."; - warnings_verbose += (warnings_verbose.empty() ? "" : warning_separator) + _("Warning: We do not appear to fully agree with our peers! You may need to upgrade, or other nodes may need to upgrade.").translated; + warnings_concise = _("Warning: We do not appear to fully agree with our peers! You may need to upgrade, or other nodes may need to upgrade."); + warnings_verbose.emplace_back(warnings_concise); + } + + if (verbose) { + return Join(warnings_verbose, Untranslated("
")); } - if (verbose) return warnings_verbose; - else return warnings_concise; + return warnings_concise; } diff --git a/src/warnings.h b/src/warnings.h index 0b880b74b15f..e87b64a86ddb 100644 --- a/src/warnings.h +++ b/src/warnings.h @@ -8,14 +8,16 @@ #include -void SetMiscWarning(const std::string& strWarning); +struct bilingual_str; + +void SetMiscWarning(const bilingual_str& warning); void SetfLargeWorkInvalidChainFound(bool flag); /** Format a string that describes several potential problems detected by the core. * @param[in] verbose bool - * - if true, get all warnings, translated (where possible), separated by
+ * - if true, get all warnings separated by
* - if false, get the most important warning * @returns the warning string */ -std::string GetWarnings(bool verbose); +bilingual_str GetWarnings(bool verbose); #endif // BITCOIN_WARNINGS_H From d7ea4757df986188d8231064230f0e6aa5d57fa9 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 15 Jun 2020 06:14:31 -0400 Subject: [PATCH 02/12] Merge #19282: RPC: Rephrase generatetoaddress help, and use PACKAGE_NAME 0f8f51544558d204a4e9d0607b0f375150529637 RPC: Rephrase generatetoaddress help, and use PACKAGE_NAME (Luke Dashjr) Pull request description: Top commit has no ACKs. Tree-SHA512: 357c6e0bd1b144213ca6cf0bfd649c7a482c2d6d5e98a254d20c8365d228dc71ae1b78aca4918fdbf065f8894ef82f8a475902d605204275bb99fe77d4b42fae --- src/rpc/mining.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index d6ee5490c381..999f2f2e60d2 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -273,7 +273,7 @@ static UniValue generatetoaddress(const JSONRPCRequest& request) RPCExamples{ "\nGenerate 11 blocks to myaddress\n" + HelpExampleCli("generatetoaddress", "11 \"myaddress\"") - + "If you are running the Dash Core wallet, you can get a new address to send the newly generated coins to with:\n" + + "If you are using the " PACKAGE_NAME " wallet, you can get a new address to send the newly generated coins to with:\n" + HelpExampleCli("getnewaddress", "")}, }.Check(request); From 8342f75fc62f225398ff5c9b62c2b337e4942f0d Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 15 Jun 2020 06:25:00 -0400 Subject: [PATCH 03/12] Merge #19276: ci: Move travis workarounds to .travis.yml fa7166759789c1282609ff3ab2e80d4f70910a9f ci: Move travis workarounds to .travis.yml (MarcoFalke) Pull request description: It seems odd to have travis related workarounds in the general ci config files. Fix that oddity by moving the travis related workarounds to the travis yaml file. For unexplained reasons, this should also work around and thus close #19171 ACKs for top commit: hebasto: ACK fa7166759789c1282609ff3ab2e80d4f70910a9f, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: b4419d38e2b41f6e4d6e6b7658f1d972c40c390a49fe78808f8640d28efd84cc6668ce292d45b7c539e65b9e2ecbad10e796cb8f9329a0f1e7d0132ce962d226 --- .travis.yml | 4 +++- ci/dash/test_unittests.sh | 1 - 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 32c6728b1ad2..a8ccd46c0df4 100644 --- a/.travis.yml +++ b/.travis.yml @@ -81,7 +81,9 @@ before_install: install: - set -o errexit; source ./ci/test/04_install.sh before_script: - - set -o errexit; source ./ci/test/05_before_script.sh + # Temporary workaround for https://github.com/bitcoin/bitcoin/issues/16368 + - for i in {1..4}; do echo "$(sleep 500)" ; done & + - set -o errexit; source ./ci/test/05_before_script.sh &> "/dev/null" script: - if [ $SECONDS -gt 1200 ]; then set +o errexit; echo "Travis early exit to cache current state"; false; else set -o errexit; source .travis/test_06_script.sh; fi after_script: diff --git a/ci/dash/test_unittests.sh b/ci/dash/test_unittests.sh index 0b7bbf46db20..1b0796b34084 100755 --- a/ci/dash/test_unittests.sh +++ b/ci/dash/test_unittests.sh @@ -24,7 +24,6 @@ export BOOST_TEST_LOG_LEVEL=test_suite cd build-ci/dashcore-$BUILD_TARGET -bash -c "${CI_WAIT}" & # Print dots in case the unit tests take a long time to run if [ "$DIRECT_WINE_EXEC_TESTS" = "true" ]; then # Inside Docker, binfmt isn't working so we can't trust in make invoking windows binaries correctly wine ./src/test/test_dash.exe From 89b9ff5e924b512928df203e955ce9253e7544f3 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 18 Jun 2020 12:59:20 -0400 Subject: [PATCH 04/12] Merge bitcoin-core/gui#3: scripted-diff: Make SeparatorStyle a scoped enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 25f3554351a99a0a695fbd2a6a0f293b3adc3d98 scripted-diff: Make SeparatorStyle a scoped enum (Hennadii Stepanov) Pull request description: This PR is [split](https://github.com/bitcoin/bitcoin/pull/17877#issuecomment-644751515) from https://github.com/bitcoin/bitcoin/pull/17877 and makes `BitcoinUnits::SeparatorStyle` a scoped enum. ACKs for top commit: MarcoFalke: review ACK 25f3554351a99a0a695fbd2a6a0f293b3adc3d98 ๐Ÿš Tree-SHA512: 578f1340a476cf79faa109a83815d3c75e26d9c18873e653d7624b52428ccb2677293116db0a60ae14c949d63b64988fc5a39c7184c2352b87b00e8ddaaaf474 --- src/qt/bitcoinamountfield.cpp | 8 ++++---- src/qt/bitcoinunits.cpp | 2 +- src/qt/bitcoinunits.h | 18 ++++++++-------- src/qt/guiutil.cpp | 2 +- src/qt/overviewpage.cpp | 32 ++++++++++++++--------------- src/qt/recentrequeststablemodel.cpp | 2 +- src/qt/test/wallettests.cpp | 4 ++-- src/qt/transactiontablemodel.cpp | 6 +++--- src/qt/transactiontablemodel.h | 2 +- src/qt/transactionview.cpp | 2 +- 10 files changed, 39 insertions(+), 39 deletions(-) diff --git a/src/qt/bitcoinamountfield.cpp b/src/qt/bitcoinamountfield.cpp index 83c0cf0e187e..c04305881db6 100644 --- a/src/qt/bitcoinamountfield.cpp +++ b/src/qt/bitcoinamountfield.cpp @@ -92,7 +92,7 @@ class AmountLineEdit: public QLineEdit if (valid) { val = qBound(m_min_amount, val, m_max_amount); - setText(BitcoinUnits::format(currentUnit, val, false, BitcoinUnits::separatorAlways)); + setText(BitcoinUnits::format(currentUnit, val, false, BitcoinUnits::SeparatorStyle::ALWAYS)); } } @@ -103,7 +103,7 @@ class AmountLineEdit: public QLineEdit void setValue(const CAmount& value) { - setText(BitcoinUnits::format(currentUnit, value, false, BitcoinUnits::separatorAlways)); + setText(BitcoinUnits::format(currentUnit, value, false, BitcoinUnits::SeparatorStyle::ALWAYS)); Q_EMIT valueChanged(); } @@ -130,7 +130,7 @@ class AmountLineEdit: public QLineEdit currentUnit = unit; amountValidator->updateUnit(unit); - setPlaceholderText(BitcoinUnits::format(currentUnit, m_min_amount, false, BitcoinUnits::separatorAlways)); + setPlaceholderText(BitcoinUnits::format(currentUnit, m_min_amount, false, BitcoinUnits::SeparatorStyle::ALWAYS)); if(valid) setValue(val); else @@ -142,7 +142,7 @@ class AmountLineEdit: public QLineEdit ensurePolished(); const QFontMetrics fm(fontMetrics()); int h = 0; - int w = GUIUtil::TextWidth(fm, BitcoinUnits::format(BitcoinUnits::DASH, BitcoinUnits::maxMoney(), false, BitcoinUnits::separatorAlways)); + int w = GUIUtil::TextWidth(fm, BitcoinUnits::format(BitcoinUnits::DASH, BitcoinUnits::maxMoney(), false, BitcoinUnits::SeparatorStyle::ALWAYS)); w += 2; // cursor blinking space w += GUIUtil::dashThemeActive() ? 24 : 0; // counteract padding from css return QSize(w, h); diff --git a/src/qt/bitcoinunits.cpp b/src/qt/bitcoinunits.cpp index 9f7ab54f47b7..126fac8e82b3 100644 --- a/src/qt/bitcoinunits.cpp +++ b/src/qt/bitcoinunits.cpp @@ -139,7 +139,7 @@ QString BitcoinUnits::format(int unit, const CAmount& nIn, bool fPlus, Separator // confused with the decimal marker. QChar thin_sp(THIN_SP_CP); int q_size = quotient_str.size(); - if (separators == separatorAlways || (separators == separatorStandard && q_size > 4)) + if (separators == SeparatorStyle::ALWAYS || (separators == SeparatorStyle::STANDARD && q_size > 4)) for (int i = 3; i < q_size; i += 3) quotient_str.insert(q_size - i, thin_sp); diff --git a/src/qt/bitcoinunits.h b/src/qt/bitcoinunits.h index e3e727881f64..55bd934a5711 100644 --- a/src/qt/bitcoinunits.h +++ b/src/qt/bitcoinunits.h @@ -47,11 +47,11 @@ class BitcoinUnits: public QAbstractListModel duffs }; - enum SeparatorStyle + enum class SeparatorStyle { - separatorNever, - separatorStandard, - separatorAlways + NEVER, + STANDARD, + ALWAYS }; //! @name Static API @@ -71,16 +71,16 @@ class BitcoinUnits: public QAbstractListModel //! Number of decimals left static int decimals(int unit); //! Format as string - static QString format(int unit, const CAmount& amount, bool plussign = false, SeparatorStyle separators = separatorStandard, bool justify = false); - static QString simpleFormat(int unit, const CAmount& amount, bool plussign=false, SeparatorStyle separators=separatorStandard); + static QString format(int unit, const CAmount& amount, bool plussign = false, SeparatorStyle separators = SeparatorStyle::STANDARD, bool justify = false); + static QString simpleFormat(int unit, const CAmount& amount, bool plussign=false, SeparatorStyle separators=SeparatorStyle::STANDARD); //! Format as string (with unit) - static QString formatWithUnit(int unit, const CAmount& amount, bool plussign=false, SeparatorStyle separators=separatorStandard); + static QString formatWithUnit(int unit, const CAmount& amount, bool plussign=false, SeparatorStyle separators=SeparatorStyle::STANDARD); //! Format as HTML string (with unit) - static QString formatHtmlWithUnit(int unit, const CAmount& amount, bool plussign=false, SeparatorStyle separators=separatorStandard); + static QString formatHtmlWithUnit(int unit, const CAmount& amount, bool plussign=false, SeparatorStyle separators=SeparatorStyle::STANDARD); //! Format as string (with unit) of fixed length to preserve privacy, if it is set. static QString formatWithPrivacy(int unit, const CAmount& amount, SeparatorStyle separators, bool privacy); //! Format as string (with unit) but floor value up to "digits" settings - static QString floorWithUnit(int unit, const CAmount& amount, bool plussign=false, SeparatorStyle separators=separatorStandard); + static QString floorWithUnit(int unit, const CAmount& amount, bool plussign=false, SeparatorStyle separators=SeparatorStyle::STANDARD); static QString floorHtmlWithPrivacy(int unit, const CAmount& amount, SeparatorStyle separators, bool privacy); //! Parse string to coin amount static bool parse(int unit, const QString &value, CAmount *val_out); diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp index 1a82f2de4a26..ca885f490c10 100644 --- a/src/qt/guiutil.cpp +++ b/src/qt/guiutil.cpp @@ -406,7 +406,7 @@ QString formatBitcoinURI(const SendCoinsRecipient &info) if (info.amount) { - ret += QString("?amount=%1").arg(BitcoinUnits::format(BitcoinUnits::DASH, info.amount, false, BitcoinUnits::separatorNever)); + ret += QString("?amount=%1").arg(BitcoinUnits::format(BitcoinUnits::DASH, info.amount, false, BitcoinUnits::SeparatorStyle::NEVER)); paramCount++; } diff --git a/src/qt/overviewpage.cpp b/src/qt/overviewpage.cpp index 682ebb5d9ea5..c417d8adaa45 100644 --- a/src/qt/overviewpage.cpp +++ b/src/qt/overviewpage.cpp @@ -81,7 +81,7 @@ class TxViewDelegate : public QAbstractItemDelegate colorForeground = qvariant_cast(indexAmount.data(Qt::ForegroundRole)); // Note: do NOT use Qt::DisplayRole, have format properly here qint64 nAmount = index.data(TransactionTableModel::AmountRole).toLongLong(); - QString strAmount = BitcoinUnits::floorWithUnit(unit, nAmount, true, BitcoinUnits::separatorAlways); + QString strAmount = BitcoinUnits::floorWithUnit(unit, nAmount, true, BitcoinUnits::SeparatorStyle::ALWAYS); painter->setPen(colorForeground); painter->drawText(rectTopHalf, Qt::AlignRight | Qt::AlignVCenter, strAmount); @@ -209,20 +209,20 @@ void OverviewPage::setBalance(const interfaces::WalletBalances& balances) int unit = walletModel->getOptionsModel()->getDisplayUnit(); m_balances = balances; if (walletModel->wallet().privateKeysDisabled()) { - ui->labelBalance->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.watch_only_balance, BitcoinUnits::separatorAlways, m_privacy)); - ui->labelUnconfirmed->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.unconfirmed_watch_only_balance, BitcoinUnits::separatorAlways, m_privacy)); - ui->labelImmature->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.immature_watch_only_balance, BitcoinUnits::separatorAlways, m_privacy)); - ui->labelTotal->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.watch_only_balance + balances.unconfirmed_watch_only_balance + balances.immature_watch_only_balance, BitcoinUnits::separatorAlways, m_privacy)); + ui->labelBalance->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); + ui->labelUnconfirmed->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.unconfirmed_watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); + ui->labelImmature->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.immature_watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); + ui->labelTotal->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.watch_only_balance + balances.unconfirmed_watch_only_balance + balances.immature_watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); } else { - ui->labelBalance->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.balance, BitcoinUnits::separatorAlways, m_privacy)); - ui->labelUnconfirmed->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.unconfirmed_balance, BitcoinUnits::separatorAlways, m_privacy)); - ui->labelImmature->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.immature_balance, BitcoinUnits::separatorAlways, m_privacy)); - ui->labelAnonymized->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.anonymized_balance, BitcoinUnits::separatorAlways, m_privacy)); - ui->labelTotal->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.balance + balances.unconfirmed_balance + balances.immature_balance, BitcoinUnits::separatorAlways, m_privacy)); - ui->labelWatchAvailable->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.watch_only_balance, BitcoinUnits::separatorAlways, m_privacy)); - ui->labelWatchPending->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.unconfirmed_watch_only_balance, BitcoinUnits::separatorAlways, m_privacy)); - ui->labelWatchImmature->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.immature_watch_only_balance, BitcoinUnits::separatorAlways, m_privacy)); - ui->labelWatchTotal->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.watch_only_balance + balances.unconfirmed_watch_only_balance + balances.immature_watch_only_balance, BitcoinUnits::separatorAlways, m_privacy)); + ui->labelBalance->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); + ui->labelUnconfirmed->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.unconfirmed_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); + ui->labelImmature->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.immature_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); + ui->labelAnonymized->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.anonymized_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); + ui->labelTotal->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.balance + balances.unconfirmed_balance + balances.immature_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); + ui->labelWatchAvailable->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); + ui->labelWatchPending->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.unconfirmed_watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); + ui->labelWatchImmature->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.immature_watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); + ui->labelWatchTotal->setText(BitcoinUnits::floorHtmlWithPrivacy(unit, balances.watch_only_balance + balances.unconfirmed_watch_only_balance + balances.immature_watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy)); } // only show immature (newly mined) balance if it's non-zero, so as not to complicate things // for the non-mining users @@ -350,7 +350,7 @@ void OverviewPage::updateCoinJoinProgress() if (!walletModel || !clientModel || clientModel->node().shutdownRequested() || !clientModel->masternodeSync().isBlockchainSynced()) return; QString strAmountAndRounds; - QString strCoinJoinAmount = BitcoinUnits::formatHtmlWithUnit(nDisplayUnit, clientModel->coinJoinOptions().getAmount() * COIN, false, BitcoinUnits::separatorAlways); + QString strCoinJoinAmount = BitcoinUnits::formatHtmlWithUnit(nDisplayUnit, clientModel->coinJoinOptions().getAmount() * COIN, false, BitcoinUnits::SeparatorStyle::ALWAYS); if(m_balances.balance == 0) { @@ -381,7 +381,7 @@ void OverviewPage::updateCoinJoinProgress() strCoinJoinAmount = strCoinJoinAmount.remove(strCoinJoinAmount.indexOf("."), BitcoinUnits::decimals(nDisplayUnit) + 1); strAmountAndRounds = strCoinJoinAmount + " / " + tr("%n Rounds", "", clientModel->coinJoinOptions().getRounds()); } else { - QString strMaxToAnonymize = BitcoinUnits::formatHtmlWithUnit(nDisplayUnit, nMaxToAnonymize, false, BitcoinUnits::separatorAlways); + QString strMaxToAnonymize = BitcoinUnits::formatHtmlWithUnit(nDisplayUnit, nMaxToAnonymize, false, BitcoinUnits::SeparatorStyle::ALWAYS); ui->labelAmountRounds->setToolTip(tr("Not enough compatible inputs to mix %2,
" "will mix %3 instead") .arg(GUIUtil::getThemedStyleQString(GUIUtil::ThemedStyle::TS_ERROR)) diff --git a/src/qt/recentrequeststablemodel.cpp b/src/qt/recentrequeststablemodel.cpp index 5544742e1e10..b11ccf06df4e 100644 --- a/src/qt/recentrequeststablemodel.cpp +++ b/src/qt/recentrequeststablemodel.cpp @@ -84,7 +84,7 @@ QVariant RecentRequestsTableModel::data(const QModelIndex &index, int role) cons if (rec->recipient.amount == 0 && role == Qt::DisplayRole) return tr("(no amount requested)"); else if (role == Qt::EditRole) - return BitcoinUnits::format(walletModel->getOptionsModel()->getDisplayUnit(), rec->recipient.amount, false, BitcoinUnits::separatorNever); + return BitcoinUnits::format(walletModel->getOptionsModel()->getDisplayUnit(), rec->recipient.amount, false, BitcoinUnits::SeparatorStyle::NEVER); else return BitcoinUnits::format(walletModel->getOptionsModel()->getDisplayUnit(), rec->recipient.amount); } diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index d948276aa22c..fc01d1fcb2b9 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -144,7 +144,7 @@ void TestGUI(interfaces::Node& node) QString balanceText = balanceLabel->text(); int unit = walletModel.getOptionsModel()->getDisplayUnit(); CAmount balance = walletModel.wallet().getBalance(); - QString balanceComparison = BitcoinUnits::formatWithUnit(unit, balance, false /*, BitcoinUnits::separatorAlways*/); + QString balanceComparison = BitcoinUnits::formatWithUnit(unit, balance, false /*, BitcoinUnits::SeparatorStyle::ALWAYS*/); QCOMPARE(balanceText, balanceComparison); } @@ -165,7 +165,7 @@ void TestGUI(interfaces::Node& node) QString balanceText = balanceLabel->text().trimmed(); int unit = walletModel.getOptionsModel()->getDisplayUnit(); CAmount balance = walletModel.wallet().getBalance(); - QString balanceComparison = BitcoinUnits::floorHtmlWithPrivacy(unit, balance, BitcoinUnits::separatorAlways, false); + QString balanceComparison = BitcoinUnits::floorHtmlWithPrivacy(unit, balance, BitcoinUnits::SeparatorStyle::ALWAYS, false); QCOMPARE(balanceText, balanceComparison); // Check Request Payment button diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp index 06f20e3effce..de49edcbd61f 100644 --- a/src/qt/transactiontablemodel.cpp +++ b/src/qt/transactiontablemodel.cpp @@ -603,7 +603,7 @@ QVariant TransactionTableModel::data(const QModelIndex &index, int role) const case ToAddress: return formatTxToAddress(rec, false); case Amount: - return formatTxAmount(rec, true, BitcoinUnits::separatorAlways); + return formatTxAmount(rec, true, BitcoinUnits::SeparatorStyle::ALWAYS); } break; case Qt::EditRole: @@ -688,14 +688,14 @@ QVariant TransactionTableModel::data(const QModelIndex &index, int role) const details.append(QString::fromStdString(rec->strAddress)); details.append(" "); } - details.append(formatTxAmount(rec, false, BitcoinUnits::separatorNever)); + details.append(formatTxAmount(rec, false, BitcoinUnits::SeparatorStyle::NEVER)); return details; } case ConfirmedRole: return rec->status.status == TransactionStatus::Status::Confirming || rec->status.status == TransactionStatus::Status::Confirmed; case FormattedAmountRole: // Used for copy/export, so don't include separators - return formatTxAmount(rec, false, BitcoinUnits::separatorNever); + return formatTxAmount(rec, false, BitcoinUnits::SeparatorStyle::NEVER); case StatusRole: return rec->status.status; } diff --git a/src/qt/transactiontablemodel.h b/src/qt/transactiontablemodel.h index 16e85b935ea0..9a5df9fd09d5 100644 --- a/src/qt/transactiontablemodel.h +++ b/src/qt/transactiontablemodel.h @@ -105,7 +105,7 @@ class TransactionTableModel : public QAbstractTableModel QString formatTxDate(const TransactionRecord *wtx) const; QString formatTxType(const TransactionRecord *wtx) const; QString formatTxToAddress(const TransactionRecord *wtx, bool tooltip) const; - QString formatTxAmount(const TransactionRecord *wtx, bool showUnconfirmed=true, BitcoinUnits::SeparatorStyle separators=BitcoinUnits::separatorStandard) const; + QString formatTxAmount(const TransactionRecord *wtx, bool showUnconfirmed=true, BitcoinUnits::SeparatorStyle separators=BitcoinUnits::SeparatorStyle::STANDARD) const; QVariant amountColor(const TransactionRecord *rec) const; QString formatTooltip(const TransactionRecord *rec) const; QVariant txStatusDecoration(const TransactionRecord *wtx) const; diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp index dbef3450ed37..7c777b4c11ee 100644 --- a/src/qt/transactionview.cpp +++ b/src/qt/transactionview.cpp @@ -583,7 +583,7 @@ void TransactionView::computeSum() for (QModelIndex index : selection){ amount += index.data(TransactionTableModel::AmountRole).toLongLong(); } - QString strAmount(BitcoinUnits::formatWithUnit(nDisplayUnit, amount, true, BitcoinUnits::separatorAlways)); + QString strAmount(BitcoinUnits::formatWithUnit(nDisplayUnit, amount, true, BitcoinUnits::SeparatorStyle::ALWAYS)); if (amount < 0) strAmount = "" + strAmount + ""; Q_EMIT trxAmount(strAmount); } From 1e87dc0e8a9419882860fc7b60babc098aec1ca9 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Sun, 21 Jun 2020 20:04:09 +1200 Subject: [PATCH 05/12] Merge #17938: Disallow automatic conversion between disparate hash types 4d7369125a82214ea42b808a32b71b315a5c3c72 Disallow automatic conversion between hash types (Ben Woosley) fa9ef2cdbed32438bdb32623af6e06f13ecd35e4 Remove an apparently unnecessary conversion (Ben Woosley) 966a22d859db37b1775e2180e5be032fc4fdf483 Explicitly support conversion between equivalent hash types (Ben Woosley) f32c1e07fd6c174ff3f6406a619550d2f6c19360 Use explicit conversion from WitnessV0KeyHash -> CKeyID (Ben Woosley) 2c54217f913967703b404747133be67cf2f4feac Use explicit conversion from PKHash -> CKeyID (Ben Woosley) a9e451f144480d7b170e49087df162989d31cd20 Convert CPubKey to WitnessV0KeyHash directly (Ben Woosley) 3fcc46812334074d2c77a6233e8a961cd0785872 Prefer explicit CScriptID construction (Ben Woosley) 0a5ea32ce605984094c5552877cb99bc81654f2c Prefer explicit uint160 conversion (Ben Woosley) Pull request description: This bases the script/standard hash types, TxDestination-related and CScriptID on a base template which does not silently convert the underlying `uintN` type. Inspired by and built on #17924. Commits are small and focused to ease review. Note some of these changes may be relative to existing bugs of the same sort as #17924. See particularly "Convert CPubKey to WitnessV0KeyHash directly" and "Remove an apparently unnecessary conversion". ACKs for top commit: achow101: ACK 4d7369125a82214ea42b808a32b71b315a5c3c72 meshcollider: re-utACK 4d7369125a82214ea42b808a32b71b315a5c3c72 Tree-SHA512: f1b3284ddc6fb6c6e726f2c22668b6d732d45eb5418262ed2b9c728f60be7be43dfb414b6ddd9915025c8dcd7f360dc3b46e997a945a2feb95b0e5c4f05d6b54 --- src/script/standard.h | 1 + src/wallet/scriptpubkeyman.cpp | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/script/standard.h b/src/script/standard.h index a2983bd50fa7..51feadd34b5c 100644 --- a/src/script/standard.h +++ b/src/script/standard.h @@ -87,6 +87,7 @@ struct ScriptHash : public BaseHash // These don't do what you'd expect. // Use ScriptHash(GetScriptForDestination(...)) instead. explicit ScriptHash(const PKHash& hash) = delete; + explicit ScriptHash(const uint160& hash) : BaseHash(hash) {} explicit ScriptHash(const CScript& script); explicit ScriptHash(const CScriptID& script); diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 4bff43c3d6e7..f01e2c2f3f56 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -689,9 +689,8 @@ bool LegacyScriptPubKeyMan::SignTransaction(CMutableTransaction& tx, const std:: SigningResult LegacyScriptPubKeyMan::SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const { - CKeyID key_id(ToKeyID(pkhash)); CKey key; - if (!GetKey(key_id, key)) { + if (!GetKey(ToKeyID(pkhash), key)) { return SigningResult::PRIVATE_KEY_NOT_AVAILABLE; } From f5cb20211935ca5bb1d579c118031910b8ccf8c2 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sat, 27 Jun 2020 08:28:19 -0400 Subject: [PATCH 06/12] Merge bitcoin-core/gui#6: Do not truncate node flag strings in debugwindow peers details tab 0ac09c9793cd6d25ef6df14d74fb960529e1b4e3 qt: Do not truncate node flag strings in debugwindow.ui peers details tab. (saibato) Pull request description: Fix: When fiddling around with new node flags other than the usual. I saw that not all possible node flag strings i.e. the UNKNOWN[..] where visible in peers details tab. Since v18.2 fixed size was set to 300 and sliding is thereby limited. A fix on my old linux cruft and small screen was to set minimumSize width to -1 or 0. Qt will then autosize the slider to the max string length. Thereby i had full display of all flags inclusive sliding without to fullscreen the window. Not sure if this is even an issue for those who can afford big screens or high res macs? Feedback welcome. BTW: nice side effect now again easy to scroll trough long version names of the node. can't wait to see strings like /Satoshi:0.23.99/NOX2NOX4NOX32 or what ever fits in the version string. ACKs for top commit: hebasto: ACK 0ac09c9793cd6d25ef6df14d74fb960529e1b4e3, tested on Linux Mint 20 (x86_64, Qt 5.12.8). promag: Tested ACK 0ac09c9793cd6d25ef6df14d74fb960529e1b4e3 on macos. Tree-SHA512: a1601b5e35f10b1fd9407b28142ca00c1b985a822be5d23be4d7d3376211450f06e17f962c44b8b40977f8f8bbbb701cac1c5abb4afb3618da76385dfac848a3 --- src/qt/forms/debugwindow.ui | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/qt/forms/debugwindow.ui b/src/qt/forms/debugwindow.ui index 3a89a5b8e814..ba7a97036b44 100644 --- a/src/qt/forms/debugwindow.ui +++ b/src/qt/forms/debugwindow.ui @@ -929,12 +929,6 @@ 426 - - - 300 - 0 - - From 54501fb5fbb23b3ae2ea454d1bb65f4e5d7f7882 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Mon, 29 Jun 2020 15:49:04 +0200 Subject: [PATCH 07/12] Merge #19403: build: improve __builtin_clz* detection 9952242c03fe587b5dff46a9f770e319146103bf build: improve builtin_clz* detection (fanquake) Pull request description: Fixes #19402. The way we currently test for `__builtin_clz*` support with `AC_CHECK_DECLS` does not work with Clang: ```bash configure:21492: clang++-10 -std=c++11 -c -g -O2 -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS conftest.cpp >&5 conftest.cpp:100:10: error: builtin functions must be directly called (void) __builtin_clz; ^ 1 error generated. ``` This also removes the `__builtin_clz()` check, as we don't actually use it anywhere, and it's trvial to re-add detection if we do start using it at some point. If this is controversial then I'll add a test for it as well. ACKs for top commit: sipa: ACK 9952242c03fe587b5dff46a9f770e319146103bf laanwj: ACK 9952242c03fe587b5dff46a9f770e319146103bf Tree-SHA512: 695abb1a694a01a25aaa483b4fffa7d598842f2ba4fe8630fbed9ce5450b915c33bf34bb16ad16a16b702dd7c91ebf49fe509a2498b9e28254fe0ec5177bbac0 --- configure.ac | 16 +++++++++++++++- src/crypto/common.h | 4 ++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index b2a755eadd56..8ec97e8591a9 100644 --- a/configure.ac +++ b/configure.ac @@ -1023,7 +1023,21 @@ AC_CHECK_DECLS([bswap_16, bswap_32, bswap_64],,, #include #endif]) -AC_CHECK_DECLS([__builtin_clz, __builtin_clzl, __builtin_clzll]) +AC_MSG_CHECKING(for __builtin_clzl) +AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ ]], [[ + (void) __builtin_clzl(0); + ]])], + [ AC_MSG_RESULT(yes); AC_DEFINE(HAVE_BUILTIN_CLZL, 1, [Define this symbol if you have __builtin_clzl])], + [ AC_MSG_RESULT(no)] +) + +AC_MSG_CHECKING(for __builtin_clzll) +AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ ]], [[ + (void) __builtin_clzll(0); + ]])], + [ AC_MSG_RESULT(yes); AC_DEFINE(HAVE_BUILTIN_CLZLL, 1, [Define this symbol if you have __builtin_clzll])], + [ AC_MSG_RESULT(no)] +) dnl Check for mallopt(M_ARENA_MAX) (to set glibc arenas) AC_MSG_CHECKING(for mallopt M_ARENA_MAX) diff --git a/src/crypto/common.h b/src/crypto/common.h index e14af1f4c40d..841eb5115ef3 100644 --- a/src/crypto/common.h +++ b/src/crypto/common.h @@ -89,12 +89,12 @@ void static inline WriteBE64(unsigned char* ptr, uint64_t x) /** Return the smallest number n such that (x >> n) == 0 (or 64 if the highest bit in x is set. */ uint64_t static inline CountBits(uint64_t x) { -#if HAVE_DECL___BUILTIN_CLZL +#if HAVE_BUILTIN_CLZL if (sizeof(unsigned long) >= sizeof(uint64_t)) { return x ? 8 * sizeof(unsigned long) - __builtin_clzl(x) : 0; } #endif -#if HAVE_DECL___BUILTIN_CLZLL +#if HAVE_BUILTIN_CLZLL if (sizeof(unsigned long long) >= sizeof(uint64_t)) { return x ? 8 * sizeof(unsigned long long) - __builtin_clzll(x) : 0; } From 94680f4ee3e3cb843d57f8c0fadd8cef3357f074 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 12 Jun 2020 15:29:48 -0400 Subject: [PATCH 08/12] Merge #19177: test: Fix and clean p2p_invalid_messages functional tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit af2a145e575f23c64909e6cf1fb323c603bda7ca Refactor resource exhaustion test (Troy Giorshev) 5c4648d17ba18e4194959963994cc6b37053f127 Fix "invalid message size" test (Troy Giorshev) ff1e7b884447a5ba10553b2d964625f94e255bdc Move size limits to module-global (Troy Giorshev) 57890abf2c7919eddfec36178b1136cd44ffe883 Remove two unneeded tests (Troy Giorshev) Pull request description: This PR touches only the p2p_invalid_messages.py functional test module. There are two main goals accomplished here. First, it fixes the "invalid message size" test, which previously made a message that was invalid for multiple reasons. Second, it refactors the file into a single consistent style. This file appears to have originally had two authors, with different styles and some test duplication. It should now be easier and quicker to understand this module, anticipating the upcoming [BIP324](https://github.com/bitcoin/bitcoin/pull/18242) and [AltNet](https://github.com/bitcoin/bitcoin/issues/18989) changes. This should probably go in ahead of #19107, but the two are not strictly related. ACKs for top commit: jnewbery: ACK af2a145e575f23c64909e6cf1fb323c603bda7ca MarcoFalke: re-ACK af2a145e57 ๐Ÿฆ Tree-SHA512: 9b57561e142c5eaefac5665f7355c8651670400b4db1a89525d2dfdd20e872d6873c4f6175c4222b6f5a8e5210cf5d6a52da69b925b673a2e2ac30a15d670d1c --- test/functional/p2p_invalid_messages.py | 156 ++++-------------------- 1 file changed, 26 insertions(+), 130 deletions(-) diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index ed97b97d8fd9..37c68302f1ca 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -4,9 +4,6 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test node responses to invalid network messages.""" -import struct -import sys - from test_framework.messages import ( CBlockHeader, @@ -27,6 +24,8 @@ wait_until, ) +MSG_LIMIT = 3 * 1024 * 1024 # 3MB, per MAX_PROTOCOL_MESSAGE_LENGTH +VALID_DATA_LIMIT = MSG_LIMIT - 5 # Account for the 5-byte length prefix class msg_unrecognized: """Nonsensical message. Modeled after similar types in test_framework.messages.""" @@ -49,115 +48,13 @@ def set_test_params(self): self.setup_clean_chain = True def run_test(self): - """ - . Test msg header - 0. Send a bunch of large (3MB) messages of an unrecognized type. Check to see - that it isn't an effective DoS against the node. - - 1. Send an oversized (3MB+) message and check that we're disconnected. - - 2. Send a few messages with an incorrect data size in the header, ensure the - messages are ignored. - """ self.test_buffer() self.test_magic_bytes() self.test_checksum() self.test_size() self.test_msgtype() self.test_large_inv() - - node = self.nodes[0] - self.node = node - node.add_p2p_connection(P2PDataStore()) - conn2 = node.add_p2p_connection(P2PDataStore()) - - msg_limit = 3 * 1024 * 1024 # 3MB, per MAX_PROTOCOL_MESSAGE_LENGTH - valid_data_limit = msg_limit - 5 # Account for the 4-byte length prefix - - # - # 0. - # - # Send as large a message as is valid, ensure we aren't disconnected but - # also can't exhaust resources. - # - msg_at_size = msg_unrecognized(str_data="b" * valid_data_limit) - assert len(msg_at_size.serialize()) == msg_limit - - self.log.info("Sending a bunch of large, junk messages to test memory exhaustion. May take a bit...") - - # Run a bunch of times to test for memory exhaustion. - for _ in range(80): - node.p2p.send_message(msg_at_size) - - # Check that, even though the node is being hammered by nonsense from one - # connection, it can still service other peers in a timely way. - for _ in range(20): - conn2.sync_with_ping(timeout=2) - - # Peer 1, despite serving up a bunch of nonsense, should still be connected. - self.log.info("Waiting for node to drop junk messages.") - node.p2p.sync_with_ping(timeout=400) - assert node.p2p.is_connected - - # - # 1. - # - # Send an oversized message, ensure we're disconnected. - # - # Under macOS this test is skipped due to an unexpected error code - # returned from the closing socket which python/asyncio does not - # yet know how to handle. - # - if sys.platform != 'darwin': - msg_over_size = msg_unrecognized(str_data="b" * (valid_data_limit + 1)) - assert len(msg_over_size.serialize()) == (msg_limit + 1) - - # An unknown message type (or *any* message type) over - # MAX_PROTOCOL_MESSAGE_LENGTH should result in a disconnect. - node.p2p.send_message(msg_over_size) - node.p2p.wait_for_disconnect(timeout=4) - - node.disconnect_p2ps() - conn = node.add_p2p_connection(P2PDataStore()) - conn.wait_for_verack() - else: - self.log.info("Skipping test p2p_invalid_messages/1 (oversized message) under macOS") - - # - # 2. - # - # Send messages with an incorrect data size in the header. - # - actual_size = 100 - msg = msg_unrecognized(str_data="b" * actual_size) - - # TODO: handle larger-than cases. I haven't been able to pin down what behavior to expect. - for wrong_size in (2, 77, 78, 79): - self.log.info("Sending a message with incorrect size of {}".format(wrong_size)) - - # Unmodified message should submit okay. - node.p2p.send_and_ping(msg) - - # A message lying about its data size results in a disconnect when the incorrect - # data size is less than the actual size. - # - # TODO: why does behavior change at 78 bytes? - # - node.p2p.send_raw_message(self._tweak_msg_data_size(msg, wrong_size)) - - # For some reason unknown to me, we sometimes have to push additional data to the - # peer in order for it to realize a disconnect. - try: - node.p2p.send_message(msg_ping(nonce=123123)) - except IOError: - pass - - node.p2p.wait_for_disconnect(timeout=10) - node.disconnect_p2ps() - node.add_p2p_connection(P2PDataStore()) - - # Node is still up. - conn = node.add_p2p_connection(P2PDataStore()) + self.test_resource_exhaustion() def test_buffer(self): self.log.info("Test message with header split across two buffers, should be received") @@ -202,14 +99,10 @@ def test_checksum(self): def test_size(self): conn = self.nodes[0].add_p2p_connection(P2PDataStore()) - with self.nodes[0].assert_debug_log(['HEADER ERROR - SIZE (badmsg, 33554433 bytes)']): - msg = conn.build_message(msg_unrecognized(str_data="d")) - cut_len = ( - 4 + # magic - 12 # msgtype - ) - # modify len to MAX_SIZE + 1 - msg = msg[:cut_len] + struct.pack(" Date: Tue, 16 Jan 2024 00:11:05 +0300 Subject: [PATCH 09/12] fix: bitcoin#18808 follow-up The bug was introduced in v19 via 5118 --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f2889fbbe3de..c0db5a0f4835 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2119,7 +2119,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic // mempool entries added before this time have likely expired from mapRelay const std::chrono::seconds longlived_mempool_time = GetTime() - RELAY_TX_CACHE_TIME; // Get last mempool request time - const std::chrono::seconds mempool_req = !pfrom.RelayAddrsWithConn() ? pfrom.m_tx_relay->m_last_mempool_req.load() + const std::chrono::seconds mempool_req = pfrom.RelayAddrsWithConn() ? pfrom.m_tx_relay->m_last_mempool_req.load() : std::chrono::seconds::min(); // Process as many TX items from the front of the getdata queue as From cc6ebdb8da2aa872609dcef595179ceb7e7fc624 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 11 Jun 2020 14:34:39 -0400 Subject: [PATCH 10/12] Merge #19083: test: msg_mempool, fRelay, and other bloomfilter tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (BACKPORT NOTICE: p2p_filter.py has also fixes for d5fbd4a92a3e7c1f8266d4cb4b639a0fe4c4c61f:test/functional/p2p_filter.py) + Merge #18726: test: check misbehavior more independently in p2p_filter.py --------------------- dca73941eb0f0a4c9b68efed3870b536f7dd6cfe scripted-diff: rename node to peer for mininodes (gzhao408) 0474ea25afc65546cbfe5f822c0212bf3e211023 [test] fix race conditions and test in p2p_filter (gzhao408) 4ef80f0827392a1310ca5a29cc1f8f5ca5d16f95 [test] sending invalid msgs to node with bloomfilters=0 causes disconnect (gzhao408) 497a619386008dfaec0db15ecaebcdfaf75f5011 [test] add BIP 37 test for node with fRelay=false (gzhao408) e8acc6015695c8439fc971a12709468995b96dcf [test] add mempool msg test for node with bloomfilter enabled (gzhao408) Pull request description: This PR adds a few tests that are bloomfilter-related, including behavior for when bloomfilters are turned _off_: 1. Tests p2p message `msg_mempool`: a node that has `peerbloomfilters` enabled should send its mempool (disabled behavior already tested [here](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_mempool.py)). 2. Tests that bloomfilter peers with [`fRelay=False`](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#extensions-to-existing-messages) in the `version` message should not receive any invs until they set the filter. The rest is the same as whatโ€™s already tested in `p2p_filter.py`. 3. Tests that peers get disconnected if they send `filterload` or `filteradd` p2p messages to a node with bloom filters disabled. 4. Refactor: renames p2p_mempool.py to p2p_nobloomfilter_messages.py. 5. Fixes race conditions in p2p_filter.py ACKs for top commit: MarcoFalke: ACK dca73941eb only changes is restoring accidentally deleted test ๐Ÿฎ jonatack: ACK dca73941eb0f0a4c9b68efed3870b536f7dd6cfe modulo a few nits if you retouch, happy to re-ACK if you take any of them but don't feel obliged to. Tree-SHA512: 442aeab0755cb8b830251ea170d1d5e6da8ac9029b3276d407a20ee3d588cc61b77b8842368de18c244056316b8c63b911776d6e106bc7c023439ab915b27ad3 --- test/functional/p2p_filter.py | 178 ++++++++++++------ test/functional/p2p_mempool.py | 34 ---- test/functional/p2p_nobloomfilter_messages.py | 43 +++++ test/functional/test_runner.py | 2 +- 4 files changed, 165 insertions(+), 92 deletions(-) delete mode 100755 test/functional/p2p_mempool.py create mode 100755 test/functional/p2p_nobloomfilter_messages.py diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py index f5daa890922a..788e57c8329c 100755 --- a/test/functional/p2p_filter.py +++ b/test/functional/p2p_filter.py @@ -16,13 +16,15 @@ msg_filterclear, msg_filterload, msg_getdata, + msg_mempool, + msg_version, ) -from test_framework.mininode import P2PInterface +from test_framework.mininode import P2PInterface, mininode_lock from test_framework.script import MAX_SCRIPT_ELEMENT_SIZE from test_framework.test_framework import BitcoinTestFramework -class FilterNode(P2PInterface): +class P2PBloomFilter(P2PInterface): # This is a P2SH watch-only wallet watch_script_pubkey = 'a914ffffffffffffffffffffffffffffffffffffffff87' # The initial filter (n=10, fp=0.000001) with just the above scriptPubKey added @@ -34,6 +36,11 @@ class FilterNode(P2PInterface): nFlags=1, ) + def __init__(self): + super().__init__() + self._tx_received = False + self._merkleblock_received = False + def on_inv(self, message): want = msg_getdata() for i in message.inv: @@ -46,10 +53,30 @@ def on_inv(self, message): self.send_message(want) def on_merkleblock(self, message): - self.merkleblock_received = True + self._merkleblock_received = True def on_tx(self, message): - self.tx_received = True + self._tx_received = True + + @property + def tx_received(self): + with mininode_lock: + return self._tx_received + + @tx_received.setter + def tx_received(self, value): + with mininode_lock: + self._tx_received = value + + @property + def merkleblock_received(self): + with mininode_lock: + return self._merkleblock_received + + @merkleblock_received.setter + def merkleblock_received(self, value): + with mininode_lock: + self._merkleblock_received = value class FilterTest(BitcoinTestFramework): @@ -63,107 +90,144 @@ def set_test_params(self): def skip_test_if_missing_module(self): self.skip_if_no_wallet() - def test_size_limits(self, filter_node): + def test_size_limits(self, filter_peer): self.log.info('Check that too large filter is rejected') with self.nodes[0].assert_debug_log(['Misbehaving']): - filter_node.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE+1))) + filter_peer.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE+1))) self.log.info('Check that max size filter is accepted') with self.nodes[0].assert_debug_log([], unexpected_msgs=['Misbehaving']): - filter_node.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE))) - filter_node.send_and_ping(msg_filterclear()) + filter_peer.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE))) + filter_peer.send_and_ping(msg_filterclear()) self.log.info('Check that filter with too many hash functions is rejected') with self.nodes[0].assert_debug_log(['Misbehaving']): - filter_node.send_and_ping(msg_filterload(data=b'\xaa', nHashFuncs=MAX_BLOOM_HASH_FUNCS+1)) + filter_peer.send_and_ping(msg_filterload(data=b'\xaa', nHashFuncs=MAX_BLOOM_HASH_FUNCS+1)) self.log.info('Check that filter with max hash functions is accepted') with self.nodes[0].assert_debug_log([], unexpected_msgs=['Misbehaving']): - filter_node.send_and_ping(msg_filterload(data=b'\xaa', nHashFuncs=MAX_BLOOM_HASH_FUNCS)) + filter_peer.send_and_ping(msg_filterload(data=b'\xaa', nHashFuncs=MAX_BLOOM_HASH_FUNCS)) # Don't send filterclear until next two filteradd checks are done self.log.info('Check that max size data element to add to the filter is accepted') with self.nodes[0].assert_debug_log([], unexpected_msgs=['Misbehaving']): - filter_node.send_and_ping(msg_filteradd(data=b'\xcc'*(MAX_SCRIPT_ELEMENT_SIZE))) + filter_peer.send_and_ping(msg_filteradd(data=b'\xcc'*(MAX_SCRIPT_ELEMENT_SIZE))) self.log.info('Check that too large data element to add to the filter is rejected') with self.nodes[0].assert_debug_log(['Misbehaving']): - filter_node.send_and_ping(msg_filteradd(data=b'\xcc'*(MAX_SCRIPT_ELEMENT_SIZE+1))) - - filter_node.send_and_ping(msg_filterclear()) - - def run_test(self): - filter_node = self.nodes[0].add_p2p_connection(FilterNode()) + filter_peer.send_and_ping(msg_filteradd(data=b'\xcc'*(MAX_SCRIPT_ELEMENT_SIZE+1))) - self.test_size_limits(filter_node) + filter_peer.send_and_ping(msg_filterclear()) - filter_node = self.nodes[0].add_p2p_connection(FilterNode()) + def test_msg_mempool(self): + self.log.info("Check that a node with bloom filters enabled services p2p mempool messages") + filter_peer = P2PBloomFilter() - self.log.info('Check that too large filter is rejected') - with self.nodes[0].assert_debug_log(['Misbehaving']): - filter_node.send_and_ping(msg_filterload(data=b'\xaa', nHashFuncs=MAX_BLOOM_HASH_FUNCS+1)) - with self.nodes[0].assert_debug_log(['Misbehaving']): - filter_node.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE+1))) - - self.log.info('Check that too large data element to add to the filter is rejected') - with self.nodes[0].assert_debug_log(['Misbehaving']): - filter_node.send_and_ping(msg_filteradd(data=b'\xcc'*(MAX_SCRIPT_ELEMENT_SIZE+1))) + self.log.info("Create a tx relevant to the peer before connecting") + filter_address = self.nodes[0].decodescript(filter_peer.watch_script_pubkey)['addresses'][0] + txid = self.nodes[0].sendtoaddress(filter_address, 90) - self.log.info('Add filtered P2P connection to the node') - filter_node.send_and_ping(filter_node.watch_filter_init) - filter_address = self.nodes[0].decodescript(filter_node.watch_script_pubkey)['addresses'][0] + self.log.info("Send a mempool msg after connecting and check that the tx is received") + self.nodes[0].add_p2p_connection(filter_peer) + filter_peer.send_and_ping(filter_peer.watch_filter_init) + self.nodes[0].p2p.send_message(msg_mempool()) + filter_peer.wait_for_tx(txid) + + def test_frelay_false(self, filter_peer): + self.log.info("Check that a node with fRelay set to false does not receive invs until the filter is set") + filter_peer.tx_received = False + filter_address = self.nodes[0].decodescript(filter_peer.watch_script_pubkey)['addresses'][0] + self.nodes[0].sendtoaddress(filter_address, 90) + # Sync to make sure the reason filter_peer doesn't receive the tx is not p2p delays + filter_peer.sync_with_ping() + assert not filter_peer.tx_received + + # Clear the mempool so that this transaction does not impact subsequent tests + self.nodes[0].generate(1) + + def test_filter(self, filter_peer): + # Set the bloomfilter using filterload + filter_peer.send_and_ping(filter_peer.watch_filter_init) + # If fRelay is not already True, sending filterload sets it to True + assert self.nodes[0].getpeerinfo()[0]['relaytxes'] + filter_address = self.nodes[0].decodescript(filter_peer.watch_script_pubkey)['addresses'][0] self.log.info('Check that we receive merkleblock and tx if the filter matches a tx in a block') block_hash = self.nodes[0].generatetoaddress(1, filter_address)[0] txid = self.nodes[0].getblock(block_hash)['tx'][0] - filter_node.wait_for_merkleblock(block_hash) - filter_node.wait_for_tx(txid) + filter_peer.wait_for_merkleblock(block_hash) + filter_peer.wait_for_tx(txid) self.log.info('Check that we only receive a merkleblock if the filter does not match a tx in a block') - filter_node.tx_received = False + filter_peer.tx_received = False block_hash = self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())[0] - filter_node.wait_for_merkleblock(block_hash) - assert not filter_node.tx_received + filter_peer.wait_for_merkleblock(block_hash) + assert not filter_peer.tx_received self.log.info('Check that we not receive a tx if the filter does not match a mempool tx') - filter_node.merkleblock_received = False - filter_node.tx_received = False + filter_peer.merkleblock_received = False + filter_peer.tx_received = False self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 90) - filter_node.sync_with_ping() - filter_node.sync_with_ping() - assert not filter_node.merkleblock_received - assert not filter_node.tx_received + filter_peer.sync_with_ping() + filter_peer.sync_with_ping() + assert not filter_peer.merkleblock_received + assert not filter_peer.tx_received - self.log.info('Check that we receive a tx in reply to a mempool msg if the filter matches a mempool tx') - filter_node.merkleblock_received = False + self.log.info('Check that we receive a tx if the filter matches a mempool tx') + filter_peer.merkleblock_received = False txid = self.nodes[0].sendtoaddress(filter_address, 90) - filter_node.wait_for_tx(txid) - assert not filter_node.merkleblock_received + filter_peer.wait_for_tx(txid) + assert not filter_peer.merkleblock_received self.log.info('Check that after deleting filter all txs get relayed again') - filter_node.send_and_ping(msg_filterclear()) + filter_peer.send_and_ping(msg_filterclear()) for _ in range(5): txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 7) - filter_node.wait_for_tx(txid) + filter_peer.wait_for_tx(txid) self.log.info('Check that request for filtered blocks is ignored if no filter is set') - filter_node.merkleblock_received = False - filter_node.tx_received = False + filter_peer.merkleblock_received = False + filter_peer.tx_received = False with self.nodes[0].assert_debug_log(expected_msgs=['received getdata']): block_hash = self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())[0] - filter_node.wait_for_inv([CInv(MSG_BLOCK, int(block_hash, 16))]) - filter_node.sync_with_ping() - assert not filter_node.merkleblock_received - assert not filter_node.tx_received + filter_peer.wait_for_inv([CInv(MSG_BLOCK, int(block_hash, 16))]) + filter_peer.sync_with_ping() + assert not filter_peer.merkleblock_received + assert not filter_peer.tx_received self.log.info('Check that sending "filteradd" if no filter is set is treated as misbehavior') with self.nodes[0].assert_debug_log(['Misbehaving']): - filter_node.send_and_ping(msg_filteradd(data=b'letsmisbehave')) + filter_peer.send_and_ping(msg_filteradd(data=b'letsmisbehave')) self.log.info("Check that division-by-zero remote crash bug [CVE-2013-5700] is fixed") - filter_node.send_and_ping(msg_filterload(data=b'', nHashFuncs=1)) - filter_node.send_and_ping(msg_filteradd(data=b'letstrytocrashthisnode')) + filter_peer.send_and_ping(msg_filterload(data=b'', nHashFuncs=1)) + filter_peer.send_and_ping(msg_filteradd(data=b'letstrytocrashthisnode')) + self.nodes[0].disconnect_p2ps() + def run_test(self): + filter_peer = self.nodes[0].add_p2p_connection(P2PBloomFilter()) + self.log.info('Test filter size limits') + self.test_size_limits(filter_peer) + + self.log.info('Test BIP 37 for a node with fRelay = True (default)') + self.test_filter(filter_peer) + self.nodes[0].disconnect_p2ps() + + self.log.info('Test BIP 37 for a node with fRelay = False') + # Add peer but do not send version yet + filter_peer_without_nrelay = self.nodes[0].add_p2p_connection(P2PBloomFilter(), send_version=False, wait_for_verack=False) + # Send version with fRelay=False + filter_peer_without_nrelay.wait_until(lambda: filter_peer_without_nrelay.is_connected, timeout=10) + version_without_fRelay = msg_version() + version_without_fRelay.nRelay = 0 + filter_peer_without_nrelay.send_message(version_without_fRelay) + filter_peer_without_nrelay.wait_for_verack() + assert not self.nodes[0].getpeerinfo()[0]['relaytxes'] + self.test_frelay_false(filter_peer_without_nrelay) + self.test_filter(filter_peer_without_nrelay) + + self.log.info('Test msg_mempool') + self.test_msg_mempool() if __name__ == '__main__': FilterTest().main() diff --git a/test/functional/p2p_mempool.py b/test/functional/p2p_mempool.py deleted file mode 100755 index da20e6de2bf2..000000000000 --- a/test/functional/p2p_mempool.py +++ /dev/null @@ -1,34 +0,0 @@ -#!/usr/bin/env python3 -# Copyright (c) 2015-2016 The Bitcoin Core developers -# Distributed under the MIT software license, see the accompanying -# file COPYING or http://www.opensource.org/licenses/mit-license.php. -"""Test p2p mempool message. - -Test that nodes are disconnected if they send mempool messages when bloom -filters are not enabled. -""" - -from test_framework.messages import msg_mempool -from test_framework.mininode import P2PInterface -from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal - -class P2PMempoolTests(BitcoinTestFramework): - def set_test_params(self): - self.setup_clean_chain = True - self.num_nodes = 1 - self.extra_args = [["-peerbloomfilters=0"]] - - def run_test(self): - # Add a p2p connection - self.nodes[0].add_p2p_connection(P2PInterface()) - - #request mempool - self.nodes[0].p2p.send_message(msg_mempool()) - self.nodes[0].p2p.wait_for_disconnect() - - #mininode must be disconnected at this point - assert_equal(len(self.nodes[0].getpeerinfo()), 0) - -if __name__ == '__main__': - P2PMempoolTests().main() diff --git a/test/functional/p2p_nobloomfilter_messages.py b/test/functional/p2p_nobloomfilter_messages.py new file mode 100755 index 000000000000..41af74ebb87e --- /dev/null +++ b/test/functional/p2p_nobloomfilter_messages.py @@ -0,0 +1,43 @@ +#!/usr/bin/env python3 +# Copyright (c) 2015-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. +"""Test invalid p2p messages for nodes with bloom filters disabled. + +Test that, when bloom filters are not enabled, nodes are disconnected if: +1. They send a p2p mempool message +2. They send a p2p filterload message +3. They send a p2p filteradd message +""" + +from test_framework.messages import msg_mempool, msg_filteradd, msg_filterload +from test_framework.mininode import P2PInterface +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal + + +class P2PNobloomfilterMessages(BitcoinTestFramework): + def set_test_params(self): + self.setup_clean_chain = True + self.num_nodes = 1 + self.extra_args = [["-peerbloomfilters=0"]] + + def test_message_causes_disconnect(self, message): + # Add a p2p connection that sends a message and check that it disconnects + peer = self.nodes[0].add_p2p_connection(P2PInterface()) + peer.send_message(message) + peer.wait_for_disconnect() + assert_equal(len(self.nodes[0].getpeerinfo()), 0) + + def run_test(self): + self.log.info("Test that node is disconnected if it sends mempool message") + self.test_message_causes_disconnect(msg_mempool()) + + self.log.info("Test that node is disconnected if it sends filterload message") + self.test_message_causes_disconnect(msg_filterload()) + + self.log.info("Test that node is disconnected if it sends filteradd message") + self.test_message_causes_disconnect(msg_filteradd(data=b'\xcc')) + +if __name__ == '__main__': + P2PNobloomfilterMessages().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index d1a7e73a5779..f4c5bc1d4580 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -181,7 +181,7 @@ 'rpc_net.py', 'wallet_keypool.py', 'wallet_keypool_hd.py', - 'p2p_mempool.py', + 'p2p_nobloomfilter_messages.py', 'p2p_filter.py', 'p2p_blocksonly.py', 'rpc_setban.py', From 65217a74a3785b51e37d8fcfecbc3c45a7597bdb Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 16 Jun 2020 15:50:24 +0800 Subject: [PATCH 11/12] Merge #19260: p2p: disconnect peers that send filterclear + update existing filter msg disconnect logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3a10d935ac8ebabdfd336569d943f042ff84b13e [p2p/refactor] move disconnect logic and remove misbehaving (gzhao408) ff8c430c6589ea72b9e169455cf6437c8623cc52 [test] test disconnect for filterclear (gzhao408) 1c6b787e0319c44f0e0bede3f4a77ac7c2089db2 [netprocessing] disconnect node that sends filterclear (gzhao408) Pull request description: Nodes that don't have bloomfilters turned on (i.e. no `NODE_BLOOM` service) should disconnect peers that send them `filterclear` P2P messages. Non-bloomfilter nodes already disconnect peers for [`filteradd` and `filterload`](https://github.com/bitcoin/bitcoin/blob/19e919217e6d62e3640525e4149de1a4ae04e74f/src/net_processing.cpp#L2218), but #8709 removed `filterclear` so it could be used to reset tx relay. This isn't needed now because using `feefilter` message is much better for this purpose (See #19204). Also refactors existing disconnect logic for `filteradd` and `filterload` into respective message handlers and removes banning for them. ACKs for top commit: jnewbery: Code review ACK 3a10d935ac8ebabdfd336569d943f042ff84b13e naumenkogs: utACK 3a10d93 gillichu: tested ACK: quick test_runner on macOS [`3a10d93`](https://github.com/bitcoin/bitcoin/commit/3a10d935ac8ebabdfd336569d943f042ff84b13e) MarcoFalke: re-ACK 3a10d935ac only change is replacing false with true ๐Ÿš Tree-SHA512: 7aad8b3c0b0e776a47ad52544f0c1250feb242320f9a2962542f5905042f77e297a1486f8cdc3bf0fb93cd00c1ab66a67b2ec426eb6da3fe4cda56b5e623620f --- src/net_processing.cpp | 24 ++++++++++--------- test/functional/p2p_nobloomfilter_messages.py | 6 ++++- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c0db5a0f4835..2c630dc42aab 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2823,14 +2823,6 @@ void PeerManagerImpl::ProcessMessage( PeerRef peer = GetPeerRef(pfrom.GetId()); if (peer == nullptr) return; - if (!(pfrom.GetLocalServices() & NODE_BLOOM) && - (msg_type == NetMsgType::FILTERLOAD || - msg_type == NetMsgType::FILTERADD)) - { - Misbehaving(pfrom.GetId(), 100); - return; - } - if (msg_type == NetMsgType::VERSION) { // Each connection can only send one version message if (pfrom.nVersion != 0) @@ -4190,6 +4182,10 @@ void PeerManagerImpl::ProcessMessage( } if (msg_type == NetMsgType::FILTERLOAD) { + if (!(pfrom.GetLocalServices() & NODE_BLOOM)) { + pfrom.fDisconnect = true; + return; + } CBloomFilter filter; vRecv >> filter; @@ -4208,6 +4204,10 @@ void PeerManagerImpl::ProcessMessage( } if (msg_type == NetMsgType::FILTERADD) { + if (!(pfrom.GetLocalServices() & NODE_BLOOM)) { + pfrom.fDisconnect = true; + return; + } std::vector vData; vRecv >> vData; @@ -4231,13 +4231,15 @@ void PeerManagerImpl::ProcessMessage( } if (msg_type == NetMsgType::FILTERCLEAR) { + if (!(pfrom.GetLocalServices() & NODE_BLOOM)) { + pfrom.fDisconnect = true; + return; + } if (!pfrom.RelayAddrsWithConn()) { return; } LOCK(pfrom.m_tx_relay->cs_filter); - if (pfrom.GetLocalServices() & NODE_BLOOM) { - pfrom.m_tx_relay->pfilter = nullptr; - } + pfrom.m_tx_relay->pfilter = nullptr; pfrom.m_tx_relay->fRelayTxes = true; return; } diff --git a/test/functional/p2p_nobloomfilter_messages.py b/test/functional/p2p_nobloomfilter_messages.py index 41af74ebb87e..8478a752e7fe 100755 --- a/test/functional/p2p_nobloomfilter_messages.py +++ b/test/functional/p2p_nobloomfilter_messages.py @@ -8,9 +8,10 @@ 1. They send a p2p mempool message 2. They send a p2p filterload message 3. They send a p2p filteradd message +4. They send a p2p filterclear message """ -from test_framework.messages import msg_mempool, msg_filteradd, msg_filterload +from test_framework.messages import msg_mempool, msg_filteradd, msg_filterload, msg_filterclear from test_framework.mininode import P2PInterface from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal @@ -39,5 +40,8 @@ def run_test(self): self.log.info("Test that node is disconnected if it sends filteradd message") self.test_message_causes_disconnect(msg_filteradd(data=b'\xcc')) + self.log.info("Test that peer is disconnected if it sends a filterclear message") + self.test_message_causes_disconnect(msg_filterclear()) + if __name__ == '__main__': P2PNobloomfilterMessages().main() From 6f02baba766e1092530519a63b7036ad490c1066 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 17 Jun 2020 06:20:04 -0400 Subject: [PATCH 12/12] Merge #19252: test: wait for disconnect in disconnect_p2ps + bloomfilter test followups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 9a40cfc558b3f7fa4fff1270f969582af17479a5 [refactor] use waiting inside disconnect_p2ps (gzhao408) aeb9fb414e2d000830287d9dd3fed7fc2eb570d2 [test] wait for disconnect_p2ps to be reflected in getpeerinfo (gzhao408) e81942d2e1288367e8da94adb2b2a88be99e4751 [test] logging and style followups for bloomfilter tests (gzhao408) Pull request description: Followup to #19083 which adds bloomfilter-related tests. 1. Make test_node `disconnect_p2ps` wait until disconnection is complete to avoid race conditions (and not place the burden on tests) from MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/19083#discussion_r437383989). And clean up any redundant `wait_until`s in the functional tests. 2. Clean up style + logging in p2p_filter.py and p2p_nobloomfilter_messages.py and jonatack's other [comments](https://github.com/bitcoin/bitcoin/pull/19083#pullrequestreview-428955784) ACKs for top commit: jonatack: Code review ACK 9a40cfc from re-reviewing the diff and `git range-diff 5cafb46 8386ad5 9a40cfc` MarcoFalke: ACK 9a40cfc558b3f7fa4fff1270f969582af17479a5 ๐Ÿ‚ Tree-SHA512: 2e14b1c12fc08a355bd5ccad7a2a734a4ccda4bc7dc7bac171cb57359819fc1599d764290729af74832fac3e2be258c5d406c701e78ab6d7262835859b9a7d87 --- test/functional/p2p_filter.py | 6 +++--- test/functional/p2p_nobloomfilter_messages.py | 17 +++++++++-------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py index 788e57c8329c..430b68f21d76 100755 --- a/test/functional/p2p_filter.py +++ b/test/functional/p2p_filter.py @@ -123,11 +123,11 @@ def test_msg_mempool(self): self.log.info("Check that a node with bloom filters enabled services p2p mempool messages") filter_peer = P2PBloomFilter() - self.log.info("Create a tx relevant to the peer before connecting") + self.log.debug("Create a tx relevant to the peer before connecting") filter_address = self.nodes[0].decodescript(filter_peer.watch_script_pubkey)['addresses'][0] txid = self.nodes[0].sendtoaddress(filter_address, 90) - self.log.info("Send a mempool msg after connecting and check that the tx is received") + self.log.debug("Send a mempool msg after connecting and check that the tx is received") self.nodes[0].add_p2p_connection(filter_peer) filter_peer.send_and_ping(filter_peer.watch_filter_init) self.nodes[0].p2p.send_message(msg_mempool()) @@ -226,8 +226,8 @@ def run_test(self): self.test_frelay_false(filter_peer_without_nrelay) self.test_filter(filter_peer_without_nrelay) - self.log.info('Test msg_mempool') self.test_msg_mempool() + if __name__ == '__main__': FilterTest().main() diff --git a/test/functional/p2p_nobloomfilter_messages.py b/test/functional/p2p_nobloomfilter_messages.py index 8478a752e7fe..accc5dc23cc0 100755 --- a/test/functional/p2p_nobloomfilter_messages.py +++ b/test/functional/p2p_nobloomfilter_messages.py @@ -4,7 +4,7 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test invalid p2p messages for nodes with bloom filters disabled. -Test that, when bloom filters are not enabled, nodes are disconnected if: +Test that, when bloom filters are not enabled, peers are disconnected if: 1. They send a p2p mempool message 2. They send a p2p filterload message 3. They send a p2p filteradd message @@ -17,31 +17,32 @@ from test_framework.util import assert_equal -class P2PNobloomfilterMessages(BitcoinTestFramework): +class P2PNoBloomFilterMessages(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 1 self.extra_args = [["-peerbloomfilters=0"]] def test_message_causes_disconnect(self, message): - # Add a p2p connection that sends a message and check that it disconnects + """Add a p2p connection that sends a message and check that it disconnects.""" peer = self.nodes[0].add_p2p_connection(P2PInterface()) peer.send_message(message) peer.wait_for_disconnect() - assert_equal(len(self.nodes[0].getpeerinfo()), 0) + assert_equal(self.nodes[0].getconnectioncount(), 0) def run_test(self): - self.log.info("Test that node is disconnected if it sends mempool message") + self.log.info("Test that peer is disconnected if it sends mempool message") self.test_message_causes_disconnect(msg_mempool()) - self.log.info("Test that node is disconnected if it sends filterload message") + self.log.info("Test that peer is disconnected if it sends filterload message") self.test_message_causes_disconnect(msg_filterload()) - self.log.info("Test that node is disconnected if it sends filteradd message") + self.log.info("Test that peer is disconnected if it sends filteradd message") self.test_message_causes_disconnect(msg_filteradd(data=b'\xcc')) self.log.info("Test that peer is disconnected if it sends a filterclear message") self.test_message_causes_disconnect(msg_filterclear()) + if __name__ == '__main__': - P2PNobloomfilterMessages().main() + P2PNoBloomFilterMessages().main()