Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update LIP-0003 #44

Merged
merged 4 commits into from Feb 26, 2020
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 27 additions & 27 deletions proposals/lip-0003.md
Expand Up @@ -7,6 +7,7 @@ Status: Draft
Type: Standards Track
Created: 2018-08-17
Updated: 2019-03-20
Requires: 0022
```

## Abstract
Expand All @@ -19,26 +20,28 @@ This LIP is licensed under the [Creative Commons Zero 1.0 Universal](https://cre

## Motivation

Currently, there is a bug in how the order of the delegates list is generated for every round. This is implemented in the `Delegates.prototype.generateDelegateList` function of [./modules/delegates.js](https://github.com/LiskHQ/lisk/blob/832c565c641c8a7436c9c164e21a35641096de93/modules/delegates.js#L399). Below you can find the reference code which generates the delegate list for every round:
Currently, there is a bug in how the order of the delegates list is generated for every round. This is implemented in the `shuffleDelegateListForRound` function of [delegates_list.ts](https://github.com/LiskHQ/lisk-sdk/blob/5997714fde5181701fa0508eebd7f3c595cbf92c/elements/lisk-dpos/src/delegates_list.ts#L81). Below you can find the reference code which generates the delegate list for every round:

```js
var seedSource = modules.rounds.calc(height).toString();
var currentSeed = crypto.createHash('sha256').update(seedSource, 'utf8').digest();

for (var i = 0, delCount = truncDelegateList.length; i < delCount; i++) {
for (var x = 0; x < 4 && i < delCount; i++, x++) {
var newIndex = currentSeed[x] % delCount;
var b = truncDelegateList[newIndex];
truncDelegateList[newIndex] = truncDelegateList[i];
truncDelegateList[i] = b;
const seedSource = round.toString();
const delegateList = [...list];

let currentSeed = hash(seedSource, 'utf8');

for (let i = 0, delCount = delegateList.length; i < delCount; i++) {
for (let x = 0; x < 4 && i < delCount; i++, x++) {
const newIndex = currentSeed[x] % delCount;
const b = delegateList[newIndex];
delegateList[newIndex] = delegateList[i];
delegateList[i] = b;
}
currentSeed = crypto.createHash('sha256').update(currentSeed).digest();
currentSeed = hash(currentSeed);
}
```

This code snippet contains two unrelated issues, either of which is sufficient to introduce the generation of non-uniform lists of delegates:

1. The third and fourth lines of code contain two `for` loops that increment the variable `i` twice, every 5th time (in the inner and outer loop). This means that the shuffling logic is skipped for every 5th delegate.
1. The fourth and fifth lines of code contain two `for` loops that increment the variable `i` twice, every 5th time (in the inner and outer loop). This means that the shuffling logic is skipped for every 5th delegate.

2. The line of code after the second `for` loop calculates the new index for the current delegate. It takes what can be assumed to be a uniformly distributed random number between 0 and 255 (`currentSeed[x]`) and calculates the mod101 of it (_delCount = 101_). This implies that some indices are more probable than others. This happens because, for example:

Expand Down Expand Up @@ -75,35 +78,32 @@ Given the study of the issue presented in the previous section, this LIP propose

The proposed fix is based on hashing each delegate’s unique identifier together with a unique identifier for the round:

- The delegate's unique identifier can be the public key or the ID of the delegate. Arguably, it is better to use the public key since the ID system might be changed in the future. However, this is something to be discussed for the reference implementation. Both options are equal in terms of generating randomness.
- The round's unique identifier can be the height of the block in which the new round starts or the height of the round itself. As an example, it can be calculated in the same way it is calculated currently, using the height of the round:
- The delegate's unique identifier is the binary address of the delegate. Note that this proposal does not depend on the implementation of [LIP 0018](https://github.com/LiskHQ/lips/blob/master/proposals/lip-0018.md). The only requirement here is that the addresses have to be unique for each forging delegate which is ensured by the protocol in any case.
- The round's unique identifier is one of the two random seed generated according to the specifications in [LIP 0022](https://github.com/LiskHQ/lips/blob/master/proposals/lip-0022.md#random-seeds-computation).

```js
modules.rounds.calc(height).toString();
```

By having a unique hash per delegate for every round, we will ensure that in every round the list of delegates will be re-ordered based on a uniform distribution.
By having a unique hash per delegate for every round, we will ensure that in every round the list of delegates will be re-ordered based on a uniform distribution. Moreover, these changes not only fix the uniformity issue (caused by the two issues raised in the [Motivation](#motivation) section), but it will also allow simpler and more human-readable code in any implementation.

## Specification

The changes will only affect the `Delegates.prototype.generateDelegateList` function mentioned above. These changes consist of removing the nested `for` loops and generating a new list of “delegate hashes” to be re-calculated and ordered every round. In particular, the proposed fix implements a new algorithm that goes as follows:
The changes affect the `shuffleDelegateListForRound` function mentioned above. These changes consist of removing the nested `for` loops and generating a new list of “delegate hashes” to be re-calculated and ordered every round. In particular, the proposed fix is as follows:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is maybe a bit to "implementation" specific. I liked having the reference to the functions in the rationale/motivation but I think the Specifications should not really mention the function name, or the for loops. Maybe something more generic: "This section specifies how the delegate set has to be ordered for each round."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it would be better to make this part more timeless and implementation independent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LIP was one of the first ever written and it is not . Also, the motivation is very specific regarding the bug in the said function. I re-phrased it and simplified it.


1. The variable `seedSource` is generated in the same way as now:
1. The variable `seedSource` is defined as follows:

```js
var seedSource = modules.rounds.calc(height).toString();
const seedSource = randomSeed1;
```
where `randomSeed1` is computed as specified in [LIP 0022](https://github.com/LiskHQ/lips/blob/master/proposals/lip-0022.md#random-seeds-computation).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we don't really need this step 1. In step 2, we could say:
delegate.roundHash = hash(randomSeed1 | delegate.address)
where randomSeed1 is the random seed for round N (we should probably define which one) as specified in [LIP 0022]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another possibility would be that the "ordering" mechanism just receives a set of delegates AND a random seed, it then returns an ordered list of delegates.

The responsability of selecting the random seed is then left to LIP 0022.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good point as it is not clear in this LIP from which round randomSeed1 is used. LIP 0022 defines basically a function randomSeed1(r) where r is a round. I think then the forger list for round r (using the vocabulary from the documentation) is computed from the set of 103 delegates and the value randomSeed1(r-1).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point for simplicity and clarification of the round, added, thanks!


2. One hash is generated per delegate using `seedSource` and the delegate public key (or ID). For simplicity, we propose to use the hashing function already utilised in the existing code, although the final choice here is left to the reference implementation. Example given in pseudo-code:
2. One hash per delegate is generated using `seedSource` and the delegate address. For simplicity, we propose to use SHA-256, which is the hashing function already utilised in the existing function. Following the current implementation, an example in pseudo-code will be:

```
forEach delegate
delegate.pubKeyHash = createHash(seedSource + delegate.publicKey)
forEach delegate in delegateList
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the specification self-contained and implementation-independent, it would be nice to shortly state what delegateList is.

delegate.roundHash = hash(seedSource | delegate.address)
```
where `|` indicates bytes concatenation.

3. The delegates are reordered according to the hashes generated in step 2. This can be done according to any criteria (in an ascending order, for example) as long as the list is sorted in a deterministic and consistent way. Hence, the sorting algorithm must be [stable](https://en.wikipedia.org/wiki/Sorting_algorithm#Stability) to generate the same list of delegates on every node on the network, even in the unlikely event of a hash collision in the second point. As an example, the `sort()` method in JavaScript can be used for this purpose.

This will not only fix the uniformity issue (caused by the two issues raised in the [Motivation](#motivation) section), but it will also reduce complexity, allowing simpler, more human-readable code in any implementation.
3. The delegates are reordered in lexicographical order according to the hashes generated in step 2.
MaximeGagnebin marked this conversation as resolved.
Show resolved Hide resolved

## Backwards Compatibility

Expand Down