Create migration election class #2535

z-bowen · 2018-09-12T11:22:17Z

Problem: We need a way to synchronize a halt to block production to manage migrations

Solution

Created the MigrationElection class

codecov-io · 2018-09-12T11:28:27Z

Codecov Report

Merging #2535 into master will increase coverage by 0.03%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master    #2535      +/-   ##
==========================================
+ Coverage   93.67%   93.71%   +0.03%     
==========================================
  Files          44       45       +1     
  Lines        2593     2625      +32     
==========================================
+ Hits         2429     2460      +31     
- Misses        164      165       +1

ldmberman · 2018-09-12T13:58:59Z

bigchaindb/commands/election_types.py

@@ -16,5 +16,14 @@
                'help': 'Path to the private key of the election initiator.'
            }
        }
+    },
+    'migration': {


This particular migration is connected to the ABCI (Tendermint) chain upgrades. We might introduce other migrations in the future. So I would call it tendermint-chain-migration or abci-chain-migration.

The keys from this dict directly translate into the CLI methods, as in election new tendermint-chain-migration. I'd prefer to keep them short if possible, for user convenience. How about chain-migration as a compromise?

The election transaction operation would also need to be changed to CHAIN_MIGRATION_ELECTION

Done. I also refactored the Class name etc.

bigchaindb/common/schema/transaction_migration_election_v2.0.yaml

ldmberman · 2018-09-12T14:20:18Z

docs/server/source/server-reference/bigchaindb-cli.md



 #### election approve

-Approve an election by voting for it. The proposal generated by executing `bigchaindb election new ...` can be approved by the validators using this command. The validator who is approving the proposal will spend all their votes i.e. if the validator has a network power of `10` then they will cast `10` votes for the proposal.


These are the changes from the other PR, you need to rebase.

ldmberman · 2018-09-12T14:26:40Z

tests/upsert_validator/test_upsert_validator_vote.py

@@ -243,15 +243,15 @@ def test_upsert_validator(b, node_key, node_keys, ed25519_node_keys):
                                          new_validator, None).sign([node_key.private_key])
    code, message = b.write_transaction(election, 'broadcast_tx_commit')
    assert code == 202
-    time.sleep(3)
+    time.sleep(1)


Why do you have a sleep here? The commit mode ensures the data is persisted when you get a response.

sleeps are inefficient because you always end up waiting for some extra time. If you instead continuously poll the resource in question and fail after a certain timeout, the test only wastes extra time in the unsuccessful case.

I tried pulling the sleep entirely, and it broke the test. There must be some lag before the tx shows up in the database...

Yeah, we need to investigate.

It fails because the test calls Tendermint, not BigchainDB (b.endpoint points to Tendermint's RPC API). To fix it, fetch validators via b.get_validators().

bigchaindb/commands/bigchaindb.py

ldmberman · 2018-09-13T12:56:21Z

bigchaindb/core.py

-                                                             self.new_height,
-                                                             self.block_transactions)
+        # Check the current block to see if any elections have concluded.
+        concluded_elections = Election.approved_elections(self.bigchaindb,


We should directly return the validator set update from approved_elections since preparing such an update based on the given block is its concern.

I'm trying to avoid logic specific to a given election type in that method. My goal here is to have one pass over the block txs where I check for all concluded elections, and then let end_block do what it needs to with those results. If I have approved_elections return a validator set for an approved ValidatorElection, then I need to add custom logic to handle that election class differently than the others.

ldmberman · 2018-09-13T12:58:52Z

bigchaindb/elections/election.py

-                return cls.on_approval(bigchain, election, new_height)
-        return None
+                # And keep the transaction filed under the election_type
+                elections[election_operation] = election.on_approval(bigchain, election, new_height)


We need to be able to foresee if a particular election updates the validator set and only persist (conclude) one of those if there are more than one. Elections that do not update the validator set are concluded too.

This is roughly how the conclusion can look like:

votes_by_election = defaultdict(list) for txn in txns: if not isinstance(txn, Vote): continue election_id = txn.asset['id'] votes_by_election[election_id].append(tx) updated_validator_set = False # only one validator set update is allowed per block for election_id, votes in votes_by_election: election = Election.from_dict(bigchain.get_transaction(election_id)) if not election.has_concluded(bigchain, election_id, votes, new_height): continue if election.get_validator_set() != bigchain.get_validators(): if updated_validator_set: continue updated_validator_set = True election.on_approval(bigchain, new_height)

What do you think?

I wanted to talk to you about this tomorrow.

The logic as I have it now allows for only one concluded election of a given type per block. This is desirable for ValidatorElections, and doesn't really matter for MigrationElections, but what do we want the generalized behavior to be?

Also, I think we can't easily avoid type-checking the elections. Election.get_validators() just gets the validator set from bigchain. The only place a ValidatorElection stores the new validator set is in its asset data. So it's probably easiest just to check the OPERATION or the class .__name__.

I am not really convinced by this new conclusion logic. Let's try to discuss this in a quick meeting?

We have no reason to have a one per block restriction when it's not about the validator set update.

A way to set elections that update validator set apart is to introduce a generic method that reports the new validator set. It can return an empty list by default whereas ValidatorElection will infer it from the asset.

ldmberman · 2018-09-14T10:34:03Z

bigchaindb/elections/election.py

@@ -35,7 +35,7 @@ class Election(Transaction):
    ELECTION_THRESHOLD = 2 / 3

    @classmethod
-    def get_validator_change(cls, bigchain, height=None):
+    def get_validator_change(cls, bigchain):
        """Return the latest change to the validator set


It's a good time to update this docstring:

there is no election_id in the collection anymore

it does not return the latest change but the change applied at the latest block height

…llow for upgrades across breaking changes Solution: Created `MigrationElection`

Solution: Adding them here

Solution: Updated the docs

Solution: Updated the definition of `election` to include the new `migration` type

Solution: - Removed the `bdb` flag from `test_get_divisble_transactions_returns_500` since it causes Tendermint to crash - Reduced the sleep cycles from 3s to 1s on `test_upsert_validator`

…ions Solution: Added it

…d operations Solution: Added it

Solution: Added an `abci` test for this method

…here is only one type of election (so we can't conclude an `upsert-validator` and a `migration` at the same height) Solution: Re-engineered the code in `Elections` that checks for `approved_elections` to check all election types simultaneously, then return concluded elections sorted by election type

Solution: - wrote a test for valid elections - refactored some fixtures to make it easier to share between `migration` and `upsert-validator` tests

Solution: Factored out the duplicate code

…torElection` and `MigrationElection` works when both election types conclude in the same block Solution: Wrote some integration tests

…ill break `Election.get_status` Solution: Reworked `get_validator_change` to look at only the latest block height or less

…lidator set, multiple other elections, per block Solution: - created a boolean flag to track which elections update the validator set - implemented a function to compute the updated validator set - reworked `approved_electios` to use the new logic - updated tests

Solution: Updated it

Solution: Fixed the test so it no longer needs `sleep`

…n types Solution: Renamed the class `ChainMigrationElection`

kansi · 2018-09-17T08:09:13Z

bigchaindb/elections/election.py

+        return self.CHANGES_VALIDATOR_SET
+
+    def get_validator_set_change(self, bigchain, new_height):
+        if self.makes_validator_set_change():


This if seems unnecessary i.e. looking at the logic from approved_elections this function will only be called if makes_validator_set_change() returns True?

The idea is that get_validator_set_change is a generic function, that is safe to call on any approved election. If the election has CHANGES_VALIDATOR_SET == False, then nothing happens. If the election has CHANGES_VALIDATOR_SET == True, then we execute the custom code to change the validator set.

The reason I built it this way is so that I can set the base Election class with CHANGES_VALIDATOR_SET == True and have the change_validator_set function raise a NotImplementedError. This addressed the concern Lev raised that someone could extend the base class and not understand how validator updates work. Anyone who makes a custom election class needs to either explicitly set CHANGES_VALIDATOR_SET to False, or deal with implementing the change_validator_set function, so in either case, they can't ignore the validator set issue.

@z-bowen such an interface might be helpful indeed but one does not expect the function named get_validator_set_change to update the database. What about doing the same before calling on_approval?

if self.CHANGES_VALIDATOR_SET: self.change_validator_set() election.on_approval(...)

I agree but it would be much easy to understand if the change to the validator set or any state mutation is done in on_approval method and nowhere else.

Yeah, that's a tradeoff. I am fine with doing everything in on_approval too.

kansi · 2018-09-17T09:26:35Z

bigchaindb/elections/election.py

+        for election_id, votes in elections.items():
+            election = bigchain.get_transaction(election_id)
+
+            if not election.has_concluded(bigchain, election_id, votes, new_height):


has_concluded also calls bigchain.get_transaction(election_id). It would be good to avoid that call.

ldmberman · 2018-09-17T09:35:57Z

bigchaindb/elections/election.py

+
+        return validator_set_change
+
+    def makes_validator_set_change(self):


There is no need to wrap the field in a method. We can make it a @property in the future if we need to transform it on read.

Solution: minor refactoring

tests/elections/test_election.py

kansi

@ldmberman suggested to merge this PR and address the comments in a separate PR which will also include the remaining necessary commands for migration. I agree with this idea so approving.

z-bowen requested review from kansi and ldmberman September 12, 2018 11:22

ldmberman reviewed Sep 12, 2018

View reviewed changes

bigchaindb/common/schema/transaction_migration_election_v2.0.yaml Outdated Show resolved Hide resolved

ldmberman reviewed Sep 12, 2018

View reviewed changes

This was referenced Sep 12, 2018

Implement $ bigchaindb election approve #2467

Closed

Implement bigchaindb migrate-tendermint new #2465

Closed

kansi reviewed Sep 13, 2018

View reviewed changes

bigchaindb/commands/bigchaindb.py Show resolved Hide resolved

ldmberman reviewed Sep 13, 2018

View reviewed changes

ldmberman reviewed Sep 14, 2018

View reviewed changes

z-bowen added 17 commits September 14, 2018 17:38

Problem: We need a way to synchronize a halt to block production to a…

f76726c

…llow for upgrades across breaking changes Solution: Created `MigrationElection`

Problem: Missed some files in my last commit

cb9730b

Solution: Adding them here

Problem: Need documentation for migration elections

023fa62

Solution: Updated the docs

Problem: MigrationElection needs CLI methods

bac26c4

Solution: Updated the definition of `election` to include the new `migration` type

Problem: abci tests are slow and unreliable

35191f0

Solution: - Removed the `bdb` flag from `test_get_divisble_transactions_returns_500` since it causes Tendermint to crash - Reduced the sleep cycles from 3s to 1s on `test_upsert_validator`

Problem: MigrationElection needed to be added to the allowed operat…

4493cad

…ions Solution: Added it

Problem: MigrationElection needed to be added to the list of allowe…

3f81e7e

…d operations Solution: Added it

Problem: Need an integration test for run_election_new_migration

af36843

Solution: Added an `abci` test for this method

Problem: Need tests for MigrationElection

c74af72

Solution: - wrote a test for valid elections - refactored some fixtures to make it easier to share between `migration` and `upsert-validator` tests

Problem: Code duplication between run_election_new_ methods

9942867

Solution: Factored out the duplicate code

Problem: Need tests to verify that the unified conclusion for `Valida…

cc0ce0e

…torElection` and `MigrationElection` works when both election types conclude in the same block Solution: Wrote some integration tests

Problem: Tendermint change to store validator changes at height h+2 w…

f2f045c

…ill break `Election.get_status` Solution: Reworked `get_validator_change` to look at only the latest block height or less

Problem: Docstring for get_vallidator_change needs to be updated

84754f6

Solution: Updated it

Problem: sleep in test_upsert_validator causing slow test execution

c5062a0

Solution: Fixed the test so it no longer needs `sleep`

Problem: MigrationElection naming may conflict with future migratio…

e599914

…n types Solution: Renamed the class `ChainMigrationElection`

z-bowen force-pushed the create-migration-election-class branch from 8fc59b6 to e599914 Compare September 14, 2018 15:43

ttmc added this to To do in BigchainDB 2.0 TODO via automation Sep 15, 2018

ttmc moved this from To do to In progress in BigchainDB 2.0 TODO Sep 15, 2018