[p2p] Fix signed integer overflow in LocalServiceInfo::nScore #33072

1. 
   b-l-u-e commented at 1:33 am on July 28, 2025: none

   Problem

   The nScore field in LocalServiceInfo struct was defined as int (32-bit signed integer), which could overflow from INT_MAX (2,147,483,647) to INT_MIN (-2,147,483,648) when incremented by SeenLocal() during version handshakes. This is undefined behavior in C++ and could affect address selection logic.

   Solution

   Implement saturation in SeenLocal() to cap nScore at std::numeric_limits<int>::max() instead of allowing overflow. This prevents undefined behavior while maintaining memory efficiency and logical correctness - after 2 billion connections, the matter is settled.

   Testing

   • Added test case LocalAddress_nScore_Overflow that verifies saturation behavior
   • Test passes, confirming the fix works as expected
   • Backward compatible - no breaking changes

   Impact

   • Prevents undefined behavior from integer overflow
   • Maintains memory efficiency (keeps int instead of int64_t)
   • Logically correct - no need for values beyond 2 billion

   Fixes #24049

2. 
   DrahtBot commented at 1:34 am on July 28, 2025: 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/33072.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   No conflicts as of last run.

3. DrahtBot added the label CI failed on Jul 28, 2025
4. 
   DrahtBot commented at 1:37 am on July 28, 2025: contributor

   🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/46822277337 LLM reason (✨ experimental): The CI failure is caused by a lint check detecting trailing whitespace errors.

   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.

5. b-l-u-e force-pushed on Jul 28, 2025
6. b-l-u-e force-pushed on Jul 28, 2025
7. 
   gmaxwell commented at 4:16 am on July 28, 2025: contributor

   This doesn’t compile, and why not just saturate? e.g. if(nScore < std::max<int>)nScore++

   “No performance impact (64-bit integers are efficient on modern systems)” sounds like LLM blather, the relevant criteria is not the performance of 64-bit integers considering how little it’s used. Memory usage might matter if this was a per-peer (or per-block/header) field, but it isn’t.

   The purpose of it is to track input addresses to see that they were, in fact, reachable. I’m pretty sure that by the 2 billionth connection claiming to connect to it that the matter is settled.

8. fix: prevent signed integer overflow in LocalServiceInfo::nScore using saturation c5c16adf0f
9. b-l-u-e force-pushed on Jul 28, 2025
10. 
    b-l-u-e commented at 10:43 am on July 28, 2025: none

    @gmaxwell Thank you for the excellent feedback! The saturation is a much better approach than changing to int64_t. I’ve implemented the saturation fix as you suggested using if (nScore < std::numeric_limits::max()) nScore++. This prevents undefined behavior while maintaining memory efficiency and logical correctness after 2 billion connections, the matter is indeed settled. I’ve also added a test case to verify the saturation behavior works correctly. Thank you for pointing me in the right direction!
11. DrahtBot removed the label CI failed on Jul 28, 2025
12. 
    achow101 commented at 3:54 pm on October 22, 2025: member

    Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

    Marking as up for grabs.

13. achow101 closed this on Oct 22, 2025

    ---

14. achow101 added the label Up for grabs on Oct 22, 2025

Contributors
b-l-u-e DrahtBot gmaxwell achow101

Labels
Up for grabs