Fix bug: no store the sync dag block #4259
Conversation
WalkthroughThe pull request introduces several modifications across multiple files. Key changes include the addition of a line to delete all DAG sync blocks in the Changes
Possibly related PRs
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (3)
sync/src/tasks/block_sync_task.rs (3)
457-457: Add logging for DAG sync block deletion.The deletion of DAG sync blocks should be logged for better debugging and monitoring. Also, consider moving this cleanup operation to after confirming successful sync of the current batch to prevent potential issues if sync fails.
- self.sync_dag_store.delete_all_dag_sync_block()?; + let deleted_count = self.sync_dag_store.delete_all_dag_sync_block()?; + info!( + "Deleted {} DAG sync blocks at checkpoint. block_number: {}, is_target_reached: {}", + deleted_count, + block_header.number(), + block_header.number() >= self.target.target_id.number() + );
Line range hint
519-553: Improve error handling and performance in fetch_blocks.The block fetching and saving logic could benefit from the following improvements:
- Better error handling beyond debug logging
- Batched operations for local store saves
- Metrics for monitoring fetch operations
async fn fetch_blocks(&self, mut block_ids: Vec<HashValue>) -> Result<Vec<BlockHeader>> { let mut result = vec![]; + let mut failed_saves = 0; block_ids.retain(|id| { match self.local_store.get_dag_sync_block(*id) { Ok(op_dag_sync_block) => { if let Some(dag_sync_block) = op_dag_sync_block { match self.sync_dag_store.save_block(dag_sync_block.block.clone()) { Ok(_) => { result.push(dag_sync_block.block.header().clone()); false // read from local store, remove from p2p request } Err(e) => { - debug!("failed to save block for: {:?}", e); + failed_saves += 1; + error!( + "Failed to save block to sync_dag_store: {:?}, block_id: {:?}", + e, id + ); true // need retaining } } } else { true // need retaining } } Err(_) => true, // need retaining } }); + if failed_saves > 0 { + warn!("Failed to save {} blocks to sync_dag_store", failed_saves); + } + + // Process remote blocks in batches for chunk in block_ids.chunks(usize::try_from(MAX_BLOCK_REQUEST_SIZE)?) { + info!("Fetching {} blocks from remote peers", chunk.len()); let remote_dag_sync_blocks = self.fetcher.fetch_blocks(chunk.to_vec()).await?; + let mut save_errors = 0; for (block, _) in remote_dag_sync_blocks { - self.local_store + if let Err(e) = self.local_store .save_dag_sync_block(starcoin_storage::block::DagSyncBlock { block: block.clone(), children: vec![], - })?; - self.sync_dag_store.save_block(block.clone())?; + }) { + save_errors += 1; + error!("Failed to save block to local store: {:?}", e); + continue; + } + if let Err(e) = self.sync_dag_store.save_block(block.clone()) { + save_errors += 1; + error!("Failed to save block to sync_dag_store: {:?}", e); + continue; + } result.push(block.header().clone()); } + if save_errors > 0 { + warn!("Failed to save {} blocks from remote peers", save_errors); + } } Ok(result) }
Line range hint
1-1000: Consider architectural improvements for sync reliability.The current implementation has several architectural aspects that could be improved:
Storage Layer Consistency:
- Consider implementing a transaction-like mechanism to ensure consistency between
local_storeandsync_dag_store- Add recovery mechanisms for partial failures during block saves
Error Handling Strategy:
- Implement a retry mechanism for failed block saves
- Consider adding circuit breakers for remote fetching
Monitoring:
- Add metrics for sync progress, success rates, and storage operations
- Consider implementing health checks for the sync process
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
flexidag/src/prune/pruning_point_manager.rs (2)
78-82: Improve debug log formatting for better readability.While adding debug logging is valuable for troubleshooting, the current format could be improved for readability.
Consider this more structured format:
- debug!("previous_pruning_point: {:?}, previous_ghostdata: {:?}, next_ghostdata: {:?}, pruning_depth: {:?}, pruning_finality: {:?}", - previous_pruning_point, previous_ghostdata, next_ghostdata, - pruning_depth, pruning_finality, - ); + debug!( + "Calculating next pruning point:\n\ + previous_point: {:?}\n\ + previous_ghost: {:?}\n\ + next_ghost: {:?}\n\ + depth: {}\n\ + finality: {}", + previous_pruning_point, + previous_ghostdata, + next_ghostdata, + pruning_depth, + pruning_finality + );
78-82: Add context about the pruning decision.To aid in debugging, consider adding the minimum required blue score to the debug log, as this is a key factor in determining the next pruning point.
Add this information to the debug log:
debug!( - "previous_pruning_point: {:?}, previous_ghostdata: {:?}, next_ghostdata: {:?}, pruning_depth: {:?}, pruning_finality: {:?}", - previous_pruning_point, previous_ghostdata, next_ghostdata, - pruning_depth, pruning_finality, + "Pruning evaluation:\n\ + min_required_blue_score: {}\n\ + previous_point: {:?}\n\ + previous_ghost: {:?}\n\ + next_ghost: {:?}\n\ + depth: {}\n\ + finality: {}", + min_required_blue_score_for_next_pruning_point, + previous_pruning_point, + previous_ghostdata, + next_ghostdata, + pruning_depth, + pruning_finality );flexidag/src/ghostdag/protocol.rs (1)
Line range hint
174-261: Improve error handling consistencyThe function has different error handling behavior based on a magic number (10000000):
- Below this number: Only logs a warning
- Above this number: Fails with an error
This dual behavior could lead to:
- Inconsistent state handling across different block ranges
- Difficulty in debugging issues in lower-numbered blocks
- Potential consensus issues if verification is skipped
Consider:
- Documenting why this threshold exists
- Making the threshold configurable
- Using consistent error handling regardless of block number
- if header.number() < 10000000 { - // no bail before 10000000 - warn!("The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), new_block_data.mergeset_blues); - } else { - bail!("The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), new_block_data.mergeset_blues); - } + let message = format!( + "The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", + header.id(), + blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), + new_block_data.mergeset_blues + ); + // Always log the issue + warn!("{}", message); + // Fail consistently to maintain integrity + bail!("{}", message);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
flexidag/src/blockdag.rs(1 hunks)flexidag/src/ghostdag/protocol.rs(1 hunks)flexidag/src/prune/pruning_point_manager.rs(1 hunks)
flexidag/src/ghostdag/protocol.rs
Outdated
| @@ -171,7 +171,7 @@ impl< | |||
| Ok(new_block_data) | |||
| } | |||
|
|
|||
| pub(crate) fn verify_and_ghostdata( | |||
| pub(crate) fn _verify_and_ghostdata( | |||
There was a problem hiding this comment.
💡 Codebase verification
Remove underscore prefix from verify_and_ghostdata method
The underscore prefix is inconsistent with the method's usage pattern. The evidence shows:
- The method is part of a public API used by other crates (
chain/src/chain.rsandchain/src/verifier/mod.rs) - It's called through
BlockDag::verify_and_ghostdatawhich is a public interface - No direct usage of the underscored version exists in the codebase
The underscore prefix suggests internal use, but this is clearly a public-facing API method. Remove the underscore to match its actual visibility and usage pattern.
🔗 Analysis chain
Clarify method visibility intention
The underscore prefix suggests this method is intended for internal/private use, but it's still marked as pub(crate). This creates confusion about the intended visibility scope.
Let's check if this method is still being used by other modules:
Consider either:
- Making it truly private by removing
pub(crate)if it's only used within this module - Removing the underscore prefix if it's meant to be used by other modules within the crate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining calls to verify_and_ghostdata
rg "verify_and_ghostdata" --type rust
Length of output: 536
Script:
#!/bin/bash
# Let's check the implementation of verify_and_ghostdata in blockdag.rs to understand the relationship
rg -A 10 "verify_and_ghostdata" flexidag/src/blockdag.rs
# Also check the trait implementation in chain/src/chain.rs
rg -A 10 "verify_and_ghostdata" chain/src/chain.rs
Length of output: 1804
Script:
#!/bin/bash
# Let's check the implementation in ghostdag/protocol.rs to understand the full context
rg -A 20 "_verify_and_ghostdata" flexidag/src/ghostdag/protocol.rs
# Also check if there are any direct usages of the underscore version
rg "_verify_and_ghostdata" --type rust
Length of output: 1085
flexidag/src/blockdag.rs
Outdated
| _blue_blocks: &[BlockHeader], | ||
| header: &BlockHeader, | ||
| ) -> Result<GhostdagData, anyhow::Error> { | ||
| self.ghost_dag_manager() | ||
| .verify_and_ghostdata(blue_blocks, header) | ||
| self.ghost_dag_manager().ghostdag(&header.parents()) | ||
| // self.ghost_dag_manager() | ||
| // .verify_and_ghostdata(blue_blocks, header) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Method name is misleading
The method name verify_and_ghostdata suggests it performs verification, but the implementation now only retrieves ghost data without verification. Consider renaming the method to better reflect its current behavior.
-pub fn verify_and_ghostdata(
+pub fn get_ghostdata(
_blue_blocks: &[BlockHeader],
header: &BlockHeader,
) -> Result<GhostdagData, anyhow::Error> {Committable suggestion skipped: line range outside the PR's diff.
Security concern: Block verification has been removed
The implementation no longer verifies the blue blocks before processing them. This could potentially allow invalid blocks to be processed, which may compromise the integrity of the DAG structure.
Consider either:
- Restoring the verification logic
- Documenting why verification is no longer needed
- _blue_blocks: &[BlockHeader],
+ blue_blocks: &[BlockHeader],
header: &BlockHeader,
) -> Result<GhostdagData, anyhow::Error> {
- self.ghost_dag_manager().ghostdag(&header.parents())
- // self.ghost_dag_manager()
- // .verify_and_ghostdata(blue_blocks, header)
+ self.ghost_dag_manager()
+ .verify_and_ghostdata(blue_blocks, header)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
flexidag/src/blockdag.rs(1 hunks)flexidag/src/ghostdag/protocol.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flexidag/src/blockdag.rs
🔇 Additional comments (2)
flexidag/src/ghostdag/protocol.rs (2)
5-5: LGTM: Import changes align with new error handling.
The removal of bail from anyhow imports is consistent with the new error handling approach.
174-174: Remove underscore prefix from method name.
As noted in past reviews, this method is part of a public API used by other crates. The underscore prefix suggests internal use, which is inconsistent with its actual usage pattern.
flexidag/src/ghostdag/protocol.rs
Outdated
| warn!("The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), new_block_data.mergeset_blues); | ||
| return self.ghostdag(&header.parents_hash()); |
There was a problem hiding this comment.
Improve error handling and logging.
The current implementation has several issues:
- Silently falling back to
ghostdagwhen blue sets don't match could mask underlying problems - The warning message could be more descriptive about why the blue sets don't match
Consider this alternative implementation:
- warn!("The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), new_block_data.mergeset_blues);
- return self.ghostdag(&header.parents_hash());
+ let expected_blues: HashSet<_> = blue_blocks.iter().map(|header| header.id()).collect();
+ let actual_blues: HashSet<_> = new_block_data.mergeset_blues.iter().skip(1).cloned().collect();
+ let missing_blues: Vec<_> = expected_blues.difference(&actual_blues).collect();
+ let extra_blues: Vec<_> = actual_blues.difference(&expected_blues).collect();
+
+ warn!(
+ "Blue set mismatch for block {:?}:\nMissing blues: {:?}\nUnexpected blues: {:?}",
+ header.id(),
+ missing_blues,
+ extra_blues
+ );
+
+ return Err(anyhow::anyhow!(
+ "Blue set verification failed for block {:?}",
+ header.id()
+ ));This change:
- Provides more detailed logging about which blues are missing or unexpected
- Propagates the error instead of silently falling back
- Makes debugging easier by exposing the actual discrepancy
Committable suggestion skipped: line range outside the PR's diff.
Benchmark for 9f9cda4Click to view benchmark
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
flexidag/src/blockdag.rs(1 hunks)
🔇 Additional comments (1)
flexidag/src/blockdag.rs (1)
567-579: Method name is still misleading.
The method name suggests it performs verification and returns ghost data, but it specifically verifies blue blocks matching. This was previously flagged in a past review.
Consider renaming to better reflect its purpose:
- pub fn verify_and_ghostdata(
+ pub fn verify_blue_blocks_and_get_ghostdata(
flexidag/src/blockdag.rs
Outdated
| let ghostdata = self.ghost_dag_manager().ghostdag(&header.parents())?; | ||
| if ghostdata | ||
| .mergeset_blues | ||
| .iter() | ||
| .skip(1) | ||
| .cloned() | ||
| .collect::<HashSet<_>>() | ||
| == blue_blocks.iter().map(|b| b.id()).collect::<HashSet<_>>() | ||
| { | ||
| Ok(ghostdata) | ||
| } else { | ||
| bail!("blue blocks are not correct for remote blue block: {:?}, local blue blocks: {:?}, header: {:?}", blue_blocks, ghostdata.mergeset_blues, header); | ||
| } |
There was a problem hiding this comment.
Fix potential panic and improve performance.
The current implementation has several issues:
skip(1)could panic ifmergeset_bluesis empty- Creating two HashSets is inefficient
- Error message could be more structured
Consider this improved implementation:
- let ghostdata = self.ghost_dag_manager().ghostdag(&header.parents())?;
- if ghostdata
- .mergeset_blues
- .iter()
- .skip(1)
- .cloned()
- .collect::<HashSet<_>>()
- == blue_blocks.iter().map(|b| b.id()).collect::<HashSet<_>>()
- {
- Ok(ghostdata)
- } else {
- bail!("blue blocks are not correct for remote blue block: {:?}, local blue blocks: {:?}, header: {:?}", blue_blocks, ghostdata.mergeset_blues, header);
- }
+ let ghostdata = self.ghost_dag_manager().ghostdag(&header.parents())?;
+ let blues = &ghostdata.mergeset_blues;
+
+ // Skip first blue block only if there are any blue blocks
+ let local_blues: HashSet<_> = blues.iter().skip(!blues.is_empty() as usize).cloned().collect();
+ let remote_blues: HashSet<_> = blue_blocks.iter().map(|b| b.id()).collect();
+
+ if local_blues == remote_blues {
+ Ok(ghostdata)
+ } else {
+ bail!(
+ "Blue blocks mismatch. Remote blue blocks: {:?}, Local blue blocks: {:?}, Header: {:?}",
+ remote_blues,
+ local_blues,
+ header
+ );
+ }📝 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 ghostdata = self.ghost_dag_manager().ghostdag(&header.parents())?; | |
| if ghostdata | |
| .mergeset_blues | |
| .iter() | |
| .skip(1) | |
| .cloned() | |
| .collect::<HashSet<_>>() | |
| == blue_blocks.iter().map(|b| b.id()).collect::<HashSet<_>>() | |
| { | |
| Ok(ghostdata) | |
| } else { | |
| bail!("blue blocks are not correct for remote blue block: {:?}, local blue blocks: {:?}, header: {:?}", blue_blocks, ghostdata.mergeset_blues, header); | |
| } | |
| let ghostdata = self.ghost_dag_manager().ghostdag(&header.parents())?; | |
| let blues = &ghostdata.mergeset_blues; | |
| // Skip first blue block only if there are any blue blocks | |
| let local_blues: HashSet<_> = blues.iter().skip(!blues.is_empty() as usize).cloned().collect(); | |
| let remote_blues: HashSet<_> = blue_blocks.iter().map(|b| b.id()).collect(); | |
| if local_blues == remote_blues { | |
| Ok(ghostdata) | |
| } else { | |
| bail!( | |
| "Blue blocks mismatch. Remote blue blocks: {:?}, Local blue blocks: {:?}, Header: {:?}", | |
| remote_blues, | |
| local_blues, | |
| header | |
| ); | |
| } |
Benchmark for 0d17403Click to view benchmark
|
Benchmark for 5c1818cClick to view benchmark
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
flexidag/src/ghostdag/protocol.rs (2)
223-224: Document the skip(1) behaviorThe comparison correctly skips the first element (selected parent) when comparing blue sets, but this behavior should be documented to explain why it's necessary.
Add a comment explaining the skip:
if new_block_data .mergeset_blues .iter() + // Skip the selected parent as it's always blue in GHOSTDAG protocol .skip(1) .cloned() .collect::<HashSet<_>>() != remote_blue_set
225-235: Enhance error messages and loggingThe current implementation has good error recovery flow but could benefit from more descriptive messages:
- The warning and error messages are too similar
- The messages don't clearly indicate the difference between initial verification and fallback verification
Consider this improvement:
- warn!("The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), new_block_data.mergeset_blues); + warn!( + "Initial blue set verification failed for block {:?}. Expected blues: {:?}, Computed blues: {:?}. Attempting recomputation...", + header.id(), + remote_blue_set, + new_block_data.mergeset_blues.iter().skip(1).cloned().collect::<Vec<_>>() + ); let ghostdata = self.ghostdag(&header.parents_hash())?; if ghostdata .mergeset_blues .iter() .skip(1) .cloned() .collect::<HashSet<_>>() != remote_blue_set { - bail!("The ghost data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), ghostdata.mergeset_blues); + bail!( + "Blue set verification failed after recomputation for block {:?}. Expected blues: {:?}, Recomputed blues: {:?}", + header.id(), + remote_blue_set, + ghostdata.mergeset_blues.iter().skip(1).cloned().collect::<Vec<_>>() + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
flexidag/src/ghostdag/protocol.rs(1 hunks)
🔇 Additional comments (2)
flexidag/src/ghostdag/protocol.rs (2)
213-216: LGTM: Clean and idiomatic HashSet creation
The code efficiently creates a HashSet of block IDs from the blue blocks, which is perfect for the subsequent set comparison operations.
213-235: Consider addressing the method naming consistency
As noted in a previous review, the method name verify_and_ghostdata should be consistent with its public usage pattern. Since this PR modifies the method's implementation, it would be a good opportunity to address the naming convention.
Let's verify the method's usage pattern:
Consider removing the underscore prefix if the method is part of the public API.
Benchmark for 4cdd21eClick to view benchmark
|
Benchmark for 97a16cbClick to view benchmark
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
flexidag/src/ghostdag/protocol.rs (1)
Line range hint
1-1: Consider future improvements to the codebaseSeveral areas could benefit from future improvements:
- The TODO comment in
blue_anticone_sizemethod suggests a needed refactor- Error handling could be more consistent across methods
- Complex logic (especially in
check_blue_candidate) could benefit from additional documentationConsider creating separate issues to track these improvements for future iterations.
flexidag/tests/tests.rs (2)
739-770: Clean up commented code and unused variables to improve readabilityThe function contains multiple commented-out lines and variables that are no longer used. This can clutter the code and make maintenance more difficult.
Consider removing unnecessary comments and variables to streamline the function:
- let start = Instant::now(); - // dag.commit(header.to_owned(), origin)?; - let duration = start.elapsed(); - println!( - "commit header: {:?}, number: {:?}, duration: {:?}", - header.id(), - header.number(), - duration - ); - // let ghostdata = dag.ghostdata(&[header.id()])?; - // let ghostdata = dag.ghostdata_by_hash(header.id())?.unwrap(); - // println!( - // "add a header: {:?}, selected_parent: {:?}, blue set: {:?}, red set: {:?}, blue anticone size: {:?}", - // header, ghostdata.selected_parent, ghostdata.mergeset_blues, ghostdata.mergeset_reds, ghostdata.blues_anticone_sizes - // ); + dag.commit(header.to_owned(), origin)?; + Ok(header)
1125-1155: Optimize the block creation loop for performanceThe loop that creates 500 blocks sequentially may lead to long test execution times:
for i in 0..500 { // Block creation and DAG commit logic }Consider optimizing the loop by:
- Reducing the number of iterations if possible.
- Using asynchronous operations to parallelize block creation and commits.
- Batching commits to minimize the overhead of individual operations.
This can improve test performance without sacrificing coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
flexidag/src/ghostdag/protocol.rs(2 hunks)flexidag/tests/tests.rs(5 hunks)
🔇 Additional comments (3)
flexidag/tests/tests.rs (3)
Line range hint 786-798: Ensure consistent difficulty settings in add_and_print_with_pruning_point
In the add_and_print_with_pruning_point function, a default difficulty is set using .with_difficulty(U256::from(10)). Verify that this difficulty level is appropriate and consistent with the rest of the test scenarios.
Please confirm that a difficulty of 10 meets the requirements for all tests that utilize this function.
1171-1173: Handle potential errors during ghost data verification
When calling dag.verify_and_ghostdata, ensure that any errors are properly handled:
let verified_ghost = dag.verify_and_ghostdata(/* parameters */)?;Add error handling to manage verification failures gracefully, which can help identify issues during testing phases.
1245-1246: Verify consistency between ghost data before and after verification
The assertion checks if the mergeset blues before and after verification are equal:
assert_eq!(ghostdata.mergeset_blues, verified_ghostdata.mergeset_blues);Confirm that this equality is expected. If discrepancies are possible due to the verification process, adjust the assertion to accommodate acceptable differences or investigate potential issues causing inconsistencies.
flexidag/src/ghostdag/protocol.rs
Outdated
| // let selected_parent = self.find_selected_parent(header.parents_hash().into_iter())?; | ||
| let selected_parent = header.parent_hash(); |
There was a problem hiding this comment.
Critical: Incorrect parent selection logic
The change bypasses the GHOSTDAG parent selection algorithm by directly using parent_hash instead of finding the parent with the highest blue work. This could lead to consensus issues as it violates the GHOSTDAG protocol specification.
Revert this change and use the original parent selection logic:
- // let selected_parent = self.find_selected_parent(header.parents_hash().into_iter())?;
- let selected_parent = header.parent_hash();
+ let selected_parent = self.find_selected_parent(header.parents_hash().into_iter())?;📝 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 selected_parent = self.find_selected_parent(header.parents_hash().into_iter())?; | |
| let selected_parent = header.parent_hash(); | |
| let selected_parent = self.find_selected_parent(header.parents_hash().into_iter())?; |
flexidag/tests/tests.rs
Outdated
| fn add_and_print_with_difficulty( | ||
| number: BlockNumber, | ||
| parent: Hash, | ||
| parents: Vec<Hash>, | ||
| origin: Hash, | ||
| difficulty: U256, | ||
| dag: &mut BlockDAG, | ||
| ) -> anyhow::Result<BlockHeader> { | ||
| let header_builder = BlockHeaderBuilder::random(); | ||
| let header = header_builder | ||
| .with_parent_hash(parent) | ||
| .with_parents_hash(parents) | ||
| .with_number(number) | ||
| .with_difficulty(difficulty) | ||
| .build(); | ||
| let start = Instant::now(); | ||
| // dag.commit(header.to_owned(), origin)?; | ||
| let duration = start.elapsed(); | ||
| println!( | ||
| "commit header: {:?}, number: {:?}, duration: {:?}", | ||
| header.id(), | ||
| header.number(), | ||
| duration | ||
| ); | ||
| // let ghostdata = dag.ghostdata(&[header.id()])?; | ||
| // let ghostdata = dag.ghostdata_by_hash(header.id())?.unwrap(); | ||
| // println!( | ||
| // "add a header: {:?}, selected_parent: {:?}, blue set: {:?}, red set: {:?}, blue anticone size: {:?}", | ||
| // header, ghostdata.selected_parent, ghostdata.mergeset_blues, ghostdata.mergeset_reds, ghostdata.blues_anticone_sizes | ||
| // ); | ||
| Ok(header) | ||
| } |
There was a problem hiding this comment.
Uncomment the block commit to ensure blocks are added to the DAG
In the add_and_print_with_difficulty function, the line responsible for committing the block to the DAG is commented out:
// dag.commit(header.to_owned(), origin)?;This means the block header is created but not actually committed, which could lead to inconsistent DAG state or missing blocks during tests.
Apply this diff to fix the issue:
-// dag.commit(header.to_owned(), origin)?;
+dag.commit(header.to_owned(), origin)?;
Benchmark for 083e3d4Click to view benchmark
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
flexidag/src/ghostdag/protocol.rs(4 hunks)
🔇 Additional comments (2)
flexidag/src/ghostdag/protocol.rs (2)
393-396: LGTM: Improved logging messages
The added logging messages provide better context about why blue candidate checking failed, which will help with debugging.
Also applies to: 445-445
213-235: 🛠️ Refactor suggestion
Improve blue set verification logic and error handling
The current implementation has several issues:
- Silently falling back to
ghostdagwhen blue sets don't match could mask underlying problems - The warning message could be more descriptive about the blue set mismatch
- The second verification step adds unnecessary complexity
Consider this improved implementation:
let remote_blue_set = blue_blocks
.iter()
.map(|header| header.id())
.collect::<HashSet<_>>();
- if new_block_data
+ let calculated_blue_set = new_block_data
.mergeset_blues
.iter()
.skip(1)
.cloned()
- .collect::<HashSet<_>>()
- != remote_blue_set
- {
- warn!("The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), new_block_data.mergeset_blues);
- let ghostdata = self.ghostdag(&header.parents_hash())?;
- if ghostdata
- .mergeset_blues
- .iter()
- .skip(1)
- .cloned()
- .collect::<HashSet<_>>()
- != remote_blue_set
- {
- bail!("The ghost data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), ghostdata.mergeset_blues);
- }
- }
+ .collect::<HashSet<_>>();
+
+ if calculated_blue_set != remote_blue_set {
+ let missing_blues: Vec<_> = remote_blue_set.difference(&calculated_blue_set).collect();
+ let extra_blues: Vec<_> = calculated_blue_set.difference(&remote_blue_set).collect();
+
+ bail!(
+ "Blue set mismatch for block {:?}:\nExpected blues: {:?}\nCalculated blues: {:?}\nMissing blues: {:?}\nExtra blues: {:?}",
+ header.id(),
+ remote_blue_set,
+ calculated_blue_set,
+ missing_blues,
+ extra_blues
+ );
+ }Likely invalid or redundant comment.
flexidag/src/ghostdag/protocol.rs
Outdated
| @@ -181,7 +181,7 @@ impl< | |||
| !parents.is_empty(), | |||
| "genesis must be added via a call to init" | |||
| ); | |||
| let selected_parent = self.find_selected_parent(header.parents_hash().into_iter())?; | |||
| let selected_parent = self.find_selected_parent(header.parents_hash().into_iter())?; | |||
There was a problem hiding this comment.
Critical: Incorrect parent selection logic
The change bypasses the GHOSTDAG parent selection algorithm by directly using header.parents_hash() instead of finding the parent with the highest blue work. This violates the GHOSTDAG protocol specification and could lead to consensus issues.
Apply this fix:
- let selected_parent = self.find_selected_parent(header.parents_hash().into_iter())?;
+ let selected_parent = self.find_selected_parent(parents.iter().copied())?;Committable suggestion skipped: line range outside the PR's diff.
Benchmark for 257dc8fClick to view benchmark
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
flexidag/tests/tests.rs (2)
781-781: Remove commented debug logging codeThe commented ghostdata logging code should either be removed or uncommented if it serves a purpose in debugging.
Apply this diff to clean up the code:
- // let ghostdata = dag.ghostdata(&[header.id()])?; - // let ghostdata = dag.ghostdata_by_hash(header.id())?.unwrap(); - // println!( - // "add a header: {:?}, selected_parent: {:?}, blue set: {:?}, red set: {:?}, blue anticone size: {:?}", - // header, ghostdata.selected_parent, ghostdata.mergeset_blues, ghostdata.mergeset_reds, ghostdata.blues_anticone_sizes - // );Also applies to: 792-796
1107-1286: Improve code maintainability and readabilityThe function has several areas that could be improved:
- Magic numbers should be constants with descriptive names
- Complex block creation logic could be extracted into helper functions
- Repeated verification code could be refactored into a reusable function
Consider these improvements:
+const CHAIN_LENGTH: u64 = 500; +const NORMAL_DIFFICULTY: u64 = 10; +const HIGH_DIFFICULTY: u64 = 99999999; +fn create_and_verify_block( + dag: &mut BlockDAG, + block: BlockHeader, + storage: &HashMap<Hash, BlockHeader>, + genesis_parent_hash: Hash, +) -> anyhow::Result<()> { + let ghost = dag.ghostdata(&block.parents())?; + let verified_ghost = dag.verify_and_ghostdata( + &ghost + .mergeset_blues + .iter() + .skip(1) + .cloned() + .map(|x| storage.get(&x).unwrap().clone()) + .collect::<Vec<_>>(), + &block, + )?; + dag.commit_trusted_block( + block.clone(), + genesis_parent_hash, + Arc::new(verified_ghost), + ) +}This would simplify the main loop:
- for i in 0..500 { + for i in 0..CHAIN_LENGTH { let block2 = - add_and_print_with_difficulty(1 + i, last.id(), vec![last.id()], U256::from(10))?; + add_and_print_with_difficulty(1 + i, last.id(), vec![last.id()], U256::from(NORMAL_DIFFICULTY))?; last = block2.clone(); storage.insert(block2.id(), block2.clone()); - let ghost = dag.ghostdata(&block2.parents())?; - let verified_ghost = dag.verify_and_ghostdata( - &ghost - .mergeset_blues - .iter() - .skip(1) - .cloned() - .map(|x| storage.get(&x).unwrap().clone()) - .collect::<Vec<_>>(), - &block2, - )?; - dag.commit_trusted_block( - block2.clone(), - genesis.parent_hash(), - Arc::new(verified_ghost), - )?; + create_and_verify_block(&mut dag, block2, &storage, genesis.parent_hash())?; bottom.push(block2); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
flexidag/src/blockdag.rs(2 hunks)flexidag/src/ghostdag/protocol.rs(3 hunks)flexidag/tests/tests.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flexidag/src/blockdag.rs
🔇 Additional comments (2)
flexidag/src/ghostdag/protocol.rs (1)
387-390: LGTM: Improved logging for failure scenarios
The added logging statements provide clear and helpful context for debugging different failure scenarios:
- Blue anticone exceeding k
- K-cluster violation detection
- Mergeset blues size validation
Also applies to: 397-400, 449-452
flexidag/tests/tests.rs (1)
22-23: LGTM!
The added imports are necessary for handling block difficulty in the new test functions.
| let remote_blue_set = blue_blocks | ||
| .iter() | ||
| .map(|header| header.id()) | ||
| .collect::<HashSet<_>>(); | ||
| if new_block_data | ||
| .mergeset_blues | ||
| .iter() | ||
| .skip(1) | ||
| .cloned() | ||
| .collect::<HashSet<_>>() | ||
| != blue_blocks | ||
| != remote_blue_set | ||
| { | ||
| warn!("The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), new_block_data.mergeset_blues); | ||
| let ghostdata = self.ghostdag(&header.parents_hash())?; | ||
| if ghostdata | ||
| .mergeset_blues | ||
| .iter() | ||
| .map(|header| header.id()) | ||
| .skip(1) | ||
| .cloned() | ||
| .collect::<HashSet<_>>() | ||
| { | ||
| if header.number() < 10000000 { | ||
| // no bail before 10000000 | ||
| warn!("The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), new_block_data.mergeset_blues); | ||
| } else { | ||
| bail!("The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), new_block_data.mergeset_blues); | ||
| != remote_blue_set | ||
| { | ||
| bail!("The ghost data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), ghostdata.mergeset_blues); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve blue set verification logic and error handling
The current implementation has several issues:
- Silently falling back to
ghostdagwhen blue sets don't match could mask underlying problems - The warning message could be more descriptive about the blue set mismatch
- The second verification step could mask underlying issues
Consider this improved implementation:
let remote_blue_set = blue_blocks
.iter()
.map(|header| header.id())
.collect::<HashSet<_>>();
- if new_block_data
+ let calculated_blue_set = new_block_data
.mergeset_blues
.iter()
.skip(1)
.cloned()
- .collect::<HashSet<_>>()
- != remote_blue_set
- {
- warn!("The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), new_block_data.mergeset_blues);
- let ghostdata = self.ghostdag(&header.parents_hash())?;
- if ghostdata
- .mergeset_blues
- .iter()
- .skip(1)
- .cloned()
- .collect::<HashSet<_>>()
- != remote_blue_set
- {
- bail!("The ghost data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), ghostdata.mergeset_blues);
- }
- }
+ .collect::<HashSet<_>>();
+
+ if calculated_blue_set != remote_blue_set {
+ let missing_blues: Vec<_> = remote_blue_set.difference(&calculated_blue_set).collect();
+ let extra_blues: Vec<_> = calculated_blue_set.difference(&remote_blue_set).collect();
+
+ bail!(
+ "Blue set mismatch for block {:?}:\nExpected blues: {:?}\nCalculated blues: {:?}\nMissing blues: {:?}\nExtra blues: {:?}",
+ header.id(),
+ remote_blue_set,
+ calculated_blue_set,
+ missing_blues,
+ extra_blues
+ );
+ }This change:
- Provides more detailed error messages showing the exact differences
- Removes the redundant second verification step
- Fails fast when a mismatch is detected
📝 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 remote_blue_set = blue_blocks | |
| .iter() | |
| .map(|header| header.id()) | |
| .collect::<HashSet<_>>(); | |
| if new_block_data | |
| .mergeset_blues | |
| .iter() | |
| .skip(1) | |
| .cloned() | |
| .collect::<HashSet<_>>() | |
| != blue_blocks | |
| != remote_blue_set | |
| { | |
| warn!("The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), new_block_data.mergeset_blues); | |
| let ghostdata = self.ghostdag(&header.parents_hash())?; | |
| if ghostdata | |
| .mergeset_blues | |
| .iter() | |
| .map(|header| header.id()) | |
| .skip(1) | |
| .cloned() | |
| .collect::<HashSet<_>>() | |
| { | |
| if header.number() < 10000000 { | |
| // no bail before 10000000 | |
| warn!("The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), new_block_data.mergeset_blues); | |
| } else { | |
| bail!("The data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), new_block_data.mergeset_blues); | |
| != remote_blue_set | |
| { | |
| bail!("The ghost data of blue set is not equal when executing the block: {:?}, for {:?}, checking data: {:?}", header.id(), blue_blocks.iter().map(|header| header.id()).collect::<Vec<_>>(), ghostdata.mergeset_blues); | |
| let remote_blue_set = blue_blocks | |
| .iter() | |
| .map(|header| header.id()) | |
| .collect::<HashSet<_>>(); | |
| let calculated_blue_set = new_block_data | |
| .mergeset_blues | |
| .iter() | |
| .skip(1) | |
| .cloned() | |
| .collect::<HashSet<_>>(); | |
| if calculated_blue_set != remote_blue_set { | |
| let missing_blues: Vec<_> = remote_blue_set.difference(&calculated_blue_set).collect(); | |
| let extra_blues: Vec<_> = calculated_blue_set.difference(&remote_blue_set).collect(); | |
| bail!( | |
| "Blue set mismatch for block {:?}:\nExpected blues: {:?}\nCalculated blues: {:?}\nMissing blues: {:?}\nExtra blues: {:?}", | |
| header.id(), | |
| remote_blue_set, | |
| calculated_blue_set, | |
| missing_blues, | |
| extra_blues | |
| ); | |
| } |
| #[test] | ||
| fn test_verification_blue_block_inconsistent() -> anyhow::Result<()> { | ||
| loop_to_blue()?; | ||
| anyhow::Result::Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with assertions
The test function is a simple wrapper around loop_to_blue() without any assertions or verification of the test results. Consider adding specific test cases and assertions to verify the expected behavior.
Example improvement:
#[test]
fn test_verification_blue_block_inconsistent() -> anyhow::Result<()> {
- loop_to_blue()?;
+ let (dag, last_block) = loop_to_blue()?;
+
+ // Add assertions to verify the expected state
+ let ghost_data = dag.ghostdata(&[last_block.id()])?;
+ assert!(ghost_data.blue_score > 0, "Blue score should be positive");
+ assert!(!ghost_data.mergeset_blues.is_empty(), "Blue set should not be empty");
+
anyhow::Result::Ok(())
}Committable suggestion skipped: line range outside the PR's diff.
Benchmark for 2d9ad34Click to view benchmark
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
flexidag/tests/tests.rs (2)
823-823: Remove commented out debug logging code.The function contains commented out debug logging code that should be removed if no longer needed.
Apply this diff to clean up:
let header = header_builder .with_parent_hash(parent) .with_parents_hash(parents) .with_number(number) .with_pruning_point(pruning_point) .with_difficulty(U256::from(10)) .build(); let start = Instant::now(); dag.commit(header.to_owned(), origin)?; let duration = start.elapsed(); println!( "commit header: {:?}, number: {:?}, duration: {:?}", header.id(), header.number(), duration ); - // let ghostdata = dag.ghostdata(&[header.id()])?; - // let ghostdata = dag.ghostdata_by_hash(header.id())?.unwrap(); - // println!( - // "add a header: {:?}, selected_parent: {:?}, blue set: {:?}, red set: {:?}, blue anticone size: {:?}", - // header, ghostdata.selected_parent, ghostdata.mergeset_blues, ghostdata.mergeset_reds, ghostdata.blues_anticone_sizes - // ); Ok(header)Also applies to: 834-838
1143-1328: Enhance test assertions for better verification coverage.While the test successfully verifies ghost data consistency, it could benefit from additional assertions to:
- Verify the blue block inconsistency conditions
- Check intermediate states during the block creation loop
- Validate the expected behavior when inconsistency is detected
Consider adding assertions like:
// Verify blue block properties assert!(ghost.blue_score > 0, "Blue score should be positive"); assert!(!ghost.mergeset_blues.is_empty(), "Blue set should not be empty"); // Verify inconsistency detection assert!(verified_ghost.mergeset_blues.len() < ghost.mergeset_blues.len(), "Verification should reduce blue set on inconsistency");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
flexidag/src/blockdag.rs(2 hunks)flexidag/tests/tests.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flexidag/src/blockdag.rs
🔇 Additional comments (2)
flexidag/tests/tests.rs (2)
22-23: LGTM!
The new imports are correctly placed and necessary for the added functionality.
446-456: LGTM! Test coverage improved.
The changes enhance the test coverage by:
- Simplifying block addition with empty iterators
- Adding new assertions to verify ancestor relationships for
selected_parent_uncle1,uncle1, anduncle2
The test logic remains robust and comprehensive.
Also applies to: 470-470, 475-494, 500-500, 508-519
| fn add_and_print_with_difficulty( | ||
| number: BlockNumber, | ||
| parent: Hash, | ||
| parents: Vec<Hash>, | ||
| difficulty: U256, | ||
| ) -> anyhow::Result<BlockHeader> { | ||
| let header_builder = BlockHeaderBuilder::random(); | ||
| let header = header_builder | ||
| .with_parent_hash(parent) | ||
| .with_parents_hash(parents) | ||
| .with_number(number) | ||
| .with_difficulty(difficulty) | ||
| .build(); | ||
| let start = Instant::now(); | ||
| let duration = start.elapsed(); | ||
| println!( | ||
| "commit header: {:?}, number: {:?}, duration: {:?}", | ||
| header.id(), | ||
| header.number(), | ||
| duration | ||
| ); | ||
|
|
||
| Ok(header) | ||
| } |
There was a problem hiding this comment.
Fix timing measurement or remove unused timing code.
The function includes timing measurement code that doesn't measure any meaningful operation:
let start = Instant::now();
let duration = start.elapsed();Since no significant operation occurs between start and elapsed(), this timing is not useful.
Apply this diff to fix:
fn add_and_print_with_difficulty(
number: BlockNumber,
parent: Hash,
parents: Vec<Hash>,
difficulty: U256,
) -> anyhow::Result<BlockHeader> {
let header_builder = BlockHeaderBuilder::random();
let header = header_builder
.with_parent_hash(parent)
.with_parents_hash(parents)
.with_number(number)
.with_difficulty(difficulty)
.build();
- let start = Instant::now();
- let duration = start.elapsed();
println!(
"commit header: {:?}, number: {:?}, duration: {:?}",
header.id(),
header.number(),
- duration
+ Duration::from_secs(0)
);
Ok(header)
}
Benchmark for 20c726aClick to view benchmark
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
flexidag/tests/tests.rs (2)
834-835: Clean up commented debug prints.Instead of leaving debug prints commented out, consider either:
- Removing them completely if they're no longer needed
- Uncommenting them if they provide valuable debugging information
- Moving them behind a debug flag if they're occasionally useful
This helps maintain cleaner and more maintainable code.
Also applies to: 837-838
1149-1328: Consider refactoring for better maintainability.The function could benefit from several improvements:
- Extract repeated block creation and verification logic into helper methods
- Encapsulate the storage HashMap in a dedicated test helper struct
- Break down the complex DAG construction into smaller, focused methods
- Add comments explaining the purpose of each section
This would make the test more maintainable and easier to understand.
Example refactoring:
struct TestDagBuilder { storage: HashMap<Hash, BlockHeader>, dag: BlockDAG, } impl TestDagBuilder { fn new(k: u64) -> Self { // Implementation } fn create_and_verify_block(&mut self, number: u64, parent: Hash, parents: Vec<Hash>, difficulty: U256) -> anyhow::Result<BlockHeader> { // Implementation } fn build_bottom_chain(&mut self, count: u64, start_block: &BlockHeader) -> anyhow::Result<Vec<BlockHeader>> { // Implementation } fn build_top_chain(&mut self, bottom_blocks: Vec<BlockHeader>) -> anyhow::Result<BlockHeader> { // Implementation } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
flexidag/tests/tests.rs(7 hunks)
🔇 Additional comments (4)
flexidag/tests/tests.rs (4)
22-23: LGTM!
The new imports are necessary for handling consensus headers and difficulty values in the new test functions.
784-807: Remove ineffective timing measurement code.
The timing measurement is ineffective as it measures an empty duration between start and elapsed(). No significant operation occurs in this interval.
Apply this diff to fix:
- let start = Instant::now();
- let duration = start.elapsed();
println!(
"commit header: {:?}, number: {:?}, duration: {:?}",
header.id(),
header.number(),
- duration
+ Duration::from_secs(0)
);823-823: LGTM!
Adding a default difficulty value is consistent with the new difficulty-aware testing approach.
1143-1147: Enhance test coverage with assertions and documentation.
The test function lacks explicit assertions to verify the expected behavior. Consider:
- Adding assertions to verify specific behaviors
- Documenting the test's purpose and expected outcomes
- Breaking down the complex DAG construction into smaller, well-documented steps
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
Bug Fixes
syncdirectory, ensuring a cleaner environment.Documentation
Chores