net/p2p: change nScore and nBestScore data types to int64

RandyMcMillan commented at 8:29 pm on January 17, 2022: contributor

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

Addresses issue: #24049

DrahtBot added the label P2P on Jan 17, 2022

shaavan commented at 2:15 pm on January 18, 2022: contributor

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.

in src/net.cpp:143 in a1071e90b7 outdated

139@@ -140,7 +140,7 @@ bool GetLocal(CService& addr, const CNetAddr *paddrPeer)
140         LOCK(cs_mapLocalHost);
141         for (const auto& entry : mapLocalHost)
142         {
143-            int nScore = entry.second.nScore;
144+            int64_t nScore = entry.second.nScore;

mzumsande commented at 3:27 pm on January 18, 2022:

Because of the line nBestScore = nScore; below, I think that nBestScore should be changed to int64_t as well.

RandyMcMillan commented at 10:51 pm on January 18, 2022:

done - good catch! thanks!

mzumsande commented at 3:53 pm on January 18, 2022: contributor

Concept ACK

RandyMcMillan force-pushed on Jan 18, 2022

RandyMcMillan commented at 10:48 pm on January 18, 2022: contributor

81d20b8e2f0097867b234bdc02cb2d7aa95b4a51

Changed nBestScore type to int64_t per @mzumsande comment.

RandyMcMillan renamed this:
~~net/p2p: change nScore data type to int64_t~~
net/p2p: change nScore and nBestScore data types to int64_t
on Jan 18, 2022

RandyMcMillan commented at 11:03 pm on January 18, 2022: contributor

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:

0bool 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;

w0xlt approved

w0xlt commented at 3:29 am on January 19, 2022: contributor

crACK 81d20b8

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

shaavan commented at 2:35 pm on January 19, 2022: contributor

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:

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

src/net: change nScore data type to int64_t 1e0703baaa

RandyMcMillan force-pushed on Jan 19, 2022

RandyMcMillan commented at 4:06 pm on January 19, 2022: contributor

1e0703baaab58915f43831c32bfd789015a8d483

updated function:

0static int GetnScore(const CService& addr)

return type to int64_t per @shaavan comment.

theStack approved

theStack commented at 5:01 pm on January 19, 2022: contributor

Concept and code-review ACK 1e0703baaab58915f43831c32bfd789015a8d483

RandyMcMillan commented at 5:23 pm on January 19, 2022: contributor

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

w0xlt approved

w0xlt commented at 5:36 pm on January 19, 2022: contributor

reACK 1e0703b

shaavan approved

shaavan commented at 5:20 am on January 20, 2022: contributor

ACK 1e0703baaab58915f43831c32bfd789015a8d483

Changes since my last review:

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

maflcko commented at 10:31 am on January 24, 2022: member

An alternative would be to saturate the int on overflow

RandyMcMillan commented at 6:06 pm on January 28, 2022: contributor

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

RandyMcMillan marked this as a draft on Jan 30, 2022

RandyMcMillan commented at 10:29 pm on January 30, 2022: contributor

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

maflcko referenced this in commit e44423c9d3 on Feb 21, 2022

maflcko commented at 2:52 pm on February 21, 2022: member

Can rebase and use SaturatingAdd?

luke-jr commented at 5:21 am on March 10, 2022: member

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?

RandyMcMillan commented at 5:23 am on March 10, 2022: contributor

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

jonatack commented at 10:02 am on March 10, 2022: contributor

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.

luke-jr commented at 3:50 pm on March 10, 2022: member

My thought is something like this:

 0--- a/src/net.cpp
 1+++ b/src/net.cpp
 2@@ -349,6 +349,20 @@ bool SeenLocal(const CService& addr)
 3     LOCK(g_maplocalhost_mutex);
 4     const auto it = mapLocalHost.find(addr);
 5     if (it == mapLocalHost.end()) return false;
 6+    if (it->second.nScore == std::numeric_limits<int>::max()) {
 7+        int lowest_inc_score = std::numeric_limits<int>::max();
 8+        for (auto& localinfo : mapLocalHost) {
 9+            if (localinfo.second.nScore > LOCAL_MAX && localinfo.second.nScore < lowest_inc_score) {
10+                lowest_inc_score = localinfo.second.nScore;
11+            }
12+        }
13+        lowest_inc_score -= LOCAL_MAX;
14+        for (auto& localinfo : mapLocalHost) {
15+            if (localinfo.second.nScore > LOCAL_MAX) {
16+                localinfo.second.nScore -= lowest_inc_score;
17+            }
18+        }
19+    }
20     ++it->second.nScore;
21     return true;
22 }

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

RandyMcMillan closed this on Mar 19, 2022

ViralTaco commented at 9:24 pm on November 2, 2022: none

My thought is something like this:

 0--- a/src/net.cpp
 1+++ b/src/net.cpp
 2@@ -349,6 +349,20 @@ bool SeenLocal(const CService& addr)
 3     LOCK(g_maplocalhost_mutex);
 4     const auto it = mapLocalHost.find(addr);
 5     if (it == mapLocalHost.end()) return false;
 6+    if (it->second.nScore == std::numeric_limits<int>::max()) {
 7+        int lowest_inc_score = std::numeric_limits<int>::max();
 8+        for (auto& localinfo : mapLocalHost) {
 9+            if (localinfo.second.nScore > LOCAL_MAX && localinfo.second.nScore < lowest_inc_score) {
10+                lowest_inc_score = localinfo.second.nScore;
11+            }
12+        }
13+        lowest_inc_score -= LOCAL_MAX;
14+        for (auto& localinfo : mapLocalHost) {
15+            if (localinfo.second.nScore > LOCAL_MAX) {
16+                localinfo.second.nScore -= lowest_inc_score;
17+            }
18+        }
19+    }
20     ++it->second.nScore;
21     return true;
22 }

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.

0  ++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.

bitcoin locked this on Nov 2, 2023

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