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

R4R: Fixing bug in genesis Equal #3502

Merged
merged 4 commits into from
Feb 6, 2019
Merged

Conversation

jleni
Copy link
Member

@jleni jleni commented Feb 5, 2019

While working on increasing coverage, I noticed a typo in a possibly critical area.
It does not seem to have an impact at this moment but I would suggest the fix gets applied soon.
I have only included a test for the specific bug/fix. In the next few days, we will increase coverage in other PRs.

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@jleni jleni force-pushed the bug/genesis_check branch from 85df591 to d88ee17 Compare February 5, 2019 17:36
@jleni jleni changed the title Fixing bug in genesis Equals Fixing bug in genesis Equal Feb 5, 2019
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

wow, good catch @jleni -- is this r4r?

@jleni jleni changed the title Fixing bug in genesis Equal WIP: Fixing bug in genesis Equal Feb 5, 2019
@jleni
Copy link
Member Author

jleni commented Feb 5, 2019

Give me a few minutes, I think I found another problem in the same area.

@jleni
Copy link
Member Author

jleni commented Feb 5, 2019

Yes, the new test reproduces another issue.
Two states with identical proposals are recognized as different.

@jleni jleni force-pushed the bug/genesis_check branch from 09e18f9 to 624558d Compare February 5, 2019 18:02
@jleni jleni force-pushed the bug/genesis_check branch from 624558d to 84a7345 Compare February 5, 2019 18:04
@jleni jleni changed the title WIP: Fixing bug in genesis Equal R4R: Fixing bug in genesis Equal Feb 5, 2019
@jleni
Copy link
Member Author

jleni commented Feb 5, 2019

@alexanderbez It is now ready for review.
But this makes me wonder if there are other similar situations where proposals are not compared correctly, etc. These things will likely surface as we increase coverage.

@jleni jleni added T:Bug ready-for-review C:genesis relating to chain genesis labels Feb 5, 2019
@cwgoes
Copy link
Contributor

cwgoes commented Feb 5, 2019

Seems to fail CI.

Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

Brilliant, thanks!

@jleni
Copy link
Member Author

jleni commented Feb 5, 2019

Seems to fail CI.

Yep :| I'll have a look after dinner. I've seen already that integration test sporadically fail. Not sure if this fail is related to the change or not.

@cwgoes
Copy link
Contributor

cwgoes commented Feb 5, 2019

Seems to fail CI.

Yep :| I'll have a look after dinner. I've seen already that integration test sporadically fail. Not sure if this fail is related to the change or not.

It might not be; I think this is a persistent issue.

@jleni
Copy link
Member Author

jleni commented Feb 5, 2019

Yep, now it passed.
Maybe I can work out tomorrow why that happens.

@cwgoes cwgoes merged commit 1766a73 into cosmos:develop Feb 6, 2019
@jleni jleni deleted the bug/genesis_check branch February 6, 2019 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:genesis relating to chain genesis T:Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants