From 4a962163566df434f7f38aa13ab74542cec956fc Mon Sep 17 00:00:00 2001 From: Lev Berman Date: Wed, 19 Sep 2018 14:44:32 +0200 Subject: [PATCH] Problem: Looking for election block is inefficient. Solution: Record placed elections, update the records upon election conclusion. --- bigchaindb/backend/localmongodb/query.py | 10 ++-- bigchaindb/backend/localmongodb/schema.py | 3 +- bigchaindb/backend/query.py | 6 +-- bigchaindb/core.py | 9 ++-- bigchaindb/elections/election.py | 49 +++++++++++++------ bigchaindb/lib.py | 13 ++--- tests/backend/localmongodb/test_schema.py | 49 +------------------ tests/conftest.py | 9 ++-- tests/elections/test_election.py | 45 +++++++++-------- tests/upsert_validator/conftest.py | 6 +-- .../test_upsert_validator_vote.py | 8 +-- 11 files changed, 90 insertions(+), 117 deletions(-) diff --git a/bigchaindb/backend/localmongodb/query.py b/bigchaindb/backend/localmongodb/query.py index 6b0793ba..5c60eabc 100644 --- a/bigchaindb/backend/localmongodb/query.py +++ b/bigchaindb/backend/localmongodb/query.py @@ -282,11 +282,14 @@ def store_validator_set(conn, validators_update): @register_query(LocalMongoDBConnection) -def store_election_results(conn, election): +def store_election(conn, election_id, height, is_concluded): return conn.run( conn.collection('elections').replace_one( - {'election_id': election['election_id']}, - election, + {'election_id': election_id, + 'height': height}, + {'election_id': election_id, + 'height': height, + 'is_concluded': is_concluded}, upsert=True, ) ) @@ -315,6 +318,7 @@ def get_election(conn, election_id): cursor = conn.run( conn.collection('elections') .find(query, projection={'_id': False}) + .sort([('height', DESCENDING)]) ) return next(cursor, None) diff --git a/bigchaindb/backend/localmongodb/schema.py b/bigchaindb/backend/localmongodb/schema.py index 053cfc3d..157ff39d 100644 --- a/bigchaindb/backend/localmongodb/schema.py +++ b/bigchaindb/backend/localmongodb/schema.py @@ -45,7 +45,8 @@ INDEXES = { ('commit_id', dict(name='pre_commit_id', unique=True)), ], 'elections': [ - ('election_id', dict(name='election_id', unique=True)), + ([('height', DESCENDING), ('election_id', ASCENDING)], + dict(name='election_id_height', unique=True)), ], 'validators': [ ('height', dict(name='height', unique=True)), diff --git a/bigchaindb/backend/query.py b/bigchaindb/backend/query.py index d8f60320..d71412fe 100644 --- a/bigchaindb/backend/query.py +++ b/bigchaindb/backend/query.py @@ -352,8 +352,8 @@ def store_validator_set(conn, validator_update): @singledispatch -def store_election_results(conn, election): - """Store election results""" +def store_election(conn, election_id, height, is_concluded): + """Store election record""" raise NotImplementedError @@ -369,7 +369,7 @@ def get_validator_set(conn, height): @singledispatch def get_election(conn, election_id): - """Return a validator set change with the specified election_id + """Return the election record """ raise NotImplementedError diff --git a/bigchaindb/core.py b/bigchaindb/core.py index 8be3cdaf..49b004c5 100644 --- a/bigchaindb/core.py +++ b/bigchaindb/core.py @@ -219,10 +219,9 @@ class App(BaseApplication): else: self.block_txn_hash = block['app_hash'] - # Process all concluded elections in the current block and get any update to the validator set - update = Election.approved_elections(self.bigchaindb, - self.new_height, - self.block_transactions) + validator_update = Election.process_block(self.bigchaindb, + self.new_height, + self.block_transactions) # Store pre-commit state to recover in case there is a crash during `commit` pre_commit_state = PreCommitState(commit_id=PRE_COMMIT_ID, @@ -230,7 +229,7 @@ class App(BaseApplication): transactions=self.block_txn_ids) logger.debug('Updating PreCommitState: %s', self.new_height) self.bigchaindb.store_pre_commit_state(pre_commit_state._asdict()) - return ResponseEndBlock(validator_updates=update) + return ResponseEndBlock(validator_updates=validator_update) def commit(self): """Store the new height and along with block hash.""" diff --git a/bigchaindb/elections/election.py b/bigchaindb/elections/election.py index 322ab92a..e4e783b2 100644 --- a/bigchaindb/elections/election.py +++ b/bigchaindb/elections/election.py @@ -217,8 +217,8 @@ class Election(Transaction): return False def get_status(self, bigchain): - concluded = self.get_election(self.id, bigchain) - if concluded: + election = self.get_election(self.id, bigchain) + if election and election['is_concluded']: return self.CONCLUDED return self.INCONCLUSIVE if self.has_validator_set_changed(bigchain) else self.ONGOING @@ -230,20 +230,17 @@ class Election(Transaction): latest_change_height = latest_change['height'] - blocks = bigchain.get_block_containing_tx(self.id) - if not blocks: - return False - election_height = blocks[0] + election = self.get_election(self.id, bigchain) + if not election: + return True - return latest_change_height > election_height + return latest_change_height > election['height'] def get_election(self, election_id, bigchain): - result = bigchain.get_election(election_id) - return result + return bigchain.get_election(election_id) - @classmethod - def store_election_results(cls, bigchain, election, height): - bigchain.store_election_results(height, election) + def store(self, bigchain, height, is_concluded): + bigchain.store_election(self.id, height, is_concluded) def show_election(self, bigchain): data = self.asset['data'] @@ -258,9 +255,33 @@ class Election(Transaction): return response @classmethod - def approved_elections(cls, bigchain, new_height, txns): + def process_block(cls, bigchain, new_height, txns): + """Looks for election and vote transactions inside the block, records + and processes elections. + + Every election is recorded in the database. + + Every vote has a chance to conclude the corresponding election. When + an election is concluded, the corresponding database record is + marked as such. + + Elections and votes are processed in the order in which they + appear in the block. + + For every election concluded in the block, calls its `on_approval` + method. The returned value of the last `on_approval`, if any, + is a validator set update to be applied in one of the following blocks. + + `on_approval` methods are implemented by elections of particular type. + The method may contain side effects but should be idempotent. To account + for other concluded elections, if it requires so, the method should + rely on the database state. + """ elections = OrderedDict() for tx in txns: + if isinstance(tx, Election): + tx.store(bigchain, new_height, is_concluded=False) + if not isinstance(tx, Vote): continue election_id = tx.asset['id'] @@ -278,7 +299,7 @@ class Election(Transaction): continue validator_update = election.on_approval(bigchain, new_height) - election.store_election_results(bigchain, election, new_height) + election.store(bigchain, new_height, is_concluded=True) return [validator_update] if validator_update else [] diff --git a/bigchaindb/lib.py b/bigchaindb/lib.py index 2dadcb90..a23a69a1 100644 --- a/bigchaindb/lib.py +++ b/bigchaindb/lib.py @@ -436,8 +436,7 @@ class BigchainDB(object): return [] if result is None else result['validators'] def get_election(self, election_id): - result = backend.query.get_election(self.connection, election_id) - return result + return backend.query.get_election(self.connection, election_id) def store_pre_commit_state(self, state): return backend.query.store_pre_commit_state(self.connection, state) @@ -481,13 +480,9 @@ class BigchainDB(object): self.store_abci_chain(block['height'] + 1, new_chain_id, False) - def store_election_results(self, height, election): - """Store election results - :param height: the block height at which the election concluded - :param election: a concluded election - """ - return backend.query.store_election_results(self.connection, {'height': height, - 'election_id': election.id}) + def store_election(self, election_id, height, is_concluded): + return backend.query.store_election(self.connection, election_id, + height, is_concluded) Block = namedtuple('Block', ('app_hash', 'height', 'transactions')) diff --git a/tests/backend/localmongodb/test_schema.py b/tests/backend/localmongodb/test_schema.py index a96e7b63..c4f6669a 100644 --- a/tests/backend/localmongodb/test_schema.py +++ b/tests/backend/localmongodb/test_schema.py @@ -3,51 +3,6 @@ # Code is Apache-2.0 and docs are CC-BY-4.0 -def test_init_creates_db_tables_and_indexes(): - import bigchaindb - from bigchaindb import backend - from bigchaindb.backend.schema import init_database - - conn = backend.connect() - dbname = bigchaindb.config['database']['name'] - - # the db is set up by the fixture so we need to remove it - conn.conn.drop_database(dbname) - - init_database() - - collection_names = conn.conn[dbname].list_collection_names() - assert set(collection_names) == { - 'transactions', 'assets', 'metadata', 'blocks', 'utxos', 'pre_commit', - 'validators', 'elections', 'abci_chains', - } - - indexes = conn.conn[dbname]['assets'].index_information().keys() - assert set(indexes) == {'_id_', 'asset_id', 'text'} - - indexes = conn.conn[dbname]['transactions'].index_information().keys() - assert set(indexes) == { - '_id_', 'transaction_id', 'asset_id', 'outputs', 'inputs'} - - indexes = conn.conn[dbname]['blocks'].index_information().keys() - assert set(indexes) == {'_id_', 'height'} - - indexes = conn.conn[dbname]['utxos'].index_information().keys() - assert set(indexes) == {'_id_', 'utxo'} - - indexes = conn.conn[dbname]['pre_commit'].index_information().keys() - assert set(indexes) == {'_id_', 'pre_commit_id'} - - indexes = conn.conn[dbname]['validators'].index_information().keys() - assert set(indexes) == {'_id_', 'height'} - - indexes = conn.conn[dbname]['abci_chains'].index_information().keys() - assert set(indexes) == {'_id_', 'height', 'chain_id'} - - indexes = conn.conn[dbname]['elections'].index_information().keys() - assert set(indexes) == {'_id_', 'election_id'} - - def test_init_database_is_graceful_if_db_exists(): import bigchaindb from bigchaindb import backend @@ -102,8 +57,8 @@ def test_create_tables(): ('output_index', 1)] indexes = conn.conn[dbname]['elections'].index_information() - assert set(indexes.keys()) == {'_id_', 'election_id'} - assert indexes['election_id']['unique'] + assert set(indexes.keys()) == {'_id_', 'election_id_height'} + assert indexes['election_id_height']['unique'] indexes = conn.conn[dbname]['pre_commit'].index_information() assert set(indexes.keys()) == {'_id_', 'pre_commit_id'} diff --git a/tests/conftest.py b/tests/conftest.py index 90d54dea..b993dd0e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -717,12 +717,13 @@ def valid_upsert_validator_election_2(b_mock, node_key, new_validator): def ongoing_validator_election(b, valid_upsert_validator_election, ed25519_node_keys): validators = b.get_validators(height=1) genesis_validators = {'validators': validators, - 'height': 0, - 'election_id': None} + 'height': 0} query.store_validator_set(b.connection, genesis_validators) - b.store_bulk_transactions([valid_upsert_validator_election]) - block_1 = Block(app_hash='hash_1', height=1, transactions=[valid_upsert_validator_election.id]) + query.store_election(b.connection, valid_upsert_validator_election.id, 1, + is_concluded=False) + block_1 = Block(app_hash='hash_1', height=1, + transactions=[valid_upsert_validator_election.id]) b.store_block(block_1._asdict()) return valid_upsert_validator_election diff --git a/tests/elections/test_election.py b/tests/elections/test_election.py index 22326a17..2258e846 100644 --- a/tests/elections/test_election.py +++ b/tests/elections/test_election.py @@ -9,7 +9,7 @@ from bigchaindb.upsert_validator.validator_election import ValidatorElection @pytest.mark.bdb -def test_approved_elections_concludes_all_elections(b): +def test_process_block_concludes_all_elections(b): validators = generate_validators([1] * 4) b.store_validator_set(1, [v['storage'] for v in validators]) @@ -39,7 +39,7 @@ def test_approved_elections_concludes_all_elections(b): app_hash='')._asdict()) b.store_bulk_transactions(txs) - Election.approved_elections(b, 1, total_votes) + Election.process_block(b, 1, txs + total_votes) validators = b.get_validators() assert len(validators) == 5 @@ -54,12 +54,11 @@ def test_approved_elections_concludes_all_elections(b): } for tx in txs: - election = b.get_election(tx.id) - assert election + assert b.get_election(tx.id)['is_concluded'] @pytest.mark.bdb -def test_approved_elections_approves_only_one_validator_update(b): +def test_process_block_approves_only_one_validator_update(b): validators = generate_validators([1] * 4) b.store_validator_set(1, [v['storage'] for v in validators]) @@ -88,19 +87,19 @@ def test_approved_elections_approves_only_one_validator_update(b): app_hash='')._asdict()) b.store_bulk_transactions(txs) - Election.approved_elections(b, 1, total_votes) + Election.process_block(b, 1, txs + 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[0].id)['is_concluded'] + assert not b.get_election(txs[1].id)['is_concluded'] @pytest.mark.bdb -def test_approved_elections_approves_after_pending_validator_update(b): +def test_process_block_approves_after_pending_validator_update(b): validators = generate_validators([1] * 4) b.store_validator_set(1, [v['storage'] for v in validators]) @@ -138,16 +137,16 @@ def test_approved_elections_approves_after_pending_validator_update(b): app_hash='')._asdict()) b.store_bulk_transactions(txs) - Election.approved_elections(b, 1, total_votes) + Election.process_block(b, 1, txs + 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_election(txs[0].id)['is_concluded'] + assert not b.get_election(txs[1].id)['is_concluded'] + assert b.get_election(txs[2].id)['is_concluded'] assert b.get_latest_abci_chain() == {'height': 2, 'chain_id': 'chain-X-migrated-at-height-1', @@ -155,7 +154,7 @@ def test_approved_elections_approves_after_pending_validator_update(b): @pytest.mark.bdb -def test_approved_elections_does_not_approve_after_validator_update(b): +def test_process_block_does_not_approve_after_validator_update(b): validators = generate_validators([1] * 4) b.store_validator_set(1, [v['storage'] for v in validators]) @@ -175,7 +174,7 @@ def test_approved_elections_does_not_approve_after_validator_update(b): app_hash='')._asdict()) b.store_bulk_transactions(txs) - Election.approved_elections(b, 1, total_votes) + Election.process_block(b, 1, txs + total_votes) b.store_block(Block(height=2, transactions=[v.id for v in total_votes], @@ -189,16 +188,16 @@ def test_approved_elections_does_not_approve_after_validator_update(b): total_votes = votes b.store_abci_chain(1, 'chain-X') - Election.approved_elections(b, 2, total_votes) + Election.process_block(b, 2, txs + total_votes) - assert not b.get_election(election.id) + assert not b.get_election(election.id)['is_concluded'] 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): +def test_process_block_applies_only_one_migration(b): validators = generate_validators([1] * 4) b.store_validator_set(1, [v['storage'] for v in validators]) @@ -225,7 +224,7 @@ def test_approved_elections_applies_only_one_migration(b): app_hash='')._asdict()) b.store_bulk_transactions(txs) - Election.approved_elections(b, 1, total_votes) + Election.process_block(b, 1, txs + total_votes) chain = b.get_latest_abci_chain() assert chain assert chain == { @@ -234,9 +233,9 @@ def test_approved_elections_applies_only_one_migration(b): 'chain_id': 'chain-X-migrated-at-height-1', } - assert b.get_election(txs[0].id) - assert not b.get_election(txs[1].id) + assert b.get_election(txs[0].id)['is_concluded'] + assert not b.get_election(txs[1].id)['is_concluded'] -def test_approved_elections_gracefully_handles_empty_block(b): - Election.approved_elections(b, 1, []) +def test_process_block_gracefully_handles_empty_block(b): + Election.process_block(b, 1, []) diff --git a/tests/upsert_validator/conftest.py b/tests/upsert_validator/conftest.py index 58b5e6dd..c39b9ed3 100644 --- a/tests/upsert_validator/conftest.py +++ b/tests/upsert_validator/conftest.py @@ -28,10 +28,8 @@ def fixed_seed_election(b_mock, node_key, new_validator): @pytest.fixture def concluded_election(b, ongoing_validator_election, ed25519_node_keys): - election_result = {'height': 2, - 'election_id': ongoing_validator_election.id} - - query.store_election_results(b.connection, election_result) + query.store_election(b.connection, ongoing_validator_election.id, + 2, is_concluded=True) return ongoing_validator_election diff --git a/tests/upsert_validator/test_upsert_validator_vote.py b/tests/upsert_validator/test_upsert_validator_vote.py index 7c49bd77..eeac3e66 100644 --- a/tests/upsert_validator/test_upsert_validator_vote.py +++ b/tests/upsert_validator/test_upsert_validator_vote.py @@ -289,10 +289,10 @@ def test_get_validator_update(b, node_keys, node_key, ed25519_node_keys): assert not election.has_concluded(b, [tx_vote0, tx_vote1]) assert election.has_concluded(b, [tx_vote0, tx_vote1, tx_vote2]) - assert Election.approved_elections(b, 4, [tx_vote0]) == [] - assert Election.approved_elections(b, 4, [tx_vote0, tx_vote1]) == [] + assert Election.process_block(b, 4, [tx_vote0]) == [] + assert Election.process_block(b, 4, [tx_vote0, tx_vote1]) == [] - update = Election.approved_elections(b, 4, [tx_vote0, tx_vote1, tx_vote2]) + update = Election.process_block(b, 4, [tx_vote0, tx_vote1, tx_vote2]) assert len(update) == 1 update_public_key = codecs.encode(update[0].pub_key.data, 'base64').decode().rstrip('\n') assert update_public_key == public_key64 @@ -315,7 +315,7 @@ def test_get_validator_update(b, node_keys, node_key, ed25519_node_keys): b.store_bulk_transactions([tx_vote0, tx_vote1]) - update = Election.approved_elections(b, 9, [tx_vote2]) + update = Election.process_block(b, 9, [tx_vote2]) assert len(update) == 1 update_public_key = codecs.encode(update[0].pub_key.data, 'base64').decode().rstrip('\n') assert update_public_key == public_key64