Merge bitcoin/bitcoin#31282: refactor: Make node_id a const& in RemoveBlockRequest

fa21f83d29 ci: Use G++ in valgrind tasks (MarcoFalke)
fabd05bf65 refactor: Fix net_processing iwyu includes (MarcoFalke)
fa1622db20 refactor: Make node_id a const& in RemoveBlockRequest (MarcoFalke)

Pull request description:

  Currently, `valgrind` is not usable on a default build with GCC. Specifically, `p2p_compactblocks.py --valgrind` gives a false-positive in `RemoveBlockRequest` when comparing `node_id` with `from_peer`. According to the upstream bug report, this happens because both symbols are on the stack and the compiler can more aggressively optimize the compare (order). See https://bugs.kde.org/show_bug.cgi?id=472329#c7

  It is possible to work around this bug by pulling at least one value from the stack. For example, by making `from_peer` a `const` reference. Alternatively, by replacing `auto [node_id, list_it]` with `const auto& [node_id, list_it]`, which is done here.

  I think this workaround is acceptable, because it does not look like valgrind can trivially fix this. The alternative would be to add a (temporary?) suppression.

  Fixes https://github.com/bitcoin/bitcoin/issues/27741

  Also, fix iwyu includes, while touching this module.

  Also, switch the CI valgrind scripts to use G++.

ACKs for top commit:
  achow101:
    ACK fa21f83d29
  TheCharlatan:
    ACK fa21f83d29
  darosior:
    utACK fa21f83d29
  ryanofsky:
    Code review ACK fa21f83d29. Code changes all look good but I'm a little confused about purpose of the third commit, so left a question about that

Tree-SHA512: 7b92cdafd525a5ac53ae2c1a7a92e599bc9b5fd5d315a694b493cd5079ac323d884393b57aa18581b7789247a588c9a27d47698de25b340bc76fc9f1dd1850b4
This commit is contained in:
Ava Chow
2025-04-14 14:22:56 -07:00
5 changed files with 54 additions and 18 deletions

View File

@@ -6,40 +6,55 @@
#include <net_processing.h>
#include <addrman.h>
#include <arith_uint256.h>
#include <banman.h>
#include <blockencodings.h>
#include <blockfilter.h>
#include <chain.h>
#include <chainparams.h>
#include <common/bloom.h>
#include <consensus/amount.h>
#include <consensus/params.h>
#include <consensus/validation.h>
#include <core_memusage.h>
#include <crypto/siphash.h>
#include <deploymentstatus.h>
#include <hash.h>
#include <flatfile.h>
#include <headerssync.h>
#include <index/blockfilterindex.h>
#include <kernel/chain.h>
#include <kernel/mempool_entry.h>
#include <logging.h>
#include <merkleblock.h>
#include <net.h>
#include <net_permissions.h>
#include <netaddress.h>
#include <netbase.h>
#include <netmessagemaker.h>
#include <node/blockstorage.h>
#include <node/connection_types.h>
#include <node/protocol_version.h>
#include <node/timeoffsets.h>
#include <node/txdownloadman.h>
#include <node/txreconciliation.h>
#include <node/warnings.h>
#include <policy/feerate.h>
#include <policy/fees.h>
#include <policy/packages.h>
#include <policy/policy.h>
#include <policy/settings.h>
#include <primitives/block.h>
#include <primitives/transaction.h>
#include <protocol.h>
#include <random.h>
#include <scheduler.h>
#include <script/script.h>
#include <serialize.h>
#include <span.h>
#include <streams.h>
#include <sync.h>
#include <tinyformat.h>
#include <txmempool.h>
#include <txorphanage.h>
#include <txrequest.h>
#include <uint256.h>
#include <util/check.h>
#include <util/strencodings.h>
#include <util/time.h>
@@ -47,11 +62,26 @@
#include <validation.h>
#include <algorithm>
#include <array>
#include <atomic>
#include <compare>
#include <cstddef>
#include <deque>
#include <exception>
#include <functional>
#include <future>
#include <initializer_list>
#include <iterator>
#include <limits>
#include <list>
#include <map>
#include <memory>
#include <optional>
#include <queue>
#include <ranges>
#include <ratio>
#include <set>
#include <span>
#include <typeinfo>
#include <utility>
@@ -1156,7 +1186,7 @@ void PeerManagerImpl::RemoveBlockRequest(const uint256& hash, std::optional<Node
Assume(mapBlocksInFlight.count(hash) <= MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK);
while (range.first != range.second) {
auto [node_id, list_it] = range.first->second;
const auto& [node_id, list_it]{range.first->second};
if (from_peer && *from_peer != node_id) {
range.first++;