blockstore: add last fec set details column #34651

AshwinSekar · 2024-01-04T18:06:51Z

Problem

There are a number of additional checks we want to perform on the last fec set of each block. This additional column stores the details necessary for these checks allowing easy replay verification with only one blockstore read.

Summary of Changes

Add last fec set details column

AshwinSekar · 2024-01-04T18:32:46Z

ledger/src/blockstore_meta.rs

+}
+
+#[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)]
+pub enum LastFecSetDetails {


This is an enum to support future extensibility when we eventually add IP check/retransmit signature verification.

Glad you already baked in an enum to allow it to grow. A few notes:

One consideration will be backwards compat; namely, making sure old code that is unaware of new enum instances can handle those instances gracefully. Not a concern for this PR, but just mentioning it so we're thinking about it a little

What will future enum instances of LastFecSetDetails look like. The current enum has just a SizeDetails; will future iterations contain a completely different struct OR SizeDetails + some new info OR something else?

I envision future iterations to be inclusive, so SizeDetails + Option<AdditionalInfo>.
We add additional info as an option in order to support backwards compatibility.

codecov · 2024-01-04T19:51:14Z

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (bbb4118) 81.8% compared to head (cf28525) 81.8%.
Report is 162 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #34651     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         824      824             
  Lines      222391   222405     +14     
=========================================
- Hits       181959   181954      -5     
- Misses      40432    40451     +19

steviez

Is it correct that we do not intend to backport this column to v1.17 ? Rather, we can let the auto-detect feature handle compatibility between master/v1.18 and v1.17 ?

steviez · 2024-01-05T03:33:42Z

ledger/src/blockstore_meta.rs

+}
+
+#[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)]
+pub enum LastFecSetDetails {


Glad you already baked in an enum to allow it to grow. A few notes:

One consideration will be backwards compat; namely, making sure old code that is unaware of new enum instances can handle those instances gracefully. Not a concern for this PR, but just mentioning it so we're thinking about it a little

What will future enum instances of LastFecSetDetails look like. The current enum has just a SizeDetails; will future iterations contain a completely different struct OR SizeDetails + some new info OR something else?

behzadnouri · 2024-01-05T15:54:12Z

ledger/src/blockstore_meta.rs

+    pub num_coding_received: usize,
+    pub num_data_received: usize,


Can you leave some comments what constitutes as "received"? with respect to different routes you might obtain a shred: turbine, repair, erasure recovery.

behzadnouri · 2024-01-05T15:57:02Z

ledger/src/blockstore_meta.rs

@@ -526,6 +526,19 @@ impl OptimisticSlotMetaVersioned {
        }
    }
 }
+
+#[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)]
+pub struct SizeDetails {


How would you expect to populate these fields?
When you receive a (code or data) shred, you do not know if that belongs to the last FEC set unless it is the very last data shred.

we wait until we receive a shred with LAST_SHRED_IN_SLOT flag. At that point we know the fec_set_index of the last fec set.
If we have already seen a coding shred at that point we can use the ErasureMeta to perform a one time scan of blockstore to see how many coding/data shreds we have received.
If not we we set the num_data_inserted and num_coding_inserted to 0.

If we receive a coding shred while num_coding_inserted is 0, we perform the one time scan using the ErasureMeta and update num_coding_inserted to 1, and num_data_inserted to the results of the scan.
For further shred insertions, we increment num_data_inserted or num_coding_inserted.

After shred insertion if the sum of num_data_inserted or num_coding_inserted has now crossed the 2 * DATA_SHREDS_PER_FEC_BLOCK (64 shreds) threshold, then we notify replay through crossbeam.
We need to notify replay here because replay could have already replayed the block and marked it as dead/invalid.

If we have already seen a coding shred at that point we can use the ErasureMeta to perform a one time scan of blockstore to see how many coding/data shreds we have received.

and what if we have not received a coding shred yet?

if not we we set the num_data_inserted and num_coding_inserted to 0.
If we receive a coding shred while num_coding_inserted is 0, we perform the one time scan using the ErasureMeta and update

AshwinSekar · 2024-01-05T16:08:24Z

Is it correct that we do not intend to backport this column to v1.17 ? Rather, we can let the auto-detect feature handle compatibility between master/v1.18 and v1.17 ?

Yes that's correct. I've kept this as a stub PR just in case we need to backport this to v1.16 for whatever reason, although if the auto-detect feature is backported to v1.16 that would be unnecessary.

steviez · 2024-01-05T17:13:09Z

Is it correct that we do not intend to backport this column to v1.17 ? ...

Yes that's correct. I've kept this as a stub PR just in case we need to backport this to v1.16 for whatever reason, although if the auto-detect feature is backported to v1.16 that would be unnecessary.

Awesome and good call on doing so; we opted to push the merkle_root_meta stub back to v1.16 instead of BP'ing the auto-detect change given that the stub method is more simple / we have done it before. Always good to have options.

behzadnouri · 2024-01-05T17:22:57Z

ledger/src/blockstore_meta.rs

+    /// These fields keep track of any shreds inserted into blockstore,
+    /// through turbine/repair or erasure recovery.
+    /// Before voting replay will check that the sum of these values are at
+    /// least 2 * `DATA_SHREDS_PER_FEC_BLOCK`.


I thought we wanted to make sure that the leader broadcasted enough many shreds of the last FEC set.

With this accounting, the leader can generate a 1:63 erasure batch (one data shred and 63 coding shred), send only 1 coding shred to a node, and that node will be able to recover the whole batch and insert the remaining shreds.

ah yeah, that's an excellent point. we'll need to differentiate the source of the shreds.
let me start a discussion to figure out the requirement.

AshwinSekar · 2024-01-29T23:17:33Z

@behzadnouri pointed out that the last_fec_set_index cannot be trusted in isolation, a leader could easily craft a replayable block that sets bogus values for these. rather than trusting this value as the shreds are inserted in order to populate this column, a better solution is to perform the check after replay:

Replay the block
Grab the last 32 data shreds
If they do not all have the same merkle root (merkle root contains last_fec_set_index), mark the block as duplicate and do not vote on it
(future) perform the retransmitter verification on these 32 shreds, and mark duplicate if failed
Freeze the bank.

AshwinSekar commented Jan 4, 2024

View reviewed changes

AshwinSekar requested review from behzadnouri and steviez January 4, 2024 18:34

steviez reviewed Jan 5, 2024

View reviewed changes

behzadnouri reviewed Jan 5, 2024

View reviewed changes

AshwinSekar added 2 commits January 5, 2024 16:11

blockstore: add last fec set details column

a11a950

pr feedback: rename received -> inserted, add comment

cf28525

AshwinSekar force-pushed the last-fec-details branch from c0cf4c8 to cf28525 Compare January 5, 2024 16:11

behzadnouri reviewed Jan 5, 2024

View reviewed changes

AshwinSekar marked this pull request as draft January 10, 2024 22:22

github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Jan 25, 2024

AshwinSekar closed this Jan 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

blockstore: add last fec set details column #34651

blockstore: add last fec set details column #34651

AshwinSekar commented Jan 4, 2024

AshwinSekar Jan 4, 2024

steviez Jan 5, 2024

AshwinSekar Jan 5, 2024

codecov bot commented Jan 4, 2024 •

edited

Loading

steviez left a comment

steviez Jan 5, 2024

behzadnouri Jan 5, 2024

behzadnouri Jan 5, 2024

AshwinSekar Jan 5, 2024 •

edited

Loading

behzadnouri Jan 5, 2024

AshwinSekar Jan 5, 2024

AshwinSekar commented Jan 5, 2024

steviez commented Jan 5, 2024

behzadnouri Jan 5, 2024

AshwinSekar Jan 5, 2024

AshwinSekar commented Jan 29, 2024 •

edited

Loading

		pub num_coding_received: usize,
		pub num_data_received: usize,

blockstore: add last fec set details column #34651

blockstore: add last fec set details column #34651

Conversation

AshwinSekar commented Jan 4, 2024

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 4, 2024 • edited Loading

Codecov Report

steviez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshwinSekar Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshwinSekar commented Jan 5, 2024

steviez commented Jan 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshwinSekar commented Jan 29, 2024 • edited Loading

codecov bot commented Jan 4, 2024 •

edited

Loading

AshwinSekar Jan 5, 2024 •

edited

Loading

AshwinSekar commented Jan 29, 2024 •

edited

Loading