Incremental tally #745

ecioppettini · 2022-02-09T20:23:10Z

Change the tally to be done incrementally (with each vote)

Some comments on a few things, briefly paraphrasing the original story. The git history is a bit messy, but I guess we'll squash anyway.

VoteCast certificate

Tally is computed incrementally, e.g. the system takes the current voting power, user’s choice, multiplies them and adds to the tally.

With the current implementation the voting power is taken at the moment of the vote, initially I wanted to keep the immutable snapshot of the TokenDistribution inside the VotePlanManager from the start, but then the order of MintToken fragments and VotePlan fragments in the block0 would matter, and I think it's a bit fragile. In any case, the behavior is the same since the voting power won't change.

Ballots are no longer stored in the ledger state.
The voting ledger stores whether a user voted for the proposal or not.
Based on this the ledger prevents a user from casting a vote more than once for the same proposal.

There were a few tests that used the stored payloads for validation, I rewrote those so the assertions are based on the tally result instead.

EncryptedTally certificate

This certificate is no longer needed and should be removed.

Questions here

I kept the binary tag number unused, mostly because it may lead to a better error if we use a new version of the library to read and old fragment for some reason, but maybe it's better to compact things (there is a TODO for this in the code).

Also, are we updating the abnf file? I think we are not, so I didn't bother.

Getting the state of vote tallying as an API user

GET /api/v0/vote/active/plans

The vote tally was available via this endpoint from the start, the only difference now is that it is available for the whole duration of the voting process (e.g. could be non-optional).

This needs to be done in jormungandr too, of course, but I removed the Option from the VoteProposalStatus.

There is only one thing that I'd like to clarify here, the total_stake in the private tally is now taken at the moment the statuses are requested. I didn't want to store it from the beginning for the same thing I mentioned above, if there are MintToken certificates processed after the VotePlan then we won't consider them, unless we add some extra code to handle that case in Ledger::new.

I don't expect this to be a performance concern at the moment, since we likely won't have that many tokens, but it depends on how often the statuses are requested too. Although, to be honest, I'm not quite convinced the total_stake should be part of the tally state, but I guess it's coupled for pragmatic reasons.

ecioppettini · 2022-02-09T20:27:57Z

chain-impl-mockchain/src/vote/manager.rs

-
-        assert_eq!(
-            vote_plan_manager
-                .start_private_tally(token_distribution, block_date, committee_id)


I removed this test since start_private_tally is not there anymore... And I don't see this testing anything else, but it's weird to delete a test, so I would appreciate a second pair of eyes at this.

zeegomo · 2022-02-09T22:11:53Z

but then the order of MintToken fragments and VotePlan fragments in the block0 would matter, and I think it's a bit fragile

Contrasting feeling for this. On one side, we usually put starting conditions in block0 besides basic account creation, but on the other hand block0 is just a block like all others, and ordering is an important property of blocks.
We should also consider VotePlans submitted after block0.

Also, are we updating the abnf file? I think we are not, so I didn't bother.

I updated it for the update proposal changes, but not sure it reflects everything. I would like it to be maintained, it's an easy way to look at chain formats without having to dig through the code.

with current implementation the voting power is taken at the moment of the vote
In any case, the behavior is the same since the voting power won't change

This only works because token transfers are not implemented, but they are not guaranteed to remain so forever (actually there are good reasons to implement that). Please add a big disclaimer in the code for this.

ecioppettini · 2022-02-09T22:36:56Z

Contrasting feeling for this. On one side, we usually put starting conditions in block0 besides basic account creation, but on the other hand block0 is just a block like all others, and ordering is an important property of blocks.
We should also consider VotePlans submitted after block0.

I mean, I think the order should matter... It's just that in this case it's a bit strange, currently the VotePlanManager creation is tied to the VotePlan certificate, but to me it would make more sense if it's instanced just when it is the actual voting date. Like, to me it makes no sense that a VotePlan should only see the tokens that were declared before, since the actual voting will be in the future.

This only works because token transfers are not implemented, but they are not guaranteed to remain so forever (actually there are good reasons to implement that). Please add a big disclaimer in the code for this.

I'm not sure a disclaimer about what? The issue is that I actually don't know what do we want even if we allowed transfers. My point was that currently both solutions are the same, so there is no much reason to choose one over the other right now.

Like, I think one option could be to take a 'snapshot' at the moment the voting starts, and even if you transfer after that, we would use the initial voting power. In this case I would actually change the current code.

But another option could be to use the voting power at the moment of the vote, and lock them (or lock them before, it's the same) after that. If we allow minting during the voting period then we would also have to wait for the end of the voting period to know the total stake... In this case I think the current way is closest, and I don't feel like a disclaimer adds too much information, in this where should we even put it? in the vote cast handler?

zeegomo · 2022-02-10T12:30:22Z

Like, to me it makes no sense that a VotePlan should only see the tokens that were declared before, since the actual voting will be in the future.

Probably something like freezing when voting starts? There was this idea of forbidding transfers when a voteplan is in the voting stage

I'm not sure a disclaimer about what?

Just a disclaimer that when we want to implement token transfers we should think of a solution for this. I was thinking to put that in the relevant part of tokens implementation

chain-impl-mockchain/src/fragment/mod.rs

chain-impl-mockchain/src/vote/manager.rs

eugene-babichenko · 2022-02-15T12:35:49Z

@zeegomo @Enzoc4 you expressed some good concerns, but I think that we should come back to this when minting and transfers during the blockchain run are actually possible.

eugene-babichenko · 2022-02-15T12:36:56Z

I think we can also remove rayon from dependencies.

eugene-babichenko · 2022-02-15T12:51:19Z

Ran benchmarks on my laptop, no significant impact on vote casting performance.

eugene-babichenko

Could you please add a test for zero token votes? Should be good to go after that.

since the Tally is always present, we don't need Option anymore

since it simplifies the ProposalManager constructor and it can be obtained easily at the end

because the tally is incremental, we need to know the stake

since the tally is done incrementally, each vote tests that

I think in the future we may want to move it inside again, but for the time being this simplifies the ledger initialization, since otherwise the order of MintToken and VotePlan fragments would matter, which is OK but IMHO a bit fragile.

by using normal borrows, at the expense of making the tests slightly more verbose

a remnant from the moment I deleted the EncryptedVoteTally cert

Previously this was done only on the `EncryptedVoteTally` certificate. So currently the committee end/finish date are not used for private voteplans. This reintroduces those checks, but moves them to the moment of decrypting the tally. This is not exactly the same behaviour as before, since previously we were only verifying that the decrypted shares were submitted after the committee start date (because otherwise the tally was None, so there is nothing to decrypt). Also, add tests for these cases, since we were only testing the public case.

since the vote/tally date validations should account for it

it's not possible anymore to cast a vote with 0 voting power, and there is already a test for that, so asserting that it is an error here wouldn't add that much information, so for now, just test with an empty tally.

zeegomo · 2022-02-24T14:50:36Z

chain-impl-mockchain/src/vote/manager.rs

+            result
+                .add_vote(Choice::new(u8::try_from(choice).unwrap()), weight)
+                .map_err(|error| match error {
+                    VoteError::InvalidChoice { options, choice } => {


How can this actually happen? We build the tally with the correct number of options ourselves

Well, I think it can't, Tally::verify should take care of validating that, I think?

ecioppettini self-assigned this Feb 9, 2022

ecioppettini commented Feb 9, 2022

View reviewed changes

eugene-babichenko reviewed Feb 15, 2022

View reviewed changes

eugene-babichenko suggested changes Feb 16, 2022

View reviewed changes

eugene-babichenko mentioned this pull request Feb 16, 2022

address vit-2568, remove panic on babysteps if max_value = 0 #750

Merged

eugene-babichenko approved these changes Feb 16, 2022

View reviewed changes

ecioppettini mentioned this pull request Feb 17, 2022

Incremental tally input-output-hk/jormungandr#3856

Merged

ecioppettini force-pushed the incremental-tally branch from a7bbd89 to fb8e159 Compare February 21, 2022 17:27

Enzo Cioppettini added 16 commits February 22, 2022 12:49

remove encrypted_vote_tally.rs

1bbdea2

vote manager implement incremental tally

1078e99

vote/ledger.rs: incremental tally needs token distribution

ab34c46

VoteProposalStatus: remove ballots and Option

f934830

since the Tally is always present, we don't need Option anymore

VoteTally: remove total stake from Private

5a6c099

since it simplifies the ProposalManager constructor and it can be obtained easily at the end

ledger: vote cast now needs token distribution

cf675a1

because the tally is incremental, we need to know the stake

rename finalize_private_tally to private_tally

8aea479

vote manager fix imports

aec41ac

derive Clone for TokenDistribution

48eef04

remove incorret tally test

387fa99

since the tally is done incrementally, each vote tests that

reintroduce missing imports

1ef6a5c

remove encrypted tally

252e570

incremental tally is always present

89ba077

cleanup imports

03dd76b

incremental: handle invalid vote error

04ebd69

vote manager tests fix imports

02ebaca

Enzo Cioppettini added 17 commits February 22, 2022 12:57

reduce imhamt cloning in token distribution

25afab6

by using normal borrows, at the expense of making the tests slightly more verbose

reintroduce token totals (fixup this)

b559fa7

fix some clippy lints

4abc5ef

move function argument to the end

0dc052f

remove extra confirm transaction in test

3fec16c

a remnant from the moment I deleted the EncryptedVoteTally cert

remove encrypted vote tally from benchmark

5aa9f90

fill fragment tag gaps

25dca59

remove rayon from dependencies

b375215

replace TallyAlreadyDecrypted for unreachable!

c563c60

since the vote/tally date validations should account for it

reject votes with zero tokens

547836b

add missing initial tokens in tests

41b65b1

add rayon to dev-dependencies for the bench

82eeff9

add vote_manager_zero_tokens_vote_should_fail

66e098d

remove useless .clone()

fe1080b

remove zero voting power vote cast from decrypt_tally panic test

445f188

it's not possible anymore to cast a vote with 0 voting power, and there is already a test for that, so asserting that it is an error here wouldn't add that much information, so for now, just test with an empty tally.

ecioppettini force-pushed the incremental-tally branch from fb8e159 to 18d10ad Compare February 22, 2022 16:36

fix borrowing error

288ba0d

ecioppettini force-pushed the incremental-tally branch from 18d10ad to 288ba0d Compare February 22, 2022 16:45

zeegomo reviewed Feb 24, 2022

View reviewed changes

ecioppettini merged commit 1626503 into master Feb 25, 2022

ecioppettini deleted the incremental-tally branch February 25, 2022 23:01

Incremental tally #745

Incremental tally #745

Uh oh!

Conversation

ecioppettini commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change the tally to be done incrementally (with each vote)

VoteCast certificate

EncryptedTally certificate

Questions here

Getting the state of vote tallying as an API user

Uh oh!

ecioppettini Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeegomo commented Feb 9, 2022

Uh oh!

ecioppettini commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeegomo commented Feb 10, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eugene-babichenko commented Feb 15, 2022

Uh oh!

eugene-babichenko commented Feb 15, 2022

Uh oh!

eugene-babichenko commented Feb 15, 2022

Uh oh!

eugene-babichenko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeegomo Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

ecioppettini Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ecioppettini commented Feb 9, 2022 •

edited

Loading

ecioppettini Feb 9, 2022 •

edited

Loading

ecioppettini commented Feb 9, 2022 •

edited

Loading

eugene-babichenko left a comment •

edited

Loading