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

blockstore: send duplicate proofs for chained merkle root conflicts #835

Merged
merged 4 commits into from
Apr 23, 2024

Conversation

AshwinSekar
Copy link

@AshwinSekar AshwinSekar commented Apr 16, 2024

alternative implementation of #102

Contributes to solana-labs#34897

@@ -542,12 +542,6 @@ impl Blockstore {
let candidate_erasure_set = ErasureSetId::new(slot, candidate_fec_set_index);
let candidate_erasure_meta: ErasureMeta = deserialize(candidate_erasure_meta.as_ref())?;

// Add this candidate to erasure metas to avoid blockstore lookup in future
Copy link
Author

Choose a reason for hiding this comment

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

no reason to do this as it is only called at the end of do_insert_shreds which does not need to perform any lookups after.

ledger/src/blockstore.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 96.20690% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (f133697) to head (48958a3).

Additional details and impacted files
@@           Coverage Diff            @@
##           master     #835    +/-   ##
========================================
  Coverage    81.9%    81.9%            
========================================
  Files         855      855            
  Lines      232134   232385   +251     
========================================
+ Hits       190134   190544   +410     
+ Misses      42000    41841   -159     

Copy link

@behzadnouri behzadnouri left a comment

Choose a reason for hiding this comment

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

overall this looks less convoluted that the previous code.

Comment on lines 128 to 129
/// Offset to the index of the first received coding shred
first_received_coding_shred_offset: usize,

Choose a reason for hiding this comment

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

Why do we need an "offset" here instead of just first_received_coding_shred_index?

Copy link
Author

Choose a reason for hiding this comment

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

I used an offset to avoid having to convert the index from u32 to usize, but I think it should be fine to just store the index directly.

Choose a reason for hiding this comment

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

For easier review, can you please move all ErasureMeta changes to a separate PR?

ShredType::Code,
);
let shred = Shred::new_from_serialized_shred(
self.get_shred_from_just_inserted_or_db(&just_inserted_shreds, shred_id)

Choose a reason for hiding this comment

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

just_inserted_shreds should always include this shred because this is a new ErasureMeta, no?

merkle_root_meta.first_received_shred_type(),
);
let shred = Shred::new_from_serialized_shred(
self.get_shred_from_just_inserted_or_db(&just_inserted_shreds, shred_id)

Choose a reason for hiding this comment

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

similarly here, shouldn't we expect shred to be already in just_inserted_shreds?

next fec set shred {next_erasure_set:?} type {:?} chains to merkle root {chained_merkle_root:?}.
Reporting as duplicate",
shred.shred_type(),
next_shred_type,

Choose a reason for hiding this comment

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

Copy link

mergify bot commented Apr 23, 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.

behzadnouri
behzadnouri previously approved these changes Apr 23, 2024
Comment on lines 1795 to 1809
let (next_shred_index, next_shred_type) = if let Some(merkle_root_meta_entry) =
merkle_root_metas.get(&next_erasure_set)
{
(
merkle_root_meta_entry.as_ref().first_received_shred_index(),
merkle_root_meta_entry.as_ref().first_received_shred_type(),
)
} else if let Some(merkle_root_meta) = self.merkle_root_meta(next_erasure_set).unwrap() {
(
merkle_root_meta.first_received_shred_index(),
merkle_root_meta.first_received_shred_type(),
)
} else {
// No shred from the next fec set has been received
return true;

Choose a reason for hiding this comment

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

Might be more readable to define next_merkle_root_meta as below and work with that.

let Some(next_merkle_root_meta) = merkle_root_metas
    .get(&next_erasure_set)
    .map(WorkingEntry::as_ref)
    .map(Cow::Borrowed)
    .or_else(|| {
        self.merkle_root_meta(next_erasure_set)
            .unwrap()
            .map(Cow::Owned)
    })
else {
    return true; // No shred from the next fec set has been received
};
let next_shred_id = ShredId::new(
    slot,
    next_merkle_root_meta.first_received_shred_index(),
    next_merkle_root_meta.first_received_shred_type(),
);

Comment on lines 1882 to 1902
let (prev_shred_index, prev_shred_type) =
if let Some(prev_merkle_root_meta_entry) = merkle_root_metas.get(&prev_erasure_set) {
(
prev_merkle_root_meta_entry
.as_ref()
.first_received_shred_index(),
prev_merkle_root_meta_entry
.as_ref()
.first_received_shred_type(),
)
} else {
let merkle_root_meta = self
.merkle_root_meta(prev_erasure_set)
.unwrap()
.expect("merkle root meta must exist for erasure meta");
(
merkle_root_meta.first_received_shred_index(),
merkle_root_meta.first_received_shred_type(),
)
};
let prev_shred_id = ShredId::new(slot, prev_shred_index, prev_shred_type);

Choose a reason for hiding this comment

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

Similarly here:

let prev_merkle_root_meta = merkle_root_metas
    .get(&prev_erasure_set)
    .map(WorkingEntry::as_ref)
    .map(Cow::Borrowed)
    .or_else(|| {
        self.merkle_root_meta(prev_erasure_set)
            .unwrap()
            .map(Cow::Owned)
    })
    .expect("merkle root meta must exist for erasure meta");
let prev_shred_id = ShredId::new(
    slot,
    prev_merkle_root_meta.first_received_shred_index(),
    prev_merkle_root_meta.first_received_shred_type(),
);

}
let (slot, _) = erasure_set.store_key();
// First coding shred from this erasure batch, check the forward merkle root chaining
if !self.has_duplicate_shreds_in_slot(slot) {

Choose a reason for hiding this comment

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

Personally rather avoid excessively indented code. This could be:

if self.has_duplicate_shreds_in_slot(slot) {
   continue;
}

to avoid extra indentation.

}
let (slot, _) = erasure_set.store_key();
// First shred from this erasure batch, check the backwards merkle root chaining
if !self.has_duplicate_shreds_in_slot(slot) {

Choose a reason for hiding this comment

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

similarly here:

if self.has_duplicate_shreds_in_slot(slot) {
    continue;
}

@AshwinSekar AshwinSekar added the automerge automerge Merge this Pull Request automatically once CI passes label Apr 23, 2024
@mergify mergify bot merged commit 175e152 into anza-xyz:master Apr 23, 2024
49 checks passed
mergify bot pushed a commit that referenced this pull request Apr 23, 2024
…835)

* blockstore: send duplicate proofs for chained merkle root conflicts

* pr feedback: avoid deserialization, inline warn logs

* remove allow[dead_code] after rebase

* pr feedback: cow to simplify, avoid extra indentation

(cherry picked from commit 175e152)
AshwinSekar added a commit that referenced this pull request Apr 24, 2024
…licts (backport of #835) (#1009)

blockstore: send duplicate proofs for chained merkle root conflicts (#835)

* blockstore: send duplicate proofs for chained merkle root conflicts

* pr feedback: avoid deserialization, inline warn logs

* remove allow[dead_code] after rebase

* pr feedback: cow to simplify, avoid extra indentation

(cherry picked from commit 175e152)

Co-authored-by: Ashwin Sekar <[email protected]>
anwayde pushed a commit to firedancer-io/agave that referenced this pull request Jul 23, 2024
…licts (backport of anza-xyz#835) (anza-xyz#1009)

blockstore: send duplicate proofs for chained merkle root conflicts (anza-xyz#835)

* blockstore: send duplicate proofs for chained merkle root conflicts

* pr feedback: avoid deserialization, inline warn logs

* remove allow[dead_code] after rebase

* pr feedback: cow to simplify, avoid extra indentation

(cherry picked from commit 175e152)

Co-authored-by: Ashwin Sekar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge automerge Merge this Pull Request automatically once CI passes feature-gate Pull Request adds or modifies a runtime feature gate v1.18
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants