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.

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

Conversation

@garyyu
Copy link
Member

commented Sep 9, 2019

Fix of #3030

The current is_on_current_chain() function has an implicit meaning of current header chain, don't remember when we did this change, anyway it impacts the logic here to find the header common between the header chain and the current body chain.

… chain
@garyyu garyyu requested a review from antiochp Sep 9, 2019
@garyyu garyyu added bug fixed labels Sep 9, 2019
@antiochp

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

The current is_on_current_chain() function has an implicit meaning of current header chain, don't remember when we did this change, anyway it impacts the logic here to find the header common between the header chain and the current body chain.

I don't fully understand what you mean here by "implicit meaning of current header chain".
This is the only way of knowing if we are on the current chain or not based on our header MMR.

We check for is_on_current_chain with the following logic -

  • put header MMR in correct state via rewind_and_reapply_header_fork
    • header MMR data file now contains sequential headers up to and including current height
    • this may have diverged from the files on disk if this is a fork (we only persist the most work chain)
  • for the header in question
    • take its height and lookup the header in our MMR at this height
    • if its the same header its on the "current" chain
    • otherwise it is on a different fork

i.e. If block abc has height 1,000 and on the current chain the header at height 1,000 is abc then we know abc is on the current chain.
Which feels a little circular but if we find block def at height 1,000 then we know abc cannot also be at height 1,000 and must be on a different fork.

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);

This comment has been minimized.

Copy link
@antiochp

antiochp Sep 9, 2019

Member

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.

@@ -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);
let mut current = self.get_block_header(&body_head.last_block_h);

This comment has been minimized.

Copy link
@antiochp

antiochp Sep 9, 2019

Member

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)

This comment has been minimized.

Copy link
@garyyu

garyyu Sep 9, 2019

Author Member

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

Copy link
Member

left a comment

Yes - thanks for finding and fixing this.
This PR fixes the issue and better explains how we are identifying which blocks to request.

@garyyu garyyu merged commit fa0fed9 into mimblewimble:master Sep 9, 2019
10 checks passed
10 checks passed
mimblewimble.grin Build #20190909.2 succeeded
Details
mimblewimble.grin (linux api/util/store) linux api/util/store succeeded
Details
mimblewimble.grin (linux chain/core/keychain) linux chain/core/keychain succeeded
Details
mimblewimble.grin (linux pool/p2p/src) linux pool/p2p/src succeeded
Details
mimblewimble.grin (linux release) linux release succeeded
Details
mimblewimble.grin (linux servers) linux servers succeeded
Details
mimblewimble.grin (macos release) macos release succeeded
Details
mimblewimble.grin (macos test) macos test succeeded
Details
mimblewimble.grin (windows release) windows release succeeded
Details
mimblewimble.grin (windows test) windows test succeeded
Details
@garyyu garyyu deleted the garyyu:fix-3030 branch Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.