Implement validator set changes and key initialization.

[afck]

With hbbft, validators will have three secrets:

• Their private account key/engine signer key, using ECDSA on the secp256k1 curve.
• A secret BLS key used by SyncKeyGen for encryption. (Or should we get rid of that?)
• A secret BLS key share used for threshold encryption by HoneyBadger and threshold signatures in BinaryAgreement's cryptographic common coin.

HoneyBadger requires a NetworkInfo for initialization, which needs

• the node's secret BLS key,
• the node's secret BLS key share,
• its ID (the engine signer address),
• the PublicKeySet that defines all validators' public key shares, and
• the map that assigns to each validator ID a public BLS key.

The non-threshold stuff (ID and BLS keys) doesn't need to change, but the key shares need to be newly generated every time the validator set changes. We also need to provide means to initialize everything at the genesis block.

The engine will require that the HBBFT POSDAO or compatible contracts are deployed in genesis.

Validator Set Changes

The engine must call BlockRewardHBBFT.reward after every block, not only to distribute block rewards but also because that automatically triggers the random selection of a new set of validators in each epoch. (See Aura.)

For randomization to work, the engine must also call RandomHBBFT.storeRandom in every block, with the newly generated random number (xoring random numbers from all the contributions).

Once a new validator set has been selected, ValidatorSetBase.emitInitiateChangeCallable will return true, and at that point, the engine must call ValidatorSetBase.emitInitiateChange—this needs to be a transaction and not a system call, since system calls cannot emit events. In Aura, this currently is signed by the block producer, but with hbbft, all validators must produce identical transactions. We need to figure out how to do that…
We could also try to not rely on the InitiateChange event and just call a function that returns the pending validator set: If that's different from the current one, we know that a change is in progress.

When the validator set contract has determined the next validator set (InitiateChange event or something else…), the hbbft engine needs to prepare for that change:

The engine needs to publish and collect all the future validator's public BLS keys (unless we remove that requirement), and initialize a new SyncKeyGen instance. A node which is not among the future validators can use SecretKey::default() as its "secret key" here, as it is not used. In the validators-elect, the constructor will return a Part message: The engine must sign and send a transaction calling KeyGenHistory.writePart with the serialized message.

While key generation is ongoing, all Part and Ack messages in the contract must be passed into SyncKeyGen, and any returned Ack messages turned into transactions using KeyGenHistory.writeAck.

After each block, we check whether SyncKeyGen::is_ready. When it is, we initialize a new HoneyBadger with SyncKeyGen::into_network_info. Any further batches that have been returned by the old instance are ignored: the next block will already be produced by the new instance, and the new set of validators.

In the first block produced by a new validator set, the system calls ValidatorSetBase.finalizeChange. (See Aura.)

Initialization

The initial validator set needs to learn about its secret BLS keys and key shares, too. And every node needs to know the public key set and the map of public BLS keys (if needed).

• The secret BLS keys (if needed) will probably be an additional configuration option in Parity.
• The public BLS keys however should be part of the initialization of ValidatorSetHBBFT, so that all nodes can read them from the genesis block.
• The public key set and secret BLS key shares could be part of the initialization of KeyGenHistory, so that immediately after the genesis block it looks as if "key generation is complete". Alternatively, these, too, could be separate configuration and chain spec entries.

It would probably be most convenient to just pass a list of Part and Ack messages into the KeyGenHistory constructor, so the the keys can be read from it just like after an actual on-chain key generation.

[varasev]

The engine must call BlockRewardHBBFT.reward after every block

Is it possible to call it by SYSTEM_ADDRESS when block is closing like in AuRa?

For randomization to work, the engine must also call RandomHBBFT.storeRandom

Will it also be called by SYSTEM_ADDRESS? Is it possible in HBBFT?

at that point, the engine must call ValidatorSetBase.emitInitiateChange
We could also try to not rely on the InitiateChange event and just call a function that returns the pending validator set: If that's different from the current one, we know that a change is in progress.

I think, for HBBFT it's better not to use the InitiateChange event and use contract's getter which would return the current validator set. Otherwise, we would have to care about transaction queue like in AuRa for the emitInitiateChange.

In the first block produced by a new validator set, the system calls ValidatorSetBase.finalizeChange

Is it possible to call it by SYSTEM_ADDRESS like in AuRa?

When I ask about SYSTEM_ADDRESS (so-called system transactions), I wonder if it is possible at all in HBBFT because I remember that there were some problems in AuRa with those.

The public BLS keys however should be part of the initialization of ValidatorSetHBBFT, so that all nodes can read them from the genesis block.

Do you mean here something like that? https://github.com/poanetwork/posdao-contracts/blob/240c1a3c3701ca094a0337006f39d663b63424f2/contracts/InitializerHBBFT.sol#L31-L33 - we did it for the InitializerHBBFT contract.

The public key set and secret BLS key shares could be part of the initialization of KeyGenHistory

Sorry, I'm a little confused by these terms. The public key set and the public BLS keys is not the same, right?

As for other points of https://hackmd.io/_Gg__CJCRleryPv8yxC8DA?view:

Hbbft will report if it detects misbehavior of another node: If a validator violates the protocol, other validators will notify the caller. Parity should forward these reports to the governance contracts and/or the user.

We can have similar reportMalicious function in the ValidatorSetHBBFT contract, but who will call that? Will it be called by one of the validators? Or by the SYSTEM_ADDRESS? Can it be called with zero gas price? And how such a function should look for HBBFT? For now, it looks like https://github.com/poanetwork/posdao-contracts/blob/240c1a3c3701ca094a0337006f39d663b63424f2/contracts/ValidatorSetHBBFT.sol#L23-L81 but it can be outdated (like other HBBFT contracts there).

Also, in AuRa we have the TxPermission contract which defines what functions can be called and by whom. Can we have such a contract for HBBFT as well?

[afck]

Is it possible to call it by SYSTEM_ADDRESS when block is closing like in AuRa?

Yes, BlockRewardHBBFT.reward will definitely continue to be a system call. We just need to decide whether the benefactors should only be the validators which got their contribution into the batch, i.e. which contributed transactions to the block, or all validators. (Because the misbehaving ones will eventually be reported and removed anyway.)

Same for finalizeChange; that's also a system call.

RandomHBBFT.storeRandom, however, we either need to call from some valid address (i.e. an address that is associated with an actual secret key), or we need to store the random value in the block header—then we can make it a system call, too. (Now that I think of it, I think that option is actually cleaner.)

for HBBFT it's better not to use the InitiateChange event

OK, so we'll just call getValidators then, and whenever the returned value is different from what HBBFT is currently using, we start key generation. 👍

I wonder if it is possible at all in HBBFT because I remember that there were some problems in AuRa with those.

The problem is only that system calls can only use information that's already in the block somehow: All their arguments must be computable by anyone who sees the block. E.g. it's not possible to use a system call to input a random number that other nodes seeing the block wouldn't know otherwise. So if we want to make it a system call, we need to use some block header field for actually storing the random number.

Do you mean here something like that?

Right! Sorry, I forgot that that functionality is already there! So we can already initialize the public keys from the genesis block. 👍

The public key set and the public BLS keys is not the same, right?

That's right; and we should probably find better names for them.
The "public key set" is really the public part of the information needed for threshold cryptography.
The public BLS keys are the public parts of the nodes' individual BLS key pairs (that are not part of a threshold scheme).

We can have similar reportMalicious function

Yes, I think it should work the same way as in Aura: Validators will make calls to reportMalicious using a transaction signed by their own key. The function looks good to me as it is.
I'm also for having the same transaction permission contract, and then we can make the malice reports zero gas cost, too.

[varasev]

Yes, BlockRewardHBBFT.reward will definitely continue to be a system call. We just need to decide whether the benefactors should only be the validators which got their contribution into the batch, i.e. which contributed transactions to the block, or all validators.

Let's pass to the benefactors array only those validators who contributed transactions to the block.

RandomHBBFT.storeRandom, however, we either need to call from some valid address (i.e. an address that is associated with an actual secret key), or we need to store the random value in the block header—then we can make it a system call, too.

Yes, that would be great if we could call it by system.

I think, for HBBFT it's better not to use the InitiateChange event and use contract's getter which would return the current validator set.

I meant the getPendingValidators getter here. The getValidators getter returns already finalized set of validators.

Or maybe it's better to make the same working with emitInitiateChange like in AuRa so that this part of the contracts wouldn't change.

Right! Sorry, I forgot that that functionality is already there! So we can already initialize the public keys from the genesis block.

Yes, however, I'm not sure yet if it works because bytes[] memory _publicKeys as a parameter is still considered as experimental Solidity feature - that's why I added there pragma experimental ABIEncoderV2; line.

So, first we should test if it is possible to pass bytes[] array to Solidity function.

I think it should work the same way as in Aura: Validators will make calls to reportMalicious using a transaction signed by their own key. The function looks good to me as it is.

Ok, so for HBBFT we could use AuRa version of reportMalicious function, right?

Let's also think about maximum compatibility of HBBFT engine implementation with the AuRa contracts so that we could make HBBFT contracts similar to AuRa ones as much as possible (except working with random contract and BlockReward.reward functions, imho, because these should differ due to different concept of HBBFT). In this case further supporting of the contracts would be much easier.

[afck]

I agree, there shouldn't be a difference regarding reportMalicious. The main HBBFT changes compared to Aura that I can think of are:

• HBBFT needs to use the KeyGenHistory contract for generating the threshold key shares.
• Validators need to exchange their public BLS keys.
• Validators need a way to make direct connections. Not sure if they have to exchange connection info via a contract for that, though. (And it could also be kept as an "optimization" for later, although without that optimization it'll be unusably slow.)
• HBBFT can directly supply random numbers that are more secure than the randomness contract's. But for now, we can use the contract and add that feature later.
• The Certifier and TxPermission contracts should allow up to N + 1 gas price 0 key generation contract calls per validator-elect.

[varasev]

Let's begin with the KeyGenHistory contract. I created a new and almost empty branch hbbft-light in the posdao-contracts repo: https://github.com/poanetwork/posdao-contracts/tree/hbbft-light

There is only one contract at the moment: https://github.com/poanetwork/posdao-contracts/blob/hbbft-light/contracts/KeyGenHistory.sol - it's almost empty and doesn't have a constructor yet.

I have first questions about the KeyGenHistory:

1. Who will call the writePart and writeAck functions? Will it be a validator defined in the ValidatorSet contract? (will add ValidatorSet later after we deal with the KeyGenHistory)

2. Since we have staking epochs which counter is incremented once per week (when a staking epoch finishes and a new one starts), should we pass the value of that counter to the PartWritten and AckWritten events? (to let the nodes know for which staking epoch the event was emitted)

3. Since during a staking epoch a validator set can change due to the possible removal of some malicious validators from the validator set, will the writePart and writeAck functions be called after that change? Or the only case when these functions can be called is a validator set changing when the staking epochs are changing?

4. As far as I understand, the KeyGenHistory should be deployed right on the genesis block (through the spec.json) and you need to emit the PartWritten and AckWritten events in genesis. Correct me please if I'm wrong. Should the constructor look like this?

   constructor(address[] memory _senders, bytes[] memory _parts, bytes[] memory _acks) public {
       require(_senders.length == _parts.length);
       require(_senders.length == _acks.length);
   
       for (uint256 i = 0; i < _senders.length; i++) {
           emit PartWritten(_senders[i], _parts[i]);
           emit AckWritten(_senders[i], _acks[i]);
       }
   }

[afck]

1. Who will call the writePart and writeAck functions?

The future validators; so at the point where key generation begins, the ValidatorSet contract will already know about them. And I think we should enforce that they can only be called if a change is in progress, and only by the new validators. (Including ones that already are validators but remain in the new set.)

2. …should we pass the value of that counter

We could, but I don't think it's strictly necessary: In any given block, at most one key generation is ongoing.

3. …will the writePart and writeAck functions be called after that change?

Yes, a new key generation begins whenever the validator set is about to change, whether it's because a staking epoch ended or because someone got banned.

4. Should the constructor look like this?

Yes, that looks exactly right. 👍

[varasev]

The future validators; so at the point where key generation begins, the ValidatorSet contract will already know about them

Ok, added TODO for that: https://github.com/poanetwork/posdao-contracts/blob/6f4b37fc5af9e88c810455e479f6a6fd8bb053aa/contracts/KeyGenHistory.sol#L21-L23

And I think we should enforce that they can only be called if a change is in progress

Ok, TODO for that: https://github.com/poanetwork/posdao-contracts/blob/6f4b37fc5af9e88c810455e479f6a6fd8bb053aa/contracts/KeyGenHistory.sol#L25-L27

Yes, that looks exactly right. 👍

Ok, added that into the contract: https://github.com/poanetwork/posdao-contracts/blob/6f4b37fc5af9e88c810455e479f6a6fd8bb053aa/contracts/KeyGenHistory.sol#L10-L18

Now, I think you can try to compile this contract with the constructor arguments and write its bytecode to the spec.json, and then see if the PartWritten and AckWritten events are emitted on the genesis block. To get the bytecode without arguments, perform npm run compile command (the contract's artifacts will appear in the build directory). To get it with the arguments, I'd propose to use the http://remix.ethereum.org or write some simple web3.js script for getting the resulting bytecode with the arguments (I could do that, but later).

One more question regarding the constructor: it uses Solidity experimental feature when the array of bytes are passed; I tried to deploy such a contract in Kovan - deployment was successful, but since this is experimental (and not yet well tested by Solidity developers) it would be better if we could pass there an array of fixed bytes32, e.g.: constructor(address[] memory _validators, bytes32[] memory _parts, bytes32[] memory _acks). So, is it possible to limit each part and ack message in 32 bytes?

[afck]

Sounds good, thank you! 👍

Unfortunately the Parts and Acks grow with the number of validators, so they don't necessarily fit into 32 bytes.

[varasev]

Unfortunately the Parts and Acks grow with the number of validators, so they don't necessarily fit into 32 bytes.

OK, I think bytes[] experimental feature will become production in the upcoming Solidity 0.6.x.