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 bugs: (1) infinite loop in e2e tests (2) missing Vote Extensions at PrepareProposal time #539

Merged
merged 6 commits into from
Mar 16, 2023

Conversation

sergio-mena
Copy link
Contributor

@sergio-mena sergio-mena commented Mar 15, 2023

Contributes to #10

While running e2e tests locally, I detected a hard-to-diagnose bug with vote extensions. This PR contains the fix.

Since I did this in isolation (due to remote work), I'll make an effort at explaining the troubleshooting process in this PR. Hence, this PR will be a succession of commits which show the sketch on how I diagnosed and fixed the bug. So, if the reviewer wants to understand what the problem was and how it was solved, the presence of the little ❌ or ✅ next to each commit below is important (you can click and check how the e2e tests failed/passed).

Additionally, I also found that the e2e tests where blocking forever when a node was crashing. This is fixed in the first commit.

  • Commit#1: fix the infinite loop in e2e runner that may block the execution indefinitely.
  • Commit#2: adapt ci.toml to only have one validator, since the bug appears when a recovered node has to propose.
  • Commit#3: under the hypothesis that something is wrong with VoteExtensionsEnableHeight, we plant a panic to have the backtrace if it happens.
    • The enable height in ci.toml is set to 1007 from genesis, so we should never return zero in this test case.
  • Commit#4: contains the fix. Every time we call reconstructLastCommit method of State, immediately after we call updateToState. Therefore the state object whose method we are calling is incomplete. The fix just queries the complete object, which is passed as a normal parameter (and which will be given to updateToState immediately after).
    • Note that, although some checks are failing on that commit (hence the ❌), e2e are now passing
  • Commit#5: remove diagnosis-related code, i.e., commit#2 and commit#3

PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

@sergio-mena sergio-mena requested a review from a team as a code owner March 15, 2023 17:18
@sergio-mena sergio-mena mentioned this pull request Mar 15, 2023
16 tasks
@sergio-mena sergio-mena added the abci Application blockchain interface label Mar 15, 2023
@sergio-mena sergio-mena self-assigned this Mar 15, 2023
Revert "Insert a `panic` to help diagnose the problem (TO REVERT)"

This reverts commit a127993.

Revert "Change ci.toml to exacerbate the probability of hitting the bug (TO REVERT)"

This reverts commit b8cca77.
@sergio-mena sergio-mena changed the title Fix bug with missing Vote Extensions at PrepareProposal time Fix bugs: (1) infinite loop in e2e tests (2) missing Vote Extensions at PrepareProposal time Mar 15, 2023
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

Great catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abci Application blockchain interface
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants