-
Notifications
You must be signed in to change notification settings - Fork 311
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 #102
Conversation
2e5fd6f
to
4712d7a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #102 +/- ##
========================================
Coverage 81.9% 82.0%
========================================
Files 851 851
Lines 231132 231510 +378
========================================
+ Hits 189484 189878 +394
+ Misses 41648 41632 -16 |
ad0a889
to
56def12
Compare
56def12
to
10f8767
Compare
stats are deceiving, ~900 lines are tests |
ledger/src/blockstore.rs
Outdated
.next_back(); | ||
if let Some((candidate_erasure_set, candidate_erasure_meta)) = candidate_erasure_entry { | ||
if candidate_erasure_meta.as_ref().next_set_index() == fec_set_index { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be shorter code to use Option::filter
here.
https://doc.rust-lang.org/std/option/enum.Option.html#method.filter
ledger/src/blockstore.rs
Outdated
.iter(IteratorMode::From( | ||
(slot, u64::from(fec_set_index)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea how efficient this IteratorMode::From
is?
i.e. how does rocksdb seek until given starting point of iterator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure, took a quick look at the source https://github.com/cockroachdb/c-rocksdb/blob/0dd42399d1f02019c982d1752beee2f145fdd2dd/internal/db/forward_iterator.cc#L286
and it looks like it has to check each file in order to find the starting point. it seems able to quickly skip files if the user key is greater than the largest element in that file.
i'll ask around for more details and get back to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure it binary searches the tree of SST's: https://github.com/facebook/rocksdb/wiki/Indexing-SST-Files-for-Better-Lookup-Performance, and then each SST has can also be binary searched for the nearest key: https://github.com/facebook/rocksdb/wiki/A-Tutorial-of-RocksDB-SST-formats
ledger/src/blockstore.rs
Outdated
}; | ||
let candidate_erasure_meta: ErasureMeta = deserialize(candidate_erasure_meta.as_ref())?; | ||
// Check if this is actually the consecutive erasure set | ||
if candidate_slot == slot && candidate_erasure_meta.next_set_index() == fec_set_index { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this check for candidate_slot == slot
before deserialize
so that it does not pay for a redundant deserialize?
ledger/src/blockstore_meta.rs
Outdated
pub(crate) fn next_set_index(&self) -> u32 { | ||
u32::try_from(self.set_index) | ||
.unwrap() | ||
.saturating_add(self.config.num_data as u32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this function return Option<u32>
to avoid unwrap
and checked_add
instead of saturating_add
?
Also, can we rename this function to next_fec_set_index
for consistency?
We should probably also rename all the existing set_index
to fec_set_index
(separate PR).
ledger/src/shred.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we actually move all shred
changes to a separate PR for easier review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#435 split it out here
ledger/src/shred.rs
Outdated
@@ -726,6 +731,22 @@ pub mod layout { | |||
} | |||
} | |||
|
|||
pub fn get_chained_merkle_root(shred: &[u8]) -> Option<Hash> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also pub(crate)
?!
ledger/src/shred/merkle.rs
Outdated
@@ -193,6 +193,18 @@ impl ShredData { | |||
Ok(Self::SIZE_OF_HEADERS + Self::capacity(proof_size, /*chained:*/ true, resigned)?) | |||
} | |||
|
|||
fn get_chained_merkle_root_offset(proof_size: u8, resigned: bool) -> Result<usize, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if adding this function, then fn chained_merkle_root_offset
should reuse it as opposed to duplicating that last line.
ledger/src/shred/merkle.rs
Outdated
@@ -364,7 +401,11 @@ impl ShredCode { | |||
Ok(Self::SIZE_OF_HEADERS + Self::capacity(proof_size, /*chained:*/ true, resigned)?) | |||
} | |||
|
|||
fn chained_merkle_root(&self) -> Result<Hash, Error> { | |||
fn get_chained_merkle_root_offset(proof_size: u8, resigned: bool) -> Result<usize, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly here, fn chained_merkle_root_offset
should call into this function instead of duplicating last line.
ledger/src/shred/merkle.rs
Outdated
pub(super) fn get_chained_merkle_root( | ||
shred: &[u8], | ||
proof_size: u8, | ||
chained: bool, | ||
resigned: bool, | ||
) -> Option<Hash> { | ||
debug_assert_eq!( | ||
shred::layout::get_shred_variant(shred).unwrap(), | ||
ShredVariant::MerkleData { | ||
proof_size, | ||
chained, | ||
resigned | ||
} | ||
); | ||
|
||
if !chained { | ||
return None; | ||
} | ||
|
||
let offset = Self::get_chained_merkle_root_offset(proof_size, resigned).ok()?; | ||
shred | ||
.get(offset..offset + SIZE_OF_MERKLE_ROOT) | ||
.map(Hash::new) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if we need this function at all, or instead in layout::get_chained_merkle_root
just call the merkle::Shred{Data,Code}::get_chained_merke_root_offset
and do the slicing there.
This function is a bit ugly, and layout::get_chained_merkle_root
only needs the offset, so maybe better not to add this function.
ledger/src/blockstore.rs
Outdated
let mut new_merkle_root_meta = false; | ||
merkle_root_metas.entry(erasure_set).or_insert_with(|| { | ||
new_merkle_root_meta = true; | ||
WorkingEntry::Dirty(MerkleRootMeta::from_shred(&shred)) | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't like this let mut ...
+ mutation pattern. It is much easier to make mistakes and introduce bugs with mutable variables. Instead
let new_merkle_root_meta = match merkle_root_metas.entry(erasure_set) {
Entry::Vacant(entry) => {
entry.insert(...);
true
},
Entry::Occupied(_) => false,
}
10f8767
to
5be1906
Compare
ledger/src/blockstore_meta.rs
Outdated
u32::try_from(self.set_index) | ||
.ok() | ||
.and_then(|fec_set_index| fec_set_index.checked_add(self.config.num_data as u32)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we avoid unsafe as
in here? something like:
let num_data = u64::try_from(self.config.num_data).ok()?;
self.set_index.checked_add(num_data).map(u32::try_from)?.ok()
ledger/src/blockstore.rs
Outdated
"Invalid erasure meta, unable to compute next fec set index {:?}", | ||
erasure_meta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://rust-lang.github.io/rust-clippy/master/index.html#/uninlined_format_args
also elsewhere in this code.
ledger/src/blockstore.rs
Outdated
candidate_erasure_meta | ||
.as_ref() | ||
.next_fec_set_index() | ||
.map(|next_fec_set_index| next_fec_set_index == fec_set_index) | ||
.unwrap_or(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need .as_ref()
here?
Also this could just be
candidate_erasure_meta.next_fec_set_index() == Some(fec_set_index)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need the .as_ref()
to convert the WorkingEntry<ErasureMeta>
into an ErasureMeta
ledger/src/blockstore.rs
Outdated
// Blockstore is empty | ||
return Ok(None); | ||
}; | ||
if candidate_slot != slot { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be just .filter
after .find
?
Also might be better not to define and use candidate_slot
in line 553, since that is just slot
.
ledger/src/blockstore.rs
Outdated
if next_fec_set_index == fec_set_index { | ||
let candidate_erasure_set = ErasureSetId::new( | ||
candidate_slot, | ||
u32::try_from(candidate_fec_set_index).unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be worth to add a comment why unwrap
is safe here.
ledger/src/blockstore.rs
Outdated
.map(|entry| (entry, false)) | ||
.unwrap_or_else(|| { | ||
( | ||
WorkingEntry::Dirty(ErasureMeta::from_coding_shred(&shred).unwrap()), | ||
true, | ||
) | ||
}); | ||
entry.insert(erasure_meta); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of false/true
above, can't we just check if the output of entry.insert(erasure_meta)
is clean or dirty?
it returns the inserted value:
https://doc.rust-lang.org/std/collections/btree_map/struct.VacantEntry.html#method.insert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, that's much cleaner
ledger/src/blockstore.rs
Outdated
let erasure_meta_entry = erasure_metas | ||
.get(&erasure_set) | ||
.expect("entry must exist after insertion above"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this double lookup seems also unnecessary and we can just use output of VacantEntry::insert
:
https://doc.rust-lang.org/std/collections/btree_map/struct.VacantEntry.html#method.insert
and OccupiedEntry::into_mut
:
https://doc.rust-lang.org/std/collections/btree_map/struct.OccupiedEntry.html#method.into_mut
let is_new_merkle_root_meta = match merkle_root_metas.entry(erasure_set) { | ||
HashMapEntry::Vacant(entry) => { | ||
entry.insert(WorkingEntry::Dirty(MerkleRootMeta::from_shred(&shred))); | ||
true | ||
} | ||
HashMapEntry::Occupied(_) => false, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this whole true/false bookkeeping of if this is a new erasure-meta/merkle-root or not pretty verbose and error prone.
Might be better if we just check for consistency when these entries are inserted into blockstore. I don't know how difficult that would be because we need to send a duplicate proof in that case.
If both erasure-meta/merkle-root structs in blockstore have a reference to the shred (type, index) which initially populated them, then we basically need a pipeline which receives shreds, obtains erasure-meta/merkle-root from them, upserts erasure-meta/merkle-root values in blockstore and if there are inconsistencies then generates and sends duplicate proofs.
i.e. separate out the process of persisting shreds into blockstore and erasure-meta/merkle-root consistency checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be possible. If we are able to add a reference to the initial shred for ErasureMeta
(MerkleRootMeta
already has it), then we could batch and perform all checks here:
agave/ledger/src/blockstore.rs
Lines 1035 to 1056 in 2c11b7a
for (erasure_set, working_erasure_meta) in erasure_metas { | |
if !working_erasure_meta.should_write() { | |
// No need to rewrite the column | |
continue; | |
} | |
let (slot, fec_set_index) = erasure_set.store_key(); | |
write_batch.put::<cf::ErasureMeta>( | |
(slot, u64::from(fec_set_index)), | |
working_erasure_meta.as_ref(), | |
)?; | |
} | |
for (erasure_set, working_merkle_root_meta) in merkle_root_metas { | |
if !working_merkle_root_meta.should_write() { | |
// No need to rewrite the column | |
continue; | |
} | |
write_batch.put::<cf::MerkleRootMeta>( | |
erasure_set.store_key(), | |
working_merkle_root_meta.as_ref(), | |
)?; | |
} |
which still has the plumbing to send duplicate proofs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could reuse the __unused_size
field as an offset to the first received coding shred in ErasureMeta
.
let me know if you prefer going with this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets explore that idea in a separate PR and see how it looks like.
Current code has become very convoluted and risky to ship; so there might be better alternative designs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be clear would you like me to continue addressing comments and merge this pr? or move this to draft and explore the alternate approach in a separate pr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. How do you feel like about that alternative implementation?
I am not sure which one would be simpler and less risky especially if we want to hit mainnet asap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay I updated this one and will explore how the alternative will look.
most of the helper functions will be the same, just moving the logic which performs the check to the blockstore write.
5be1906
to
d78794e
Compare
ledger/src/blockstore.rs
Outdated
&self, | ||
erasure_set: ErasureSetId, | ||
erasure_metas: &BTreeMap<ErasureSetId, WorkingEntry<ErasureMeta>>, | ||
) -> Result<Option<ErasureSetId>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this function actually return the ErasureMeta
(which it already has during processing)?
I am guessing we will always need to look up the ErasureMeta
from the returned ErasureSetId
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't need the erasure meta. I need the erasure set to look up the merkle root meta to access the merkle root for the comparison in check_backwards_chained_merkle_root_consistency
.
ledger/src/blockstore.rs
Outdated
let Some(erasure_meta_entry) = erasure_metas.get(&erasure_set) else { | ||
error!( | ||
"Checking chained merkle root consistency on an erasure set {erasure_set:?} | ||
that is not loaded in memory, programmer error", | ||
); | ||
return true; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be an expect
?
ledger/src/blockstore.rs
Outdated
if let HashMapEntry::Vacant(entry) = merkle_root_metas.entry(next_erasure_set) { | ||
if let Some(meta) = self.merkle_root_meta(next_erasure_set).unwrap() { | ||
entry.insert(WorkingEntry::Clean(meta)); | ||
} | ||
} | ||
if let Some(next_merkle_root_meta) = | ||
merkle_root_metas.get(&next_erasure_set).map(AsRef::as_ref) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we avoid this double hash-map lookup here with a match
expression?
similar to line 1328
ledger/src/blockstore.rs
Outdated
let Some(chained_merkle_root) = chained_merkle_root else { | ||
// Chained merkle roots have not been enabled yet | ||
return true; | ||
}; | ||
if merkle_root == Some(chained_merkle_root) { | ||
return true; | ||
} | ||
false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just equivalent to
chained_merkle_root.is_none() || chained_merkle_root == merkle_root
ledger/src/blockstore.rs
Outdated
// This can happen if the previous erasure meta was not consistent with the shred | ||
// received. The merkle root meta is only created if the shred is inserted, | ||
// however the erasure meta always exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this;
When do we create and store erasure meta from the shred received, but do not store its merkle_root meta?
Can you plz point me to where that logic is in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right, we should always be storing both
If the shred passes the preliminary checks, an erasure meta is created:
agave/ledger/src/blockstore.rs
Lines 1248 to 1255 in 5eab6ae
let erasure_meta_entry = erasure_metas.entry(erasure_set).or_insert_with(|| { | |
self.erasure_meta(erasure_set) | |
.expect("Expect database get to succeed") | |
.map(WorkingEntry::Clean) | |
.unwrap_or_else(|| { | |
WorkingEntry::Dirty(ErasureMeta::from_coding_shred(&shred).unwrap()) | |
}) | |
}); |
We then perform the following checks:
agave/ledger/src/blockstore.rs
Line 1258 in 5eab6ae
if !erasure_meta.check_coding_shred(&shred) { agave/ledger/src/blockstore.rs
Lines 1307 to 1309 in 5eab6ae
let result = self .insert_coding_shred(index_meta, &shred, write_batch) .is_ok();
I had assumed (1) could fail, but looking at the code this seems impossible.
We finally insert the merkle meta if both checks succeed:
agave/ledger/src/blockstore.rs
Lines 1311 to 1318 in 5eab6ae
if result { | |
index_meta_working_set_entry.did_insert_occur = true; | |
metrics.num_inserted += 1; | |
merkle_root_metas | |
.entry(erasure_set) | |
.or_insert(WorkingEntry::Dirty(MerkleRootMeta::from_shred(&shred))); | |
} |
I had previously guarded this because the merkle root meta has a reference to the first received shred and I didn't want to end up in scenarios where this reference is inconsistent with blockstore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although it is unlikely, (2) could fail but then we probably have bigger issues.
So I think fine to just unwrap / expect
here.
let is_new_merkle_root_meta = match merkle_root_metas.entry(erasure_set) { | ||
HashMapEntry::Vacant(entry) => { | ||
entry.insert(WorkingEntry::Dirty(MerkleRootMeta::from_shred(&shred))); | ||
true | ||
} | ||
HashMapEntry::Occupied(_) => false, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets explore that idea in a separate PR and see how it looks like.
Current code has become very convoluted and risky to ship; so there might be better alternative designs.
1515747
to
a45f9a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, but I don't trust my own review here.
This code has become extremely ugly and convoluted, and I am not sure it is suitable for v1.18 backport given imminent mainnet upgrade.
We really need to redesign this pipeline into a more sane shape.
ledger/src/blockstore.rs
Outdated
let Some(next_fec_set_index) = candidate_erasure_meta.next_fec_set_index() else { | ||
return Err(BlockstoreError::InvalidErasureConfig); | ||
}; | ||
if next_fec_set_index == fec_set_index { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to add candidate_erasure_meta
to erasure_metas
to avoid rocksdb search next time around?
How about I split out the helper function Then we can compare this method of plumbing, to the "check when writing to blockstore" approach and see what makes sense to backport. |
sounds good. |
47ac414
to
4e01a31
Compare
4e01a31
to
4fbcee6
Compare
@behzadnouri i've added the alternate implementation here #835 |
closing in favor of #835 |
Continued from solana-labs#35316
Problem
Chained merkle roots are not yet verified on the receiving node
Summary of Changes
Generate a duplicate proof if a block contains improperly chained merkle root.
Note: we do not generate a proof for FEC set 0 that chains from the last FEC set of the parent, as we are unsure which block is the duplicate. this decision is deferred until we see votes.
Contributes to solana-labs#34897