Skip to content

Change ifelse branches in CountVotes() - #367

Merged
aguycalled merged 4 commits into
navcoin:masterfrom
aguycalled:change-logic-count-votes
Dec 15, 2018
Merged

Change ifelse branches in CountVotes()#367
aguycalled merged 4 commits into
navcoin:masterfrom
aguycalled:change-logic-count-votes

Conversation

@aguycalled

@aguycalled aguycalled commented Dec 14, 2018

Copy link
Copy Markdown
Member

This PR fixes a bug in the logic of the CountVotes() function which can cause a proposal to morph from a Expired state back to an Accepted state when a node reindex the chainstate db and the proposal expired being accepted.

The fix involves changing the structure of some conditionals, so in the case when a proposal state and status do not match the next else if branches are not triggered. It also considers a proposal unexpired state as always expired. See the changes list for more details.

This issue is critical as it can cause two different states in different nodes and a fork in the network.

aguycalled pushed a commit to aguycalled/navcoin-core that referenced this pull request Dec 14, 2018
- Bumps version to v4.5.1
- Sets a fork height at 2722100
- Requires blocks to signal version bit 21 when fork happens.
Merge after navcoin#367 navcoin#369 and navcoin#370 or when its related issues are fixed.
@aguycalled aguycalled mentioned this pull request Dec 14, 2018
@proletesseract

Copy link
Copy Markdown
Member

@marcus290 what have you tested on this fix? do we need to write or update any unit tests?

@proletesseract

Copy link
Copy Markdown
Member

im compiling and testing this now

@proletesseract proletesseract self-assigned this Dec 15, 2018
@proletesseract

Copy link
Copy Markdown
Member

I've compiled and run the tests and they all pass. I also compiled the master branch and ran the tests against that and they pass there too. So I'm unable to say just by the tests that the bug is fixed. I am wondering if when we find a bug like this, the first step should be to write a test that fails when the bug is present but should pass when the bug is fixed. Therefore we can definitively say that the bug is fixed. I am going to see if i can modify the test to reproduce the issue on master and resolve it on the patch branch.

@marcus290

Copy link
Copy Markdown
Contributor

@craigmacgregor there are two issues -

  1. Reindexing will recalculate expired proposals.
  2. Expired proposals will change to expired (fState: 3 under listproposals) but the status will show "accepted waiting for end of voting period". At the end of the voting cycle, the proposal will be reaccepted.

Steps to reproduce the issues:

  • open testnet and let the wallet fully sync. Then reopen the wallet with -reindex-chainstate. A previously expired proposal will reappear when running listproposals because its expiry has been recalculated.
  • create a short life proposal on regnet and let it expire. Then check the status upon expiry and after the next voting cycle.

I manually retested both these issues with this commit and they are fixed according to my tests.

Agreed we should write tests.

@aguycalled
aguycalled merged commit c59cd80 into navcoin:master Dec 15, 2018
aguycalled pushed a commit to skreener/navcoin-core that referenced this pull request Feb 3, 2019
- Bumps version to v4.5.1
- Sets a fork height at 2722100
- Requires blocks to signal version bit 21 when fork happens.
Merge after navcoin#367 navcoin#369 and navcoin#370 or when its related issues are fixed.
aguycalled pushed a commit to skreener/navcoin-core that referenced this pull request Feb 3, 2019
* Change ifelse branches count votes

* Change ifelse branches count votes for prequests

* a proposal in state expired is always expired

* add test
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