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

[network/p2p] Redesign Push Gossip #2772

Merged
merged 60 commits into from Feb 29, 2024
Merged

[network/p2p] Redesign Push Gossip #2772

merged 60 commits into from Feb 29, 2024

Conversation

patrick-ogrady
Copy link
Contributor

@patrick-ogrady patrick-ogrady commented Feb 26, 2024

Related: ava-labs/coreth#497

Why this should be merged

A network incident was caused by network amplification due to re-push gossip. This removes push gossip for transactions learned about over the p2p network.

How this works

  • Initially pushing a transaction to the network is now expected to be done solely by the node issuing the transaction.
  • Nodes will still perform pull gossip to fully spread the transaction throughout the network.
  • For simplicity, this change also removes the legacy push gossip mechanisms from all primary network chains.
  • All critical parameters to this algorithm can be parameterized by the node issuing transactions. The default gossip values are intentionally set fairly conservatively, and can be modified if E2E TTF of transactions is longer than expected.

The added parameters to the P-chain and X-chain network configs are:

  • push-gossip-discarded-cache-size
  • push-gossip-max-regossip-frequency
  • push-gossip-frequency

The added parameters to the C-chain config are:

  • push-gossip-frequency
  • pull-gossip-frequency

How this was tested

  • Additional unit tests
  • CI

StephenButtolph and others added 4 commits February 27, 2024 16:08
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Copy link
Contributor

@StephenButtolph StephenButtolph left a comment

Choose a reason for hiding this comment

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

Once we have a coreth rc + finalize the default values - this LGTM

@StephenButtolph StephenButtolph changed the title [network/p2p] Cleanup Gossip Implementation [network/p2p] Redesign Push Gossip Feb 28, 2024
@StephenButtolph StephenButtolph added this pull request to the merge queue Feb 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 29, 2024
@StephenButtolph StephenButtolph added this pull request to the merge queue Feb 29, 2024
Merged via the queue into master with commit c5da946 Feb 29, 2024
17 checks passed
@StephenButtolph StephenButtolph deleted the gossip-cleanup branch February 29, 2024 03:12
@atinst
Copy link

atinst commented Mar 2, 2024

Does this in a sense reduce network security?

mboben pushed a commit to mboben/avalanchego that referenced this pull request Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants