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 invalid root #3005

Merged
merged 2 commits into from
Aug 29, 2019
Merged

Fix invalid root #3005

merged 2 commits into from
Aug 29, 2019

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Aug 28, 2019

This PR fixes a long standing bug that surfaces with an error like -

20190828 04:04:21.569 INFO grin_chain::chain - Rejected block 00001e7bcb37 at 321399: Error { inner:

Invalid Root }

This happens when we process two competing forks simultaneously. But only if we process header first or compact block header, and have not yet processed the first full block.

  • Process header A, updating header_head but not yet updating head (full block chain head)
  • Process header A' (fork at same height as A)
    • We check if we need to rewind the header MMR by incorrectly comparing A's prev_header to head (should be header_head which was updated in previous step).
    • We have not yet processed full block A so we incorrectly determine the header MMR does not need rewinding.
    • We append header A' to the header MMR that already contains A and hence build an invalid MMR.

We basically have a race condition here between headers A and A' and we do not rewind the header MMR correctly for the latest one processed.

Note - This problem is hidden as soon as we process the full block A as head and header_head are identical at that point. So using head is safe here and the rewind works as expected.


This is a simple fix - when creating header extensions for A and A' we need to treat header_head as the rightmost item in the MMR as this is consistent with the current MMR state.


Added test coverage to exercise this scenario.
Also took the opportunity to clean up some of the existing test setup code.

@antiochp
Copy link
Member Author

antiochp commented Aug 28, 2019

@lehnberg We may want to consider a 2.0.1 bugfix release for this one. I believe we have seen multiple reports of this in the wild.

Actually - I think we should just nudge people to use master is they need a fix for this (assuming we merge this to master).

// We want to use the current head of header chain here.
// Caller is responsible for rewinding the header MMR back
// to a previous header as necessary when processing a fork.
let head = batch.header_head()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 the puzzle of header head and body head :-) thanks very much for finding this long standing bug.

I'm a little bit confused on the comment:

Caller is responsible for rewinding the header MMR back to a previous header as necessary when processing a fork.

Does it ONLY refer to a calling of the extension.force_rollback() to set that flag?

Copy link
Member Author

@antiochp antiochp Aug 29, 2019

Choose a reason for hiding this comment

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

Say we are at block height 5 on both main chain and header chain.

1 <- 2 <- 3 <- 4 <- 5

If we now receive a block at height 4' that forks from the main chain then we need to "rewind" back to block height 3 to be able to process this new block.

1 <- 2 <- 3 <- 4'

We would then rollback the MMR as this block did not increase the total work, i.e. not a reorg, just a fork.

If we then saw 5' that did increase total work compared to 5 then we would rewind as before and then persist the MMR and update the head (and header_head) to reflect the chain after the reorg.

@antiochp
Copy link
Member Author

Merging this.

@antiochp antiochp merged commit 357bc11 into mimblewimble:master Aug 29, 2019
@antiochp antiochp deleted the fix_invalid_root branch August 29, 2019 09:15
@j01tz j01tz mentioned this pull request Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants