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.

Sign up for GitHub

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

Jump to bottom

adds feature to enable chained Merkle shreds #34916

Merged
merged 1 commit into from
Jan 27, 2024
Merged

adds feature to enable chained Merkle shreds #34916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 29 additions & 4 deletions core/src/shred_fetch_stage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,22 @@ impl ShredFetchStage {

// Limit shreds to 2 epochs away.
let max_slot = last_slot + 2 * slots_per_epoch;
let should_drop_legacy_shreds =
|shred_slot| should_drop_legacy_shreds(shred_slot, &feature_set, &epoch_schedule);
let should_drop_legacy_shreds = |shred_slot| {
check_feature_activation(
&feature_set::drop_legacy_shreds::id(),
shred_slot,
&feature_set,
&epoch_schedule,
)
};
let enable_chained_merkle_shreds = |shred_slot| {
check_feature_activation(
&feature_set::enable_chained_merkle_shreds::id(),
shred_slot,
&feature_set,
&epoch_schedule,
)
};
let turbine_disabled = turbine_disabled.load(Ordering::Relaxed);
for packet in packet_batch.iter_mut().filter(|p| !p.meta().discard()) {
if turbine_disabled
Expand All @@ -115,6 +129,7 @@ impl ShredFetchStage {
max_slot,
shred_version,
should_drop_legacy_shreds,
enable_chained_merkle_shreds,
&mut stats,
)
{
Expand Down Expand Up @@ -394,13 +409,15 @@ pub(crate) fn receive_repair_quic_packets(
}
}

// Returns true if the feature is effective for the shred slot.
#[must_use]
fn should_drop_legacy_shreds(
fn check_feature_activation(
Copy link
Contributor

Choose a reason for hiding this comment

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

could merge with cluster_nodes::check_feature_activation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That one uses a root_bank for less verbosity.
This one uses feature_set and epoch_schedule because holding onto root bank here has these issues: #33078

We can't make cluster_nodes one call this one because it adds dependency on core crate which we want to avoid.

Copy link
Contributor

Choose a reason for hiding this comment

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

could we make the cluster_nodes one also use feature_set and epoch_schedule so that both callers can use it?
this code is also duplicated in duplicate_shred_handler, but because cluster_nodes is in solana-turbine it would have added a circular dependency.
perhaps the best thing would be to move this into feature_set so that everyone can use it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could we make the cluster_nodes one also use feature_set and epoch_schedule so that both callers can use it?

The expanded form here is an unfortunate consequence of #33078 and is both more verbose and less self-contained; I feel like cluster_nodes one taking the root-bank as the argument is already the better code.

perhaps the best thing would be to move this into feature_set so that everyone can use it

I wouldn't suggest doing that because this one epoch lag is only relevant when working with raw shreds and we don't want to encourage using it in other instances.

Either way, this commit is only renaming existing code and adding a single argument. We can address the code duplication (which pre-exists this commit) separately.
We also need to keep the code change small, because if we are targeting v1.18 for these patches, then this needs to be backported.

feature: &Pubkey,
shred_slot: Slot,
feature_set: &FeatureSet,
epoch_schedule: &EpochSchedule,
) -> bool {
match feature_set.activated_slot(&feature_set::drop_legacy_shreds::id()) {
match feature_set.activated_slot(feature) {
None => false,
Some(feature_slot) => {
let feature_epoch = epoch_schedule.get_epoch(feature_slot);
Expand Down Expand Up @@ -451,6 +468,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats,
));
let coding = solana_ledger::shred::Shredder::generate_coding_shreds(
Expand All @@ -465,6 +483,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats,
));
}
Expand All @@ -487,6 +506,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats,
));
assert_eq!(stats.index_overrun, 1);
Expand All @@ -509,6 +529,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats,
));
assert_eq!(stats.slot_out_of_range, 1);
Expand All @@ -519,6 +540,7 @@ mod tests {
max_slot,
345, // shred_version
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats,
));
assert_eq!(stats.shred_version_mismatch, 1);
Expand All @@ -530,6 +552,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats,
));

Expand All @@ -552,6 +575,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats,
));

Expand All @@ -564,6 +588,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats,
));
}
Expand Down
18 changes: 18 additions & 0 deletions ledger/src/shred.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -916,6 +916,7 @@ pub fn should_discard_shred(
max_slot: Slot,
shred_version: u16,
should_drop_legacy_shreds: impl Fn(Slot) -> bool,
enable_chained_merkle_shreds: impl Fn(Slot) -> bool,
stats: &mut ShredFetchStats,
) -> bool {
debug_assert!(root < max_slot);
Expand Down Expand Up @@ -999,13 +1000,19 @@ pub fn should_discard_shred(
stats.num_shreds_merkle_code = stats.num_shreds_merkle_code.saturating_add(1);
}
ShredVariant::MerkleCode(_, /*chained:*/ true) => {
if !enable_chained_merkle_shreds(slot) {
return true;
}
stats.num_shreds_merkle_code_chained =
stats.num_shreds_merkle_code_chained.saturating_add(1);
}
ShredVariant::MerkleData(_, /*chained:*/ false) => {
stats.num_shreds_merkle_data = stats.num_shreds_merkle_data.saturating_add(1);
}
ShredVariant::MerkleData(_, /*chained:*/ true) => {
if !enable_chained_merkle_shreds(slot) {
return true;
}
stats.num_shreds_merkle_data_chained =
stats.num_shreds_merkle_data_chained.saturating_add(1);
}
Expand Down Expand Up @@ -1209,6 +1216,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats
));
assert_eq!(stats, ShredFetchStats::default());
Expand All @@ -1220,6 +1228,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats
));
assert_eq!(stats.index_overrun, 1);
Expand All @@ -1231,6 +1240,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats
));
assert_eq!(stats.index_overrun, 2);
Expand All @@ -1242,6 +1252,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats
));
assert_eq!(stats.index_overrun, 3);
Expand All @@ -1253,6 +1264,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats
));
assert_eq!(stats.index_overrun, 4);
Expand All @@ -1264,6 +1276,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats
));
assert_eq!(stats.bad_parent_offset, 1);
Expand All @@ -1285,6 +1298,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats
));

Expand All @@ -1305,6 +1319,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats
));
assert_eq!(1, stats.index_out_of_bounds);
Expand All @@ -1326,6 +1341,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats
));
packet.buffer_mut()[OFFSET_OF_SHRED_VARIANT] = u8::MAX;
Expand All @@ -1336,6 +1352,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats
));
assert_eq!(1, stats.bad_shred_type);
Expand All @@ -1348,6 +1365,7 @@ mod tests {
max_slot,
shred_version,
|_| false, // should_drop_legacy_shreds
|_| true, // enable_chained_merkle_shreds
&mut stats
));
assert_eq!(1, stats.bad_shred_type);
Expand Down
5 changes: 5 additions & 0 deletions sdk/src/feature_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,10 @@ pub mod enable_gossip_duplicate_proof_ingestion {
solana_sdk::declare_id!("FNKCMBzYUdjhHyPdsKG2LSmdzH8TCHXn3ytj8RNBS4nG");
}

pub mod enable_chained_merkle_shreds {
solana_sdk::declare_id!("7uZBkJXJ1HkuP6R3MJfZs7mLwymBcDbKdqbF51ZWLier");
}

lazy_static! {
/// Map of feature identifiers to user-visible description
pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
Expand Down Expand Up @@ -965,6 +969,7 @@ lazy_static! {
(curve25519_restrict_msm_length::id(), "restrict curve25519 multiscalar multiplication vector lengths #34763"),
(cost_model_requested_write_lock_cost::id(), "cost model uses number of requested write locks #34819"),
(enable_gossip_duplicate_proof_ingestion::id(), "enable gossip duplicate proof ingestion #32963"),
(enable_chained_merkle_shreds::id(), "Enable chained Merkle shreds #34916"),
/*************** ADD NEW FEATURES HERE ***************/
]
.iter()
Expand Down