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.

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

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().

Fixed

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.

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.

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

Done

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.

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

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.