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(p2p): avoid receiving duplicated blocks #818

Merged
merged 11 commits into from
May 15, 2024

Conversation

srene
Copy link
Contributor

@srene srene commented May 7, 2024

PR Standards

This PR is aimed at preventing the reception of duplicated blocks from p2p network, by updating Gossipsub router configuration. It also prevents adding blocks to cache when already received. This improves performance and syncing times, because receiving too many packets from p2p may block applying blocks received from DA.

Opening a pull request should be able to meet the following requirements


Close #795 #803
<-- Briefly describe the content of this pull request -->

For Author:

  • Targeted PR against correct branch
  • included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@srene srene requested a review from a team as a code owner May 7, 2024 15:03
@srene srene self-assigned this May 7, 2024
block/manager.go Show resolved Hide resolved
@srene srene requested a review from mtsitrin May 8, 2024 13:32
@srene srene marked this pull request as draft May 8, 2024 15:38
@srene srene marked this pull request as ready for review May 8, 2024 15:40
@srene srene marked this pull request as draft May 8, 2024 15:43
Copy link

codecov bot commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.83%. Comparing base (851b312) to head (a4c4ae8).
Report is 15 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #818      +/-   ##
==========================================
- Coverage   40.44%   39.83%   -0.62%     
==========================================
  Files         120      118       -2     
  Lines       18935    18740     -195     
==========================================
- Hits         7659     7465     -194     
- Misses      10201    10226      +25     
+ Partials     1075     1049      -26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@srene srene marked this pull request as ready for review May 9, 2024 07:29
@srene srene marked this pull request as draft May 9, 2024 07:43
@srene srene force-pushed the srene/795-avoid-receiving-duplicated-blocks branch from 446fc64 to 22ddcf6 Compare May 9, 2024 07:46
@srene srene marked this pull request as ready for review May 9, 2024 08:45
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

not sure if I understood what is the point of this pr?
There was too much demand for the mutex before?

block/manager.go Show resolved Hide resolved
@srene
Copy link
Contributor Author

srene commented May 9, 2024

not sure if I understood what is the point of this pr? There was too much demand for the mutex before?

it avoids receiving extra duplicates from gossipsub protocol by passing extra options in p2p/client.go, this reduces the demand of the mutex. also it avoids adding already received blocks to the cache, in case it happens.

@srene srene requested a review from danwt May 10, 2024 07:27
p2p/client.go Outdated

ps, err := pubsub.NewGossipSub(ctx, c.Host)
//We add WithSeenMessagesTTL (with 1 year time) option to avoid ever requesting already seen blocks
ps, err := pubsub.NewGossipSub(ctx, c.Host, pubsub.WithSeenMessagesTTL(8760*time.Hour))
Copy link
Contributor

Choose a reason for hiding this comment

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

can u explain this 1 year time? previously u had
time.Duration(int(c.blockTime.Nanoseconds()*int64(c.conf.GossipCacheSize))))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i removed the ttl option and replaced gossipsub version with a new version where seen packets are not expiring

@mtsitrin mtsitrin self-requested a review May 12, 2024 07:07
block/manager.go Show resolved Hide resolved
@srene srene requested a review from danwt May 13, 2024 10:48
block/manager.go Outdated Show resolved Hide resolved
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

nice!

@srene srene linked an issue May 14, 2024 that may be closed by this pull request
@omritoptix
Copy link
Contributor

@mtsitrin can we merge?

@mtsitrin mtsitrin merged commit feb40f2 into main May 15, 2024
5 of 6 checks passed
@mtsitrin mtsitrin deleted the srene/795-avoid-receiving-duplicated-blocks branch May 15, 2024 09:38
omritoptix pushed a commit that referenced this pull request May 18, 2024
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.

applying cached blocks received from p2p is slow discard duplicated packets received via gossip
4 participants