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

JIT EBBs in the ThreadNet tests require ExtLedgerState in block production #2048

Merged

Conversation

nfrisby
Copy link
Contributor

@nfrisby nfrisby commented May 8, 2020

Fixes #2047.

PR #1942 changed blockProduction to take a LedgerState instead of
ExtLedgerState, which suffices for all forging. However, the ThreadNet tests'
current approach to EBBs requires anticipating during block production whether
the produced block will be invalid in anyway. (The node does not emit the EBB
unless the EBB unless it is also emitting a valid block.) And that anticipatory
validation must include protocol-level errors, which requires the full
ExtLedgerState.

It may be possible to instead change the tests to allow for EBBs without
successors, but I'm don't see how, so I currently propose leaving that for future work if we
prioritize it.

Edit: PR 2176 essentially fixed this failure mode en passant. This PR now just simplifies the JIT EBB logic accordingly.

@nfrisby nfrisby requested a review from edsko May 8, 2020 00:40
@nfrisby nfrisby force-pushed the nfrisby/issue-2047-jit-ebb-extledger-blockproduction branch from f2acddc to ad6a8a1 Compare May 18, 2020 19:35
@nfrisby nfrisby requested a review from mrBliss May 18, 2020 19:35
@nfrisby nfrisby force-pushed the nfrisby/issue-2047-jit-ebb-extledger-blockproduction branch 3 times, most recently from 9290c8d to f39085d Compare May 29, 2020 00:07
@nfrisby
Copy link
Contributor Author

nfrisby commented May 29, 2020

@edsko and I reviewed on a call. Thanks!

Plan: re-expanding the produceBlock API as this PR currently does is undesirable. Edsko saw alignment with other concerns and so pressed for the new CannotLead mechanism (see merged PR #2169). I'm going to rebase this PR to rely on that instead; the goal is that the tests should never generate an invalid block because the CannotLead check will abort the thread upstream. Until Issue #2173, this will also involve dialing back the Gen NodeJoinPlan so that it avoids all PBftExceededSignThreshold errors.

@nfrisby
Copy link
Contributor Author

nfrisby commented May 29, 2020

Once PR #2176 is merged, this PR will change dramatically; it will merely simplify the JIT EBB generator, under the assumption that (at least in the test suite for now) nodes now never forge invalid blocks!

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.

Just marking this as changes required. The changes are already described by @nfrisby himself, just want to avoid an accidental merge.

@nfrisby nfrisby force-pushed the nfrisby/issue-2047-jit-ebb-extledger-blockproduction branch from f39085d to 72575e6 Compare June 1, 2020 13:57
@nfrisby nfrisby force-pushed the nfrisby/issue-2047-jit-ebb-extledger-blockproduction branch from 72575e6 to 7f8dee7 Compare June 1, 2020 14:00
@nfrisby nfrisby requested a review from edsko June 1, 2020 14:02
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.

Nice, glad that that worked out!

@edsko
Copy link
Contributor

edsko commented Jun 1, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 1, 2020

@iohk-bors iohk-bors bot merged commit 218126a into master Jun 1, 2020
@iohk-bors iohk-bors bot deleted the nfrisby/issue-2047-jit-ebb-extledger-blockproduction branch June 1, 2020 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JIT EBB require ExtLedgerState in block production
2 participants