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(network): only send responded updates on handshake/ping/pong #3463

Merged
merged 1 commit into from
Feb 8, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Feb 3, 2022

Motivation

During the initial sync, Zebra sends a peer timestamp update to the address book for every inbound message.

This uses approximately:

  • 2 million address book update channel messages
  • 2 million time system calls
  • 2 million address book mutex locks

When I sync testnet from a few nodes on my local network, it takes about an hour to get to the tip.

Solution

I changed Zebra to send a peer timestamp update to the address book after the handshake, and for every inbound Ping or Pong message. This uses approximately 1 thousand address book update channel messages, time system calls, and address book mutex locks, a 2000x decrease.

After this change, when I sync testnet from a few nodes on my local network, it takes about 30-45 minutes to get to the tip.

This change is correct because:

  • Zebra's peer heartbeat task sends a ping every minute, and closes the connection if it doesn't get a pong within 20 seconds. So Zebra does at least one timestamp update in each MIN_PEER_RECONNECTION_DELAY interval. (Typically, there should be 1-3 per interval.)
  • The other peer should also send regular inbound pings, but we don't require them for correctness.

The impact of this change is that peer timestamps update every 1-2 minutes, rather than potentially hundreds of times per second. This might change Zebra's internal reconnection order slightly for some peers.

But Zebra already truncates peer timestamps to the nearest 30 seconds before sending them externally. So this change won't have much impact on the times that we send to other peers.

Mainnet sync time doesn't change on my machine. So it probably depends on network latency, block size, or verification CPU time.

Review

@jvff can review this PR.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

@teor2345 teor2345 added C-bug Category: This is a bug I-slow Problems with performance or responsiveness P-Optional ✨ A-network Area: Network protocol updates or fixes labels Feb 3, 2022
@teor2345 teor2345 requested a review from jvff February 3, 2022 22:20
@teor2345 teor2345 self-assigned this Feb 3, 2022
@codecov
Copy link

codecov bot commented Feb 3, 2022

Codecov Report

Merging #3463 (fb41d8c) into main (499ae89) will increase coverage by 0.10%.
The diff coverage is 73.94%.

@@            Coverage Diff             @@
##             main    #3463      +/-   ##
==========================================
+ Coverage   78.34%   78.44%   +0.10%     
==========================================
  Files         267      273       +6     
  Lines       31526    31771     +245     
==========================================
+ Hits        24698    24923     +225     
- Misses       6828     6848      +20     

@teor2345 teor2345 force-pushed the only-ping-updates-address-book branch from a174760 to fb41d8c Compare February 7, 2022 20:32
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Added a comment so it won't merge immediately :)

@teor2345
Copy link
Contributor Author

teor2345 commented Feb 8, 2022

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Feb 8, 2022

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request Feb 8, 2022
@mergify mergify bot merged commit 294ea7e into main Feb 8, 2022
@mergify mergify bot deleted the only-ping-updates-address-book branch February 8, 2022 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes C-bug Category: This is a bug I-slow Problems with performance or responsiveness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants