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

Gossip recently verified block hashes to peers #2729

Merged
merged 13 commits into from
Oct 7, 2021
Merged

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Sep 3, 2021

Motivation

Zebra needs to broadcast verified block hashes to its peers, so they learn about new blocks.

Designs

Grow and Reset chain tip changes should be sent in block gossips.

Tasks can wait until they are near the tip using SyncStatus::wait_until_close_to_tip:

/// Wait until the synchronization is likely close to the tip.
///
/// Returns an error if communication with the synchronizer is lost.
pub async fn wait_until_close_to_tip(&mut self) -> Result<(), watch::error::RecvError> {

Tasks can await tip changes using ChainTipChange::tip_change and TipAction:

pub async fn tip_change(&mut self) -> Result<TipAction, watch::error::RecvError> {

Solution

  • wait for state best tip changes
  • wait until Zebra is close to the tip
  • make sure we have the latest tip change
  • broadcast each change to all ready peers
  • test that committed blocks are gossiped to peers

Closes #2712.

Testing

Here are the manual tests I did:

  • run on an ephemeral instance: no gossips during initial sync
  • run on a synced instance: gossips after the 100 non-finalized blocks are synced
  • looked at the outbound inv messages using the Grafana dashboard: more inv messages than before this change

Review

@jvff or @oxarbitrage can review this PR.

Reviewer Checklist

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

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement P-Medium A-network Area: Network protocol updates or fixes labels Sep 3, 2021
@teor2345 teor2345 requested review from jvff and upbqdn September 3, 2021 05:01
@teor2345 teor2345 added P-Low and removed P-Medium labels Sep 3, 2021
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Looking good so far. I added one idea about how to try to simplify the code.

zebrad/src/components/sync/gossip.rs Outdated Show resolved Hide resolved
@mpguerra mpguerra linked an issue Sep 6, 2021 that may be closed by this pull request
@mpguerra mpguerra modified the milestone: 2021 Sprint 18 Sep 6, 2021
Base automatically changed from mempool-activation to main September 6, 2021 23:33
@teor2345 teor2345 changed the base branch from main to block-locator-tip-panic September 24, 2021 00:57
@teor2345 teor2345 changed the base branch from block-locator-tip-panic to block-locator-chain-tip-panics September 24, 2021 05:14
@teor2345 teor2345 removed the request for review from upbqdn September 24, 2021 05:26
@teor2345 teor2345 force-pushed the block-gossip branch 2 times, most recently from 618248c to 3a23a40 Compare September 30, 2021 03:34
@teor2345 teor2345 changed the base branch from block-locator-chain-tip-panics to main September 30, 2021 03:34
@teor2345 teor2345 removed the P-Low label Sep 30, 2021
@teor2345 teor2345 marked this pull request as ready for review October 5, 2021 05:08
@teor2345
Copy link
Contributor Author

teor2345 commented Oct 5, 2021

I manually tested this PR with:

  • fully synced and new ephemeral nodes
  • on testnet and mainnet

And it works without panicking (so #2789 and #2800 have not regressed).

@teor2345 teor2345 added the P-Low label Oct 6, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented Oct 6, 2021

(This is lower priority, because it's not blocking any other work.)

oxarbitrage
oxarbitrage previously approved these changes Oct 6, 2021
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Looks great to me, i made just a few very minor comments. Feel free to merge here.

zebrad/src/components/inbound/tests.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound/tests.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound/tests.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound/tests.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound/tests.rs Show resolved Hide resolved
zebrad/src/components/inbound/tests.rs Outdated Show resolved Hide resolved
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 A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broadcast AdvertiseBlock to peers when we commit a new block
4 participants