net, refactor: remove unneeded exports, use helpers over low-level code, use optional

jonatack commented at 5:27 pm on July 14, 2023: contributor

and other improvements noticed while reviewing #27411.

Addresses #27411 (review) and #27411 (review).

See commit messages for details.

DrahtBot commented at 5:27 pm on July 14, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, stickies-v, achow101

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.

jonatack renamed this:
~~net, refactor: remove unneeded exports, improve separation, use std::optional~~
net, refactor: remove unneeded exports, use helpers over low-level code, use optional
on Jul 14, 2023

jonatack marked this as ready for review on Jul 14, 2023

bitcoin deleted a comment on Jul 15, 2023

in src/net.cpp:2915 in b252918caa outdated

2910@@ -2915,10 +2911,8 @@ uint64_t CConnman::CalculateKeyedNetGroup(const CAddress& address) const
2911     return GetDeterministicRandomizer(RANDOMIZER_ID_NETGROUP).Write(vchNetGroup.data(), vchNetGroup.size()).Finalize();
2912 }
2913 
2914-void CaptureMessageToFile(const CAddress& addr,
2915-                          const std::string& msg_type,
2916-                          Span<const unsigned char> data,
2917-                          bool is_incoming)
2918+// Dump binary message to file, with timestamp.
2919+static void CaptureMessageToFile(const CAddress& addr, const std::string& msg_type, Span<const unsigned char> data, bool is_incoming)

stickies-v commented at 6:01 pm on July 17, 2023:

nit: not sure this is an improvement? I find the existing format easier to read, with less LoC change.

jonatack commented at 8:23 pm on July 17, 2023:

I made the declaration on one line for the smaller diff, and I don’t mind reading the parameter list left-to-right. But sure, kept it multi-line.

in src/net.cpp:175 in b252918caa outdated

172@@ -177,7 +173,8 @@ bool GetLocal(CService& addr, const CNode& peer)
173             }
174         }
175     }
176-    return nBestScore >= 0;
177+    if (nBestScore == -1) return std::nullopt;
178+    return addr;

stickies-v commented at 6:09 pm on July 17, 2023:

I think this can be simplified further?

 0diff --git a/src/net.cpp b/src/net.cpp
 1index 0d6b4f04a..c6c4c9a3e 100644
 2--- a/src/net.cpp
 3+++ b/src/net.cpp
 4@@ -149,7 +149,7 @@ uint16_t GetListenPort()
 5 [[nodiscard]] static std::optional<CService> GetLocal(const CNode& peer)
 6 {
 7     if (!fListen) return std::nullopt;
 8-    CService addr;
 9+    std::optional<CService> addr;
10     int nBestScore = -1;
11     int nBestReachability = -1;
12     {
13@@ -167,13 +167,12 @@ uint16_t GetListenPort()
14             int nReachability = entry.first.GetReachabilityFrom(peer.addr);
15             if (nReachability > nBestReachability || (nReachability == nBestReachability && nScore > nBestScore))
16             {
17-                addr = CService(entry.first, entry.second.nPort);
18+                addr.emplace(CService(entry.first, entry.second.nPort));
19                 nBestReachability = nReachability;
20                 nBestScore = nScore;
21             }
22         }
23     }
24-    if (nBestScore == -1) return std::nullopt;
25     return addr;
26 }
27

jonatack commented at 8:22 pm on July 17, 2023:

Very nice, done.

in src/net.cpp:209 in b252918caa outdated

204@@ -208,9 +205,8 @@ static std::vector<CAddress> ConvertSeeds(const std::vector<uint8_t> &vSeedsIn)
205 // one by discovery.
206 CService GetLocalAddress(const CNode& peer)
207 {
208-    CService addr;
209-    if (GetLocal(addr, peer)) {
210-        return addr;
211+    if (std::optional<CService> addr = GetLocal(peer)) {
212+        return addr.value();

stickies-v commented at 6:13 pm on July 17, 2023:

Entire function can be simplified to just:

0    return GetLocal(peer).value_or(CService{CNetAddr(), GetListenPort()});

jonatack commented at 8:22 pm on July 17, 2023:

Much better, thank you!

in src/netaddress.h:169 in b252918caa outdated

160@@ -161,9 +161,15 @@ class CNetAddr
161      */
162     bool SetSpecial(const std::string& addr);
163 
164+    [[nodiscard]] bool IsIPv4() const { return m_net == NET_IPV4; } // IPv4 mapped address (::FFFF:0:0/96, 0.0.0.0/0)
165+    [[nodiscard]] bool IsIPv6() const { return m_net == NET_IPV6; } // IPv6 address (not mapped IPv4, not Tor)
166+    [[nodiscard]] bool IsTor() const { return m_net == NET_ONION; }
167+    [[nodiscard]] bool IsI2P() const { return m_net == NET_I2P; }
168+    [[nodiscard]] bool IsCJDNS() const { return m_net == NET_CJDNS; }
169+    [[nodiscard]] bool IsTorOrI2P() const { return IsTor() || IsI2P(); }

stickies-v commented at 6:21 pm on July 17, 2023:

I don’t see the benefit of this helper function. It doesn’t really make things more readable imo (the underlying calls are short and clear enough for me). It also feels quite arbitrary: what about IsTorOrCJDNS(), IsI2POrCJDNS(), …?

jonatack commented at 8:31 pm on July 17, 2023:

Dropped the commit and instead added an IsPrivacyNet() helper for GetLocal() only, where it cleans things up. When we’ll likely need to add IsCJDNS() to it later on (when there are more nodes running it), it will be simple to do and will keep GetLocal() easy to read.

stickies-v commented at 10:48 am on July 18, 2023:

Yeah I think that’s a good approach.

stickies-v commented at 6:23 pm on July 17, 2023: contributor

Concept ACK but I think some changes are unnecessary churn

jonatack force-pushed on Jul 17, 2023

jonatack commented at 10:37 pm on July 17, 2023: contributor

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

in src/netaddress.h:170 in 5d7188ed92 outdated

165+    [[nodiscard]] bool IsIPv6() const { return m_net == NET_IPV6; } // IPv6 address (not mapped IPv4, not Tor)
166+    [[nodiscard]] bool IsTor() const { return m_net == NET_ONION; }
167+    [[nodiscard]] bool IsI2P() const { return m_net == NET_I2P; }
168+    [[nodiscard]] bool IsCJDNS() const { return m_net == NET_CJDNS; }
169+    [[nodiscard]] bool IsPrivacyNet() const { return IsTor() || IsI2P(); }
170+    [[nodiscard]] bool AddrHasCJDNSPrefix() const { return m_addr[0] == 0xfc; }

stickies-v commented at 10:32 am on July 18, 2023:

nit: would not include the ~classname in method name

0    [[nodiscard]] bool HasCJDNSPrefix() const { return m_addr[0] == 0xfc; }

stickies-v commented at 11:14 am on July 18, 2023:

Maybe let’s not move all these lines? A bit unnecessary churn, is fine as is imo.

jonatack commented at 5:07 pm on July 18, 2023:

Done

jonatack commented at 5:08 pm on July 18, 2023:

Done

in src/net.cpp:161 in 5d7188ed92 outdated

168-            if (our_net != peers_net &&
169-                (our_net == NET_ONION || our_net == NET_I2P ||
170-                 peers_net == NET_ONION || peers_net == NET_I2P)) {
171+            // For privacy reasons, don't advertise our privacy-network address to other
172+            // networks, and don't advertise our other-network address to privacy networks.
173+            // Note: consider adding IsCJDNS() to IsPrivacyNet() when there are more CJDNS peers;

stickies-v commented at 10:35 am on July 18, 2023:

nit: is “TODO” easier (i.e. more likely someone does it) to grep than “Note:”? Also, I think these last 2 lines are better placed near the definition of IsPrivacyNet().

jonatack commented at 5:07 pm on July 18, 2023:

Done

in src/net.cpp:163 in 5d7188ed92 outdated

170-                 peers_net == NET_ONION || peers_net == NET_I2P)) {
171+            // For privacy reasons, don't advertise our privacy-network address to other
172+            // networks, and don't advertise our other-network address to privacy networks.
173+            // Note: consider adding IsCJDNS() to IsPrivacyNet() when there are more CJDNS peers;
174+            // see https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1497176155
175+            if (entry.first.GetNetwork() != peer.ConnectedThroughNetwork()

stickies-v commented at 10:42 am on July 18, 2023:

As we’re now touching all these LoC, would suggest to clean up even further. Slightly increases the diff but worth it for readability imo.

 0diff --git a/src/net.cpp b/src/net.cpp
 1index eee179c8a..b35725ea2 100644
 2--- a/src/net.cpp
 3+++ b/src/net.cpp
 4@@ -154,21 +154,21 @@ uint16_t GetListenPort()
 5     int nBestReachability = -1;
 6     {
 7         LOCK(g_maplocalhost_mutex);
 8-        for (const auto& entry : mapLocalHost)
 9+        for (const auto& [local_addr, service_info] : mapLocalHost)
10         {
11             // For privacy reasons, don't advertise our privacy-network address to other
12             // networks, and don't advertise our other-network address to privacy networks.
13             // Note: consider adding IsCJDNS() to IsPrivacyNet() when there are more CJDNS peers;
14             // see [#27411 (comment)](/bitcoin-bitcoin/27411/#issuecomment-1497176155)
15-            if (entry.first.GetNetwork() != peer.ConnectedThroughNetwork()
16-                && (entry.first.IsPrivacyNet() || peer.addr.IsPrivacyNet() || peer.m_inbound_onion)) {
17+            if (local_addr.GetNetwork() != peer.ConnectedThroughNetwork()
18+                && (local_addr.IsPrivacyNet() || peer.addr.IsPrivacyNet() || peer.m_inbound_onion)) {
19                 continue;
20             }
21-            int nScore = entry.second.nScore;
22-            int nReachability = entry.first.GetReachabilityFrom(peer.addr);
23+            int nScore{service_info.nScore};
24+            int nReachability{local_addr.GetReachabilityFrom(peer.addr)};
25             if (nReachability > nBestReachability || (nReachability == nBestReachability && nScore > nBestScore))
26             {
27-                addr.emplace(CService{entry.first, entry.second.nPort});
28+                addr.emplace(CService{local_addr, service_info.nPort});
29                 nBestReachability = nReachability;
30                 nBestScore = nScore;
31             }

jonatack commented at 5:08 pm on July 18, 2023:

Nice! Took the liberty of adding a commit credited to you. LMK if that’s ok and if you’d like any changes.

stickies-v commented at 8:32 pm on July 18, 2023:

Absolutely, thanks. Just left some small comments.

stickies-v commented at 11:05 am on July 18, 2023: contributor

Approach ACK 5d7188ed92908237ce51bbe6ed0a97472825c6b8

jonatack force-pushed on Jul 18, 2023

in src/netaddress.h:192 in c69a74229d outdated

190     bool IsRoutable() const;
191     bool IsInternal() const;
192     bool IsValid() const;
193 
194+    /**
195+     * Whether this object is a privacy network for self-advertisement purposes in `GetLocal()`.

stickies-v commented at 8:31 pm on July 18, 2023:

nit: no need to specify where it’s used for a public method imo

0     * Whether this object is a privacy network.

jonatack commented at 5:54 pm on July 19, 2023:

nit: no need to specify where it’s used for a public method imo

Done.

in src/net.cpp:169 in 2d2575b92d outdated

178-            {
179-                addr = CService(entry.first, entry.second.nPort);
180-                nBestReachability = nReachability;
181-                nBestScore = nScore;
182+            const int reachability{local_addr.GetReachabilityFrom(peer.addr)};
183+            const int score{local_service_info.nScore};

stickies-v commented at 8:35 pm on July 18, 2023:

nit: any reason the order of these lines got switched? if not necessary, wouldn’t do that

jonatack commented at 8:44 pm on July 18, 2023:

Alphabetical order, first use order, and consistency (see the last use).

jonatack commented at 5:55 pm on July 19, 2023:

Dropped these changes.

in src/net.cpp:154 in 2d2575b92d outdated

153+    if (!fListen) return false;
154 
155-    int nBestScore = -1;
156-    int nBestReachability = -1;
157+    int best_reachability = -1;
158+    int best_score = -1;

stickies-v commented at 8:36 pm on July 18, 2023:

nit: not sure the snake case is worth the added LoC (+further down). Would prefer to keep as is.

jonatack commented at 5:55 pm on July 19, 2023:

Dropped the snakecase updates.

luke-jr commented at 9:11 pm on July 18, 2023: member

I thought we were going to avoid unproductive refactors?

jonatack commented at 10:36 pm on July 18, 2023: contributor

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!)

in src/net.cpp:163 in c69a74229d outdated

171-            const Network peers_net{peer.ConnectedThroughNetwork()};
172-            if (our_net != peers_net &&
173-                (our_net == NET_ONION || our_net == NET_I2P ||
174-                 peers_net == NET_ONION || peers_net == NET_I2P)) {
175+            if (local_addr.GetNetwork() != peer.ConnectedThroughNetwork()
176+                && (local_addr.IsPrivacyNet() || peer.addr.IsPrivacyNet() || peer.m_inbound_onion)) {

vasild commented at 12:34 pm on July 19, 2023:

For remote peers we really need to combine the IsPrivacyNet() call with || peer.m_inbound_onion which looks easy to miss in the future, what about introducing also CNode::IsPrivacyNet() which does return addr.IsPrivacyNet() || m_inbound_onion;? Or at least a mention in the CNetAddr::IsPrivacyNet() doc/comment.

jonatack commented at 5:51 pm on July 19, 2023:

Good idea! Done.

in src/netaddress.h:182 in c69a74229d outdated

178@@ -179,14 +179,22 @@ class CNetAddr
179     bool IsRFC6052() const; // IPv6 well-known prefix for IPv4-embedded address (64:FF9B::/96)
180     bool IsRFC6145() const; // IPv6 IPv4-translated address (::FFFF:0:0:0/96) (actually defined in RFC2765)
181     bool IsHeNet() const;   // IPv6 Hurricane Electric - https://he.net (2001:0470::/36)
182-    bool IsTor() const;
183-    bool IsI2P() const;
184-    bool IsCJDNS() const;
185+    [[nodiscard]] bool IsTor() const { return m_net == NET_ONION; }

vasild commented at 12:54 pm on July 19, 2023:

This is ok as it is and I do not insist to change it. My personal preference, feel free to ignore it, is to separate declaration (interface) from definition (implementation). This makes the former easier to read by an user who only needs to call the interface without being bothered with implementation details.

Inlining is desirable for performance purposes, does this change have any impact on the performance?

Both out-of-line definition and inlining are achievable:

 0class CNetAddr
 1{
 2    ...
 3    [[nodiscard]] bool IsTor() const;
 4    ...
 5}
 6
 7...
 8
 9inline bool CNetAddr::IsTor() const
10{
11    return m_net == NET_ONION;
12}

jonatack commented at 5:51 pm on July 19, 2023:

Done, changed from implicit in the declarations to explicit in the definitions.

jonatack commented at 6:15 pm on July 19, 2023:

Done, changed from implicit in the declarations to explicit in the definitions.

The CI is unhappy with that, so reverting the last commit to c69a74229da514228d3388e9652d2ea2e89224d7.

vasild approved

vasild commented at 12:55 pm on July 19, 2023: contributor

ACK c69a74229da514228d3388e9652d2ea2e89224d7

Good changes, easy to review. Thanks!

About the commit messages: Move IsPeerAddrLocalGood() from header to implementation it was in the .cpp file before (it was not in the header file). Better: make IsPeerAddrLocalGood() private in net.cpp since it is not used outside of that file. Same for Move CaptureMessageToFile() from header to implementation and Move GetLocal() from header to implementation.

Move IsPeerAddrLocalGood() declaration from header to implementation deccf1c484

Move CaptureMessageToFile() declaration from header to implementation 11426f6557

Move GetLocal() declaration from header to implementation 5ba73cd0ee

Add and use CNetAddr::HasCJDNSPrefix() helper fb4265747c

GetLocal() type-safety, naming, const, and formatting cleanups

Co-authored-by: Jon Atack <jon@atack.com>

df488563b2

jonatack force-pushed on Jul 19, 2023

jonatack commented at 6:00 pm on July 19, 2023: contributor

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

jonatack force-pushed on Jul 19, 2023

DrahtBot added the label CI failed on Jul 19, 2023

Add CNetAddr::IsPrivacyNet() and CNode::IsConnectedThroughPrivacyNet()

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

07f5891588

Use higher-level CNetAddr and CNode helpers in net.cpp

rather than low-level comparisons with Network enum values.

f1304db136

Convert GetLocal() to std::optional and remove out-param

Co-authored-by: stickies-v <stickies-v@protonmail.com>

5316ae5dd8

Inline short, often-called, rarely-changed basic CNetAddr getters

and make them nodiscard.

Member functions containing a few lines of code are usually inlined, either
implicitly by defining them in the declaration as done here, or declared inline.

References
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-inline
https://google.github.io/styleguide/cppguide#Inline_Functions
https://www.ibm.com/docs/en/i/7.1?topic=only-inline-member-functions-c

4ecfd3eaf4

jonatack force-pushed on Jul 19, 2023

DrahtBot removed the label CI failed on Jul 20, 2023

DrahtBot added the label CI failed on Jul 25, 2023

DrahtBot removed the label CI failed on Jul 27, 2023

fanquake commented at 9:09 am on August 1, 2023: member

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

There are 3 commits (5ba73cd0ee1e661ec4d041ac8ae7a60cfd31f0c0, df488563b280c63f5d74d5ac0fcb1a2cad489d55, 5316ae5dd8d90623f9bb883bb253fa6463ee4d34) 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 (review).

maflcko commented at 9:34 am on August 1, 2023: member

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

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.

vasild approved

vasild commented at 10:16 am on August 1, 2023: contributor

ACK 4ecfd3eaf434d868455466e001adae4b68515ab8

I am also ok to add new functions in the same commit in which they are going to be used.

vasild commented at 10:31 am on August 1, 2023: contributor

offtopic [[nodiscard]]

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

where ignoring the result is harmful, it probably means a bug in the program, for example write(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)
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.

jonatack commented at 7:35 pm on August 2, 2023: contributor

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!)

maflcko requested review from stickies-v on Aug 3, 2023

stickies-v approved

stickies-v commented at 10:20 am on August 3, 2023: contributor

ACK 4ecfd3eaf434d868455466e001adae4b68515ab8

That said:

I was ~0 on the [[nodiscard]] changes before, but am now in favour of removing all the ones in this PR. The only place where it made sense, imo, was in 5ba73cd0ee1e661ec4d041ac8ae7a60cfd31f0c0 for [[nodiscard]] static bool GetLocal(CService& addr, const CNode& peer) where it’s dangerous to use the out-parameter without checking the return value. Now, in all cases, the functions are entirely self explanatory and I don’t really see any footguns. Keeping [[nodiscard]] for (potentially) dangerous places and catching inefficiences with clang-tidy e.a. seems like a much more productive way over filling up the codebase with [[nodiscard]] statements that mostly just make it less legible.
I’d prefer dropping the last commit 4ecfd3eaf434d868455466e001adae4b68515ab8, it doesn’t really add anything and could be considered arbitrary (for new code I’d slightly prefer inlining it as done in this PR, but I can see other people preferring the current, and then it’s not worth the churn imo)

achow101 assigned achow101 on Sep 20, 2023

achow101 commented at 10:48 am on September 21, 2023: member

~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.

achow101 unassigned achow101 on Sep 21, 2023

achow101 commented at 3:26 pm on September 21, 2023: member

ACK 4ecfd3eaf434d868455466e001adae4b68515ab8

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

achow101 merged this on Sep 21, 2023

achow101 closed this on Sep 21, 2023

jonatack deleted the branch on Sep 21, 2023

Frank-GER referenced this in commit c395fd12c5 on Sep 25, 2023

sidhujag referenced this in commit 5728391033 on Sep 26, 2023

fanquake commented at 11:10 am on March 12, 2024: member

This change is the cause of:

 0 node0 stderr ==75935== Thread 25 b-msghand:
 1==75935== Conditional jump or move depends on uninitialised value(s)
 2==75935==    at 0x1955B8: _M_reset (optional:313)
 3==75935==    by 0x1955B8: ~_Optional_payload (optional:437)
 4==75935==    by 0x1955B8: ~_Optional_base (optional:508)
 5==75935==    by 0x1955B8: GetLocalAddress(CNode const&) (???:220)
 6==75935==    by 0x1956A3: GetLocalAddrForPeer(CNode&) (net.cpp:240)
 7==75935==    by 0x1D091F: MaybeSendAddr (net_processing.cpp:5259)
 8==75935==    by 0x1D091F: (anonymous namespace)::PeerManagerImpl::SendMessages(CNode*) (???:5453)
 9==75935==    by 0x1AA183: CConnman::ThreadMessageHandler() (net.cpp:2886)
10==75935==    by 0x750627: operator() (std_function.h:591)
11==75935==    by 0x750627: util::TraceThread(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>) (???:21)
12==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)
13==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)
14==75935==    by 0x1B290F: _M_invoke<0UL, 1UL, 2UL> (std_thread.h:292)
15==75935==    by 0x1B290F: operator() (std_thread.h:299)
16==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)
17==75935==    by 0x4C501BF: execute_native_thread_routine (thread.cc:104)
18==75935==    by 0x4F85E37: start_thread (pthread_create.c:447)
19==75935==    by 0x4FF0E5B: thread_start (clone.S:79)
20==75935== 
21{
22   <insert_a_suppression_name_here>
23   Memcheck:Cond
24   fun:_M_reset
25   fun:~_Optional_payload
26   fun:~_Optional_base
27   fun:_Z15GetLocalAddressRK5CNode
28   fun:_Z19GetLocalAddrForPeerR5CNode
29   fun:MaybeSendAddr
30   fun:_ZN12_GLOBAL__N_115PeerManagerImpl12SendMessagesEP5CNode
31   fun:_ZN8CConnman20ThreadMessageHandlerEv
32   fun:operator()
33   fun:_ZN4util11TraceThreadESt17basic_string_viewIcSt11char_traitsIcEESt8functionIFvvEE
34   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)>
35   fun:__invoke<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>), const char *, (lambda at net.cpp:3231:71)>
36   fun:_M_invoke<0UL, 1UL, 2UL>
37   fun:operator()
38   fun:_ZNSt6thread11_State_implINS_8_InvokerISt5tupleIJPFvSt17basic_string_viewIcSt11char_traitsIcEESt8functionIFvvEEEPKcZN8CConnman5StartER10CSchedulerRKNSE_7OptionsEE3$_5EEEEE6_M_runEv
39   fun:execute_native_thread_routine
40   fun:start_thread
41   fun:thread_start
42}
43==75935== 
44==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.

vasild commented at 1:48 pm on March 12, 2024: contributor

@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 :roll_eyes:

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

fanquake commented at 1:56 pm on March 12, 2024: member

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.

vasild commented at 2:22 pm on March 12, 2024: contributor

Thanks, continued the discussion at https://github.com/bitcoin/bitcoin/issues/29635

net, refactor: remove unneeded exports, use helpers over low-level code, use optional #28078

Reviews

Conflicts