The nScore field in LocalServiceInfo could overflow during version handshakes that wrap from INT_MAX to INT_MIN triggering undefined behavior in address selection
SeenLocal() now saturates nScore at std::numeric_limits <int>::max(), so further increments stay capped
The test LocalAddress_nScore_Overflow covers the saturation behavior
This should fix the issue #24049
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34028.
See the guideline for information on the review process. A summary of reviews will appear here.
Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):
2026-02-12 11:39:44
319@@ -320,7 +320,10 @@ bool SeenLocal(const CService& addr)
320 LOCK(g_maplocalhost_mutex);
321 const auto it = mapLocalHost.find(addr);
322 if (it == mapLocalHost.end()) return false;
323- ++it->second.nScore;
324+
325+ if (it->second.nScore < std::numeric_limits<int>::max()) {
I just ran a mutation test for this PR, here’s a mutant that wasn’t killed and might be worth addressing (writing tests that can detect this change):
0diff --git a/src/net.cpp b/src/net.cpp
1index bbe24c8d3e..ab159e0123 100644
2--- a/src/net.cpp
3+++ b/src/net.cpp
4@@ -321,7 +321,7 @@ bool SeenLocal(const CService& addr)
5 const auto it = mapLocalHost.find(addr);
6 if (it == mapLocalHost.end()) return false;
7
8- if (it->second.nScore < std::numeric_limits<int>::max()) {
9+ if (it->second.nScore > std::numeric_limits<int>::max()) {
10 ++it->second.nScore;
11 }
12 return true;
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/actions/runs/20009656888/job/58081216641
LLM reason (✨ experimental): Lint check failed due to trailing whitespace detected (trailing_whitespace) in the source, causing the docker lint run to exit with code 1.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Could turn into draft while CI is red?
@codeabysss Are you still working on this?
yes still working on this
824+ BOOST_CHECK(SeenLocal(addr));
825+ BOOST_CHECK_EQUAL(GetnScore(addr), std::numeric_limits<int>::max());
826+ }
827+
828+ RemoveLocal(addr);
829+ g_reachable_nets.Remove(NET_IPV4);
This line needs to go, or other tests might fail because they assume it’s there.
(Failure depends on the order tests run in, which is semi-random)
97@@ -102,6 +98,9 @@ static constexpr bool DEFAULT_V2_TRANSPORT{true};
98
99 typedef int64_t NodeId;
100
101+/** Get the score of a local address. */
102+int GetnScore(const CService& addr);
A couple comments:
It seems like some of the wording in the OP was copied from the other PR #33072 and I think it would be better if you put things in your own words (my personal preference). Discussion in the other PR also mentioned that this has little to do with memory efficiency since it’s just 4 bytes at the end of the day. I think the commit message could also be cleaned up a bit with a short title and then a descriptive paragraph underneath.
The nScore field would wrap from INT_MAX to INT_MIN when incremented
repeatedly, causing undefined behavior.
Saturate nScore at INT_MAX in SeenLocal() to prevent overflow.
Add test for saturation behavior.
Signed-off-by: codeabysss <codeabysss@proton.me>