From d859c40be92d218202f21dae8b5b4686a8e86589 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 12 Jun 2023 10:32:26 +0100 Subject: [PATCH 1/8] Merge bitcoin/bitcoin#27625: p2p: Stop relaying non-mempool txs BACKPORT NOTE: Version 2 asset unlocks are announced to supporting peers as MSG_ASSET_UNLOCK keyed by GetInstanceHash(), and ProcessGetData passes that hash to FindTxForGetData. mapRelay used to keep the announced hash, so a peer could still fetch the unlock after it was mined. The replacement most-recent-block cache was keyed by txid only, so the same request now returns NOTFOUND once the unlock leaves the mempool. ---- faa2976a56ea7cdfd77ce2580a89ce493b57b5d4 Remove mapRelay (MarcoFalke) fccecd75fed50a59ec4d54d6dc9bd9a406ea6b30 net_processing: relay txs from m_most_recent_block (Anthony Towns) Pull request description: `mapRelay` (used to relay announced transactions that are no longer in the mempool) has issues: * It doesn't have an absolute memory limit, only an implicit one based on the rate of transaction announcements * It doesn't have a use-case EDIT: see below Fix all issues by removing `mapRelay`. For more context, on why a transaction may have been removed from the mempool, see https://github.com/bitcoin/bitcoin/blob/c2f2abd0a4f4bd18bfca41b632d21d803479f3f4/src/txmempool.h#L228-L238 For my rationale on why it is fine to not relay them: Reason | | Rationale -- | -- | -- `EXPIRY` | Expired from mempool | Mempool expiry is by default 2 weeks and can not be less than 1 hour, so a transaction can not be in `mapRelay` while expiring, unless a re-broadcast happened. This should be fine, because the transaction will be re-added to the mempool and potentially announced/relayed on the next re-broadcast. `SIZELIMIT` | Removed in size limiting | A low fee transaction, which will be relayed by a different peer after `GETDATA_TX_INTERVAL` or after we sent a `notfound` message. Assuming it ever made it to another peer, otherwise it will happen on re-broadcast (same as with `EXPIRY` above). `REORG` | Removed for reorganization | Block races are rare, so reorgs should be rarer. Also, the transaction is likely to be re-accepted via the `disconnectpool` later on. If not, it seems fine to let the originating wallet deal with rebroadcast in this case. `BLOCK` | Removed for block | EDIT: Needed for compact block relay, see https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544047433 `CONFLICT` | Removed for conflict with in-block transaction | The peer won't be able to add the tx to the mempool anyway, unless it is on a different block, in which case it seems fine to let the originating wallet take care of the rebroadcast (if needed). `REPLACED` | Removed for replacement | EDIT: Also needed for compact block relay, see https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544171255 ? ACKs for top commit: sdaftuar: ACK faa2976a56ea7cdfd77ce2580a89ce493b57b5d4 ajtowns: ACK faa2976a56ea7cdfd77ce2580a89ce493b57b5d4 glozow: code review ACK faa2976a56ea7cdfd77ce2580a89ce493b57b5d4 Tree-SHA512: 64ae3e387b001bf6bd5b6c938e7317f4361f9bc0b8cc5d8f63a16cda2408d2f634a22f8157dfcd8957502ef358208292ec91e7d70c9c2d8a8c47cc0114ecfebd Co-authored-by: fanquake --- src/net_processing.cpp | 54 +++++++++++++++------------------- test/functional/p2p_leak_tx.py | 1 + 2 files changed, 25 insertions(+), 30 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9002e875603e..10ae4ebe2874 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -113,9 +113,7 @@ static constexpr auto GETDATA_TX_INTERVAL{60s}; /** Limit to avoid sending big packets. Not used in processing incoming GETDATA for compatibility */ static const unsigned int MAX_GETDATA_SZ = 1000; -/** How long to cache transactions in mapRelay for normal relay */ -static constexpr auto RELAY_TX_CACHE_TIME = 15min; -/** How long a transaction has to be in the mempool before it can unconditionally be relayed (even when not in mapRelay). */ +/** How long a transaction has to be in the mempool before it can unconditionally be relayed. */ static constexpr auto UNCONDITIONAL_RELAY_DELAY = 2min; /** Headers download timeout. * Timeout = base + per_header * (expected number of headers) */ @@ -1076,6 +1074,7 @@ class PeerManagerImpl final : public PeerManager std::shared_ptr m_most_recent_block GUARDED_BY(m_most_recent_block_mutex); std::shared_ptr m_most_recent_compact_block GUARDED_BY(m_most_recent_block_mutex); uint256 m_most_recent_block_hash GUARDED_BY(m_most_recent_block_mutex); + std::unique_ptr> m_most_recent_block_txs GUARDED_BY(m_most_recent_block_mutex); /** Height of the highest block announced using BIP 152 high-bandwidth mode. */ int m_highest_fast_announce GUARDED_BY(::cs_main){0}; @@ -1117,7 +1116,7 @@ class PeerManagerImpl final : public PeerManager /** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */ CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) - EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex); + EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, NetEventsInterface::g_msgproc_mutex); void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, peer.m_getdata_requests_mutex, NetEventsInterface::g_msgproc_mutex) @@ -1130,12 +1129,6 @@ class PeerManagerImpl final : public PeerManager void ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const BlockTransactions& block_transactions) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex, !m_most_recent_block_mutex); - /** Relay map (txid -> CTransactionRef) */ - typedef std::map MapRelay; - MapRelay mapRelay GUARDED_BY(NetEventsInterface::g_msgproc_mutex); - /** Expiration-time ordered list of (expire time, relay map entry) pairs. */ - std::deque> g_relay_expiration GUARDED_BY(NetEventsInterface::g_msgproc_mutex); - /** * When a peer sends us a valid block, instruct it to announce blocks to us * using CMPCTBLOCK if possible by adding its nodeid to the end of @@ -2306,10 +2299,20 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha std::async(std::launch::deferred, [&] { return msgMaker.Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })}; { + auto most_recent_block_txs = std::make_unique>(); + for (const auto& tx : pblock->vtx) { + most_recent_block_txs->emplace(tx->GetHash(), tx); + // A MSG_ASSET_UNLOCK getdata identifies a version 2 asset unlock by its instance hash + if (IsAssetUnlockWithStableTxid(*tx)) { + most_recent_block_txs->emplace(tx->GetInstanceHash(), tx); + } + } + LOCK(m_most_recent_block_mutex); m_most_recent_block_hash = hashBlock; m_most_recent_block = pblock; m_most_recent_compact_block = pcmpctblock; + m_most_recent_block_txs = std::move(most_recent_block_txs); } m_connman.ForEachNode([this, pindex, &lazy_ser, &hashBlock](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { @@ -2987,13 +2990,17 @@ CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, } } - // Otherwise, the transaction must have been announced recently. - if (tx_relay.m_recently_announced_invs.contains(txid)) { - // If it was, it can be relayed from either the mempool... - if (txinfo.tx) return std::move(txinfo.tx); - // ... or the relay pool. - auto mi = mapRelay.find(txid); - if (mi != mapRelay.end()) return mi->second; + // Otherwise, the transaction might have been announced recently. + bool recent = tx_relay.m_recently_announced_invs.contains(txid); + if (recent && txinfo.tx) return std::move(txinfo.tx); + + // Or it might be from the most recent block + { + LOCK(m_most_recent_block_mutex); + if (m_most_recent_block_txs != nullptr) { + auto it = m_most_recent_block_txs->find(txid); + if (it != m_most_recent_block_txs->end()) return it->second; + } } return {}; @@ -6832,19 +6839,6 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // Send tx_relay->m_recently_announced_invs.insert(announce_hash); nRelayedTransactions++; - { - // Expire old relay messages - while (!g_relay_expiration.empty() && g_relay_expiration.front().first < current_time) - { - mapRelay.erase(g_relay_expiration.front().second); - g_relay_expiration.pop_front(); - } - - auto ret = mapRelay.emplace(announce_hash, std::move(txinfo.tx)); - if (ret.second) { - g_relay_expiration.emplace_back(current_time + RELAY_TX_CACHE_TIME, ret.first); - } - } tx_relay->m_tx_inventory_known_filter.insert(announce_hash); queueAndMaybePushInv(CInv(nInvType, announce_hash)); } diff --git a/test/functional/p2p_leak_tx.py b/test/functional/p2p_leak_tx.py index 69b6246c30c2..65a6925a8db8 100755 --- a/test/functional/p2p_leak_tx.py +++ b/test/functional/p2p_leak_tx.py @@ -48,6 +48,7 @@ def test_tx_in_block(self): def test_notfound_on_unannounced_tx(self): self.log.info("Check that we don't leak txs to inbound peers that we haven't yet announced to") + self.gen_node.disconnect_p2ps() inbound_peer = self.gen_node.add_p2p_connection(P2PNode()) # An "attacking" inbound peer MAX_REPEATS = 100 From d310b1a81d286863bcc5122517b3047a0b03c5d2 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 13 Jun 2023 10:23:51 -0400 Subject: [PATCH 2/8] Merge bitcoin/bitcoin#23962: Use `int32_t` type for most transaction size/weight values 3ef756a5b558a1dd2fcb93bc0d4237707aa04f3f Remove txmempool implicit-integer-sign-change sanitizer suppressions (Hennadii Stepanov) d2f6d2a95a9f6c1632c1ed3b5b5b67a49eb71d6b Use `int32_t` type for most transaction size/weight values (Hennadii Stepanov) Pull request description: From bitcoin/bitcoin#23957 which has been incorporated into this PR: > A file-wide suppression is problematic because it will wave through future violations, potentially bugs. > > Fix that by using per-statement casts. > > This refactor doesn't change behavior because the now explicit casts were previously done implicitly. > > Similar to commit 8b5a4de904b414fb3a818732cd0a2c90b91bc275 ACKs for top commit: achow101: ACK 3ef756a5b558a1dd2fcb93bc0d4237707aa04f3f 0xB10C: ACK 3ef756a5b558a1dd2fcb93bc0d4237707aa04f3f. I've focused my testing and code review on the tracepoint related changes. The docs, the test, and the mempool_monitor.py demo script are updated. I ran the `interface_usdt_mempool.py` test and the `mempool_monitor.py` script. The `mempool_monitor.py` output looks correct. Xekyo: codereview ACK 3ef756a5b558a1dd2fcb93bc0d4237707aa04f3f ryanofsky: Code review ACK 3ef756a5b558a1dd2fcb93bc0d4237707aa04f3f. Since last review, just rebased with more type changes in test and tracing code Tree-SHA512: 397407f72165b6fb85ff1794eb1447836c4f903efed1a05d7a9704c88aa9b86f330063964370bbd59f6b5e322e04e7ea8e467805d58dce381e68f7596433330f Co-authored-by: Andrew Chow --- contrib/tracing/mempool_monitor.py | 4 ++-- doc/tracing.md | 4 ++-- src/kernel/mempool_entry.h | 18 +++++++++------- src/txmempool.cpp | 26 +++++++++++------------ src/txmempool.h | 2 +- test/functional/interface_usdt_mempool.py | 4 ++-- test/sanitizer_suppressions/ubsan | 1 - 7 files changed, 30 insertions(+), 29 deletions(-) diff --git a/contrib/tracing/mempool_monitor.py b/contrib/tracing/mempool_monitor.py index 7e8790dd233f..79a89b7e1eb3 100755 --- a/contrib/tracing/mempool_monitor.py +++ b/contrib/tracing/mempool_monitor.py @@ -27,7 +27,7 @@ struct added_event { u8 hash[HASH_LENGTH]; - u64 vsize; + s32 vsize; s64 fee; }; @@ -35,7 +35,7 @@ { u8 hash[HASH_LENGTH]; char reason[MAX_REMOVAL_REASON_LENGTH]; - u64 vsize; + s32 vsize; s64 fee; u64 entry_time; }; diff --git a/doc/tracing.md b/doc/tracing.md index 16ee467ad9de..d7a1139cac88 100644 --- a/doc/tracing.md +++ b/doc/tracing.md @@ -220,7 +220,7 @@ about the transaction. Arguments passed: 1. Transaction ID (hash) as `pointer to unsigned chars` (i.e. 32 bytes in little-endian) -2. Transaction virtual size as `uint64` +2. Transaction virtual size as `int32` 3. Transaction fee as `int64` #### Tracepoint `mempool:removed` @@ -231,7 +231,7 @@ about the transaction. Arguments passed: 1. Transaction ID (hash) as `pointer to unsigned chars` (i.e. 32 bytes in little-endian) 2. Removal reason as `pointer to C-style String` (max. length 9 characters) -3. Transaction virtual size as `uint64` +3. Transaction virtual size as `int32` 4. Transaction fee as `int64` 5. Transaction mempool entry time (epoch) as `uint64` diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h index 8465eef35b26..ada944b16a6b 100644 --- a/src/kernel/mempool_entry.h +++ b/src/kernel/mempool_entry.h @@ -80,7 +80,7 @@ class CTxMemPoolEntry 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 int32_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 @@ -93,12 +93,14 @@ class CTxMemPoolEntry // 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 + // Using int64_t instead of int32_t to avoid signed integer overflow issues. + int64_t nSizeWithDescendants; //!< ... and size CAmount nModFeesWithDescendants; //!< ... and total fees (all including us) // Analogous statistics for ancestor transactions uint64_t nCountWithAncestors{1}; - uint64_t nSizeWithAncestors; + // Using int64_t instead of int32_t to avoid signed integer overflow issues. + int64_t nSizeWithAncestors; CAmount nModFeesWithAncestors; int64_t nSigOpCountWithAncestors; @@ -137,7 +139,7 @@ class CTxMemPoolEntry tx = std::move(new_tx); } const CAmount& GetFee() const { return nFee; } - size_t GetTxSize() const + int32_t GetTxSize() const { return GetVirtualTransactionSize(nTxSize, sigOpCount, ::nBytesPerSigOp); } @@ -149,9 +151,9 @@ class CTxMemPoolEntry const LockPoints& GetLockPoints() const { return lockPoints; } // Adjusts the descendant state. - void UpdateDescendantState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount); + void UpdateDescendantState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount); // Adjusts the ancestor state - void UpdateAncestorState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps); + void UpdateAncestorState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps); // Updates the modified fees with descendants/ancestors. void UpdateModifiedFee(CAmount fee_diff) { @@ -167,13 +169,13 @@ class CTxMemPoolEntry } uint64_t GetCountWithDescendants() const { return nCountWithDescendants; } - uint64_t GetSizeWithDescendants() const { return nSizeWithDescendants; } + int64_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; } + int64_t GetSizeWithAncestors() const { return nSizeWithAncestors; } CAmount GetModFeesWithAncestors() const { return nModFeesWithAncestors; } int64_t GetSigOpCountWithAncestors() const { return nSigOpCountWithAncestors; } diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 26eb67e7b6e4..af6d15dee4d7 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -83,7 +83,7 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendan } // descendants now contains all in-mempool descendants of updateIt. // Update and add to cached descendant map - int64_t modifySize = 0; + int32_t modifySize = 0; CAmount modifyFee = 0; int64_t modifyCount = 0; for (const CTxMemPoolEntry& descendant : descendants) { @@ -99,7 +99,7 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendan // Don't directly remove the transaction here -- doing so would // invalidate iterators in cachedDescendants. Mark it for removal // by inserting into descendants_to_remove. - if (descendant.GetCountWithAncestors() > uint64_t(m_limits.ancestor_count) || descendant.GetSizeWithAncestors() > uint64_t(m_limits.ancestor_size_vbytes)) { + if (descendant.GetCountWithAncestors() > uint64_t(m_limits.ancestor_count) || descendant.GetSizeWithAncestors() > m_limits.ancestor_size_vbytes) { descendants_to_remove.insert(descendant.GetTx().GetHash()); } } @@ -284,8 +284,8 @@ void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors for (const CTxMemPoolEntry& parent : parents) { UpdateChild(mapTx.iterator_to(parent), it, add); } - const int64_t updateCount = (add ? 1 : -1); - const int64_t updateSize = updateCount * it->GetTxSize(); + const int32_t updateCount = (add ? 1 : -1); + const int32_t updateSize{updateCount * it->GetTxSize()}; const CAmount updateFee = updateCount * it->GetModifiedFee(); for (txiter ancestorIt : setAncestors) { mapTx.modify(ancestorIt, [=](CTxMemPoolEntry& e) { e.UpdateDescendantState(updateSize, updateFee, updateCount); }); @@ -329,7 +329,7 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b setEntries setDescendants; CalculateDescendants(removeIt, setDescendants); setDescendants.erase(removeIt); // don't update state for self - int64_t modifySize = -((int64_t)removeIt->GetTxSize()); + int32_t modifySize = -removeIt->GetTxSize(); CAmount modifyFee = -removeIt->GetModifiedFee(); int modifySigOps = -removeIt->GetSigOpCount(); for (txiter dit : setDescendants) { @@ -371,21 +371,21 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b } } -void CTxMemPoolEntry::UpdateDescendantState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount) +void CTxMemPoolEntry::UpdateDescendantState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount) { nSizeWithDescendants += modifySize; - assert(int64_t(nSizeWithDescendants) > 0); + assert(nSizeWithDescendants > 0); nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, modifyFee); - nCountWithDescendants += modifyCount; + nCountWithDescendants += uint64_t(modifyCount); assert(int64_t(nCountWithDescendants) > 0); } -void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps) +void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps) { nSizeWithAncestors += modifySize; - assert(int64_t(nSizeWithAncestors) > 0); + assert(nSizeWithAncestors > 0); nModFeesWithAncestors = SaturatingAdd(nModFeesWithAncestors, modifyFee); - nCountWithAncestors += modifyCount; + nCountWithAncestors += uint64_t(modifyCount); assert(int64_t(nCountWithAncestors) > 0); nSigOpCountWithAncestors += modifySigOps; assert(int(nSigOpCountWithAncestors) >= 0); @@ -1449,7 +1449,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei // Verify ancestor state is correct. auto ancestors{AssumeCalculateMemPoolAncestors(__func__, *it, Limits::NoLimits())}; uint64_t nCountCheck = ancestors.size() + 1; - uint64_t nSizeCheck = it->GetTxSize(); + int32_t nSizeCheck = it->GetTxSize(); CAmount nFeesCheck = it->GetModifiedFee(); unsigned int nSigOpCheck = it->GetSigOpCount(); @@ -1470,7 +1470,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei // Check children against mapNextTx CTxMemPoolEntry::Children setChildrenCheck; auto iter = mapNextTx.lower_bound(COutPoint(it->GetTx().GetHash(), 0)); - uint64_t child_sizes = 0; + int32_t child_sizes{0}; for (; iter != mapNextTx.end() && iter->first->hash == it->GetTx().GetHash(); ++iter) { txiter childit = mapTx.find(iter->second->GetHash()); assert(childit != mapTx.end()); // mapNextTx points to in-mempool transactions diff --git a/src/txmempool.h b/src/txmempool.h index c2fea79b0c7f..212531a59f33 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -222,7 +222,7 @@ struct TxMempoolInfo CAmount fee; /** Virtual size of the transaction. */ - size_t vsize; + int32_t vsize; /** The fee delta. */ int64_t nFeeDelta; diff --git a/test/functional/interface_usdt_mempool.py b/test/functional/interface_usdt_mempool.py index 802153099f35..7b28a844c9f2 100755 --- a/test/functional/interface_usdt_mempool.py +++ b/test/functional/interface_usdt_mempool.py @@ -35,7 +35,7 @@ struct added_event { u8 hash[HASH_LENGTH]; - u64 vsize; + s32 vsize; s64 fee; }; @@ -43,7 +43,7 @@ { u8 hash[HASH_LENGTH]; char reason[MAX_REMOVAL_REASON_LENGTH]; - u64 vsize; + s32 vsize; s64 fee; u64 entry_time; }; diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index c20a58eec210..255177987004 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -76,7 +76,6 @@ implicit-integer-sign-change:prevector.h 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:util/strencodings.cpp implicit-integer-sign-change:util/strencodings.h implicit-integer-sign-change:validation.cpp From c8151fbc983de0280ba4608ecbf056760b878817 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 20 Jun 2023 21:34:34 +0100 Subject: [PATCH 3/8] Merge bitcoin/bitcoin#27890: refactor: Make m_count_with_* in CTxMemPoolEntry int64_t, drop UBSAN supp fa76f0d0efccd1ea272a46060022eea3e998268e refactor: Make m_count_with_* in CTxMemPoolEntry int64_t, drop UBSAN supp (MarcoFalke) Pull request description: This is a refactor as long as no signed integer overflow appears. In normal operation and absent bugs, signed integer overflow should never happen in the touched code paths. The main benefit of this refactor is to drop the file-wide ubsan suppression `unsigned-integer-overflow:txmempool.cpp`. For now, this only changes the internal private representation and the publicly returned type remains `uint64_t`. ACKs for top commit: glozow: ACK fa76f0d0ef ryanofsky: Code review ACK fa76f0d0efccd1ea272a46060022eea3e998268e Tree-SHA512: a09e33a915d60c65d369d44ba1a45ce4a6a76e6dc2bea43216ba02b5eab0b74e214b2c7cc44360493f2c483d18d96e4636b7a75b23050976efc80e38de852c39 Co-authored-by: glozow --- src/kernel/mempool_entry.h | 10 +++++----- src/txmempool.cpp | 8 ++++---- test/sanitizer_suppressions/ubsan | 1 - 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h index ada944b16a6b..a44d9de9f7b4 100644 --- a/src/kernel/mempool_entry.h +++ b/src/kernel/mempool_entry.h @@ -58,7 +58,7 @@ struct CompareIteratorByHash { * ("descendant" transactions). * * When a new entry is added to the mempool, we update the descendant state - * (nCountWithDescendants, nSizeWithDescendants, and nModFeesWithDescendants) for + * (m_count_with_descendants, nSizeWithDescendants, and nModFeesWithDescendants) for * all ancestors of the newly added transaction. * */ @@ -92,13 +92,13 @@ class CTxMemPoolEntry // 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 + int64_t m_count_with_descendants{1}; //!< number of descendant transactions // Using int64_t instead of int32_t to avoid signed integer overflow issues. int64_t nSizeWithDescendants; //!< ... and size CAmount nModFeesWithDescendants; //!< ... and total fees (all including us) // Analogous statistics for ancestor transactions - uint64_t nCountWithAncestors{1}; + int64_t m_count_with_ancestors{1}; // Using int64_t instead of int32_t to avoid signed integer overflow issues. int64_t nSizeWithAncestors; CAmount nModFeesWithAncestors; @@ -168,13 +168,13 @@ class CTxMemPoolEntry lockPoints = lp; } - uint64_t GetCountWithDescendants() const { return nCountWithDescendants; } + uint64_t GetCountWithDescendants() const { return m_count_with_descendants; } int64_t GetSizeWithDescendants() const { return nSizeWithDescendants; } CAmount GetModFeesWithDescendants() const { return nModFeesWithDescendants; } bool GetSpendsCoinbase() const { return spendsCoinbase; } - uint64_t GetCountWithAncestors() const { return nCountWithAncestors; } + uint64_t GetCountWithAncestors() const { return m_count_with_ancestors; } int64_t GetSizeWithAncestors() const { return nSizeWithAncestors; } CAmount GetModFeesWithAncestors() const { return nModFeesWithAncestors; } int64_t GetSigOpCountWithAncestors() const { return nSigOpCountWithAncestors; } diff --git a/src/txmempool.cpp b/src/txmempool.cpp index af6d15dee4d7..3f9323a22784 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -376,8 +376,8 @@ void CTxMemPoolEntry::UpdateDescendantState(int32_t modifySize, CAmount modifyFe nSizeWithDescendants += modifySize; assert(nSizeWithDescendants > 0); nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, modifyFee); - nCountWithDescendants += uint64_t(modifyCount); - assert(int64_t(nCountWithDescendants) > 0); + m_count_with_descendants += modifyCount; + assert(m_count_with_descendants > 0); } void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps) @@ -385,8 +385,8 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee, nSizeWithAncestors += modifySize; assert(nSizeWithAncestors > 0); nModFeesWithAncestors = SaturatingAdd(nModFeesWithAncestors, modifyFee); - nCountWithAncestors += uint64_t(modifyCount); - assert(int64_t(nCountWithAncestors) > 0); + m_count_with_ancestors += modifyCount; + assert(m_count_with_ancestors > 0); nSigOpCountWithAncestors += modifySigOps; assert(int(nSigOpCountWithAncestors) >= 0); } diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index 255177987004..518a7447034f 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -62,7 +62,6 @@ unsigned-integer-overflow:GetQuorumQuarterMembersBySnapshot unsigned-integer-overflow:policy/fees.cpp unsigned-integer-overflow:prevector.h unsigned-integer-overflow:EvalScript -unsigned-integer-overflow:txmempool.cpp unsigned-integer-overflow:xoroshiro128plusplus.h unsigned-integer-overflow:bitset_detail::PopCount implicit-integer-sign-change:addrman.h From 9edb6d932747699cfdc7d939aec6eccdbb18b313 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 22 Jun 2023 15:43:18 +0100 Subject: [PATCH 4/8] Merge bitcoin/bitcoin#27831: test: handle failed `assert_equal()` assertions in bcc callback functions 61f4b9b7ad6e992a9dbbbb091e9b7ba9abe529ac Manage exceptions in bcc callback functions (virtu) Pull request description: Address #27380 (and similar future issues) by handling failed `assert_equal()` assertions in bcc callback functions ### Problem Exceptions are not propagated in ctype callback functions used by bcc. This means an AssertionError exception raised by `assert_equal()` to signal a failed assertion is not getting caught and properly logged. Instead, the error is logged to stdout and execution of the callback stops. The current workaround to check whether all `assert_equal()` assertions in a callback succeeded is to increment a success counter after the assertions (which only gets incremented if none exception is raised and stops execution). Then, outside the callback, the success counter can be used to check whether a callback executed successfully. One issue with the described workaround is that when an exception occurs, there is no way of telling which of the `assert_equal()` statements caused the exception; moreover, there is no way of inspecting how the pieces of data that got compared in `assert_equal()` differed (often a crucial clue when debugging what went wrong). This problem is happening in #27380: Sporadically, in the `mempool:rejected` test, execution does not reach the end of the callback function and the success counter is not incremented. Thus, the test fails when comparing the counter to its expected value of one. Without knowing which of the asserts failed any why it failed, this issue is hard to debug. ### Solution Two fixes come to mind. The first involves having the callback function make event data accessible outside the callback and inspecting the event using `assert_equal()` outside the callback. This solution still requires a counter in the callback in order to tell whether a callback was actually executed or if instead the call to perf_buffer_poll() timed out. The second fix entails wrapping all relevant `assert_equal()` statements inside callback functions into try-catch blocks and manually logging AssertionErrors. While not as elegant in terms of design, this approach can be more pragmatic for more complex tests (e.g., ones involving multiple events, events of different types, or the order of events). The solution proposed here is to select the most pragmatic fix on a case-by-case basis: Tests in `interface_usdt_net.py`, `interface_usdt_mempool.py` and `interface_usdt_validation.py` have been refactored to use the first approach, while the second approach was chosen for `interface_usdt_utxocache.py` (partly to provide a reference for the second approach, but mainly because the utxocache tests are the most intricate tests, and refactoring them to use the first approach would negatively impact their readability). Lastly, `interface_usdt_coinselection.py` was kept unchanged because it does not use `assert_equal()` statements inside callback functions. ACKs for top commit: 0xB10C: Reviewed the changes since my last review. ACK 61f4b9b7ad6e992a9dbbbb091e9b7ba9abe529ac. I've tested that the combined log contains both exceptions by modifying `interface_usdt_utxocache.py`. willcl-ark: utACK 61f4b9b stickies-v: utACK 61f4b9b7a Tree-SHA512: 85cdaabf370d4f09a9eab6af9ce7c796cd9d08cb91f38f021f71adda34c5f643331022dd09cadb95be2185dad6016c95cbb8942e41e4fbd566a49bf431c5141a Co-authored-by: fanquake --- test/functional/interface_usdt_mempool.py | 51 +++++++++----------- test/functional/interface_usdt_validation.py | 11 +---- 2 files changed, 23 insertions(+), 39 deletions(-) diff --git a/test/functional/interface_usdt_mempool.py b/test/functional/interface_usdt_mempool.py index 7b28a844c9f2..199de10dae40 100755 --- a/test/functional/interface_usdt_mempool.py +++ b/test/functional/interface_usdt_mempool.py @@ -113,6 +113,7 @@ def added_test(self): EXPECTED_ADDED_EVENTS = 1 handled_added_events = 0 + event = None self.log.info("Hooking into mempool:added tracepoint...") node = self.nodes[0] @@ -121,11 +122,8 @@ def added_test(self): bpf = BPF(text=MEMPOOL_TRACEPOINTS_PROGRAM, usdt_contexts=[ctx], debug=0) def handle_added_event(_, data, __): - nonlocal handled_added_events + nonlocal event, handled_added_events event = bpf["added_events"].event(data) - assert_equal(txid, bytes(event.hash)[::-1].hex()) - assert_equal(vsize, event.vsize) - assert_equal(fee, event.fee) handled_added_events += 1 bpf["added_events"].open_perf_buffer(handle_added_event) @@ -133,9 +131,6 @@ def handle_added_event(_, data, __): self.log.info("Sending transaction...") fee = Decimal(31200) tx = self.wallet.send_self_transfer(from_node=node, fee=fee / COIN) - # expected data - txid = tx["txid"] - vsize = tx["tx"].get_vsize() self.log.info("Polling buffer...") bpf.perf_buffer_poll(timeout=200) @@ -143,10 +138,13 @@ def handle_added_event(_, data, __): self.log.info("Cleaning up mempool...") self.generate(node, 1) - bpf.cleanup() - self.log.info("Ensuring mempool:added event was handled successfully...") assert_equal(EXPECTED_ADDED_EVENTS, handled_added_events) + assert_equal(bytes(event.hash)[::-1].hex(), tx["txid"]) + assert_equal(event.vsize, tx["tx"].get_vsize()) + assert_equal(event.fee, fee) + + bpf.cleanup() def removed_test(self): """Expire a transaction from the mempool and make sure the tracepoint returns @@ -154,6 +152,7 @@ def removed_test(self): EXPECTED_REMOVED_EVENTS = 1 handled_removed_events = 0 + event = None self.log.info("Hooking into mempool:removed tracepoint...") node = self.nodes[0] @@ -162,13 +161,8 @@ def removed_test(self): bpf = BPF(text=MEMPOOL_TRACEPOINTS_PROGRAM, usdt_contexts=[ctx], debug=0) def handle_removed_event(_, data, __): - nonlocal handled_removed_events + nonlocal event, handled_removed_events event = bpf["removed_events"].event(data) - assert_equal(txid, bytes(event.hash)[::-1].hex()) - assert_equal(reason, event.reason.decode("UTF-8")) - assert_equal(vsize, event.vsize) - assert_equal(fee, event.fee) - assert_equal(entry_time, event.entry_time) handled_removed_events += 1 bpf["removed_events"].open_perf_buffer(handle_removed_event) @@ -176,10 +170,7 @@ def handle_removed_event(_, data, __): self.log.info("Sending transaction...") fee = Decimal(31200) tx = self.wallet.send_self_transfer(from_node=node, fee=fee / COIN) - # expected data txid = tx["txid"] - reason = "expiry" - vsize = tx["tx"].get_vsize() self.log.info("Fast-forwarding time to mempool expiry...") entry_time = node.getmempoolentry(txid)["time"] @@ -193,10 +184,15 @@ def handle_removed_event(_, data, __): self.log.info("Polling buffer...") bpf.perf_buffer_poll(timeout=200) - bpf.cleanup() - self.log.info("Ensuring mempool:removed event was handled successfully...") assert_equal(EXPECTED_REMOVED_EVENTS, handled_removed_events) + assert_equal(bytes(event.hash)[::-1].hex(), txid) + assert_equal(event.reason.decode("UTF-8"), "expiry") + assert_equal(event.vsize, tx["tx"].get_vsize()) + assert_equal(event.fee, fee) + assert_equal(event.entry_time, entry_time) + + bpf.cleanup() def rejected_test(self): """Create an invalid transaction and make sure the tracepoint returns @@ -204,6 +200,7 @@ def rejected_test(self): EXPECTED_REJECTED_EVENTS = 1 handled_rejected_events = 0 + event = None self.log.info("Adding P2P connection...") node = self.nodes[0] @@ -215,10 +212,8 @@ def rejected_test(self): bpf = BPF(text=MEMPOOL_TRACEPOINTS_PROGRAM, usdt_contexts=[ctx], debug=0) def handle_rejected_event(_, data, __): - nonlocal handled_rejected_events + nonlocal event, handled_rejected_events event = bpf["rejected_events"].event(data) - assert_equal(txid, bytes(event.hash)[::-1].hex()) - assert_equal(reason, event.reason.decode("UTF-8")) handled_rejected_events += 1 bpf["rejected_events"].open_perf_buffer(handle_rejected_event) @@ -227,17 +222,15 @@ def handle_rejected_event(_, data, __): tx = self.wallet.create_self_transfer(fee_rate=Decimal(0)) node.p2ps[0].send_txs_and_test([tx["tx"]], node, success=False) - # expected data - txid = tx["tx"].hash - reason = "min relay fee not met" - self.log.info("Polling buffer...") bpf.perf_buffer_poll(timeout=200) - bpf.cleanup() - self.log.info("Ensuring mempool:rejected event was handled successfully...") assert_equal(EXPECTED_REJECTED_EVENTS, handled_rejected_events) + assert_equal(bytes(event.hash)[::-1].hex(), tx["tx"].hash) + assert_equal(event.reason.decode("UTF-8"), "min relay fee not met") + + bpf.cleanup() def run_test(self): """Tests the mempool:added, mempool:removed, and mempool:rejected diff --git a/test/functional/interface_usdt_validation.py b/test/functional/interface_usdt_validation.py index 88dfd4d53e57..e00b8ee7a2bf 100755 --- a/test/functional/interface_usdt_validation.py +++ b/test/functional/interface_usdt_validation.py @@ -101,16 +101,7 @@ def handle_blockconnected(_, data, __): nonlocal events, blocks_checked event = ctypes.cast(data, ctypes.POINTER(Block)).contents self.log.info(f"handle_blockconnected(): {event}") - block_hash = bytes(event.hash[::-1]).hex() - block = expected_blocks[block_hash] - assert_equal(block["hash"], block_hash) - assert_equal(block["height"], event.height) - assert_equal(len(block["tx"]), event.transactions) - assert_equal(len([tx["vin"] for tx in block["tx"]]), event.inputs) - assert_equal(0, event.sigops) # no sigops in coinbase tx - # only plausibility checks - assert event.duration > 0 - del expected_blocks[block_hash] + events.append(event) blocks_checked += 1 bpf["block_connected"].open_perf_buffer( From a5ebbd374c8d1c541d51dd5458abbea9fd5b16bc Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Sun, 10 Sep 2023 14:06:17 +0100 Subject: [PATCH 5/8] Merge bitcoin/bitcoin#27944: test: various USDT functional test cleanups (27831 follow-ups) 9f55773a370a0d039e727445ccee6b84e05f562a test: refactor: usdt_mempool: store all events (stickies-v) bc432704505eb165dd86de39ea3434c6fb7a2514 test: refactor: remove unnecessary nonlocal (stickies-v) 326db63a6819813db55ba0d01ab4fe80f7a0d818 test: log sanity check assertion failures (stickies-v) f5525ad6808df6afc38e5c6e4767ab577e30629c test: store utxocache events (stickies-v) f1b99ac94fb77340c4d3a5b4bbc3df28009bc773 test: refactor: deduplicate handle_utxocache_* logic (stickies-v) ad90ba36bd930f00753643cd1fe0af72d1c828c2 test: refactor: rename inbound to is_inbound (stickies-v) afc0224cdbe73e326addf5fb98a3e95d941f2104 test: refactor: remove unnecessary blocks_checked counter (stickies-v) Pull request description: Various cleanups to the USDT functional tests, largely (but not exclusively) follow-ups to https://github.com/bitcoin/bitcoin/pull/27831#pullrequestreview-1491438045. Except for slightly different logging behaviour in "test: store utxocache events" and "test: log sanity check assertion failures", this is a refactor PR, removing unnecessary code and (imo) making it more readable and maintainable. The rationale for each change is in the corresponding commit message. Note: except for "test: store utxocache events" (which relies on its parent, and I separated into two commits because we may want the parent but not the child), all commits are stand-alone and I'm okay with dropping one/multiple commits if they turn out to be controversial or undesired. ACKs for top commit: 0xB10C: ACK 9f55773a370a0d039e727445ccee6b84e05f562a. Reviewed the code and ran the USDT interface tests. I stepped through the commits and think all changes are reasonable. Tree-SHA512: 6c37a0265b6c26d4f9552a056a690b8f86f7304bd33b4419febd8b17369cf6af799cb87c16df35d0c2a1b839ad31de24661d4384eafa88816c2051c522fd3bf5 Co-authored-by: fanquake --- test/functional/interface_usdt_mempool.py | 33 ++++------ test/functional/interface_usdt_net.py | 11 ++-- test/functional/interface_usdt_utxocache.py | 68 +++++++++----------- test/functional/interface_usdt_validation.py | 5 +- 4 files changed, 47 insertions(+), 70 deletions(-) diff --git a/test/functional/interface_usdt_mempool.py b/test/functional/interface_usdt_mempool.py index 199de10dae40..c89401d551de 100755 --- a/test/functional/interface_usdt_mempool.py +++ b/test/functional/interface_usdt_mempool.py @@ -111,9 +111,7 @@ def added_test(self): """Add a transaction to the mempool and make sure the tracepoint returns the expected txid, vsize, and fee.""" - EXPECTED_ADDED_EVENTS = 1 - handled_added_events = 0 - event = None + events = [] self.log.info("Hooking into mempool:added tracepoint...") node = self.nodes[0] @@ -122,9 +120,7 @@ def added_test(self): bpf = BPF(text=MEMPOOL_TRACEPOINTS_PROGRAM, usdt_contexts=[ctx], debug=0) def handle_added_event(_, data, __): - nonlocal event, handled_added_events - event = bpf["added_events"].event(data) - handled_added_events += 1 + events.append(bpf["added_events"].event(data)) bpf["added_events"].open_perf_buffer(handle_added_event) @@ -139,7 +135,8 @@ def handle_added_event(_, data, __): self.generate(node, 1) self.log.info("Ensuring mempool:added event was handled successfully...") - assert_equal(EXPECTED_ADDED_EVENTS, handled_added_events) + assert_equal(1, len(events)) + event = events[0] assert_equal(bytes(event.hash)[::-1].hex(), tx["txid"]) assert_equal(event.vsize, tx["tx"].get_vsize()) assert_equal(event.fee, fee) @@ -150,9 +147,7 @@ def removed_test(self): """Expire a transaction from the mempool and make sure the tracepoint returns the expected txid, expiry reason, vsize, and fee.""" - EXPECTED_REMOVED_EVENTS = 1 - handled_removed_events = 0 - event = None + events = [] self.log.info("Hooking into mempool:removed tracepoint...") node = self.nodes[0] @@ -161,9 +156,7 @@ def removed_test(self): bpf = BPF(text=MEMPOOL_TRACEPOINTS_PROGRAM, usdt_contexts=[ctx], debug=0) def handle_removed_event(_, data, __): - nonlocal event, handled_removed_events - event = bpf["removed_events"].event(data) - handled_removed_events += 1 + events.append(bpf["removed_events"].event(data)) bpf["removed_events"].open_perf_buffer(handle_removed_event) @@ -185,7 +178,8 @@ def handle_removed_event(_, data, __): bpf.perf_buffer_poll(timeout=200) self.log.info("Ensuring mempool:removed event was handled successfully...") - assert_equal(EXPECTED_REMOVED_EVENTS, handled_removed_events) + assert_equal(1, len(events)) + event = events[0] assert_equal(bytes(event.hash)[::-1].hex(), txid) assert_equal(event.reason.decode("UTF-8"), "expiry") assert_equal(event.vsize, tx["tx"].get_vsize()) @@ -198,9 +192,7 @@ def rejected_test(self): """Create an invalid transaction and make sure the tracepoint returns the expected txid, rejection reason, peer id, and peer address.""" - EXPECTED_REJECTED_EVENTS = 1 - handled_rejected_events = 0 - event = None + events = [] self.log.info("Adding P2P connection...") node = self.nodes[0] @@ -212,9 +204,7 @@ def rejected_test(self): bpf = BPF(text=MEMPOOL_TRACEPOINTS_PROGRAM, usdt_contexts=[ctx], debug=0) def handle_rejected_event(_, data, __): - nonlocal event, handled_rejected_events - event = bpf["rejected_events"].event(data) - handled_rejected_events += 1 + events.append(bpf["rejected_events"].event(data)) bpf["rejected_events"].open_perf_buffer(handle_rejected_event) @@ -226,7 +216,8 @@ def handle_rejected_event(_, data, __): bpf.perf_buffer_poll(timeout=200) self.log.info("Ensuring mempool:rejected event was handled successfully...") - assert_equal(EXPECTED_REJECTED_EVENTS, handled_rejected_events) + assert_equal(1, len(events)) + event = events[0] assert_equal(bytes(event.hash)[::-1].hex(), tx["tx"].hash) assert_equal(event.reason.decode("UTF-8"), "min relay fee not met") diff --git a/test/functional/interface_usdt_net.py b/test/functional/interface_usdt_net.py index c55a7976bff4..ca6151973660 100755 --- a/test/functional/interface_usdt_net.py +++ b/test/functional/interface_usdt_net.py @@ -121,11 +121,11 @@ def __repr__(self): checked_outbound_version_msg = 0 events = [] - def check_p2p_message(event, inbound): + def check_p2p_message(event, is_inbound): nonlocal checked_inbound_version_msg, checked_outbound_version_msg if event.msg_type.decode("utf-8") == "version": self.log.info( - f"check_p2p_message(): {'inbound' if inbound else 'outbound'} {event}") + f"check_p2p_message(): {'inbound' if is_inbound else 'outbound'} {event}") peer = self.nodes[0].getpeerinfo()[0] msg = msg_version() msg.deserialize(BytesIO(bytes(event.msg[:event.msg_size]))) @@ -133,13 +133,12 @@ def check_p2p_message(event, inbound): assert_equal(peer["addr"], event.peer_addr.decode("utf-8")) assert_equal(peer["connection_type"], event.peer_conn_type.decode("utf-8")) - if inbound: + if is_inbound: checked_inbound_version_msg += 1 else: checked_outbound_version_msg += 1 def handle_inbound(_, data, __): - nonlocal events event = ctypes.cast(data, ctypes.POINTER(P2PMessage)).contents events.append((event, True)) @@ -157,8 +156,8 @@ def handle_outbound(_, data, __): self.log.info( "check receipt and content of in- and outbound version messages") - for event, inbound in events: - check_p2p_message(event, inbound) + for event, is_inbound in events: + check_p2p_message(event, is_inbound) assert_equal(EXPECTED_INOUTBOUND_VERSION_MSG, checked_inbound_version_msg) assert_equal(EXPECTED_INOUTBOUND_VERSION_MSG, diff --git a/test/functional/interface_usdt_utxocache.py b/test/functional/interface_usdt_utxocache.py index e5103e184b73..28caffd19255 100755 --- a/test/functional/interface_usdt_utxocache.py +++ b/test/functional/interface_usdt_utxocache.py @@ -252,43 +252,30 @@ def test_add_spent(self): # that the handle_* functions succeeded. EXPECTED_HANDLE_ADD_SUCCESS = 2 EXPECTED_HANDLE_SPENT_SUCCESS = 1 - handle_add_succeeds = 0 - handle_spent_succeeds = 0 - expected_utxocache_spents = [] expected_utxocache_adds = [] + expected_utxocache_spents = [] + + actual_utxocache_adds = [] + actual_utxocache_spents = [] + + def compare_utxo_with_event(utxo, event): + """Compare a utxo dict to the event produced by BPF""" + assert_equal(utxo["txid"], bytes(event.txid[::-1]).hex()) + assert_equal(utxo["index"], event.index) + assert_equal(utxo["height"], event.height) + assert_equal(utxo["value"], event.value) + assert_equal(utxo["is_coinbase"], event.is_coinbase) def handle_utxocache_add(_, data, __): - nonlocal handle_add_succeeds event = ctypes.cast(data, ctypes.POINTER(UTXOCacheChange)).contents self.log.info(f"handle_utxocache_add(): {event}") - add = expected_utxocache_adds.pop(0) - try: - assert_equal(add["txid"], bytes(event.txid[::-1]).hex()) - assert_equal(add["index"], event.index) - assert_equal(add["height"], event.height) - assert_equal(add["value"], event.value) - assert_equal(add["is_coinbase"], event.is_coinbase) - except AssertionError: - self.log.exception("Assertion failed") - else: - handle_add_succeeds += 1 + actual_utxocache_adds.append(event) def handle_utxocache_spent(_, data, __): - nonlocal handle_spent_succeeds event = ctypes.cast(data, ctypes.POINTER(UTXOCacheChange)).contents self.log.info(f"handle_utxocache_spent(): {event}") - spent = expected_utxocache_spents.pop(0) - try: - assert_equal(spent["txid"], bytes(event.txid[::-1]).hex()) - assert_equal(spent["index"], event.index) - assert_equal(spent["height"], event.height) - assert_equal(spent["value"], event.value) - assert_equal(spent["is_coinbase"], event.is_coinbase) - except AssertionError: - self.log.exception("Assertion failed") - else: - handle_spent_succeeds += 1 + actual_utxocache_spents.append(event) bpf["utxocache_add"].open_perf_buffer(handle_utxocache_add) bpf["utxocache_spent"].open_perf_buffer(handle_utxocache_spent) @@ -324,19 +311,18 @@ def handle_utxocache_spent(_, data, __): "is_coinbase": block_index == 0, }) - assert_equal(EXPECTED_HANDLE_ADD_SUCCESS, len(expected_utxocache_adds)) - assert_equal(EXPECTED_HANDLE_SPENT_SUCCESS, - len(expected_utxocache_spents)) - bpf.perf_buffer_poll(timeout=200) - bpf.cleanup() + + assert_equal(EXPECTED_HANDLE_ADD_SUCCESS, len(expected_utxocache_adds), len(actual_utxocache_adds)) + assert_equal(EXPECTED_HANDLE_SPENT_SUCCESS, len(expected_utxocache_spents), len(actual_utxocache_spents)) self.log.info( f"check that we successfully traced {EXPECTED_HANDLE_ADD_SUCCESS} adds and {EXPECTED_HANDLE_SPENT_SUCCESS} spent") - assert_equal(0, len(expected_utxocache_adds)) - assert_equal(0, len(expected_utxocache_spents)) - assert_equal(EXPECTED_HANDLE_ADD_SUCCESS, handle_add_succeeds) - assert_equal(EXPECTED_HANDLE_SPENT_SUCCESS, handle_spent_succeeds) + for expected_utxo, actual_event in zip(expected_utxocache_adds + expected_utxocache_spents, + actual_utxocache_adds + actual_utxocache_spents): + compare_utxo_with_event(expected_utxo, actual_event) + + bpf.cleanup() def test_flush(self): """ Tests the utxocache:flush tracepoint API. @@ -367,9 +353,13 @@ def handle_utxocache_flush(_, data, __): "size": event.size }) # sanity checks only - assert event.memory > 0 - assert event.duration > 0 - handle_flush_succeeds += 1 + try: + assert event.memory > 0 + assert event.duration > 0 + except AssertionError: + self.log.exception("Assertion error") + else: + handle_flush_succeeds += 1 bpf["utxocache_flush"].open_perf_buffer(handle_utxocache_flush) diff --git a/test/functional/interface_usdt_validation.py b/test/functional/interface_usdt_validation.py index e00b8ee7a2bf..d02ca9df5354 100755 --- a/test/functional/interface_usdt_validation.py +++ b/test/functional/interface_usdt_validation.py @@ -86,7 +86,6 @@ def __repr__(self): self.duration) BLOCKS_EXPECTED = 2 - blocks_checked = 0 expected_blocks = dict() events = [] @@ -98,11 +97,9 @@ def __repr__(self): usdt_contexts=[ctx], debug=0, cflags=["-Wno-error=implicit-function-declaration"]) def handle_blockconnected(_, data, __): - nonlocal events, blocks_checked event = ctypes.cast(data, ctypes.POINTER(Block)).contents self.log.info(f"handle_blockconnected(): {event}") events.append(event) - blocks_checked += 1 bpf["block_connected"].open_perf_buffer( handle_blockconnected) @@ -127,7 +124,7 @@ def handle_blockconnected(_, data, __): # only plausibility checks assert event.duration > 0 del expected_blocks[block_hash] - assert_equal(BLOCKS_EXPECTED, blocks_checked) + assert_equal(BLOCKS_EXPECTED, len(events)) assert_equal(0, len(expected_blocks)) bpf.cleanup() From cbbf96d7078520064bb069d7411c5cdcb2070efc Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 17 Jul 2023 15:00:45 +0100 Subject: [PATCH 6/8] Merge bitcoin/bitcoin#28088: test: Disable known broken USDT test faf8be7c32be00f660eba90d3f07313fb25d5d1c test: Disable known broken USDT test (MarcoFalke) Pull request description: The failure is known and running into more failures doesn't help anyone. Not disabling the test would be a waste of CPU and developer time. https://github.com/bitcoin/bitcoin/issues/27380 Top commit has no ACKs. Tree-SHA512: d0469153b00d6b30e10a21bcd52d508fcf9f796ff2468f59aff75020a82c718bcae85caf4b58397dea6fd9e210b501353fd51567f979c6b57d3b1bb23d318216 Co-authored-by: fanquake --- test/functional/interface_usdt_mempool.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/functional/interface_usdt_mempool.py b/test/functional/interface_usdt_mempool.py index c89401d551de..10b244150cc6 100755 --- a/test/functional/interface_usdt_mempool.py +++ b/test/functional/interface_usdt_mempool.py @@ -219,7 +219,10 @@ def handle_rejected_event(_, data, __): assert_equal(1, len(events)) event = events[0] assert_equal(bytes(event.hash)[::-1].hex(), tx["tx"].hash) - assert_equal(event.reason.decode("UTF-8"), "min relay fee not met") + # The next test is already known to fail, so disable it to avoid + # wasting CPU time and developer time. See + # https://github.com/bitcoin/bitcoin/issues/27380 + #assert_equal(event.reason.decode("UTF-8"), "min relay fee not met") bpf.cleanup() From f2c4d398738b37fdccf7c9e1a7f9548c36cbcd8c Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 28 Jun 2023 15:01:51 -0400 Subject: [PATCH 7/8] Merge bitcoin/bitcoin#27927: util: Allow std::byte and char Span serialization fa38d862358b87219b12bf31236c52f28d9fc5d6 Use only Span{} constructor for byte-like types where possible (MarcoFalke) fa257bc8312b91c2d281f48ca2500d9cba353cc5 util: Allow std::byte and char Span serialization (MarcoFalke) Pull request description: Seems odd to require developers to cast all byte-like spans passed to serialization to `unsigned char`-spans. Fix that by passing and accepting byte-like spans as-is. Finally, add tests and update the code to use just `Span` where possible. ACKs for top commit: sipa: utACK fa38d862358b87219b12bf31236c52f28d9fc5d6 achow101: ACK fa38d862358b87219b12bf31236c52f28d9fc5d6 ryanofsky: Code review ACK fa38d862358b87219b12bf31236c52f28d9fc5d6. This looks great. The second commit really removes a lot of boilerplate and shows why the first commit is useful. Tree-SHA512: 788592d9ff515c3ebe73d48f9ecbb8d239f5b985af86f09974e508cafb0ca6d73a959350295246b4dfb496149bc56330a0b5d659fc434ba6723dbaba0b7a49e5 Co-authored-by: Andrew Chow --- src/bench/load_external.cpp | 2 +- src/net.cpp | 4 ++-- src/pubkey.h | 4 ++-- src/test/fuzz/p2p_transport_serialization.cpp | 2 +- src/test/serialize_tests.cpp | 14 +++++++------- src/uint256.h | 2 +- src/wallet/test/db_tests.cpp | 2 +- src/wallet/wallet.cpp | 2 +- src/wallet/walletdb.cpp | 2 +- 9 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/bench/load_external.cpp b/src/bench/load_external.cpp index 2ec7b8d59423..33667b02cc5e 100644 --- a/src/bench/load_external.cpp +++ b/src/bench/load_external.cpp @@ -34,7 +34,7 @@ static void LoadExternalBlockFile(benchmark::Bench& bench) ss << static_cast(benchmark::data::block813851.size()); // We can't use the streaming serialization (ss << benchmark::data::block813851) // because that first writes a compact size. - ss.write(MakeByteSpan(benchmark::data::block813851)); + ss << Span{benchmark::data::block813851}; // Create the test file. { diff --git a/src/net.cpp b/src/net.cpp index dbf34dbbe5ce..d072c33aa305 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -5069,13 +5069,13 @@ static void CaptureMessageToFile(const CAddress& addr, AutoFile f{fsbridge::fopen(path, "ab")}; ser_writedata64(f, now.count()); - f.write(MakeByteSpan(msg_type)); + f << Span{msg_type}; for (auto i = msg_type.length(); i < CMessageHeader::COMMAND_SIZE; ++i) { f << uint8_t{'\0'}; } uint32_t size = data.size(); ser_writedata32(f, size); - f.write(AsBytes(data)); + f << data; } std::function void Unserialize(Stream& s) { const unsigned int len(::ReadCompactSize(s)); if (len <= SIZE) { - s.read(AsWritableBytes(Span{vch, len})); + s >> Span{vch, len}; if (len != size()) { Invalidate(); } diff --git a/src/test/fuzz/p2p_transport_serialization.cpp b/src/test/fuzz/p2p_transport_serialization.cpp index 10953cf169e5..cf168942c6a0 100644 --- a/src/test/fuzz/p2p_transport_serialization.cpp +++ b/src/test/fuzz/p2p_transport_serialization.cpp @@ -84,7 +84,7 @@ FUZZ_TARGET(p2p_transport_serialization, .init = initialize_p2p_transport_serial assert(msg.m_time == m_time); std::vector header; - auto msg2 = CNetMsgMaker{msg.m_recv.GetVersion()}.Make(msg.m_type, MakeUCharSpan(msg.m_recv)); + auto msg2 = CNetMsgMaker{msg.m_recv.GetVersion()}.Make(msg.m_type, Span{msg.m_recv}); bool queued = send_transport.SetMessageToSend(msg2); assert(queued); std::optional known_more; diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp index df4c8bdd4933..890db816f3b0 100644 --- a/src/test/serialize_tests.cpp +++ b/src/test/serialize_tests.cpp @@ -239,32 +239,32 @@ BOOST_AUTO_TEST_CASE(noncanonical) std::vector::size_type n; // zero encoded with three bytes: - ss.write(MakeByteSpan("\xfd\x00\x00").first(3)); + ss << Span{"\xfd\x00\x00"}.first(3); BOOST_CHECK_EXCEPTION(ReadCompactSize(ss), std::ios_base::failure, isCanonicalException); // 0xfc encoded with three bytes: - ss.write(MakeByteSpan("\xfd\xfc\x00").first(3)); + ss << Span{"\xfd\xfc\x00"}.first(3); BOOST_CHECK_EXCEPTION(ReadCompactSize(ss), std::ios_base::failure, isCanonicalException); // 0xfd encoded with three bytes is OK: - ss.write(MakeByteSpan("\xfd\xfd\x00").first(3)); + ss << Span{"\xfd\xfd\x00"}.first(3); n = ReadCompactSize(ss); BOOST_CHECK(n == 0xfd); // zero encoded with five bytes: - ss.write(MakeByteSpan("\xfe\x00\x00\x00\x00").first(5)); + ss << Span{"\xfe\x00\x00\x00\x00"}.first(5); BOOST_CHECK_EXCEPTION(ReadCompactSize(ss), std::ios_base::failure, isCanonicalException); // 0xffff encoded with five bytes: - ss.write(MakeByteSpan("\xfe\xff\xff\x00\x00").first(5)); + ss << Span{"\xfe\xff\xff\x00\x00"}.first(5); BOOST_CHECK_EXCEPTION(ReadCompactSize(ss), std::ios_base::failure, isCanonicalException); // zero encoded with nine bytes: - ss.write(MakeByteSpan("\xff\x00\x00\x00\x00\x00\x00\x00\x00").first(9)); + ss << Span{"\xff\x00\x00\x00\x00\x00\x00\x00\x00"}.first(9); BOOST_CHECK_EXCEPTION(ReadCompactSize(ss), std::ios_base::failure, isCanonicalException); // 0x01ffffff encoded with nine bytes: - ss.write(MakeByteSpan("\xff\xff\xff\xff\x01\x00\x00\x00\x00").first(9)); + ss << Span{"\xff\xff\xff\xff\x01\x00\x00\x00\x00"}.first(9); BOOST_CHECK_EXCEPTION(ReadCompactSize(ss), std::ios_base::failure, isCanonicalException); } diff --git a/src/uint256.h b/src/uint256.h index e32612c5e911..db629dff2ea6 100644 --- a/src/uint256.h +++ b/src/uint256.h @@ -79,7 +79,7 @@ class base_blob template void Serialize(Stream& s) const { - s.write(MakeByteSpan(m_data)); + s << Span(m_data); } template diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp index 47c160fd64d5..6863c38f5c51 100644 --- a/src/wallet/test/db_tests.cpp +++ b/src/wallet/test/db_tests.cpp @@ -200,7 +200,7 @@ BOOST_AUTO_TEST_CASE(db_cursor_prefix_byte_test) for (const auto& database : TestDatabases(m_path_root)) { std::unique_ptr batch = database->MakeBatch(); for (const auto& [k, v] : {e, p, ps, f, fs, ff, ffs}) { - batch->Write(MakeUCharSpan(k), MakeUCharSpan(v)); + batch->Write(Span{k}, Span{v}); } CheckPrefix(*batch, StringBytes(""), {e, p, ps, f, fs, ff, ffs}); CheckPrefix(*batch, StringBytes("prefix"), {p, ps}); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 683a34e60293..0fb7fcf1a801 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4889,7 +4889,7 @@ bool CWallet::MigrateToSQLite(bilingual_str& error) bool began = batch->TxnBegin(); assert(began); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution. for (const auto& [key, value] : records) { - if (!batch->Write(MakeUCharSpan(key), MakeUCharSpan(value))) { + if (!batch->Write(Span{key}, Span{value})) { batch->TxnAbort(); m_database->Close(); fs::remove(m_database->Filename()); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index b8e7af1a7e26..7e1e0dbaa653 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1273,7 +1273,7 @@ bool WalletBatch::EraseRecords(const std::unordered_set& types) } // Make a copy of key to avoid data being deleted by the following read of the type - Span key_data = MakeUCharSpan(key); + Span key_data{key}; std::string type; key >> type; From 4403b48a850f090ef9dd1a4998eb2e9bf44baa00 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 29 Jun 2023 17:18:01 -0400 Subject: [PATCH 8/8] Merge bitcoin/bitcoin#27978: refactor: Drop unsafe AsBytePtr function 7c853619ee9ea17a79f1234b6c7871a45ceadcb9 refactor: Drop unsafe AsBytePtr function (Ryan Ofsky) Pull request description: Replace calls to `AsBytePtr` with calls to `AsBytes` or `reinterpret_cast`. `AsBytePtr` is just a wrapper around `reinterpret_cast`. It accepts any type of pointer as an argument and uses `reinterpret_cast` to cast the argument to a `std::byte` pointer. Despite taking any type of pointer as an argument, it is not useful to call `AsBytePtr` on most types of pointers, because byte representations of most types will be platform specific or undefined. Also, because it is named similarly to the `AsBytes` function, `AsBytePtr` looks safer than it actually is. Both `AsBytes` and `AsBytePtr` call reinterpret_cast internally and may be unsafe to use with certain types, but AsBytes at least has some type checking and can only be called on `Span` objects, while `AsBytePtr` can be called on any pointer argument. The change was motivated by discussion on #27973 and #27927 and is compatible with those PRs ACKs for top commit: jonatack: re-ACK 7c853619ee9ea17a79f1234b6c7871a45ceadcb9 sipa: utACK 7c853619ee9ea17a79f1234b6c7871a45ceadcb9 achow101: ACK 7c853619ee9ea17a79f1234b6c7871a45ceadcb9 Tree-SHA512: 200d858b1d4d579f081a7f9a14d488a99713b4918b4564ac3dd5c18578d927dbd6426e62e02f49f04a3fa73ca02ff7109c495cb0b92bec43c27d9b74e2f95757 Co-authored-by: Andrew Chow