[net processing] Reduce cs_main scope in MaybeDiscourageAndDisconnect()

jnewbery commented at 8:34 pm on July 8, 2020: member

The motivation for this PR is to reduce the scope of cs_main locking in misbehavior logic. It is the first set of commits from a larger branch to move the misbehavior data out of CNodeState and into a new struct that doesn’t take cs_main.

There are some very minor behavior changes in this branch, such as:

Not checking for discouragement/disconnect in ProcessMessages() (and instead relying on the following check in SendMessages())
Checking for discouragement/disconnect as the first action in SendMessages() (and not doing ping message sending first)
Continuing through SendMessages() if MaybeDiscourageAndDisconnect() doesn’t disconnect the peer (rather than dropping out of SendMessages()

jnewbery renamed this:
~~[net processing] Reduce cs_main scope MaybeDiscourageAndDisconnect()~~
[net processing] Reduce cs_main scope in MaybeDiscourageAndDisconnect()
on Jul 8, 2020

DrahtBot added the label P2P on Jul 8, 2020

DrahtBot commented at 11:32 pm on July 8, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19316 ([net] Cleanup logic around connection types by amitiuttarwar)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jnewbery force-pushed on Jul 9, 2020

in src/net_processing.cpp:3569 in 5722b338d0 outdated

3571 bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode)
3572 {
3573-    AssertLockHeld(cs_main);
3574-    CNodeState &state = *State(pnode.GetId());
3575+    {
3576+        LOCK(cs_main);

MarcoFalke commented at 9:53 am on July 9, 2020:

0net_processing.cpp:3574:9: error: acquiring mutex 'cs_main' that is already held [-Werror,-Wthread-safety-analysis]
1
2        LOCK(cs_main);
3
4        ^

MarcoFalke commented at 9:55 am on July 9, 2020:

You might have to use clang++ to reproduce locally

jnewbery commented at 10:38 am on July 9, 2020:

Ah. I didn’t remove the lock annotation from the function declaration. Fixed.

jonatack commented at 12:03 pm on July 9, 2020:

Saw this warning as well @ 5722b338d. Now building at 37e726579.

jnewbery force-pushed on Jul 9, 2020

in src/net_processing.cpp:3882 in edd1a8a5c7 outdated

3882@@ -3881,8 +3883,6 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3883     {
3884         LOCK(cs_main);
3885 
3886-        if (MaybeDiscourageAndDisconnect(*pto)) return true;

promag commented at 11:23 am on July 9, 2020:

edd1a8a5c76a15174985fe0a1a876eba9e7ee33c

Do you have an idea why this wasn’t in the top?

jnewbery commented at 1:59 pm on July 9, 2020:

Because it used to be SendRejectsAndCheckIfBanned(), and before that it was just sending rejects. Those required cs_main, which is taken after the ping processing at the top.

I’ve added that explanation to the commit log.

in src/net_processing.cpp:3588 in 37e7265796 outdated

3602+
3603+    if (pnode.HasPermission(PF_NOBAN)) {
3604+        // Log but don't disconnect
3605+        LogPrintf("Warning: not punishing whitelisted peer %s!\n", pnode.addr.ToString());
3606+        return false;
3607+    } else if (pnode.m_manual_connection) {

promag commented at 11:28 am on July 9, 2020:

37e7265796102df35ec83b53fe9114e07d214910

Maybe drop these else since you early return on each block.

jnewbery commented at 2:26 pm on July 9, 2020:

Agree. That’s much clearer.

in src/net_processing.cpp:3577 in 37e7265796 outdated

3596-            if (m_banman) {
3597-                m_banman->Discourage(pnode.addr);
3598-            }
3599-            connman->DisconnectNode(pnode.addr);
3600-        }
3601+    } // cs_main

promag commented at 11:30 am on July 9, 2020:

37e7265796102df35ec83b53fe9114e07d214910

nit, I only find this comment useful when the block is big enough.

jnewbery commented at 2:26 pm on July 9, 2020:

That’s fair, but I don’t think it does any harm.

in src/net_processing.cpp:3582 in 37e7265796 outdated

3601+    } // cs_main
3602+
3603+    if (pnode.HasPermission(PF_NOBAN)) {
3604+        // Log but don't disconnect
3605+        LogPrintf("Warning: not punishing whitelisted peer %s!\n", pnode.addr.ToString());
3606+        return false;

promag commented at 11:31 am on July 9, 2020:

37e7265796102df35ec83b53fe9114e07d214910

No test for this right?

jnewbery commented at 2:28 pm on July 9, 2020:

It seems that feature_maxuploadtarget.py tests the -noban permission (by verifying that a peer stays connected after exceeding the banscore.

promag commented at 11:35 am on July 9, 2020: member

Code LGTM 37e7265796102df35ec83b53fe9114e07d214910, some comments though.

jnewbery commented at 2:28 pm on July 9, 2020: member

Thanks for the review @promag . I’ve addressed your comments.

jnewbery force-pushed on Jul 9, 2020

in src/net_processing.cpp:3595 in c3dde2b52a outdated

3609+    if (pnode.m_manual_connection) {
3610+        // Peer is a manual connection - log but don't disconnect
3611+        LogPrintf("Warning: not punishing manually-connected peer %s!\n", pnode.addr.ToString());
3612+        return false;
3613+    }
3614+

jonatack commented at 4:29 pm on July 9, 2020:

The whitespace linter needs appeasing, looks like extra whitespace in lines 3589 and 3595.

0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1
2@@ -3573,16 +3582,18 @@ bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode)
3
4+
5
6+
7
8^---- failure generated from test/lint/lint-whitespace.sh

(which won’t prevent me from reviewing tomorrow am, so no rush)

jnewbery force-pushed on Jul 9, 2020

in src/net_processing.cpp:3569 in f54af5e586 outdated

3562@@ -3563,32 +3563,48 @@ void ProcessMessage(
3563     return;
3564 }
3565 
3566+/** Maybe disconnect a peer and discourage future connections from its address.
3567+ *
3568+ * @param[in]   pnode           The node to check.
3569+ * @return                      True if the peer will be disconnected.

dongcarl commented at 8:03 pm on July 9, 2020:

What about something along the lines of “True if the peer was marked for disconnection in this function”?

jnewbery commented at 8:20 pm on July 9, 2020:

Sure. Happy to make that change if I need to retouch this PR.

jonatack commented at 11:28 am on July 10, 2020:

Agree with @dongcarl. Maybe “True if the peer is marked for disconnection in this function.”

jnewbery commented at 12:29 pm on July 10, 2020:

done

dongcarl commented at 8:03 pm on July 9, 2020: member

Code Review ACK f54af5e58649d527d53990259f77892ad07f7ffa! Happy to see 2020-06-cs-main-split starting to be PR’d :relaxed:

For a minute, I was wondering if it’d be appropriate to move the MaybeDiscourageAndDisconnect up to ThreadMessageHandler, then realized that would involve changing NetEventsInterface so nope.

in src/net_processing.cpp:3604 in f54af5e586 outdated

3619         return true;
3620     }
3621-    return false;
3622+
3623+    // Normal case: Disconnect the peer and discourage all nodes sharing the address
3624+    LogPrintf("Disconnecting and discouraging peer %s!\n", pnode.addr.ToString());

jonatack commented at 11:08 am on July 10, 2020:

f54af5e nit suggestion while refactoring this function

 0@@ -3581,27 +3581,29 @@ bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode)
 1         state.m_should_discourage = false;
 2     } // cs_main
 3 
 4+    std::string peer{pnode.addr.ToString()};
 5
 6-        LogPrintf("Warning: not punishing whitelisted peer %s!\n", pnode.addr.ToString());
 7+        LogPrintf("Warning: not punishing whitelisted peer %s!\n", peer);
 8 
 9-        LogPrintf("Warning: not punishing manually-connected peer %s!\n", pnode.addr.ToString());
10+        LogPrintf("Warning: not punishing manually-connected peer %s!\n", peer);
11 
12local address
13-        LogPrintf("Warning: disconnecting but not discouraging local peer %s!\n", pnode.addr.ToString());
14+        LogPrintf("Warning: disconnecting but not discouraging local peer %s!\n", peer);
15 
16-    LogPrintf("Disconnecting and discouraging peer %s!\n", pnode.addr.ToString());
17+    LogPrintf("Disconnecting and discouraging peer %s!\n", peer);

jnewbery commented at 12:28 pm on July 10, 2020:

Done (but called the variable peer_name since a future commit adds a Peer object to this function.

in src/net_processing.cpp:3586 in f54af5e586 outdated

3600-        }
3601+    } // cs_main
3602+
3603+    if (pnode.HasPermission(PF_NOBAN)) {
3604+        // Peer has the NOBAN permission flag - log but don't disconnect
3605+        LogPrintf("Warning: not punishing whitelisted peer %s!\n", pnode.addr.ToString());

jonatack commented at 11:10 am on July 10, 2020:

f54af5e58649d527d53990259f77892ad07f7ffa maybe log “NOBAN|noban permission (flag)”

0        LogPrintf("Warning: not punishing peer with NOBAN permission %s!\n", pnode.addr.ToString());

jnewbery commented at 12:28 pm on July 10, 2020:

I expect this PR will need to be rebased on #19474, so I’ve copied the log text from there.

in src/net_processing.cpp:3592 in f54af5e586 outdated

3606+        return false;
3607+    }
3608+
3609+    if (pnode.m_manual_connection) {
3610+        // Peer is a manual connection - log but don't disconnect
3611+        LogPrintf("Warning: not punishing manually-connected peer %s!\n", pnode.addr.ToString());

jonatack commented at 11:21 am on July 10, 2020:

f54af5e58649d527d nit while touching this code, perhaps fix up the logging output: s/manually-connected/manually connected/

jnewbery commented at 12:29 pm on July 10, 2020:

done

in src/net_processing.cpp:3858 in f54af5e586 outdated

3862-        if (!pto->fSuccessfullyConnected || pto->fDisconnect)
3863-            return true;
3864 
3865-        // If we get here, the outgoing message serialization version is set and can't change.
3866-        const CNetMsgMaker msgMaker(pto->GetSendVersion());
3867+    if (MaybeDiscourageAndDisconnect(*pto)) return true;

jonatack commented at 11:45 am on July 10, 2020:

38c96ef in this commit that removes MaybeDiscourageAndDisconnect() from ProcessMessages(), future reviewers may appreciate a comment here in the remaining call to make the order dependency explicit.

e.g. that MaybeDiscourageAndDisconnect() here depends on all misbehavior calls (currently Misbehaving) occurring in ProcessMessages which needs to remain called before SendMessages in CConnman::ThreadMessageHandler… if I am not confused.

jnewbery commented at 12:44 pm on July 10, 2020:

nMisbehavior is a tally in CNodeState that can be incremented from anywhere. That almost always happens inside a ProcessMessages() call (because we increment the misbehavior score when receiving a bad messages from a peer), but not always. See, for example, the call to MaybePunishNodeForBlock() inside BlockChecked(), which is an asynchronous callback from the validation interface, executed on the scheduler thread.

As long as MaybeDiscourageAndDisconnect() is called regularly for the node, then the misbehavior score exceeding the 100 threshold will eventually result in the peer being punished. It doesn’t really matter where that MaybeDiscourageAndDisconnect() happens, but it makes most sense in SendMessages() which is where we do general peer housekeeping/maintenance.

jonatack commented at 6:28 am on July 11, 2020:

Thanks, @jnewbery, makes sense.

The previous behavior before this PR had me wondering if there were particular optimisation reasons for it, but I don’t see any.

jonatack commented at 11:58 am on July 10, 2020: member

ACK f54af5e58649 with a few comments below.

Commit message in f54af5e58649: If we don’t disconnect a peer in MaybeDiscourageAndDisconnect because it has NOBAN permissions or it’s a manual connection, continue SendMessages processing rather than exiting early -> Was this a bit of a bug? EDIT: the former behavior does not seem to have been an anti-DoS optimisation.
Commit message 38c96ef Misbehaving()… is only called in ProcessMessages() -> as it’s called from MaybePunishNodeForBlock, MaybePunishNodeForTx, SendBlockTransactions, ProcessHeadersMessage, perhaps write “Misbehaving() calls originate from callers in ProcessMessages()…”

Feel free to ignore the suggestions below.

jnewbery force-pushed on Jul 10, 2020

jnewbery commented at 12:50 pm on July 10, 2020: member

Thanks for the review @jonatack. I’ve addressed your inline comments. For your commit log comments:

Commit message in f54af5e: If we don’t disconnect a peer in MaybeDiscourageAndDisconnect because it has NOBAN permissions or it’s a manual connection, continue SendMessages processing rather than exiting early -> Was this a bit of a bug?

Not a bug, but inefficient and confusing. I’ve updated the commit log to include that.

Commit message 38c96ef Misbehaving()… is only called in ProcessMessages() -> as it’s called from MaybePunishNodeForBlock, MaybePunishNodeForTx, SendBlockTransactions, ProcessHeadersMessage, perhaps write “Misbehaving() calls originate from callers in ProcessMessages()…”

I’ve now realized that this commit log was incorrect (see #19472 (review)). I’ve updated the log to be more accurate. Let me know what you think

DrahtBot added the label Needs rebase on Jul 10, 2020

jnewbery force-pushed on Jul 10, 2020

jnewbery commented at 4:30 pm on July 10, 2020: member

rebased

[net processing] Change cs_main TRY_LOCK to LOCK in SendMessages()

This was changed to TRY_LOCK in #1117 to fix a potential deadlock
between cs_main and cs_vSend. cs_vSend was split into cs_vSend and
cs_sendProcessing in #9535 (and cs_sendProcessing was changed from a
TRY_LOCK to a LOCK in the same PR).

Since cs_vSend can no longer be taken before cs_main, revert this to a
LOCK().

This commit leaves part of the code with bad indentation. That is fixed
by the next (whitespace change only) commit.

1a1c23f8d4

[net processing] Fix bad indentation in SendMessages()

Hint for reviewers: review ignoring whitespace changes.

a1d5a428a2

DrahtBot removed the label Needs rebase on Jul 10, 2020

in src/net_processing.cpp:3594 in e874c6dd10 outdated

3614+        return false;
3615+    }
3616+
3617+    if (pnode.addr.IsLocal()) {
3618+        // Peer is on a local address. Disconnect this peer, but don't discourage the local address
3619+        LogPrintf("Warning: disconnecting but not discouraging local peer %s!\n", peer_name);

MarcoFalke commented at 8:22 pm on July 10, 2020:

I know this is unrelated, but is there any reason to not log the peer id? All other peer-related messages log the id, so logging the address+port makes it harder to relate this log to all other logs.

practicalswift commented at 9:18 pm on July 10, 2020:

Also, isn’t the general policy that we should try to avoid logging IP addresses (at least when the user is not opting in to detailed -debug=net logging)?

FWIW: IP addresses are considered personal data in some jurisdictions (EU via GDPR?) IIRC.

jnewbery commented at 6:09 am on July 11, 2020:

We’ve been logging the peer address for node misbehavior since banning was introduced: https://github.com/bitcoin/bitcoin/commit/15f3ad4dbdf0d245b936e68c51a7ecb9f472d2cd#diff-9a82240fe7dfe86564178691cc57f2f1R757 , and these logs have probably just persisted all that time.

I’ve changed this to log the peer id instead of the address.

jonatack commented at 6:49 am on July 11, 2020:

Wow! – banscore goes way back, TIL

MarcoFalke commented at 8:29 am on July 11, 2020:

Thanks for fixing!

jnewbery force-pushed on Jul 11, 2020

[net processing] Only call MaybeDiscourageAndDisconnect from SendMessages

`nMisbehavior` is a tally in `CNodeState` that can be incremented from
anywhere. That almost always happens inside a `ProcessMessages()` call
(because we increment the misbehavior score when receiving a bad
messages from a peer), but not always. See, for example, the call to
`MaybePunishNodeForBlock()` inside `BlockChecked()`, which is an
asynchronous callback from the validation interface, executed on the
scheduler thread.

As long as `MaybeDiscourageAndDisconnect()` is called regularly for the
node, then the misbehavior score exceeding the 100 threshold will
eventually result in the peer being punished. It doesn't really matter
where that `MaybeDiscourageAndDisconnect()` happens, but it makes most
sense in `SendMessages()` which is where we do general peer
housekeeping/maintenance.

Therefore, remove the `MaybeDiscourageAndDisconnect()` call in
`ProcessMessages()` and move the `MaybeDiscourageAndDisconnect()` call
in `SendMessages()` to the top of the function. This moves it out of the
cs_main lock scope, so take that lock directly inside
`MaybeDiscourageAndDisconnect()`.

Historic note: `MaybeDiscourageAndDisconnect()` was previously
`SendRejectsAndCheckIfBanned()`, and before that was just sending
rejects.  All of those things required cs_main, which is why
`MaybeDiscourageAndDisconnect()` was called after the ping logic.

a49781e56d

jnewbery force-pushed on Jul 11, 2020

jonatack commented at 6:10 am on July 11, 2020: member

re-ACK a37331f per git range-diff 505b4ed f54af5e a37331f

[net processing] Continue SendMessages processing if not disconnecting peer

If we don't disconnect a peer in MaybeDiscourageAndDisconnect because it
has NOBAN permissions or it's a manual connection, continue SendMessages
processing rather than exiting early.

The previous behaviour was that we'd miss the SendMessages processing on
this iteration of the MessageHandler loop. That's not a problem since
SendMessages() would just be called again on the next iteration, but it
was slightly inefficient and confusing.

655b195747

jnewbery force-pushed on Jul 11, 2020

jonatack commented at 6:45 am on July 11, 2020: member

re-ACK 655b195 per git range-diff 505b4ed f54af5e 655b195, code/commit messages review, a bit of code history, and debug build.

Maybe (only if you have to retouch again):

0 bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode)
1 {
2-    NodeId peer_id{pnode.GetId()};
3+    const NodeId peer_id{pnode.GetId()};

in src/net_processing.cpp:3592 in 655b195747

3606+        LogPrintf("Warning: not punishing manually connected peer %d!\n", peer_id);
3607+        return false;
3608+    }
3609+
3610+    if (pnode.addr.IsLocal()) {
3611+        // Peer is on a local address. Disconnect this peer, but don't discourage the local address

MarcoFalke commented at 9:08 am on July 11, 2020:

style-nit in commit 655b195747:

I don’t like comments that repeat the source code like pnode.addr.IsLocal() // Peer is on a local address. Ideally source code should be self-documenting and in this case simply reading the condition and the logged format string should be sufficient to understand what is being done.

However, a comment could make sense to explain why something is done. I presume local addresses are not discouraged because all tor-proxy nodes run on the same local address?

(Same for other comments in this function)

I know that this comment has been there before, but since you refactor this function, the comments could be fixed as well.

jnewbery commented at 7:17 am on July 24, 2020:

Done in a follow-up: https://github.com/jnewbery/bitcoin/commits/2020-07-tidy-misbehavior

in src/net_processing.cpp:3570 in 655b195747

3567-    LOCK(cs_main);
3568-    CNodeState &state = *State(pnode.GetId());
3569+    NodeId peer_id{pnode.GetId()};
3570+    {
3571+        LOCK(cs_main);
3572+        CNodeState &state = *State(peer_id);

MarcoFalke commented at 9:10 am on July 11, 2020:

clang-format style-nit in commit 655b1957470c39bcab64917747c9f467444bd809

0        CNodeState& state = *State(peer_id);

jnewbery commented at 7:17 am on July 24, 2020:

Done in a follow-up: https://github.com/jnewbery/bitcoin/commits/2020-07-tidy-misbehavior

in src/net_processing.cpp:3838 in 1a1c23f8d4 outdated

3837@@ -3838,7 +3838,7 @@ class CompareInvMempoolOrder
3838 bool PeerLogicValidation::SendMessages(CNode* pto)

MarcoFalke commented at 9:16 am on July 11, 2020:

Style-nit in commit body of c581cc16bb5e490c0960ccf440de2d1e5f23c417:

Instead of a GitHub-branded #1117, which either needs online-access to GitHub or two local commands to show the referenced commit (git log --grep='#1117 ' --merges), what about simply saying commit c581cc16bb.

Same for the other one, but since it has two commits, maybe the merge commit can be mentioned commit 82274c0

jnewbery commented at 7:18 am on July 24, 2020:

Thanks. Will refer directly to commit hashes in future.

MarcoFalke approved

MarcoFalke commented at 9:18 am on July 11, 2020: member

ACK 655b195747 only some style-nits 🚁

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 655b195747 only some style-nits 🚁
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhTIwv/U4oykZ05nHVcYquBiztxhgGyKa9f8uaOypMsTYR6eEWe6L5jcOi7wDYc
 80YHHVelm/057+8BOihx6BtYoiStnOzewawMsh2CNXYZPxs9fEJyP5lbTRVLpRN2S
 9GSwEnXTlrg6N8cZtncmfZRG6XSR9YwwLvS3FXwReTBr9DCE5SPeFz/kUzXin0z2Z
10mRG58cYJW4FiYSfRqvd34c3gI8bbJJC9Fz55tc8ZdgXVM9evVeUGQCr1F/cOTbYi
11cd0gAkMNbaUuEk3KzO0rK+1iK5oFNo+FCNcU8XG1i1au6tnd+fEBqWOxGJ/TA8VB
12m4eI3GHoC4chH4jaOskIYPgSkC5v4ubMV6lImvK3rvud67+5gi/2ZILLxLwjpdCN
13KAAKqJTQDGduGgcdt6Tezk1XGmwoJ+CV/sl2dH9INWrEWTsqy9ZQ2sr9uPRPSFva
14znujKqFX9MhNkm+tS7XCsRNuJRr3zJN5x4jq9K/tjcCUnfOcCYOJUCmQ2wMpWyBo
152pMOadPJ
16=ZRAY
17-----END PGP SIGNATURE-----

Timestamp of file with hash cedbe81897bd1c68d1b2c18d961a7b55b239097a710335927096fe5482ada3fd -

promag commented at 10:06 pm on July 12, 2020: member

Code review ACK 655b1957470c39bcab64917747c9f467444bd809.

laanwj added this to the "Blockers" column in a project

jnewbery commented at 7:16 am on July 24, 2020: member

Thanks for the review @MarcoFalke . Rather than invalidate the three ACKs on this branch, I’ve added a commit to the next branch that addresses your comments: https://github.com/jnewbery/bitcoin/tree/2020-07-tidy-misbehavior.

I think this PR may be ready for merge soon.

jnewbery commented at 7:21 am on July 24, 2020: member

@jonatack

Maybe (only if you have to retouch again):

bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode) …

Done in follow-up: https://github.com/jnewbery/bitcoin/commits/2020-07-tidy-misbehavior

in src/net_processing.cpp:3841 in 1a1c23f8d4 outdated

3837@@ -3838,7 +3838,7 @@ class CompareInvMempoolOrder
3838 bool PeerLogicValidation::SendMessages(CNode* pto)
3839 {
3840     const Consensus::Params& consensusParams = Params().GetConsensus();
3841-    {
3842+

ariard commented at 2:36 pm on July 24, 2020:

For future work, do we still need cs_sendProcessing ? It only guards m_next_addr_send/m_next_local_addr_send, both of them only RW in SendMessages, inside ThreadMessageHandler.

Is this a relic from the past or do you see another reason ?

jnewbery commented at 2:53 pm on July 24, 2020:

I’ve removed it from m_next_addr_send and m_next_local_addr_send in https://github.com/jnewbery/bitcoin/commits/2020-06-cs-main-split. It could theoretically also be removed from guarding SendMessages() since we only ever call that from the message handler thread.

ariard commented at 2:36 pm on July 24, 2020: member

Code Review ACK 655b195

laanwj merged this on Jul 24, 2020

laanwj closed this on Jul 24, 2020

jnewbery deleted the branch on Jul 24, 2020

jnewbery commented at 3:48 pm on July 24, 2020: member

Thanks for review everyone. The next PR is #19583 (including the suggested fixups by @jonatack and @MarcoFalke)

sidhujag referenced this in commit 1188a57dd5 on Jul 24, 2020

laanwj removed this from the "Blockers" column in a project

theuni commented at 6:10 pm on July 24, 2020: member

I’m worried that we may end up spending a more significant amount of time in SendMessages now, whereas before it was opportunistic. Do you have any idea what the TRY_LOCK hit/miss ratio looked like for a busy node over a few days?

I’d also be curious to know what the cpu usage/profile of a busy node looks like before/after this change.

jnewbery commented at 6:44 pm on July 24, 2020: member

Thanks for looking at this @theuni!

Do you have any idea what the TRY_LOCK hit/miss ratio looked like for a busy node over a few days?

I don’t know exactly but I’d expect it to be very high (ie we almost always take the lock). What other threads are you expecting to be holding cs_main on a regular basis?

MarcoFalke commented at 7:44 am on July 25, 2020: member

I guess this depends on the use case and what other modules are used that might aggressively poll cs_main. (GUI, wallet, indices, RPC, …)

deadalnix referenced this in commit 7749e8d4b3 on Jan 6, 2021

DrahtBot locked this on Feb 15, 2022

[net processing] Reduce cs_main scope in MaybeDiscourageAndDisconnect() #19472

Conflicts