-
Notifications
You must be signed in to change notification settings - Fork 290
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
Release v2.0.3 #4243
Release v2.0.3 #4243
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -45,6 +45,7 @@ use starcoin_types::{ | |||||||||||||||||||||||||||||||||||||
use starcoin_vm_types::access_path::AccessPath; | ||||||||||||||||||||||||||||||||||||||
use starcoin_vm_types::account_config::genesis_address; | ||||||||||||||||||||||||||||||||||||||
use starcoin_vm_types::genesis_config::{ChainId, ConsensusStrategy}; | ||||||||||||||||||||||||||||||||||||||
use starcoin_vm_types::on_chain_config::FlexiDagConfigV2; | ||||||||||||||||||||||||||||||||||||||
use starcoin_vm_types::on_chain_resource::Epoch; | ||||||||||||||||||||||||||||||||||||||
use std::cmp::min; | ||||||||||||||||||||||||||||||||||||||
use std::iter::Extend; | ||||||||||||||||||||||||||||||||||||||
|
@@ -1389,7 +1390,7 @@ impl ChainReader for BlockChain { | |||||||||||||||||||||||||||||||||||||
.get_block_header_by_hash(header.parent_hash())? | ||||||||||||||||||||||||||||||||||||||
.ok_or_else(|| format_err!("cannot find parent block header"))?; | ||||||||||||||||||||||||||||||||||||||
let next_ghostdata = self.dag().verify_and_ghostdata(uncles, header)?; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
let (pruning_depth, pruning_finality) = self.get_pruning_config(); | ||||||||||||||||||||||||||||||||||||||
if self.status().head().pruning_point() != HashValue::zero() { | ||||||||||||||||||||||||||||||||||||||
let previous_ghostdata = if previous_header.pruning_point() == HashValue::zero() { | ||||||||||||||||||||||||||||||||||||||
let genesis = self | ||||||||||||||||||||||||||||||||||||||
|
@@ -1409,6 +1410,8 @@ impl ChainReader for BlockChain { | |||||||||||||||||||||||||||||||||||||
previous_ghostdata.as_ref(), | ||||||||||||||||||||||||||||||||||||||
header.pruning_point(), | ||||||||||||||||||||||||||||||||||||||
&next_ghostdata, | ||||||||||||||||||||||||||||||||||||||
pruning_depth, | ||||||||||||||||||||||||||||||||||||||
pruning_finality, | ||||||||||||||||||||||||||||||||||||||
)?; | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
@@ -1422,6 +1425,18 @@ impl ChainReader for BlockChain { | |||||||||||||||||||||||||||||||||||||
fn get_pruning_height(&self) -> BlockNumber { | ||||||||||||||||||||||||||||||||||||||
self.get_pruning_height() | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
fn get_pruning_config(&self) -> (u64, u64) { | ||||||||||||||||||||||||||||||||||||||
let reader = AccountStateReader::new(&self.statedb); | ||||||||||||||||||||||||||||||||||||||
let FlexiDagConfigV2 { | ||||||||||||||||||||||||||||||||||||||
pruning_depth, | ||||||||||||||||||||||||||||||||||||||
pruning_finality, | ||||||||||||||||||||||||||||||||||||||
} = reader | ||||||||||||||||||||||||||||||||||||||
.get_dag_config() | ||||||||||||||||||||||||||||||||||||||
.unwrap_or_default() | ||||||||||||||||||||||||||||||||||||||
.unwrap_or_default(); | ||||||||||||||||||||||||||||||||||||||
(pruning_depth, pruning_finality) | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+1430
to
+1439
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider handling errors explicitly in The method Refactor the method to handle errors properly: -fn get_pruning_config(&self) -> (u64, u64) {
- let reader = AccountStateReader::new(&self.statedb);
- let FlexiDagConfigV2 {
- pruning_depth,
- pruning_finality,
- } = reader
- .get_dag_config()
- .unwrap_or_default()
- .unwrap_or_default();
- (pruning_depth, pruning_finality)
-}
+fn get_pruning_config(&self) -> Result<(u64, u64)> {
+ let reader = AccountStateReader::new(&self.statedb);
+ if let Some(config) = reader.get_dag_config()? {
+ Ok((config.pruning_depth, config.pruning_finality))
+ } else {
+ Err(format_err!("Failed to retrieve DAG configuration"))
+ }
+} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
impl BlockChain { | ||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -15,31 +15,25 @@ use crate::{ | |||||||||||||||||||||||||||||||||||||||||||
pub struct PruningPointManagerT<T: ReachabilityStoreReader + Clone> { | ||||||||||||||||||||||||||||||||||||||||||||
reachability_service: MTReachabilityService<T>, | ||||||||||||||||||||||||||||||||||||||||||||
ghost_dag_store: DbGhostdagStore, | ||||||||||||||||||||||||||||||||||||||||||||
pruning_depth: u64, | ||||||||||||||||||||||||||||||||||||||||||||
pruning_finality: u64, | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
impl<T: ReachabilityStoreReader + Clone> PruningPointManagerT<T> { | ||||||||||||||||||||||||||||||||||||||||||||
pub fn new( | ||||||||||||||||||||||||||||||||||||||||||||
reachability_service: MTReachabilityService<T>, | ||||||||||||||||||||||||||||||||||||||||||||
ghost_dag_store: DbGhostdagStore, | ||||||||||||||||||||||||||||||||||||||||||||
pruning_depth: u64, | ||||||||||||||||||||||||||||||||||||||||||||
pruning_finality: u64, | ||||||||||||||||||||||||||||||||||||||||||||
) -> Self { | ||||||||||||||||||||||||||||||||||||||||||||
Self { | ||||||||||||||||||||||||||||||||||||||||||||
reachability_service, | ||||||||||||||||||||||||||||||||||||||||||||
ghost_dag_store, | ||||||||||||||||||||||||||||||||||||||||||||
pruning_depth, | ||||||||||||||||||||||||||||||||||||||||||||
pruning_finality, | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
pub fn reachability_service(&self) -> MTReachabilityService<T> { | ||||||||||||||||||||||||||||||||||||||||||||
self.reachability_service.clone() | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
pub fn finality_score(&self, blue_score: u64) -> u64 { | ||||||||||||||||||||||||||||||||||||||||||||
blue_score / self.pruning_finality | ||||||||||||||||||||||||||||||||||||||||||||
pub(crate) fn finality_score(&self, blue_score: u64, pruning_finality: u64) -> u64 { | ||||||||||||||||||||||||||||||||||||||||||||
blue_score / pruning_finality | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+35
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prevent potential division by zero in The Apply this diff to add a validation check: pub(crate) fn finality_score(&self, blue_score: u64, pruning_finality: u64) -> u64 {
+ if pruning_finality == 0 {
+ // Handle the error appropriately, e.g., return an error or default value
+ return 0; // or consider returning an error
+ }
blue_score / pruning_finality
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
pub fn prune( | ||||||||||||||||||||||||||||||||||||||||||||
|
@@ -69,17 +63,20 @@ impl<T: ReachabilityStoreReader + Clone> PruningPointManagerT<T> { | |||||||||||||||||||||||||||||||||||||||||||
previous_pruning_point: HashValue, | ||||||||||||||||||||||||||||||||||||||||||||
previous_ghostdata: &GhostdagData, | ||||||||||||||||||||||||||||||||||||||||||||
next_ghostdata: &GhostdagData, | ||||||||||||||||||||||||||||||||||||||||||||
pruning_depth: u64, | ||||||||||||||||||||||||||||||||||||||||||||
pruning_finality: u64, | ||||||||||||||||||||||||||||||||||||||||||||
) -> anyhow::Result<HashValue> { | ||||||||||||||||||||||||||||||||||||||||||||
let min_required_blue_score_for_next_pruning_point = | ||||||||||||||||||||||||||||||||||||||||||||
(self.finality_score(previous_ghostdata.blue_score) + 1) * self.pruning_finality; | ||||||||||||||||||||||||||||||||||||||||||||
(self.finality_score(previous_ghostdata.blue_score, pruning_finality) + 1) | ||||||||||||||||||||||||||||||||||||||||||||
* pruning_finality; | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+66
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate The Consider adding validation at the beginning of the function: pub(crate) fn next_pruning_point(
&self,
previous_pruning_point: HashValue,
previous_ghostdata: &GhostdagData,
next_ghostdata: &GhostdagData,
pruning_depth: u64,
pruning_finality: u64,
) -> anyhow::Result<HashValue> {
+ if pruning_depth == 0 || pruning_finality == 0 {
+ return Err(anyhow::anyhow!("pruning_depth and pruning_finality must be greater than zero"));
+ }
let min_required_blue_score_for_next_pruning_point =
(self.finality_score(previous_ghostdata.blue_score, pruning_finality) + 1)
* pruning_finality; 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
debug!( | ||||||||||||||||||||||||||||||||||||||||||||
"min_required_blue_score_for_next_pruning_point: {:?}", | ||||||||||||||||||||||||||||||||||||||||||||
min_required_blue_score_for_next_pruning_point | ||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||
let mut latest_pruning_ghost_data = previous_ghostdata.to_compact(); | ||||||||||||||||||||||||||||||||||||||||||||
if min_required_blue_score_for_next_pruning_point + self.pruning_depth | ||||||||||||||||||||||||||||||||||||||||||||
if min_required_blue_score_for_next_pruning_point + pruning_depth | ||||||||||||||||||||||||||||||||||||||||||||
<= next_ghostdata.blue_score | ||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||
for child in self.reachability_service().forward_chain_iterator( | ||||||||||||||||||||||||||||||||||||||||||||
|
@@ -92,13 +89,11 @@ impl<T: ReachabilityStoreReader + Clone> PruningPointManagerT<T> { | |||||||||||||||||||||||||||||||||||||||||||
"child: {:?}, observer2.blue_score: {:?}, next_pruning_ghostdata.blue_score: {:?}", | ||||||||||||||||||||||||||||||||||||||||||||
child, next_ghostdata.blue_score, next_pruning_ghostdata.blue_score | ||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||
if next_ghostdata.blue_score - next_pruning_ghostdata.blue_score | ||||||||||||||||||||||||||||||||||||||||||||
< self.pruning_depth | ||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||
if next_ghostdata.blue_score - next_pruning_ghostdata.blue_score < pruning_depth { | ||||||||||||||||||||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+92
to
93
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prevent potential underflow in blue score subtraction In the condition: if next_ghostdata.blue_score - next_pruning_ghostdata.blue_score < pruning_depth {
break;
} There is a potential for underflow if Modify the condition to ensure +if next_pruning_ghostdata.blue_score > next_ghostdata.blue_score {
+ break;
+}
if next_ghostdata.blue_score - next_pruning_ghostdata.blue_score < pruning_depth {
break;
} |
||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
if self.finality_score(next_pruning_ghostdata.blue_score) | ||||||||||||||||||||||||||||||||||||||||||||
> self.finality_score(latest_pruning_ghost_data.blue_score) | ||||||||||||||||||||||||||||||||||||||||||||
if self.finality_score(next_pruning_ghostdata.blue_score, pruning_finality) | ||||||||||||||||||||||||||||||||||||||||||||
> self.finality_score(latest_pruning_ghost_data.blue_score, pruning_finality) | ||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||
latest_pruning_ghost_data = CompactGhostdagData { | ||||||||||||||||||||||||||||||||||||||||||||
blue_score: next_pruning_ghostdata.blue_score, | ||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -373,13 +373,11 @@ impl Genesis { | |
pub fn init_storage_for_test_with_param( | ||
net: &ChainNetwork, | ||
k: KType, | ||
pruning_depth: u64, | ||
pruning_finality: u64, | ||
) -> Result<(Arc<Storage>, ChainInfo, Self, BlockDAG)> { | ||
debug!("init storage by genesis for test. {net:?}"); | ||
let storage = Arc::new(Storage::new(StorageInstance::new_cache_instance())?); | ||
let genesis = Self::load_or_build(net)?; | ||
let dag = BlockDAG::create_for_testing_with_parameters(k, pruning_depth, pruning_finality)?; | ||
let dag = BlockDAG::create_for_testing_with_parameters(k)?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Update all instances of The function
Ensure that any additional call sites are similarly updated to align with the new function signature. 🔗 Analysis chainUpdate function signature to match the new usage. The Consider updating the function signature to match the new usage: - pub fn init_storage_for_test_with_param(
- net: &ChainNetwork,
- k: KType,
- pruning_depth: u64,
- pruning_finality: u64,
- ) -> Result<(Arc<Storage>, ChainInfo, Self, BlockDAG)> {
+ pub fn init_storage_for_test_with_param(
+ net: &ChainNetwork,
+ k: KType,
+ ) -> Result<(Arc<Storage>, ChainInfo, Self, BlockDAG)> { Also, ensure that all calls to this function throughout the codebase are updated accordingly. To verify the impact of this change, let's search for other occurrences of this function: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for other occurrences of init_storage_for_test_with_param
rg "init_storage_for_test_with_param" --type rust
Length of output: 239 |
||
let chain_info = genesis.execute_genesis_block(net, storage.clone(), dag.clone())?; | ||
Ok((storage, chain_info, genesis, dag)) | ||
} | ||
|
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.
Improve code readability and fix typo.
The addition of new parameters to
calc_mergeset_and_tips
enhances the pruning logic. However, there are a couple of issues to address:The use of magic numbers (4 and 3) reduces code readability and maintainability. Consider defining these as named constants with clear descriptions of their purpose.
There's a typo in the variable name
prevous_ghostdata
(should beprevious_ghostdata
).Here's a suggested improvement:
Also, consider adding comments explaining the significance of
PRUNING_INTERVAL
andPRUNING_DEPTH
.