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

Add PBFT CannotLead checks #2176

Merged
merged 3 commits into from
Jun 1, 2020
Merged

Add PBFT CannotLead checks #2176

merged 3 commits into from
Jun 1, 2020

Conversation

nc6
Copy link
Contributor

@nc6 nc6 commented May 29, 2020

  • Check that we are the current delegate of the genesis key
  • Check that more blocks than the singing threshold have not been issued.

Addresses #2172

@nc6 nc6 requested review from edsko and nfrisby May 29, 2020 15:13
@edsko
Copy link
Contributor

edsko commented May 29, 2020

Ah! Also addresses #2173 .

@edsko
Copy link
Contributor

edsko commented May 29, 2020

@nfrisby could you confirm that this means that your tests will work without the invalid blocks check? That would be a great indication that this is doing the right thing.

@nfrisby
Copy link
Contributor

nfrisby commented May 29, 2020

@nfrisby could you confirm that this means that your tests will work without the invalid blocks check? That would be a great indication that this is doing the right thing.

OK. I'll develop a PR on top of this one to simplify the test suite accordingly.

Might not want to block this on that -- not sure how the schedule will proceed.

@nfrisby
Copy link
Contributor

nfrisby commented May 29, 2020

Bah. CI failure demonstrates that the answer is "No". I'll increase my priority on it. The particular test that failed is a corner case.

@nfrisby
Copy link
Contributor

nfrisby commented May 29, 2020

FYI https://github.com/input-output-hk/ouroboros-network/tree/nfrisby/issue-2172

I've pushed a commit to that branch that extends this PR. It updates the test infrastructure as necessary.

Please cherry-pick it onto this one, if you like it.

The test infrastructure has so far been expecting invalid blocks to be caught by the ChainDB. Since some tests knowingly induce invalid blocks, those tests had been validating such block rejections.

As of this PR, those invalid blocks are never even forged. But we still want to validate the failure to lead. (The fact that we no longer could do so is why the CI failed.) My commit therefore adjusts the test infrastructure to validate CannotLeads instead of BlockRejections.

@edsko
Copy link
Contributor

edsko commented Jun 1, 2020

@nfrisby that sounds great. I will take a look at this PR now, as well as the commit you refer to, and then hopefully merge.

@edsko
Copy link
Contributor

edsko commented Jun 1, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 1, 2020

👎 Rejected by too few approved reviews

Copy link
Contributor

@edsko edsko left a comment

Choose a reason for hiding this comment

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

Thanks Nick and Nick :)

@edsko
Copy link
Contributor

edsko commented Jun 1, 2020

bors merge

iohk-bors bot added a commit that referenced this pull request Jun 1, 2020
2176: Add PBFT CannotLead checks r=edsko a=nc6

- Check that we are the current delegate of the genesis key
- Check that more blocks than the singing threshold have not been issued.

Addresses #2172

Co-authored-by: Nicholas Clarke <[email protected]>
Co-authored-by: Nicolas Frisby <[email protected]>
Co-authored-by: Edsko de Vries <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 1, 2020

Build failed

nc6 and others added 3 commits June 1, 2020 09:31
Also delete an accidental `p` in a comment, and delete a comment about the
"mock genesis block" not having a number; there _is_ no mock genesis block.
When we are at genesis, there /are/ no blocks.
@edsko
Copy link
Contributor

edsko commented Jun 1, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 1, 2020

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.

3 participants