This simple refactoring PR has the goal to improve the readability of the Misbehaving method by
score_before and score_now (to avoid repeatedly calculating the former)
tACK https://github.com/bitcoin/bitcoin/pull/22495/commits/9459fe5ae7690bf26c410567a16c7139812924ea
Agreed that the code looks cleaner than before.
Verified p2p_invalid_messages and p2p_filter test producing expected misbehavior.
1281 | @@ -1282,12 +1282,18 @@ void PeerManagerImpl::Misbehaving(const NodeId pnode, const int howmuch, const s 1282 | LOCK(peer->m_misbehavior_mutex); 1283 | peer->m_misbehavior_score += howmuch; 1284 | const std::string message_prefixed = message.empty() ? "" : (": " + message); 1285 | - if (peer->m_misbehavior_score >= DISCOURAGEMENT_THRESHOLD && peer->m_misbehavior_score - howmuch < DISCOURAGEMENT_THRESHOLD) { 1286 | - LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d) DISCOURAGE THRESHOLD EXCEEDED%s\n", pnode, peer->m_misbehavior_score - howmuch, peer->m_misbehavior_score, message_prefixed); 1287 | + 1288 | + const int score_before{peer->m_misbehavior_score - howmuch};
How about initializing this variable before incrementing m_misbehavior_score (so you're not adding howmuch and then subtracting it):
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index f89ecc21a8..bee7d5f241 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1280,11 +1280,12 @@ void PeerManagerImpl::Misbehaving(const NodeId pnode, const int howmuch, const s
if (peer == nullptr) return;
LOCK(peer->m_misbehavior_mutex);
- peer->m_misbehavior_score += howmuch;
- const std::string message_prefixed = message.empty() ? "" : (": " + message);
- const int score_before{peer->m_misbehavior_score - howmuch};
+ const int score_before{peer->m_misbehavior_score};
+ peer->m_misbehavior_score += howmuch;
const int score_now{peer->m_misbehavior_score};
+
+ const std::string message_prefixed = message.empty() ? "" : (": " + message);
std::string warning{""};
if (score_now >= DISCOURAGEMENT_THRESHOLD && score_before < DISCOURAGEMENT_THRESHOLD) {
Good idea, done. Agree that it's more logical to only read howmuch only once for the purpose of modifying the score.
1281 | @@ -1282,12 +1282,18 @@ void PeerManagerImpl::Misbehaving(const NodeId pnode, const int howmuch, const s 1282 | LOCK(peer->m_misbehavior_mutex); 1283 | peer->m_misbehavior_score += howmuch; 1284 | const std::string message_prefixed = message.empty() ? "" : (": " + message); 1285 | - if (peer->m_misbehavior_score >= DISCOURAGEMENT_THRESHOLD && peer->m_misbehavior_score - howmuch < DISCOURAGEMENT_THRESHOLD) { 1286 | - LogPrint(BCLog::NET, "Misbehaving: peer=%d (%d -> %d) DISCOURAGE THRESHOLD EXCEEDED%s\n", pnode, peer->m_misbehavior_score - howmuch, peer->m_misbehavior_score, message_prefixed); 1287 | + 1288 | + const int score_before{peer->m_misbehavior_score - howmuch}; 1289 | + const int score_now{peer->m_misbehavior_score}; 1290 | + std::string warning{""};
No need to explicitly initialize this to an empty string. std::string's default initializer creates and empty string:
std::string warning;
Thanks, done.
- introduce constant variables `score_before` and
`score_after` in order to improve readability
- deduplicate calls to LogPrint(), eliminates else-branch
Force-pushed with the suggestions by jnewbery (https://github.com/bitcoin/bitcoin/pull/22495#discussion_r676447854, #22495 (review)). Thanks to all reviewers so far!
utACK 8858e88c840197cdcabea07dd1380ef2aa4ece02