From 3414093578211eb758d72187e6c9cabed771307b Mon Sep 17 00:00:00 2001 From: Lev Berman Date: Tue, 18 Sep 2018 16:37:03 +0200 Subject: [PATCH] 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 | 91 +++++++------- .../migrations/chain_migration_election.py | 4 +- .../upsert_validator/validator_election.py | 28 +++-- tests/elections/test_election.py | 113 ++++++++++++++++-- 4 files changed, 171 insertions(+), 65 deletions(-) diff --git a/bigchaindb/elections/election.py b/bigchaindb/elections/election.py index 40861a52..322ab92a 100644 --- a/bigchaindb/elections/election.py +++ b/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': } """ - height = bigchain.get_latest_block()['height'] - return bigchain.get_validator_change(height) + latest_block = bigchain.get_latest_block() + 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 - is exactly equal to the validator set encoded in the election outputs. + * Elections can only be concluded if the validator set has not changed + 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): - total_votes = sum(current_validators.values()) - if (votes_committed < (2/3) * total_votes) and \ - (votes_committed + votes_current >= (2/3)*total_votes): - return True + 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) - latest_change_height = latest_change['height'] - election_height = bigchain.get_block_containing_tx(self.id)[0] + return self.INCONCLUSIVE if self.has_validator_set_changed(bigchain) else self.ONGOING - if latest_change_height >= election_height: - return self.INCONCLUSIVE - else: - return self.ONGOING + def has_validator_set_changed(self, bigchain): + latest_change = self.get_validator_change(bigchain) + if latest_change is None: + 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_set_change = [] + validator_update = None 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(): - 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) + validator_update = election.on_approval(bigchain, 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): - 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): + def on_approval(self, bigchain, new_height): raise NotImplementedError diff --git a/bigchaindb/migrations/chain_migration_election.py b/bigchaindb/migrations/chain_migration_election.py index 8e80b979..129e6684 100644 --- a/bigchaindb/migrations/chain_migration_election.py +++ b/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(cls, bigchain, election, new_height): + def on_approval(self, bigchain, *args, **kwargs): bigchain.migrate_abci_chain() diff --git a/bigchaindb/upsert_validator/validator_election.py b/bigchaindb/upsert_validator/validator_election.py index 856b31a1..f39f78b2 100644 --- a/bigchaindb/upsert_validator/validator_election.py +++ b/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): - # The new validator set comes into effect from height = new_height+1 - # (upcoming changes to Tendermint will change this to height = new_height+2) + def has_concluded(self, bigchain, *args, **kwargs): + latest_block = bigchain.get_latest_block() + 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] - bigchain.store_validator_set(new_height+1, updated_validator_set) - return encode_validator(self.asset['data']) + updated_validator_set = [v for v in updated_validator_set + if v['voting_power'] > 0] - @classmethod - def on_approval(cls, bigchain, election, new_height): - pass + # 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']) diff --git a/tests/elections/test_election.py b/tests/elections/test_election.py index 0aed1816..22326a17 100644 --- a/tests/elections/test_election.py +++ b/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)