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: always send a coding shred for chained merkle root conflicts #1353

Merged
merged 3 commits into from
May 28, 2024

Conversation

AshwinSekar
Copy link

Problem

#835 has a chance to send a chained merkle root conflict duplicate proof consisting of 2 data shreds.
With 2 data shreds we cannot verify that they are from adjacent FEC sets. We require that the shred from the lower FEC set be a coding shred.

Summary of Changes

  • Modify the backwards check to always send the coding shred from lower FEC set. We know this is available because there must be an erasure meta.
  • Modify tests to check the upgrade scenario, where the first received coding shred index is not reliable (skip the check in this case).

This ensures that we always send a coding shred for the lower FEC set, meaning the gossip handler can verify that duplicate proofs are for adjacent FEC sets.
Without this verification an attacker can arbitrarily pick non adjacent FEC sets to "prove" that every block is duplicate

Contributes to solana-labs#34897

@@ -1904,35 +1902,33 @@ impl Blockstore {
return true;
};

let Some(prev_merkle_root_meta) = merkle_root_metas
Copy link
Author

Choose a reason for hiding this comment

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

This is safe because self.previous_erasure_set() on line 1896 must have found an ErasureMeta for prev_erasure_set in order to reach here.

Choose a reason for hiding this comment

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

Shouldn't fn previous_erasure_set actually return that ErasureMeta instead of looking it up again here?

Copy link
Author

Choose a reason for hiding this comment

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

yeah good point, I didn't do this last time because we only needed the merkle root.
Updated it to return the meta as well.

error!(
"Shred {prev_shred_id:?} indicated by merkle root meta {prev_merkle_root_meta:?} is missing from blockstore.
This should only happen in extreme cases where blockstore cleanup has caught up to the root.
warn!(
Copy link
Author

Choose a reason for hiding this comment

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

We enter this if either:

  • Blockstore cleanup has caught up to root and prev_shred has been cleaned up
  • prev_erasure_meta was inserted with an older version and prev_erasure_meta.first_received_coding_shred_index() points to shred 0 which we do not have.

@codecov-commenter
Copy link

codecov-commenter commented May 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.7%. Comparing base (097aa8e) to head (1f28b82).

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #1353     +/-   ##
=========================================
- Coverage    82.7%    82.7%   -0.1%     
=========================================
  Files         876      876             
  Lines      371180   371185      +5     
=========================================
  Hits       307275   307275             
- Misses      63905    63910      +5     

@AshwinSekar AshwinSekar force-pushed the chain-dup-proof-coding branch 4 times, most recently from 5d1506a to f1c4bf8 Compare May 22, 2024 19:47
@AshwinSekar AshwinSekar merged commit ecb491c into anza-xyz:master May 28, 2024
41 checks passed
@AshwinSekar AshwinSekar deleted the chain-dup-proof-coding branch May 28, 2024 18:24
Copy link

mergify bot commented Jun 4, 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 Jun 4, 2024
…cts (#1353)

* blockstore: always send a coding shred for chained merkle root conflicts

* pr feedback: return ErasureMeta

* fix format string indentation

(cherry picked from commit ecb491c)

# Conflicts:
#	ledger/src/blockstore.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants