Create abstract election class by z-bowen · Pull Request #2498 · bigchaindb/bigchaindb

z-bowen · 2018-08-31T11:59:39Z

**Problem: ValidatorElection and MigrationElection need to inherit from a generic Election class

Solution

Factored the common logic out of ValidatorElection and moved it to a common parent class, Election

…from a common `Election` class Solution: Factored the common logic out of `ValidatorElection` and moved it to `Election` parent class

Solution: Reverting the changes

codecov-io · 2018-08-31T12:17:28Z

Codecov Report

Merging #2498 into master will decrease coverage by 0.15%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##           master    #2498      +/-   ##
==========================================
- Coverage   93.53%   93.37%   -0.16%     
==========================================
  Files          42       43       +1     
  Lines        2551     2582      +31     
==========================================
+ Hits         2386     2411      +25     
- Misses        165      171       +6

kansi · 2018-09-03T08:17:31Z

bigchaindb/backend/localmongodb/query.py


 @register_query(LocalMongoDBConnection)
-def get_validator_set_by_election_id(conn, election_id):
+def get_result_by_election_id(conn, election_id, table):


A more appropriate function name could be get_election_result/get_election_result_by_id ?

What is the point of this generalization?

Good suggestion. Changed the name.

When checking the election status, the first thing I want to do is look and see if anything with this election_id has been written to the relevant Mongo table, (ie has the vote been previoously concluded.)

ldmberman · 2018-09-03T08:18:49Z

bigchaindb/backend/localmongodb/query.py


 @register_query(LocalMongoDBConnection)
-def get_validator_set_by_election_id(conn, election_id):
+def get_result_by_election_id(conn, election_id, table):


Creating a new collection for every election might be cumbersome. Instead, we can introduce an elections collection storing documents of the form:

{ 'election_id': '...', 'type': '...', 'height': X }

What would be the use of 'type'?

type is what is operation in the transaction schema. For instance, if you want to find out if there was a validator update caused by the given election ID, you need to filter by type.

I don't think you need type once you have the election id, there shouldn't be two elections with same election id and yet have different operations

Yes, but when you receive an election ID in the command line, you do not know its type for sure. A user may look for a validator election status providing a migration election ID by mistake.

kansi · 2018-09-03T08:28:05Z

bigchaindb/common/election.py

+                'election_id': <election_id_that_approved_the_change>
+            }
+        }
+        """


I think this doc string is wrong?

:return: { 'height': <block_height>, 'validators': <validator_set>, 'election_id': <election_id_that_approved_the_change> }

Nope. I checked it. It's correct.

Don't think so, bigchain.get_validator_change(height) calls backend.query.get_validator_set(self.connection, height) which returns the validator object of the following format,

{'height': <block_height>, 'validators': <validator_set>, 'election_id': <election_id_that_approved_the_change> }

kansi · 2018-09-03T08:32:02Z

bigchaindb/common/election.py

+    def count_votes(cls, election_pk, transactions, getter=getattr):
+        votes = 0
+        for txn in transactions:
+            if getter(txn, 'operation') == 'VALIDATOR_ELECTION_VOTE':


The generalized class cannot contain hard-coded value.

But 'VALIDATOR_ELECTION_VOTE' still exists

ldmberman · 2018-09-03T08:35:48Z

bigchaindb/common/election.py

+        return result
+
+    @classmethod
+    def is_approved(cls, bigchain, new_height, txns):


is_approved as a name suggests the function returns a boolean, while in fact it returns either [] or an arbitrary on_approve result.

I would add a function called get_approved_election_or_none, which returns what its name says. Additionally, in the ValidatorElection class I would keep get_validators_update, which in turn would use the generic get_approved_election_or_none.

What do you think?

Solution: Refactored `get_result_by_election_id` to `get_election_result_by_id`

kansi · 2018-09-03T10:23:26Z

bigchaindb/backend/query.py

 @singledispatch
-def get_asset_tokens_for_public_key(connection, asset_id,
-                                    public_key, operation):
+def get_asset_tokens_for_public_key(connection, asset_id, public_key):


…ables Solution: Remove `DB_TABLE` property from `Election` class

Solution: Election ID has to be unique but not every validator set record has it. MongoDB does not support partial indexes, does not even allow for multiple Nones. This is a temporary fix since we are introducing an `election` collection to store election IDs in bigchaindb#2498.

Solution: Election ID has to be unique but not every validator set record has it. MongoDB does not support partial indexes, does not even allow for multiple Nones. This is a temporary fix since we are introducing an `election` collection to store election IDs in #2498.

kansi · 2018-09-03T14:01:26Z

bigchaindb/common/election.py

+
+    # NOTE: this transaction class extends create so the operation inheritance is achieved
+    # by setting an ELECTION_TYPE and renaming CREATE = ELECTION_TYPE and ALLOWED_OPERATIONS = (ELECTION_TYPE,)
+    ELECTION_TYPE = None


In an attempt to make the code more uniform it would good to rename ELECTION_TYPE to OPERATION so that further in future when CREATE and TRANSFER both become an independent class they would also have OPERATION field and the registeration would look as follows,

Transaction.register_type(Create.OPERATION, Create) Transaction.register_type(Transfer.OPEATION, Transfer) Transaction.register_type(ValidatorElection.OPERATION, ValidatorElection) Transaction.register_type(ValidatorElectionVote.OPERATION, ValidatorElectionVote)

…ir own tables" This reverts commit db45374.

… for an election method Solution: Finished the refactoring

Solution: Created the `elections` table with secondary_index `election_id`

…tions in the `elections` table Solution: Updated the class to use the new table

Solution: Renamed, refactored and moved the `Vote` class to tie in with the more general `Election` base class

Solution: Fixed the docstring

Solution: Pointed the reference to the class variable

…ote` Solution: Renamed `TX_SCHEMA_VALIDATOR_ELECTION_VOTE` as `TX_SCHEMA_VOTE`

Solution: Renamed `ELECTION_TYPE` to `OPERATION`

kansi · 2018-09-05T13:20:31Z

bigchaindb/common/schema/transaction_vote_v2.0.yaml

  operation:
    type: string
-    value: "VALIDATOR_ELECTION_VOTE"
+    value: "OPERATION"


Value is supposed to be VOTE

It is weird that the CI tests are still passing even when the election created using .generate would not match the above schema!

but the value is still "OPERATION"?

kansi · 2018-09-05T13:21:11Z

bigchaindb/common/vote.py

-    TRANSFER = VALIDATOR_ELECTION_VOTE
-    ALLOWED_OPERATIONS = (VALIDATOR_ELECTION_VOTE,)
+    TRANSFER = VOTE
+    ALLOWED_OPERATIONS = (VOTE,)


OPERATION = VOTE should also be specified.

I don't see OPERATION = VOTE specified anywhere

kansi · 2018-09-05T13:23:47Z

bigchaindb/common/election.py

+        bigchain.store_election(height, election)
+
+    @classmethod
+    def is_approved(cls, bigchain, new_height, txns):


is_approved as the name suggests should return True or False but it is returning [] or the respective election's proposed change.

A comment was folded.

kansi · 2018-09-05T13:26:06Z

bigchaindb/lib.py


        self.store_abci_chain(block['height'] + 1, new_chain_id, False)

+    def store_election(self, height, election):


The doc string says Store election results but the function name says store_election. It would be better to name the function too as store_election_results

I still see the function name as store_election

Solution: Addressed comments from PR

kansi · 2018-09-06T09:51:01Z

bigchaindb/elections/election.py

+            :param current_transactions: (list) A list of transactions to be validated along with the election
+
+        Returns:
+            ValidatorElection object


Return value should also have a generalized name

bigchaindb/elections/vote.py

kansi · 2018-09-06T10:06:16Z

tests/tendermint/test_core.py

    assert resp.validator_updates[0] == encode_validator(validator)

-    updates = b.get_validator_update()
+    updates = b.is_approved()


There is no is_approved I think. It seems to be changed to on_approved

approved_updates is the replacement method for is_approved. This test is skipped though. Are we planning on fixing it?

kansi · 2018-09-06T10:07:09Z

tests/tendermint/test_lib.py

    query.store_validator_update(b.connection, validator_update)

-    updates = b.get_validator_update()
+    updates = b.is_approved()


There is no is_approved. It seems to be changed to on_approved

Same here. Are we going to un-skip this test?

Solution: Removed the variable and hardcoded everything to use the `Vote` class

Solution: Addressing comments

…_class

kansi · 2018-09-07T10:03:35Z

bigchaindb/upsert_validator/validator_election.py

-        else:
-            return self.ONGOING
+        updated_validator_set = [v for v in updated_validator_set if v['voting_power'] > 0]
+        bigchain.store_validator_set(new_height+1, updated_validator_set, election.id)


As we decided in our discussion we will not store election.id in the validators collection instead store the fact that election.id concluded in the elections collections

kansi · 2018-09-07T10:08:46Z

bigchaindb/elections/election.py

+        input_conditions = []
+
+        duplicates = any(txn for txn in current_transactions if txn.id == self.id)
+        if bigchain.get_transaction(self.id) or duplicates:


It would be better to use bigchain.is_committed instead of bigchain.get_transaction

I don't see anything by that name in bigchaindb/lib.py, and it doesn't work when I try to execute bigchain.get_commited in the tests.

If we want to create this method can we do it in another PR? It doesn't seem necessary to get Election working

Sorry, I was talking about this. Fixed the comment. Also, the reason I am suggesting this change is that the get_transaction required 3 database calls while is_committed requires only 1.

…a `validator_set` Solution: Removed the code to store the `election_id` and aligned the tests

…roperties Solution: Added a random `uuid4` seed to enforce uniqueness

bigchaindb/elections/election.py

kansi · 2018-09-10T08:25:12Z

tests/upsert_validator/test_upsert_validator_vote.py

+    update = ValidatorElection.approved_update(b, 4, [tx_vote2])
+    if update:
+        update_public_key = codecs.encode(update.pub_key.data, 'base64').decode().rstrip('\n')
+    assert update


It seems that we expect the update to have the new validator set. Since this is a test we don't need to play safe by using if update, we could do the following too,

update = ValidatorElection.approved_update(b, 4, [tx_vote2]) assert update update_public_key = codecs.encode(update.pub_key.data, 'base64').decode().rstrip('\n')

kansi · 2018-09-10T08:27:49Z

tests/upsert_validator/test_upsert_validator_vote.py

+    update_public_key = None
+    if update:
+        update_public_key = codecs.encode(update.pub_key.data, 'base64').decode().rstrip('\n')
+    assert update


We could do the following and avoid if and update_public_key = None

update = ValidatorElection.approved_update(b, 4, [tx_vote0, tx_vote1, tx_vote2]) assert update update_public_key = codecs.encode(update.pub_key.data, 'base64').decode().rstrip('\n')

…the tx is in the db Solution: use `is_committed` instead of `get_transaction`

ldmberman · 2018-09-10T08:52:09Z

bigchaindb/backend/localmongodb/schema.py

                                                  name='height',
                                                  unique=True,)
-
-    logger.info('Create `abci_chains.chain_id` secondary index.')


There is a conflict resolution leftover.

ldmberman · 2018-09-10T08:53:25Z

bigchaindb/backend/query.py



+@singledispatch
+def store_election_results(conn, validator_update):


The parameter was not renamed.

ldmberman · 2018-09-10T08:56:33Z

bigchaindb/common/schema/transaction_v2.0.yaml

@@ -63,7 +63,7 @@ definitions:
    - CREATE
    - TRANSFER
    - VALIDATOR_ELECTION


We also need a generic name instead of VALIDATOR_ELECTION now.

What purpose would that serve since the name would always be overriden by the specific election type?

True, there is no purpose in such an operation..

But extending the base schema every time a new election is introduced does not sound necessary either.

I would not require the operation in the base spec. Every specific spec can require the particular operation itself.

Does not have to be done in this PR though.

I don't think we do...

Since Election is a base class, and will be extended with distinct election classes, shouldn't each of those get their own operations? The ValidatorElection is still using VALIDATOR_ELECTION as it's operation.

Yes. An extra point (we can address it later) is that bigchaindb/common/schema/transaction_v2.0.yaml does not have to require the operation to be from the given set. It just makes us extend this list every time a new operation is introduced.

ldmberman · 2018-09-10T08:59:32Z

bigchaindb/common/schema/transaction_validator_election_v2.0.yaml

          - node_id
          - public_key
          - power
+          - seed


We don't have to make it required.

Should it be required for the validator election?

We do not have to require it there either.

It's generated automatically on creation of the election, so it will always be present, regardless. But I can remove the required tag, anyway.

Solution: Removed the parameter

kansi

Great work 🎆

ldmberman · 2018-09-10T14:59:44Z

bigchaindb/elections/election.py

+        :return: {
+            'height': <block_height>,
+            'validators': <validator_set>,
+            'election_id': <election_id_that_approved_the_change>


election_id is not there anymore.

…_update` Solution: Renamed to `election`

ldmberman · 2018-09-10T15:03:32Z

bigchaindb/elections/vote.py

        (inputs, outputs) = cls.validate_transfer(inputs, recipients, election_id, metadata)
-        election_vote = cls(cls.VALIDATOR_ELECTION_VOTE, {'id': election_id}, inputs, outputs, metadata)
+        election_vote = cls(cls.OPERATION, {'id': election_id}, inputs, outputs, metadata)
        cls.validate_schema(election_vote.to_dict(), skip_id=True)


We can also remove skip_id from here.

~~Because Vote is always a TRANSFER, and we always validate how identifiers are built for transfers.~~

This is not true actually..

Discussed offline - we can remove skip_id but then we also need update from_dict in common/transaction.py so that it does not pass skip_id but calls validate_id after validating the schema.

ldmberman · 2018-09-10T15:12:51Z

bigchaindb/elections/election.py

+        return self
+
+    @classmethod
+    def generate(cls, initiator, voters, election_data, metadata=None):


What do you think about adding an additional parameter generate_random_seed=True? - it will simplify those tests that currently have to mock uuid4 and will clearly display to the reader that generating a seed is optional.

Solution: Replaced it

Solution: Removed the requirement

z-bowen added 2 commits August 31, 2018 10:31

Merge branch 'master' into abstract_election_class

7de4226

Problem: ValidatorElection and MigrationElection need to inherit …

3d67565

…from a common `Election` class Solution: Factored the common logic out of `ValidatorElection` and moved it to `Election` parent class

z-bowen requested review from kansi and ldmberman August 31, 2018 11:59

Problem: Adding base58 as a requirement seems to break the build...

4a631ef

Solution: Reverting the changes

kansi reviewed Sep 3, 2018

View reviewed changes

ldmberman reviewed Sep 3, 2018

View reviewed changes

kansi reviewed Sep 3, 2018

View reviewed changes

ldmberman reviewed Sep 3, 2018

View reviewed changes

Problem: Responding to a suggestion for improved method naming

560a815

Solution: Refactored `get_result_by_election_id` to `get_election_result_by_id`

kansi reviewed Sep 3, 2018

View reviewed changes

Problem: No need to store different types of elections in their own t…

db45374

…ables Solution: Remove `DB_TABLE` property from `Election` class

ldmberman mentioned this pull request Sep 3, 2018

Problem: No MongoDB support for election ID index. #2507

Merged

kansi reviewed Sep 3, 2018

View reviewed changes

z-bowen added 11 commits September 4, 2018 10:55

Revert "Problem: No need to store different types of elections in the…

bd76b31

…ir own tables" This reverts commit db45374.

Problem: Missed a method in Bigchain class when updating the naming…

b468504

… for an election method Solution: Finished the refactoring

Problem: Need a table to store data for all elections

e7dae7d

Solution: Created the `elections` table with secondary_index `election_id`

Merge branch 'master' into abstract_election_class

fe8eb84

Problem: Election class needs to be updated to store concluded elec…

84a7cf7

…tions in the `elections` table Solution: Updated the class to use the new table

Problem: UpsertValidatorVote can be generalized to just be Vote

1e3caf8

Solution: Renamed, refactored and moved the `Vote` class to tie in with the more general `Election` base class

Problem: Error in docstring return signature

30a5b93

Solution: Fixed the docstring

Problem: Hardcoded reference to the VOTE_TYPE in Election base class

7d756bf

Solution: Pointed the reference to the class variable

Problem: Schema still refers to ValidatorElectionVote instead of `V…

331b52c

…ote` Solution: Renamed `TX_SCHEMA_VALIDATOR_ELECTION_VOTE` as `TX_SCHEMA_VOTE`

Problem: Election class variable ELECTION_TYPE is overly specific

0c64fbf

Solution: Renamed `ELECTION_TYPE` to `OPERATION`

Merge branch 'master' into abstract_election_class

b6a5150

kansi suggested changes Sep 5, 2018

View reviewed changes

Problem: Multiple small issues with style etc.

fdacca7

Solution: Addressed comments from PR

kansi reviewed Sep 6, 2018

View reviewed changes

bigchaindb/elections/vote.py Show resolved Hide resolved

kansi reviewed Sep 6, 2018

View reviewed changes

z-bowen added 3 commits September 6, 2018 17:16

Problem: Election class variable to VOTE_TYPE unnecessary

2eb32d9

Solution: Removed the variable and hardcoded everything to use the `Vote` class

Problem: Minor style issues with PR

38fcd4d

Solution: Addressing comments

Merge remote-tracking branch 'upstream/master' into abstract_election…

eae7182

…_class

kansi reviewed Sep 7, 2018

View reviewed changes

z-bowen added 2 commits September 7, 2018 15:29

Problem: Decided to no longer include the election_id when storing …

52caaa6

…a `validator_set` Solution: Removed the code to store the `election_id` and aligned the tests

Problem: election_id is not unique if two elections have the same p…

d696a51

…roperties Solution: Added a random `uuid4` seed to enforce uniqueness

kansi reviewed Sep 10, 2018

View reviewed changes

bigchaindb/elections/election.py Show resolved Hide resolved

kansi reviewed Sep 10, 2018

View reviewed changes

z-bowen added 2 commits September 10, 2018 10:47

Problem: Election.validate uses an inefficient query to check that …

06ea940

…the tx is in the db Solution: use `is_committed` instead of `get_transaction`

Merge branch 'master' into abstract_election_class

15b5f73

ldmberman reviewed Sep 10, 2018

View reviewed changes

Problem: boolean skip_id parameter is always called with True

e302a4b

Solution: Removed the parameter

kansi approved these changes Sep 10, 2018

View reviewed changes

ldmberman reviewed Sep 10, 2018

View reviewed changes

Problem: store_election_results parameter is still named `validator…

9a3c084

…_update` Solution: Renamed to `election`

ldmberman reviewed Sep 10, 2018

View reviewed changes

z-bowen added 2 commits September 10, 2018 17:16

Problem: Logging statement accidentally removed during merge

1899876

Solution: Replaced it

Problem: Election parameter seed marked as required unnecessarily

25a5620

Solution: Removed the requirement

ldmberman merged commit 0fe749d into bigchaindb:master Sep 11, 2018


		self.store_abci_chain(block['height'] + 1, new_chain_id, False)

		def store_election(self, height, election):



		@singledispatch
		def store_election_results(conn, validator_update):

Conversation

z-bowen commented Aug 31, 2018

Solution

Uh oh!

codecov-io commented Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kansi Sep 3, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kansi Sep 3, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kansi Sep 5, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Aug 31, 2018 •

edited

Loading

kansi Sep 3, 2018 •

edited

Loading

kansi Sep 3, 2018 •

edited

Loading

kansi Sep 5, 2018 •

edited

Loading

kansi Sep 7, 2018 •

edited

Loading

kansi Sep 10, 2018 •

edited

Loading