Remove peers from the node_id_to_descriptor even without init

[TheBlueMatt]

When a peer has finished the noise handshake, but has not yet
completed the lightning Init-based handshake, they will be
present in the node_id_to_descriptor set, even though
Peer::handshake_complete() returns false. Thus, when we go to
disconnect such a peer, we must ensure that we remove it from the
descriptor set as well.

Failing to do so caused an Inconsistent peers set state! panic in
the C bindings network handler.

[codecov-commenter]

Codecov Report

Patch coverage: 86.41% and project coverage change: +0.09 🎉

Comparison is base (8311581) 87.21% compared to head (157e496) 87.30%.

❗ Current head 157e496 differs from pull request most recent head 48946d9. Consider uploading reports for the commit 48946d9 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2060      +/-   ##
==========================================
+ Coverage   87.21%   87.30%   +0.09%     
==========================================
  Files         100      100              
  Lines       44516    45143     +627     
  Branches    44516    45143     +627     
==========================================
+ Hits        38825    39413     +588     
- Misses       5691     5730      +39     

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	60.69% <86.41%> (+2.28%)	⬆️
lightning/src/util/events.rs	25.50% <0.00%> (-0.58%)	⬇️
lightning-rapid-gossip-sync/src/processing.rs	93.12% <0.00%> (+0.29%)	⬆️
lightning/src/util/indexed_map.rs	80.95% <0.00%> (+2.00%)	⬆️
lightning-block-sync/src/rpc.rs	92.02% <0.00%> (+2.20%)	⬆️
lightning-rapid-gossip-sync/src/lib.rs	81.13% <0.00%> (+3.52%)	⬆️
lightning/src/util/ser.rs	88.06% <0.00%> (+4.29%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[wpaulino]

LGTM after squash

[TheBlueMatt]

Squashed, added one more assertion, and added a race-detection test.

[TheBlueMatt]

Oops, set the new test to std-only cause it uses threads.

[valentinewallace]

ACK with 1 comment

[wpaulino]

Was able to repro locally – LGTM after squash. +1 for adding a comment to the new test.

[TheBlueMatt]

Added a comment in the test and squashed:

$ git diff-tree -U1 ea88c3cf7 b3dc23aa8
diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index e6566cc0d..742bf3fdc 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -2309,2 +2309,5 @@ mod tests {
 	fn fuzz_threaded_connections() {
+		// Spawn two threads which repeatedly connect two peers together, leading to "got second
+		// connection with peer" disconnections and rapid reconnect. This previously found an issue
+		// with our internal map consistency, and is a generally godd smoke test of disconnection.
 		let cfgs = Arc::new(create_peermgr_cfgs(2));

[TheBlueMatt]

$ git diff-tree -U0 b3dc23aa 48946d9a
diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index 742bf3fdc..41bbf5b49 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -2312 +2312 @@ mod tests {
-		// with our internal map consistency, and is a generally godd smoke test of disconnection.
+		// with our internal map consistency, and is a generally good smoke test of disconnection.

[TheBlueMatt]

CI is being dumb, somehow the job got cancelled, but I'm just gonna merge and fix it later if anything breaks 🚀