Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug: no store the sync dag block #4259

Open
wants to merge 16 commits into
base: dag-master
Choose a base branch
from
Open

Fix bug: no store the sync dag block #4259

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
a6403bc
no store the sync dag block
jackzhhuang Nov 4, 2024
c173559
add rm sync in kube file
jackzhhuang Nov 4, 2024
48c9bac
use ghostdata to verify the dag block
jackzhhuang Nov 6, 2024
c8b259a
use ghodata if the verify is not correct
jackzhhuang Nov 6, 2024
1a6c851
use ghostdata instead of verify
jackzhhuang Nov 6, 2024
264a5d6
add verification code for discovering the blue blocks error
jackzhhuang Nov 6, 2024
3b2a145
verify and if it fails it use ghostdata
jackzhhuang Nov 7, 2024
197bc4f
bail out if the ghost data checking is wrong
jackzhhuang Nov 7, 2024
adfcb23
remove to print red blocks avoiding lots log data
jackzhhuang Nov 8, 2024
e06797d
test dag verification
jackzhhuang Nov 11, 2024
86ee296
add log to trace
jackzhhuang Nov 11, 2024
8250159
add reachability data with parents
jackzhhuang Nov 11, 2024
573bc89
fix fmt
jackzhhuang Nov 13, 2024
e3dca70
use unordered_mergeset_without_selected_parent when storing into the …
jackzhhuang Nov 13, 2024
248b7fb
rm commented codes
jackzhhuang Nov 14, 2024
e4eff6c
add uncle test in reachability
jackzhhuang Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions flexidag/src/ghostdag/protocol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,22 +210,29 @@ impl<
}
}

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);
Comment on lines +213 to +235
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve blue set verification logic and error handling

The current implementation has several issues:

  1. Silently falling back to ghostdag when blue sets don't match could mask underlying problems
  2. The warning message could be more descriptive about the blue set mismatch
  3. 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:

  1. Provides more detailed error messages showing the exact differences
  2. Removes the redundant second verification step
  3. 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.

Suggested change
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
);
}

}
}

Expand Down
5 changes: 5 additions & 0 deletions flexidag/src/prune/pruning_point_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@ impl<T: ReachabilityStoreReader + Clone> PruningPointManagerT<T> {
min_required_blue_score_for_next_pruning_point
);

debug!("previous_pruning_point: {:?}, previous_ghostdata: {:?}, next_ghostdata: {:?}, pruning_depth: {:?}, pruning_finality: {:?}",
previous_pruning_point, previous_ghostdata, next_ghostdata,
pruning_depth, pruning_finality,
);

let mut latest_pruning_ghost_data = previous_ghostdata.to_compact();
if min_required_blue_score_for_next_pruning_point + pruning_depth
<= next_ghostdata.blue_score
Expand Down
2 changes: 1 addition & 1 deletion kube/manifest/starcoin-vega.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ spec:
- -c
args:
-
rm -rf /sc-data/vega/starcoin.ipc /sc-data/vega/starcoindb/db/starcoindb/LOCK /sc-data/vega/genesis_config.json;
rm -rf /sc-data/vega/sync /sc-data/vega/starcoin.ipc /sc-data/vega/starcoindb/db/starcoindb/LOCK /sc-data/vega/genesis_config.json;
id=$(echo -e $POD_NAME|awk -F'-' '{print $2}') && IFS='; ' read -r -a node_keys <<< $NODE_KEYS &&
node_key=${node_keys[$id]};
if [ ! -z $node_key ]; then
Expand Down
2 changes: 1 addition & 1 deletion sync/src/tasks/block_sync_task.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,7 @@ where
if block_header.number() % ASYNC_BLOCK_COUNT == 0
|| block_header.number() >= self.target.target_id.number()
{
self.sync_dag_store.delete_all_dag_sync_block()?;
self.find_absent_ancestor(vec![block_header.clone()])
.await?;

Expand All @@ -474,7 +475,6 @@ where
block: block.clone(),
children: vec![],
})?;
self.sync_dag_store.save_block(block)?;
anyhow::Ok(ParallelSign::NeedMoreBlocks)
}
};
Expand Down
Loading