From 6971e790c32253ecddb6108e796afd3892ced7fe Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Fri, 23 Apr 2021 14:57:05 +0200 Subject: [PATCH 01/35] gui: add Direction column to peers tab Co-authored-by: Jarol Rodriguez --- src/qt/peertablemodel.cpp | 10 ++++++++-- src/qt/peertablemodel.h | 4 ++++ src/qt/peertablesortproxy.cpp | 2 ++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/qt/peertablemodel.cpp b/src/qt/peertablemodel.cpp index 11441481bb..1df02b7a93 100644 --- a/src/qt/peertablemodel.cpp +++ b/src/qt/peertablemodel.cpp @@ -113,8 +113,13 @@ QVariant PeerTableModel::data(const QModelIndex &index, int role) const case NetNodeId: return (qint64)rec->nodeStats.nodeid; case Address: - // prepend to peer address down-arrow symbol for inbound connection and up-arrow for outbound connection - return QString::fromStdString((rec->nodeStats.fInbound ? "↓ " : "↑ ") + rec->nodeStats.addrName); + return QString::fromStdString(rec->nodeStats.addrName); + case Direction: + return QString(rec->nodeStats.fInbound ? + //: An Inbound Connection from a Peer. + tr("Inbound") : + //: An Outbound Connection to a Peer. + tr("Outbound")); case ConnectionType: return GUIUtil::ConnectionTypeToQString(rec->nodeStats.m_conn_type, /* prepend_direction */ false); case Network: @@ -134,6 +139,7 @@ QVariant PeerTableModel::data(const QModelIndex &index, int role) const case NetNodeId: case Address: return {}; + case Direction: case ConnectionType: case Network: return QVariant(Qt::AlignCenter); diff --git a/src/qt/peertablemodel.h b/src/qt/peertablemodel.h index 3d195342f1..f08f315324 100644 --- a/src/qt/peertablemodel.h +++ b/src/qt/peertablemodel.h @@ -47,6 +47,7 @@ class PeerTableModel : public QAbstractTableModel enum ColumnIndex { NetNodeId = 0, Address, + Direction, ConnectionType, Network, Ping, @@ -81,6 +82,9 @@ public Q_SLOTS: /*: Title of Peers Table column which contains the IP/Onion/I2P address of the connected peer. */ tr("Address"), + /*: Title of Peers Table column which indicates the direction + the peer connection was initiated from. */ + tr("Direction"), /*: Title of Peers Table column which describes the type of peer connection. The "type" describes why the connection exists. */ tr("Type"), diff --git a/src/qt/peertablesortproxy.cpp b/src/qt/peertablesortproxy.cpp index 78932da8d4..f92eef48f1 100644 --- a/src/qt/peertablesortproxy.cpp +++ b/src/qt/peertablesortproxy.cpp @@ -26,6 +26,8 @@ bool PeerTableSortProxy::lessThan(const QModelIndex& left_index, const QModelInd return left_stats.nodeid < right_stats.nodeid; case PeerTableModel::Address: return left_stats.addrName.compare(right_stats.addrName) < 0; + case PeerTableModel::Direction: + return left_stats.fInbound > right_stats.fInbound; // default sort Inbound, then Outbound case PeerTableModel::ConnectionType: return left_stats.m_conn_type < right_stats.m_conn_type; case PeerTableModel::Network: From 93cc53a2b27eeb05fe00b8bf17465037815473a1 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 5 Dec 2017 15:57:12 -0500 Subject: [PATCH 02/35] gui: Unregister wallet notifications before unloading wallets This change was originally part of both bitcoin/bitcoin#10102 and bitcoin/bitcoin#19101 and is required for both because it avoids the IPC wallet implementation in bitcoin/bitcoin#10102 and the WalletContext implementation in bitcoin/bitcoin#19101 needing to deal with notification objects that have stale pointers to deleted wallets. --- src/qt/bitcoin.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 9e6cf56d31..772078ad92 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -352,6 +352,17 @@ void BitcoinApplication::requestShutdown() window->setClientModel(nullptr); pollShutdownTimer->stop(); +#ifdef ENABLE_WALLET + // Delete wallet controller here manually, instead of relying on Qt object + // tracking (https://doc.qt.io/qt-5/objecttrees.html). This makes sure + // walletmodel m_handle_* notification handlers are deleted before wallets + // are unloaded, which can simplify wallet implementations. It also avoids + // these notifications having to be handled while GUI objects are being + // destroyed, making GUI code less fragile as well. + delete m_wallet_controller; + m_wallet_controller = nullptr; +#endif // ENABLE_WALLET + delete clientModel; clientModel = nullptr; From 4830f4912a538600e9e9133442e9f8d929006630 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Thu, 3 Jun 2021 09:54:25 +0100 Subject: [PATCH 03/35] qt: Refactor open date range to use std::optional --- src/qt/transactionfilterproxy.cpp | 17 +++++------------ src/qt/transactionfilterproxy.h | 13 ++++++------- src/qt/transactionview.cpp | 14 ++++++++------ 3 files changed, 19 insertions(+), 25 deletions(-) diff --git a/src/qt/transactionfilterproxy.cpp b/src/qt/transactionfilterproxy.cpp index a631f497af..75cbd6b3be 100644 --- a/src/qt/transactionfilterproxy.cpp +++ b/src/qt/transactionfilterproxy.cpp @@ -9,15 +9,8 @@ #include -// Earliest date that can be represented (far in the past) -const QDateTime TransactionFilterProxy::MIN_DATE = QDateTime::fromTime_t(0); -// Last date that can be represented (far in the future) -const QDateTime TransactionFilterProxy::MAX_DATE = QDateTime::fromTime_t(0xFFFFFFFF); - TransactionFilterProxy::TransactionFilterProxy(QObject *parent) : QSortFilterProxyModel(parent), - dateFrom(MIN_DATE), - dateTo(MAX_DATE), m_search_string(), typeFilter(ALL_TYPES), watchOnlyFilter(WatchOnlyFilter_All), @@ -46,8 +39,8 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex & return false; QDateTime datetime = index.data(TransactionTableModel::DateRole).toDateTime(); - if (datetime < dateFrom || datetime > dateTo) - return false; + if (dateFrom && datetime < *dateFrom) return false; + if (dateTo && datetime > *dateTo) return false; QString address = index.data(TransactionTableModel::AddressRole).toString(); QString label = index.data(TransactionTableModel::LabelRole).toString(); @@ -65,10 +58,10 @@ bool TransactionFilterProxy::filterAcceptsRow(int sourceRow, const QModelIndex & return true; } -void TransactionFilterProxy::setDateRange(const QDateTime &from, const QDateTime &to) +void TransactionFilterProxy::setDateRange(const std::optional& from, const std::optional& to) { - this->dateFrom = from; - this->dateTo = to; + dateFrom = from; + dateTo = to; invalidateFilter(); } diff --git a/src/qt/transactionfilterproxy.h b/src/qt/transactionfilterproxy.h index 693b363692..09bc9e75db 100644 --- a/src/qt/transactionfilterproxy.h +++ b/src/qt/transactionfilterproxy.h @@ -10,6 +10,8 @@ #include #include +#include + /** Filter the transaction list according to pre-specified rules. */ class TransactionFilterProxy : public QSortFilterProxyModel { @@ -18,10 +20,6 @@ class TransactionFilterProxy : public QSortFilterProxyModel public: explicit TransactionFilterProxy(QObject *parent = nullptr); - /** Earliest date that can be represented (far in the past) */ - static const QDateTime MIN_DATE; - /** Last date that can be represented (far in the future) */ - static const QDateTime MAX_DATE; /** Type filter bit field (all types) */ static const quint32 ALL_TYPES = 0xFFFFFFFF; @@ -34,7 +32,8 @@ class TransactionFilterProxy : public QSortFilterProxyModel WatchOnlyFilter_No }; - void setDateRange(const QDateTime &from, const QDateTime &to); + /** Filter transactions between date range. Use std::nullopt for open range. */ + void setDateRange(const std::optional& from, const std::optional& to); void setSearchString(const QString &); /** @note Type filter takes a bit field created with TYPE() or ALL_TYPES @@ -55,8 +54,8 @@ class TransactionFilterProxy : public QSortFilterProxyModel bool filterAcceptsRow(int source_row, const QModelIndex & source_parent) const override; private: - QDateTime dateFrom; - QDateTime dateTo; + std::optional dateFrom; + std::optional dateTo; QString m_search_string; quint32 typeFilter; WatchOnlyFilter watchOnlyFilter; diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp index 1e8e012dcf..e7e87c4e5e 100644 --- a/src/qt/transactionview.cpp +++ b/src/qt/transactionview.cpp @@ -19,6 +19,8 @@ #include +#include + #include #include #include @@ -266,26 +268,26 @@ void TransactionView::chooseDate(int idx) { case All: transactionProxyModel->setDateRange( - TransactionFilterProxy::MIN_DATE, - TransactionFilterProxy::MAX_DATE); + std::nullopt, + std::nullopt); break; case Today: transactionProxyModel->setDateRange( GUIUtil::StartOfDay(current), - TransactionFilterProxy::MAX_DATE); + std::nullopt); break; case ThisWeek: { // Find last Monday QDate startOfWeek = current.addDays(-(current.dayOfWeek()-1)); transactionProxyModel->setDateRange( GUIUtil::StartOfDay(startOfWeek), - TransactionFilterProxy::MAX_DATE); + std::nullopt); } break; case ThisMonth: transactionProxyModel->setDateRange( GUIUtil::StartOfDay(QDate(current.year(), current.month(), 1)), - TransactionFilterProxy::MAX_DATE); + std::nullopt); break; case LastMonth: transactionProxyModel->setDateRange( @@ -295,7 +297,7 @@ void TransactionView::chooseDate(int idx) case ThisYear: transactionProxyModel->setDateRange( GUIUtil::StartOfDay(QDate(current.year(), 1, 1)), - TransactionFilterProxy::MAX_DATE); + std::nullopt); break; case Range: dateRangeWidget->setVisible(true); From b851a92c06cd1d6a40a5527ae39d60ae7fa33fb9 Mon Sep 17 00:00:00 2001 From: klementtan Date: Mon, 26 Jul 2021 00:08:19 +0800 Subject: [PATCH 04/35] cli: Add progress bar for -getinfo Co-authored-by: jonatack --- src/bitcoin-cli.cpp | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index 1ec6411e32..bc0af6398c 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -884,6 +884,29 @@ static void GetWalletBalances(UniValue& result) result.pushKV("balances", balances); } +/** + * GetProgressBar contructs a progress bar with 5% intervals. + * + * @param[in] progress The proportion of the progress bar to be filled between 0 and 1. + * @param[out] progress_bar String representation of the progress bar. + */ +static void GetProgressBar(double progress, std::string& progress_bar) +{ + if (progress < 0 || progress > 1) return; + + static constexpr double INCREMENT{0.05}; + static const std::string COMPLETE_BAR{"\u2592"}; + static const std::string INCOMPLETE_BAR{"\u2591"}; + + for (int i = 0; i < progress / INCREMENT; ++i) { + progress_bar += COMPLETE_BAR; + } + + for (int i = 0; i < (1 - progress) / INCREMENT; ++i) { + progress_bar += INCOMPLETE_BAR; + } +} + /** * ParseGetInfoResult takes in -getinfo result in UniValue object and parses it * into a user friendly UniValue string to be printed on the console. @@ -926,7 +949,17 @@ static void ParseGetInfoResult(UniValue& result) std::string result_string = strprintf("%sChain: %s%s\n", BLUE, result["chain"].getValStr(), RESET); result_string += strprintf("Blocks: %s\n", result["blocks"].getValStr()); result_string += strprintf("Headers: %s\n", result["headers"].getValStr()); - result_string += strprintf("Verification progress: %.4f%%\n", result["verificationprogress"].get_real() * 100); + + const double ibd_progress{result["verificationprogress"].get_real()}; + std::string ibd_progress_bar; + // Display the progress bar only if IBD progress is less than 99% + if (ibd_progress < 0.99) { + GetProgressBar(ibd_progress, ibd_progress_bar); + // Add padding between progress bar and IBD progress + ibd_progress_bar += " "; + } + + result_string += strprintf("Verification progress: %s%.4f%%\n", ibd_progress_bar, ibd_progress * 100); result_string += strprintf("Difficulty: %s\n\n", result["difficulty"].getValStr()); result_string += strprintf( From d930c7f5b091687eb4208a5ffe8a2abe311d8054 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sat, 31 Jul 2021 21:56:59 +0200 Subject: [PATCH 05/35] p2p, rpc, test: address rate-limiting follow-ups --- src/net_processing.cpp | 19 ++++++++----------- src/rpc/net.cpp | 2 ++ test/functional/p2p_addr_relay.py | 22 +++++++++++----------- 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9da2fe5d6f..823ff9e92f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -161,7 +161,7 @@ static constexpr size_t MAX_ADDR_TO_SEND{1000}; static constexpr double MAX_ADDR_RATE_PER_SECOND{0.1}; /** The soft limit of the address processing token bucket (the regular MAX_ADDR_RATE_PER_SECOND * based increments won't go above this, but the MAX_ADDR_TO_SEND increment following GETADDR - * is exempt from this limit. */ + * is exempt from this limit). */ static constexpr size_t MAX_ADDR_PROCESSING_TOKEN_BUCKET{MAX_ADDR_TO_SEND}; // Internal stuff @@ -263,14 +263,14 @@ struct Peer { std::atomic_bool m_wants_addrv2{false}; /** Whether this peer has already sent us a getaddr message. */ bool m_getaddr_recvd{false}; - /** Number of addr messages that can be processed from this peer. Start at 1 to + /** Number of addresses that can be processed from this peer. Start at 1 to * permit self-announcement. */ double m_addr_token_bucket{1.0}; /** When m_addr_token_bucket was last updated */ std::chrono::microseconds m_addr_token_timestamp{GetTime()}; /** Total number of addresses that were dropped due to rate limiting. */ std::atomic m_addr_rate_limited{0}; - /** Total number of addresses that were processed (excludes rate limited ones). */ + /** Total number of addresses that were processed (excludes rate-limited ones). */ std::atomic m_addr_processed{0}; /** Set of txids to reconsider once their parent transactions have been accepted **/ @@ -2848,11 +2848,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, return; // Apply rate limiting. - if (rate_limited) { - if (peer->m_addr_token_bucket < 1.0) { + if (peer->m_addr_token_bucket < 1.0) { + if (rate_limited) { ++num_rate_limit; continue; } + } else { peer->m_addr_token_bucket -= 1.0; } // We only bother storing full nodes, though this may include @@ -2880,12 +2881,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } peer->m_addr_processed += num_proc; peer->m_addr_rate_limited += num_rate_limit; - LogPrint(BCLog::NET, "Received addr: %u addresses (%u processed, %u rate-limited) from peer=%d%s\n", - vAddr.size(), - num_proc, - num_rate_limit, - pfrom.GetId(), - fLogIPs ? ", peeraddr=" + pfrom.addr.ToString() : ""); + LogPrint(BCLog::NET, "Received addr: %u addresses (%u processed, %u rate-limited) from peer=%d\n", + vAddr.size(), num_proc, num_rate_limit, pfrom.GetId()); m_addrman.Add(vAddrOk, pfrom.addr, 2 * 60 * 60); if (vAddr.size() < 1000) peer->m_getaddr_sent = false; diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index abc9ec3ce3..4d066b8957 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -151,6 +151,8 @@ static RPCHelpMan getpeerinfo() {RPCResult::Type::NUM, "n", "The heights of blocks we're currently asking from this peer"}, }}, {RPCResult::Type::BOOL, "addr_relay_enabled", "Whether we participate in address relay with this peer"}, + {RPCResult::Type::NUM, "addr_processed", "The total number of addresses processed, excluding those dropped due to rate limiting"}, + {RPCResult::Type::NUM, "addr_rate_limited", "The total number of addresses dropped due to rate limiting"}, {RPCResult::Type::ARR, "permissions", "Any special permissions that have been granted to this peer", { {RPCResult::Type::STR, "permission_type", Join(NET_PERMISSIONS_DOC, ",\n") + ".\n"}, diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index 95743a1bf5..f2a682df8b 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -311,7 +311,7 @@ def blocksonly_mode_tests(self): self.nodes[0].disconnect_p2ps() - def send_addrs_and_test_rate_limiting(self, peer, no_relay, new_addrs, total_addrs): + def send_addrs_and_test_rate_limiting(self, peer, no_relay, *, new_addrs, total_addrs): """Send an addr message and check that the number of addresses processed and rate-limited is as expected""" peer.send_and_ping(self.setup_rand_addr_msg(new_addrs)) @@ -329,27 +329,26 @@ def send_addrs_and_test_rate_limiting(self, peer, no_relay, new_addrs, total_add assert_equal(addrs_rate_limited, max(0, total_addrs - peer.tokens)) def rate_limit_tests(self): - self.mocktime = int(time.time()) self.restart_node(0, []) self.nodes[0].setmocktime(self.mocktime) - for contype, no_relay in [("outbound-full-relay", False), ("block-relay-only", True), ("inbound", False)]: - self.log.info(f'Test rate limiting of addr processing for {contype} peers') - if contype == "inbound": + for conn_type, no_relay in [("outbound-full-relay", False), ("block-relay-only", True), ("inbound", False)]: + self.log.info(f'Test rate limiting of addr processing for {conn_type} peers') + if conn_type == "inbound": peer = self.nodes[0].add_p2p_connection(AddrReceiver()) else: - peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=0, connection_type=contype) + peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=0, connection_type=conn_type) # Send 600 addresses. For all but the block-relay-only peer this should result in addresses being processed. - self.send_addrs_and_test_rate_limiting(peer, no_relay, 600, 600) + self.send_addrs_and_test_rate_limiting(peer, no_relay, new_addrs=600, total_addrs=600) # Send 600 more addresses. For the outbound-full-relay peer (which we send a GETADDR, and thus will # process up to 1001 incoming addresses), this means more addresses will be processed. - self.send_addrs_and_test_rate_limiting(peer, no_relay, 600, 1200) + self.send_addrs_and_test_rate_limiting(peer, no_relay, new_addrs=600, total_addrs=1200) # Send 10 more. As we reached the processing limit for all nodes, no more addresses should be procesesd. - self.send_addrs_and_test_rate_limiting(peer, no_relay, 10, 1210) + self.send_addrs_and_test_rate_limiting(peer, no_relay, new_addrs=10, total_addrs=1210) # Advance the time by 100 seconds, permitting the processing of 10 more addresses. # Send 200 and verify that 10 are processed. @@ -357,7 +356,7 @@ def rate_limit_tests(self): self.nodes[0].setmocktime(self.mocktime) peer.increment_tokens(10) - self.send_addrs_and_test_rate_limiting(peer, no_relay, 200, 1410) + self.send_addrs_and_test_rate_limiting(peer, no_relay, new_addrs=200, total_addrs=1410) # Advance the time by 1000 seconds, permitting the processing of 100 more addresses. # Send 200 and verify that 100 are processed. @@ -365,9 +364,10 @@ def rate_limit_tests(self): self.nodes[0].setmocktime(self.mocktime) peer.increment_tokens(100) - self.send_addrs_and_test_rate_limiting(peer, no_relay, 200, 1610) + self.send_addrs_and_test_rate_limiting(peer, no_relay, new_addrs=200, total_addrs=1610) self.nodes[0].disconnect_p2ps() + if __name__ == '__main__': AddrTest().main() From fafe896a0b870d85250927bd5374caf73d379468 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 5 Aug 2021 12:06:23 +0200 Subject: [PATCH 06/35] test: Set regtest.BIP66Height = 102 to speed up tests --- doc/release-notes.md | 3 +++ src/chainparams.cpp | 2 +- test/functional/feature_dersig.py | 7 ++++--- test/functional/rpc_blockchain.py | 3 ++- test/functional/test_framework/blocktools.py | 1 + 5 files changed, 11 insertions(+), 5 deletions(-) diff --git a/doc/release-notes.md b/doc/release-notes.md index 61c65d5a3e..01ef3610c9 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -108,6 +108,9 @@ RPC Tests ----- +- For the `regtest` network the BIP 66 (DERSIG) activation height was changed + from 1251 to 102. (#22632) + Credits ======= diff --git a/src/chainparams.cpp b/src/chainparams.cpp index 0b3242b1aa..c3bbb147be 100644 --- a/src/chainparams.cpp +++ b/src/chainparams.cpp @@ -393,7 +393,7 @@ class CRegTestParams : public CChainParams { consensus.BIP34Height = 2; // BIP34 activated on regtest (Block at height 1 not enforced for testing purposes) consensus.BIP34Hash = uint256(); consensus.BIP65Height = 1351; // BIP65 activated on regtest (Used in functional tests) - consensus.BIP66Height = 1251; // BIP66 activated on regtest (Used in functional tests) + consensus.BIP66Height = 102; // BIP66 activated on regtest (Block at height 101 and earlier not enforced for testing purposes) consensus.CSVHeight = 432; // CSV activated on regtest (Used in rpc activation tests) consensus.SegwitHeight = 0; // SEGWIT is always activated on regtest unless overridden consensus.MinBIP9WarningHeight = 0; diff --git a/test/functional/feature_dersig.py b/test/functional/feature_dersig.py index eb027c554a..5dd6cb6cb2 100755 --- a/test/functional/feature_dersig.py +++ b/test/functional/feature_dersig.py @@ -4,10 +4,11 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test BIP66 (DER SIG). -Test that the DERSIG soft-fork activates at (regtest) height 1251. +Test the DERSIG soft-fork activation on regtest. """ from test_framework.blocktools import ( + DERSIG_HEIGHT, create_block, create_coinbase, ) @@ -23,8 +24,6 @@ MiniWalletMode, ) -DERSIG_HEIGHT = 1251 - # A canonical signature consists of: # <30> <02> <02> @@ -90,8 +89,10 @@ def run_test(self): block.rehash() block.solve() + assert_equal(self.nodes[0].getblockcount(), DERSIG_HEIGHT - 2) self.test_dersig_info(is_active=False) # Not active as of current tip and next block does not need to obey rules peer.send_and_ping(msg_block(block)) + assert_equal(self.nodes[0].getblockcount(), DERSIG_HEIGHT - 1) self.test_dersig_info(is_active=True) # Not active as of current tip, but next block must obey rules assert_equal(self.nodes[0].getbestblockhash(), block.hash) diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index 721e3f93a3..1e73dcf5cd 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -27,6 +27,7 @@ from test_framework.address import ADDRESS_BCRT1_P2WSH_OP_TRUE from test_framework.blocktools import ( + DERSIG_HEIGHT, create_block, create_coinbase, TIME_GENESIS_BLOCK, @@ -141,7 +142,7 @@ def _test_getblockchaininfo(self): assert_equal(res['softforks'], { 'bip34': {'type': 'buried', 'active': True, 'height': 2}, - 'bip66': {'type': 'buried', 'active': False, 'height': 1251}, + 'bip66': {'type': 'buried', 'active': True, 'height': DERSIG_HEIGHT}, 'bip65': {'type': 'buried', 'active': False, 'height': 1351}, 'csv': {'type': 'buried', 'active': False, 'height': 432}, 'segwit': {'type': 'buried', 'active': True, 'height': 0}, diff --git a/test/functional/test_framework/blocktools.py b/test/functional/test_framework/blocktools.py index a88a119e6f..0c7aaee306 100644 --- a/test/functional/test_framework/blocktools.py +++ b/test/functional/test_framework/blocktools.py @@ -55,6 +55,7 @@ COINBASE_MATURITY = 100 # Soft-fork activation heights +DERSIG_HEIGHT = 102 # BIP 66 CLTV_HEIGHT = 1351 CSV_ACTIVATION_HEIGHT = 432 From f95bbf58aaf72aab8a9c5827b1f162f3b8ac38f4 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 15 Jul 2021 07:12:29 +0100 Subject: [PATCH 07/35] misc package validation doc improvements --- src/rpc/rawtransaction.cpp | 2 +- src/validation.h | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index c617b0389c..00e77d89e5 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -903,7 +903,7 @@ static RPCHelpMan testmempoolaccept() RPCResult{ RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n" "Returns results for each transaction in the same order they were passed in.\n" - "It is possible for transactions to not be fully validated ('allowed' unset) if another transaction failed.\n", + "Transactions that cannot be fully validated due to failures in other transactions will not contain an 'allowed' result.\n", { {RPCResult::Type::OBJ, "", "", { diff --git a/src/validation.h b/src/validation.h index 9d8d7c06a9..b80fa9d328 100644 --- a/src/validation.h +++ b/src/validation.h @@ -199,7 +199,8 @@ struct PackageMempoolAcceptResult /** * Map from wtxid to finished MempoolAcceptResults. The client is responsible * for keeping track of the transaction objects themselves. If a result is not - * present, it means validation was unfinished for that transaction. + * present, it means validation was unfinished for that transaction. If there + * was a package-wide error (see result in m_state), m_tx_results will be empty. */ std::map m_tx_results; @@ -227,7 +228,8 @@ MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPoo * @param[in] txns Group of transactions which may be independent or contain * parent-child dependencies. The transactions must not conflict * with each other, i.e., must not spend the same inputs. If any -* dependencies exist, parents must appear before children. +* dependencies exist, parents must appear anywhere in the list +* before their children. * @returns a PackageMempoolAcceptResult which includes a MempoolAcceptResult for each transaction. * If a transaction fails, validation will exit early and some results may be missing. */ From 97dd1c729d2bbedf9527b914c0cc8267b8a7c21b Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 15 Jul 2021 06:54:36 +0100 Subject: [PATCH 08/35] MOVEONLY: add helper function for calculating ancestors and checking limits --- src/txmempool.cpp | 72 ++++++++++++++++++++++++++++++----------------- src/txmempool.h | 16 +++++++++++ 2 files changed, 62 insertions(+), 26 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index c5a4bbf1b0..53de2d2618 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -150,33 +150,15 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashes UpdateForDescendants(it, mapMemPoolDescendantsToUpdate, setAlreadyIncluded); } } - -bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntries &setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string &errString, bool fSearchForParents /* = true */) const +bool CTxMemPool::CalculateAncestorsAndCheckLimits(const CTxMemPoolEntry& entry, + setEntries& setAncestors, + CTxMemPoolEntry::Parents &staged_ancestors, + uint64_t limitAncestorCount, + uint64_t limitAncestorSize, + uint64_t limitDescendantCount, + uint64_t limitDescendantSize, + std::string &errString) const { - CTxMemPoolEntry::Parents staged_ancestors; - const CTransaction &tx = entry.GetTx(); - - if (fSearchForParents) { - // Get parents of this transaction that are in the mempool - // GetMemPoolParents() is only valid for entries in the mempool, so we - // iterate mapTx to find parents. - for (unsigned int i = 0; i < tx.vin.size(); i++) { - std::optional piter = GetIter(tx.vin[i].prevout.hash); - if (piter) { - staged_ancestors.insert(**piter); - if (staged_ancestors.size() + 1 > limitAncestorCount) { - errString = strprintf("too many unconfirmed parents [limit: %u]", limitAncestorCount); - return false; - } - } - } - } else { - // If we're not searching for parents, we require this to be an - // entry in the mempool already. - txiter it = mapTx.iterator_to(entry); - staged_ancestors = it->GetMemPoolParentsConst(); - } - size_t totalSizeWithAncestors = entry.GetTxSize(); while (!staged_ancestors.empty()) { @@ -216,6 +198,44 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntr return true; } +bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, + setEntries &setAncestors, + uint64_t limitAncestorCount, + uint64_t limitAncestorSize, + uint64_t limitDescendantCount, + uint64_t limitDescendantSize, + std::string &errString, + bool fSearchForParents /* = true */) const +{ + CTxMemPoolEntry::Parents staged_ancestors; + const CTransaction &tx = entry.GetTx(); + + if (fSearchForParents) { + // Get parents of this transaction that are in the mempool + // GetMemPoolParents() is only valid for entries in the mempool, so we + // iterate mapTx to find parents. + for (unsigned int i = 0; i < tx.vin.size(); i++) { + std::optional piter = GetIter(tx.vin[i].prevout.hash); + if (piter) { + staged_ancestors.insert(**piter); + if (staged_ancestors.size() + 1 > limitAncestorCount) { + errString = strprintf("too many unconfirmed parents [limit: %u]", limitAncestorCount); + return false; + } + } + } + } else { + // If we're not searching for parents, we require this to already be an + // entry in the mempool and use the entry's cached parents. + txiter it = mapTx.iterator_to(entry); + staged_ancestors = it->GetMemPoolParentsConst(); + } + + return CalculateAncestorsAndCheckLimits(entry, setAncestors, staged_ancestors, + limitAncestorCount, limitAncestorSize, + limitDescendantCount, limitDescendantSize, errString); +} + void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors) { CTxMemPoolEntry::Parents parents = it->GetMemPoolParents(); diff --git a/src/txmempool.h b/src/txmempool.h index ae4b16d377..76ca83c25c 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -585,6 +585,22 @@ class CTxMemPool */ std::set m_unbroadcast_txids GUARDED_BY(cs); + + /** + * Helper function to populate setAncestors with all the ancestors of entry and apply ancestor + * and descendant limits. + * param@[out] setAncestors Will be populated with all mempool ancestors of entry. + * param@[in] staged_ancestors Should contain mempool parents of entry. + */ + bool CalculateAncestorsAndCheckLimits(const CTxMemPoolEntry& entry, + setEntries& setAncestors, + CTxMemPoolEntry::Parents &staged_ancestors, + uint64_t limitAncestorCount, + uint64_t limitAncestorSize, + uint64_t limitDescendantCount, + uint64_t limitDescendantSize, + std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs); + public: indirectmap mapNextTx GUARDED_BY(cs); std::map mapDeltas GUARDED_BY(cs); From f551841d3ec080a2d7a7988c7b35088dff6c5830 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 15 Jul 2021 07:09:22 +0100 Subject: [PATCH 09/35] [refactor] pass size/count instead of entry to CalculateAncestorsAndCheckLimits This does not change existing behavior. The ancestor/descendant limits are inclusive of the entries themselves, but CalculateAncestorsAndCheckLimits() does not need access to them. --- src/txmempool.cpp | 17 ++++++++++------- src/txmempool.h | 13 ++++++++----- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 53de2d2618..4a992bf2a4 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -150,16 +150,18 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashes UpdateForDescendants(it, mapMemPoolDescendantsToUpdate, setAlreadyIncluded); } } -bool CTxMemPool::CalculateAncestorsAndCheckLimits(const CTxMemPoolEntry& entry, + +bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size, + size_t entry_count, setEntries& setAncestors, - CTxMemPoolEntry::Parents &staged_ancestors, + CTxMemPoolEntry::Parents& staged_ancestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string &errString) const { - size_t totalSizeWithAncestors = entry.GetTxSize(); + size_t totalSizeWithAncestors = entry_size; while (!staged_ancestors.empty()) { const CTxMemPoolEntry& stage = staged_ancestors.begin()->get(); @@ -169,10 +171,10 @@ bool CTxMemPool::CalculateAncestorsAndCheckLimits(const CTxMemPoolEntry& entry, staged_ancestors.erase(stage); totalSizeWithAncestors += stageit->GetTxSize(); - if (stageit->GetSizeWithDescendants() + entry.GetTxSize() > limitDescendantSize) { + if (stageit->GetSizeWithDescendants() + entry_size > limitDescendantSize) { errString = strprintf("exceeds descendant size limit for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limitDescendantSize); return false; - } else if (stageit->GetCountWithDescendants() + 1 > limitDescendantCount) { + } else if (stageit->GetCountWithDescendants() + entry_count > limitDescendantCount) { errString = strprintf("too many descendants for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limitDescendantCount); return false; } else if (totalSizeWithAncestors > limitAncestorSize) { @@ -188,7 +190,7 @@ bool CTxMemPool::CalculateAncestorsAndCheckLimits(const CTxMemPoolEntry& entry, if (setAncestors.count(parent_it) == 0) { staged_ancestors.insert(parent); } - if (staged_ancestors.size() + setAncestors.size() + 1 > limitAncestorCount) { + if (staged_ancestors.size() + setAncestors.size() + entry_count > limitAncestorCount) { errString = strprintf("too many unconfirmed ancestors [limit: %u]", limitAncestorCount); return false; } @@ -231,7 +233,8 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, staged_ancestors = it->GetMemPoolParentsConst(); } - return CalculateAncestorsAndCheckLimits(entry, setAncestors, staged_ancestors, + return CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /* entry_count */ 1, + setAncestors, staged_ancestors, limitAncestorCount, limitAncestorSize, limitDescendantCount, limitDescendantSize, errString); } diff --git a/src/txmempool.h b/src/txmempool.h index 76ca83c25c..71345ffb5d 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -587,12 +587,15 @@ class CTxMemPool /** - * Helper function to populate setAncestors with all the ancestors of entry and apply ancestor - * and descendant limits. - * param@[out] setAncestors Will be populated with all mempool ancestors of entry. - * param@[in] staged_ancestors Should contain mempool parents of entry. + * Helper function to calculate all in-mempool ancestors of staged_ancestors and apply ancestor + * and descendant limits (including staged_ancestors thsemselves, entry_size and entry_count). + * param@[in] entry_size Virtual size to include in the limits. + * param@[in] entry_count How many entries to include in the limits. + * param@[in] staged_ancestors Should contain entries in the mempool. + * param@[out] setAncestors Will be populated with all mempool ancestors. */ - bool CalculateAncestorsAndCheckLimits(const CTxMemPoolEntry& entry, + bool CalculateAncestorsAndCheckLimits(size_t entry_size, + size_t entry_count, setEntries& setAncestors, CTxMemPoolEntry::Parents &staged_ancestors, uint64_t limitAncestorCount, From c6e016aa139c8363e9b38bbc1ba0dca55700b8a7 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 15 Jul 2021 07:18:18 +0100 Subject: [PATCH 10/35] [mempool] check ancestor/descendant limits for packages When calculating ancestor/descendant counts for transactions in the package, as a heuristic, count every transaction in the package as an ancestor and descendant of every other transaction in the package. This may overestimate, but will not underestimate, the ancestor/descendant counts. This shortcut still produces an accurate count for packages of 1 parent + 1 child. --- src/txmempool.cpp | 35 +++++++++++++++++++++++++++++++++++ src/txmempool.h | 23 +++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 4a992bf2a4..d5a888ac67 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -200,6 +200,41 @@ bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size, return true; } +bool CTxMemPool::CheckPackageLimits(const Package& package, + uint64_t limitAncestorCount, + uint64_t limitAncestorSize, + uint64_t limitDescendantCount, + uint64_t limitDescendantSize, + std::string &errString) const +{ + CTxMemPoolEntry::Parents staged_ancestors; + size_t total_size = 0; + for (const auto& tx : package) { + total_size += GetVirtualTransactionSize(*tx); + for (const auto& input : tx->vin) { + std::optional piter = GetIter(input.prevout.hash); + if (piter) { + staged_ancestors.insert(**piter); + if (staged_ancestors.size() + package.size() > limitAncestorCount) { + errString = strprintf("too many unconfirmed parents [limit: %u]", limitAncestorCount); + return false; + } + } + } + } + // When multiple transactions are passed in, the ancestors and descendants of all transactions + // considered together must be within limits even if they are not interdependent. This may be + // stricter than the limits for each individual transaction. + setEntries setAncestors; + const auto ret = CalculateAncestorsAndCheckLimits(total_size, package.size(), + setAncestors, staged_ancestors, + limitAncestorCount, limitAncestorSize, + limitDescendantCount, limitDescendantSize, errString); + // It's possible to overestimate the ancestor/descendant totals. + if (!ret) errString.insert(0, "possibly "); + return ret; +} + bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntries &setAncestors, uint64_t limitAncestorCount, diff --git a/src/txmempool.h b/src/txmempool.h index 71345ffb5d..0a84a6e6b1 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -700,6 +701,28 @@ class CTxMemPool */ bool CalculateMemPoolAncestors(const CTxMemPoolEntry& entry, setEntries& setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string& errString, bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs); + /** Calculate all in-mempool ancestors of a set of transactions not already in the mempool and + * check ancestor and descendant limits. Heuristics are used to estimate the ancestor and + * descendant count of all entries if the package were to be added to the mempool. The limits + * are applied to the union of all package transactions. For example, if the package has 3 + * transactions and limitAncestorCount = 25, the union of all 3 sets of ancestors (including the + * transactions themselves) must be <= 22. + * @param[in] package Transaction package being evaluated for acceptance + * to mempool. The transactions need not be direct + * ancestors/descendants of each other. + * @param[in] limitAncestorCount Max number of txns including ancestors. + * @param[in] limitAncestorSize Max virtual size including ancestors. + * @param[in] limitDescendantCount Max number of txns including descendants. + * @param[in] limitDescendantSize Max virtual size including descendants. + * @param[out] errString Populated with error reason if a limit is hit. + */ + bool CheckPackageLimits(const Package& package, + uint64_t limitAncestorCount, + uint64_t limitAncestorSize, + uint64_t limitDescendantCount, + uint64_t limitDescendantSize, + std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs); + /** Populate setDescendants with all in-mempool descendants of hash. * Assumes that setDescendants includes all in-mempool descendants of anything * already in it. */ From ee458d84fc187d69f002ebead6fccc4f4f9c0744 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 2 Aug 2021 13:42:42 +0200 Subject: [PATCH 11/35] Add missing const to CAddrMan::Check_() Also: Always compile the function signature to avoid similar issues in the future. --- src/addrman.cpp | 21 ++++++++++++++------- src/addrman.h | 4 ---- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 96139182d3..c5c6dfbb86 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -431,9 +431,9 @@ CAddrInfo CAddrMan::Select_(bool newOnly) const } } -#ifdef DEBUG_ADDRMAN -int CAddrMan::Check_() +int CAddrMan::Check_() const { +#ifdef DEBUG_ADDRMAN AssertLockHeld(cs); std::unordered_set setTried; @@ -458,8 +458,10 @@ int CAddrMan::Check_() return -4; mapNew[n] = info.nRefCount; } - if (mapAddr[info] != n) + const auto it{mapAddr.find(info)}; + if (it == mapAddr.end() || it->second != n) { return -5; + } if (info.nRandomPos < 0 || (size_t)info.nRandomPos >= vRandom.size() || vRandom[info.nRandomPos] != n) return -14; if (info.nLastTry < 0) @@ -478,10 +480,13 @@ int CAddrMan::Check_() if (vvTried[n][i] != -1) { if (!setTried.count(vvTried[n][i])) return -11; - if (mapInfo[vvTried[n][i]].GetTriedBucket(nKey, m_asmap) != n) + const auto it{mapInfo.find(vvTried[n][i])}; + if (it == mapInfo.end() || it->second.GetTriedBucket(nKey, m_asmap) != n) { return -17; - if (mapInfo[vvTried[n][i]].GetBucketPosition(nKey, false, n) != i) + } + if (it->second.GetBucketPosition(nKey, false, n) != i) { return -18; + } setTried.erase(vvTried[n][i]); } } @@ -492,8 +497,10 @@ int CAddrMan::Check_() if (vvNew[n][i] != -1) { if (!mapNew.count(vvNew[n][i])) return -12; - if (mapInfo[vvNew[n][i]].GetBucketPosition(nKey, true, n) != i) + const auto it{mapInfo.find(vvNew[n][i])}; + if (it == mapInfo.end() || it->second.GetBucketPosition(nKey, true, n) != i) { return -19; + } if (--mapNew[vvNew[n][i]] == 0) mapNew.erase(vvNew[n][i]); } @@ -507,9 +514,9 @@ int CAddrMan::Check_() if (nKey.IsNull()) return -16; +#endif // DEBUG_ADDRMAN return 0; } -#endif void CAddrMan::GetAddr_(std::vector& vAddr, size_t max_addresses, size_t max_pct, std::optional network) const { diff --git a/src/addrman.h b/src/addrman.h index 1dd1932421..16be374f7b 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -738,19 +738,15 @@ class CAddrMan void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs) { -#ifdef DEBUG_ADDRMAN AssertLockHeld(cs); const int err = Check_(); if (err) { LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err); } -#endif } -#ifdef DEBUG_ADDRMAN //! Perform consistency check. Returns an error code or zero. int Check_() const EXCLUSIVE_LOCKS_REQUIRED(cs); -#endif /** * Return all or many randomly selected addresses, optionally by network. From fa9710f62c29c7f8d71c9f281001c9b5e70946bf Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 23 Jul 2021 10:48:34 +0100 Subject: [PATCH 12/35] [addrman] Add deterministic argument to CAddrMan ctor Removes the need for tests to update nKey and insecure_rand after constructing a CAddrMan. --- src/addrman.h | 10 ++++++---- src/bench/addrman.cpp | 16 +++++++++------- src/init.cpp | 2 +- src/test/addrman_tests.cpp | 15 ++------------- src/test/fuzz/addrman.cpp | 3 ++- src/test/fuzz/connman.cpp | 2 +- src/test/fuzz/data_stream.cpp | 2 +- src/test/fuzz/deserialize.cpp | 2 +- src/test/net_tests.cpp | 20 +++++++------------- src/test/util/setup_common.cpp | 2 +- 10 files changed, 31 insertions(+), 43 deletions(-) diff --git a/src/addrman.h b/src/addrman.h index 16be374f7b..2503f53a35 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -493,9 +493,11 @@ class CAddrMan mapAddr.clear(); } - CAddrMan() + explicit CAddrMan(bool deterministic) + : insecure_rand{deterministic} { Clear(); + if (deterministic) nKey.SetNull(); } ~CAddrMan() @@ -637,13 +639,13 @@ class CAddrMan //! secret key to randomize bucket select with uint256 nKey; - //! Source of random numbers for randomization in inner loops - mutable FastRandomContext insecure_rand GUARDED_BY(cs); - //! A mutex to protect the inner data structures. mutable Mutex cs; private: + //! Source of random numbers for randomization in inner loops + mutable FastRandomContext insecure_rand GUARDED_BY(cs); + //! Serialization versions. enum Format : uint8_t { V0_HISTORICAL = 0, //!< historic format, before commit e6b343d88 diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index b7bd8a3261..e1175e44ec 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -72,7 +72,7 @@ static void AddrManAdd(benchmark::Bench& bench) { CreateAddresses(); - CAddrMan addrman; + CAddrMan addrman(/* deterministic */ false); bench.run([&] { AddAddressesToAddrMan(addrman); @@ -82,7 +82,7 @@ static void AddrManAdd(benchmark::Bench& bench) static void AddrManSelect(benchmark::Bench& bench) { - CAddrMan addrman; + CAddrMan addrman(/* deterministic */ false); FillAddrMan(addrman); @@ -94,7 +94,7 @@ static void AddrManSelect(benchmark::Bench& bench) static void AddrManGetAddr(benchmark::Bench& bench) { - CAddrMan addrman; + CAddrMan addrman(/* deterministic */ false); FillAddrMan(addrman); @@ -112,10 +112,12 @@ static void AddrManGood(benchmark::Bench& bench) * we want to do the same amount of work in every loop iteration. */ bench.epochs(5).epochIterations(1); + const size_t addrman_count{bench.epochs() * bench.epochIterations()}; - std::vector addrmans(bench.epochs() * bench.epochIterations()); - for (auto& addrman : addrmans) { - FillAddrMan(addrman); + std::vector> addrmans(addrman_count); + for (size_t i{0}; i < addrman_count; ++i) { + addrmans[i] = std::make_unique(/* deterministic */ false); + FillAddrMan(*addrmans[i]); } auto markSomeAsGood = [](CAddrMan& addrman) { @@ -130,7 +132,7 @@ static void AddrManGood(benchmark::Bench& bench) uint64_t i = 0; bench.run([&] { - markSomeAsGood(addrmans.at(i)); + markSomeAsGood(*addrmans.at(i)); ++i; }); } diff --git a/src/init.cpp b/src/init.cpp index 1b406bed28..2250834111 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1164,7 +1164,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)}; assert(!node.addrman); - node.addrman = std::make_unique(); + node.addrman = std::make_unique(/* deterministic */ false); assert(!node.banman); node.banman = std::make_unique(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME)); assert(!node.connman); diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 79c7102c4f..0ab6cd0a2b 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -21,24 +21,13 @@ class CAddrManTest : public CAddrMan bool deterministic; public: explicit CAddrManTest(bool makeDeterministic = true, - std::vector asmap = std::vector()) + std::vector asmap = std::vector()) + : CAddrMan(makeDeterministic) { - if (makeDeterministic) { - // Set addrman addr placement to be deterministic. - MakeDeterministic(); - } deterministic = makeDeterministic; m_asmap = asmap; } - //! Ensure that bucket placement is always the same for testing purposes. - void MakeDeterministic() - { - LOCK(cs); - nKey.SetNull(); - insecure_rand = FastRandomContext(true); - } - CAddrInfo* Find(const CNetAddr& addr, int* pnId = nullptr) { LOCK(cs); diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 4c29a8ee53..b5e946c528 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -29,7 +29,8 @@ class CAddrManDeterministic : public CAddrMan FuzzedDataProvider& m_fuzzed_data_provider; explicit CAddrManDeterministic(FuzzedDataProvider& fuzzed_data_provider) - : m_fuzzed_data_provider(fuzzed_data_provider) + : CAddrMan(/* deterministic */ true) + , m_fuzzed_data_provider(fuzzed_data_provider) { WITH_LOCK(cs, insecure_rand = FastRandomContext{ConsumeUInt256(fuzzed_data_provider)}); if (fuzzed_data_provider.ConsumeBool()) { diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index bbec5943af..a32cb54465 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -25,7 +25,7 @@ FUZZ_TARGET_INIT(connman, initialize_connman) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; SetMockTime(ConsumeTime(fuzzed_data_provider)); - CAddrMan addrman; + CAddrMan addrman(/* deterministic */ false); CConnman connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addrman, fuzzed_data_provider.ConsumeBool()}; CNetAddr random_netaddr; CNode random_node = ConsumeNode(fuzzed_data_provider); diff --git a/src/test/fuzz/data_stream.cpp b/src/test/fuzz/data_stream.cpp index 473caec6ff..63bc4b2afe 100644 --- a/src/test/fuzz/data_stream.cpp +++ b/src/test/fuzz/data_stream.cpp @@ -21,6 +21,6 @@ FUZZ_TARGET_INIT(data_stream_addr_man, initialize_data_stream_addr_man) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider); - CAddrMan addr_man; + CAddrMan addr_man(/* deterministic */ false); CAddrDB::Read(addr_man, data_stream); } diff --git a/src/test/fuzz/deserialize.cpp b/src/test/fuzz/deserialize.cpp index d5b56cb7cd..6ab3389123 100644 --- a/src/test/fuzz/deserialize.cpp +++ b/src/test/fuzz/deserialize.cpp @@ -188,7 +188,7 @@ FUZZ_TARGET_DESERIALIZE(blockmerkleroot, { BlockMerkleRoot(block, &mutated); }) FUZZ_TARGET_DESERIALIZE(addrman_deserialize, { - CAddrMan am; + CAddrMan am(/* deterministic */ false); DeserializeFromFuzzingInput(buffer, am); }) FUZZ_TARGET_DESERIALIZE(blockheader_deserialize, { diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index acbbf357d2..ff9e9b7a07 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -34,13 +34,9 @@ class CAddrManSerializationMock : public CAddrMan public: virtual void Serialize(CDataStream& s) const = 0; - //! Ensure that bucket placement is always the same for testing purposes. - void MakeDeterministic() - { - LOCK(cs); - nKey.SetNull(); - insecure_rand = FastRandomContext(true); - } + CAddrManSerializationMock() + : CAddrMan(/* deterministic */ true) + {} }; class CAddrManUncorrupted : public CAddrManSerializationMock @@ -105,7 +101,6 @@ BOOST_AUTO_TEST_CASE(cnode_listen_port) BOOST_AUTO_TEST_CASE(caddrdb_read) { CAddrManUncorrupted addrmanUncorrupted; - addrmanUncorrupted.MakeDeterministic(); CService addr1, addr2, addr3; BOOST_CHECK(Lookup("250.7.1.1", addr1, 8333, false)); @@ -124,7 +119,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read) // Test that the de-serialization does not throw an exception. CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted); bool exceptionThrown = false; - CAddrMan addrman1; + CAddrMan addrman1(/* deterministic */ false); BOOST_CHECK(addrman1.size() == 0); try { @@ -141,7 +136,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read) // Test that CAddrDB::Read creates an addrman with the correct number of addrs. CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted); - CAddrMan addrman2; + CAddrMan addrman2(/* deterministic */ false); BOOST_CHECK(addrman2.size() == 0); BOOST_CHECK(CAddrDB::Read(addrman2, ssPeers2)); BOOST_CHECK(addrman2.size() == 3); @@ -151,12 +146,11 @@ BOOST_AUTO_TEST_CASE(caddrdb_read) BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted) { CAddrManCorrupted addrmanCorrupted; - addrmanCorrupted.MakeDeterministic(); // Test that the de-serialization of corrupted addrman throws an exception. CDataStream ssPeers1 = AddrmanToStream(addrmanCorrupted); bool exceptionThrown = false; - CAddrMan addrman1; + CAddrMan addrman1(/* deterministic */ false); BOOST_CHECK(addrman1.size() == 0); try { unsigned char pchMsgTmp[4]; @@ -172,7 +166,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted) // Test that CAddrDB::Read leaves addrman in a clean state if de-serialization fails. CDataStream ssPeers2 = AddrmanToStream(addrmanCorrupted); - CAddrMan addrman2; + CAddrMan addrman2(/* deterministic */ false); BOOST_CHECK(addrman2.size() == 0); BOOST_CHECK(!CAddrDB::Read(addrman2, ssPeers2)); BOOST_CHECK(addrman2.size() == 0); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2d044af184..b1567d924b 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -193,7 +193,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector(); + m_node.addrman = std::make_unique(/* deterministic */ false); m_node.banman = std::make_unique(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); m_node.connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests. m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman, From 10aac241455a3270462d49b53732477ed97623e7 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 19 Jul 2021 10:45:32 +0100 Subject: [PATCH 13/35] [tests] Make deterministic addrman use nKey = 1 addrman_tests fail when consistency checks are enabled, since the tests set the deterministic test addrman's nKey value to zero, which is an invalid value. Change this so that deterministic addrman's nKey value is set to 1. This requires updating a few tests that are using magic values derived from nKey being set to 0. --- src/addrman.h | 2 +- src/test/addrman_tests.cpp | 96 ++++++++++++++++++++------------------ 2 files changed, 52 insertions(+), 46 deletions(-) diff --git a/src/addrman.h b/src/addrman.h index 2503f53a35..2c6ffbe523 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -497,7 +497,7 @@ class CAddrMan : insecure_rand{deterministic} { Clear(); - if (deterministic) nKey.SetNull(); + if (deterministic) nKey = uint256{1}; } ~CAddrMan() diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 0ab6cd0a2b..b7da1c8896 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -78,7 +78,7 @@ class CAddrManTest : public CAddrMan CAddrMan::Clear(); if (deterministic) { LOCK(cs); - nKey.SetNull(); + nKey = uint256{1}; insecure_rand = FastRandomContext(true); } } @@ -256,24 +256,27 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions) CNetAddr source = ResolveIP("252.2.2.2"); - BOOST_CHECK_EQUAL(addrman.size(), 0U); + uint32_t num_addrs{0}; + + BOOST_CHECK_EQUAL(addrman.size(), num_addrs); - for (unsigned int i = 1; i < 18; i++) { - CService addr = ResolveService("250.1.1." + ToString(i)); + while (num_addrs < 22) { // Magic number! 250.1.1.1 - 250.1.1.22 do not collide with deterministic key = 1 + CService addr = ResolveService("250.1.1." + ToString(++num_addrs)); BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source)); //Test: No collision in new table yet. - BOOST_CHECK_EQUAL(addrman.size(), i); + BOOST_CHECK_EQUAL(addrman.size(), num_addrs); } //Test: new table collision! - CService addr1 = ResolveService("250.1.1.18"); + CService addr1 = ResolveService("250.1.1." + ToString(++num_addrs)); + uint32_t collisions{1}; BOOST_CHECK(addrman.Add(CAddress(addr1, NODE_NONE), source)); - BOOST_CHECK_EQUAL(addrman.size(), 17U); + BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions); - CService addr2 = ResolveService("250.1.1.19"); + CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs)); BOOST_CHECK(addrman.Add(CAddress(addr2, NODE_NONE), source)); - BOOST_CHECK_EQUAL(addrman.size(), 18U); + BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions); } BOOST_AUTO_TEST_CASE(addrman_tried_collisions) @@ -282,25 +285,28 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions) CNetAddr source = ResolveIP("252.2.2.2"); - BOOST_CHECK_EQUAL(addrman.size(), 0U); + uint32_t num_addrs{0}; + + BOOST_CHECK_EQUAL(addrman.size(), num_addrs); - for (unsigned int i = 1; i < 80; i++) { - CService addr = ResolveService("250.1.1." + ToString(i)); + while (num_addrs < 64) { // Magic number! 250.1.1.1 - 250.1.1.64 do not collide with deterministic key = 1 + CService addr = ResolveService("250.1.1." + ToString(++num_addrs)); BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source)); addrman.Good(CAddress(addr, NODE_NONE)); //Test: No collision in tried table yet. - BOOST_CHECK_EQUAL(addrman.size(), i); + BOOST_CHECK_EQUAL(addrman.size(), num_addrs); } //Test: tried table collision! - CService addr1 = ResolveService("250.1.1.80"); + CService addr1 = ResolveService("250.1.1." + ToString(++num_addrs)); + uint32_t collisions{1}; BOOST_CHECK(addrman.Add(CAddress(addr1, NODE_NONE), source)); - BOOST_CHECK_EQUAL(addrman.size(), 79U); + BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions); - CService addr2 = ResolveService("250.1.1.81"); + CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs)); BOOST_CHECK(addrman.Add(CAddress(addr2, NODE_NONE), source)); - BOOST_CHECK_EQUAL(addrman.size(), 80U); + BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions); } BOOST_AUTO_TEST_CASE(addrman_find) @@ -850,9 +856,9 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) { CAddrManTest addrman; - // Add twenty two addresses. + // Add 35 addresses. CNetAddr source = ResolveIP("252.2.2.2"); - for (unsigned int i = 1; i < 23; i++) { + for (unsigned int i = 1; i < 36; i++) { CService addr = ResolveService("250.1.1."+ToString(i)); BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source)); addrman.Good(addr); @@ -862,20 +868,20 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); } - // Collision between 23 and 19. - CService addr23 = ResolveService("250.1.1.23"); - BOOST_CHECK(addrman.Add(CAddress(addr23, NODE_NONE), source)); - addrman.Good(addr23); + // Collision between 36 and 19. + CService addr36 = ResolveService("250.1.1.36"); + BOOST_CHECK(addrman.Add(CAddress(addr36, NODE_NONE), source)); + addrman.Good(addr36); - BOOST_CHECK(addrman.size() == 23); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "250.1.1.19:0"); + BOOST_CHECK(addrman.size() == 36); + BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.19:0"); - // 23 should be discarded and 19 not evicted. + // 36 should be discarded and 19 not evicted. addrman.ResolveCollisions(); BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); // Lets create two collisions. - for (unsigned int i = 24; i < 33; i++) { + for (unsigned int i = 37; i < 59; i++) { CService addr = ResolveService("250.1.1."+ToString(i)); BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source)); addrman.Good(addr); @@ -885,17 +891,17 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) } // Cause a collision. - CService addr33 = ResolveService("250.1.1.33"); - BOOST_CHECK(addrman.Add(CAddress(addr33, NODE_NONE), source)); - addrman.Good(addr33); - BOOST_CHECK(addrman.size() == 33); + CService addr59 = ResolveService("250.1.1.59"); + BOOST_CHECK(addrman.Add(CAddress(addr59, NODE_NONE), source)); + addrman.Good(addr59); + BOOST_CHECK(addrman.size() == 59); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "250.1.1.27:0"); + BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.10:0"); // Cause a second collision. - BOOST_CHECK(!addrman.Add(CAddress(addr23, NODE_NONE), source)); - addrman.Good(addr23); - BOOST_CHECK(addrman.size() == 33); + BOOST_CHECK(!addrman.Add(CAddress(addr36, NODE_NONE), source)); + addrman.Good(addr36); + BOOST_CHECK(addrman.size() == 59); BOOST_CHECK(addrman.SelectTriedCollision().ToString() != "[::]:0"); addrman.ResolveCollisions(); @@ -911,9 +917,9 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) // Empty addrman should return blank addrman info. BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); - // Add twenty two addresses. + // Add 35 addresses CNetAddr source = ResolveIP("252.2.2.2"); - for (unsigned int i = 1; i < 23; i++) { + for (unsigned int i = 1; i < 36; i++) { CService addr = ResolveService("250.1.1."+ToString(i)); BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source)); addrman.Good(addr); @@ -923,34 +929,34 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); } - // Collision between 23 and 19. - CService addr = ResolveService("250.1.1.23"); + // Collision between 36 and 19. + CService addr = ResolveService("250.1.1.36"); BOOST_CHECK(addrman.Add(CAddress(addr, NODE_NONE), source)); addrman.Good(addr); - BOOST_CHECK(addrman.size() == 23); + BOOST_CHECK_EQUAL(addrman.size(), 36); CAddrInfo info = addrman.SelectTriedCollision(); - BOOST_CHECK(info.ToString() == "250.1.1.19:0"); + BOOST_CHECK_EQUAL(info.ToString(), "250.1.1.19:0"); // Ensure test of address fails, so that it is evicted. addrman.SimConnFail(info); - // Should swap 23 for 19. + // Should swap 36 for 19. addrman.ResolveCollisions(); BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); - // If 23 was swapped for 19, then this should cause no collisions. + // If 36 was swapped for 19, then this should cause no collisions. BOOST_CHECK(!addrman.Add(CAddress(addr, NODE_NONE), source)); addrman.Good(addr); BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); - // If we insert 19 is should collide with 23. + // If we insert 19 it should collide with 36 CService addr19 = ResolveService("250.1.1.19"); BOOST_CHECK(!addrman.Add(CAddress(addr19, NODE_NONE), source)); addrman.Good(addr19); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "250.1.1.23:0"); + BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.36:0"); addrman.ResolveCollisions(); BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); From 90b3e482e911fde73133a157c3b354471682275a Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Thu, 5 Aug 2021 16:57:45 -0400 Subject: [PATCH 14/35] release: Release with separate SHA256SUMS and sig files This allows us to remove the rfc4880 EOL hacks and release with a SHA256SUMS.asc file that's a combination of all signer signatures. --- contrib/guix/guix-attest | 16 ---------------- doc/release-process.md | 23 ++++++----------------- 2 files changed, 6 insertions(+), 33 deletions(-) diff --git a/contrib/guix/guix-attest b/contrib/guix/guix-attest index dcf709b542..1503c330b2 100755 --- a/contrib/guix/guix-attest +++ b/contrib/guix/guix-attest @@ -159,20 +159,6 @@ Hint: You may wish to remove the existing attestations and their signatures by EOF } -# Given a document with unix line endings (just ) in stdin, make all lines -# end in and make sure there's no trailing at the end of the file. -# -# This is necessary as cleartext signatures are calculated on text after their -# line endings are canonicalized. -# -# For more information: -# 1. https://security.stackexchange.com/a/104261 -# 2. https://datatracker.ietf.org/doc/html/rfc4880#section-7.1 -# -rfc4880_normalize_document() { - sed 's/$/\r/' | head -c -2 -} - echo "Attesting to build outputs for version: '${VERSION}'" echo "" @@ -188,7 +174,6 @@ mkdir -p "$outsigdir" cat "${noncodesigned_fragments[@]}" \ | sort -u \ | sort -k2 \ - | rfc4880_normalize_document \ > "$temp_noncodesigned" if [ -e noncodesigned.SHA256SUMS ]; then # The SHA256SUMS already exists, make sure it's exactly what we @@ -216,7 +201,6 @@ mkdir -p "$outsigdir" cat "${sha256sum_fragments[@]}" \ | sort -u \ | sort -k2 \ - | rfc4880_normalize_document \ > "$temp_all" if [ -e all.SHA256SUMS ]; then # The SHA256SUMS already exists, make sure it's exactly what we diff --git a/doc/release-process.md b/doc/release-process.md index c57fa5b23a..1b6472e812 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -199,26 +199,13 @@ popd ### After 3 or more people have guix-built and their results match: -Combine `all.SHA256SUMS` and `all.SHA256SUMS.asc` into a clear-signed -`SHA256SUMS.asc` message: - -```sh -echo -e "-----BEGIN PGP SIGNED MESSAGE-----\nHash: SHA256\n\n$(cat all.SHA256SUMS)\n$(cat filename.txt.asc)" > SHA256SUMS.asc -``` - -Here's an equivalent, more readable command if you're confident that you won't -mess up whitespaces when copy-pasting: +Combine the `all.SHA256SUMS.asc` file from all signers into `SHA256SUMS.asc`: ```bash -cat << EOF > SHA256SUMS.asc ------BEGIN PGP SIGNED MESSAGE----- -Hash: SHA256 - -$(cat all.SHA256SUMS) -$(cat all.SHA256SUMS.asc) -EOF +cat "$VERSION"/*/all.SHA256SUMS.asc > SHA256SUMS.asc ``` + - Upload to the bitcoincore.org server (`/var/www/bin/bitcoin-core-${VERSION}`): 1. The contents of `./bitcoin/guix-build-${VERSION}/output`, except for `*-debug*` files. @@ -230,7 +217,9 @@ EOF as save storage space *do not upload these to the bitcoincore.org server, nor put them in the torrent*. - 2. The combined clear-signed message you just created `SHA256SUMS.asc` + 2. The `SHA256SUMS` file + + 3. The `SHA256SUMS.asc` combined signature file you just created - Create a torrent of the `/var/www/bin/bitcoin-core-${VERSION}` directory such that at the top level there is only one file: the `bitcoin-core-${VERSION}` From d451b60d22576dff7a2c8d6a8b5880d9d69e397c Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Thu, 5 Aug 2021 19:00:57 -0400 Subject: [PATCH 15/35] guix-verify: Non-zero exit code when anything fails Previously, if verification fails, the correct message will be printed, but the exit code would still be 0. --- contrib/guix/guix-verify | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/contrib/guix/guix-verify b/contrib/guix/guix-verify index e4863f115b..02ae022741 100755 --- a/contrib/guix/guix-verify +++ b/contrib/guix/guix-verify @@ -77,11 +77,13 @@ verify() { echo "" echo "Hint: Either the signature is invalid or the public key is missing" echo "" + failure=1 elif ! diff --report-identical "$compare_manifest" "$current_manifest" 1>&2; then echo "ERR: The SHA256SUMS attestation in these two directories differ:" echo " '${compare_manifest}'" echo " '${current_manifest}'" echo "" + failure=1 else echo "Verified: '${current_manifest}'" echo "" @@ -166,3 +168,7 @@ if (( ${#all_noncodesigned[@]} + ${#all_all[@]} == 0 )); then echo "" exit 1 fi + +if [ -n "$failure" ]; then + exit 1 +fi From 3cd663a5d33aa7ef87994e452bced7f192d021a0 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 15 Jul 2021 07:29:19 +0100 Subject: [PATCH 16/35] [policy] ancestor/descendant limits for packages --- src/validation.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 1b3d00bc6d..ec457da5cc 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1079,6 +1079,19 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: m_viewmempool.PackageAddTransaction(ws.m_ptx); } + // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, + // because it's unnecessary. Also, CPFP carve out can increase the limit for individual + // transactions, but this exemption is not extended to packages in CheckPackageLimits(). + std::string err_string; + if (txns.size() > 1 && + !m_pool.CheckPackageLimits(txns, m_limit_ancestors, m_limit_ancestor_size, m_limit_descendants, + m_limit_descendant_size, err_string)) { + // All transactions must have individually passed mempool ancestor and descendant limits + // inside of PreChecks(), so this is separate from an individual transaction error. + package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", err_string); + return PackageMempoolAcceptResult(package_state, std::move(results)); + } + for (Workspace& ws : workspaces) { PrecomputedTransactionData txdata; if (!PolicyScriptChecks(args, ws, txdata)) { From f8253d69d6f02850995a11eeb71fedc22e6f6575 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 3 Aug 2021 15:30:43 +0100 Subject: [PATCH 17/35] extract/rename helper functions from rpc_packages.py MOVEONLY; no change in behavior. Rename because there is another helper funciton in chain_transaction in test_framework.util.py --- test/functional/rpc_packages.py | 59 ++++-------------------- test/functional/test_framework/wallet.py | 53 +++++++++++++++++++++ 2 files changed, 62 insertions(+), 50 deletions(-) diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index 4b2ed20958..3cb4154601 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -22,6 +22,11 @@ from test_framework.util import ( assert_equal, ) +from test_framework.wallet import ( + create_child_with_parents, + create_raw_chain, + make_chain, +) class RPCPackagesTest(BitcoinTestFramework): def set_test_params(self): @@ -78,26 +83,6 @@ def run_test(self): self.test_conflicting() self.test_rbf() - def chain_transaction(self, parent_txid, parent_value, n=0, parent_locking_script=None): - """Build a transaction that spends parent_txid.vout[n] and produces one output with - amount = parent_value with a fee deducted. - Return tuple (CTransaction object, raw hex, nValue, scriptPubKey of the output created). - """ - node = self.nodes[0] - inputs = [{"txid": parent_txid, "vout": n}] - my_value = parent_value - Decimal("0.0001") - outputs = {self.address : my_value} - rawtx = node.createrawtransaction(inputs, outputs) - prevtxs = [{ - "txid": parent_txid, - "vout": n, - "scriptPubKey": parent_locking_script, - "amount": parent_value, - }] if parent_locking_script else None - signedtx = node.signrawtransactionwithkey(hexstring=rawtx, privkeys=self.privkeys, prevtxs=prevtxs) - assert signedtx["complete"] - tx = tx_from_hex(signedtx["hex"]) - return (tx, signedtx["hex"], my_value, tx.vout[0].scriptPubKey.hex()) def test_independent(self): self.log.info("Test multiple independent transactions in a package") @@ -148,20 +133,7 @@ def test_independent(self): def test_chain(self): node = self.nodes[0] first_coin = self.coins.pop() - - # Chain of 25 transactions - parent_locking_script = None - txid = first_coin["txid"] - chain_hex = [] - chain_txns = [] - value = first_coin["amount"] - - for _ in range(25): - (tx, txhex, value, parent_locking_script) = self.chain_transaction(txid, value, 0, parent_locking_script) - txid = tx.rehash() - chain_hex.append(txhex) - chain_txns.append(tx) - + (chain_hex, chain_txns) = create_raw_chain(node, first_coin, self.address, self.privkeys) self.log.info("Check that testmempoolaccept requires packages to be sorted by dependency") assert_equal(node.testmempoolaccept(rawtxs=chain_hex[::-1]), [{"txid": tx.rehash(), "wtxid": tx.getwtxid(), "package-error": "package-not-sorted"} for tx in chain_txns[::-1]]) @@ -201,7 +173,7 @@ def test_multiple_children(self): child_value = value - Decimal("0.0001") # Child A - (_, tx_child_a_hex, _, _) = self.chain_transaction(parent_txid, child_value, 0, parent_locking_script_a) + (_, tx_child_a_hex, _, _) = make_chain(node, self.address, self.privkeys, parent_txid, child_value, 0, parent_locking_script_a) assert not node.testmempoolaccept([tx_child_a_hex])[0]["allowed"] # Child B @@ -226,19 +198,6 @@ def test_multiple_children(self): node.sendrawtransaction(rawtx) assert_equal(testres_single, testres_multiple_ab) - def create_child_with_parents(self, parents_tx, values, locking_scripts): - """Creates a transaction that spends the first output of each parent in parents_tx.""" - num_parents = len(parents_tx) - total_value = sum(values) - inputs = [{"txid": tx.rehash(), "vout": 0} for tx in parents_tx] - outputs = {self.address : total_value - num_parents * Decimal("0.0001")} - rawtx_child = self.nodes[0].createrawtransaction(inputs, outputs) - prevtxs = [] - for i in range(num_parents): - prevtxs.append({"txid": parents_tx[i].rehash(), "vout": 0, "scriptPubKey": locking_scripts[i], "amount": values[i]}) - signedtx_child = self.nodes[0].signrawtransactionwithkey(hexstring=rawtx_child, privkeys=self.privkeys, prevtxs=prevtxs) - assert signedtx_child["complete"] - return signedtx_child["hex"] def test_multiple_parents(self): node = self.nodes[0] @@ -253,12 +212,12 @@ def test_multiple_parents(self): for _ in range(num_parents): parent_coin = self.coins.pop() value = parent_coin["amount"] - (tx, txhex, value, parent_locking_script) = self.chain_transaction(parent_coin["txid"], value) + (tx, txhex, value, parent_locking_script) = make_chain(node, self.address, self.privkeys, parent_coin["txid"], value) package_hex.append(txhex) parents_tx.append(tx) values.append(value) parent_locking_scripts.append(parent_locking_script) - child_hex = self.create_child_with_parents(parents_tx, values, parent_locking_scripts) + child_hex = create_child_with_parents(node, self.address, self.privkeys, parents_tx, values, parent_locking_scripts) # Package accept should work with the parents in any order (as long as parents come before child) for _ in range(10): random.shuffle(package_hex) diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py index 609553c6d0..e1f159e4ec 100644 --- a/test/functional/test_framework/wallet.py +++ b/test/functional/test_framework/wallet.py @@ -16,6 +16,7 @@ CTxIn, CTxInWitness, CTxOut, + tx_from_hex, ) from test_framework.script import ( CScript, @@ -176,3 +177,55 @@ def create_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_ def sendrawtransaction(self, *, from_node, tx_hex): from_node.sendrawtransaction(tx_hex) self.scan_tx(from_node.decoderawtransaction(tx_hex)) + +def make_chain(node, address, privkeys, parent_txid, parent_value, n=0, parent_locking_script=None): + """Build a transaction that spends parent_txid.vout[n] and produces one output with + amount = parent_value with a fee deducted. + Return tuple (CTransaction object, raw hex, nValue, scriptPubKey of the output created). + """ + inputs = [{"txid": parent_txid, "vout": n}] + my_value = parent_value - Decimal("0.0001") + outputs = {address : my_value} + rawtx = node.createrawtransaction(inputs, outputs) + prevtxs = [{ + "txid": parent_txid, + "vout": n, + "scriptPubKey": parent_locking_script, + "amount": parent_value, + }] if parent_locking_script else None + signedtx = node.signrawtransactionwithkey(hexstring=rawtx, privkeys=privkeys, prevtxs=prevtxs) + assert signedtx["complete"] + tx = tx_from_hex(signedtx["hex"]) + return (tx, signedtx["hex"], my_value, tx.vout[0].scriptPubKey.hex()) + +def create_child_with_parents(node, address, privkeys, parents_tx, values, locking_scripts): + """Creates a transaction that spends the first output of each parent in parents_tx.""" + num_parents = len(parents_tx) + total_value = sum(values) + inputs = [{"txid": tx.rehash(), "vout": 0} for tx in parents_tx] + outputs = {address : total_value - num_parents * Decimal("0.0001")} + rawtx_child = node.createrawtransaction(inputs, outputs) + prevtxs = [] + for i in range(num_parents): + prevtxs.append({"txid": parents_tx[i].rehash(), "vout": 0, "scriptPubKey": locking_scripts[i], "amount": values[i]}) + signedtx_child = node.signrawtransactionwithkey(hexstring=rawtx_child, privkeys=privkeys, prevtxs=prevtxs) + assert signedtx_child["complete"] + return signedtx_child["hex"] + +def create_raw_chain(node, first_coin, address, privkeys, chain_length=25): + """Helper function: create a "chain" of chain_length transactions. The nth transaction in the + chain is a child of the n-1th transaction and parent of the n+1th transaction. + """ + parent_locking_script = None + txid = first_coin["txid"] + chain_hex = [] + chain_txns = [] + value = first_coin["amount"] + + for _ in range(chain_length): + (tx, txhex, value, parent_locking_script) = make_chain(node, address, privkeys, txid, value, 0, parent_locking_script) + txid = tx.rehash() + chain_hex.append(txhex) + chain_txns.append(tx) + + return (chain_hex, chain_txns) From 313c09f7b7beddfdb74c284720d209c81dfdb94f Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 5 Aug 2021 14:01:51 +0100 Subject: [PATCH 18/35] [test] helper function to increase transaction weight --- test/functional/test_framework/wallet.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py index e1f159e4ec..c36415ee91 100644 --- a/test/functional/test_framework/wallet.py +++ b/test/functional/test_framework/wallet.py @@ -4,8 +4,10 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """A limited-functionality wallet, which may replace a real wallet in tests""" +from copy import deepcopy from decimal import Decimal from enum import Enum +from random import choice from typing import Optional from test_framework.address import ADDRESS_BCRT1_P2WSH_OP_TRUE from test_framework.key import ECKey @@ -28,6 +30,7 @@ ) from test_framework.util import ( assert_equal, + assert_greater_than_or_equal, satoshi_round, ) @@ -229,3 +232,23 @@ def create_raw_chain(node, first_coin, address, privkeys, chain_length=25): chain_txns.append(tx) return (chain_hex, chain_txns) + +def bulk_transaction(tx, node, target_weight, privkeys, prevtxs=None): + """Pad a transaction with extra outputs until it reaches a target weight (or higher). + returns CTransaction object + """ + tx_heavy = deepcopy(tx) + assert_greater_than_or_equal(target_weight, tx_heavy.get_weight()) + while tx_heavy.get_weight() < target_weight: + random_spk = "6a4d0200" # OP_RETURN OP_PUSH2 512 bytes + for _ in range(512*2): + random_spk += choice("0123456789ABCDEF") + tx_heavy.vout.append(CTxOut(0, bytes.fromhex(random_spk))) + # Re-sign the transaction + if privkeys: + signed = node.signrawtransactionwithkey(tx_heavy.serialize().hex(), privkeys, prevtxs) + return tx_from_hex(signed["hex"]) + # OP_TRUE + tx_heavy.wit.vtxinwit = [CTxInWitness()] + tx_heavy.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])] + return tx_heavy From 2b6b26e57c24d2f0abd442c1c33098e3121572ce Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 5 Aug 2021 15:30:25 +0100 Subject: [PATCH 19/35] [test] parameterizable fee for make_chain and create_child_with_parents --- test/functional/test_framework/wallet.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py index c36415ee91..ba5b95f930 100644 --- a/test/functional/test_framework/wallet.py +++ b/test/functional/test_framework/wallet.py @@ -34,6 +34,7 @@ satoshi_round, ) +DEFAULT_FEE = Decimal("0.0001") class MiniWalletMode(Enum): """Determines the transaction type the MiniWallet is creating and spending. @@ -181,13 +182,13 @@ def sendrawtransaction(self, *, from_node, tx_hex): from_node.sendrawtransaction(tx_hex) self.scan_tx(from_node.decoderawtransaction(tx_hex)) -def make_chain(node, address, privkeys, parent_txid, parent_value, n=0, parent_locking_script=None): +def make_chain(node, address, privkeys, parent_txid, parent_value, n=0, parent_locking_script=None, fee=DEFAULT_FEE): """Build a transaction that spends parent_txid.vout[n] and produces one output with amount = parent_value with a fee deducted. Return tuple (CTransaction object, raw hex, nValue, scriptPubKey of the output created). """ inputs = [{"txid": parent_txid, "vout": n}] - my_value = parent_value - Decimal("0.0001") + my_value = parent_value - fee outputs = {address : my_value} rawtx = node.createrawtransaction(inputs, outputs) prevtxs = [{ @@ -201,12 +202,12 @@ def make_chain(node, address, privkeys, parent_txid, parent_value, n=0, parent_l tx = tx_from_hex(signedtx["hex"]) return (tx, signedtx["hex"], my_value, tx.vout[0].scriptPubKey.hex()) -def create_child_with_parents(node, address, privkeys, parents_tx, values, locking_scripts): +def create_child_with_parents(node, address, privkeys, parents_tx, values, locking_scripts, fee=DEFAULT_FEE): """Creates a transaction that spends the first output of each parent in parents_tx.""" num_parents = len(parents_tx) total_value = sum(values) inputs = [{"txid": tx.rehash(), "vout": 0} for tx in parents_tx] - outputs = {address : total_value - num_parents * Decimal("0.0001")} + outputs = {address : total_value - fee} rawtx_child = node.createrawtransaction(inputs, outputs) prevtxs = [] for i in range(num_parents): From accf3d5868460b4b14ab607fd66ac985b086fbb3 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 15 Jul 2021 07:29:26 +0100 Subject: [PATCH 20/35] [test] mempool package ancestor/descendant limits --- test/functional/mempool_package_limits.py | 475 ++++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 476 insertions(+) create mode 100755 test/functional/mempool_package_limits.py diff --git a/test/functional/mempool_package_limits.py b/test/functional/mempool_package_limits.py new file mode 100755 index 0000000000..749ec6aa77 --- /dev/null +++ b/test/functional/mempool_package_limits.py @@ -0,0 +1,475 @@ +#!/usr/bin/env python3 +# Copyright (c) 2021 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 logic for limiting mempool and package ancestors/descendants.""" + +from decimal import Decimal + +from test_framework.address import ADDRESS_BCRT1_P2WSH_OP_TRUE +from test_framework.test_framework import BitcoinTestFramework +from test_framework.messages import ( + COIN, + CTransaction, + CTxInWitness, + tx_from_hex, + WITNESS_SCALE_FACTOR, +) +from test_framework.script import ( + CScript, + OP_TRUE, +) +from test_framework.util import ( + assert_equal, +) +from test_framework.wallet import ( + bulk_transaction, + create_child_with_parents, + make_chain, +) + +class MempoolPackageLimitsTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 1 + self.setup_clean_chain = True + + def run_test(self): + self.log.info("Generate blocks to create UTXOs") + node = self.nodes[0] + self.privkeys = [node.get_deterministic_priv_key().key] + self.address = node.get_deterministic_priv_key().address + self.coins = [] + # The last 100 coinbase transactions are premature + for b in node.generatetoaddress(200, self.address)[:100]: + coinbase = node.getblock(blockhash=b, verbosity=2)["tx"][0] + self.coins.append({ + "txid": coinbase["txid"], + "amount": coinbase["vout"][0]["value"], + "scriptPubKey": coinbase["vout"][0]["scriptPubKey"], + }) + + self.test_chain_limits() + self.test_desc_count_limits() + self.test_anc_count_limits() + self.test_anc_count_limits_2() + self.test_anc_count_limits_bushy() + + # The node will accept our (nonstandard) extra large OP_RETURN outputs + self.restart_node(0, extra_args=["-acceptnonstdtxn=1"]) + self.test_anc_size_limits() + self.test_desc_size_limits() + + def test_chain_limits_helper(self, mempool_count, package_count): + node = self.nodes[0] + assert_equal(0, node.getmempoolinfo()["size"]) + first_coin = self.coins.pop() + spk = None + txid = first_coin["txid"] + chain_hex = [] + chain_txns = [] + value = first_coin["amount"] + + for i in range(mempool_count + package_count): + (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk) + txid = tx.rehash() + if i < mempool_count: + node.sendrawtransaction(txhex) + assert_equal(node.getrawmempool(verbose=True)[txid]["ancestorcount"], i + 1) + else: + chain_hex.append(txhex) + chain_txns.append(tx) + testres_too_long = node.testmempoolaccept(rawtxs=chain_hex) + for txres in testres_too_long: + assert_equal(txres["package-error"], "package-mempool-limits") + + # Clear mempool and check that the package passes now + node.generate(1) + assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=chain_hex)]) + + def test_chain_limits(self): + """Create chains from mempool and package transactions that are longer than 25, + but only if both in-mempool and in-package transactions are considered together. + This checks that both mempool and in-package transactions are taken into account when + calculating ancestors/descendant limits. + """ + self.log.info("Check that in-package ancestors count for mempool ancestor limits") + + # 24 transactions in the mempool and 2 in the package. The parent in the package has + # 24 in-mempool ancestors and 1 in-package descendant. The child has 0 direct parents + # in the mempool, but 25 in-mempool and in-package ancestors in total. + self.test_chain_limits_helper(24, 2) + # 2 transactions in the mempool and 24 in the package. + self.test_chain_limits_helper(2, 24) + # 13 transactions in the mempool and 13 in the package. + self.test_chain_limits_helper(13, 13) + + def test_desc_count_limits(self): + """Create an 'A' shaped package with 24 transactions in the mempool and 2 in the package: + M1 + ^ ^ + M2a M2b + . . + . . + . . + M12a ^ + ^ M13b + ^ ^ + Pa Pb + The top ancestor in the package exceeds descendant limits but only if the in-mempool and in-package + descendants are all considered together (24 including in-mempool descendants and 26 including both + package transactions). + """ + node = self.nodes[0] + assert_equal(0, node.getmempoolinfo()["size"]) + self.log.info("Check that in-mempool and in-package descendants are calculated properly in packages") + # Top parent in mempool, M1 + first_coin = self.coins.pop() + parent_value = (first_coin["amount"] - Decimal("0.0002")) / 2 # Deduct reasonable fee and make 2 outputs + inputs = [{"txid": first_coin["txid"], "vout": 0}] + outputs = [{self.address : parent_value}, {ADDRESS_BCRT1_P2WSH_OP_TRUE : parent_value}] + rawtx = node.createrawtransaction(inputs, outputs) + + parent_signed = node.signrawtransactionwithkey(hexstring=rawtx, privkeys=self.privkeys) + assert parent_signed["complete"] + parent_tx = tx_from_hex(parent_signed["hex"]) + parent_txid = parent_tx.rehash() + node.sendrawtransaction(parent_signed["hex"]) + + package_hex = [] + + # Chain A + spk = parent_tx.vout[0].scriptPubKey.hex() + value = parent_value + txid = parent_txid + for i in range(12): + (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk) + txid = tx.rehash() + if i < 11: # M2a... M12a + node.sendrawtransaction(txhex) + else: # Pa + package_hex.append(txhex) + + # Chain B + value = parent_value - Decimal("0.0001") + rawtx_b = node.createrawtransaction([{"txid": parent_txid, "vout": 1}], {self.address : value}) + tx_child_b = tx_from_hex(rawtx_b) # M2b + tx_child_b.wit.vtxinwit = [CTxInWitness()] + tx_child_b.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])] + tx_child_b_hex = tx_child_b.serialize().hex() + node.sendrawtransaction(tx_child_b_hex) + spk = tx_child_b.vout[0].scriptPubKey.hex() + txid = tx_child_b.rehash() + for i in range(12): + (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk) + txid = tx.rehash() + if i < 11: # M3b... M13b + node.sendrawtransaction(txhex) + else: # Pb + package_hex.append(txhex) + + assert_equal(24, node.getmempoolinfo()["size"]) + assert_equal(2, len(package_hex)) + testres_too_long = node.testmempoolaccept(rawtxs=package_hex) + for txres in testres_too_long: + assert_equal(txres["package-error"], "package-mempool-limits") + + # Clear mempool and check that the package passes now + node.generate(1) + assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=package_hex)]) + + def test_anc_count_limits(self): + """Create a 'V' shaped chain with 24 transactions in the mempool and 3 in the package: + M1a M1b + ^ ^ + M2a M2b + . . + . . + . . + M12a M12b + ^ ^ + Pa Pb + ^ ^ + Pc + The lowest descendant, Pc, exceeds ancestor limits, but only if the in-mempool + and in-package ancestors are all considered together. + """ + node = self.nodes[0] + assert_equal(0, node.getmempoolinfo()["size"]) + package_hex = [] + parents_tx = [] + values = [] + scripts = [] + + self.log.info("Check that in-mempool and in-package ancestors are calculated properly in packages") + + # Two chains of 13 transactions each + for _ in range(2): + spk = None + top_coin = self.coins.pop() + txid = top_coin["txid"] + value = top_coin["amount"] + for i in range(13): + (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk) + txid = tx.rehash() + if i < 12: + node.sendrawtransaction(txhex) + else: # Save the 13th transaction for the package + package_hex.append(txhex) + parents_tx.append(tx) + scripts.append(spk) + values.append(value) + + # Child Pc + child_hex = create_child_with_parents(node, self.address, self.privkeys, parents_tx, values, scripts) + package_hex.append(child_hex) + + assert_equal(24, node.getmempoolinfo()["size"]) + assert_equal(3, len(package_hex)) + testres_too_long = node.testmempoolaccept(rawtxs=package_hex) + for txres in testres_too_long: + assert_equal(txres["package-error"], "package-mempool-limits") + + # Clear mempool and check that the package passes now + node.generate(1) + assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=package_hex)]) + + def test_anc_count_limits_2(self): + """Create a 'Y' shaped chain with 24 transactions in the mempool and 2 in the package: + M1a M1b + ^ ^ + M2a M2b + . . + . . + . . + M12a M12b + ^ ^ + Pc + ^ + Pd + The lowest descendant, Pd, exceeds ancestor limits, but only if the in-mempool + and in-package ancestors are all considered together. + """ + node = self.nodes[0] + assert_equal(0, node.getmempoolinfo()["size"]) + parents_tx = [] + values = [] + scripts = [] + + self.log.info("Check that in-mempool and in-package ancestors are calculated properly in packages") + # Two chains of 12 transactions each + for _ in range(2): + spk = None + top_coin = self.coins.pop() + txid = top_coin["txid"] + value = top_coin["amount"] + for i in range(12): + (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk) + txid = tx.rehash() + value -= Decimal("0.0001") + node.sendrawtransaction(txhex) + if i == 11: + # last 2 transactions will be the parents of Pc + parents_tx.append(tx) + values.append(value) + scripts.append(spk) + + # Child Pc + pc_hex = create_child_with_parents(node, self.address, self.privkeys, parents_tx, values, scripts) + pc_tx = tx_from_hex(pc_hex) + pc_value = sum(values) - Decimal("0.0002") + pc_spk = pc_tx.vout[0].scriptPubKey.hex() + + # Child Pd + (_, pd_hex, _, _) = make_chain(node, self.address, self.privkeys, pc_tx.rehash(), pc_value, 0, pc_spk) + + assert_equal(24, node.getmempoolinfo()["size"]) + testres_too_long = node.testmempoolaccept(rawtxs=[pc_hex, pd_hex]) + for txres in testres_too_long: + assert_equal(txres["package-error"], "package-mempool-limits") + + # Clear mempool and check that the package passes now + node.generate(1) + assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=[pc_hex, pd_hex])]) + + def test_anc_count_limits_bushy(self): + """Create a tree with 20 transactions in the mempool and 6 in the package: + M1...M4 M5...M8 M9...M12 M13...M16 M17...M20 + ^ ^ ^ ^ ^ (each with 4 parents) + P0 P1 P2 P3 P4 + ^ ^ ^ ^ ^ (5 parents) + PC + Where M(4i+1)...M+(4i+4) are the parents of Pi and P0, P1, P2, P3, and P4 are the parents of PC. + P0... P4 individually only have 4 parents each, and PC has no in-mempool parents. But + combined, PC has 25 in-mempool and in-package parents. + """ + node = self.nodes[0] + assert_equal(0, node.getmempoolinfo()["size"]) + package_hex = [] + parent_txns = [] + parent_values = [] + scripts = [] + for _ in range(5): # Make package transactions P0 ... P4 + gp_tx = [] + gp_values = [] + gp_scripts = [] + for _ in range(4): # Make mempool transactions M(4i+1)...M(4i+4) + parent_coin = self.coins.pop() + value = parent_coin["amount"] + txid = parent_coin["txid"] + (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value) + gp_tx.append(tx) + gp_values.append(value) + gp_scripts.append(spk) + node.sendrawtransaction(txhex) + # Package transaction Pi + pi_hex = create_child_with_parents(node, self.address, self.privkeys, gp_tx, gp_values, gp_scripts) + package_hex.append(pi_hex) + pi_tx = tx_from_hex(pi_hex) + parent_txns.append(pi_tx) + parent_values.append(Decimal(pi_tx.vout[0].nValue) / COIN) + scripts.append(pi_tx.vout[0].scriptPubKey.hex()) + # Package transaction PC + package_hex.append(create_child_with_parents(node, self.address, self.privkeys, parent_txns, parent_values, scripts)) + + assert_equal(20, node.getmempoolinfo()["size"]) + assert_equal(6, len(package_hex)) + testres = node.testmempoolaccept(rawtxs=package_hex) + for txres in testres: + assert_equal(txres["package-error"], "package-mempool-limits") + + # Clear mempool and check that the package passes now + node.generate(1) + assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=package_hex)]) + + def test_anc_size_limits(self): + """Test Case with 2 independent transactions in the mempool and a parent + child in the + package, where the package parent is the child of both mempool transactions (30KvB each): + A B + ^ ^ + C + ^ + D + The lowest descendant, D, exceeds ancestor size limits, but only if the in-mempool + and in-package ancestors are all considered together. + """ + node = self.nodes[0] + assert_equal(0, node.getmempoolinfo()["size"]) + parents_tx = [] + values = [] + scripts = [] + target_weight = WITNESS_SCALE_FACTOR * 1000 * 30 # 30KvB + high_fee = Decimal("0.003") # 10 sats/vB + self.log.info("Check that in-mempool and in-package ancestor size limits are calculated properly in packages") + # Mempool transactions A and B + for _ in range(2): + spk = None + top_coin = self.coins.pop() + txid = top_coin["txid"] + value = top_coin["amount"] + (tx, _, _, _) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk, high_fee) + bulked_tx = bulk_transaction(tx, node, target_weight, self.privkeys) + node.sendrawtransaction(bulked_tx.serialize().hex()) + parents_tx.append(bulked_tx) + values.append(Decimal(bulked_tx.vout[0].nValue) / COIN) + scripts.append(bulked_tx.vout[0].scriptPubKey.hex()) + + # Package transaction C + small_pc_hex = create_child_with_parents(node, self.address, self.privkeys, parents_tx, values, scripts, high_fee) + pc_tx = bulk_transaction(tx_from_hex(small_pc_hex), node, target_weight, self.privkeys) + pc_value = Decimal(pc_tx.vout[0].nValue) / COIN + pc_spk = pc_tx.vout[0].scriptPubKey.hex() + pc_hex = pc_tx.serialize().hex() + + # Package transaction D + (small_pd, _, val, spk) = make_chain(node, self.address, self.privkeys, pc_tx.rehash(), pc_value, 0, pc_spk, high_fee) + prevtxs = [{ + "txid": pc_tx.rehash(), + "vout": 0, + "scriptPubKey": spk, + "amount": val, + }] + pd_tx = bulk_transaction(small_pd, node, target_weight, self.privkeys, prevtxs) + pd_hex = pd_tx.serialize().hex() + + assert_equal(2, node.getmempoolinfo()["size"]) + testres_too_heavy = node.testmempoolaccept(rawtxs=[pc_hex, pd_hex]) + for txres in testres_too_heavy: + assert_equal(txres["package-error"], "package-mempool-limits") + + # Clear mempool and check that the package passes now + node.generate(1) + assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=[pc_hex, pd_hex])]) + + def test_desc_size_limits(self): + """Create 3 mempool transactions and 2 package transactions (25KvB each): + Ma + ^ ^ + Mb Mc + ^ ^ + Pd Pe + The top ancestor in the package exceeds descendant size limits but only if the in-mempool + and in-package descendants are all considered together. + """ + node = self.nodes[0] + assert_equal(0, node.getmempoolinfo()["size"]) + target_weight = 21 * 1000 * WITNESS_SCALE_FACTOR + high_fee = Decimal("0.0021") # 10 sats/vB + self.log.info("Check that in-mempool and in-package descendant sizes are calculated properly in packages") + # Top parent in mempool, Ma + first_coin = self.coins.pop() + parent_value = (first_coin["amount"] - high_fee) / 2 # Deduct fee and make 2 outputs + inputs = [{"txid": first_coin["txid"], "vout": 0}] + outputs = [{self.address : parent_value}, {ADDRESS_BCRT1_P2WSH_OP_TRUE: parent_value}] + rawtx = node.createrawtransaction(inputs, outputs) + parent_tx = bulk_transaction(tx_from_hex(rawtx), node, target_weight, self.privkeys) + node.sendrawtransaction(parent_tx.serialize().hex()) + + package_hex = [] + for j in range(2): # Two legs (left and right) + # Mempool transaction (Mb and Mc) + mempool_tx = CTransaction() + spk = parent_tx.vout[j].scriptPubKey.hex() + value = Decimal(parent_tx.vout[j].nValue) / COIN + txid = parent_tx.rehash() + prevtxs = [{ + "txid": txid, + "vout": j, + "scriptPubKey": spk, + "amount": value, + }] + if j == 0: # normal key + (tx_small, _, _, _) = make_chain(node, self.address, self.privkeys, txid, value, j, spk, high_fee) + mempool_tx = bulk_transaction(tx_small, node, target_weight, self.privkeys, prevtxs) + else: # OP_TRUE + inputs = [{"txid": txid, "vout": 1}] + outputs = {self.address: value - high_fee} + small_tx = tx_from_hex(node.createrawtransaction(inputs, outputs)) + mempool_tx = bulk_transaction(small_tx, node, target_weight, None, prevtxs) + node.sendrawtransaction(mempool_tx.serialize().hex()) + + # Package transaction (Pd and Pe) + spk = mempool_tx.vout[0].scriptPubKey.hex() + value = Decimal(mempool_tx.vout[0].nValue) / COIN + txid = mempool_tx.rehash() + (tx_small, _, _, _) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk, high_fee) + prevtxs = [{ + "txid": txid, + "vout": 0, + "scriptPubKey": spk, + "amount": value, + }] + package_tx = bulk_transaction(tx_small, node, target_weight, self.privkeys, prevtxs) + package_hex.append(package_tx.serialize().hex()) + + assert_equal(3, node.getmempoolinfo()["size"]) + assert_equal(2, len(package_hex)) + testres_too_heavy = node.testmempoolaccept(rawtxs=package_hex) + for txres in testres_too_heavy: + assert_equal(txres["package-error"], "package-mempool-limits") + + # Clear mempool and check that the package passes now + node.generate(1) + assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=package_hex)]) + +if __name__ == "__main__": + MempoolPackageLimitsTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index fecf52d53a..1a1a6a263a 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -218,6 +218,7 @@ 'rpc_createmultisig.py --legacy-wallet', 'rpc_createmultisig.py --descriptors', 'rpc_packages.py', + 'mempool_package_limits.py', 'feature_versionbits_warning.py', 'rpc_preciousblock.py', 'wallet_importprunedfunds.py --legacy-wallet', From 6a5ccd65c704253b7442b54064f5ba281c34fd26 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 6 Aug 2021 21:49:14 +0300 Subject: [PATCH 21/35] scripted-diff: Rename JoinErrors in more general MakeUnorderedList -BEGIN VERIFY SCRIPT- sed -i -e 's/JoinErrors/MakeUnorderedList/' -- src/qt/bitcoin.cpp -END VERIFY SCRIPT- --- src/qt/bitcoin.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 89b5ec6f4a..ababab31d3 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -144,7 +144,7 @@ static void initTranslations(QTranslator &qtTranslatorBase, QTranslator &qtTrans QApplication::installTranslator(&translator); } -static std::string JoinErrors(const std::vector& errors) +static std::string MakeUnorderedList(const std::vector& errors) { return Join(errors, "\n", [](const std::string& error) { return "- " + error; }); } @@ -158,13 +158,13 @@ static bool InitSettings() std::vector errors; if (!gArgs.ReadSettingsFile(&errors)) { bilingual_str error = _("Settings file could not be read"); - InitError(Untranslated(strprintf("%s:\n%s\n", error.original, JoinErrors(errors)))); + InitError(Untranslated(strprintf("%s:\n%s\n", error.original, MakeUnorderedList(errors)))); QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Reset | QMessageBox::Abort); /*: Explanatory text shown on startup when the settings file cannot be read. Prompts user to make a choice between resetting or aborting. */ messagebox.setInformativeText(QObject::tr("Do you want to reset settings to default values, or to abort without making changes?")); - messagebox.setDetailedText(QString::fromStdString(JoinErrors(errors))); + messagebox.setDetailedText(QString::fromStdString(MakeUnorderedList(errors))); messagebox.setTextFormat(Qt::PlainText); messagebox.setDefaultButton(QMessageBox::Reset); switch (messagebox.exec()) { @@ -180,14 +180,14 @@ static bool InitSettings() errors.clear(); if (!gArgs.WriteSettingsFile(&errors)) { bilingual_str error = _("Settings file could not be written"); - InitError(Untranslated(strprintf("%s:\n%s\n", error.original, JoinErrors(errors)))); + InitError(Untranslated(strprintf("%s:\n%s\n", error.original, MakeUnorderedList(errors)))); QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Ok); /*: Explanatory text shown on startup when the settings file could not be written. Prompts user to check that we have the ability to write to the file. Explains that the user has the option of running without a settings file.*/ messagebox.setInformativeText(QObject::tr("A fatal error occured. Check that settings file is writable, or try running with -nosettings.")); - messagebox.setDetailedText(QString::fromStdString(JoinErrors(errors))); + messagebox.setDetailedText(QString::fromStdString(MakeUnorderedList(errors))); messagebox.setTextFormat(Qt::PlainText); messagebox.setDefaultButton(QMessageBox::Ok); messagebox.exec(); From 77a90f03acd551bcc538f6728939cc2ed8c6a3c4 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 6 Aug 2021 21:51:57 +0300 Subject: [PATCH 22/35] refactor: Move MakeUnorderedList into util/string.h to make it reusable --- src/qt/bitcoin.cpp | 8 ++------ src/util/string.h | 8 ++++++++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index ababab31d3..09f453363d 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -144,11 +145,6 @@ static void initTranslations(QTranslator &qtTranslatorBase, QTranslator &qtTrans QApplication::installTranslator(&translator); } -static std::string MakeUnorderedList(const std::vector& errors) -{ - return Join(errors, "\n", [](const std::string& error) { return "- " + error; }); -} - static bool InitSettings() { if (!gArgs.GetSettingsPath()) { @@ -186,7 +182,7 @@ static bool InitSettings() /*: Explanatory text shown on startup when the settings file could not be written. Prompts user to check that we have the ability to write to the file. Explains that the user has the option of running without a settings file.*/ - messagebox.setInformativeText(QObject::tr("A fatal error occured. Check that settings file is writable, or try running with -nosettings.")); + messagebox.setInformativeText(QObject::tr("A fatal error occurred. Check that settings file is writable, or try running with -nosettings.")); messagebox.setDetailedText(QString::fromStdString(MakeUnorderedList(errors))); messagebox.setTextFormat(Qt::PlainText); messagebox.setDefaultButton(QMessageBox::Ok); diff --git a/src/util/string.h b/src/util/string.h index b26facc502..5617e4acc1 100644 --- a/src/util/string.h +++ b/src/util/string.h @@ -64,6 +64,14 @@ inline std::string Join(const std::vector& list, const std::string& return Join(list, separator); } +/** + * Create an unordered multi-line list of items. + */ +inline std::string MakeUnorderedList(const std::vector& items) +{ + return Join(items, "\n", [](const std::string& item) { return "- " + item; }); +} + /** * Check if a string does not contain any embedded NUL (\0) characters */ From bb56486a170aacb355f4a973f0cd40ab3918a0cd Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 6 Aug 2021 21:59:14 +0300 Subject: [PATCH 23/35] refactor: Reuse MakeUnorderedList where possible --- src/rpc/blockchain.cpp | 3 ++- src/util/system.cpp | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 4956ee39e9..909019d796 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -36,6 +36,7 @@ #include #include #include +#include #include #include #include @@ -1328,7 +1329,7 @@ static RPCHelpMan verifychain() "\nVerifies blockchain database.\n", { {"checklevel", RPCArg::Type::NUM, RPCArg::DefaultHint{strprintf("%d, range=0-4", DEFAULT_CHECKLEVEL)}, - strprintf("How thorough the block verification is:\n - %s", Join(CHECKLEVEL_DOC, "\n- "))}, + strprintf("How thorough the block verification is:\n%s", MakeUnorderedList(CHECKLEVEL_DOC))}, {"nblocks", RPCArg::Type::NUM, RPCArg::DefaultHint{strprintf("%d, 0=all", DEFAULT_CHECKBLOCKS)}, "The number of blocks to check."}, }, RPCResult{ diff --git a/src/util/system.cpp b/src/util/system.cpp index 258ba2f235..30d4103819 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -502,11 +502,11 @@ bool ArgsManager::InitSettings(std::string& error) std::vector errors; if (!ReadSettingsFile(&errors)) { - error = strprintf("Failed loading settings file:\n- %s\n", Join(errors, "\n- ")); + error = strprintf("Failed loading settings file:\n%s\n", MakeUnorderedList(errors)); return false; } if (!WriteSettingsFile(&errors)) { - error = strprintf("Failed saving settings file:\n- %s\n", Join(errors, "\n- ")); + error = strprintf("Failed saving settings file:\n%s\n", MakeUnorderedList(errors)); return false; } return true; From ae23faba6fc5cabc896f1175456d1018576f912d Mon Sep 17 00:00:00 2001 From: lsilva01 Date: Tue, 27 Jul 2021 14:28:23 -0300 Subject: [PATCH 24/35] Add a new RPC command: restorewallet --- src/rpc/client.cpp | 1 + src/wallet/rpcwallet.cpp | 119 +++++++++++++++++++++++++++++++-------- 2 files changed, 96 insertions(+), 24 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 9b5d181c4e..4357ab2bb3 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -187,6 +187,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "createwallet", 5, "descriptors"}, { "createwallet", 6, "load_on_startup"}, { "createwallet", 7, "external_signer"}, + { "restorewallet", 2, "load_on_startup"}, { "loadwallet", 1, "load_on_startup"}, { "unloadwallet", 1, "load_on_startup"}, { "getnodeaddresses", 0, "count"}, diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 2e2300f887..2a5b547858 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2572,6 +2572,37 @@ static RPCHelpMan listwallets() }; } +static std::tuple, std::vector> LoadWalletHelper(WalletContext& context, UniValue load_on_start_param, const std::string wallet_name) +{ + DatabaseOptions options; + DatabaseStatus status; + options.require_existing = true; + bilingual_str error; + std::vector warnings; + std::optional load_on_start = load_on_start_param.isNull() ? std::nullopt : std::optional(load_on_start_param.get_bool()); + std::shared_ptr const wallet = LoadWallet(*context.chain, wallet_name, load_on_start, options, status, error, warnings); + + if (!wallet) { + // Map bad format to not found, since bad format is returned when the + // wallet directory exists, but doesn't contain a data file. + RPCErrorCode code = RPC_WALLET_ERROR; + switch (status) { + case DatabaseStatus::FAILED_NOT_FOUND: + case DatabaseStatus::FAILED_BAD_FORMAT: + code = RPC_WALLET_NOT_FOUND; + break; + case DatabaseStatus::FAILED_ALREADY_LOADED: + code = RPC_WALLET_ALREADY_LOADED; + break; + default: // RPC_WALLET_ERROR is returned for all other cases. + break; + } + throw JSONRPCError(code, error.original); + } + + return { wallet, warnings }; +} + static RPCHelpMan loadwallet() { return RPCHelpMan{"loadwallet", @@ -2598,30 +2629,7 @@ static RPCHelpMan loadwallet() WalletContext& context = EnsureWalletContext(request.context); const std::string name(request.params[0].get_str()); - DatabaseOptions options; - DatabaseStatus status; - options.require_existing = true; - bilingual_str error; - std::vector warnings; - std::optional load_on_start = request.params[1].isNull() ? std::nullopt : std::optional(request.params[1].get_bool()); - std::shared_ptr const wallet = LoadWallet(*context.chain, name, load_on_start, options, status, error, warnings); - if (!wallet) { - // Map bad format to not found, since bad format is returned when the - // wallet directory exists, but doesn't contain a data file. - RPCErrorCode code = RPC_WALLET_ERROR; - switch (status) { - case DatabaseStatus::FAILED_NOT_FOUND: - case DatabaseStatus::FAILED_BAD_FORMAT: - code = RPC_WALLET_NOT_FOUND; - break; - case DatabaseStatus::FAILED_ALREADY_LOADED: - code = RPC_WALLET_ALREADY_LOADED; - break; - default: // RPC_WALLET_ERROR is returned for all other cases. - break; - } - throw JSONRPCError(code, error.original); - } + auto [wallet, warnings] = LoadWalletHelper(context, request.params[1], name); UniValue obj(UniValue::VOBJ); obj.pushKV("name", wallet->GetName()); @@ -2795,6 +2803,68 @@ static RPCHelpMan createwallet() }; } +static RPCHelpMan restorewallet() +{ + return RPCHelpMan{ + "restorewallet", + "\nRestore and loads a wallet from backup.\n", + { + {"wallet_name", RPCArg::Type::STR, RPCArg::Optional::NO, "The name that will be applied to the restored wallet"}, + {"backup_file", RPCArg::Type::STR, RPCArg::Optional::NO, "The backup file that will be used to restore the wallet."}, + {"load_on_startup", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED_NAMED_ARG, "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."}, + }, + RPCResult{ + RPCResult::Type::OBJ, "", "", + { + {RPCResult::Type::STR, "name", "The wallet name if restored successfully."}, + {RPCResult::Type::STR, "warning", "Warning message if wallet was not loaded cleanly."}, + } + }, + RPCExamples{ + HelpExampleCli("restorewallet", "\"testwallet\" \"home\\backups\\backup-file.bak\"") + + HelpExampleRpc("restorewallet", "\"testwallet\" \"home\\backups\\backup-file.bak\"") + + HelpExampleCliNamed("restorewallet", {{"wallet_name", "testwallet"}, {"backup_file", "home\\backups\\backup-file.bak\""}, {"load_on_startup", true}}) + + HelpExampleRpcNamed("restorewallet", {{"wallet_name", "testwallet"}, {"backup_file", "home\\backups\\backup-file.bak\""}, {"load_on_startup", true}}) + }, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue +{ + + WalletContext& context = EnsureWalletContext(request.context); + + std::string backup_file = request.params[1].get_str(); + + if (!fs::exists(backup_file)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Backup file does not exist"); + } + + std::string wallet_name = request.params[0].get_str(); + + const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), wallet_name); + + if (fs::exists(wallet_path)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Wallet name already exists."); + } + + if (!TryCreateDirectories(wallet_path)) { + throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Failed to create database path '%s'. Database already exists.", wallet_path.string())); + } + + auto wallet_file = wallet_path / "wallet.dat"; + + fs::copy_file(backup_file, wallet_file, fs::copy_option::fail_if_exists); + + auto [wallet, warnings] = LoadWalletHelper(context, request.params[2], wallet_name); + + UniValue obj(UniValue::VOBJ); + obj.pushKV("name", wallet->GetName()); + obj.pushKV("warning", Join(warnings, Untranslated("\n")).original); + + return obj; + +}, + }; +} + static RPCHelpMan unloadwallet() { return RPCHelpMan{"unloadwallet", @@ -4639,6 +4709,7 @@ static const CRPCCommand commands[] = { "wallet", &bumpfee, }, { "wallet", &psbtbumpfee, }, { "wallet", &createwallet, }, + { "wallet", &restorewallet, }, { "wallet", &dumpprivkey, }, { "wallet", &dumpwallet, }, { "wallet", &encryptwallet, }, From 5fe8100ff36fed6d50c2a25b028f57b25af3504c Mon Sep 17 00:00:00 2001 From: lsilva01 Date: Fri, 30 Jul 2021 14:59:27 -0300 Subject: [PATCH 25/35] Change the wallet_backup.py test to use the restorewallet RPC command instead of restoring wallets manually. --- test/functional/wallet_backup.py | 45 ++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 16 deletions(-) diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index 05a0ef0ea1..c7a983556d 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -111,6 +111,18 @@ def erase_three(self): os.remove(os.path.join(self.nodes[1].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename)) os.remove(os.path.join(self.nodes[2].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename)) + def restore_nonexistent_wallet(self): + node = self.nodes[3] + nonexistent_wallet_file = os.path.join(self.nodes[0].datadir, 'nonexistent_wallet.bak') + wallet_name = "res0" + assert_raises_rpc_error(-8, "Backup file does not exist", node.restorewallet, wallet_name, nonexistent_wallet_file) + + def restore_wallet_existent_name(self): + node = self.nodes[3] + wallet_file = os.path.join(self.nodes[0].datadir, 'wallet.bak') + wallet_name = "res0" + assert_raises_rpc_error(-8, "Wallet name already exists.", node.restorewallet, wallet_name, wallet_file) + def init_three(self): self.init_wallet(0) self.init_wallet(1) @@ -169,26 +181,27 @@ def run_test(self): ## # Test restoring spender wallets from backups ## - self.log.info("Restoring using wallet.dat") - self.stop_three() - self.erase_three() + self.log.info("Restoring wallets on node 3 using backup files") - # Start node2 with no chain - shutil.rmtree(os.path.join(self.nodes[2].datadir, self.chain, 'blocks')) - shutil.rmtree(os.path.join(self.nodes[2].datadir, self.chain, 'chainstate')) + self.restore_nonexistent_wallet() - # Restore wallets from backup - shutil.copyfile(os.path.join(self.nodes[0].datadir, 'wallet.bak'), os.path.join(self.nodes[0].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename)) - shutil.copyfile(os.path.join(self.nodes[1].datadir, 'wallet.bak'), os.path.join(self.nodes[1].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename)) - shutil.copyfile(os.path.join(self.nodes[2].datadir, 'wallet.bak'), os.path.join(self.nodes[2].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename)) + backup_file_0 = os.path.join(self.nodes[0].datadir, 'wallet.bak') + backup_file_1 = os.path.join(self.nodes[1].datadir, 'wallet.bak') + backup_file_2 = os.path.join(self.nodes[2].datadir, 'wallet.bak') - self.log.info("Re-starting nodes") - self.start_three() - self.sync_blocks() + self.nodes[3].restorewallet("res0", backup_file_0) + self.nodes[3].restorewallet("res1", backup_file_1) + self.nodes[3].restorewallet("res2", backup_file_2) + + res0_rpc = self.nodes[3].get_wallet_rpc("res0") + res1_rpc = self.nodes[3].get_wallet_rpc("res1") + res2_rpc = self.nodes[3].get_wallet_rpc("res2") + + assert_equal(res0_rpc.getbalance(), balance0) + assert_equal(res1_rpc.getbalance(), balance1) + assert_equal(res2_rpc.getbalance(), balance2) - assert_equal(self.nodes[0].getbalance(), balance0) - assert_equal(self.nodes[1].getbalance(), balance1) - assert_equal(self.nodes[2].getbalance(), balance2) + self.restore_wallet_existent_name() if not self.options.descriptors: self.log.info("Restoring using dumped wallet") From ad28b66e98c9bb3bc7af2545654842544a798601 Mon Sep 17 00:00:00 2001 From: Prateek Sancheti Date: Thu, 29 Jul 2021 21:57:57 +0530 Subject: [PATCH 26/35] qt: Add SubFeeFromAmount option --- src/qt/forms/optionsdialog.ui | 10 ++++++++++ src/qt/optionsdialog.cpp | 1 + src/qt/optionsmodel.cpp | 11 +++++++++++ src/qt/optionsmodel.h | 3 +++ src/qt/sendcoinsentry.cpp | 4 +++- 5 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/qt/forms/optionsdialog.ui b/src/qt/forms/optionsdialog.ui index bd72328c02..864d550551 100644 --- a/src/qt/forms/optionsdialog.ui +++ b/src/qt/forms/optionsdialog.ui @@ -200,6 +200,16 @@ W&allet + + + + Whether to set subtract fee from amount as default or not. + + + Subtract &fee from amount by default + + + diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp index b12fe96567..5ad4fc9b33 100644 --- a/src/qt/optionsdialog.cpp +++ b/src/qt/optionsdialog.cpp @@ -239,6 +239,7 @@ void OptionsDialog::setMapper() /* Wallet */ mapper->addMapping(ui->spendZeroConfChange, OptionsModel::SpendZeroConfChange); mapper->addMapping(ui->coinControlFeatures, OptionsModel::CoinControlFeatures); + mapper->addMapping(ui->subFeeFromAmount, OptionsModel::SubFeeFromAmount); mapper->addMapping(ui->externalSignerPath, OptionsModel::ExternalSignerPath); /* Network */ diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index 24a4e9ee96..d87fc1f84a 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -124,6 +124,11 @@ void OptionsModel::Init(bool resetSettings) if (!gArgs.SoftSetArg("-signer", settings.value("external_signer_path").toString().toStdString())) { addOverriddenOption("-signer"); } + + if (!settings.contains("SubFeeFromAmount")) { + settings.setValue("SubFeeFromAmount", false); + } + m_sub_fee_from_amount = settings.value("SubFeeFromAmount", false).toBool(); #endif // Network @@ -335,6 +340,8 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const return settings.value("bSpendZeroConfChange"); case ExternalSignerPath: return settings.value("external_signer_path"); + case SubFeeFromAmount: + return m_sub_fee_from_amount; #endif case DisplayUnit: return nDisplayUnit; @@ -460,6 +467,10 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in setRestartRequired(true); } break; + case SubFeeFromAmount: + m_sub_fee_from_amount = value.toBool(); + settings.setValue("SubFeeFromAmount", m_sub_fee_from_amount); + break; #endif case DisplayUnit: setDisplayUnit(value); diff --git a/src/qt/optionsmodel.h b/src/qt/optionsmodel.h index 535843e8ba..203ee27ad8 100644 --- a/src/qt/optionsmodel.h +++ b/src/qt/optionsmodel.h @@ -61,6 +61,7 @@ class OptionsModel : public QAbstractListModel Language, // QString UseEmbeddedMonospacedFont, // bool CoinControlFeatures, // bool + SubFeeFromAmount, // bool ThreadsScriptVerif, // int Prune, // bool PruneSize, // int @@ -88,6 +89,7 @@ class OptionsModel : public QAbstractListModel QString getThirdPartyTxUrls() const { return strThirdPartyTxUrls; } bool getUseEmbeddedMonospacedFont() const { return m_use_embedded_monospaced_font; } bool getCoinControlFeatures() const { return fCoinControlFeatures; } + bool getSubFeeFromAmount() const { return m_sub_fee_from_amount; } const QString& getOverriddenByCommandLine() { return strOverriddenByCommandLine; } /* Explicit setters */ @@ -112,6 +114,7 @@ class OptionsModel : public QAbstractListModel QString strThirdPartyTxUrls; bool m_use_embedded_monospaced_font; bool fCoinControlFeatures; + bool m_sub_fee_from_amount; /* settings that were overridden by command-line */ QString strOverriddenByCommandLine; diff --git a/src/qt/sendcoinsentry.cpp b/src/qt/sendcoinsentry.cpp index 683c0441fa..5fa5165615 100644 --- a/src/qt/sendcoinsentry.cpp +++ b/src/qt/sendcoinsentry.cpp @@ -97,7 +97,9 @@ void SendCoinsEntry::clear() ui->payTo->clear(); ui->addAsLabel->clear(); ui->payAmount->clear(); - ui->checkboxSubtractFeeFromAmount->setCheckState(Qt::Unchecked); + if (model && model->getOptionsModel()) { + ui->checkboxSubtractFeeFromAmount->setChecked(model->getOptionsModel()->getSubFeeFromAmount()); + } ui->messageTextLabel->clear(); ui->messageTextLabel->hide(); ui->messageLabel->hide(); From 62b125fd197879f112322a1f67a318d6ab22e67a Mon Sep 17 00:00:00 2001 From: Prateek Sancheti Date: Tue, 10 Aug 2021 15:13:30 +0530 Subject: [PATCH 27/35] qt, refactor: Fix indentation --- src/qt/forms/optionsdialog.ui | 70 +++++++++++++++++------------------ 1 file changed, 35 insertions(+), 35 deletions(-) diff --git a/src/qt/forms/optionsdialog.ui b/src/qt/forms/optionsdialog.ui index 864d550551..2ff1445709 100644 --- a/src/qt/forms/optionsdialog.ui +++ b/src/qt/forms/optionsdialog.ui @@ -51,20 +51,20 @@ - - - - - Enabling pruning significantly reduces the disk space required to store transactions. All blocks are still fully validated. Reverting this setting requires re-downloading the entire blockchain. - - - Prune &block storage to - - - - - - + + + + + Enabling pruning significantly reduces the disk space required to store transactions. All blocks are still fully validated. Reverting this setting requires re-downloading the entire blockchain. + + + Prune &block storage to + + + + + + @@ -245,27 +245,27 @@ External Signer (e.g. hardware wallet) - - - - - - &External signer script path - - - externalSignerPath - - - - - - - Full path to a Bitcoin Core compatible script (e.g. C:\Downloads\hwi.exe or /Users/you/Downloads/hwi.py). Beware: malware can steal your coins! - - - - - + + + + + + &External signer script path + + + externalSignerPath + + + + + + + Full path to a Bitcoin Core compatible script (e.g. C:\Downloads\hwi.exe or /Users/you/Downloads/hwi.py). Beware: malware can steal your coins! + + + + + From 0237d95323dec7c65b7223c314afdf63aab6b11b Mon Sep 17 00:00:00 2001 From: Prateek Sancheti Date: Sat, 7 Aug 2021 13:47:17 +0530 Subject: [PATCH 28/35] qt: Add Load PSBT functionaliy with nowallet --- src/qt/bitcoingui.cpp | 3 +++ src/qt/psbtoperationsdialog.cpp | 33 +++++++++++++++++--------- src/qt/walletframe.cpp | 42 ++++++++++++++++++++++++++++++--- src/qt/walletframe.h | 1 + src/qt/walletview.cpp | 42 --------------------------------- src/qt/walletview.h | 2 -- 6 files changed, 65 insertions(+), 58 deletions(-) diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index 863225099a..fe606519af 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -110,6 +110,9 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty connect(activity, &CreateWalletActivity::finished, activity, &QObject::deleteLater); activity->create(); }); + connect(walletFrame, &WalletFrame::message, [this](const QString& title, const QString& message, unsigned int style) { + this->message(title, message, style); + }); setCentralWidget(walletFrame); } else #endif // ENABLE_WALLET diff --git a/src/qt/psbtoperationsdialog.cpp b/src/qt/psbtoperationsdialog.cpp index 2adfeeaaf0..289fb9f7c8 100644 --- a/src/qt/psbtoperationsdialog.cpp +++ b/src/qt/psbtoperationsdialog.cpp @@ -47,18 +47,22 @@ void PSBTOperationsDialog::openWithPSBT(PartiallySignedTransaction psbtx) { m_transaction_data = psbtx; - bool complete; - size_t n_could_sign; - FinalizePSBT(psbtx); // Make sure all existing signatures are fully combined before checking for completeness. - TransactionError err = m_wallet_model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, &n_could_sign, m_transaction_data, complete); - if (err != TransactionError::OK) { - showStatus(tr("Failed to load transaction: %1") - .arg(QString::fromStdString(TransactionErrorString(err).translated)), StatusLevel::ERR); - return; + bool complete = FinalizePSBT(psbtx); // Make sure all existing signatures are fully combined before checking for completeness. + if (m_wallet_model) { + size_t n_could_sign; + TransactionError err = m_wallet_model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, &n_could_sign, m_transaction_data, complete); + if (err != TransactionError::OK) { + showStatus(tr("Failed to load transaction: %1") + .arg(QString::fromStdString(TransactionErrorString(err).translated)), + StatusLevel::ERR); + return; + } + m_ui->signTransactionButton->setEnabled(!complete && !m_wallet_model->wallet().privateKeysDisabled() && n_could_sign > 0); + } else { + m_ui->signTransactionButton->setEnabled(false); } m_ui->broadcastTransactionButton->setEnabled(complete); - m_ui->signTransactionButton->setEnabled(!complete && !m_wallet_model->wallet().privateKeysDisabled() && n_could_sign > 0); updateTransactionDisplay(); } @@ -133,7 +137,7 @@ void PSBTOperationsDialog::saveTransaction() { } CTxDestination address; ExtractDestination(out.scriptPubKey, address); - QString amount = BitcoinUnits::format(m_wallet_model->getOptionsModel()->getDisplayUnit(), out.nValue); + QString amount = BitcoinUnits::format(m_client_model->getOptionsModel()->getDisplayUnit(), out.nValue); QString address_str = QString::fromStdString(EncodeDestination(address)); filename_suggestion.append(address_str + "-" + amount); first = false; @@ -224,6 +228,10 @@ void PSBTOperationsDialog::showStatus(const QString &msg, StatusLevel level) { } size_t PSBTOperationsDialog::couldSignInputs(const PartiallySignedTransaction &psbtx) { + if (!m_wallet_model) { + return 0; + } + size_t n_signed; bool complete; TransactionError err = m_wallet_model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, false /* bip32derivs */, &n_signed, m_transaction_data, complete); @@ -246,7 +254,10 @@ void PSBTOperationsDialog::showTransactionStatus(const PartiallySignedTransactio case PSBTRole::SIGNER: { QString need_sig_text = tr("Transaction still needs signature(s)."); StatusLevel level = StatusLevel::INFO; - if (m_wallet_model->wallet().privateKeysDisabled()) { + if (!m_wallet_model) { + need_sig_text += " " + tr("(But no wallet is loaded.)"); + level = StatusLevel::WARN; + } else if (m_wallet_model->wallet().privateKeysDisabled()) { need_sig_text += " " + tr("(But this wallet cannot sign transactions.)"); level = StatusLevel::WARN; } else if (n_could_sign < 1) { diff --git a/src/qt/walletframe.cpp b/src/qt/walletframe.cpp index a1f357e0db..3d8bc0c7c5 100644 --- a/src/qt/walletframe.cpp +++ b/src/qt/walletframe.cpp @@ -4,12 +4,18 @@ #include +#include +#include +#include #include +#include #include #include #include +#include +#include #include #include #include @@ -184,10 +190,40 @@ void WalletFrame::gotoVerifyMessageTab(QString addr) void WalletFrame::gotoLoadPSBT(bool from_clipboard) { - WalletView *walletView = currentWalletView(); - if (walletView) { - walletView->gotoLoadPSBT(from_clipboard); + std::string data; + + if (from_clipboard) { + std::string raw = QApplication::clipboard()->text().toStdString(); + bool invalid; + data = DecodeBase64(raw, &invalid); + if (invalid) { + Q_EMIT message(tr("Error"), tr("Unable to decode PSBT from clipboard (invalid base64)"), CClientUIInterface::MSG_ERROR); + return; + } + } else { + QString filename = GUIUtil::getOpenFileName(this, + tr("Load Transaction Data"), QString(), + tr("Partially Signed Transaction (*.psbt)"), nullptr); + if (filename.isEmpty()) return; + if (GetFileSize(filename.toLocal8Bit().data(), MAX_FILE_SIZE_PSBT) == MAX_FILE_SIZE_PSBT) { + Q_EMIT message(tr("Error"), tr("PSBT file must be smaller than 100 MiB"), CClientUIInterface::MSG_ERROR); + return; + } + std::ifstream in(filename.toLocal8Bit().data(), std::ios::binary); + data = std::string(std::istreambuf_iterator{in}, {}); } + + std::string error; + PartiallySignedTransaction psbtx; + if (!DecodeRawPSBT(psbtx, data, error)) { + Q_EMIT message(tr("Error"), tr("Unable to decode PSBT") + "\n" + QString::fromStdString(error), CClientUIInterface::MSG_ERROR); + return; + } + + PSBTOperationsDialog* dlg = new PSBTOperationsDialog(this, currentWalletModel(), clientModel); + dlg->openWithPSBT(psbtx); + dlg->setAttribute(Qt::WA_DeleteOnClose); + dlg->exec(); } void WalletFrame::encryptWallet() diff --git a/src/qt/walletframe.h b/src/qt/walletframe.h index 4f77bd716f..fe42293abc 100644 --- a/src/qt/walletframe.h +++ b/src/qt/walletframe.h @@ -48,6 +48,7 @@ class WalletFrame : public QFrame Q_SIGNALS: void createWalletButtonClicked(); + void message(const QString& title, const QString& message, unsigned int style); private: QStackedWidget *walletStack; diff --git a/src/qt/walletview.cpp b/src/qt/walletview.cpp index 3b8cf4c7ed..2326af80b6 100644 --- a/src/qt/walletview.cpp +++ b/src/qt/walletview.cpp @@ -8,7 +8,6 @@ #include #include #include -#include #include #include #include @@ -21,13 +20,10 @@ #include #include -#include #include #include #include -#include -#include #include #include #include @@ -205,44 +201,6 @@ void WalletView::gotoVerifyMessageTab(QString addr) signVerifyMessageDialog->setAddress_VM(addr); } -void WalletView::gotoLoadPSBT(bool from_clipboard) -{ - std::string data; - - if (from_clipboard) { - std::string raw = QApplication::clipboard()->text().toStdString(); - bool invalid; - data = DecodeBase64(raw, &invalid); - if (invalid) { - Q_EMIT message(tr("Error"), tr("Unable to decode PSBT from clipboard (invalid base64)"), CClientUIInterface::MSG_ERROR); - return; - } - } else { - QString filename = GUIUtil::getOpenFileName(this, - tr("Load Transaction Data"), QString(), - tr("Partially Signed Transaction (*.psbt)"), nullptr); - if (filename.isEmpty()) return; - if (GetFileSize(filename.toLocal8Bit().data(), MAX_FILE_SIZE_PSBT) == MAX_FILE_SIZE_PSBT) { - Q_EMIT message(tr("Error"), tr("PSBT file must be smaller than 100 MiB"), CClientUIInterface::MSG_ERROR); - return; - } - std::ifstream in(filename.toLocal8Bit().data(), std::ios::binary); - data = std::string(std::istreambuf_iterator{in}, {}); - } - - std::string error; - PartiallySignedTransaction psbtx; - if (!DecodeRawPSBT(psbtx, data, error)) { - Q_EMIT message(tr("Error"), tr("Unable to decode PSBT") + "\n" + QString::fromStdString(error), CClientUIInterface::MSG_ERROR); - return; - } - - PSBTOperationsDialog* dlg = new PSBTOperationsDialog(this, walletModel, clientModel); - dlg->openWithPSBT(psbtx); - dlg->setAttribute(Qt::WA_DeleteOnClose); - dlg->exec(); -} - bool WalletView::handlePaymentRequest(const SendCoinsRecipient& recipient) { return sendCoinsPage->handlePaymentRequest(recipient); diff --git a/src/qt/walletview.h b/src/qt/walletview.h index fedf06b710..5c42a9ffc0 100644 --- a/src/qt/walletview.h +++ b/src/qt/walletview.h @@ -83,8 +83,6 @@ public Q_SLOTS: void gotoSignMessageTab(QString addr = ""); /** Show Sign/Verify Message dialog and switch to verify message tab */ void gotoVerifyMessageTab(QString addr = ""); - /** Load Partially Signed Bitcoin Transaction */ - void gotoLoadPSBT(bool from_clipboard = false); /** Show incoming transaction notification for new transactions. From a4d78546b0858602c60c03fdf8b35ca666ab2e56 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 23 Oct 2020 22:03:24 +0100 Subject: [PATCH 29/35] [addrman] Make addrman consistency checks a runtime option Currently addrman consistency checks are a compile time option, and are not enabled in our CI. It's unlikely anyone is running these consistency checks. Make them a runtime option instead, where users can enable addrman consistency checks every n operations (similar to mempool tests). Update the addrman unit tests to do internal consistency checks every 100 operations (checking on every operations causes the test runtime to increase by several seconds). Also assert on a failed addrman consistency check to terminate program execution. --- src/addrman.cpp | 6 ++++-- src/addrman.h | 20 ++++++++++++++------ src/bench/addrman.cpp | 8 ++++---- src/init.cpp | 6 ++++-- src/test/addrman_tests.cpp | 2 +- src/test/fuzz/addrman.cpp | 2 +- src/test/fuzz/connman.cpp | 2 +- src/test/fuzz/data_stream.cpp | 2 +- src/test/fuzz/deserialize.cpp | 2 +- src/test/net_tests.cpp | 10 +++++----- src/test/util/setup_common.cpp | 2 +- 11 files changed, 37 insertions(+), 25 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index c5c6dfbb86..8e2fc67569 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -433,9 +433,12 @@ CAddrInfo CAddrMan::Select_(bool newOnly) const int CAddrMan::Check_() const { -#ifdef DEBUG_ADDRMAN AssertLockHeld(cs); + // Run consistency checks 1 in m_consistency_check_ratio times if enabled + if (m_consistency_check_ratio == 0) return 0; + if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return 0; + std::unordered_set setTried; std::unordered_map mapNew; @@ -514,7 +517,6 @@ int CAddrMan::Check_() const if (nKey.IsNull()) return -16; -#endif // DEBUG_ADDRMAN return 0; } diff --git a/src/addrman.h b/src/addrman.h index 2c6ffbe523..3ee8c3ee09 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -26,6 +26,9 @@ #include #include +/** Default for -checkaddrman */ +static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0}; + /** * Extended statistics about a CAddress */ @@ -124,8 +127,8 @@ class CAddrInfo : public CAddress * attempt was unsuccessful. * * Bucket selection is based on cryptographic hashing, using a randomly-generated 256-bit key, which should not * be observable by adversaries. - * * Several indexes are kept for high performance. Defining DEBUG_ADDRMAN will introduce frequent (and expensive) - * consistency checks for the entire data structure. + * * Several indexes are kept for high performance. Setting m_consistency_check_ratio with the -checkaddrman + * configuration option will introduce (expensive) consistency checks for the entire data structure. */ //! total number of buckets for tried addresses @@ -493,8 +496,9 @@ class CAddrMan mapAddr.clear(); } - explicit CAddrMan(bool deterministic) - : insecure_rand{deterministic} + explicit CAddrMan(bool deterministic, int32_t consistency_check_ratio) + : insecure_rand{deterministic}, + m_consistency_check_ratio{consistency_check_ratio} { Clear(); if (deterministic) nKey = uint256{1}; @@ -700,6 +704,9 @@ class CAddrMan //! Holds addrs inserted into tried table that collide with existing entries. Test-before-evict discipline used to resolve these collisions. std::set m_tried_collisions; + /** Perform consistency checks every m_consistency_check_ratio operations (if non-zero). */ + const int32_t m_consistency_check_ratio; + //! Find an entry. CAddrInfo* Find(const CNetAddr& addr, int *pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs); @@ -737,13 +744,14 @@ class CAddrMan CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs); //! Consistency check - void Check() const - EXCLUSIVE_LOCKS_REQUIRED(cs) + void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs) { AssertLockHeld(cs); + const int err = Check_(); if (err) { LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err); + assert(false); } } diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index e1175e44ec..5ae2dafd5a 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -72,7 +72,7 @@ static void AddrManAdd(benchmark::Bench& bench) { CreateAddresses(); - CAddrMan addrman(/* deterministic */ false); + CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0); bench.run([&] { AddAddressesToAddrMan(addrman); @@ -82,7 +82,7 @@ static void AddrManAdd(benchmark::Bench& bench) static void AddrManSelect(benchmark::Bench& bench) { - CAddrMan addrman(/* deterministic */ false); + CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0); FillAddrMan(addrman); @@ -94,7 +94,7 @@ static void AddrManSelect(benchmark::Bench& bench) static void AddrManGetAddr(benchmark::Bench& bench) { - CAddrMan addrman(/* deterministic */ false); + CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0); FillAddrMan(addrman); @@ -116,7 +116,7 @@ static void AddrManGood(benchmark::Bench& bench) std::vector> addrmans(addrman_count); for (size_t i{0}; i < addrman_count; ++i) { - addrmans[i] = std::make_unique(/* deterministic */ false); + addrmans[i] = std::make_unique(/* deterministic */ false, /* consistency_check_ratio */ 0); FillAddrMan(*addrmans[i]); } diff --git a/src/init.cpp b/src/init.cpp index 2250834111..9154fc0a6f 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -501,7 +501,8 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-checkblocks=", strprintf("How many blocks to check at startup (default: %u, 0 = all)", DEFAULT_CHECKBLOCKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checklevel=", strprintf("How thorough the block verification of -checkblocks is: %s (0-4, default: %u)", Join(CHECKLEVEL_DOC, ", "), DEFAULT_CHECKLEVEL), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checkblockindex", strprintf("Do a consistency check for the block tree, chainstate, and other validation data structures occasionally. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); - argsman.AddArg("-checkmempool=", strprintf("Run checks every transactions (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-checkaddrman=", strprintf("Run addrman consistency checks every operations. Use 0 to disable. (default: %u)", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-checkmempool=", strprintf("Run mempool consistency checks every transactions. Use 0 to disable. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block %s (default: %u)", defaultChainParams->Checkpoints().GetHeight(), DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-deprecatedrpc=", "Allows deprecated RPC method(s) to be used", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-stopafterblockimport", strprintf("Stop running after importing blocks from disk (default: %u)", DEFAULT_STOPAFTERBLOCKIMPORT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); @@ -1164,7 +1165,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)}; assert(!node.addrman); - node.addrman = std::make_unique(/* deterministic */ false); + auto check_addrman = std::clamp(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000); + node.addrman = std::make_unique(/* deterministic */ false, /* consistency_check_ratio */ check_addrman); assert(!node.banman); node.banman = std::make_unique(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME)); assert(!node.connman); diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index b7da1c8896..3c64461605 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -22,7 +22,7 @@ class CAddrManTest : public CAddrMan public: explicit CAddrManTest(bool makeDeterministic = true, std::vector asmap = std::vector()) - : CAddrMan(makeDeterministic) + : CAddrMan(makeDeterministic, /* consistency_check_ratio */ 100) { deterministic = makeDeterministic; m_asmap = asmap; diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index b5e946c528..60fba5730a 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -29,7 +29,7 @@ class CAddrManDeterministic : public CAddrMan FuzzedDataProvider& m_fuzzed_data_provider; explicit CAddrManDeterministic(FuzzedDataProvider& fuzzed_data_provider) - : CAddrMan(/* deterministic */ true) + : CAddrMan(/* deterministic */ true, /* consistency_check_ratio */ 0) , m_fuzzed_data_provider(fuzzed_data_provider) { WITH_LOCK(cs, insecure_rand = FastRandomContext{ConsumeUInt256(fuzzed_data_provider)}); diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index a32cb54465..0e323ddc20 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -25,7 +25,7 @@ FUZZ_TARGET_INIT(connman, initialize_connman) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; SetMockTime(ConsumeTime(fuzzed_data_provider)); - CAddrMan addrman(/* deterministic */ false); + CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0); CConnman connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addrman, fuzzed_data_provider.ConsumeBool()}; CNetAddr random_netaddr; CNode random_node = ConsumeNode(fuzzed_data_provider); diff --git a/src/test/fuzz/data_stream.cpp b/src/test/fuzz/data_stream.cpp index 63bc4b2afe..53400082ab 100644 --- a/src/test/fuzz/data_stream.cpp +++ b/src/test/fuzz/data_stream.cpp @@ -21,6 +21,6 @@ FUZZ_TARGET_INIT(data_stream_addr_man, initialize_data_stream_addr_man) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider); - CAddrMan addr_man(/* deterministic */ false); + CAddrMan addr_man(/* deterministic */ false, /* consistency_check_ratio */ 0); CAddrDB::Read(addr_man, data_stream); } diff --git a/src/test/fuzz/deserialize.cpp b/src/test/fuzz/deserialize.cpp index 6ab3389123..49503e8dc6 100644 --- a/src/test/fuzz/deserialize.cpp +++ b/src/test/fuzz/deserialize.cpp @@ -188,7 +188,7 @@ FUZZ_TARGET_DESERIALIZE(blockmerkleroot, { BlockMerkleRoot(block, &mutated); }) FUZZ_TARGET_DESERIALIZE(addrman_deserialize, { - CAddrMan am(/* deterministic */ false); + CAddrMan am(/* deterministic */ false, /* consistency_check_ratio */ 0); DeserializeFromFuzzingInput(buffer, am); }) FUZZ_TARGET_DESERIALIZE(blockheader_deserialize, { diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index ff9e9b7a07..1915f9c7d5 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -35,7 +35,7 @@ class CAddrManSerializationMock : public CAddrMan virtual void Serialize(CDataStream& s) const = 0; CAddrManSerializationMock() - : CAddrMan(/* deterministic */ true) + : CAddrMan(/* deterministic */ true, /* consistency_check_ratio */ 100) {} }; @@ -119,7 +119,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read) // Test that the de-serialization does not throw an exception. CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted); bool exceptionThrown = false; - CAddrMan addrman1(/* deterministic */ false); + CAddrMan addrman1(/* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman1.size() == 0); try { @@ -136,7 +136,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read) // Test that CAddrDB::Read creates an addrman with the correct number of addrs. CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted); - CAddrMan addrman2(/* deterministic */ false); + CAddrMan addrman2(/* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); BOOST_CHECK(CAddrDB::Read(addrman2, ssPeers2)); BOOST_CHECK(addrman2.size() == 3); @@ -150,7 +150,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted) // Test that the de-serialization of corrupted addrman throws an exception. CDataStream ssPeers1 = AddrmanToStream(addrmanCorrupted); bool exceptionThrown = false; - CAddrMan addrman1(/* deterministic */ false); + CAddrMan addrman1(/* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman1.size() == 0); try { unsigned char pchMsgTmp[4]; @@ -166,7 +166,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted) // Test that CAddrDB::Read leaves addrman in a clean state if de-serialization fails. CDataStream ssPeers2 = AddrmanToStream(addrmanCorrupted); - CAddrMan addrman2(/* deterministic */ false); + CAddrMan addrman2(/* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); BOOST_CHECK(!CAddrDB::Read(addrman2, ssPeers2)); BOOST_CHECK(addrman2.size() == 0); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index b1567d924b..a58f4ba25a 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -193,7 +193,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector(/* deterministic */ false); + m_node.addrman = std::make_unique(/* deterministic */ false, /* consistency_check_ratio */ 0); m_node.banman = std::make_unique(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); m_node.connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests. m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman, From 45babb2788745ff12f6ea72560341875f80684b9 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Thu, 12 Aug 2021 16:15:55 -0400 Subject: [PATCH 30/35] builder-keys: add jamesob http://keyserver.ubuntu.com/pks/lookup?search=0x25F27A38A47AD566&fingerprint=on&hash=on&op=vindex This is also the key I sign GitHub commits with. --- contrib/builder-keys/keys.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/builder-keys/keys.txt b/contrib/builder-keys/keys.txt index db28cd07a0..7be3885ed1 100644 --- a/contrib/builder-keys/keys.txt +++ b/contrib/builder-keys/keys.txt @@ -19,6 +19,7 @@ D35176BE9264832E4ACA8986BF0792FBE95DC863 fivepiece (fivepiece) 01CDF4627A3B88AAE4A571C87588242FBE38D3A8 Gavin Andresen (gavinandresen) D1DBF2C4B96F2DEBF4C16654410108112E7EA81F Hennadii Stepanov (hebasto) A2FD494D0021AA9B4FA58F759102B7AE654A4A5A Ilyas Ridhuan (IlyasRidhuan) +2688F5A9A4BE0F295E921E8A25F27A38A47AD566 James O'Beirne (jamesob) D3F22A3A4C366C2DCB66D3722DA9C5A7FA81EA35 Jarol Rodriguez (jarolrod) 7480909378D544EA6B6DCEB7535B12980BB8A4D3 Jeffri H Frontz (jhfrontz) D3CC177286005BB8FF673294C5242A1AB3936517 jl2012 (jl2012) From 2de222c40198d3d528668d78cc52e2ce3fa96765 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 11 Aug 2021 22:03:56 -0400 Subject: [PATCH 31/35] wallet: Use GetSelectionAmount for target value calculations For target value calculations, GetSelectionAmount should be used, not m_effective_value or m_value. Specifically, ApproximateBestSubset mistakenly uses m_value when calculating whether the target value has been met. This has been changed to use GetSelectionAmount. --- src/wallet/coinselection.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 6d502e1df1..25b1ee07e4 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -195,7 +195,7 @@ static void ApproximateBestSubset(const std::vector& groups, const //the selection random. if (nPass == 0 ? insecure_rand.randbool() : !vfIncluded[i]) { - nTotal += groups[i].m_value; + nTotal += groups[i].GetSelectionAmount(); vfIncluded[i] = true; if (nTotal >= nTargetValue) { @@ -205,7 +205,7 @@ static void ApproximateBestSubset(const std::vector& groups, const nBest = nTotal; vfBest = vfIncluded; } - nTotal -= groups[i].m_value; + nTotal -= groups[i].GetSelectionAmount(); vfIncluded[i] = false; } } From d9262324e80da32725e21d4e0fbfee56f25490b1 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 11 Aug 2021 22:05:51 -0400 Subject: [PATCH 32/35] wallet: Assert that enough was selected to cover the fees When the fee is not subtracted from the outputs, the amount that has been reserved for the fee (change_and_fee - change_amount) must be enough to cover the fee that is needed. It would be a bug to not do so, so use an assert to make this obvious if such a situation were to occur. --- src/wallet/spend.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index cd51ead539..3bb7134b24 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -778,6 +778,10 @@ bool CWallet::CreateTransactionInternal( fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); } + // The only time that fee_needed should be less than the amount available for fees (in change_and_fee - change_amount) is when + // we are subtracting the fee from the outputs. If this occurs at any other time, it is a bug. + assert(coin_selection_params.m_subtract_fee_outputs || fee_needed <= change_and_fee - change_amount); + // Update nFeeRet in case fee_needed changed due to dropping the change output if (fee_needed <= change_and_fee - change_amount) { nFeeRet = change_and_fee - change_amount; From 92885c4f69a5e6fc4989677d6e5be8a666fbff0d Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 11 Aug 2021 22:07:15 -0400 Subject: [PATCH 33/35] test: Test for ApproximateBestSubset edge case with too little fees ApproximateBestSubset had an edge case (due to not using GetSelectionAmount) where it was possible for it to return success but fail to select enough to cover transaction fees. A test is added that could trigger this failure prior to the fix being implemented. --- test/functional/rpc_fundrawtransaction.py | 57 +++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index fa98c44152..2524eaa7a5 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -99,6 +99,7 @@ def run_test(self): self.test_subtract_fee_with_presets() self.test_transaction_too_large() self.test_include_unsafe() + self.test_22670() def test_change_position(self): """Ensure setting changePosition in fundraw with an exact match is handled properly.""" @@ -969,6 +970,62 @@ def test_include_unsafe(self): signedtx = wallet.signrawtransactionwithwallet(fundedtx['hex']) wallet.sendrawtransaction(signedtx['hex']) + def test_22670(self): + # In issue #22670, it was observed that ApproximateBestSubset may + # choose enough value to cover the target amount but not enough to cover the transaction fees. + # This leads to a transaction whose actual transaction feerate is lower than expected. + # However at normal feerates, the difference between the effective value and the real value + # that this bug is not detected because the transaction fee must be at least 0.01 BTC (the minimum change value). + # Otherwise the targeted minimum change value will be enough to cover the transaction fees that were not + # being accounted for. So the minimum relay fee is set to 0.1 BTC/kvB in this test. + self.log.info("Test issue 22670 ApproximateBestSubset bug") + # Make sure the default wallet will not be loaded when restarted with a high minrelaytxfee + self.nodes[0].unloadwallet(self.default_wallet_name, False) + feerate = Decimal("0.1") + self.restart_node(0, [f"-minrelaytxfee={feerate}", "-discardfee=0"]) # Set high minrelayfee, set discardfee to 0 for easier calculation + + self.nodes[0].loadwallet(self.default_wallet_name, True) + funds = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + self.nodes[0].createwallet(wallet_name="tester") + tester = self.nodes[0].get_wallet_rpc("tester") + + # Because this test is specifically for ApproximateBestSubset, the target value must be greater + # than any single input available, and require more than 1 input. So we make 3 outputs + for i in range(0, 3): + funds.sendtoaddress(tester.getnewaddress(address_type="bech32"), 1) + self.nodes[0].generate(1) + + # Create transactions in order to calculate fees for the target bounds that can trigger this bug + change_tx = tester.fundrawtransaction(tester.createrawtransaction([], [{funds.getnewaddress(): 1.5}])) + tx = tester.createrawtransaction([], [{funds.getnewaddress(): 2}]) + no_change_tx = tester.fundrawtransaction(tx, {"subtractFeeFromOutputs": [0]}) + + overhead_fees = feerate * len(tx) / 2 / 1000 + cost_of_change = change_tx["fee"] - no_change_tx["fee"] + fees = no_change_tx["fee"] + assert_greater_than(fees, 0.01) + + def do_fund_send(target): + create_tx = tester.createrawtransaction([], [{funds.getnewaddress(): lower_bound}]) + funded_tx = tester.fundrawtransaction(create_tx) + signed_tx = tester.signrawtransactionwithwallet(funded_tx["hex"]) + assert signed_tx["complete"] + decoded_tx = tester.decoderawtransaction(signed_tx["hex"]) + assert_equal(len(decoded_tx["vin"]), 3) + assert tester.testmempoolaccept([signed_tx["hex"]])[0]["allowed"] + + # We want to choose more value than is available in 2 inputs when considering the fee, + # but not enough to need 3 inputs when not considering the fee. + # So the target value must be at least 2.00000001 - fee. + lower_bound = Decimal("2.00000001") - fees + # The target value must be at most 2 - cost_of_change - not_input_fees - min_change (these are all + # included in the target before ApproximateBestSubset). + upper_bound = Decimal("2.0") - cost_of_change - overhead_fees - Decimal("0.01") + assert_greater_than_or_equal(upper_bound, lower_bound) + do_fund_send(lower_bound) + do_fund_send(upper_bound) + + self.restart_node(0) if __name__ == '__main__': RawTransactionsTest().main() From 4844b74ba73ecc6d336a52b4dc4cd144a01b0ea2 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Fri, 13 Aug 2021 11:25:49 +0200 Subject: [PATCH 34/35] p2p: log addrman consistency checks --- src/addrman.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/addrman.cpp b/src/addrman.cpp index 8e2fc67569..e8f98f727b 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -439,6 +439,8 @@ int CAddrMan::Check_() const if (m_consistency_check_ratio == 0) return 0; if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return 0; + LogPrint(BCLog::ADDRMAN, "Addrman checks started: new %i, tried %i, total %u\n", nNew, nTried, vRandom.size()); + std::unordered_set setTried; std::unordered_map mapNew; @@ -517,6 +519,7 @@ int CAddrMan::Check_() const if (nKey.IsNull()) return -16; + LogPrint(BCLog::ADDRMAN, "Addrman checks completed successfully\n"); return 0; } From 7d95777417709d827e42e87a951fcf29824583b3 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Fri, 13 Aug 2021 15:41:42 -0400 Subject: [PATCH 35/35] builder-keys: Add dongcarl --- contrib/builder-keys/keys.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/builder-keys/keys.txt b/contrib/builder-keys/keys.txt index db28cd07a0..77c28109b8 100644 --- a/contrib/builder-keys/keys.txt +++ b/contrib/builder-keys/keys.txt @@ -5,6 +5,7 @@ E944AE667CF960B1004BC32FCA662BE18B877A60 Andreas Schildbach (aschildbach) 590B7292695AFFA5B672CBB2E13FC145CD3F4304 Antoine Poinsot (darosior) 0AD83877C1F0CD1EE9BD660AD7CC770B81FD22A8 Ben Carman (benthecarman) 912FD3228387123DC97E0E57D5566241A0295FA9 BtcDrak (btcdrak) +04017A2A6D9A0CCDC81D8EC296AB007F1A7ED999 Carl Dong (dongcarl) C519EBCF3B926298946783EFF6430754120EC2F4 Christian Decker (cdecker) 18AE2F798E0D239755DA4FD24B79F986CBDF8736 Chun Kuan Le (ken2812221) 101598DC823C1B5F9A6624ABA5E0907A0380E6C3 CoinForensics (CoinForensics)