Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: correctly handle peer updates while waiting to reconnect #4052

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

msgmaxim
Copy link
Contributor

Pull Request

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

In existing code it is hard to differentiate between "unknown peer" and "peer scheduled for reconnection, entry temporarily removed", so now active_connections stores ConnectionState enum with ReconnectionScheduled as a placeholder for when we don't have an actual ZMQ socket. This made it more clear what edge cases we have around reconnecting and how to best handle them, leading to the following fixes/improvements:

  • when trying to send a message we no longer incorrectly log "Peer not registered" if in fact we are just reconnecting to it
  • we now correctly clean up the state after a node that deregisters while we are waiting to reconnect to it (and the reconnection will be cancelled too)
  • when node updates their peer info record while we are waiting to reconnect to it, it used to be possible that old state wasn't correctly cleared, but now it is done correctly

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #4052 (9d7ad00) into main (d69ebb6) will increase coverage by 0%.
Report is 1 commits behind head on main.
The diff coverage is 52%.

@@          Coverage Diff          @@
##            main   #4052   +/-   ##
=====================================
  Coverage     72%     72%           
=====================================
  Files        369     369           
  Lines      59322   59301   -21     
  Branches   59322   59301   -21     
=====================================
+ Hits       42676   42678    +2     
+ Misses     14528   14503   -25     
- Partials    2118    2120    +2     
Files Coverage Δ
engine/src/p2p/core/socket.rs 92% <ø> (-1%) ⬇️
engine/src/p2p/core.rs 80% <52%> (-2%) ⬇️

... and 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@msgmaxim msgmaxim merged commit b540e42 into main Sep 28, 2023
44 checks passed
@msgmaxim msgmaxim deleted the fix/p2p-reconnection-states branch September 28, 2023 23:58
syan095 added a commit that referenced this pull request Oct 3, 2023
* origin/main:
  Feat: don't include dust btc amounts on rotation (#4063)
  chore: update dependency and config.toml for RUSTSEC-2023-0065 (#4066)
  fix: loop_select conditions (PRO-587) (#4061)
  chore: remove unused config items (#4064)
  feat: size limit for CCM (#4015)
  fix: warn -> info (#4060)
  Fix: correctly handle peer updates while waiting to reconnect (#4052)

# Conflicts:
#	state-chain/chains/src/lib.rs
dandanlen pushed a commit that referenced this pull request Oct 4, 2023
fix: better handling of p2p reconnection states
dandanlen pushed a commit that referenced this pull request Oct 4, 2023
fix: better handling of p2p reconnection states
dandanlen pushed a commit that referenced this pull request Oct 9, 2023
fix: better handling of p2p reconnection states
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants