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

Reviews

See the guideline for information on the review process.

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:

• #27581 (net: Continuous ASMap health check by fjahr)
• #27071 (Handle CJDNS from LookupSubNet() by vasild)
• #25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es by vasild)

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.

Thank you for the excellent review @stickies-v -- updated to take your feedback.

I thought we were going to avoid unproductive refactors?

I thought we were going to avoid unproductive refactors?

I'm unaware of the context behind that comment but see similar refactoring in many pull requests and, in general, there are good reasons for them; in this case, removing unneeded exports and simplifying and removing code. (If you'd like to review a pull that fixes something, may I re-review beg for #27231 that was updated to take your feedback? Thanks!)

Updated to take all feedback by @stickies-v and @vasild -- thank you!

I think some of the changes here are fine, but seem to be done somewhat verbosely.

There are 3 commits (5ba73cd, df48856, 5316ae5) that independently change/refactor GetLocal(). Any reason they can't be combined, and avoid touching the same line of code 3 times?

Speaking generally, not sure about the addition of [nodiscard]. It's not clear that is something that other project contributors agree with, i.e: discussion in this thread: #27675 (comment).

It's not clear that is something that other project contributors agree with, i.e: discussion in this thread: #27675 (comment).

I can see the desire to enforce it everywhere, but then it should be done via clang-tidy or a clang-tidy plugin. Ideally without modifying the source code at all.

offtopic [[nodiscard]]

There are 3 classes of functions wrt [[nodiscard]]:

1. where ignoring the result is harmful, it probably means a bug in the program, for example write(2)
2. where ignoring the result is ok and the code is equivalent without the call, aka it does not make sense to call the function, its call should be removed because it clutters the source code and makes the program slower, for example strlen(3)
3. where ignoring the result is ok and the function has side effects which are desirable at the call site, for example snprintf(3)

I guess [[nodiscard]] is desirable for 1. and 2. but not for 3.

Re offtopic [[nodiscard]] yes, and some contributors have been using it consistently in cases 1 and 2. I think it's fine for an author not to take a review suggestion to use it, or for the two cases mentioned, to use it in code they write or touch. A bit like the usage of const, perhaps. It's a handy addition to C++. If I have it wrong in a place, happy to update.

There are 3 commits (5ba73cd, df48856, 5316ae5) that independently change/refactor GetLocal().

In this case, the changes have only two lines of diff in common out of a few dozen, so there's little redundancy. They are different kinds of changes that make more sense separately and are easier to review that way -- which I strive for, like the comment above: "Good changes, easy to review. Thanks!" (Thanks!)

~0

I don't think that PRs like this are a good use of reviewers' time, especially since this conflicts with a priority project. Minor refactors such as this cause code churn, conflict with other high priority PRs, and really only clean up the code style. It does not seem to meaningfully contribute to larger projects/goals of this project.

ACK 4ecfd3e

Merging as the 324 PRs need rebasing anyways. However I don't think we should have PRs like this in the future.

This change is the cause of:

 node0 stderr ==75935== Thread 25 b-msghand:
==75935== Conditional jump or move depends on uninitialised value(s)
==75935==    at 0x1955B8: _M_reset (optional:313)
==75935==    by 0x1955B8: ~_Optional_payload (optional:437)
==75935==    by 0x1955B8: ~_Optional_base (optional:508)
==75935==    by 0x1955B8: GetLocalAddress(CNode const&) (???:220)
==75935==    by 0x1956A3: GetLocalAddrForPeer(CNode&) (net.cpp:240)
==75935==    by 0x1D091F: MaybeSendAddr (net_processing.cpp:5259)
==75935==    by 0x1D091F: (anonymous namespace)::PeerManagerImpl::SendMessages(CNode*) (???:5453)
==75935==    by 0x1AA183: CConnman::ThreadMessageHandler() (net.cpp:2886)
==75935==    by 0x750627: operator() (std_function.h:591)
==75935==    by 0x750627: util::TraceThread(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>) (???:21)
==75935==    by 0x1B290F: __invoke_impl<void, void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>), const char *, (lambda at net.cpp:3231:71)> (invoke.h:61)
==75935==    by 0x1B290F: __invoke<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>), const char *, (lambda at net.cpp:3231:71)> (invoke.h:96)
==75935==    by 0x1B290F: _M_invoke<0UL, 1UL, 2UL> (std_thread.h:292)
==75935==    by 0x1B290F: operator() (std_thread.h:299)
==75935==    by 0x1B290F: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>), char const*, CConnman::Start(CScheduler&, CConnman::Options const&)::$_5> > >::_M_run() (std_thread.h:244)
==75935==    by 0x4C501BF: execute_native_thread_routine (thread.cc:104)
==75935==    by 0x4F85E37: start_thread (pthread_create.c:447)
==75935==    by 0x4FF0E5B: thread_start (clone.S:79)
==75935== 
{
   <insert_a_suppression_name_here>
   Memcheck:Cond
   fun:_M_reset
   fun:~_Optional_payload
   fun:~_Optional_base
   fun:_Z15GetLocalAddressRK5CNode
   fun:_Z19GetLocalAddrForPeerR5CNode
   fun:MaybeSendAddr
   fun:_ZN12_GLOBAL__N_115PeerManagerImpl12SendMessagesEP5CNode
   fun:_ZN8CConnman20ThreadMessageHandlerEv
   fun:operator()
   fun:_ZN4util11TraceThreadESt17basic_string_viewIcSt11char_traitsIcEESt8functionIFvvEE
   fun:__invoke_impl<void, void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>), const char *, (lambda at net.cpp:3231:71)>
   fun:__invoke<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>), const char *, (lambda at net.cpp:3231:71)>
   fun:_M_invoke<0UL, 1UL, 2UL>
   fun:operator()
   fun:_ZNSt6thread11_State_implINS_8_InvokerISt5tupleIJPFvSt17basic_string_viewIcSt11char_traitsIcEESt8functionIFvvEEEPKcZN8CConnman5StartER10CSchedulerRKNSE_7OptionsEE3$_5EEEEE6_M_runEv
   fun:execute_native_thread_routine
   fun:start_thread
   fun:thread_start
}
==75935== 
==75935== Exit program on first error (--exit-on-first-error=yes)

which currently happens when using Clang 17 on aarch64 with Valgrind 3.22.0. May be similar to #27741, however that is with GCC 12 on x86_64.

@fanquake,

• Which version of the code is having this error? I guess you saw it in some CI on "latest" master?
• How did you figure out that this change is the cause of the above?
• Is this reproducable and if yes, how?

At a cursory look the code seems ok 🙄

#27741 looks unrelated, or at least is a problem that happens somewhere else in the code.

Which version of the code is having this error?

This version, up until current master.

I guess you saw it in some CI on "latest" master?

No. I compiled and ran tests locally.

How did you figure out that this change is the cause of the above?

I looked at the error, and then bisected based on who had recently changed this line. The commit prior to this PR being merged does not produce any error.

Is this reproducable and if yes, how?

Yes. Compile using Clang 17.0.6 and run the functional tests under --valgrind (3.22.0). It may require being on an aarch64.

Thanks, continued the discussion at #29635