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

Open
wants to merge 3 commits into
base: master
from
Open

Update LIP-0003 #44

wants to merge 3 commits into from

Conversation

@IkerAlus
Copy link
Member

IkerAlus commented Feb 11, 2020

  • Use delegate address instead of delegate public key
  • Add the dependency of LIP-0022 to generate the round's unique identifier
  • Update functions and links
  • Re-organize content
- Use delegate address instead of public keys
- Add the dependency of LIP-0022 to generate the round's unique identifier
- Update functions and links
- Re-organize content
@IkerAlus IkerAlus requested review from ricott1, janhack and MaximeGagnebin Feb 11, 2020
@IkerAlus IkerAlus self-assigned this Feb 11, 2020
proposals/lip-0003.md Outdated Show resolved Hide resolved
proposals/lip-0003.md Outdated Show resolved Hide resolved
proposals/lip-0003.md Outdated Show resolved Hide resolved
proposals/lip-0003.md Outdated Show resolved Hide resolved
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. This sorting must be deterministic and consistent. This means it has to 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.

This comment has been minimized.

Copy link
@janhack

janhack Feb 12, 2020

Contributor

We would need to define the order of the delegate list before calling the sorting stable algorithm so that tie-breaking in case of a hash collision is defined. Originally, the delegate list was always given by rank. With standby delegates, we may want to assume lexicographic order by address before the sorting.

This comment has been minimized.

Copy link
@ricott1

ricott1 Feb 13, 2020

Contributor

but then wouldn't we be assuming no collision in the address (which would imply collision in the hash if I understand correctly)? So that would not solve the problem. Thinking about it, the problem is not hash collision of 32 bytes, but of 20 bytes (maybe this was clear)

This comment has been minimized.

Copy link
@ricott1

ricott1 Feb 13, 2020

Contributor

disregard my previous comments, addresses are unique identifiers

This comment has been minimized.

Copy link
@IkerAlus

IkerAlus Feb 13, 2020

Author Member

I think it is an overkill to impose a certain order in the input or to do an extra sorting before the actual ordering for this reason. Here we would need to have a hash collision AND a different input list order in the nodes (in practice this input list should be the same for every node if they run our software). I think we are safe by just assuming that SHA-256 is collision resistant (as it is done normally). Therefore I am going to remove the last sentences of this bullet point, that by the way they are more rationale than specification.

proposals/lip-0003.md Outdated Show resolved Hide resolved
Copy link
Contributor

janhack left a comment

I added some comments regarding some clarifications. Moreover, I think we should not use a hyphen when referencing a LIP (i.e., write LIP 0022 instead of LIP-0022) as also done in LIP 0001.

proposals/lip-0003.md Outdated Show resolved Hide resolved
proposals/lip-0003.md Outdated Show resolved Hide resolved
Copy link
Contributor

ricott1 left a comment

Minor comments

@IkerAlus IkerAlus requested review from janhack and MaximeGagnebin Feb 13, 2020

## 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:

This comment has been minimized.

Copy link
@MaximeGagnebin

MaximeGagnebin Feb 14, 2020

Contributor

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."

This comment has been minimized.

Copy link
@janhack

janhack Feb 14, 2020

Contributor

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

This comment has been minimized.

Copy link
@IkerAlus

IkerAlus Feb 18, 2020

Author Member

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.

```
where `randomSeed1` is computed as specified in [LIP 0022](https://github.com/LiskHQ/lips/blob/master/proposals/lip-0022.md#random-seeds-computation).

This comment has been minimized.

Copy link
@MaximeGagnebin

MaximeGagnebin Feb 14, 2020

Contributor

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]

This comment has been minimized.

Copy link
@MaximeGagnebin

MaximeGagnebin Feb 14, 2020

Contributor

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.

This comment has been minimized.

Copy link
@janhack

janhack Feb 14, 2020

Contributor

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

This comment has been minimized.

Copy link
@IkerAlus

IkerAlus Feb 18, 2020

Author Member

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

proposals/lip-0003.md Outdated Show resolved Hide resolved
Copy link
Contributor

janhack left a comment

Consider the comments in the specification section.


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

This comment has been minimized.

Copy link
@janhack

janhack Feb 14, 2020

Contributor

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

- Simplify specs and make them more general
- Specify the round for `randomSeed1 `
- Add delegate address tie breaking
@IkerAlus IkerAlus requested review from janhack and MaximeGagnebin Feb 18, 2020
Copy link
Contributor

MaximeGagnebin left a comment

Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.