refactor: Make node_id a const& in RemoveBlockRequest #31282

1. 
   maflcko commented at 8:54 am on November 13, 2024: member

   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 #27741

   Also, fix iwyu includes, while touching this module.

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

2. 
   DrahtBot commented at 8:54 am on November 13, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31282.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	TheCharlatan
   Concept ACK	Sjors

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30975 (Add multiprocess binaries to release build by Sjors)
   • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
   • #28710 (Remove the legacy wallet and BDB dependency by achow101)
   • #27837 (net: introduce block tracker to retry to download blocks after failure by furszy)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

3. maflcko renamed this:
   refactor: Make node_id a const& in RemoveBlockRequest
   refactor: Make node_id a const& in RemoveBlockRequest
   on Nov 13, 2024
4. in src/net_processing.cpp:1188 in fa91206b3c outdated

   1183@@ -1155,7 +1184,7 @@ void PeerManagerImpl::RemoveBlockRequest(const uint256& hash, std::optional<Node
   1184     Assume(mapBlocksInFlight.count(hash) <= MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK);
   1185 
   1186     while (range.first != range.second) {
   1187-        auto [node_id, list_it] = range.first->second;
   1188+        const auto& [node_id, list_it]{range.first->second};
   

   ---

   ---

   

   Sjors commented at 9:59 am on November 13, 2024:

   fa91206b3c570dc17ee18565ade04067a88e4ef8: this seems fine to me. The code is nicer and happens to fix a false positive.
5. 
   Sjors commented at 9:59 am on November 13, 2024: member

   Concept ACK

   Only studied the main one line change.

6. maflcko renamed this:
   refactor: Make node_id a const& in RemoveBlockRequest
   refactor: Make node_id a const& in RemoveBlockRequest
   on Nov 13, 2024
7. DrahtBot renamed this:
   refactor: Make node_id a const& in RemoveBlockRequest
   refactor: Make node_id a const& in RemoveBlockRequest
   on Nov 13, 2024
8. DrahtBot added the label Refactoring on Nov 13, 2024
9. refactor: Make node_id a const& in RemoveBlockRequest

   This works around a valgrind false-positive.

   fa1622db20
10. refactor: Fix net_processing iwyu includes fabd05bf65
11. ci: Use G++ in valgrind tasks fa21f83d29
12. maflcko force-pushed on Nov 13, 2024
13. 
    TheCharlatan commented at 1:42 pm on November 13, 2024: contributor

    Looks like iwyu is still complaining about some of the includes:

    0[08:23:24.120] (/ci_container_base/src/net_processing.h has correct #includes/fwd-decls)
    1[08:23:24.120] 
    2[08:23:24.120] /ci_container_base/src/net_processing.cpp should add these lines:
    3[08:23:24.120] 
    4[08:23:24.120] /ci_container_base/src/net_processing.cpp should remove these lines:
    5[08:23:24.120] - #include <deploymentstatus.h>  // lines 21-21
    6[08:23:24.120] - #include <node/warnings.h>  // lines 39-39
    

14. 
    maflcko commented at 1:55 pm on November 13, 2024: member

    Looks like iwyu is still complaining about some of the includes:

    That’s an upstream bug. Maybe a separate issue can be used to track those?

15. in ci/test/00_setup_env_native_fuzz_with_valgrind.sh:20 in fa21f83d29

    16@@ -17,8 +17,4 @@ export FUZZ_TESTS_CONFIG="--valgrind"
    17 export GOAL="install"
    18 export BITCOIN_CONFIG="\
    19  -DBUILD_FOR_FUZZING=ON \
    20- -DSANITIZERS=fuzzer \
    

    ---

    ---

    

    TheCharlatan commented at 11:52 am on November 14, 2024:

    Why not keep using clang for this job?

    ---

    

    maflcko commented at 12:01 pm on November 14, 2024:

    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.

    ---

    

    TheCharlatan commented at 12:22 pm on November 14, 2024:

    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.

    ---

    

    maflcko commented at 12:24 pm on November 14, 2024:

    The only benefit would be the symbolizer, but that’s not really needed with valgrind.
16. TheCharlatan approved
17. 
    TheCharlatan commented at 12:58 pm on November 14, 2024: contributor

    ACK fa21f83d2983d97006ec1e3c47634dc0fe0349dc
18. DrahtBot requested review from Sjors on Nov 14, 2024
19. maflcko requested review from fanquake on Nov 20, 2024

Contributors
maflcko DrahtBot Sjors TheCharlatan

Review Requested
Sjors fanquake

Labels
Refactoring