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

uses Merkle shreds in broadcast duplicates #35115

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

behzadnouri
Copy link
Contributor

@behzadnouri behzadnouri commented Feb 6, 2024

Problem

Testing with legacy shreds is not good.

Summary of Changes

Use Merkle shreds in broadcast duplicates.

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fddfc84) 81.6% compared to head (869f2f9) 81.6%.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #35115     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         830      830             
  Lines      225031   225031             
=========================================
- Hits       183748   183683     -65     
- Misses      41283    41348     +65     

@behzadnouri behzadnouri added automerge Merge this Pull Request automatically once CI passes and removed automerge Merge this Pull Request automatically once CI passes labels Feb 6, 2024
@@ -176,7 +176,7 @@ impl BroadcastRun for BroadcastDuplicatesRun {
None, // chained_merkle_root
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use broadcast_utils::get_chained_merkle_root_from_parent( similar to the other stages? or saving that for a separate pr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is done in #35058
But it is failing tests. So I extracted this out for now until the other one is debugged.

Copy link
Contributor

Choose a reason for hiding this comment

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

the test is probably relying on being able to send a different last shred to different nodes and creating a separate valid version of the block. I'm assuming that no longer works with merkle shreds haha.

Copy link
Contributor

Choose a reason for hiding this comment

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

can take a look at the test in the AM, this change looks fine to merge

@@ -194,7 +194,7 @@ impl BroadcastRun for BroadcastDuplicatesRun {
None, // chained_merkle_root
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly chaining this to the previous fec set

@behzadnouri behzadnouri merged commit 7a95e4f into solana-labs:master Feb 7, 2024
35 checks passed
@behzadnouri behzadnouri deleted the merkle-duplicates branch February 7, 2024 16:02
@behzadnouri behzadnouri added the v1.18 PRs that should be backported to v1.18 label Feb 22, 2024
Copy link
Contributor

mergify bot commented Feb 22, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify bot pushed a commit that referenced this pull request Feb 22, 2024
The  commit migrates away from legacy shreds in duplicate shreds tests.

(cherry picked from commit 7a95e4f)
mergify bot added a commit that referenced this pull request Feb 22, 2024
#35290)

uses Merkle shreds in broadcast duplicates (#35115)

The  commit migrates away from legacy shreds in duplicate shreds tests.

(cherry picked from commit 7a95e4f)

Co-authored-by: behzad nouri <behzadnouri@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.18 PRs that should be backported to v1.18
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants