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: find common header between the header chain and the current body chain #3033

Merged
merged 2 commits into from
Sep 9, 2019
Merged

fix: find common header between the header chain and the current body chain #3033

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c961898
fix: find common header between the header chain and the current body…
garyyu Sep 9, 2019
54fc220
add some code comments
garyyu Sep 9, 2019
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
45 changes: 26 additions & 19 deletions chain/src/chain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,11 +791,11 @@ impl Chain {
let mut oldest_height = 0;
let mut oldest_hash = ZERO_HASH;

let mut current = self.get_block_header(&header_head.last_block_h);
garyyu marked this conversation as resolved.
Show resolved Hide resolved
let mut current = self.get_block_header(&body_head.last_block_h);
Copy link
Member

Choose a reason for hiding this comment

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

Lets rename this usage of current to make it clearer that we want to check for headers in our db based on body_head.

This is a 2 step process.

  • start with head (head of the full block chain)
    • if we do not have a full block in the db for this head then its an error
    • if we do not have the header in the db for this head then its an error
  • traverse back through the full block chain from head until we find a header that "is on current chain"
    • this is our "fork point" between existing header chain and full block chain
  • now we want to traverse back through our header chain from header_head back to this fork point
    • these are the blocks that we need to request (we have the header but not the full block)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put some of yours comments to the source code comments, and renamed the 2nd current.
pls let me whether it's better now.

if current.is_err() {
error!(
"{}: header_head not found in chain db: {} at {}",
caller, header_head.last_block_h, header_head.height,
"{}: body_head not found in chain db: {} at {}",
caller, body_head.last_block_h, body_head.height,
);
return Ok(false);
}
Expand All @@ -807,34 +807,41 @@ impl Chain {
while let Ok(header) = current {
// break out of the while loop when we find a header common
// between the header chain and the current body chain
if header.height <= body_head.height {
if let Ok(_) = self.is_on_current_chain(&header) {
break;
}
if let Ok(_) = self.is_on_current_chain(&header) {
oldest_height = header.height;
oldest_hash = header.hash();
break;
}

oldest_height = header.height;
oldest_hash = header.hash();
if let Some(hs) = hashes {
hs.push(oldest_hash);
}
current = self.get_previous_header(&header);
}

// Go through the header chain reversely to get all those headers after oldest_height.
if let Some(hs) = hashes {
let mut current = self.get_block_header(&header_head.last_block_h);
Copy link
Member

@antiochp antiochp Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I see we are redefining current in this PR here, but I don't think we need to do this. Earlier this is body_head and now this is redefined based on header_head.

Edit: We do need to do this, but I think we should make it explicit in terms of naming - current is too ambiguous, particularly when we reuse it later on for a different purpose.

I think it should consistently be based on a header from the db starting at header_head and working back until we find one that also refers to a full block in our db.

Ok I think I get it now. Let me think through what you are doing here a bit more.

while let Ok(header) = current {
if header.height <= oldest_height {
break;
}
hs.push(header.hash());
current = self.get_previous_header(&header);
}
}

if oldest_height < header_head.height.saturating_sub(horizon) {
if oldest_height > 0 {
if oldest_hash != ZERO_HASH {
// this is the normal case. for example:
// body head height is 1 (and not a fork), oldest_height will be 2
// body head height is 0 (a typical fresh node), oldest_height will be 1
// body head height is 10,001 (but at a fork), oldest_height will be 10,001
// body head height is 10,005 (but at a fork with depth 5), oldest_height will be 10,001
// body head height is 1 (and not a fork), oldest_height will be 1
// body head height is 0 (a typical fresh node), oldest_height will be 0
// body head height is 10,001 (but at a fork with depth 1), oldest_height will be 10,000
// body head height is 10,005 (but at a fork with depth 5), oldest_height will be 10,000
debug!(
"{}: need a state sync for txhashset. oldest block which is not on local chain: {} at {}",
caller, oldest_hash, oldest_height,
);
} else {
// this is the abnormal case, when is_on_current_chain() already return Err, and even for genesis block.
error!("{}: corrupted storage? oldest_height is 0 when check_txhashset_needed. state sync is needed", caller);
// this is the abnormal case, when is_on_current_chain() always return Err, and even for genesis block.
error!("{}: corrupted storage? state sync is needed", caller);
}
Ok(true)
} else {
Expand Down