Call emitInitiateChange function by validator

[varasev]

(related to #49)

I've missed one detail:

There can be a case when the validator set is not changed at the beginning of new staking epoch (due to delegators did nothing during the finished staking epoch).

The getPendingValidators() getter will return the same set in that case, and as a result, the finalizeChange() function won't be called by Parity (because the set wasn't changed).

But we need to call finalizeChange() anyway when the new staking epoch begins.

Can we use the changeRequestCount() getter instead of getPendingValidators()? Its value is incremented each time when the set of validators is changed or when the new staking epoch begins.

[varasev]

@DemiMarie please clarify, will the Parity trigger the validator set changing (and call finalizeChange as a result) if the getPendingValidators() returns the same validator set, but in the different order?

For example, getPendingValidators() returns the next validator set:

1. 0x1092a1E3A3F2FB2024830Dd12064a4B33fF8EbAe
2. 0xeE385a1df869A468883107B0C06fA8791b28A04f
3. 0x71385ae87c4b93db96f02f952be1f7a63f6057a6

Then imagine that at some block it returns the same set but in another order:

1. 0x71385ae87c4b93db96f02f952be1f7a63f6057a6
2. 0x1092a1E3A3F2FB2024830Dd12064a4B33fF8EbAe
3. 0xeE385a1df869A468883107B0C06fA8791b28A04f

Will it be considered as a new set? (and call finalizeChange() as a result?)

[DemiMarie]

@varasev I am having trouble figuring out the answer, sorry. The code in question is very confusing.

If the order doesn’t matter, I suggest explicitly sorting the return values in Rust code.

[DemiMarie]

@varasev Parity will call finalizeChange whenever it opens a block.

Can finalizeChange be made idempotent? That way, excess calls will not be a problem.

[varasev]

@DemiMarie We have the issue #49. As far as I understand, you implemented this in #53.

The question is:

For example, getPendingValidators() returns the next validator set:

1. 0x1092a1E3A3F2FB2024830Dd12064a4B33fF8EbAe
2. 0xeE385a1df869A468883107B0C06fA8791b28A04f
3. 0x71385ae87c4b93db96f02f952be1f7a63f6057a6

Then imagine that at some block it returns the same set but in another order:

1. 0x71385ae87c4b93db96f02f952be1f7a63f6057a6
2. 0x1092a1E3A3F2FB2024830Dd12064a4B33fF8EbAe
3. 0xeE385a1df869A468883107B0C06fA8791b28A04f

Will it be considered by Parity as a new set according to #49?

[afck]

They are indeed read from the contract in order. And AuthorityRound uses that order to determine the current proposer. So I'd say yes, it does make a difference and the order matters.

However, there is currently no code that tries to detect validator set changes at all, because there's nothing in Aura that's triggered by them: Calling newValidatorSet() is done by the block reward contract itself now, is it? I'm not sure about finalizeChange(): It's called in AuthorityRound::on_new_block if epoch_begin. That is called in OpenBlock::new, with is_epoch_begin as an argument, which is called in lots of places…
The relevant one seems to be in Client::prepare_open_block, where:

let is_epoch_begin = chain.epoch_transition(best_header.number(), h).is_some();

However, BlockChain::epoch_transition seems to just look up some "transition candidates" into the BlockChainDB, and check whether the block hash is among them. It looks like the candidates are inserted using BlockChain::insert_epoch_transition, which is called in Client::check_epoch_end, which in turn calls AuthorityRound::is_epoch_end.

This comment and early return I don't understand at all: I think we always need to care for epochs??
Anyway, next ValidatorSet::is_epoch_end is called, and only if that returns None, some "store for pending transitions" is searched…

For some reason, SafeContract::is_epoch_end (and therefore also Contract::is_epoch_end) always return None!

So back to the store of pending transitions: These are also passed in by the caller, and in this case use Chain::get_pending_transition. These are inserted with Chain::insert_pending_transition, which in turn is called in Client::check_epoch_end_signal—if a call to AuthorityRound::signals_epoch_end returns an EpochChange::Yes(_). This again directly returns No if self.immediate_transitions is true for some reason, and otherwise delegates to ValidatorSet::signals_epoch_end, which in our case asks extract_from_block, which returns Some with the result of getPendingValidators using the default caller (Is that correct—is that for the current block?) unless that contract call failed!

So: My guess is that we're calling finalizeChange in every block, which is wrong and #49 would need to be reopened.
@DemiMarie: Could you please have a look, too, and double-check?

[varasev]

My guess is that we're calling finalizeChange in every block, which is wrong

Exactly, the finalizeChange definitely shouldn't be called every block - I've checked that with original version of Parity with a simple ValidatorSet contract.

The docs on https://wiki.parity.io/Validator-Set.html say that finalizeChange Called when an initiated change reaches finality and is activated.

And finalizeChange Also called when the contract is first enabled for consensus - it means that it is called once on the block number 1 (I checked that).

I repeat here just in case what I wrote in our Slack channel:

According to Parity docs and AFAIK the finalizeChange function is called by Parity in the next case by default:

1. some function of ValidatorSet smart contract emitted InitiateChange event with a new list of validators;

2. this event caught by Parity;

3. the new validator set applied on Parity side;

4. the finalizeChange is called to inform the ValidatorSet contract that the new list of validators (which was passed on the point 1) is applied by the engine.

According to #49 all we need to do is to use getPendingValidators() getter instead of InitiateChange event to know that the validator set is changed and to get the returned set.

I implemented the next fix in the contracts: #71 (comment)

In my implementation, the order of validators returned by getPendingValidators() has the meaning because if the order of validators changed in getPendingValidators() when new staking epoch starts, the contract treats this as the new set of validators and expects that Parity will call finalizeChange by itself.

If getPendingValidators() returns exactly the same set of validators in the same order when new staking epoch starts, the Parity engine won't call finalizeChange() because nothing is changed, so the contract will call this function by itself.

The contracts expect that the different order of the same set of validators in getPendingValidators() will trigger Parity to re-apply the set in the engine and call finalizeChange according to #49

I asked the question #71 (comment) to make sure that Parity will re-apply the inner set of validators and call finalizeChange() even when getPendingValidators() returns the same set of validators but in a different order.

[afck]

all we need to do is to use getPendingValidators() getter instead of InitiateChange

Yes, that's the problem: Every block with an InitiateChange event means an epoch transition, but with getPendingValidators, we'd need to compare the result to the previous one, which wasn't done in #49, I think. Reopening…

[DemiMarie]

@afck I have not done tests, but from reading the code, it seems clear that finalizeChange() is being called every block.

Is there some reason that we cannot just use the InitiateChange event? Or have getPendingValidators() fail when there is no epoch change?

[varasev]

Is there some reason that we cannot just use the InitiateChange event?

We can't use InitiateChange event in newValidatorSet() because it is called by BlockReward.reward() which is called by SYSTEM_ADDRESS.

[afck]

Isn't that the old problem with the log events: that we're currently making the call that initiates the validator set change as a system call and not a transaction, so that it can't log events?

Would it be feasible to compare the returned validator list with the existing one here, and return No if they are identical? (Not sure how exactly the validators field works and what to do if there's no previous entry, though…)

(Edit: Ninja'd by three seconds! 😁)

[DemiMarie]

Another thought: can we make getPendingValidators() revert if called at the wrong time?

@varasev

[DemiMarie]

@afck I can try to figure out where to find the existing one. It is feasible if and only if I can find the old validator set.

[varasev]

@DemiMarie It would be easier if we just used changeRequestCount() getter as a signal that getPendingValidators() should be called and the returned validator set should be applied with followed calling of finalizeChange by the engine (according to Parity docs).

The main thing is that the contract expects that finalizeChange() will always be called by the engine after newValidatorSet() is called and after the new validator set (returned by getPendingValidators()) is applied in Parity. Even when the validator set is not changed. Because finalizeChange() begins new staking epoch.

[varasev]

So, the algorithm must be the next, I think:

1. Check if changeRequestCount() was incremented.

2. If it was, call getPendingValidators() and get its returned validator set.

3. Pass this set of validators to Parity's routine where the InitiateChange event is handled (I don't know how and where it's implemented in Parity).

4. Parity then should apply the validator set and call finalizeChange() according to the docs https://wiki.parity.io/Validator-Set.html

[DemiMarie]

@varasev is it okay for newValidatorSet to set a flag that is cleared on finalizeChange?

[DemiMarie]

That way, I can just write a hasValidatorSetChanged function.

[varasev]

@varasev is it okay for newValidatorSet to set a flag that is cleared on finalizeChange?

We already have such a flag:

• https://github.com/poanetwork/pos-contracts/blob/dfd91898cbceee92fb4cae3ced6207b663c1d2bc/contracts/abstracts/ValidatorSetBase.sol#L575
• https://github.com/poanetwork/pos-contracts/blob/dfd91898cbceee92fb4cae3ced6207b663c1d2bc/contracts/abstracts/ValidatorSetBase.sol#L106

That way, I can just write a hasValidatorSetChanged function.

Parity should request and get the updated validator set in two cases:

1. a new staking epoch begins (i.e. right after newValidatorSet() is called);
2. a malicious validator is removed.

The algorithm with changeRequestCount() I suggested above takes into account these two cases because the changeRequestCount value is incremented each time when the malicious validator is removed from the set or when newValidatorSet() is called.

Do you have a better idea?

[DemiMarie]

@varasev can we call _setValidatorSetApplyBlock(0) from _banValidator(address)? The advantage of a separate boolean flag is that it avoids needing more complex logic on the Parity side. The problem with calling changeRequestCount() is that it does not indicate what the old value was.

[varasev]

Ok, we can go another way:

Each validator will call emitInitiateChange function on his turn (when he produces a block) with zero gas price.

The contracts set initiateChangeAllowed boolean flag to true when the validator set is changed due to misbehavior or when new staking epoch begins.

So, all the validator has to do for this in Parity code is to send a regular transaction with zero gas price for calling emitInitiateChange() function on his turn.

There is a nuance that if InitiateChange has already been emitted but not yet finalized, other InitiateChange events emitted during these several blocks (between emitting and finalizing) will be ignored by Parity. I'll think how to deal with this on the contracts level. Solved by a queue of InitiateChange events: poanetwork/posdao-contracts@d8e9c18

[DemiMarie]

@varasev is it harmful to call emitInitiateChange when it is not necessary?

[DemiMarie]

@varasev does that mean that I can revert the changes in Parity about not using InitiateChange?

[varasev]

@varasev is it harmful to call emitInitiateChange when it is not necessary?

I've just added emitInitiateChangeCallable() getter: poanetwork/posdao-contracts@ca0b980 - if it returns true, the validator can call emitInitiateChange.

@varasev does that mean that I can revert the changes in Parity about not using InitiateChange?

Yes, we should revert all the changes made within #49.

[DemiMarie]

@varasev Can emitInitiateChange just be a no-op if emitInitiateChangeCallable() returns false? That would save some work on the Parity side.

[varasev]

@DemiMarie Yes, in the case when there is nothing to do, emitInitiateChange function just return and does nothing, but the corresponding transaction calling this functions, of course, will be mined anyway.

We could replace returns with requires if that is necessary for Parity code. For example:

    function emitInitiateChange() external {
        require(isValidator(msg.sender));
        require(emitInitiateChangeCallable());
        (address[] memory newSet, bool newStakingEpoch) = _dequeuePendingValidators();
        if (newSet.length > 0) {
            emit InitiateChange(blockhash(block.number - 1), newSet);
            _setInitiateChangeAllowed(false);
            _setQueueValidators(newSet, newStakingEpoch);
        }
    }

In this case the function will revert if there is nothing to do.

[afck]

That would be great: We wouldn't need to change the epoch transition code. 👍
My only concerns are:

• With our current method of making service transactions (putting them into the queue, like we already do for the randomness contract), is it guaranteed that the transaction actually makes it into the current block?
• Does that even work in on_close_block, when the block's gas limit may already have been reached? Can and should we make the call in on_new_block instead?
• Unlike system calls, which are implicit, these are now explicit transactions, and we need to enforce that the block authors make them. Ideally, we should make a block invalid if emitInitiateChange hasn't been called but should have been. At least we should report the author as malicious in that case.

(So: Maybe this approach just works, and is easiest to implement; but we need to dig through the Parity code to make sure these three points are fine.)

[varasev]

With our current method of making service transactions (putting them into the queue, like we already do for the randomness contract), is it guaranteed that the transaction actually makes it into the current block?

I think there is nothing wrong if the calling of emitInitiateChange() will be made on the next block after the current or even on the block after the next because it's not time critical operation.

Does that even work in on_close_block, when the block's gas limit may already have been reached? Can and should we make the call in on_new_block instead?

Yes, it's better to make the call in on_new_block, imho. Because the service transaction consumes the gas as the regular transaction and takes into account the gas limit of the block. By the way, the same goes for RandomAuRa functions (commitHash and revealSecret).

Unlike system calls, which are implicit, these are now explicit transactions, and we need to enforce that the block authors make them. Ideally, we should make a block invalid if emitInitiateChange hasn't been called but should have been. At least we should report the author as malicious in that case.

Ideally yes, it's a good idea. But if the current validator doesn't call emitInitiateChange, the next validator (which is honest) can make the call.

[DemiMarie]

@varasev My understanding is that somebody needs to call emitInitateChange, but the network will be okay as log as any of the validators do it. Is that accurate? Could it be (say) called as a side-effect of the calls to the randomness contract?

[varasev]

My understanding is that somebody needs to call emitInitateChange, but the network will be okay as log as any of the validators do it. Is that accurate?

Actually, we don't need the next condition: https://github.com/poanetwork/pos-contracts/blob/ca0b98040ed9d04b52b1ceabd2c6adc60d123d63/contracts/abstracts/ValidatorSetBase.sol#L93

I removed that: poanetwork/posdao-contracts@19a4136

So, now anybody (not only validator) can call emitInitiateChange.

Could it be (say) called as a side-effect of the calls to the randomness contract?

The RandomAuRa.commitHash and RandomAuRa.revealSecret functions are called by validators not every block according to #51. There can be a big gap between callings, so this proposal doesn't suit for calling emitInitiateChange. The emitInitiateChange needs to be called separately: each validator should call emitInitiateChange function on his turn (ideally when he produces a block).

[afck]

Right, so for now we should just make a constant call to emitInitiateChangeCallable, and if that's true, a transaction call to emitInitiateChange, both in on_new_block and with the same method* as for the randomness contract for now. I'd prefer to make the constant call explicitly (even if it happens inside emitInitiateChange again), because otherwise we'd add a no-op transaction to each block where it's false.

(*) I still suspect that this is an unnecessary indirection and there's probably a more direct way to add the transaction to the current open block without adding it to the queue, but I haven't quite figured that out yet. I added my thoughs as #75.

[DemiMarie]

Fixed.