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 6 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
15 changes: 13 additions & 2 deletions flexidag/src/blockdag.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,8 +564,19 @@ impl BlockDAG {
blue_blocks: &[BlockHeader],
header: &BlockHeader,
) -> Result<GhostdagData, anyhow::Error> {
self.ghost_dag_manager()
.verify_and_ghostdata(blue_blocks, header)
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);
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential panic and improve performance.

The current implementation has several issues:

  1. skip(1) could panic if mergeset_blues is empty
  2. Creating two HashSets is inefficient
  3. 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.

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

}
pub fn check_upgrade(&self, main: &BlockHeader, genesis_id: HashValue) -> anyhow::Result<()> {
// set the state with key 0
Expand Down
12 changes: 4 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 @@ -2,7 +2,7 @@ use super::util::Refs;
use crate::consensusdb::schemadb::{GhostdagStoreReader, HeaderStoreReader, RelationsStoreReader};
use crate::reachability::reachability_service::ReachabilityService;
use crate::types::{ghostdata::GhostdagData, ordering::*};
use anyhow::{bail, ensure, Context, Result};
use anyhow::{ensure, Context, Result};
use parking_lot::RwLock;
use starcoin_crypto::HashValue as Hash;
use starcoin_logger::prelude::*;
Expand Down Expand Up @@ -171,7 +171,7 @@ impl<
Ok(new_block_data)
}

pub(crate) fn verify_and_ghostdata(
pub(crate) fn _verify_and_ghostdata(
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Remove underscore prefix from verify_and_ghostdata method

The underscore prefix is inconsistent with the method's usage pattern. The evidence shows:

  1. The method is part of a public API used by other crates (chain/src/chain.rs and chain/src/verifier/mod.rs)
  2. It's called through BlockDag::verify_and_ghostdata which is a public interface
  3. 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:

  1. Making it truly private by removing pub(crate) if it's only used within this module
  2. 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

&self,
blue_blocks: &[BlockHeader],
header: &BlockHeader,
Expand Down Expand Up @@ -221,12 +221,8 @@ impl<
.map(|header| header.id())
.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);
}
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());
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error handling and logging.

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 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:

  1. Provides more detailed logging about which blues are missing or unexpected
  2. Propagates the error instead of silently falling back
  3. Makes debugging easier by exposing the actual discrepancy

Committable suggestion skipped: line range outside the PR's diff.

}

let blue_score = self
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