Release v2.0.3 #4243
Release v2.0.3 #4243
Conversation
WalkthroughThe pull request involves multiple changes across various files, primarily focusing on dependency updates, method additions, and modifications related to pruning configurations within the blockchain framework. Notable updates include the revision of the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (5)
chain/api/src/chain.rs (1)
119-119: Add documentation for the new method.The addition of the
get_pruning_configmethod is appropriate and aligns with the pruning-related changes mentioned in the PR summary. However, to improve code clarity and maintainability, please add documentation for this method.Consider adding a doc comment explaining the purpose of the method and the meaning of the returned values. For example:
/// Returns the pruning configuration as a tuple of (pruning_depth, pruning_finality). /// /// - `pruning_depth`: The depth at which pruning occurs in the blockchain. /// - `pruning_finality`: The number of blocks to keep for finality purposes. fn get_pruning_config(&self) -> (u64, u64);This documentation will help other developers understand the purpose and usage of this method.
state/api/src/chain_state.rs (1)
204-206: LGTM: New method for retrieving DAG configurationThe
get_dag_configmethod is a well-implemented addition to theAccountStateReaderstruct. It correctly uses the existingget_on_chain_configmethod to retrieve theFlexiDagConfigV2.Consider adding a brief documentation comment to explain the purpose of this method and what
FlexiDagConfigV2represents. For example:/// Retrieves the on-chain FlexiDagConfigV2, which contains parameters for pruning depth and finality. pub fn get_dag_config(&self) -> Result<Option<FlexiDagConfigV2>> { self.get_on_chain_config::<FlexiDagConfigV2>() }sync/src/parallel/executor.rs (1)
106-110: Approve with suggestions: Sleep duration added to reduce CPU usageThe addition of a sleep duration after yielding control is a good approach to reduce CPU usage when waiting for parent blocks. This change likely improves overall system efficiency by allowing other tasks to proceed.
However, consider the following suggestions:
Make the sleep duration configurable:
Instead of hardcoding the 100ms duration, consider making it a configurable parameter. This would allow for fine-tuning based on different network conditions or system requirements.Add a comment explaining the purpose:
To improve code maintainability, add a brief comment explaining why this sleep was introduced.Here's a suggested implementation incorporating these changes:
// Configuration struct (add this at an appropriate place in your code) pub struct DagBlockExecutorConfig { pub parent_block_wait_sleep_duration: Duration, } // In the DagBlockExecutor struct, add: config: DagBlockExecutorConfig, // In the start_to_execute method: Ok(false) => { tokio::task::yield_now().await; // Sleep to reduce CPU usage while waiting for parent blocks tokio::time::sleep(self.config.parent_block_wait_sleep_duration).await }This approach allows for easier adjustment of the sleep duration and clearly communicates the purpose of the added delay.
chain/mock/src/mock_chain.rs (1)
47-47: LGTM! Consider updating the method name.The simplification of the
new_with_paramsmethod by removing the pruning-related parameters is a good change. It aligns with the overall goal of reducing complexity in method signatures.Consider renaming the method to
new_with_k_paramor something similar, as it now only takes thekparameter in addition to the network. This would make the method name more descriptive of its current functionality.genesis/src/lib.rs (1)
Line range hint
1-524: Summary: Update related to pruning configuration removalThis change appears to be part of a larger refactoring effort to remove pruning-related parameters from the BlockDAG creation process. While the modification itself is straightforward, it's important to ensure that this change is consistent across the entire codebase.
Key points:
- The
BlockDAG::create_for_testing_with_parameterscall has been updated to removepruning_depthandpruning_finalityparameters.- The function signature of
init_storage_for_test_with_paramneeds to be updated to reflect this change.- All calls to
init_storage_for_test_with_paramthroughout the project should be reviewed and updated if necessary.Please ensure that these changes align with the overall project goals regarding pruning configuration and that all related parts of the codebase are consistently updated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
- Cargo.toml (1 hunks)
- chain/api/src/chain.rs (1 hunks)
- chain/mock/src/mock_chain.rs (2 hunks)
- chain/src/chain.rs (4 hunks)
- chain/tests/test_prune.rs (1 hunks)
- flexidag/src/blockdag.rs (6 hunks)
- flexidag/src/prune/pruning_point_manager.rs (3 hunks)
- flexidag/tests/tests.rs (4 hunks)
- genesis/src/lib.rs (1 hunks)
- node/src/node.rs (0 hunks)
- state/api/src/chain_state.rs (2 hunks)
- sync/src/block_connector/block_connector_service.rs (1 hunks)
- sync/src/parallel/executor.rs (1 hunks)
- vm/types/src/on_chain_config/flexi_dag_config.rs (2 hunks)
- vm/types/src/on_chain_config/mod.rs (0 hunks)
💤 Files with no reviewable changes (2)
- node/src/node.rs
- vm/types/src/on_chain_config/mod.rs
🧰 Additional context used
🔇 Additional comments (24)
chain/tests/test_prune.rs (1)
9-9: LGTM. Verify test effectiveness with new initialization.The simplification of the
MockChain::new_with_paramsmethod call aligns with the broader modifications in theMockChainstruct. This change removes the explicit setting ofpruning_depthandpruning_finalityparameters.Please ensure that:
- The test still effectively checks the pruning functionality with the new initialization.
- If the removed parameters (4 and 3) were significant for this specific test case, verify that their absence doesn't reduce the test's coverage.
Consider updating the test documentation to reflect this change in initialization, explaining how pruning parameters are now determined if it's relevant to understanding the test setup.
state/api/src/chain_state.rs (2)
21-21: LGTM: New import for FlexiDagConfigV2The import of
FlexiDagConfigV2fromstarcoin_vm_types::on_chain_configis consistent with the existing import style and is necessary for the new method being added.
21-21: Summary: Successful integration of FlexiDagConfigV2The changes in this file are minimal and well-integrated:
- A new import for
FlexiDagConfigV2has been added.- A new method
get_dag_confighas been implemented in theAccountStateReaderstruct.These additions enhance the functionality related to DAG configuration retrieval, which aligns with the PR's focus on improving pruning configurations. The changes are consistent with the existing code structure and do not introduce any apparent issues or conflicts.
Also applies to: 204-206
Cargo.toml (2)
Line range hint
1-457: File review completeThe only significant change in this file is the update to the
starcoin-frameworkdependency. The rest of theCargo.tomlconfiguration remains unchanged. This update is likely part of regular maintenance to keep dependencies up-to-date.
457-457: Dependency update: starcoin-frameworkThe
starcoin-frameworkdependency has been updated to a new commit hash:-starcoin-framework = { git = "https://github.com/starcoinorg/starcoin-framework", rev = "94bcd77e80232b169cf95754ef4e87775645afcd" } +starcoin-framework = { git = "https://github.com/starcoinorg/starcoin-framework", rev = "a2546b32ad26d048e97d729e45153e0cba26ba18" }This update likely includes bug fixes, feature additions, or performance improvements in the starcoin-framework library. Please ensure that this change is compatible with the current codebase and doesn't introduce any breaking changes.
To verify the changes in the starcoin-framework between the two versions, you can run the following script:
This script will show the commit messages and detailed changes between the two versions, helping you understand the scope of the update.
✅ Verification successful
Dependency update verified: starcoin-framework
The
starcoin-frameworkdependency has been successfully updated to commita2546b32ad26d048e97d729e45153e0cba26ba18. The changes introduced are additive, including new configurations and upgrade scripts, and do not modify or remove existing functionality. This update is compatible with the current codebase and does not introduce any breaking changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare changes between two versions of starcoin-framework OLD_REV="94bcd77e80232b169cf95754ef4e87775645afcd" NEW_REV="a2546b32ad26d048e97d729e45153e0cba26ba18" echo "Comparing starcoin-framework changes between $OLD_REV and $NEW_REV" echo "----------------------------------------------------------------" git clone https://github.com/starcoinorg/starcoin-framework.git tmp_starcoin_framework cd tmp_starcoin_framework git log --oneline $OLD_REV..$NEW_REV echo "----------------------------------------------------------------" echo "Detailed diff:" git diff $OLD_REV $NEW_REV cd .. rm -rf tmp_starcoin_frameworkLength of output: 3779
flexidag/tests/tests.rs (5)
892-892: Update BlockDAG creation to use onlykparameterThe
BlockDAG::create_for_testing_with_parametersmethod now only takes thekparameter. This change simplifies the BlockDAG creation for testing purposes.
1055-1060: Consistent update ofcalc_mergeset_and_tipsmethod callThis is another instance of updating the
calc_mergeset_and_tipsmethod call with the new pruning parameters. The change is consistent with the previous update.
1077-1077: Update BlockDAG creation intest_verification_blue_blockSimilar to the change in the
test_prunefunction, the BlockDAG creation here has been updated to use only thekparameter. This change maintains consistency across the test functions.
Line range hint
892-1077: Summary of changes in flexidag/tests/tests.rsThe modifications in this file focus on updating the BlockDAG creation and pruning-related method calls in the test functions. Key changes include:
- Simplifying
BlockDAG::create_for_testing_with_parametersto use only thekparameter.- Updating
calc_mergeset_and_tipsmethod calls to includepruning_depthandpruning_finalityparameters.These changes improve the flexibility of pruning configuration in tests and maintain consistency across different test functions. The modifications appear to be part of a larger refactoring effort to enhance the BlockDAG implementation.
No issues or errors were introduced by these changes, and they seem to align well with the intended improvements to the pruning functionality.
1014-1019: Updatecalc_mergeset_and_tipsmethod call with pruning parametersThe
calc_mergeset_and_tipsmethod now includespruning_depthandpruning_finalityas parameters. This change allows for more flexible pruning configuration during testing.To ensure this change is consistent across the codebase, let's check for other occurrences of
calc_mergeset_and_tips:flexidag/src/prune/pruning_point_manager.rs (2)
79-81: Ensure correctness in pruning conditionThe condition that checks whether pruning should occur:
if min_required_blue_score_for_next_pruning_point + pruning_depth <= next_ghostdata.blue_score { // ... }appears to be correctly implemented, effectively determining if the pruning point needs to be updated based on the provided parameters.
95-96: Comparison of finality scores is correctly implementedThe comparison between the finality scores of
next_pruning_ghostdataandlatest_pruning_ghost_datacorrectly determines if the pruning point should be updated:if self.finality_score(next_pruning_ghostdata.blue_score, pruning_finality) > self.finality_score(latest_pruning_ghost_data.blue_score, pruning_finality) { // Update latest_pruning_ghost_data }This logic aligns with the intended pruning mechanism.
sync/src/block_connector/block_connector_service.rs (2)
411-413: Retrieve pruning configuration parametersThe code correctly retrieves
pruning_depthandpruning_finalityfromget_pruning_config(), preparing them for use in the DAG calculation.
414-419: Updatecalc_mergeset_and_tipscall with pruning parametersThe call to
dag.calc_mergeset_and_tipsnow includespruning_depthandpruning_finality, aligning with the updated method signature and ensuring accurate mergeset and tips calculation.flexidag/src/blockdag.rs (6)
57-57: Simplifiedcreate_blockdagfunctionThe
create_blockdagfunction now callsSelf::newwithDEFAULT_GHOSTDAG_Kanddag_storage, removing unnecessary parameters and simplifying the initialization process.
73-73: InitializePruningPointManagerwithout pruning parametersThe
PruningPointManager::newmethod is now called withoutpruning_depthandpruning_finality. Verify thatPruningPointManageris correctly initialized and that it obtains these parameters when needed, perhaps from a configuration or context.
91-94: Updatedcreate_for_testing_with_parametersFunctionThe
create_for_testing_with_parametersfunction has been modified to excludepruning_depthandpruning_finalityfrom its parameters, aligning it with the changes made to thenewfunction. This streamlines the testing setup.
535-536: Addedpruning_depthandpruning_finalityParameters toverify_pruning_pointSimilarly, the
verify_pruning_pointfunction now includespruning_depthandpruning_finalityas parameters. Verify that all calls to this function have been updated accordingly.To check all invocations of
verify_pruning_point:#!/bin/bash # Description: Find all calls to `verify_pruning_point` and confirm they include the new parameters. rg 'verify_pruning_point\(' src/ -A 3 -B 3Also applies to: 542-543
478-479: Addedpruning_depthandpruning_finalityParameters tocalc_mergeset_and_tipsThe
calc_mergeset_and_tipsfunction now requirespruning_depthandpruning_finalityas parameters. Ensure that all calls to this function provide the correct values for these parameters.To verify all invocations of
calc_mergeset_and_tipsare updated:#!/bin/bash # Description: Find all calls to `calc_mergeset_and_tips` and ensure they pass the new parameters. rg 'calc_mergeset_and_tips\(' src/ -A 3 -B 3Also applies to: 492-493
Line range hint
60-73: Removedpruning_depthandpruning_finalityfromnewfunctionThe
newfunction no longer requirespruning_depthandpruning_finalityas parameters. Ensure that any initialization or configuration dependent on these parameters is appropriately adjusted elsewhere in the codebase.To confirm that the removal of these parameters does not cause any issues, please verify their usage:
chain/src/chain.rs (4)
48-48: LGTM!The import statement for
FlexiDagConfigV2is added correctly.
1393-1393: Verify the correctness ofget_pruning_configusageThe method
get_pruning_configis used to retrieve pruning parameters. Please ensure that this method is correctly implemented and returns the expected values forpruning_depthandpruning_finality.Would you like assistance in verifying the implementation of
get_pruning_config?
1413-1414: Ensure correct parameters forverify_pruning_pointThe parameters
pruning_depthandpruning_finalityare passed toself.dag().verify_pruning_point. Please confirm that these parameters match the method's expected signature and order.Let me know if you need help verifying the method signature.
1428-1429:⚠️ Potential issueFix infinite recursion in
get_pruning_heightmethodThe
get_pruning_heightmethod calls itself recursively without a base case, causing infinite recursion and potential stack overflow.Apply this diff to provide a proper implementation:
-fn get_pruning_height(&self) -> BlockNumber { - self.get_pruning_height() +fn get_pruning_height(&self) -> BlockNumber { + let chain_id = self.status().head().chain_id(); + if chain_id.is_vega() { + 4000000 + } else if chain_id.is_proxima() { + 850000 + } else if chain_id.is_halley() { + 4090000 + } else if chain_id.is_main() { + 1 + } else if chain_id.is_dag_test() || chain_id.is_test() || chain_id.is_dev() { + BlockNumber::MAX + } else { + 1 + } +}Likely invalid or redundant comment.
chain/mock/src/mock_chain.rs
Outdated
| let MineNewDagBlockInfo { | ||
| tips: pruned_tips, | ||
| blue_blocks, | ||
| pruning_point, | ||
| } = self | ||
| .head | ||
| .dag() | ||
| .calc_mergeset_and_tips(previous_pruning, prevous_ghostdata.as_ref())?; | ||
| } = self.head.dag().calc_mergeset_and_tips( | ||
| previous_pruning, | ||
| prevous_ghostdata.as_ref(), | ||
| 4,3 | ||
| )?; |
There was a problem hiding this comment.
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:
+ const PRUNING_INTERVAL: u64 = 4;
+ const PRUNING_DEPTH: u64 = 3;
- prevous_ghostdata
+ previous_ghostdata
- self.head.dag().calc_mergeset_and_tips(
- previous_pruning,
- prevous_ghostdata.as_ref(),
- 4,3
- )?;
+ self.head.dag().calc_mergeset_and_tips(
+ previous_pruning,
+ previous_ghostdata.as_ref(),
+ PRUNING_INTERVAL,
+ PRUNING_DEPTH
+ )?;Also, consider adding comments explaining the significance of PRUNING_INTERVAL and PRUNING_DEPTH.
Committable suggestion was skipped due to low confidence.
| ) -> 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.
💡 Codebase verification
Update all instances of init_storage_for_test_with_param to match the new signature.
The function init_storage_for_test_with_param has been modified to remove the pruning_depth and pruning_finality parameters in genesis/src/lib.rs. However, there is at least one other usage in chain/mock/src/mock_chain.rs that still includes these parameters. All call sites must be updated to ensure consistency and prevent potential issues.
-
File:
chain/mock/src/mock_chain.rsUpdate the function call to remove the deprecated parameters:
- let (storage, chain_info, _, dag) = Genesis::init_storage_for_test_with_param(&net, k, pruning_depth, pruning_finality)?; + let (storage, chain_info, _, dag) = Genesis::init_storage_for_test_with_param(&net, k)?;
Ensure that any additional call sites are similarly updated to align with the new function signature.
🔗 Analysis chain
Update function signature to match the new usage.
The BlockDAG::create_for_testing_with_parameters call has been modified to remove the pruning_depth and pruning_finality parameters. However, the function signature of init_storage_for_test_with_param still includes these parameters. This inconsistency should be addressed.
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 executed
The 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
| pub fn type_tag() -> TypeTag { | ||
| TypeTag::Struct(Box::new(StructTag { | ||
| address: CORE_CODE_ADDRESS, | ||
| module: Identifier::new(MV_FLEXI_DAG_CONFIG_MODULE_NAME).unwrap(), | ||
| name: Identifier::new(MV_FLEXI_DAG_CONFIG_STRUCT_NAME_V2).unwrap(), | ||
| type_params: vec![], | ||
| })) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Duplicate type_tag Methods to Reduce Code Duplication
The type_tag methods in both FlexiDagConfig and FlexiDagConfigV2 are nearly identical except for the struct name. To adhere to the DRY (Don't Repeat Yourself) principle and enhance maintainability, consider extracting the common logic into a helper function.
Example refactoring:
fn generate_type_tag(struct_name: &str) -> TypeTag {
TypeTag::Struct(Box::new(StructTag {
address: CORE_CODE_ADDRESS,
module: Identifier::new(MV_FLEXI_DAG_CONFIG_MODULE_NAME).unwrap(),
name: Identifier::new(struct_name).unwrap(),
type_params: vec![],
}))
}
impl FlexiDagConfig {
pub fn type_tag() -> TypeTag {
generate_type_tag(MV_FLEXI_DAG_CONFIG_STRUCT_NAME)
}
}
impl FlexiDagConfigV2 {
pub fn type_tag() -> TypeTag {
generate_type_tag(MV_FLEXI_DAG_CONFIG_STRUCT_NAME_V2)
}
}| const G_PRUNING_DEPTH: u64 = 17280; | ||
| const G_PRUNING_FINALITY: u64 = 8640; |
There was a problem hiding this comment.
Use Consistent Naming for Constants
The constants G_PRUNING_DEPTH and G_PRUNING_FINALITY use the G_ prefix, whereas existing constants use the MV_ prefix (e.g., MV_FLEXI_DAG_CONFIG_MODULE_NAME). For consistency, consider aligning the naming convention across constants.
Apply this diff to rename the constants:
-const G_PRUNING_DEPTH: u64 = 17280;
-const G_PRUNING_FINALITY: u64 = 8640;
+const MV_PRUNING_DEPTH: u64 = 17280;
+const MV_PRUNING_FINALITY: u64 = 8640;Update all references to these constants accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const G_PRUNING_DEPTH: u64 = 17280; | |
| const G_PRUNING_FINALITY: u64 = 8640; | |
| const MV_PRUNING_DEPTH: u64 = 17280; | |
| const MV_PRUNING_FINALITY: u64 = 8640; |
| impl Default for FlexiDagConfigV2 { | ||
| fn default() -> Self { | ||
| Self { | ||
| pruning_depth: G_PRUNING_DEPTH, | ||
| pruning_finality: G_PRUNING_FINALITY, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encapsulate Default Values as Associated Constants
To improve code organization and encapsulation, consider defining PRUNING_DEPTH and PRUNING_FINALITY as associated constants within FlexiDagConfigV2. This keeps related values within the struct's scope and enhances readability.
Apply this refactoring:
-const G_PRUNING_DEPTH: u64 = 17280;
-const G_PRUNING_FINALITY: u64 = 8640;
impl Default for FlexiDagConfigV2 {
fn default() -> Self {
Self {
- pruning_depth: G_PRUNING_DEPTH,
- pruning_finality: G_PRUNING_FINALITY,
+ pruning_depth: Self::PRUNING_DEPTH_DEFAULT,
+ pruning_finality: Self::PRUNING_FINALITY_DEFAULT,
}
}
}
+impl FlexiDagConfigV2 {
+ pub const PRUNING_DEPTH_DEFAULT: u64 = 17280;
+ pub const PRUNING_FINALITY_DEFAULT: u64 = 8640;
+
+ // existing methods...
+}Committable suggestion was skipped due to low confidence.
| if next_ghostdata.blue_score - next_pruning_ghostdata.blue_score < pruning_depth { | ||
| break; |
There was a problem hiding this comment.
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 next_pruning_ghostdata.blue_score is greater than next_ghostdata.blue_score, which could cause unexpected behavior due to wrapping of unsigned integers. Consider adjusting the logic to prevent underflow.
Modify the condition to ensure next_pruning_ghostdata.blue_score is not greater than next_ghostdata.blue_score:
+if next_pruning_ghostdata.blue_score > next_ghostdata.blue_score {
+ break;
+}
if next_ghostdata.blue_score - next_pruning_ghostdata.blue_score < pruning_depth {
break;
}| pub(crate) fn finality_score(&self, blue_score: u64, pruning_finality: u64) -> u64 { | ||
| blue_score / pruning_finality |
There was a problem hiding this comment.
Prevent potential division by zero in finality_score
The finality_score function divides blue_score by pruning_finality, but there is no check to ensure pruning_finality is not zero. This could lead to a division by zero error. Consider adding a check to prevent this scenario.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(crate) fn finality_score(&self, blue_score: u64, pruning_finality: u64) -> u64 { | |
| blue_score / pruning_finality | |
| 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 | |
| } |
| 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; |
There was a problem hiding this comment.
Validate pruning_depth and pruning_finality parameters
The next_pruning_point function now accepts pruning_depth and pruning_finality as parameters. To ensure robust operation, validate that these parameters are greater than zero to prevent potential runtime errors such as division by zero or incorrect logical comparisons.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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; | |
| 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; |
| 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) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling errors explicitly in get_pruning_config
The method get_pruning_config uses multiple unwrap_or_default() calls, which may suppress errors and return default values unintentionally. Consider handling errors explicitly to ensure any issues are caught and addressed appropriately.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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")) | |
| } | |
| } |
Benchmark for 0871fd5Click to view benchmark
|
sanlee42
force-pushed
the
pruning-hardfork
branch
from
b3e0c88
to
e274d56
Compare
Benchmark for 934e3b7Click to view benchmark
|
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Tests