-
Notifications
You must be signed in to change notification settings - Fork 34
blockstore: compute and populate DoubleMerkleRootMeta #613
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
Conversation
ledger/src/blockstore.rs
Outdated
| BlockstoreError::FailedDoubleMerkleRootConstruction(slot, block_location) | ||
| })? | ||
| .into_iter() | ||
| .flatten() |
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.
opted to just store the proof as flattened to avoid the extra work when serving repair - will have to flatten when sending over the wire and unflatten when receiver verifies anyway
5fb2269 to
956cf65
Compare
| .delete_file_in_range(from_slot, to_slot) | ||
| .is_ok() | ||
| & self | ||
| .parent_meta_cf |
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.
oof, good catch
We'll need to set the block id for the header in broadcast atm, as that's where we construct the header. |
ksn6
left a comment
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.
Join nodes as each FEC set comes in, no need to wait until the end and perform up to 1000 blockstore lookups for FEC set roots, this can be done in line when populating new MerkleRootMetas
to clarify - this would be an online algorithm to calculate the double merkle root + merkle proofs, correct? would we store the intermediate state in an in-memory map? or blockstore as well?
and - we aren't really performing 1000 blockstore lookups right? get_or_compute_double_merkle_root performs exactly one lookup per FEC set atm
ledger/src/blockstore.rs
Outdated
| .flatten() | ||
| .copied() | ||
| .collect(); | ||
| debug_assert!( |
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.
debug_assert_eq!
ledger/src/blockstore.rs
Outdated
| let Some(slot_meta) = self | ||
| .meta_cf | ||
| .get(slot) | ||
| .expect("Blockstore operations must succeed") | ||
| else { | ||
| return Err(BlockstoreProcessorError::FailedToLoadMeta); | ||
| }; |
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.
Rather than let-else-return error, we can do (here and elsewhere in the function):
| let Some(slot_meta) = self | |
| .meta_cf | |
| .get(slot) | |
| .expect("Blockstore operations must succeed") | |
| else { | |
| return Err(BlockstoreProcessorError::FailedToLoadMeta); | |
| }; | |
| let slot_meta = self | |
| .meta_cf | |
| .get(slot) | |
| .expect("Blockstore operations must succeed") | |
| .ok_or(BlockstoreProcessorError::FailedToLoadMeta)?; |
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.
yeah good call, this fn can just return an Option, any error apart from the slot not being full indicates an inconsistency in blockstore or a bug in the merkle tree impl
ledger/src/blockstore.rs
Outdated
| if !slot_meta.is_full() { | ||
| return Err(BlockstoreProcessorError::SlotNotFull(slot, block_location)); | ||
| } | ||
|
|
||
| let Some(last_index) = slot_meta.last_index else { | ||
| return Err(BlockstoreProcessorError::SlotNotFull(slot, block_location)); | ||
| }; |
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 !slot_meta.is_full() { | |
| return Err(BlockstoreProcessorError::SlotNotFull(slot, block_location)); | |
| } | |
| let Some(last_index) = slot_meta.last_index else { | |
| return Err(BlockstoreProcessorError::SlotNotFull(slot, block_location)); | |
| }; | |
| let last_index = slot_meta | |
| .last_index | |
| .filter(|_| slot_meta.is_full()) | |
| .ok_or(BlockstoreProcessorError::SlotNotFull(slot, block_location))?; |
ledger/src/blockstore.rs
Outdated
| let erasure_set_id = ErasureSetId::new(slot, fec_set_index); | ||
|
|
||
| let Some(merkle_root) = self | ||
| .merkle_root_meta_from_location(erasure_set_id, block_location) |
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.
A single invocation to .multi_get_keys is probably better here, in case we don't get to the online merkle idea.
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.
yeah good call that's definitely better - updated
956cf65 to
4013fce
Compare
ksn6
left a comment
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
ledger/src/blockstore.rs
Outdated
|
|
||
| let last_index = slot_meta.last_index.expect("Slot is full"); | ||
|
|
||
| // This function is only used post Alpenglow, so implicitely gated by SIMD-0317 as that is a prereq |
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.
nit: implicitly
|
|
||
| // Collect merkle roots for each FEC set | ||
| let fec_set_indices = | ||
| (0..fec_set_count).map(|i| (slot, (i * DATA_SHREDS_PER_FEC_BLOCK) 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.
Would use checked_mul + return None on failure, just in case.
Slightly far-fetched - given proposals to remove CU limits and have block limits only be time-based, if a malicious leader constructed a block with a very large number of no-ops, I wonder whether we could have 2^32 / DATA_SHREDS_PER_FEC_SET_BLOCK ~ 135M FEC sets, which would then cause the cluster to crash here.
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're actually protected by that type of attack here
Lines 801 to 803 in 079cd89
| if index >= MAX_DATA_SHREDS_PER_SLOT as u32 { | |
| stats.index_out_of_bounds += 1; | |
| return true; |
Before the shred is even inserted into blockstore we verify that the index is less than 32k, so fec_set_count cannot be more than ~1000
ledger/src/blockstore.rs
Outdated
| let mut merkle_tree_leaves = self | ||
| .merkle_root_meta_cf | ||
| .multi_get_bytes(&keys) | ||
| .map(|get_result| { | ||
| let bytes = get_result | ||
| .expect("Blockstore operations must succeed") | ||
| .expect("Merkle root meta must exist for all fec sets in full slot"); | ||
| let merkle_root = bincode::deserialize::<MerkleRootMeta>(bytes.as_ref()) | ||
| .expect("Merkle root meta column only contains valid MerkleRootMetas") | ||
| .merkle_root() | ||
| .expect("Legacy shreds no longer exist, merkle root must be present"); | ||
| Ok(merkle_root) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| // Add parent info as the last leaf | ||
| let parent_info_hash = hashv(&[ | ||
| &parent_meta.parent_slot.to_le_bytes(), | ||
| parent_meta.parent_block_id.as_ref(), | ||
| ]); | ||
| merkle_tree_leaves.push(Ok(parent_info_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.
| let mut merkle_tree_leaves = self | |
| .merkle_root_meta_cf | |
| .multi_get_bytes(&keys) | |
| .map(|get_result| { | |
| let bytes = get_result | |
| .expect("Blockstore operations must succeed") | |
| .expect("Merkle root meta must exist for all fec sets in full slot"); | |
| let merkle_root = bincode::deserialize::<MerkleRootMeta>(bytes.as_ref()) | |
| .expect("Merkle root meta column only contains valid MerkleRootMetas") | |
| .merkle_root() | |
| .expect("Legacy shreds no longer exist, merkle root must be present"); | |
| Ok(merkle_root) | |
| }) | |
| .collect::<Vec<_>>(); | |
| // Add parent info as the last leaf | |
| let parent_info_hash = hashv(&[ | |
| &parent_meta.parent_slot.to_le_bytes(), | |
| parent_meta.parent_block_id.as_ref(), | |
| ]); | |
| merkle_tree_leaves.push(Ok(parent_info_hash)); | |
| let merkle_tree_leaves: Vec<_> = self | |
| .merkle_root_meta_cf | |
| .multi_get_bytes(&keys) | |
| .map(|get_result| { | |
| let bytes = get_result | |
| .expect("Blockstore operations must succeed") | |
| .expect("Merkle root meta must exist for all fec sets in full slot"); | |
| bincode::deserialize::<MerkleRootMeta>(bytes.as_ref()) | |
| .expect("Merkle root meta column only contains valid MerkleRootMetas") | |
| .merkle_root() | |
| .expect("Legacy shreds no longer exist, merkle root must be present") | |
| }) | |
| // Add parent info as the last leaf | |
| .chain(std::iter::once(hashv(&[ | |
| &parent_meta.parent_slot.to_le_bytes(), | |
| parent_meta.parent_block_id.as_ref(), | |
| ]))) | |
| .collect(); |
ledger/src/blockstore.rs
Outdated
| let tree_size = fec_set_count + 1; | ||
| let mut proofs = Vec::with_capacity(tree_size); | ||
|
|
||
| for leaf_index in 0..tree_size { | ||
| let proof_iter = make_merkle_proof(leaf_index, tree_size, &merkle_tree); | ||
| let proof: Vec<u8> = proof_iter | ||
| .flat_map(|proof| { | ||
| proof | ||
| .expect("Merkle proof construction cannot have failed") | ||
| .as_slice() | ||
| }) | ||
| .copied() | ||
| .collect(); | ||
| debug_assert_eq!( | ||
| proof.len(), | ||
| get_proof_size(tree_size) as usize * SIZE_OF_MERKLE_PROOF_ENTRY | ||
| ); | ||
| proofs.push(proof); | ||
| } |
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.
| let tree_size = fec_set_count + 1; | |
| let mut proofs = Vec::with_capacity(tree_size); | |
| for leaf_index in 0..tree_size { | |
| let proof_iter = make_merkle_proof(leaf_index, tree_size, &merkle_tree); | |
| let proof: Vec<u8> = proof_iter | |
| .flat_map(|proof| { | |
| proof | |
| .expect("Merkle proof construction cannot have failed") | |
| .as_slice() | |
| }) | |
| .copied() | |
| .collect(); | |
| debug_assert_eq!( | |
| proof.len(), | |
| get_proof_size(tree_size) as usize * SIZE_OF_MERKLE_PROOF_ENTRY | |
| ); | |
| proofs.push(proof); | |
| } | |
| let tree_size = fec_set_count + 1; | |
| let proofs: Vec<Vec<u8>> = (0..tree_size) | |
| .map(|leaf_index| { | |
| make_merkle_proof(leaf_index, tree_size, &merkle_tree) | |
| .map(|hash| hash.expect("Merkle proof construction cannot fail")) | |
| .flat_map(|hash| hash.as_ref()) | |
| .copied() | |
| .collect() | |
| }) | |
| .inspect(|proof| { | |
| debug_assert_eq!( | |
| proof.len(), | |
| get_proof_size(tree_size) as usize * SIZE_OF_MERKLE_PROOF_ENTRY | |
| ); | |
| }) | |
| .collect(); |
ksn6
left a comment
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
Adds an unoptimized double merkle root computation to get the ball rolling.
Computes and stores the double merkle root and associated proofs once the block is full.
This places all the blockstore lookups in the hot path (computation will run once block has been frozen).
Future optimizations:
Next PR will actually call this fn and set block id in either replay or broadcast