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

Handle getBlockHeader special cases for proofs #394

Merged
merged 2 commits into from
Jun 11, 2019

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Jun 10, 2019

Issue Number

#386

Overview

  • I make sure we don't crash in getBlockHeader when it contains a proof. (see added comment in getBlockHeader)
  • I added an empty, non-genesis BFT block as a golden test

Comments

  • Not tested with Genesis/Praos
  • I verified that the test fails without the fix
  • Restructuring the unit tests with a TestVector type might be a good idea soon. But I didn't do it yet.

@Anviking Anviking self-assigned this Jun 10, 2019
@Anviking Anviking force-pushed the anviking/386/handle-header-proofs branch 3 times, most recently from 9536786 to 0b40ed5 Compare June 10, 2019 21:35
@Anviking Anviking requested a review from KtorZ June 10, 2019 22:16
@Anviking Anviking force-pushed the anviking/386/handle-header-proofs branch from 0b40ed5 to 50bf389 Compare June 10, 2019 22:27
@Anviking Anviking force-pushed the anviking/386/handle-header-proofs branch from 50bf389 to 7af29d8 Compare June 11, 2019 08:21
@Anviking Anviking force-pushed the anviking/386/handle-header-proofs branch from 7af29d8 to 38a02d9 Compare June 11, 2019 10:04
@Anviking Anviking requested a review from akegalj June 11, 2019 11:17
Copy link
Contributor

@akegalj akegalj left a comment

Choose a reason for hiding this comment

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

This looks good to me. Lets wait for Matthias to comment as well


-- We could make sure we get the right kind of proof, but we don't need to.
-- Just checking that the length is not totally wrong, is much simpler
-- and gives us sanity about the binary format being correct.
Copy link
Contributor

Choose a reason for hiding this comment

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

I also think this is good enough

Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

LGTM!

@KtorZ KtorZ merged commit e897eaf into master Jun 11, 2019
@KtorZ KtorZ deleted the anviking/386/handle-header-proofs branch June 11, 2019 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants