Skip to content

Interim fix for #391 cfund votingCycle increments#396

Merged
marcus290 merged 15 commits into
navcoin:masterfrom
marcus290:cfund-last-cycle-temp-fix
Feb 7, 2019
Merged

Interim fix for #391 cfund votingCycle increments#396
marcus290 merged 15 commits into
navcoin:masterfrom
marcus290:cfund-last-cycle-temp-fix

Conversation

@marcus290
Copy link
Copy Markdown
Contributor

@marcus290 marcus290 commented Jan 9, 2019

This PR is an interim fix for #391.

Uses aguycalled@9c1db64 to stop votingCycles being recalculated if there is a reorg. Adds a test to check this, which fails in the current master branch but passes with this patch.

Expired proposals and payment requests will still increment +1 over their maximum number of cycles as per the existing cfund design. But the string output from toJson() will be limited to the max number of cycles.

Also stops votes from being counted in the votingCycle nCyclesPaymentRequestVoting + 1 and nCyclesProposalVoting + 1.

@aguycalled
Copy link
Copy Markdown
Member

ideally tests should be duplicated, with one of them launching the node with flag rejectversionbit=n to verify behaviour in pre-fork scenarios and the other verifying behaviour in post-fork scenarios

@marcus290 marcus290 closed this Jan 10, 2019
@marcus290 marcus290 reopened this Jan 10, 2019
@proletesseract
Copy link
Copy Markdown
Member

Isn't this change cosmetic only? Do we need to attach the change to the version bit change?

@matt-auckland matt-auckland added this to the 4.5.2 milestone Jan 31, 2019
Comment thread src/consensus/cfund.cpp Outdated
Comment thread src/consensus/cfund.cpp Outdated
Comment thread src/consensus/cfund.cpp Outdated
Comment thread src/consensus/cfund.cpp
Comment thread src/consensus/cfund.cpp
@matt-auckland
Copy link
Copy Markdown
Member

I've reviewed the code except for the tests, the code looks good for the most part.
Will check the tests later. For now see the comments I've left.

Copy link
Copy Markdown
Member

@matt-auckland matt-auckland left a comment

Choose a reason for hiding this comment

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

Approved pending Travis build

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.

4 participants