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

Replace periodic push gossip with pull gossip for block discovery #2365

Closed
wants to merge 15 commits into from

Conversation

StephenButtolph
Copy link
Contributor

@StephenButtolph StephenButtolph commented Nov 23, 2023

Why this should be merged

See a good write-up on push+pull gossip vs solely push gossip here: ava-labs/coreth#266. In practice, nodes already perform some amount of push+pull gossip (push happens with queries and pull happens with chits). However if a node hasn't learned about a new block, then the node will never start the querying process. Perviously, we relied on other peers in the network to push Put messages to the node to learn of new blocks to accept.

How this works

This replaces the periodic gossiping of Put messages with periodically sending GetAcceptedFrontier messages.

How this was tested

  • CI
  • Running on Fuji (with modified code to drop Put messages to ensure un-upgraded nodes weren't allowing the node to progress).

@StephenButtolph StephenButtolph added consensus This involves consensus networking This involves networking labels Nov 23, 2023
@StephenButtolph StephenButtolph added this to the v1.10.17 milestone Nov 23, 2023
@StephenButtolph StephenButtolph self-assigned this Nov 23, 2023
@StephenButtolph
Copy link
Contributor Author

This PR currently stops periodically gossiping Put messages. We should keep sending Put messages until we are guaranteed that other nodes in the network no longer rely on them.

chains/manager.go Outdated Show resolved Hide resolved
@StephenButtolph StephenButtolph changed the base branch from dev to add-new-ttf-metric November 24, 2023 15:50
fs.Uint(ConsensusAppConcurrencyKey, constants.DefaultConsensusAppConcurrency, "Maximum number of goroutines to use when handling App messages on a chain")
fs.Duration(ConsensusShutdownTimeoutKey, constants.DefaultConsensusShutdownTimeout, "Timeout before killing an unresponsive chain")
fs.Duration(ConsensusGossipAcceptedFrontierFrequencyKey, constants.DefaultAcceptedFrontierGossipFrequency, "Frequency of polling and gossiping accepted frontiers")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Felt like this should be next to the related configs

Base automatically changed from add-new-ttf-metric to dev November 24, 2023 17:45
@StephenButtolph
Copy link
Contributor Author

Closing in favor of #2367 (which shows an ~500ms TTF improvement over this PR)

@dhrubabasu dhrubabasu deleted the replace-periodic-gossip branch November 28, 2023 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus This involves consensus networking This involves networking
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants