Problem: (BEP-21) no way to propose validator election by kansi · Pull Request #2392 · bigchaindb/bigchaindb

kansi · 2018-07-18T13:28:30Z

Solution: Integrate the new election spec specified in BEP-21

Implements one of the tasks in issue #2372

Solution: Integrate the new election spec specifed in BEP-21

Solution: Handle operation to class for invalid transactions

codecov-io · 2018-07-18T13:42:16Z

Codecov Report

Merging #2392 into master will increase coverage by 0.36%.
The diff coverage is 96.38%.

@@            Coverage Diff             @@
##           master    #2392      +/-   ##
==========================================
+ Coverage    86.9%   87.26%   +0.36%     
==========================================
  Files          38       39       +1     
  Lines        2168     2270     +102     
==========================================
+ Hits         1884     1981      +97     
- Misses        284      289       +5

ldmberman · 2018-07-23T13:12:50Z

bigchaindb/upsert_validator/validator_election.py

+                                               TX_SCHEMA_VERSION, __file__)
+
+
+class ValidatorElection(Transaction):


Looks like the unspent_outputs, inputs_valid, and get_asset_id methods are broken now.

These functions expect self.operation to be one of Transaction.CREATE, Transaction.TRANSFER, what is not the case for ValidatorElection.

ldmberman · 2018-07-23T13:14:54Z

bigchaindb/upsert_validator/validator_election.py

+class ValidatorElection(Transaction):
+
+    VALIDATOR_ELECTION = 'VALIDATOR_ELECTION'
+    # NOTE: this transaction class extends create so the operation inheritence is achieved


May be it is cleaner to override cls.create to raise an exception, what do you think? Or do you want to use ValidatorElection for creating election votes later?

May be it is cleaner to override cls.create to raise an exception, what do you think?

Cloud you please elaborate? What would be achieved if we override cls.create and raise an exception here?

Currently, there are three methods for instantiating an election transaction - ValidatorElection.create, ValidatorElection.generate, and the constructor. It is easier to read and use the code if we only keep one. The problem with create though is that it comes from the parent so raising an exception is a way to explain that this method is not for use. Ideally, we should have a base transaction class without create and transfer.

Ideally, we should have a base transaction class without create and transfer.

That is the target. In this PR I am trying to consolidate the methods which are required in the base Transaction class so other new transaction classes can be written by extending it.

ldmberman · 2018-07-23T13:16:13Z

bigchaindb/upsert_validator/validator_election.py

+
+    @classmethod
+    def from_dict(cls, tx):
+        cls.validate_id(tx)


Does it make sense to update the base from_dict instead?

I think we can improve this by keeping the first two validations and then calling super().from_dict()?

But why don't you add these two to the base class? Why do we want to validate this part only for election transactions?

ldmberman · 2018-07-23T14:20:48Z

bigchaindb/upsert_validator/validator_election.py

+
+        # Check whether the voters and their votes is same to that of the
+        # validators and their voting power in the network
+        return (current_topology.items() == voters.items())


Although the new Python guarantees the key order, I would compare dicts directly current_topology == voters, for simplicity.

ldmberman · 2018-07-23T14:26:01Z

bigchaindb/upsert_validator/validator_election.py

+
+        # NOTE: Proposer should be a single node
+        if len(self.inputs) != 1:
+            return False


Returning False is inconsistent with raising exceptions and is not documented.

ldmberman · 2018-07-23T14:38:17Z

bigchaindb/tendermint/lib.py

            raise DoubleSpend('tx "{}" spends inputs twice'.format(txid))
        elif transactions:
-            transaction = Transaction.from_db(self, transactions[0])
+            transaction = operation_class(transactions[0]).from_db(self, transactions[0])


To make the core transaction module independent from the plugin modules, we can do DI the following way:

class Transaction: type_registry = {} @classmethod def register_type(typ, clas): type_registry[typ] = clas @classmethod def from_whatever(tx): return cls(type_registry[tx['operation']])

Then, inside a_plugin.py one can do smth like:

from core import Transaction Transaction.register_type('election', ValidatorElection)

What do you think?

ldmberman · 2018-07-23T14:50:00Z

tests/upsert_validator/test_validator_election.py

+
+    b_mock.store_bulk_transactions([valid_election])
+
+    with pytest.raises(DuplicateTransaction):


Looks like these two with pytest.raises(DuplicateTransaction): are following the same code path.

Not quite, the previous exception will be generated because valid_election is a part of current_transactions list and the second exception would be generated because valid_election has been stored in MongoDB and should be read from there to raise the exception.

I think, you just need to call valid_election.validate(b_mock) in the second case. Otherwise you depend on the order of the checks.

oh yeah, my bad.

ldmberman · 2018-07-23T14:51:16Z

bigchaindb/upsert_validator/validator_election.py

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


Is there a particular reason for having both the constructor and generate?

ldmberman · 2018-07-23T15:21:21Z

bigchaindb/upsert_validator/validator_election.py

+            return False
+
+        # NOTE: Check if all validators have been assigned votes equal to their voting power
+        return self.is_same_topology(current_validators, self.outputs)


It would be helpful to also consider elections which add up more than 1/3 of the current total validator power to be invalid.

ldmberman · 2018-07-24T12:40:32Z

bigchaindb/upsert_validator/validator_election.py

+
+        # NOTE: Proposer should be a single node
+        if len(self.inputs) != 1:
+            raise TypeError('`tx_signers` must be a list instance or length one')


It makes sense to have a specifc exception in the case like this so that a caller can easily distinguish exceptions.

You mean a custom exception?

ldmberman · 2018-07-24T12:41:35Z

bigchaindb/upsert_validator/validator_election.py

+            raise TypeError('`tx_signers` must be a list instance or length one')
+
+        # NOTE: change more than 1/3 of the current power is not allowed
+        if self.asset['data']['power'] > (1/3)*sum(current_validators.values()):


Isn't it >= 1/3?

Solution: Election which allocate power more 1/3 of current total power should be considered invalid

Solution: Create registry for store transaction types and their corresponding classes

Solution: Import Transaction form models.py module

Solution: Override methods and raise execptions for code clarity

…election

kansi · 2018-07-25T15:24:16Z

bigchaindb/lib.py

                                        self.connection, *unspent_outputs)

-    def get_transaction(self, transaction_id, include_status=False):
+    def get_transaction(self, transaction_id, include_status=False, cls=Transaction):


No need to used the additional cls i.e. .from_dict implicitly chooses the correct class.

Solution: the `.from_dict` method implicitly detects the class so `cls` parameter can be removed

Solution: All errors should generate exceptions extended from ValidationError class

ldmberman · 2018-07-26T10:25:30Z

bigchaindb/upsert_validator/validator_election.py

+        # NOTE: Check if the proposer is a validator.
+        [election_initiator_node_pub_key] = self.inputs[0].owners_before
+        if election_initiator_node_pub_key not in current_validators.keys():
+            raise InvalidProposer('Public key is not a part of the validator set')


Can you add a test case for an invalid proposer?

ldmberman · 2018-07-26T10:29:40Z

bigchaindb/upsert_validator/validator_election.py

+        Args:
+            bigchain (BigchainDB): an instantiated bigchaindb.tendermint.BigchainDB object.
+
+        Returns:


The "Returns" section is outdated.

ldmberman · 2018-07-26T10:31:41Z

bigchaindb/upsert_validator/validator_election.py

+          alloacted according to the voting power of each validator node.
+
+        Args:
+            bigchain (BigchainDB): an instantiated bigchaindb.tendermint.BigchainDB object.


The location of BigchainDB has been changed.

ldmberman · 2018-07-26T10:36:02Z

bigchaindb/upsert_validator/validator_election.py

+    def validate(self, bigchain, current_transactions=[]):
+        """Validate election transaction
+        NOTE:
+        * A valid election is initiated by an existing validator.


What do you think about putting a reference to the BEP here? Then, a reader will get the complete context and we won't have to maintain the validation documentation in two places.

Solution: Add tests which raise InvalidPropser exception if the public key not a part of the validator set and raises MultipleInputsError if more than 1 validators `owners_before` in the election.

kansi · 2018-07-27T08:57:05Z

Removing @vrde from review as he is on holiday.

muawiakh · 2018-07-27T14:24:40Z

bigchaindb/common/exceptions.py

+    """Raised if the public key is not a part of the validator set"""
+
+
+class UnequalValidatorSet(ValidationError):


nitpick: InvalidValidatorSet makes more sense for this exception?

I thought about that name InvalidValidatorSet, but the code here tries to detect if the validator sets are the same or different. The only thing we wish to imply is their inequality and not validity.

muawiakh

lgtm, would be good to have an acceptance test later on when this feature is merged.

Problem: (BEP-21) no way to propose validator election

a8e5da3

Solution: Integrate the new election spec specifed in BEP-21

kansi requested review from ldmberman and vrde July 18, 2018 13:28

kansi mentioned this pull request Jul 18, 2018

Upsert validator feature incomplete #2372

Closed

14 tasks

Problem: Integration tests failing

53ba0a3

Solution: Handle operation to class for invalid transactions

ldmberman reviewed Jul 23, 2018

View reviewed changes

kansi force-pushed the 21/validator_election branch from 330a9a2 to 930d76b Compare July 24, 2018 12:38

ldmberman reviewed Jul 24, 2018

View reviewed changes

Problem: Power for proposed validator not validated

6a00c08

Solution: Election which allocate power more 1/3 of current total power should be considered invalid

kansi force-pushed the 21/validator_election branch 2 times, most recently from 4113407 to 6a00c08 Compare July 24, 2018 12:49

kansi added 4 commits July 25, 2018 14:51

Problem: Cannot detect transaction type and class dynamically

5426732

Solution: Create registry for store transaction types and their corresponding classes

Problem: Incorrent import for Transaction class

04d9aee

Solution: Import Transaction form models.py module

Problem: Invalid/redundant methods in side ValidatorElection

f8e0dee

Solution: Override methods and raise execptions for code clarity

Merge remote-tracking branch 'ssh_upstream/master' into 21/validator_…

610e4ba

…election

kansi commented Jul 25, 2018

View reviewed changes

kansi added 2 commits July 25, 2018 17:47

Problem: Unnecessary cls parameter for get_transaction

67d439c

Solution: the `.from_dict` method implicitly detects the class so `cls` parameter can be removed

Problem: Inconsitant error handling

bd88aca

Solution: All errors should generate exceptions extended from ValidationError class

ldmberman reviewed Jul 26, 2018

View reviewed changes

Problem: No tests for invalid proposer or multiple inputs

c6d7667

Solution: Add tests which raise InvalidPropser exception if the public key not a part of the validator set and raises MultipleInputsError if more than 1 validators `owners_before` in the election.

ldmberman approved these changes Jul 26, 2018

View reviewed changes

kansi requested review from muawiakh and removed request for vrde July 27, 2018 08:57

muawiakh reviewed Jul 27, 2018

View reviewed changes

muawiakh approved these changes Jul 27, 2018

View reviewed changes

ldmberman merged commit 7dcdefc into bigchaindb:master Jul 27, 2018

kansi mentioned this pull request Aug 21, 2018

Problem: upsert-validator feature is error prone #2314

Closed

4 tasks

		TX_SCHEMA_VERSION, __file__)


		class ValidatorElection(Transaction):


		b_mock.store_bulk_transactions([valid_election])

		with pytest.raises(DuplicateTransaction):

		"""Raised if the public key is not a part of the validator set"""


		class UnequalValidatorSet(ValidationError):

Conversation

kansi commented Jul 18, 2018 • edited by ttmc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 18, 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!

kansi Jul 24, 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!

kansi Jul 24, 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!

kansi Jul 24, 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!

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 commented Jul 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muawiakh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

kansi commented Jul 18, 2018 •

edited by ttmc

Loading

codecov-io commented Jul 18, 2018 •

edited

Loading

kansi Jul 24, 2018 •

edited

Loading

kansi Jul 24, 2018 •

edited

Loading

kansi Jul 24, 2018 •

edited

Loading