Validator epochs/ZIP25 #1298
Validator epochs/ZIP25 #1298
Conversation
zilliqa/src/consensus.rs
Outdated
| if self.block_is_first_in_epoch(parent.number() + 1) { | ||
| // Here we ignore any errors and proceed with block creation, under the philosophy that | ||
| // it is better to have the network run but with broken epochs, than it would be to | ||
| // crash the entire network on a failing epoch transition. | ||
| match state.tick_epoch() { | ||
| Ok(()) => (), | ||
| Err(e) => error!("Unable to transition the epoch - EVM error: {e}"), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Our consensus now depends on successful EVM calls - and not just read-only checks as before, but a complex write transaction.
This raises the question of what to do if the tick_epoch() call fails for any reason. Should we assume that it must never fail, and crash the node if it does (potentially crashing the network if a buggy state is reached)? The approach taken here is to soft-ignore errors; the network then will remain alive, but with a static validator set since epoch changes will be broken. Measures can then be taken to upgrade to fix the root cause of the issue (e.g. upgrading the deposit contract) to restore normal functionality.
But, especially right now in the early testnet phases, perhaps hard-crashing the network on any bugs might be preferable to discover any issues immediately.
…ers) and unstaking (full balance withdrawal).
zilliqa/src/contracts/deposit.sol
Outdated
| uint256 physical = queue.head + idx; | ||
| // Wrap the physical index in case it is out-of-bounds of the buffer. | ||
| if (physical >= queue.values.length) { | ||
| return queue.values.length - physical; |
There was a problem hiding this comment.
I'm sure this will fail due to underflow :-)
I guess you want to return physical - queue.values.length;
zilliqa/src/contracts/deposit.sol
Outdated
| bytes[] committeeKeys; | ||
|
|
||
| KeysQueue[WAIT_EPOCHS] pendingQueues; | ||
| KeysQueue[WAIT_EPOCHS] unstakingQueues; |
There was a problem hiding this comment.
we would need to use KeysQueue[UNBONDING_PERIOD_EPOCHS] unstakingQueues; instead where UNBONDING_PERIOD_EPOCHS is set to e.g. 48 (2 days)
zilliqa/src/contracts/deposit.sol
Outdated
| @@ -1,26 +1,138 @@ | |||
| // SPDX-License-Identifier: MIT OR Apache-2.0 | |||
| pragma solidity ^0.8.20; | |||
|
|
|||
| // Implementation of a circular queue/buffer of validator public keys | |||
There was a problem hiding this comment.
I wonder if we could simply use fixed arrays of dynamic arrays of type Value[][WAIT_EPOCHS] instead, with elements being pushed during the epoch and popped until the respective dynamic array is empty when tickEpoch() is called.
zilliqa/src/contracts/deposit.sol
Outdated
| } | ||
|
|
||
| function currentQueueIndex() private view returns (uint8) { | ||
| return uint8(currentEpoch() % WAIT_EPOCHS); |
There was a problem hiding this comment.
would work if the WAIT_PERIOD were the same for staking and unstaking, but for the latter we'll need a longer unbonding period
zilliqa/src/contracts/deposit.sol
Outdated
| _stakerKeys.push(blsPubKey); | ||
| // If staker info isn't stored yet, do so | ||
| // peerId is guaranteed to be non-zero for all stakers | ||
| if (_stakersMap[blsPubKey].controlAddress == address(0)) { |
There was a problem hiding this comment.
due to this condition the deposit() function now can't be used for updating the validators rewardAddress i.e. we should provide an extra function
zilliqa/src/contracts/deposit.sol
Outdated
| } | ||
|
|
||
| // Apply penalty, if any (might be 0 e.g. for voluntary suspension) | ||
| // TODO: collect these rewards somewhere? Right now they're effectively burned |
There was a problem hiding this comment.
a fraction of the slashing penalty shall be paid as whistleblower reward to whoever submitted the slashing evidence, the rest must be burned back to the zero account
zilliqa/src/contracts/deposit.sol
Outdated
| uint8 queueIndex = currentQueueIndex(); | ||
|
|
||
| // Process pending queue for this epoch | ||
| for (uint i = 0; i < pendingQueues[queueIndex].length(); ++i) { |
There was a problem hiding this comment.
Malicious validators could mount a DoS attack by calling depositTopup() a lot of times with small values so that tickEpoch() would need to consume more than the block limit iterating through the pendingQueue
zilliqa/src/contracts/deposit.sol
Outdated
| _stakersMap[blsPubKey].keyIndex = _stakerKeys.length + 1; | ||
| _stakerKeys.push(blsPubKey); | ||
| // If staker info isn't stored yet, do so | ||
| // peerId is guaranteed to be non-zero for all stakers |
There was a problem hiding this comment.
Is it or do have have to check, and even if it is, why is the comment relevant here?
zilliqa/src/contracts/deposit.sol
Outdated
| // no associated amount | ||
| } | ||
|
|
||
| function eject( |
There was a problem hiding this comment.
this is the successor of tempRemoveStaker() and will be removed once penalties are fully implemented, right?
JamesHinshelwood
force-pushed
the
validator-epochs-zip25
branch
from
81007db
to
7ede49f
Compare
Preliminary draft of the new validator state machine, implementing epochs.
tick_epoch()function in the deposit contract.An important consequence of all this is that our consensus now depends on an EVM call succeeding. Inline comment on this in the code below.
Further work out of scope of this PR:
deposit.sol.)