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

Rollback fixes #8

Merged
merged 2 commits into from May 17, 2019
Merged

Rollback fixes #8

merged 2 commits into from May 17, 2019

Conversation

rooooooooob
Copy link

@rooooooooob rooooooooob commented May 16, 2019

During testing of rollback feature, two issues were found that resulted in manlfunctions or panics:

HeaderHash vs BlockHash misconversion

HeaderHash and BlockHash are the same hash but just Blake2b256 vs raw byte slice of the same hash.
This caused header_hash() to re-hash the hash, giving incorrect hashes
and thus breaking the rollback code that it was written for.

This change was tested with the new rollback code and with a test
assert.

Asserts not taking into account boundary blocks not increasing difficulty

Epoch boundary blocks don't contribute to the difficulty of the chain,
but some asserts in Storage written for the rollback feature did not
take this into account, which caused panics during rollback.

These changes were tested under rollback (and existing unit tests).

HeaderHash and BlockHash are the same hash but just Blake2b256 vs raw byte slice of the same hash.
This caused header_hash() to re-hash the hash, giving incorrect hashes
and thus breaking the rollback code that it was written for.

This change was tested with the new rollback code and with a test
assert.
Epoch boundary blocks don't contribute to the difficulty of the chain,
but some asserts in Storage written for the rollback feature did not
take this into account, which caused panics during rollback.

These changes were tested under rollback (and existing unit tests).
@rooooooooob rooooooooob changed the title Fix LooseChainHeightEntry::header_hash() Rollback fixes May 16, 2019
Copy link

@vsubhuman vsubhuman left a comment

Choose a reason for hiding this comment

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

Nice! 🙌

@vsubhuman vsubhuman merged commit 217e5ef into master-emurgo May 17, 2019
rooooooooob added a commit that referenced this pull request May 20, 2019
HeaderHash::new() will double-hash the value, leading to panics and bad
behavior. This is the same issue as in #8 and I have grepped all other
uses of HeaderHash::new() and I did not see any other incorrect uses
besides this and #8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants