Clean the pruning log and add config for parallel and parents count #4372
Conversation
WalkthroughThis pull request introduces several configuration and functionality enhancements across the Starcoin blockchain's core components. The primary changes focus on improving block production, synchronization, and pruning processes. Key modifications include adding a new 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: 4
🧹 Nitpick comments (7)
flexidag/src/blockdag.rs (3)
106-111: Consider refactoring duplicated error handling codeThe error handling logic in lines 106-111 is similar to the code in lines 121-126. To improve maintainability and reduce code duplication, consider refactoring this logic into a helper function.
121-126: Consider refactoring duplicated error handling codeSimilar to lines 106-111, the error handling in lines 121-126 can be refactored into a helper function to enhance code readability and reduce duplication.
719-721: Format multi-line expression as a single lineThe pipeline warning indicates that the multi-line expression in lines 719-721 should be formatted as a single line to adhere to code style guidelines.
Consider reformatting the code as:
let next_pruning_point = self.pruning_point_manager().next_pruning_point(previous_pruning_point, previous_ghostdata, &next_ghostdata, pruning_depth, pruning_finality)?;🧰 Tools
🪛 GitHub Actions: Build and Test
[warning] 719-721: Code formatting issue: Multi-line expression should be formatted as a single line
config/src/miner_config.rs (2)
10-10: Document the rationale for G_MAX_PARENTS_COUNT value.Please add a doc comment explaining why 16 was chosen as the default maximum parents count. This helps future maintainers understand the reasoning behind this constant.
+/// Default maximum number of parent blocks allowed. +/// Set to 16 because... <add rationale here> pub static G_MAX_PARENTS_COUNT: u64 = 16;
70-72: Add documentation for the maximum_parents_count method.The method lacks documentation explaining its purpose and behavior.
+ /// Returns the configured maximum number of parent blocks allowed. + /// If not configured, returns the default value from G_MAX_PARENTS_COUNT. pub fn maximum_parents_count(&self) -> u64 { self.maximum_parents_count.unwrap_or(G_MAX_PARENTS_COUNT) }flexidag/src/ghostdag/protocol.rs (1)
489-491: Add documentation for the k method.Please add documentation explaining the purpose of this method and what the k value represents in the GHOSTDAG protocol.
+ /// Returns the k parameter of the GHOSTDAG protocol. + /// This parameter limits the size of the anticone that can be blue. + /// For more details, see: https://eprint.iacr.org/2018/104.pdf pub fn k(&self) -> KType { self.k }chain/src/chain.rs (1)
1384-1397: LGTM! Good addition of latest pruning point to ghostdata verification.The changes properly handle the pruning point retrieval with appropriate error handling. Consider extracting the pruning point retrieval logic into a separate method for better readability.
impl ChainReader for BlockChain { + fn get_latest_pruning_point(&self) -> Result<Option<HashValue>> { + Ok(match self.storage.get_startup_info()? { + Some(startup_info) => self + .storage + .get_block_header_by_hash(startup_info.main)? + .map(|head_block| head_block.pruning_point()), + None => None, + }) + } + fn verify_and_ghostdata( &self, uncles: &[BlockHeader], header: &BlockHeader, ) -> Result<starcoin_dag::types::ghostdata::GhostdagData> { - let latest_pruning_point = { - match self.storage.get_startup_info().unwrap_or(None) { - Some(startup_info) => self - .storage - .get_block_header_by_hash(startup_info.main) - .unwrap_or(None) - .map(|head_block| head_block.pruning_point()), - None => None, - } - }; - + let latest_pruning_point = self.get_latest_pruning_point()?; Ok(self .dag() .verify_and_ghostdata(uncles, header, latest_pruning_point)?) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
chain/mock/src/mock_chain.rs(2 hunks)chain/src/chain.rs(1 hunks)config/src/lib.rs(1 hunks)config/src/miner_config.rs(4 hunks)config/src/sync_config.rs(3 hunks)flexidag/src/blockdag.rs(9 hunks)flexidag/src/consensusdb/db.rs(1 hunks)flexidag/src/ghostdag/protocol.rs(1 hunks)flexidag/tests/tests.rs(3 hunks)sync/src/block_connector/block_connector_service.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- flexidag/tests/tests.rs
🧰 Additional context used
🪛 GitHub Actions: Build and Test
flexidag/src/blockdag.rs
[error] 709-709: Left behind trailing whitespace at the end of line
[error] 726-726: Left behind trailing whitespace at the end of line
[warning] 719-721: Code formatting issue: Multi-line expression should be formatted as a single line
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run benchmark
🔇 Additional comments (9)
config/src/sync_config.rs (3)
32-42: Well-defined addition of 'lightweight_sync_max_gap' fieldThe addition of the
lightweight_sync_max_gapfield enhances synchronization configuration flexibility. The field is properly annotated with serialization attributes and command-line argument definitions.
54-56: Accessor method for 'lightweight_sync_max_gap' is correctly implementedThe
lightweight_sync_max_gapmethod provides convenient access to the new configuration field.
69-71: Correct merging logic for 'lightweight_sync_max_gap'The
merge_with_optmethod appropriately updateslightweight_sync_max_gapfrom command-line options if provided.flexidag/src/consensusdb/db.rs (1)
107-107: Verify impact of changing from synchronous to asynchronous batch writeSwitching from
raw_write_batch_synctoraw_write_batchmay affect data durability guarantees. Ensure that this change does not introduce data consistency issues, especially in scenarios involving system crashes or power failures.To confirm whether data durability is maintained, please review the write options and ensure that write-ahead logging or synchronous writes are appropriately configured if required.
config/src/miner_config.rs (1)
116-119: LGTM!The implementation of merging the maximum_parents_count configuration is correct.
chain/mock/src/mock_chain.rs (1)
7-7: LGTM!The import and usage of G_MAX_PARENTS_COUNT is appropriate for the pruning functionality.
Also applies to: 270-270
config/src/lib.rs (1)
36-36: LGTM!Making the miner_config module public is appropriate as it contains the new G_MAX_PARENTS_COUNT constant that needs to be accessible from other crates.
sync/src/block_connector/block_connector_service.rs (2)
139-152: LGTM! Good improvements to the gap calculation logic.The changes make the gap configurable while maintaining safe arithmetic operations. The added debug logging will help with troubleshooting synchronization issues.
456-456: LGTM! Good addition of configurable maximum parents count.The change allows flexible configuration of the maximum number of parent blocks during mining.
jackzhhuang
force-pushed
the
enhance-parallel3
branch
from
f2953b1 to
9ac9006
Compare
…ain net 2, add checking for the first pruning point
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
flexidag/src/blockdag.rs (1)
777-796: Adjust logging levels for detailed messagesMultiple
info!log statements are used withincheck_historical_blockfor detailed conditional messages. Consider usingdebug!ortrace!levels for these logs to prevent cluttering the info logs and to better categorize log severity.chain/src/chain.rs (1)
1385-1394: Simplify nested unwrap_or calls for better readability.The nested
unwrap_orcalls make the code harder to follow. Consider refactoring for better readability and error handling.- let latest_pruning_point = { - match self.storage.get_startup_info().unwrap_or(None) { - Some(startup_info) => self - .storage - .get_block_header_by_hash(startup_info.main) - .unwrap_or(None) - .map(|head_block| head_block.pruning_point()), - None => None, - } - }; + let latest_pruning_point = self + .storage + .get_startup_info() + .ok() + .flatten() + .and_then(|startup_info| { + self.storage + .get_block_header_by_hash(startup_info.main) + .ok() + .flatten() + .map(|head_block| head_block.pruning_point()) + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
chain/mock/src/mock_chain.rs(2 hunks)chain/src/chain.rs(2 hunks)config/src/lib.rs(1 hunks)config/src/miner_config.rs(4 hunks)config/src/sync_config.rs(3 hunks)flexidag/src/blockdag.rs(9 hunks)flexidag/src/consensusdb/db.rs(1 hunks)flexidag/src/ghostdag/protocol.rs(1 hunks)flexidag/tests/tests.rs(3 hunks)sync/src/block_connector/block_connector_service.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- config/src/lib.rs
- flexidag/tests/tests.rs
- flexidag/src/ghostdag/protocol.rs
- chain/mock/src/mock_chain.rs
- flexidag/src/consensusdb/db.rs
- config/src/miner_config.rs
🔇 Additional comments (5)
config/src/sync_config.rs (3)
31-42: LGTMThe addition of
lightweight_sync_max_gaptoSyncConfigis implemented correctly, enhancing synchronization flexibility.
54-56: LGTMThe new method
lightweight_sync_max_gapcorrectly returns the configuration value.
69-71: LGTMThe
merge_with_optmethod properly merges thelightweight_sync_max_gapoption from command-line arguments.sync/src/block_connector/block_connector_service.rs (1)
139-152: LGTMThe updated logic in
is_near_blockcorrectly utilizes the newlightweight_sync_max_gapconfiguration parameter, improving synchronization behavior.chain/src/chain.rs (1)
1396-1398: LGTM! The DAG verification call is properly updated.The changes correctly pass the latest pruning point to the DAG's verify_and_ghostdata method while maintaining proper error handling.
| let a_blue_work = self | ||
| .storage | ||
| .ghost_dag_store | ||
| .get_blue_work(*a) | ||
| .unwrap_or_else(|e| { | ||
| panic!( | ||
| "the ghostdag data should be existed for {:?}, e: {:?}", | ||
| a, e | ||
| ) | ||
| }); | ||
| let b_blue_work = self | ||
| .storage | ||
| .ghost_dag_store | ||
| .get_blue_work(*b) | ||
| .unwrap_or_else(|e| { | ||
| panic!( | ||
| "the ghostdag data should be existed for {:?}, e: {:?}", | ||
| b, e | ||
| ) | ||
| }); | ||
| if a_blue_work == b_blue_work { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid panicking inside the sorting comparator
Using panic! inside the sorted_by comparator may cause the application to terminate unexpectedly if get_blue_work fails. Consider handling the error gracefully by propagating the error or using a default value.
Apply this diff to handle the error without panicking:
.sorted_by(|a, b| {
let a_blue_work = self
.storage
.ghost_dag_store
.get_blue_work(*a)
- .unwrap_or_else(|e| {
- panic!(
- "the ghostdag data should be existed for {:?}, e: {:?}",
- a, e
- )
- });
+ .unwrap_or_default();
let b_blue_work = self
.storage
.ghost_dag_store
.get_blue_work(*b)
- .unwrap_or_else(|e| {
- panic!(
- "the ghostdag data should be existed for {:?}, e: {:?}",
- b, e
- )
- });
+ .unwrap_or_default();
if a_blue_work == b_blue_work {
a.cmp(b)
} else {
b_blue_work.cmp(&a_blue_work)
}
})📝 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 a_blue_work = self | |
| .storage | |
| .ghost_dag_store | |
| .get_blue_work(*a) | |
| .unwrap_or_else(|e| { | |
| panic!( | |
| "the ghostdag data should be existed for {:?}, e: {:?}", | |
| a, e | |
| ) | |
| }); | |
| let b_blue_work = self | |
| .storage | |
| .ghost_dag_store | |
| .get_blue_work(*b) | |
| .unwrap_or_else(|e| { | |
| panic!( | |
| "the ghostdag data should be existed for {:?}, e: {:?}", | |
| b, e | |
| ) | |
| }); | |
| if a_blue_work == b_blue_work { | |
| let a_blue_work = self | |
| .storage | |
| .ghost_dag_store | |
| .get_blue_work(*a) | |
| .unwrap_or_default(); | |
| let b_blue_work = self | |
| .storage | |
| .ghost_dag_store | |
| .get_blue_work(*b) | |
| .unwrap_or_default(); | |
| if a_blue_work == b_blue_work { |
Benchmark for 94baa71Click to view benchmark
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
flexidag/src/blockdag.rs (1)
654-692:⚠️ Potential issueReplace panic with safe error handling in the sorting comparator.
Using
panic!inside a sorting comparator is dangerous as it can crash the application. Consider usingunwrap_or_default()or a default comparison whenget_blue_workfails.This is a duplicate of a previous review comment. The suggested fix remains the same:
- Replace panic with safe error handling
- Use default values when ghost data retrieval fails
🧹 Nitpick comments (2)
flexidag/src/prune/pruning_point_manager.rs (2)
45-47: Document the zero hash case handling.The special handling of
HashValue::zero()should be documented to explain why this case warrants immediate return of current tips. This helps maintainers understand the relationship between zero hash and pruning point equality.Add a comment explaining the zero hash case:
+ // HashValue::zero() indicates [explain special case], which should be treated + // the same as when current and next pruning points match if next_pruning_point == HashValue::zero() { return Ok(dag_state.tips.clone()); }
45-50: Consider refactoring duplicate return logic.Both conditions (
HashValue::zero()and equality check) return the same result. Consider combining them to improve maintainability.- if next_pruning_point == HashValue::zero() { - return Ok(dag_state.tips.clone()); - } - if current_pruning_point == next_pruning_point { + if next_pruning_point == HashValue::zero() || current_pruning_point == next_pruning_point { return Ok(dag_state.tips.clone()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
flexidag/src/blockdag.rs(10 hunks)flexidag/src/prune/pruning_point_manager.rs(1 hunks)
🔇 Additional comments (4)
flexidag/src/blockdag.rs (4)
Line range hint
106-126: LGTM! Improved error handling implementation.The refactored error handling is more efficient and provides better error messages. Using
has()checks instead of direct retrieval is a good optimization.
767-803: LGTM! Well-documented historical block check implementation.The function is well-structured with:
- Clear documentation explaining the purpose and special cases
- Comprehensive logging for each decision path
- Proper handling of edge cases, especially for the Vega chain
806-817: LGTM! Clean implementation of verification logic.The function effectively:
- Uses the new historical block check to determine processing path
- Properly delegates to appropriate processing methods
- Maintains clean error handling
Line range hint
323-355: Fix inconsistent parent hash usage in virtual selected parent hints.There's an inconsistency in the parent hash used for hinting virtual selected parent:
- In the first implementation (lines 330), it uses
header.parent_hash()- In the second implementation (line 485), it also uses
header.parent_hash()- However, in the first case, this is used when
header.pruning_point()is zero, while in the second case, it's used when the pruning point differs from the reindex root.This inconsistency could lead to different behavior in these two scenarios.
Let's verify the impact:
Also applies to: 483-490
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
Configuration Enhancements
Synchronization Improvements
Performance Optimizations
New Features
kvalue in the GhostdagManager.