Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

ReworkVoteTallyCache to better represent purpose #360

Merged
merged 3 commits into from
Dec 4, 2018

Conversation

rain-on
Copy link
Contributor

@rain-on rain-on commented Dec 4, 2018

VoteTallyCache has been refactored to simplify some logic.

Resolved issue whereby an existing voteTally could be polluted with votes from the next block.

@rain-on rain-on requested review from ajsutton and jframe December 4, 2018 01:26
@rain-on rain-on changed the title Renaming functions in Votetally and cache to better represent their p… ReworkVoteTallyCache to better represent purpose Dec 4, 2018
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM.

voteTallyUpdater.updateForBlock(h, tally);
voteTallyCache.put(h.getHash(), tally.copy());
voteTallyUpdater.updateForBlock(h, mutableVoteTally);
voteTallyCache.put(h.getHash(), mutableVoteTally.copy());
Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, this is the key to the bug fix. Previously the VoteTally that was originally retrieved from the cache was modified and then a copy added for the new block, resulting in the original and new both being the vote tally for the new block. Now it copies before modifying so the original value is unchanged.

…urpose

VoteTallyCache has been refactored to simplify some logic.
@rain-on rain-on force-pushed the vtc branch 2 times, most recently from f8673aa to 556b72e Compare December 4, 2018 03:49
@rain-on rain-on merged commit aac4fd4 into PegaSysEng:master Dec 4, 2018
@rain-on rain-on deleted the vtc branch January 16, 2019 21:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants