From 067ea04869fcf058be37e7db34475df59c0aadb9 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Fri, 18 Nov 2022 16:24:08 -0800 Subject: [PATCH 01/10] Merge bitcoin/bitcoin#17786: refactor: Nuke policy/fees->mempool circular dependencies c8dc0e3eaa9e0f956c5177bcb69632beb0d51770 refactor: Inline `CTxMemPoolEntry` class's functions (Hennadii Stepanov) 75bbe594e54402ed248ecf2bdfe54e8283d20fff refactor: Move `CTxMemPoolEntry` class to its own module (Hennadii Stepanov) Pull request description: This PR: - gets rid of the `policy/fees` -> `txmempool` -> `policy/fees` circular dependency - is an alternative to #13949, which nukes only one circular dependency ACKs for top commit: ryanofsky: Code review ACK c8dc0e3eaa9e0f956c5177bcb69632beb0d51770. Just include and whitespace changes since last review, and there's a moveonly commit now so it's very easy to review theStack: Code-review ACK c8dc0e3eaa9e0f956c5177bcb69632beb0d51770 glozow: utACK c8dc0e3eaa9e0f956c5177bcb69632beb0d51770, agree these changes are an improvement. Tree-SHA512: 36ece824e6ed3ab1a1e198b30a906c8ac12de24545f840eb046958a17315ac9260c7de26e11e2fbab7208adc3d74918db7a7e389444130f8810548ca2e81af41 Co-authored-by: glozow --- src/Makefile.am | 1 + src/bench/mempool_eviction.cpp | 1 + src/bench/mempool_stress.cpp | 1 + src/bench/rpc_mempool.cpp | 1 + src/net_processing.cpp | 1 + src/node/interfaces.cpp | 1 + src/policy/fees.cpp | 2 +- src/rpc/mempool.cpp | 1 + src/test/fuzz/policy_estimator.cpp | 1 + src/test/fuzz/util/mempool.cpp | 2 +- src/test/util/setup_common.cpp | 1 + src/txmempool.cpp | 36 ----- src/txmempool.h | 134 +---------------- src/txmempool_entry.h | 187 ++++++++++++++++++++++++ src/validation.cpp | 1 + test/lint/lint-circular-dependencies.py | 1 - test/sanitizer_suppressions/ubsan | 2 + 17 files changed, 202 insertions(+), 172 deletions(-) create mode 100644 src/txmempool_entry.h diff --git a/src/Makefile.am b/src/Makefile.am index 22f28dbbd102..81f892dbf447 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -416,6 +416,7 @@ BITCOIN_CORE_H = \ torcontrol.h \ txdb.h \ txmempool.h \ + txmempool_entry.h \ txorphanage.h \ txrequest.h \ undo.h \ diff --git a/src/bench/mempool_eviction.cpp b/src/bench/mempool_eviction.cpp index a1fea6f7419f..e2f25466fcdb 100644 --- a/src/bench/mempool_eviction.cpp +++ b/src/bench/mempool_eviction.cpp @@ -6,6 +6,7 @@ #include #include #include +#include static void AddTx(const CTransactionRef& tx, const CAmount& nFee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs) diff --git a/src/bench/mempool_stress.cpp b/src/bench/mempool_stress.cpp index 0cf4e83ef8ce..0081b1983e48 100644 --- a/src/bench/mempool_stress.cpp +++ b/src/bench/mempool_stress.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include diff --git a/src/bench/rpc_mempool.cpp b/src/bench/rpc_mempool.cpp index dbfd187c07b0..7c6c19468677 100644 --- a/src/bench/rpc_mempool.cpp +++ b/src/bench/rpc_mempool.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 740a4e966d27..4edcee4099d2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 4900dcf785af..1b7e0760ec00 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -60,6 +60,7 @@ #include #include #include +#include #include #include #include diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 85bf397ffcbc..6f50dfecff40 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -15,7 +15,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index d433c04f13e7..cba931529978 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp index ca725210cfc9..ac4774010b6d 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include diff --git a/src/test/fuzz/util/mempool.cpp b/src/test/fuzz/util/mempool.cpp index 5041093a868f..3659401f0c37 100644 --- a/src/test/fuzz/util/mempool.cpp +++ b/src/test/fuzz/util/mempool.cpp @@ -8,7 +8,7 @@ #include #include #include -#include +#include #include diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 6355676e9c88..1e3831ab827d 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -49,6 +49,7 @@ #include #include #include +#include #include #include #include diff --git a/src/txmempool.cpp b/src/txmempool.cpp index a7754ea05052..f88d7b33761c 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -50,42 +50,6 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp) return true; } -CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee, - int64_t time, unsigned int entry_height, - bool spends_coinbase, int64_t sigops_count, LockPoints lp) - : tx{tx}, - nFee{fee}, - nTxSize(tx->GetTotalSize()), - nUsageSize{RecursiveDynamicUsage(tx)}, - nTime{time}, - entryHeight{entry_height}, - spendsCoinbase{spends_coinbase}, - sigOpCount{sigops_count}, - m_modified_fee{nFee}, - lockPoints{lp}, - nSizeWithDescendants{GetTxSize()}, - nModFeesWithDescendants{nFee}, - nSizeWithAncestors{GetTxSize()}, - nModFeesWithAncestors{nFee}, - nSigOpCountWithAncestors{sigOpCount} {} - -void CTxMemPoolEntry::UpdateModifiedFee(CAmount newFeeDelta) -{ - nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, newFeeDelta); - nModFeesWithAncestors = SaturatingAdd(nModFeesWithAncestors, newFeeDelta); - m_modified_fee = SaturatingAdd(m_modified_fee, newFeeDelta); -} - -void CTxMemPoolEntry::UpdateLockPoints(const LockPoints& lp) -{ - lockPoints = lp; -} - -size_t CTxMemPoolEntry::GetTxSize() const -{ - return GetVirtualTransactionSize(nTxSize, sigOpCount, ::nBytesPerSigOp); -} - void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendants, const std::set& setExclude, std::set& descendants_to_remove) { diff --git a/src/txmempool.h b/src/txmempool.h index 429a4d2a44ed..6e9eca5164bb 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -30,6 +30,7 @@ #include #include #include +#include #include #include @@ -54,144 +55,11 @@ using CBLSLazyPublicKey = CBLSLazyWrapper; /** Fake height value used in Coin to signify they are only in the memory pool (since 0.8) */ static const uint32_t MEMPOOL_HEIGHT = 0x7FFFFFFF; -struct LockPoints { - // Will be set to the blockchain height and median time past - // values that would be necessary to satisfy all relative locktime - // constraints (BIP68) of this tx given our view of block chain history - int height{0}; - int64_t time{0}; - // As long as the current chain descends from the highest height block - // containing one of the inputs used in the calculation, then the cached - // values are still valid even after a reorg. - CBlockIndex* maxInputBlock{nullptr}; -}; - /** * Test whether the LockPoints height and time are still valid on the current chain */ bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp) EXCLUSIVE_LOCKS_REQUIRED(cs_main); -struct CompareIteratorByHash { - // SFINAE for T where T is either a pointer type (e.g., a txiter) or a reference_wrapper - // (e.g. a wrapped CTxMemPoolEntry&) - template - bool operator()(const std::reference_wrapper& a, const std::reference_wrapper& b) const - { - return a.get().GetTx().GetHash() < b.get().GetTx().GetHash(); - } - template - bool operator()(const T& a, const T& b) const - { - return a->GetTx().GetHash() < b->GetTx().GetHash(); - } -}; -/** \class CTxMemPoolEntry - * - * CTxMemPoolEntry stores data about the corresponding transaction, as well - * as data about all in-mempool transactions that depend on the transaction - * ("descendant" transactions). - * - * When a new entry is added to the mempool, we update the descendant state - * (nCountWithDescendants, nSizeWithDescendants, and nModFeesWithDescendants) for - * all ancestors of the newly added transaction. - * - */ - -class CTxMemPoolEntry -{ -public: - typedef std::reference_wrapper CTxMemPoolEntryRef; - // two aliases, should the types ever diverge - typedef std::set Parents; - typedef std::set Children; - -private: - CTxMemPoolEntry(const CTxMemPoolEntry&) = default; - struct ExplicitCopyTag { - explicit ExplicitCopyTag() = default; - }; - const CTransactionRef tx; - mutable Parents m_parents; - mutable Children m_children; - const CAmount nFee; //!< Cached to avoid expensive parent-transaction lookups - const size_t nTxSize; //!< ... and avoid recomputing tx size - const size_t nUsageSize; //!< ... and total memory usage - const int64_t nTime; //!< Local time when entering the mempool - const unsigned int entryHeight; //!< Chain height when entering the mempool - const bool spendsCoinbase; //!< keep track of transactions that spend a coinbase - const int64_t sigOpCount; //!< Legacy sig ops plus P2SH sig op count - CAmount m_modified_fee; //!< Used for determining the priority of the transaction for mining in a block - LockPoints lockPoints; //!< Track the height and time at which tx was final - - // Information about descendants of this transaction that are in the - // mempool; if we remove this transaction we must remove all of these - // descendants as well. - uint64_t nCountWithDescendants{1}; //!< number of descendant transactions - uint64_t nSizeWithDescendants; //!< ... and size - CAmount nModFeesWithDescendants; //!< ... and total fees (all including us) - - // Analogous statistics for ancestor transactions - uint64_t nCountWithAncestors{1}; - uint64_t nSizeWithAncestors; - CAmount nModFeesWithAncestors; - int64_t nSigOpCountWithAncestors; - -public: - CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee, - int64_t time, unsigned int entry_height, - bool spends_coinbase, - int64_t sigops_count, LockPoints lp); - - CTxMemPoolEntry(ExplicitCopyTag, const CTxMemPoolEntry& entry) : CTxMemPoolEntry(entry) {} - CTxMemPoolEntry& operator=(const CTxMemPoolEntry&) = delete; - CTxMemPoolEntry(CTxMemPoolEntry&&) = delete; - CTxMemPoolEntry& operator=(CTxMemPoolEntry&&) = delete; - - static constexpr ExplicitCopyTag ExplicitCopy{}; - const CTransaction& GetTx() const { return *this->tx; } - CTransactionRef GetSharedTx() const { return this->tx; } - const CAmount& GetFee() const { return nFee; } - size_t GetTxSize() const; - std::chrono::seconds GetTime() const { return std::chrono::seconds{nTime}; } - unsigned int GetHeight() const { return entryHeight; } - int64_t GetSigOpCount() const { return sigOpCount; } - CAmount GetModifiedFee() const { return m_modified_fee; } - size_t DynamicMemoryUsage() const { return nUsageSize; } - const LockPoints& GetLockPoints() const { return lockPoints; } - - // Adjusts the descendant state. - void UpdateDescendantState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount); - // Adjusts the ancestor state - void UpdateAncestorState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps); - // Updates the modified fees with descendants/ancestors. - void UpdateModifiedFee(CAmount newFeeDelta); - // Update the LockPoints after a reorg - void UpdateLockPoints(const LockPoints& lp); - - uint64_t GetCountWithDescendants() const { return nCountWithDescendants; } - uint64_t GetSizeWithDescendants() const { return nSizeWithDescendants; } - CAmount GetModFeesWithDescendants() const { return nModFeesWithDescendants; } - - bool GetSpendsCoinbase() const { return spendsCoinbase; } - - uint64_t GetCountWithAncestors() const { return nCountWithAncestors; } - uint64_t GetSizeWithAncestors() const { return nSizeWithAncestors; } - CAmount GetModFeesWithAncestors() const { return nModFeesWithAncestors; } - int64_t GetSigOpCountWithAncestors() const { return nSigOpCountWithAncestors; } - - const Parents& GetMemPoolParentsConst() const { return m_parents; } - const Children& GetMemPoolChildrenConst() const { return m_children; } - Parents& GetMemPoolParents() const { return m_parents; } - Children& GetMemPoolChildren() const { return m_children; } - - mutable size_t vTxHashesIdx; //!< Index in mempool's vTxHashes - - // If this is a proTx, this will be the hash of the key for which this ProTx was valid - mutable uint256 validForProTxKey; - mutable bool isKeyChangeProTx{false}; - mutable Epoch::Marker m_epoch_marker; //!< epoch when last touched, useful for graph algorithms -}; - // extracts a transaction hash from CTxMemPoolEntry or CTransactionRef struct mempoolentry_txid { diff --git a/src/txmempool_entry.h b/src/txmempool_entry.h new file mode 100644 index 000000000000..bec4e02319bc --- /dev/null +++ b/src/txmempool_entry.h @@ -0,0 +1,187 @@ +// Copyright (c) 2009-2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_TXMEMPOOL_ENTRY_H +#define BITCOIN_TXMEMPOOL_ENTRY_H + +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +class CBlockIndex; + +struct LockPoints { + // Will be set to the blockchain height and median time past + // values that would be necessary to satisfy all relative locktime + // constraints (BIP68) of this tx given our view of block chain history + int height{0}; + int64_t time{0}; + // As long as the current chain descends from the highest height block + // containing one of the inputs used in the calculation, then the cached + // values are still valid even after a reorg. + CBlockIndex* maxInputBlock{nullptr}; +}; + +struct CompareIteratorByHash { + // SFINAE for T where T is either a pointer type (e.g., a txiter) or a reference_wrapper + // (e.g. a wrapped CTxMemPoolEntry&) + template + bool operator()(const std::reference_wrapper& a, const std::reference_wrapper& b) const + { + return a.get().GetTx().GetHash() < b.get().GetTx().GetHash(); + } + template + bool operator()(const T& a, const T& b) const + { + return a->GetTx().GetHash() < b->GetTx().GetHash(); + } +}; + +/** \class CTxMemPoolEntry + * + * CTxMemPoolEntry stores data about the corresponding transaction, as well + * as data about all in-mempool transactions that depend on the transaction + * ("descendant" transactions). + * + * When a new entry is added to the mempool, we update the descendant state + * (nCountWithDescendants, nSizeWithDescendants, and nModFeesWithDescendants) for + * all ancestors of the newly added transaction. + * + */ + +class CTxMemPoolEntry +{ +public: + typedef std::reference_wrapper CTxMemPoolEntryRef; + // two aliases, should the types ever diverge + typedef std::set Parents; + typedef std::set Children; + +private: + CTxMemPoolEntry(const CTxMemPoolEntry&) = default; + struct ExplicitCopyTag { + explicit ExplicitCopyTag() = default; + }; + const CTransactionRef tx; + mutable Parents m_parents; + mutable Children m_children; + const CAmount nFee; //!< Cached to avoid expensive parent-transaction lookups + const size_t nTxSize; //!< ... and avoid recomputing tx size + const size_t nUsageSize; //!< ... and total memory usage + const int64_t nTime; //!< Local time when entering the mempool + const unsigned int entryHeight; //!< Chain height when entering the mempool + const bool spendsCoinbase; //!< keep track of transactions that spend a coinbase + const int64_t sigOpCount; //!< Legacy sig ops plus P2SH sig op count + CAmount m_modified_fee; //!< Used for determining the priority of the transaction for mining in a block + LockPoints lockPoints; //!< Track the height and time at which tx was final + + // Information about descendants of this transaction that are in the + // mempool; if we remove this transaction we must remove all of these + // descendants as well. + uint64_t nCountWithDescendants{1}; //!< number of descendant transactions + uint64_t nSizeWithDescendants; //!< ... and size + CAmount nModFeesWithDescendants; //!< ... and total fees (all including us) + + // Analogous statistics for ancestor transactions + uint64_t nCountWithAncestors{1}; + uint64_t nSizeWithAncestors; + CAmount nModFeesWithAncestors; + int64_t nSigOpCountWithAncestors; + +public: + CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee, + int64_t time, unsigned int entry_height, + bool spends_coinbase, + int64_t sigops_count, LockPoints lp) + : tx{tx}, + nFee{fee}, + nTxSize(tx->GetTotalSize()), + nUsageSize{RecursiveDynamicUsage(tx)}, + nTime{time}, + entryHeight{entry_height}, + spendsCoinbase{spends_coinbase}, + sigOpCount{sigops_count}, + m_modified_fee{nFee}, + lockPoints{lp}, + nSizeWithDescendants{GetTxSize()}, + nModFeesWithDescendants{nFee}, + nSizeWithAncestors{GetTxSize()}, + nModFeesWithAncestors{nFee}, + nSigOpCountWithAncestors{sigOpCount} {} + + CTxMemPoolEntry(ExplicitCopyTag, const CTxMemPoolEntry& entry) : CTxMemPoolEntry(entry) {} + CTxMemPoolEntry& operator=(const CTxMemPoolEntry&) = delete; + CTxMemPoolEntry(CTxMemPoolEntry&&) = delete; + CTxMemPoolEntry& operator=(CTxMemPoolEntry&&) = delete; + + static constexpr ExplicitCopyTag ExplicitCopy{}; + const CTransaction& GetTx() const { return *this->tx; } + CTransactionRef GetSharedTx() const { return this->tx; } + const CAmount& GetFee() const { return nFee; } + size_t GetTxSize() const + { + return GetVirtualTransactionSize(nTxSize, sigOpCount, ::nBytesPerSigOp); + } + std::chrono::seconds GetTime() const { return std::chrono::seconds{nTime}; } + unsigned int GetHeight() const { return entryHeight; } + int64_t GetSigOpCount() const { return sigOpCount; } + CAmount GetModifiedFee() const { return m_modified_fee; } + size_t DynamicMemoryUsage() const { return nUsageSize; } + const LockPoints& GetLockPoints() const { return lockPoints; } + + // Adjusts the descendant state. + void UpdateDescendantState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount); + // Adjusts the ancestor state + void UpdateAncestorState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps); + // Updates the modified fees with descendants/ancestors. + void UpdateModifiedFee(CAmount fee_diff) + { + nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, fee_diff); + nModFeesWithAncestors = SaturatingAdd(nModFeesWithAncestors, fee_diff); + m_modified_fee = SaturatingAdd(m_modified_fee, fee_diff); + } + + // Update the LockPoints after a reorg + void UpdateLockPoints(const LockPoints& lp) + { + lockPoints = lp; + } + + uint64_t GetCountWithDescendants() const { return nCountWithDescendants; } + uint64_t GetSizeWithDescendants() const { return nSizeWithDescendants; } + CAmount GetModFeesWithDescendants() const { return nModFeesWithDescendants; } + + bool GetSpendsCoinbase() const { return spendsCoinbase; } + + uint64_t GetCountWithAncestors() const { return nCountWithAncestors; } + uint64_t GetSizeWithAncestors() const { return nSizeWithAncestors; } + CAmount GetModFeesWithAncestors() const { return nModFeesWithAncestors; } + int64_t GetSigOpCountWithAncestors() const { return nSigOpCountWithAncestors; } + + const Parents& GetMemPoolParentsConst() const { return m_parents; } + const Children& GetMemPoolChildrenConst() const { return m_children; } + Parents& GetMemPoolParents() const { return m_parents; } + Children& GetMemPoolChildren() const { return m_children; } + + mutable size_t vTxHashesIdx; //!< Index in mempool's vTxHashes + + // If this is a proTx, this will be the hash of the key for which this ProTx was valid + mutable uint256 validForProTxKey; + mutable bool isKeyChangeProTx{false}; + mutable Epoch::Marker m_epoch_marker; //!< epoch when last touched, useful for graph algorithms +}; + +#endif // BITCOIN_TXMEMPOOL_ENTRY_H diff --git a/src/validation.cpp b/src/validation.cpp index 9fbd0725b67c..b00e1782be6d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -39,6 +39,7 @@ #include #include #include +#include #include #include #include diff --git a/test/lint/lint-circular-dependencies.py b/test/lint/lint-circular-dependencies.py index 13d3b388c268..6aa244c901a5 100755 --- a/test/lint/lint-circular-dependencies.py +++ b/test/lint/lint-circular-dependencies.py @@ -15,7 +15,6 @@ "chainparamsbase -> util/system -> chainparamsbase", "node/blockstorage -> validation -> node/blockstorage", "node/utxo_snapshot -> validation -> node/utxo_snapshot", - "policy/fees -> txmempool -> policy/fees", "qt/addresstablemodel -> qt/walletmodel -> qt/addresstablemodel", "qt/recentrequeststablemodel -> qt/walletmodel -> qt/recentrequeststablemodel", "qt/transactiontablemodel -> qt/walletmodel -> qt/transactiontablemodel", diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index 5560aada773a..b63bd9d85593 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -59,6 +59,7 @@ unsigned-integer-overflow:policy/fees.cpp unsigned-integer-overflow:prevector.h unsigned-integer-overflow:EvalScript unsigned-integer-overflow:txmempool.cpp +unsigned-integer-overflow:txmempool_entry.h unsigned-integer-overflow:xoroshiro128plusplus.h implicit-integer-sign-change:addrman.h implicit-integer-sign-change:compat/stdin.cpp @@ -72,6 +73,7 @@ implicit-integer-sign-change:script/bitcoinconsensus.cpp implicit-integer-sign-change:script/interpreter.cpp implicit-integer-sign-change:serialize.h implicit-integer-sign-change:txmempool.cpp +implicit-integer-sign-change:txmempool_entry.h implicit-integer-sign-change:util/strencodings.cpp implicit-integer-sign-change:util/strencodings.h implicit-integer-sign-change:validation.cpp From f4573785fb091722366bd13acf75c36575f64c1c Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 21 Nov 2022 11:32:25 +0100 Subject: [PATCH 02/10] Merge bitcoin/bitcoin#26508: RPC/Blockchain: Minor improvements for scanblocks & scantxoutset docs/errors f9869843a664391a493fca4b0ad2828f944cb13a RPC/blockchain: scan{blocks,txoutset>: Further doc improvements (Luke Dashjr) 54b45e155e02cba19975be0bb826ff748d7e895e RPC/Blockchain: Clarify invalid-action error in scanblocks & scantxoutset (Luke Dashjr) Pull request description: * Clarify invalid-action error in scanblocks & scantxoutset * Mention action=='start' only returns after scan completes (already in scantxoutset) * Document `relevant_blocks` ACKs for top commit: kristapsk: utACK f9869843a664391a493fca4b0ad2828f944cb13a aureleoules: ACK f9869843a664391a493fca4b0ad2828f944cb13a MarnixCroes: ACK f9869843a664391a493fca4b0ad2828f944cb13a Tree-SHA512: a37c9cc8a9a2f59376e8d8ed7dbf5e140eb3fefb4b7c19a23fc8190f3aef060bda1f0d5d06dc81cd7dca9e871d65f6c8094bab6e8d42e0bcef0fc7ffd2342d09 Co-authored-by: MacroFake --- src/rpc/blockchain.cpp | 13 +++++++------ test/functional/rpc_scanblocks.py | 2 +- test/functional/rpc_scantxoutset.py | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index ce1257653d10..974697e39e12 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -2726,7 +2726,7 @@ static RPCHelpMan scantxoutset() result.pushKV("unspents", unspents); result.pushKV("total_amount", ValueFromAmount(total_in)); } else { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid command"); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid action '%s'", request.params[0].get_str())); } return result; }, @@ -2775,12 +2775,13 @@ static RPCHelpMan scanblocks() }, { scan_result_status_none, - RPCResult{"When action=='start'", RPCResult::Type::OBJ, "", "", { + RPCResult{"When action=='start'; only returns after scan completes", RPCResult::Type::OBJ, "", "", { {RPCResult::Type::NUM, "from_height", "The height we started the scan from"}, {RPCResult::Type::NUM, "to_height", "The height we ended the scan at"}, - {RPCResult::Type::ARR, "relevant_blocks", "", {{RPCResult::Type::STR_HEX, "blockhash", "A relevant blockhash"},}}, - }, - }, + {RPCResult::Type::ARR, "relevant_blocks", "Blocks that may have matched a scanobject.", { + {RPCResult::Type::STR_HEX, "blockhash", "A relevant blockhash"}, + }}, + }}, RPCResult{"when action=='status' and a scan is currently in progress", RPCResult::Type::OBJ, "", "", { {RPCResult::Type::NUM, "progress", "Approximate percent complete"}, {RPCResult::Type::NUM, "current_height", "Height of the block currently being scanned"}, @@ -2923,7 +2924,7 @@ static RPCHelpMan scanblocks() ret.pushKV("relevant_blocks", blocks); } else { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid command"); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid action '%s'", request.params[0].get_str())); } return ret; }, diff --git a/test/functional/rpc_scanblocks.py b/test/functional/rpc_scanblocks.py index b0e4b169a26a..2bbdd5e19a4e 100755 --- a/test/functional/rpc_scanblocks.py +++ b/test/functional/rpc_scanblocks.py @@ -121,7 +121,7 @@ def run_test(self): assert_equal(node.scanblocks("abort"), False) # test invalid command - assert_raises_rpc_error(-8, "Invalid command", node.scanblocks, "foobar") + assert_raises_rpc_error(-8, "Invalid action 'foobar'", node.scanblocks, "foobar") if __name__ == '__main__': diff --git a/test/functional/rpc_scantxoutset.py b/test/functional/rpc_scantxoutset.py index b5ff9cc5cdd9..7c6b07eedd4b 100755 --- a/test/functional/rpc_scantxoutset.py +++ b/test/functional/rpc_scantxoutset.py @@ -128,7 +128,7 @@ def run_test(self): assert_raises_rpc_error(-1, "scanobjects argument is required for the start action", self.nodes[0].scantxoutset, "start") # Check that invalid command give error - assert_raises_rpc_error(-8, "Invalid command", self.nodes[0].scantxoutset, "invalid_command") + assert_raises_rpc_error(-8, "Invalid action 'invalid_command'", self.nodes[0].scantxoutset, "invalid_command") if __name__ == "__main__": From bab1b8c4753ee2eb684e72b7cb05ae5e07f53c99 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 21 Nov 2022 17:27:28 +0100 Subject: [PATCH 03/10] Merge bitcoin/bitcoin#26545: test: Remove unused sanitizer suppressions fadb71403910cdb91b6173d5dc909d3db72742ba test: Remove unused sanitizer suppressions (MacroFake) Pull request description: ACKs for top commit: fanquake: ACK fadb71403910cdb91b6173d5dc909d3db72742ba hebasto: ACK fadb71403910cdb91b6173d5dc909d3db72742ba Tree-SHA512: 28d4d2eacdbad567434f0f792dbcaa424344dbd13e8686c72f1cbf8a79343b6ee9661246b13ab3f295757269861405315f42180ef49b69411d0b45f9b7796dd7 Co-authored-by: MacroFake --- test/sanitizer_suppressions/ubsan | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index b63bd9d85593..5560aada773a 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -59,7 +59,6 @@ unsigned-integer-overflow:policy/fees.cpp unsigned-integer-overflow:prevector.h unsigned-integer-overflow:EvalScript unsigned-integer-overflow:txmempool.cpp -unsigned-integer-overflow:txmempool_entry.h unsigned-integer-overflow:xoroshiro128plusplus.h implicit-integer-sign-change:addrman.h implicit-integer-sign-change:compat/stdin.cpp @@ -73,7 +72,6 @@ implicit-integer-sign-change:script/bitcoinconsensus.cpp implicit-integer-sign-change:script/interpreter.cpp implicit-integer-sign-change:serialize.h implicit-integer-sign-change:txmempool.cpp -implicit-integer-sign-change:txmempool_entry.h implicit-integer-sign-change:util/strencodings.cpp implicit-integer-sign-change:util/strencodings.h implicit-integer-sign-change:validation.cpp From f84716715b89990c4b8adefd82920a765d626cda Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Fri, 25 Nov 2022 16:41:54 +0000 Subject: [PATCH 04/10] Merge bitcoin/bitcoin#26561: fuzz: Move-only net utils fa3b2cf277394a8db22e92f7ac385db42e60dd20 fuzz: Move-only net utils (MarcoFalke) Pull request description: This should speed up fuzz builds when `src/test/fuzz/util.h` is modified. Also, it makes sense on its own. ACKs for top commit: dergoegge: ACK fa3b2cf277394a8db22e92f7ac385db42e60dd20 Tree-SHA512: 03d6abeb728ac8eb3f28167e8ac43d8d6e7e1b1738ec14f58a36e17502081fdde2d56f2d47a9e11b991754667e83b2eb22d154e394c0c1c4ffa0945db86b7e21 Co-authored-by: fanquake --- ci/dash/lint-tidy.sh | 1 + src/test/fuzz/net.cpp | 1 + src/test/fuzz/net_permissions.cpp | 1 + src/test/fuzz/node_eviction.cpp | 1 + src/test/fuzz/pow.cpp | 1 + src/test/fuzz/process_message.cpp | 1 + src/test/fuzz/process_messages.cpp | 1 + src/test/fuzz/socks5.cpp | 1 + src/test/fuzz/util.cpp | 310 +-------------------------- src/test/fuzz/util.h | 118 ----------- src/test/fuzz/util/mempool.cpp | 2 + src/test/fuzz/util/mempool.h | 8 +- src/test/fuzz/util/net.cpp | 324 ++++++++++++++++++++++++++++- src/test/fuzz/util/net.h | 129 +++++++++++- 14 files changed, 467 insertions(+), 432 deletions(-) diff --git a/ci/dash/lint-tidy.sh b/ci/dash/lint-tidy.sh index 0d2aa9383428..eafe932517e6 100755 --- a/ci/dash/lint-tidy.sh +++ b/ci/dash/lint-tidy.sh @@ -115,6 +115,7 @@ iwyu_tool.py \ "src/rpc/signmessage.cpp" \ "src/test/fuzz/string.cpp" \ "src/test/fuzz/txorphan.cpp" \ + "src/test/fuzz/util/" \ "src/util/bip32.cpp" \ "src/util/bytevectorhash.cpp" \ "src/util/check.cpp" \ diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 0089d0f2e8a2..61f7fca659e7 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include diff --git a/src/test/fuzz/net_permissions.cpp b/src/test/fuzz/net_permissions.cpp index 9d43f6b96f44..ac40bc714a41 100644 --- a/src/test/fuzz/net_permissions.cpp +++ b/src/test/fuzz/net_permissions.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include diff --git a/src/test/fuzz/node_eviction.cpp b/src/test/fuzz/node_eviction.cpp index e27b2545807e..0f204babfa31 100644 --- a/src/test/fuzz/node_eviction.cpp +++ b/src/test/fuzz/node_eviction.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include diff --git a/src/test/fuzz/pow.cpp b/src/test/fuzz/pow.cpp index eadf9328f127..25af251aedf4 100644 --- a/src/test/fuzz/pow.cpp +++ b/src/test/fuzz/pow.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index d142a26a4630..f6fe8ed1caca 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 476e01c2c7a3..e22373f63eff 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include diff --git a/src/test/fuzz/socks5.cpp b/src/test/fuzz/socks5.cpp index 04883aaec85a..8b4b64ea94f4 100644 --- a/src/test/fuzz/socks5.cpp +++ b/src/test/fuzz/socks5.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp index 0a137913a507..a46313ac10b2 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -2,319 +2,16 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include -#include -#include #include #include #include +#include #include #include #include #include -FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider) - : m_fuzzed_data_provider{fuzzed_data_provider}, m_selectable{fuzzed_data_provider.ConsumeBool()} -{ - m_socket = fuzzed_data_provider.ConsumeIntegralInRange(INVALID_SOCKET - 1, INVALID_SOCKET); -} - -FuzzedSock::~FuzzedSock() -{ - // Sock::~Sock() will be called after FuzzedSock::~FuzzedSock() and it will call - // close(m_socket) if m_socket is not INVALID_SOCKET. - // Avoid closing an arbitrary file descriptor (m_socket is just a random very high number which - // theoretically may concide with a real opened file descriptor). - m_socket = INVALID_SOCKET; -} - -FuzzedSock& FuzzedSock::operator=(Sock&& other) -{ - assert(false && "Move of Sock into FuzzedSock not allowed."); - return *this; -} - -ssize_t FuzzedSock::Send(const void* data, size_t len, int flags) const -{ - constexpr std::array send_errnos{ - EACCES, - EAGAIN, - EALREADY, - EBADF, - ECONNRESET, - EDESTADDRREQ, - EFAULT, - EINTR, - EINVAL, - EISCONN, - EMSGSIZE, - ENOBUFS, - ENOMEM, - ENOTCONN, - ENOTSOCK, - EOPNOTSUPP, - EPIPE, - EWOULDBLOCK, - }; - if (m_fuzzed_data_provider.ConsumeBool()) { - return len; - } - const ssize_t r = m_fuzzed_data_provider.ConsumeIntegralInRange(-1, len); - if (r == -1) { - SetFuzzedErrNo(m_fuzzed_data_provider, send_errnos); - } - return r; -} - -ssize_t FuzzedSock::Recv(void* buf, size_t len, int flags) const -{ - // Have a permanent error at recv_errnos[0] because when the fuzzed data is exhausted - // SetFuzzedErrNo() will always return the first element and we want to avoid Recv() - // returning -1 and setting errno to EAGAIN repeatedly. - constexpr std::array recv_errnos{ - ECONNREFUSED, - EAGAIN, - EBADF, - EFAULT, - EINTR, - EINVAL, - ENOMEM, - ENOTCONN, - ENOTSOCK, - EWOULDBLOCK, - }; - assert(buf != nullptr || len == 0); - if (len == 0 || m_fuzzed_data_provider.ConsumeBool()) { - const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; - if (r == -1) { - SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos); - } - return r; - } - std::vector random_bytes; - bool pad_to_len_bytes{m_fuzzed_data_provider.ConsumeBool()}; - if (m_peek_data.has_value()) { - // `MSG_PEEK` was used in the preceding `Recv()` call, return `m_peek_data`. - random_bytes.assign({m_peek_data.value()}); - if ((flags & MSG_PEEK) == 0) { - m_peek_data.reset(); - } - pad_to_len_bytes = false; - } else if ((flags & MSG_PEEK) != 0) { - // New call with `MSG_PEEK`. - random_bytes = m_fuzzed_data_provider.ConsumeBytes(1); - if (!random_bytes.empty()) { - m_peek_data = random_bytes[0]; - pad_to_len_bytes = false; - } - } else { - random_bytes = m_fuzzed_data_provider.ConsumeBytes( - m_fuzzed_data_provider.ConsumeIntegralInRange(0, len)); - } - if (random_bytes.empty()) { - const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; - if (r == -1) { - SetFuzzedErrNo(m_fuzzed_data_provider, recv_errnos); - } - return r; - } - std::memcpy(buf, random_bytes.data(), random_bytes.size()); - if (pad_to_len_bytes) { - if (len > random_bytes.size()) { - std::memset((char*)buf + random_bytes.size(), 0, len - random_bytes.size()); - } - return len; - } - if (m_fuzzed_data_provider.ConsumeBool() && std::getenv("FUZZED_SOCKET_FAKE_LATENCY") != nullptr) { - std::this_thread::sleep_for(std::chrono::milliseconds{2}); - } - return random_bytes.size(); -} - -int FuzzedSock::Connect(const sockaddr*, socklen_t) const -{ - // Have a permanent error at connect_errnos[0] because when the fuzzed data is exhausted - // SetFuzzedErrNo() will always return the first element and we want to avoid Connect() - // returning -1 and setting errno to EAGAIN repeatedly. - constexpr std::array connect_errnos{ - ECONNREFUSED, - EAGAIN, - ECONNRESET, - EHOSTUNREACH, - EINPROGRESS, - EINTR, - ENETUNREACH, - ETIMEDOUT, - }; - if (m_fuzzed_data_provider.ConsumeBool()) { - SetFuzzedErrNo(m_fuzzed_data_provider, connect_errnos); - return -1; - } - return 0; -} - -int FuzzedSock::Bind(const sockaddr*, socklen_t) const -{ - // Have a permanent error at bind_errnos[0] because when the fuzzed data is exhausted - // SetFuzzedErrNo() will always set the global errno to bind_errnos[0]. We want to - // avoid this method returning -1 and setting errno to a temporary error (like EAGAIN) - // repeatedly because proper code should retry on temporary errors, leading to an - // infinite loop. - constexpr std::array bind_errnos{ - EACCES, - EADDRINUSE, - EADDRNOTAVAIL, - EAGAIN, - }; - if (m_fuzzed_data_provider.ConsumeBool()) { - SetFuzzedErrNo(m_fuzzed_data_provider, bind_errnos); - return -1; - } - return 0; -} - -int FuzzedSock::Listen(int) const -{ - // Have a permanent error at listen_errnos[0] because when the fuzzed data is exhausted - // SetFuzzedErrNo() will always set the global errno to listen_errnos[0]. We want to - // avoid this method returning -1 and setting errno to a temporary error (like EAGAIN) - // repeatedly because proper code should retry on temporary errors, leading to an - // infinite loop. - constexpr std::array listen_errnos{ - EADDRINUSE, - EINVAL, - EOPNOTSUPP, - }; - if (m_fuzzed_data_provider.ConsumeBool()) { - SetFuzzedErrNo(m_fuzzed_data_provider, listen_errnos); - return -1; - } - return 0; -} - -std::unique_ptr FuzzedSock::Accept(sockaddr* addr, socklen_t* addr_len) const -{ - constexpr std::array accept_errnos{ - ECONNABORTED, - EINTR, - ENOMEM, - }; - if (m_fuzzed_data_provider.ConsumeBool()) { - SetFuzzedErrNo(m_fuzzed_data_provider, accept_errnos); - return std::unique_ptr(); - } - return std::make_unique(m_fuzzed_data_provider); -} - -int FuzzedSock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const -{ - constexpr std::array getsockopt_errnos{ - ENOMEM, - ENOBUFS, - }; - if (m_fuzzed_data_provider.ConsumeBool()) { - SetFuzzedErrNo(m_fuzzed_data_provider, getsockopt_errnos); - return -1; - } - if (opt_val == nullptr) { - return 0; - } - std::memcpy(opt_val, - ConsumeFixedLengthByteVector(m_fuzzed_data_provider, *opt_len).data(), - *opt_len); - return 0; -} - -int FuzzedSock::SetSockOpt(int, int, const void*, socklen_t) const -{ - constexpr std::array setsockopt_errnos{ - ENOMEM, - ENOBUFS, - }; - if (m_fuzzed_data_provider.ConsumeBool()) { - SetFuzzedErrNo(m_fuzzed_data_provider, setsockopt_errnos); - return -1; - } - return 0; -} - -int FuzzedSock::GetSockName(sockaddr* name, socklen_t* name_len) const -{ - constexpr std::array getsockname_errnos{ - ECONNRESET, - ENOBUFS, - }; - if (m_fuzzed_data_provider.ConsumeBool()) { - SetFuzzedErrNo(m_fuzzed_data_provider, getsockname_errnos); - return -1; - } - *name_len = m_fuzzed_data_provider.ConsumeData(name, *name_len); - return 0; -} - -bool FuzzedSock::SetNonBlocking() const -{ - constexpr std::array setnonblocking_errnos{ - EBADF, - EPERM, - }; - if (m_fuzzed_data_provider.ConsumeBool()) { - SetFuzzedErrNo(m_fuzzed_data_provider, setnonblocking_errnos); - return false; - } - return true; -} - -bool FuzzedSock::IsSelectable(bool is_select) const -{ - return m_selectable; -} - -bool FuzzedSock::Wait(std::chrono::milliseconds timeout, Event requested, SocketEventsParams event_params, Event* occurred) const -{ - constexpr std::array wait_errnos{ - EBADF, - EINTR, - EINVAL, - }; - if (m_fuzzed_data_provider.ConsumeBool()) { - SetFuzzedErrNo(m_fuzzed_data_provider, wait_errnos); - return false; - } - if (occurred != nullptr) { - *occurred = m_fuzzed_data_provider.ConsumeBool() ? requested : 0; - } - return true; -} - -bool FuzzedSock::WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per_sock, SocketEventsParams event_params) const -{ - for (auto& [sock, events] : events_per_sock) { - (void)sock; - events.occurred = m_fuzzed_data_provider.ConsumeBool() ? events.requested : 0; - } - return true; -} - -bool FuzzedSock::IsConnected(std::string& errmsg) const -{ - if (m_fuzzed_data_provider.ConsumeBool()) { - return true; - } - errmsg = "disconnected at random by the fuzzer"; - return false; -} - -void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, CNode& node) noexcept -{ - connman.Handshake(node, - /*successfully_connected=*/fuzzed_data_provider.ConsumeBool(), - /*remote_services=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS), - /*local_services=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS), - /*version=*/fuzzed_data_provider.ConsumeIntegralInRange(MIN_PEER_PROTO_VERSION, std::numeric_limits::max()), - /*relay_txs=*/fuzzed_data_provider.ConsumeBool()); -} std::vector ConstructPubKeyBytes(FuzzedDataProvider& fuzzed_data_provider, Span byte_data, const bool compressed) noexcept { @@ -484,11 +181,6 @@ bool ContainsSpentInput(const CTransaction& tx, const CCoinsViewCache& inputs) n return false; } -CAddress ConsumeAddress(FuzzedDataProvider& fuzzed_data_provider) noexcept -{ - return {ConsumeService(fuzzed_data_provider), ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS), NodeSeconds{std::chrono::seconds{fuzzed_data_provider.ConsumeIntegral()}}}; -} - FILE* FuzzedFileProvider::open() { SetFuzzedErrNo(m_fuzzed_data_provider); diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index 28d656c1fb03..7609ffc81ea6 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -13,9 +13,6 @@ #include #include #include -#include -#include -#include #include #include