Separate pending and effective validator updates.

- Pending validator updates do not prevent elections from concluding.
- ValidatorElection overrides has_conclude to not conclude when there is a pending update for the matching height.
- Effective validator updates deem past elections inconclusive.

bigchaindb/elections/election.py

@@ -1,7 +1,7 @@
 # Copyright BigchainDB GmbH and BigchainDB contributors
 # SPDX-License-Identifier: (Apache-2.0 AND CC-BY-4.0)
 # Code is Apache-2.0 and docs are CC-BY-4.0
-from collections import defaultdict
+from collections import OrderedDict
 
 import base58
 from uuid import uuid4
@@ -22,9 +22,13 @@ from bigchaindb.common.schema import (_validate_schema,
 
 
 class Election(Transaction):
+    """Represents election transactions.
+
+       To implement a custom election, create a class deriving from this one
+       with OPERATION set to the election operation, ALLOWED_OPERATIONS
+       set to (OPERATION,), CREATE set to OPERATION.
+    """
 
-    # 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,)
     OPERATION = None
     # Custom validation schema
     TX_SCHEMA_CUSTOM = None
@@ -34,7 +38,6 @@ class Election(Transaction):
     INCONCLUSIVE = 'inconclusive'
     # Vote ratio to approve an election
     ELECTION_THRESHOLD = 2 / 3
-    CHANGES_VALIDATOR_SET = True
 
     @classmethod
     def get_validator_change(cls, bigchain):
@@ -45,8 +48,10 @@ class Election(Transaction):
             'validators': <validator_set>
         }
         """
-        height = bigchain.get_latest_block()['height']
+        latest_block = bigchain.get_latest_block()
-        return bigchain.get_validator_change(height)
+        if latest_block is None:
+            return None
+        return bigchain.get_validator_change(latest_block['height'])
 
     @classmethod
     def get_validators(cls, bigchain, height=None):
@@ -186,11 +191,11 @@ class Election(Transaction):
                                                                   election_pk))
         return self.count_votes(election_pk, txns, dict.get)
 
-    def has_concluded(self, bigchain, current_votes=[], height=None):
+    def has_concluded(self, bigchain, current_votes=[]):
         """Check if the election can be concluded or not.
 
-        * Elections can only be concluded if the current validator set
+        * Elections can only be concluded if the validator set has not changed
-          is exactly equal to the validator set encoded in the election outputs.
+          since the election was initiated.
         * Elections can be concluded only if the current votes form a supermajority.
 
         Custom elections may override this function and introduce additional checks.
@@ -199,13 +204,16 @@ class Election(Transaction):
         election_pk = self.to_public_key(self.id)
         votes_committed = self.get_commited_votes(bigchain, election_pk)
         votes_current = self.count_votes(election_pk, current_votes)
-        current_validators = self.get_validators(bigchain, height)
 
-        if self.is_same_topology(current_validators, self.outputs):
+        if self.has_validator_set_changed(bigchain):
+            return False
+
+        current_validators = self.get_validators(bigchain)
         total_votes = sum(current_validators.values())
         if (votes_committed < (2/3) * total_votes) and \
                 (votes_committed + votes_current >= (2/3)*total_votes):
             return True
+
         return False
 
     def get_status(self, bigchain):
@@ -213,14 +221,21 @@ class Election(Transaction):
         if concluded:
             return self.CONCLUDED
 
-        latest_change = self.get_validator_change(bigchain)
+        return self.INCONCLUSIVE if self.has_validator_set_changed(bigchain) else self.ONGOING
-        latest_change_height = latest_change['height']
-        election_height = bigchain.get_block_containing_tx(self.id)[0]
 
-        if latest_change_height >= election_height:
+    def has_validator_set_changed(self, bigchain):
-            return self.INCONCLUSIVE
+        latest_change = self.get_validator_change(bigchain)
-        else:
+        if latest_change is None:
-            return self.ONGOING
+            return False
+
+        latest_change_height = latest_change['height']
+
+        blocks = bigchain.get_block_containing_tx(self.id)
+        if not blocks:
+            return False
+        election_height = blocks[0]
+
+        return latest_change_height > election_height
 
     def get_election(self, election_id, bigchain):
         result = bigchain.get_election(election_id)
@@ -244,44 +259,28 @@ class Election(Transaction):
 
     @classmethod
     def approved_elections(cls, bigchain, new_height, txns):
-        elections = defaultdict(list)
+        elections = OrderedDict()
         for tx in txns:
             if not isinstance(tx, Vote):
                 continue
             election_id = tx.asset['id']
+            if election_id not in elections:
+                elections[election_id] = []
             elections[election_id].append(tx)
 
-        validator_set_updated = False
+        validator_update = None
-        validator_set_change = []
         for election_id, votes in elections.items():
             election = bigchain.get_transaction(election_id)
             if election is None:
                 continue
 
-            if not election.has_concluded(bigchain, votes, new_height):
+            if not election.has_concluded(bigchain, votes):
                 continue
 
-            if election.makes_validator_set_change():
+            validator_update = election.on_approval(bigchain, new_height)
-                if validator_set_updated:
-                    continue
-                validator_set_change.append(election.get_validator_set_change(bigchain, new_height))
-                validator_set_updated = True
-
-            election.on_approval(bigchain, election, new_height)
             election.store_election_results(bigchain, election, new_height)
 
-        return validator_set_change
+        return [validator_update] if validator_update else []
 
-    def makes_validator_set_change(self):
+    def on_approval(self, bigchain, new_height):
-        return self.CHANGES_VALIDATOR_SET
-
-    def get_validator_set_change(self, bigchain, new_height):
-        if self.makes_validator_set_change():
-            return self.change_validator_set(bigchain, new_height)
-
-    def change_validator_set(self, bigchain, new_height):
-        raise NotImplementedError
-
-    @classmethod
-    def on_approval(cls, bigchain, election, new_height):
         raise NotImplementedError

bigchaindb/migrations/chain_migration_election.py

@@ -8,7 +8,6 @@ class ChainMigrationElection(Election):
     CREATE = OPERATION
     ALLOWED_OPERATIONS = (OPERATION,)
     TX_SCHEMA_CUSTOM = TX_SCHEMA_CHAIN_MIGRATION_ELECTION
-    CHANGES_VALIDATOR_SET = False
 
     def has_concluded(self, bigchaindb, *args, **kwargs):
         chain = bigchaindb.get_latest_abci_chain()
@@ -19,6 +18,5 @@ class ChainMigrationElection(Election):
 
         return super().has_concluded(bigchaindb, *args, **kwargs)
 
-    @classmethod
+    def on_approval(self, bigchain, *args, **kwargs):
-    def on_approval(cls, bigchain, election, new_height):
         bigchain.migrate_abci_chain()

bigchaindb/upsert_validator/validator_election.py

@@ -36,18 +36,28 @@ class ValidatorElection(Election):
         super(ValidatorElection, cls).validate_schema(tx)
         validate_asset_public_key(tx['asset']['data']['public_key'])
 
-    def change_validator_set(self, bigchain, new_height):
+    def has_concluded(self, bigchain, *args, **kwargs):
-        # The new validator set comes into effect from height = new_height+1
+        latest_block = bigchain.get_latest_block()
-        # (upcoming changes to Tendermint will change this to height = new_height+2)
+        if latest_block is not None:
+            latest_block_height = latest_block['height']
+            latest_validator_change = bigchain.get_validator_change()['height']
+
+            # TODO change to `latest_block_height + 2` when upgrading to Tendermint 0.24.0.
+            if latest_validator_change == latest_block_height + 1:
+                # do not conclude the election if there is a change assigned already
+                return False
+
+        return super().has_concluded(bigchain, *args, **kwargs)
+
+    def on_approval(self, bigchain, new_height):
         validator_updates = [self.asset['data']]
         curr_validator_set = bigchain.get_validators(new_height)
         updated_validator_set = new_validator_set(curr_validator_set,
                                                   validator_updates)
 
-        updated_validator_set = [v for v in updated_validator_set if v['voting_power'] > 0]
+        updated_validator_set = [v for v in updated_validator_set
+                                 if v['voting_power'] > 0]
+
+        # TODO change to `new_height + 2` when upgrading to Tendermint 0.24.0.
         bigchain.store_validator_set(new_height + 1, updated_validator_set)
         return encode_validator(self.asset['data'])
-
-    @classmethod
-    def on_approval(cls, bigchain, election, new_height):
-        pass

tests/elections/test_election.py

@@ -17,18 +17,19 @@ def test_approved_elections_concludes_all_elections(b):
 
     public_key = validators[0]['public_key']
     private_key = validators[0]['private_key']
-    election, votes = generate_election(b,
-                                        ValidatorElection,
-                                        public_key, private_key,
-                                        new_validator['election'])
-    txs = [election]
-    total_votes = votes
 
     election, votes = generate_election(b,
                                         ChainMigrationElection,
                                         public_key, private_key,
                                         {})
 
+    txs = [election]
+    total_votes = votes
+
+    election, votes = generate_election(b,
+                                        ValidatorElection,
+                                        public_key, private_key,
+                                        new_validator['election'])
     txs += [election]
     total_votes += votes
 
@@ -58,7 +59,7 @@ def test_approved_elections_concludes_all_elections(b):
 
 
 @pytest.mark.bdb
-def test_approved_elections_applies_only_one_validator_update(b):
+def test_approved_elections_approves_only_one_validator_update(b):
     validators = generate_validators([1] * 4)
     b.store_validator_set(1, [v['storage'] for v in validators])
 
@@ -98,6 +99,104 @@ def test_approved_elections_applies_only_one_validator_update(b):
     assert not b.get_election(txs[1].id)
 
 
+@pytest.mark.bdb
+def test_approved_elections_approves_after_pending_validator_update(b):
+    validators = generate_validators([1] * 4)
+    b.store_validator_set(1, [v['storage'] for v in validators])
+
+    new_validator = generate_validators([1])[0]
+
+    public_key = validators[0]['public_key']
+    private_key = validators[0]['private_key']
+    election, votes = generate_election(b,
+                                        ValidatorElection,
+                                        public_key, private_key,
+                                        new_validator['election'])
+    txs = [election]
+    total_votes = votes
+
+    another_validator = generate_validators([1])[0]
+
+    election, votes = generate_election(b,
+                                        ValidatorElection,
+                                        public_key, private_key,
+                                        another_validator['election'])
+    txs += [election]
+    total_votes += votes
+
+    election, votes = generate_election(b,
+                                        ChainMigrationElection,
+                                        public_key, private_key,
+                                        {})
+
+    txs += [election]
+    total_votes += votes
+
+    b.store_abci_chain(1, 'chain-X')
+    b.store_block(Block(height=1,
+                        transactions=[tx.id for tx in txs],
+                        app_hash='')._asdict())
+    b.store_bulk_transactions(txs)
+
+    Election.approved_elections(b, 1, total_votes)
+
+    validators = b.get_validators()
+    assert len(validators) == 5
+    assert new_validator['storage'] in validators
+    assert another_validator['storage'] not in validators
+
+    assert b.get_election(txs[0].id)
+    assert not b.get_election(txs[1].id)
+    assert b.get_election(txs[2].id)
+
+    assert b.get_latest_abci_chain() == {'height': 2,
+                                         'chain_id': 'chain-X-migrated-at-height-1',
+                                         'is_synced': False}
+
+
+@pytest.mark.bdb
+def test_approved_elections_does_not_approve_after_validator_update(b):
+    validators = generate_validators([1] * 4)
+    b.store_validator_set(1, [v['storage'] for v in validators])
+
+    new_validator = generate_validators([1])[0]
+
+    public_key = validators[0]['public_key']
+    private_key = validators[0]['private_key']
+    election, votes = generate_election(b,
+                                        ValidatorElection,
+                                        public_key, private_key,
+                                        new_validator['election'])
+    txs = [election]
+    total_votes = votes
+
+    b.store_block(Block(height=1,
+                        transactions=[tx.id for tx in txs],
+                        app_hash='')._asdict())
+    b.store_bulk_transactions(txs)
+
+    Election.approved_elections(b, 1, total_votes)
+
+    b.store_block(Block(height=2,
+                        transactions=[v.id for v in total_votes],
+                        app_hash='')._asdict())
+
+    election, votes = generate_election(b,
+                                        ChainMigrationElection,
+                                        public_key, private_key,
+                                        {})
+    txs = [election]
+    total_votes = votes
+
+    b.store_abci_chain(1, 'chain-X')
+    Election.approved_elections(b, 2, total_votes)
+
+    assert not b.get_election(election.id)
+    assert b.get_latest_abci_chain() == {'height': 1,
+                                         'chain_id': 'chain-X',
+                                         'is_synced': True}
+
+
 @pytest.mark.bdb
 def test_approved_elections_applies_only_one_migration(b):
     validators = generate_validators([1] * 4)