net/p2p: change nScore and nBestScore data types to int64_t #24090

Changing nScore and nBestScore to a fixed-width integer guarantees to be the same size on any architecture and avoids UB.

Addresses issue: #24049

Concept ACK

It makes sense to convert nScore to int64_t from int to avoid UB. However, I shall take a further look in the codebase to check if doing so might not be causing a yet unforeseen issue.

Concept ACK

81d20b8e2f0097867b234bdc02cb2d7aa95b4a51

Changed nBestScore type to int64_t per @mzumsande comment.

Concept ACK

It makes sense to convert nScore to int64_t from int to avoid UB. However, I shall take a further look in the codebase to check if doing so might not be causing a yet unforeseen issue.

The scope of nBestScore is limited to the function:

bool GetLocal(CService& addr, const CNetAddr *paddrPeer)

https://github.com/bitcoin/bitcoin/blob/81d20b8e2f0097867b234bdc02cb2d7aa95b4a51/src/net.cpp#L137

which returns a boolean based on the evaluation:

return nBestScore >= 0;

crACK 81d20b8

Changing nScore and nBestScore to a fixed-width integer guarantees to be the same size on any architecture and avoids UB.

Since the type of nScore is changed to int64_t, I think it’s logical to change the return type of GetnScore from int to int64_t.

Line 194 in src/net.cpp:

static int GetnScore(const CService& addr)
{
    LOCK(cs_mapLocalHost);
    const auto it = mapLocalHost.find(addr);
    return (it != mapLocalHost.end()) ? it->second.nScore : 0;
}

1e0703baaab58915f43831c32bfd789015a8d483

updated function:

static int GetnScore(const CService& addr)

return type to int64_t per @shaavan comment.

Concept and code-review ACK 1e0703baaab58915f43831c32bfd789015a8d483

yes - bravo to @shaavan for his thorough code review!

reACK 1e0703b

ACK 1e0703baaab58915f43831c32bfd789015a8d483

Changes since my last review:

• Changed the return type of GetnScore from int to int64_t to match the nScore's type.

An alternative would be to saturate the int on overflow

agree - clamping the int is more prudent for memory usage.

@w0xlt - I like the idea of saturating (clamping) this variable - open to suggestions/patches. Not sure which size is appropriate.

Can rebase and use SaturatingAdd?

Saturating means once an IP reaches max, it won't ever get swapped out for a newer IP, right?

Maybe instead, when we would increment beyond max, we should subtract the lowest (>LOCAL_MAX) current score from all mapLocalHost entries? I think that would preserve the current behaviour.

guarantees to be the same size

Does this matter?

@luke-jr - if you have an idea - post a patch - I will apply it.

ACK 1e0703baaab58915f43831c32bfd789015a8d483 code review, rebased, debug build, ran unit tests.

That said, changing the one line that increments nScore in SeenLocal() to use SaturatingAdd (recently added in #24224) looks to me like a preferable and more minimal change.

An additional reason might be that if any math operations to nScore are added in the future, the C++ tendency to auto-convert to int might be a good reason to leave nScore as int here, if I'm not confused and lacking caffeine.

My thought is something like this:

--- a/src/net.cpp
+++ b/src/net.cpp
@@ -349,6 +349,20 @@ bool SeenLocal(const CService& addr)
     LOCK(g_maplocalhost_mutex);
     const auto it = mapLocalHost.find(addr);
     if (it == mapLocalHost.end()) return false;
+    if (it->second.nScore == std::numeric_limits<int>::max()) {
+        int lowest_inc_score = std::numeric_limits<int>::max();
+        for (auto& localinfo : mapLocalHost) {
+            if (localinfo.second.nScore > LOCAL_MAX && localinfo.second.nScore < lowest_inc_score) {
+                lowest_inc_score = localinfo.second.nScore;
+            }
+        }
+        lowest_inc_score -= LOCAL_MAX;
+        for (auto& localinfo : mapLocalHost) {
+            if (localinfo.second.nScore > LOCAL_MAX) {
+                localinfo.second.nScore -= lowest_inc_score;
+            }
+        }
+    }
     ++it->second.nScore;
     return true;
 }

But I'm not 100% sure of it yet.

My thought is something like this:

--- a/src/net.cpp
+++ b/src/net.cpp
@@ -349,6 +349,20 @@ bool SeenLocal(const CService& addr)
     LOCK(g_maplocalhost_mutex);
     const auto it = mapLocalHost.find(addr);
     if (it == mapLocalHost.end()) return false;
+    if (it->second.nScore == std::numeric_limits<int>::max()) {
+        int lowest_inc_score = std::numeric_limits<int>::max();
+        for (auto& localinfo : mapLocalHost) {
+            if (localinfo.second.nScore > LOCAL_MAX && localinfo.second.nScore < lowest_inc_score) {
+                lowest_inc_score = localinfo.second.nScore;
+            }
+        }
+        lowest_inc_score -= LOCAL_MAX;
+        for (auto& localinfo : mapLocalHost) {
+            if (localinfo.second.nScore > LOCAL_MAX) {
+                localinfo.second.nScore -= lowest_inc_score;
+            }
+        }
+    }
     ++it->second.nScore;
     return true;
 }

But I'm not 100% sure of it yet.

Does the code inside the if (INT_MAX == it->second.nscore) need to have linear ([or] is it binomial?) time complexity? You're updating EVERY node in a map because the nScore in ONE of the is equal to INT_MAX? If you only intend to keep the ordering: there are many ways to do it in constant, time.

All of that, and it happens to be a breaking change.

  ++reinterpret_cast<unsigned&>(it->second.nScore);

Keeps everything unchanged, gets rid of the undefined behavior making it (until c++20: implementation specified; since c++20: well-defined) behavior.

What do you think?

EDIT: Removed a paragraph since the problem I described in it couldn't happen without a data race, anyway.