adds feature to enable chained Merkle shreds #34916
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #34916 +/- ##
=========================================
- Coverage 81.6% 81.6% -0.1%
=========================================
Files 830 830
Lines 224746 224901 +155
=========================================
+ Hits 183512 183609 +97
- Misses 41234 41292 +58 |
sdk/src/feature_set.rs
Outdated
| @@ -960,6 +964,7 @@ lazy_static! { | |||
| (enable_zk_proof_from_account::id(), "Enable zk token proof program to read proof from accounts instead of instruction data #34750"), | |||
| (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_chained_merkle_shreds::id(), "Enable chained Merkle shreds #"), | |||
| #[must_use] | ||
| fn should_drop_legacy_shreds( | ||
| fn check_feature_activation( |
There was a problem hiding this comment.
could merge with cluster_nodes::check_feature_activation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
behzadnouri
force-pushed
the
enable-chained-merkle-shreds
branch
from
f379050
to
1e69ef5
Compare
|
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. |
During a cluster upgrade when only half of the cluster can ingest the new shred variant, sending shreds of the new variant can cause nodes to diverge. The commit adds a feature to enable chained Merkle shreds explicitly. (cherry picked from commit d4fdcd9)
…) (#35083) adds feature to enable chained Merkle shreds (#34916) During a cluster upgrade when only half of the cluster can ingest the new shred variant, sending shreds of the new variant can cause nodes to diverge. The commit adds a feature to enable chained Merkle shreds explicitly. (cherry picked from commit d4fdcd9) Co-authored-by: behzad nouri <behzadnouri@gmail.com>
Problem
During a cluster upgrade when only half of the cluster can ingest the new shred variant, sending shreds of the new variant can cause nodes to diverge.
Summary of Changes
Added feature to enable chained Merkle shreds explicitly.