Skip to content

refactor: Make node_id a const& in RemoveBlockRequest #31282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
achow101 merged 3 commits into from
Apr 14, 2025
Merged

refactor: Make node_id a const& in RemoveBlockRequest #31282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa1622d
refactor: Make node_id a const& in RemoveBlockRequest
Nov 13, 2024
fabd05b
refactor: Fix net_processing iwyu includes
Nov 13, 2024
fa21f83
ci: Use G++ in valgrind tasks
Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions ci/test/00_setup_env_native_fuzz_with_valgrind.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export LC_ALL=C.UTF-8

export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04"
export CONTAINER_NAME=ci_native_fuzz_valgrind
export PACKAGES="clang-16 llvm-16 libclang-rt-16-dev libevent-dev libboost-dev libsqlite3-dev valgrind"
export PACKAGES="libevent-dev libboost-dev libsqlite3-dev valgrind"
export NO_DEPENDS=1
export RUN_UNIT_TESTS=false
export RUN_FUNCTIONAL_TESTS=false
Expand All @@ -17,8 +17,4 @@ export FUZZ_TESTS_CONFIG="--valgrind"
export GOAL="install"
export BITCOIN_CONFIG="\
-DBUILD_FOR_FUZZING=ON \
-DSANITIZERS=fuzzer \
Copy link
Contributor

@sedited sedited Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not keep using clang for this job?

Copy link
Member Author

@maflcko maflcko Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It requires adding a suppression, or some other workaround at some point, see #29635 (comment)

Given that GCC does not require suppressions, it may be preferable for now.

Though, I am happy to drop the commit, and leave it for a follow-up. I just thought, it would be nice to include here, so that testing is easy for those that want.

Copy link
Contributor

@sedited sedited Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmh, I was more concerned about losing the sanitizer, but I guess there is nothing really gained by having it turned on when running valgrind.

Copy link
Member Author

@maflcko maflcko Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only benefit would be the symbolizer, but that's not really needed with valgrind.

Copy link
Contributor

@ryanofsky ryanofsky Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "ci: Use G++ in valgrind tasks" (fa21f83)

I think it would be helpful if this commit message give some hint about why this change is being made. Currently no reason is given and even looking at this PR and understanding a little bit about the bug and trying to read discussion at #29635 (comment) I don't think I get it. If "Currently, valgrind is not usable on a default build with GCC" according to the PR and requires a workaround, why would we switch CI from clang to GCC? That seems like it would just require more workarounds in the future? I'm not questioning the approach and am fine with this change, I just think it would be good to have more explanation of it somewhere, ideally in the commit message.

Copy link
Member

@achow101 achow101 Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the point is that we want valgrind to be running on builds that are as close as possible to what we release? Since we use gcc for release builds, we would want to have this CI use gcc as well.

Copy link
Member Author

@maflcko maflcko Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just think it would be good to have more explanation of it somewhere, ideally in the commit message.

Sorry for missing this message and not updating the commit message before merge. There are several reasons, for example:

  • With the fix in this pull, G++ is the only compiler currently that makes the CI script work on any arch (amd64, aarch64). So using G++ can make it easier to run the CI task on any such machine.
  • This pull is working around a G++ and valgrind incompatibility, so using the two in the CI task can make it easier to test.
  • As Ava points out, G++ is used in the releases. Also, G++ seems to be used more widely by developers? So using G++ in the CI could help support that better. However, I don't think it matters much, and anything is fine here. We should strive to support both compilers to work with valgrind.

-DCMAKE_C_COMPILER=clang-16 \
-DCMAKE_CXX_COMPILER=clang++-16 \
"
export LLVM_SYMBOLIZER_PATH="/usr/bin/llvm-symbolizer-16"
4 changes: 1 addition & 3 deletions ci/test/00_setup_env_native_valgrind.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,12 @@ export LC_ALL=C.UTF-8

export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04"
export CONTAINER_NAME=ci_native_valgrind
export PACKAGES="valgrind clang-16 llvm-16 libclang-rt-16-dev python3-zmq libevent-dev libboost-dev libdb5.3++-dev libzmq3-dev libsqlite3-dev"
export PACKAGES="valgrind python3-zmq libevent-dev libboost-dev libdb5.3++-dev libzmq3-dev libsqlite3-dev"
export USE_VALGRIND=1
export NO_DEPENDS=1
export TEST_RUNNER_EXTRA="--exclude feature_init,rpc_bind,feature_bind_extra" # feature_init excluded for now, see https://github.com/bitcoin/bitcoin/issues/30011 ; bind tests excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547
export GOAL="install"
# TODO enable GUI
export BITCOIN_CONFIG="\
-DWITH_ZMQ=ON -DWITH_BDB=ON -DWARN_INCOMPATIBLE_BDB=OFF -DBUILD_GUI=OFF \
-DCMAKE_C_COMPILER=clang-16 \
-DCMAKE_CXX_COMPILER=clang++-16 \
"
5 changes: 2 additions & 3 deletions contrib/valgrind.supp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@
# --error-limit=no build/src/test/test_bitcoin
#
# Note that suppressions may depend on OS and/or library versions.
# Tested on:
# * aarch64 (Ubuntu Noble system libs, clang, without gui)
# * x86_64 (Ubuntu Noble system libs, clang, without gui)
# Tested on aarch64 and x86_64 with Ubuntu Noble system libs, using clang-16
# and GCC, without gui.
{
Suppress libdb warning - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=662917
Memcheck:Cond
Expand Down
42 changes: 36 additions & 6 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,57 +1,87 @@
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-2022 The Bitcoin Core developers
// Copyright (c) 2009-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#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>
#include <util/trace.h>
#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>

Expand Down Expand Up @@ -1155,7 +1185,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};
Copy link
Member

@Sjors Sjors Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fa91206b3c570dc17ee18565ade04067a88e4ef8: this seems fine to me. The code is nicer and happens to fix a false positive.


if (from_peer && *from_peer != node_id) {
range.first++;
Expand Down
17 changes: 15 additions & 2 deletions src/net_processing.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,34 @@
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-2022 The Bitcoin Core developers
// Copyright (c) 2009-present 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_NET_PROCESSING_H
#define BITCOIN_NET_PROCESSING_H

#include <consensus/amount.h>
#include <net.h>
#include <protocol.h>
#include <threadsafety.h>
#include <txorphanage.h>
#include <validationinterface.h>

#include <atomic>
#include <chrono>
#include <cstdint>
#include <memory>
#include <optional>
#include <string>
#include <vector>

class AddrMan;
class CChainParams;
class CTxMemPool;
class ChainstateManager;
class BanMan;
class CBlockIndex;
class CScheduler;
class DataStream;
class uint256;

namespace node {
class Warnings;
Expand Down