From 315ae14f3f5c98ae4c4476e4bb260b9086c773a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Sat, 16 Nov 2019 23:41:16 +0000 Subject: [PATCH 01/15] gui: Fix itemWalletAddress leak when not tree mode Github-Pull: #18578 Rebased-From: e8123eae40eb264bbb71007d0eb074901f0e2fe5 --- src/qt/coincontroldialog.cpp | 5 ++--- src/qt/coincontroldialog.h | 1 - 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp index 9495ba389ad..ec61a4f5dd2 100644 --- a/src/qt/coincontroldialog.cpp +++ b/src/qt/coincontroldialog.cpp @@ -612,8 +612,7 @@ void CoinControlDialog::updateView() int nDisplayUnit = model->getOptionsModel()->getDisplayUnit(); for (const auto& coins : model->wallet().listCoins()) { - CCoinControlWidgetItem *itemWalletAddress = new CCoinControlWidgetItem(); - itemWalletAddress->setCheckState(COLUMN_CHECKBOX, Qt::Unchecked); + CCoinControlWidgetItem* itemWalletAddress{nullptr}; QString sWalletAddress = QString::fromStdString(EncodeDestination(coins.first)); QString sWalletLabel = model->getAddressTableModel()->labelForAddress(sWalletAddress); if (sWalletLabel.isEmpty()) @@ -622,7 +621,7 @@ void CoinControlDialog::updateView() if (treeMode) { // wallet address - ui->treeWidget->addTopLevelItem(itemWalletAddress); + itemWalletAddress = new CCoinControlWidgetItem(ui->treeWidget); itemWalletAddress->setFlags(flgTristate); itemWalletAddress->setCheckState(COLUMN_CHECKBOX, Qt::Unchecked); diff --git a/src/qt/coincontroldialog.h b/src/qt/coincontroldialog.h index efc06a76568..fcf34356f11 100644 --- a/src/qt/coincontroldialog.h +++ b/src/qt/coincontroldialog.h @@ -31,7 +31,6 @@ class CCoinControlWidgetItem : public QTreeWidgetItem { public: explicit CCoinControlWidgetItem(QTreeWidget *parent, int type = Type) : QTreeWidgetItem(parent, type) {} - explicit CCoinControlWidgetItem(int type = Type) : QTreeWidgetItem(type) {} explicit CCoinControlWidgetItem(QTreeWidgetItem *parent, int type = Type) : QTreeWidgetItem(parent, type) {} bool operator<(const QTreeWidgetItem &other) const; From fb821731eb12906996bffdf4b3633d7fe47c85a7 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 27 Apr 2020 13:59:37 -0700 Subject: [PATCH 02/15] [net processing] ignore tx GETDATA from blocks-only peers Co-Authored-By: John Newbery Github-Pull: #18808 Rebased-From: 047ceac142246b5d51056a51dbf4645b31802be4 --- src/net_processing.cpp | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index d3089c41769..01067e58a6d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1564,15 +1564,13 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm std::vector vNotFound; const CNetMsgMaker msgMaker(pfrom->GetSendVersion()); - // Note that if we receive a getdata for a MSG_TX or MSG_WITNESS_TX from a - // block-relay-only outbound peer, we will stop processing further getdata - // messages from this peer (likely resulting in our peer eventually - // disconnecting us). - if (pfrom->m_tx_relay != nullptr) { - // mempool entries added before this time have likely expired from mapRelay - const std::chrono::seconds longlived_mempool_time = GetTime() - RELAY_TX_CACHE_TIME; - const std::chrono::seconds mempool_req = pfrom->m_tx_relay->m_last_mempool_req.load(); + // 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->m_tx_relay != nullptr ? pfrom->m_tx_relay->m_last_mempool_req.load() + : std::chrono::seconds::min(); + { LOCK(cs_main); while (it != pfrom->vRecvGetData.end() && (it->type == MSG_TX || it->type == MSG_WITNESS_TX)) { @@ -1582,8 +1580,12 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm if (pfrom->fPauseSend) break; - const CInv &inv = *it; - it++; + const CInv &inv = *it++; + + if (pfrom->m_tx_relay == nullptr) { + // Ignore GETDATA requests for transactions from blocks-only peers. + continue; + } // Send stream from relay memory bool push = false; From 1e73d7248a10863dc99a93f1db36d035c17f29d7 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 27 Apr 2020 14:00:21 -0700 Subject: [PATCH 03/15] [net processing] ignore unknown INV types in GETDATA messages Co-Authored-By: John Newbery Github-Pull: #18808 Rebased-From: e257cf71c851e25e1a533bf1d4296f6b55c81332 --- src/net_processing.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 01067e58a6d..a6f9445a9df 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1614,18 +1614,14 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm } // release cs_main if (it != pfrom->vRecvGetData.end() && !pfrom->fPauseSend) { - const CInv &inv = *it; + const CInv &inv = *it++; if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK || inv.type == MSG_CMPCT_BLOCK || inv.type == MSG_WITNESS_BLOCK) { - it++; ProcessGetBlockData(pfrom, chainparams, inv, connman); } + // else: If the first item on the queue is an unknown type, we erase it + // and continue processing the queue on the next call. } - // Unknown types in the GetData stay in vRecvGetData and block any future - // message from this peer, see vRecvGetData check in ProcessMessages(). - // Depending on future p2p changes, we might either drop unknown getdata on - // the floor or disconnect the peer. - pfrom->vRecvGetData.erase(pfrom->vRecvGetData.begin(), it); if (!vNotFound.empty()) { From 011532e380bb1a42eac9e79a17b35531f768becf Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 27 Apr 2020 14:52:10 -0700 Subject: [PATCH 04/15] [test] test that an invalid GETDATA doesn't prevent processing of future messages Co-Authored-By: John Newbery Github-Pull: #18808 Rebased-From: 2f032556e08a04807c71eb02104ca9589eaadf1b --- test/functional/p2p_getdata.py | 51 ++++++++++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 52 insertions(+) create mode 100755 test/functional/p2p_getdata.py diff --git a/test/functional/p2p_getdata.py b/test/functional/p2p_getdata.py new file mode 100755 index 00000000000..fd94a09d802 --- /dev/null +++ b/test/functional/p2p_getdata.py @@ -0,0 +1,51 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020 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 GETDATA processing behavior""" +from collections import defaultdict + +from test_framework.messages import ( + CInv, + msg_getdata, +) +from test_framework.mininode import ( + mininode_lock, + P2PInterface, +) +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import wait_until + +class P2PStoreBlock(P2PInterface): + + def __init__(self): + super().__init__() + self.blocks = defaultdict(int) + + def on_block(self, message): + message.block.calc_sha256() + self.blocks[message.block.sha256] += 1 + +class GetdataTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 1 + + def run_test(self): + self.nodes[0].add_p2p_connection(P2PStoreBlock()) + + self.log.info("test that an invalid GETDATA doesn't prevent processing of future messages") + + # Send invalid message and verify that node responds to later ping + invalid_getdata = msg_getdata() + invalid_getdata.inv.append(CInv(t=0, h=0)) # INV type 0 is invalid. + self.nodes[0].p2ps[0].send_and_ping(invalid_getdata) + + # Check getdata still works by fetching tip block + best_block = int(self.nodes[0].getbestblockhash(), 16) + good_getdata = msg_getdata() + good_getdata.inv.append(CInv(t=2, h=best_block)) + self.nodes[0].p2ps[0].send_and_ping(good_getdata) + wait_until(lambda: self.nodes[0].p2ps[0].blocks[best_block] == 1, timeout=30, lock=mininode_lock) + +if __name__ == '__main__': + GetdataTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index faa2dee4ed6..9f885ae4b09 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -145,6 +145,7 @@ BASE_SCRIPTS = [ 'rpc_deprecated.py', 'wallet_disable.py', 'p2p_addr_relay.py', + 'p2p_getdata.py', 'rpc_net.py', 'wallet_keypool.py', 'p2p_mempool.py', From a3fe458a1e477cacd19e7e0edb8e7bb965067115 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 28 Apr 2020 20:28:51 -0400 Subject: [PATCH 05/15] [docs] Improve commenting in ProcessGetData() Github-Pull: #18808 Rebased-From: 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7 --- src/net_processing.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a6f9445a9df..2db2619a811 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1573,10 +1573,14 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm { LOCK(cs_main); + // Process as many TX items from the front of the getdata queue as + // possible, since they're common and it's efficient to batch process + // them. while (it != pfrom->vRecvGetData.end() && (it->type == MSG_TX || it->type == MSG_WITNESS_TX)) { if (interruptMsgProc) return; - // Don't bother if send buffer is too full to respond anyway + // The send buffer provides backpressure. If there's no space in + // the buffer, pause processing until the next call. if (pfrom->fPauseSend) break; @@ -1613,6 +1617,8 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm } } // release cs_main + // Only process one BLOCK item per call, since they're uncommon and can be + // expensive to process. if (it != pfrom->vRecvGetData.end() && !pfrom->fPauseSend) { const CInv &inv = *it++; if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK || inv.type == MSG_CMPCT_BLOCK || inv.type == MSG_WITNESS_BLOCK) { From ca4dac48c5675af3fc53db6740a0b70fef622b0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Wed, 29 Apr 2020 10:40:39 +0100 Subject: [PATCH 06/15] rpc: Add mutex to guard deadlineTimers Github-Pull: #18814 Rebased-From: a2e6db5c4f1bb52a8814102b628e51652493d06a --- src/rpc/server.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index e2618c16dad..219979f0953 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -25,7 +25,8 @@ static std::string rpcWarmupStatus GUARDED_BY(cs_rpcWarmup) = "RPC server starte /* Timer-creating functions */ static RPCTimerInterface* timerInterface = nullptr; /* Map of name to timer. */ -static std::map > deadlineTimers; +static Mutex g_deadline_timers_mutex; +static std::map > deadlineTimers GUARDED_BY(g_deadline_timers_mutex); static bool ExecuteCommand(const CRPCCommand& command, const JSONRPCRequest& request, UniValue& result, bool last_handler); struct RPCCommandExecutionInfo @@ -298,7 +299,7 @@ void InterruptRPC() void StopRPC() { LogPrint(BCLog::RPC, "Stopping RPC\n"); - deadlineTimers.clear(); + WITH_LOCK(g_deadline_timers_mutex, deadlineTimers.clear()); DeleteAuthCookie(); g_rpcSignals.Stopped(); } @@ -486,6 +487,7 @@ void RPCRunLater(const std::string& name, std::function func, int64_t nS { if (!timerInterface) throw JSONRPCError(RPC_INTERNAL_ERROR, "No timer handler registered for RPC"); + LOCK(g_deadline_timers_mutex); deadlineTimers.erase(name); LogPrint(BCLog::RPC, "queue run of timer %s in %i seconds (using %s)\n", name, nSeconds, timerInterface->Name()); deadlineTimers.emplace(name, std::unique_ptr(timerInterface->NewTimer(func, nSeconds*1000))); From 251e321ad7d9ddb938e8a07ddfbe90739f0bafdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Wed, 29 Apr 2020 10:45:12 +0100 Subject: [PATCH 07/15] rpc: Relock wallet only if most recent callback Github-Pull: #18814 Rebased-From: 9f59dde9740d065118bdddde75ef9f4e4603a7b1 --- src/wallet/rpcwallet.cpp | 8 +++++++- src/wallet/wallet.h | 4 +++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 61ad2f11985..a5036413c82 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1917,6 +1917,9 @@ static UniValue walletpassphrase(const JSONRPCRequest& request) }.Check(request); int64_t nSleepTime; + int64_t relock_time; + // Prevent concurrent calls to walletpassphrase with the same wallet. + LOCK(pwallet->m_unlock_mutex); { auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); @@ -1955,6 +1958,7 @@ static UniValue walletpassphrase(const JSONRPCRequest& request) pwallet->TopUpKeyPool(); pwallet->nRelockTime = GetTime() + nSleepTime; + relock_time = pwallet->nRelockTime; } // rpcRunLater must be called without cs_wallet held otherwise a deadlock @@ -1966,9 +1970,11 @@ static UniValue walletpassphrase(const JSONRPCRequest& request) // wallet before the following callback is called. If a valid shared pointer // is acquired in the callback then the wallet is still loaded. std::weak_ptr weak_wallet = wallet; - pwallet->chain().rpcRunLater(strprintf("lockwallet(%s)", pwallet->GetName()), [weak_wallet] { + pwallet->chain().rpcRunLater(strprintf("lockwallet(%s)", pwallet->GetName()), [weak_wallet, relock_time] { if (auto shared_wallet = weak_wallet.lock()) { LOCK(shared_wallet->cs_wallet); + // Skip if this is not the most recent rpcRunLater callback. + if (shared_wallet->nRelockTime != relock_time) return; shared_wallet->Lock(); shared_wallet->nRelockTime = 0; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 6c54c72e76b..7446a4889a4 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -867,8 +867,10 @@ public: std::vector GetDestValues(const std::string& prefix) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Holds a timestamp at which point the wallet is scheduled (externally) to be relocked. Caller must arrange for actual relocking to occur via Lock(). - int64_t nRelockTime = 0; + int64_t nRelockTime GUARDED_BY(cs_wallet){0}; + // Used to prevent concurrent calls to walletpassphrase RPC. + Mutex m_unlock_mutex; bool Unlock(const SecureString& strWalletPassphrase, bool accept_no_keys = false); bool ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, const SecureString& strNewWalletPassphrase); bool EncryptWallet(const SecureString& strWalletPassphrase); From ed0afe8c1ff37926cc5bdcb0e8e4983e194e6d61 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Mon, 4 May 2020 22:27:19 -0400 Subject: [PATCH 08/15] test: Add test for conflicted wallet tx notifications Add test coverage for conflicted wallet transaction notifications so we improve current behavior and avoid future regressions https://github.com/bitcoin/bitcoin/pull/9240 - accidental break https://github.com/bitcoin/bitcoin/issues/9479 - bug report https://github.com/bitcoin/bitcoin/pull/9371 - fix https://github.com/bitcoin/bitcoin/pull/16624 - accidental break https://github.com/bitcoin/bitcoin/issues/18325 - bug report https://github.com/bitcoin/bitcoin/pull/18600 - potential fix Github-Pull: #18878 Rebased-From: f963a680515eda66429b3d1537a7baf281ab9283 --- test/functional/feature_notifications.py | 68 +++++++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-) diff --git a/test/functional/feature_notifications.py b/test/functional/feature_notifications.py index b110a559c09..47200b6cc6f 100755 --- a/test/functional/feature_notifications.py +++ b/test/functional/feature_notifications.py @@ -5,12 +5,14 @@ """Test the -alertnotify, -blocknotify and -walletnotify options.""" import os -from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE +from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE, keyhash_to_p2pkh from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, wait_until, connect_nodes, + disconnect_nodes, + hex_str_to_bytes, ) # Linux allow all characters other than \x00 @@ -81,8 +83,72 @@ class NotificationsTest(BitcoinTestFramework): # directory content should equal the generated transaction hashes txids_rpc = list(map(lambda t: notify_outputname(self.wallet, t['txid']), self.nodes[1].listtransactions("*", block_count))) assert_equal(sorted(txids_rpc), sorted(os.listdir(self.walletnotify_dir))) + for tx_file in os.listdir(self.walletnotify_dir): + os.remove(os.path.join(self.walletnotify_dir, tx_file)) + + # Conflicting transactions tests. Give node 0 same wallet seed as + # node 1, generate spends from node 0, and check notifications + # triggered by node 1 + self.log.info("test -walletnotify with conflicting transactions") + self.nodes[0].sethdseed(seed=self.nodes[1].dumpprivkey(keyhash_to_p2pkh(hex_str_to_bytes(self.nodes[1].getwalletinfo()['hdseedid'])[::-1]))) + self.nodes[0].rescanblockchain() + self.nodes[0].generatetoaddress(100, ADDRESS_BCRT1_UNSPENDABLE) + + # Generate transaction on node 0, sync mempools, and check for + # notification on node 1. + tx1 = self.nodes[0].sendtoaddress(address=ADDRESS_BCRT1_UNSPENDABLE, amount=1, replaceable=True) + assert_equal(tx1 in self.nodes[0].getrawmempool(), True) + self.sync_mempools() + self.expect_wallet_notify([tx1]) + + # Generate bump transaction, sync mempools, and check for bump1 + # notification. In the future, per + # https://github.com/bitcoin/bitcoin/pull/9371, it might be better + # to have notifications for both tx1 and bump1. + bump1 = self.nodes[0].bumpfee(tx1)["txid"] + assert_equal(bump1 in self.nodes[0].getrawmempool(), True) + self.sync_mempools() + self.expect_wallet_notify([bump1]) + + # Add bump1 transaction to new block, checking for a notification + # and the correct number of confirmations. + self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE) + self.sync_blocks() + self.expect_wallet_notify([bump1]) + assert_equal(self.nodes[1].gettransaction(bump1)["confirmations"], 1) + + # Generate a second transaction to be bumped. + tx2 = self.nodes[0].sendtoaddress(address=ADDRESS_BCRT1_UNSPENDABLE, amount=1, replaceable=True) + assert_equal(tx2 in self.nodes[0].getrawmempool(), True) + self.sync_mempools() + self.expect_wallet_notify([tx2]) + + # Bump tx2 as bump2 and generate a block on node 0 while + # disconnected, then reconnect and check for notifications on node 1 + # about newly confirmed bump2 and newly conflicted tx2. Currently + # only the bump2 notification is sent. Ideally, notifications would + # be sent both for bump2 and tx2, which was the previous behavior + # before being broken by an accidental change in PR + # https://github.com/bitcoin/bitcoin/pull/16624. The bug is reported + # in issue https://github.com/bitcoin/bitcoin/issues/18325. + disconnect_nodes(self.nodes[0], 1) + bump2 = self.nodes[0].bumpfee(tx2)["txid"] + self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE) + assert_equal(self.nodes[0].gettransaction(bump2)["confirmations"], 1) + assert_equal(tx2 in self.nodes[1].getrawmempool(), True) + connect_nodes(self.nodes[0], 1) + self.sync_blocks() + self.expect_wallet_notify([bump2]) + assert_equal(self.nodes[1].gettransaction(bump2)["confirmations"], 1) # TODO: add test for `-alertnotify` large fork notifications + def expect_wallet_notify(self, tx_ids): + wait_until(lambda: len(os.listdir(self.walletnotify_dir)) >= len(tx_ids), timeout=10) + assert_equal(sorted(notify_outputname(self.wallet, tx_id) for tx_id in tx_ids), sorted(os.listdir(self.walletnotify_dir))) + for tx_file in os.listdir(self.walletnotify_dir): + os.remove(os.path.join(self.walletnotify_dir, tx_file)) + + if __name__ == '__main__': NotificationsTest().main() From ff4dc2075031e9a49220cc27a270aeabe8954989 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Tue, 5 May 2020 23:56:21 +0100 Subject: [PATCH 09/15] gui: Fix manual coin control with multiple wallets loaded Github-Pull: #18894 Rebased-From: a8b5f1b133d4f23975a3fbfb7a415b17261466ee --- src/qt/coincontroldialog.cpp | 52 +++++++++++++++--------------------- src/qt/coincontroldialog.h | 8 +++--- src/qt/sendcoinsdialog.cpp | 50 +++++++++++++--------------------- src/qt/sendcoinsdialog.h | 2 ++ 4 files changed, 45 insertions(+), 67 deletions(-) diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp index ec61a4f5dd2..88e73043837 100644 --- a/src/qt/coincontroldialog.cpp +++ b/src/qt/coincontroldialog.cpp @@ -41,10 +41,11 @@ bool CCoinControlWidgetItem::operator<(const QTreeWidgetItem &other) const { return QTreeWidgetItem::operator<(other); } -CoinControlDialog::CoinControlDialog(const PlatformStyle *_platformStyle, QWidget *parent) : +CoinControlDialog::CoinControlDialog(CCoinControl& coin_control, WalletModel* _model, const PlatformStyle *_platformStyle, QWidget *parent) : QDialog(parent), ui(new Ui::CoinControlDialog), - model(nullptr), + m_coin_control(coin_control), + model(_model), platformStyle(_platformStyle) { ui->setupUi(this); @@ -134,6 +135,13 @@ CoinControlDialog::CoinControlDialog(const PlatformStyle *_platformStyle, QWidge ui->radioTreeMode->click(); if (settings.contains("nCoinControlSortColumn") && settings.contains("nCoinControlSortOrder")) sortView(settings.value("nCoinControlSortColumn").toInt(), (static_cast(settings.value("nCoinControlSortOrder").toInt()))); + + if(_model->getOptionsModel() && _model->getAddressTableModel()) + { + updateView(); + updateLabelLocked(); + CoinControlDialog::updateLabels(m_coin_control, _model, this); + } } CoinControlDialog::~CoinControlDialog() @@ -146,18 +154,6 @@ CoinControlDialog::~CoinControlDialog() delete ui; } -void CoinControlDialog::setModel(WalletModel *_model) -{ - this->model = _model; - - if(_model && _model->getOptionsModel() && _model->getAddressTableModel()) - { - updateView(); - updateLabelLocked(); - CoinControlDialog::updateLabels(_model, this); - } -} - // ok button void CoinControlDialog::buttonBoxClicked(QAbstractButton* button) { @@ -183,8 +179,8 @@ void CoinControlDialog::buttonSelectAllClicked() ui->treeWidget->topLevelItem(i)->setCheckState(COLUMN_CHECKBOX, state); ui->treeWidget->setEnabled(true); if (state == Qt::Unchecked) - coinControl()->UnSelectAll(); // just to be sure - CoinControlDialog::updateLabels(model, this); + m_coin_control.UnSelectAll(); // just to be sure + CoinControlDialog::updateLabels(m_coin_control, model, this); } // context menu @@ -369,15 +365,15 @@ void CoinControlDialog::viewItemChanged(QTreeWidgetItem* item, int column) COutPoint outpt(uint256S(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt()); if (item->checkState(COLUMN_CHECKBOX) == Qt::Unchecked) - coinControl()->UnSelect(outpt); + m_coin_control.UnSelect(outpt); else if (item->isDisabled()) // locked (this happens if "check all" through parent node) item->setCheckState(COLUMN_CHECKBOX, Qt::Unchecked); else - coinControl()->Select(outpt); + m_coin_control.Select(outpt); // selection changed -> update labels if (ui->treeWidget->isEnabled()) // do not update on every click for (un)select all - CoinControlDialog::updateLabels(model, this); + CoinControlDialog::updateLabels(m_coin_control, model, this); } // TODO: Remove this temporary qt5 fix after Qt5.3 and Qt5.4 are no longer used. @@ -402,7 +398,7 @@ void CoinControlDialog::updateLabelLocked() else ui->labelLocked->setVisible(false); } -void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) +void CoinControlDialog::updateLabels(CCoinControl& m_coin_control, WalletModel *model, QDialog* dialog) { if (!model) return; @@ -434,7 +430,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) bool fWitness = false; std::vector vCoinControl; - coinControl()->ListSelected(vCoinControl); + m_coin_control.ListSelected(vCoinControl); size_t i = 0; for (const auto& out : model->wallet().getCoins(vCoinControl)) { @@ -445,7 +441,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) const COutPoint& outpt = vCoinControl[i++]; if (out.is_spent) { - coinControl()->UnSelect(outpt); + m_coin_control.UnSelect(outpt); continue; } @@ -498,7 +494,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) nBytes -= 34; // Fee - nPayFee = model->wallet().getMinimumFee(nBytes, *coinControl(), nullptr /* returned_target */, nullptr /* reason */); + nPayFee = model->wallet().getMinimumFee(nBytes, m_coin_control, nullptr /* returned_target */, nullptr /* reason */); if (nPayAmount > 0) { @@ -590,12 +586,6 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) label->setVisible(nChange < 0); } -CCoinControl* CoinControlDialog::coinControl() -{ - static CCoinControl coin_control; - return &coin_control; -} - void CoinControlDialog::updateView() { if (!model || !model->getOptionsModel() || !model->getAddressTableModel()) @@ -695,13 +685,13 @@ void CoinControlDialog::updateView() // disable locked coins if (model->wallet().isLockedCoin(output)) { - coinControl()->UnSelect(output); // just to be sure + m_coin_control.UnSelect(output); // just to be sure itemOutput->setDisabled(true); itemOutput->setIcon(COLUMN_CHECKBOX, platformStyle->SingleColorIcon(":/icons/lock_closed")); } // set checkbox - if (coinControl()->IsSelected(output)) + if (m_coin_control.IsSelected(output)) itemOutput->setCheckState(COLUMN_CHECKBOX, Qt::Checked); } diff --git a/src/qt/coincontroldialog.h b/src/qt/coincontroldialog.h index fcf34356f11..c8a91e5f581 100644 --- a/src/qt/coincontroldialog.h +++ b/src/qt/coincontroldialog.h @@ -42,20 +42,18 @@ class CoinControlDialog : public QDialog Q_OBJECT public: - explicit CoinControlDialog(const PlatformStyle *platformStyle, QWidget *parent = nullptr); + explicit CoinControlDialog(CCoinControl& coin_control, WalletModel* model, const PlatformStyle *platformStyle, QWidget *parent = nullptr); ~CoinControlDialog(); - void setModel(WalletModel *model); - // static because also called from sendcoinsdialog - static void updateLabels(WalletModel*, QDialog*); + static void updateLabels(CCoinControl& m_coin_control, WalletModel*, QDialog*); static QList payAmounts; - static CCoinControl *coinControl(); static bool fSubtractFeeFromAmount; private: Ui::CoinControlDialog *ui; + CCoinControl& m_coin_control; WalletModel *model; int sortColumn; Qt::SortOrder sortOrder; diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index a8c82aaf6c4..2a6ef412212 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -57,6 +57,7 @@ SendCoinsDialog::SendCoinsDialog(const PlatformStyle *_platformStyle, QWidget *p ui(new Ui::SendCoinsDialog), clientModel(nullptr), model(nullptr), + m_coin_control(new CCoinControl), fNewRecipientAllowed(true), fFeeMinimized(true), platformStyle(_platformStyle) @@ -262,14 +263,9 @@ void SendCoinsDialog::on_sendButton_clicked() WalletModelTransaction currentTransaction(recipients); WalletModel::SendCoinsReturn prepareStatus; - // Always use a CCoinControl instance, use the CoinControlDialog instance if CoinControl has been enabled - CCoinControl ctrl; - if (model->getOptionsModel()->getCoinControlFeatures()) - ctrl = *CoinControlDialog::coinControl(); + updateCoinControlState(*m_coin_control); - updateCoinControlState(ctrl); - - prepareStatus = model->prepareTransaction(currentTransaction, ctrl); + prepareStatus = model->prepareTransaction(currentTransaction, *m_coin_control); // process prepareStatus and on error generate message shown to user processSendCoinsReturn(prepareStatus, @@ -413,7 +409,7 @@ void SendCoinsDialog::on_sendButton_clicked() } if (!send_failure) { accept(); - CoinControlDialog::coinControl()->UnSelectAll(); + m_coin_control->UnSelectAll(); coinControlUpdateLabels(); } fNewRecipientAllowed = true; @@ -422,7 +418,7 @@ void SendCoinsDialog::on_sendButton_clicked() void SendCoinsDialog::clear() { // Clear coin control settings - CoinControlDialog::coinControl()->UnSelectAll(); + m_coin_control->UnSelectAll(); ui->checkBoxCoinControlChange->setChecked(false); ui->lineEditCoinControlChange->clear(); coinControlUpdateLabels(); @@ -645,17 +641,11 @@ void SendCoinsDialog::on_buttonMinimizeFee_clicked() void SendCoinsDialog::useAvailableBalance(SendCoinsEntry* entry) { - // Get CCoinControl instance if CoinControl is enabled or create a new one. - CCoinControl coin_control; - if (model->getOptionsModel()->getCoinControlFeatures()) { - coin_control = *CoinControlDialog::coinControl(); - } - // Include watch-only for wallets without private key - coin_control.fAllowWatchOnly = model->wallet().privateKeysDisabled(); + m_coin_control->fAllowWatchOnly = model->wallet().privateKeysDisabled(); // Calculate available amount to send. - CAmount amount = model->wallet().getAvailableBalance(coin_control); + CAmount amount = model->wallet().getAvailableBalance(*m_coin_control); for (int i = 0; i < ui->entries->count(); ++i) { SendCoinsEntry* e = qobject_cast(ui->entries->itemAt(i)->widget()); if (e && !e->isHidden() && e != entry) { @@ -714,12 +704,11 @@ void SendCoinsDialog::updateSmartFeeLabel() { if(!model || !model->getOptionsModel()) return; - CCoinControl coin_control; - updateCoinControlState(coin_control); - coin_control.m_feerate.reset(); // Explicitly use only fee estimation rate for smart fee labels + updateCoinControlState(*m_coin_control); + m_coin_control->m_feerate.reset(); // Explicitly use only fee estimation rate for smart fee labels int returned_target; FeeReason reason; - CFeeRate feeRate = CFeeRate(model->wallet().getMinimumFee(1000, coin_control, &returned_target, &reason)); + CFeeRate feeRate = CFeeRate(model->wallet().getMinimumFee(1000, *m_coin_control, &returned_target, &reason)); ui->labelSmartFee->setText(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), feeRate.GetFeePerK()) + "/kB"); @@ -790,7 +779,7 @@ void SendCoinsDialog::coinControlFeatureChanged(bool checked) ui->frameCoinControl->setVisible(checked); if (!checked && model) // coin control features disabled - CoinControlDialog::coinControl()->SetNull(); + m_coin_control->SetNull(); coinControlUpdateLabels(); } @@ -798,8 +787,7 @@ void SendCoinsDialog::coinControlFeatureChanged(bool checked) // Coin Control: button inputs -> show actual coin control dialog void SendCoinsDialog::coinControlButtonClicked() { - CoinControlDialog dlg(platformStyle); - dlg.setModel(model); + CoinControlDialog dlg(*m_coin_control, model, platformStyle); dlg.exec(); coinControlUpdateLabels(); } @@ -809,7 +797,7 @@ void SendCoinsDialog::coinControlChangeChecked(int state) { if (state == Qt::Unchecked) { - CoinControlDialog::coinControl()->destChange = CNoDestination(); + m_coin_control->destChange = CNoDestination(); ui->labelCoinControlChangeLabel->clear(); } else @@ -825,7 +813,7 @@ void SendCoinsDialog::coinControlChangeEdited(const QString& text) if (model && model->getAddressTableModel()) { // Default to no change address until verified - CoinControlDialog::coinControl()->destChange = CNoDestination(); + m_coin_control->destChange = CNoDestination(); ui->labelCoinControlChangeLabel->setStyleSheet("QLabel{color:red;}"); const CTxDestination dest = DecodeDestination(text.toStdString()); @@ -848,7 +836,7 @@ void SendCoinsDialog::coinControlChangeEdited(const QString& text) QMessageBox::Yes | QMessageBox::Cancel, QMessageBox::Cancel); if(btnRetVal == QMessageBox::Yes) - CoinControlDialog::coinControl()->destChange = dest; + m_coin_control->destChange = dest; else { ui->lineEditCoinControlChange->setText(""); @@ -867,7 +855,7 @@ void SendCoinsDialog::coinControlChangeEdited(const QString& text) else ui->labelCoinControlChangeLabel->setText(tr("(no label)")); - CoinControlDialog::coinControl()->destChange = dest; + m_coin_control->destChange = dest; } } } @@ -879,7 +867,7 @@ void SendCoinsDialog::coinControlUpdateLabels() if (!model || !model->getOptionsModel()) return; - updateCoinControlState(*CoinControlDialog::coinControl()); + updateCoinControlState(*m_coin_control); // set pay amounts CoinControlDialog::payAmounts.clear(); @@ -897,10 +885,10 @@ void SendCoinsDialog::coinControlUpdateLabels() } } - if (CoinControlDialog::coinControl()->HasSelected()) + if (m_coin_control->HasSelected()) { // actual coin control calculation - CoinControlDialog::updateLabels(model, this); + CoinControlDialog::updateLabels(*m_coin_control, model, this); // show coin control stats ui->labelCoinControlAutomaticallySelected->hide(); diff --git a/src/qt/sendcoinsdialog.h b/src/qt/sendcoinsdialog.h index 86422c40303..8eadc5a2255 100644 --- a/src/qt/sendcoinsdialog.h +++ b/src/qt/sendcoinsdialog.h @@ -12,6 +12,7 @@ #include #include +class CCoinControl; class ClientModel; class PlatformStyle; class SendCoinsEntry; @@ -60,6 +61,7 @@ private: Ui::SendCoinsDialog *ui; ClientModel *clientModel; WalletModel *model; + std::unique_ptr m_coin_control; bool fNewRecipientAllowed; bool fFeeMinimized; const PlatformStyle *platformStyle; From 37a620748bd3578eda1c74daad8df8451d13b989 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 23 Apr 2020 14:04:02 -0400 Subject: [PATCH 10/15] test: Add unregister_validation_interface_race test This commit is (intentionally) adding a broken test. The test is broken because it registering a subscriber object that can go out of scope while events are still being sent. To run the broken test and reproduce the bug: - Remove comment /** and */ - ./configure --with-sanitizers=address - export ASAN_OPTIONS=detect_leaks=0 - make - while ./src/test/test_bitcoin -t validationinterface_tests/unregister_validation_interface_race --catch_system_errors=no ; do true; done Github-Pull: #18742 Rebased-From: fab6d060ce5f580db538070beec1c5518c8c777c --- src/test/validationinterface_tests.cpp | 36 ++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/test/validationinterface_tests.cpp b/src/test/validationinterface_tests.cpp index 208be928521..14f09ae9050 100644 --- a/src/test/validationinterface_tests.cpp +++ b/src/test/validationinterface_tests.cpp @@ -12,6 +12,42 @@ BOOST_FIXTURE_TEST_SUITE(validationinterface_tests, TestingSetup) +/** +struct TestSubscriberNoop final : public CValidationInterface { + void BlockChecked(const CBlock&, const BlockValidationState&) override {} +}; + +BOOST_AUTO_TEST_CASE(unregister_validation_interface_race) +{ + std::atomic generate{true}; + + // Start thread to generate notifications + std::thread gen{[&] { + const CBlock block_dummy; + const BlockValidationState state_dummy; + while (generate) { + GetMainSignals().BlockChecked(block_dummy, state_dummy); + } + }}; + + // Start thread to consume notifications + std::thread sub{[&] { + // keep going for about 1 sec, which is 250k iterations + for (int i = 0; i < 250000; i++) { + TestSubscriberNoop sub{}; + RegisterValidationInterface(&sub); + UnregisterValidationInterface(&sub); + } + // tell the other thread we are done + generate = false; + }}; + + gen.join(); + sub.join(); + BOOST_CHECK(!generate); +} +*/ + class TestInterface : public CValidationInterface { public: From cc7d34465bbb0195d8bcd9143097840a2e9765f2 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 27 Apr 2020 10:35:32 -0400 Subject: [PATCH 11/15] miner: Avoid stack-use-after-return in validationinterface This is achieved by switching to a shared_ptr. Also, switch the validationinterfaces in the tests to use shared_ptrs for the same reason. Github-Pull: #18742 Rebased-From: 7777f2a4bb1f9d843bc50a4e35085cfbb2808780 --- src/rpc/mining.cpp | 12 ++++++------ src/test/validation_block_tests.cpp | 10 +++++----- src/test/validationinterface_tests.cpp | 8 +++----- 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index da9d583fa79..94590e0da44 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -724,7 +724,7 @@ static UniValue getblocktemplate(const JSONRPCRequest& request) return result; } -class submitblock_StateCatcher : public CValidationInterface +class submitblock_StateCatcher final : public CValidationInterface { public: uint256 hash; @@ -792,17 +792,17 @@ static UniValue submitblock(const JSONRPCRequest& request) } bool new_block; - submitblock_StateCatcher sc(block.GetHash()); - RegisterValidationInterface(&sc); + auto sc = std::make_shared(block.GetHash()); + RegisterSharedValidationInterface(sc); bool accepted = ProcessNewBlock(Params(), blockptr, /* fForceProcessing */ true, /* fNewBlock */ &new_block); - UnregisterValidationInterface(&sc); + UnregisterSharedValidationInterface(sc); if (!new_block && accepted) { return "duplicate"; } - if (!sc.found) { + if (!sc->found) { return "inconclusive"; } - return BIP22ValidationResult(sc.state); + return BIP22ValidationResult(sc->state); } static UniValue submitheader(const JSONRPCRequest& request) diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index afb3db36a2a..e61ea13b021 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -32,7 +32,7 @@ struct MinerTestingSetup : public RegTestingSetup { BOOST_FIXTURE_TEST_SUITE(validation_block_tests, MinerTestingSetup) -struct TestSubscriber : public CValidationInterface { +struct TestSubscriber final : public CValidationInterface { uint256 m_expected_tip; explicit TestSubscriber(uint256 tip) : m_expected_tip(tip) {} @@ -175,8 +175,8 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering) LOCK(cs_main); initial_tip = ::ChainActive().Tip(); } - TestSubscriber sub(initial_tip->GetBlockHash()); - RegisterValidationInterface(&sub); + auto sub = std::make_shared(initial_tip->GetBlockHash()); + RegisterSharedValidationInterface(sub); // create a bunch of threads that repeatedly process a block generated above at random // this will create parallelism and randomness inside validation - the ValidationInterface @@ -208,10 +208,10 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering) UninterruptibleSleep(std::chrono::milliseconds{100}); } - UnregisterValidationInterface(&sub); + UnregisterSharedValidationInterface(sub); LOCK(cs_main); - BOOST_CHECK_EQUAL(sub.m_expected_tip, ::ChainActive().Tip()->GetBlockHash()); + BOOST_CHECK_EQUAL(sub->m_expected_tip, ::ChainActive().Tip()->GetBlockHash()); } /** diff --git a/src/test/validationinterface_tests.cpp b/src/test/validationinterface_tests.cpp index 14f09ae9050..d2fc20e625a 100644 --- a/src/test/validationinterface_tests.cpp +++ b/src/test/validationinterface_tests.cpp @@ -12,7 +12,6 @@ BOOST_FIXTURE_TEST_SUITE(validationinterface_tests, TestingSetup) -/** struct TestSubscriberNoop final : public CValidationInterface { void BlockChecked(const CBlock&, const BlockValidationState&) override {} }; @@ -34,9 +33,9 @@ BOOST_AUTO_TEST_CASE(unregister_validation_interface_race) std::thread sub{[&] { // keep going for about 1 sec, which is 250k iterations for (int i = 0; i < 250000; i++) { - TestSubscriberNoop sub{}; - RegisterValidationInterface(&sub); - UnregisterValidationInterface(&sub); + auto sub = std::make_shared(); + RegisterSharedValidationInterface(sub); + UnregisterSharedValidationInterface(sub); } // tell the other thread we are done generate = false; @@ -46,7 +45,6 @@ BOOST_AUTO_TEST_CASE(unregister_validation_interface_race) sub.join(); BOOST_CHECK(!generate); } -*/ class TestInterface : public CValidationInterface { From cf2a6e2a390ad18a616d7f2718688375f2576577 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 14 May 2020 18:31:57 +0200 Subject: [PATCH 12/15] test: Remove const to work around compiler error on xenial Fix the following error in travis: test/validationinterface_tests.cpp:26:36: error: default initialization of an object of const type 'const BlockValidationState' without a user-provided default constructor const BlockValidationState state_dummy; Github-Pull: #18975 Rebased-From: 050e2ee6f28e7b31c167013be7313726e34084e9 --- src/test/validationinterface_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/validationinterface_tests.cpp b/src/test/validationinterface_tests.cpp index d2fc20e625a..ceba689e529 100644 --- a/src/test/validationinterface_tests.cpp +++ b/src/test/validationinterface_tests.cpp @@ -23,7 +23,7 @@ BOOST_AUTO_TEST_CASE(unregister_validation_interface_race) // Start thread to generate notifications std::thread gen{[&] { const CBlock block_dummy; - const BlockValidationState state_dummy; + BlockValidationState state_dummy; while (generate) { GetMainSignals().BlockChecked(block_dummy, state_dummy); } From 6161c94a6108ebddafe4e95c14bde4cdc3f8c01c Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 8 May 2020 14:18:13 -0400 Subject: [PATCH 13/15] [net processing] Only send a getheaders for one block in an INV Headers-first is the primary method of announcement on the network. If a node fell back sending blocks by inv, it's probably for a re-org. The final block hash provided should be the highest, so send a getheaders and then fetch the blocks we need to catch up. Github-Pull: #18962 Rebased-From: 746736639e6d05acdb85c866d4c605c947d4c500 --- src/net_processing.cpp | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 2db2619a811..2a61f84a085 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2261,6 +2261,7 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec uint32_t nFetchFlags = GetFetchFlags(pfrom); const auto current_time = GetTime(); + uint256* best_block{nullptr}; for (CInv &inv : vInv) { @@ -2277,17 +2278,14 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec if (inv.type == MSG_BLOCK) { UpdateBlockAvailability(pfrom->GetId(), inv.hash); if (!fAlreadyHave && !fImporting && !fReindex && !mapBlocksInFlight.count(inv.hash)) { - // We used to request the full block here, but since headers-announcements are now the - // primary method of announcement on the network, and since, in the case that a node - // fell back to inv we probably have a reorg which we should get the headers for first, - // we now only provide a getheaders response here. When we receive the headers, we will - // then ask for the blocks we need. - connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexBestHeader), inv.hash)); - LogPrint(BCLog::NET, "getheaders (%d) %s to peer=%d\n", pindexBestHeader->nHeight, inv.hash.ToString(), pfrom->GetId()); + // Headers-first is the primary method of announcement on + // the network. If a node fell back to sending blocks by inv, + // it's probably for a re-org. The final block hash + // provided should be the highest, so send a getheaders and + // then fetch the blocks we need to catch up. + best_block = &inv.hash; } - } - else - { + } else { pfrom->AddInventoryKnown(inv); if (fBlocksOnly) { LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol, disconnecting peer=%d\n", inv.hash.ToString(), pfrom->GetId()); @@ -2298,6 +2296,12 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec } } } + + if (best_block != nullptr) { + connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexBestHeader), *best_block)); + LogPrint(BCLog::NET, "getheaders (%d) %s to peer=%d\n", pindexBestHeader->nHeight, best_block->ToString(), pfrom->GetId()); + } + return true; } From 9a8fb4cf4ba472a5c3e1b9b71d31673f881a4896 Mon Sep 17 00:00:00 2001 From: practicalswift Date: Fri, 24 Apr 2020 12:29:47 +0000 Subject: [PATCH 14/15] fuzz: Remove enumeration of expected deserialization exceptions in ProcessMessage(...) fuzzer Github-Pull: #18757 Rebased-From: fdceb6328382ac0f9d643f9d42ba0509062d7d48 --- src/test/fuzz/process_message.cpp | 24 +----------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index 9e3586d162d..42550319046 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -19,16 +19,13 @@ #include #include -#include #include #include #include #include #include #include -#include #include -#include #include #include @@ -44,19 +41,6 @@ const std::string LIMIT_TO_MESSAGE_TYPE{TO_STRING(MESSAGE_TYPE)}; const std::string LIMIT_TO_MESSAGE_TYPE; #endif -const std::map> EXPECTED_DESERIALIZATION_EXCEPTIONS = { - {"CDataStream::read(): end of data: iostream error", {"addr", "block", "blocktxn", "cmpctblock", "feefilter", "filteradd", "filterload", "getblocks", "getblocktxn", "getdata", "getheaders", "headers", "inv", "notfound", "ping", "sendcmpct", "tx"}}, - {"CompactSize exceeds limit of type: iostream error", {"cmpctblock"}}, - {"differential value overflow: iostream error", {"getblocktxn"}}, - {"index overflowed 16 bits: iostream error", {"getblocktxn"}}, - {"index overflowed 16-bits: iostream error", {"cmpctblock"}}, - {"indexes overflowed 16 bits: iostream error", {"getblocktxn"}}, - {"non-canonical ReadCompactSize(): iostream error", {"addr", "block", "blocktxn", "cmpctblock", "filteradd", "filterload", "getblocks", "getblocktxn", "getdata", "getheaders", "headers", "inv", "notfound", "tx"}}, - {"ReadCompactSize(): size too large: iostream error", {"addr", "block", "blocktxn", "cmpctblock", "filteradd", "filterload", "getblocks", "getblocktxn", "getdata", "getheaders", "headers", "inv", "notfound", "tx"}}, - {"Superfluous witness record: iostream error", {"block", "blocktxn", "cmpctblock", "tx"}}, - {"Unknown transaction optional data: iostream error", {"block", "blocktxn", "cmpctblock", "tx"}}, -}; - const RegTestingSetup* g_setup; } // namespace @@ -86,13 +70,7 @@ void test_one_input(const std::vector& buffer) g_setup->m_node.peer_logic->InitializeNode(&p2p_node); try { (void)ProcessMessage(&p2p_node, random_message_type, random_bytes_data_stream, GetTimeMillis(), Params(), *g_setup->m_node.mempool, g_setup->m_node.connman.get(), g_setup->m_node.banman.get(), std::atomic{false}); - } catch (const std::ios_base::failure& e) { - const std::string exception_message{e.what()}; - const auto p = EXPECTED_DESERIALIZATION_EXCEPTIONS.find(exception_message); - if (p == EXPECTED_DESERIALIZATION_EXCEPTIONS.cend() || p->second.count(random_message_type) == 0) { - std::cout << "Unexpected exception when processing message type \"" << random_message_type << "\": " << exception_message << std::endl; - assert(false); - } + } catch (const std::ios_base::failure&) { } SyncWithValidationInterfaceQueue(); } From 245c862cfd4883ea91b53d766abb00a9c3c1ea5c Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 15 May 2020 15:13:56 +0800 Subject: [PATCH 15/15] test: disable script fuzz tests Given that #18413 has not been backported. --- src/Makefile.test.include | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/Makefile.test.include b/src/Makefile.test.include index d53477793cd..56c887af787 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -104,8 +104,6 @@ FUZZ_TARGETS = \ test/fuzz/script \ test/fuzz/script_deserialize \ test/fuzz/script_flags \ - test/fuzz/script_ops \ - test/fuzz/scriptnum_ops \ test/fuzz/service_deserialize \ test/fuzz/signature_checker \ test/fuzz/snapshotmetadata_deserialize \ @@ -893,17 +891,18 @@ test_fuzz_script_flags_LDADD = $(FUZZ_SUITE_LD_COMMON) test_fuzz_script_flags_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) test_fuzz_script_flags_SOURCES = test/fuzz/script_flags.cpp -test_fuzz_script_ops_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) -test_fuzz_script_ops_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) -test_fuzz_script_ops_LDADD = $(FUZZ_SUITE_LD_COMMON) -test_fuzz_script_ops_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) -test_fuzz_script_ops_SOURCES = test/fuzz/script_ops.cpp +# Disabled for now, as #18413 has not been backported. +# test_fuzz_script_ops_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) +# test_fuzz_script_ops_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) +# test_fuzz_script_ops_LDADD = $(FUZZ_SUITE_LD_COMMON) +# test_fuzz_script_ops_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) +# test_fuzz_script_ops_SOURCES = test/fuzz/script_ops.cpp -test_fuzz_scriptnum_ops_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) -test_fuzz_scriptnum_ops_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) -test_fuzz_scriptnum_ops_LDADD = $(FUZZ_SUITE_LD_COMMON) -test_fuzz_scriptnum_ops_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) -test_fuzz_scriptnum_ops_SOURCES = test/fuzz/scriptnum_ops.cpp +# test_fuzz_scriptnum_ops_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) +# test_fuzz_scriptnum_ops_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) +# test_fuzz_scriptnum_ops_LDADD = $(FUZZ_SUITE_LD_COMMON) +# test_fuzz_scriptnum_ops_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) +# test_fuzz_scriptnum_ops_SOURCES = test/fuzz/scriptnum_ops.cpp test_fuzz_service_deserialize_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) -DSERVICE_DESERIALIZE=1 test_fuzz_service_deserialize_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)