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-0022 #45

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

Update LIP-0022 #45

wants to merge 6 commits into from

Conversation

@IkerAlus
Copy link
Member

IkerAlus commented Feb 11, 2020

  • Use delegate address instead of delegate public keys in the algorithm and delegate lists.
  • Remove the changes to LIP-0003 from the specifications
  • Update reference to functions and links
- Use delegate address instead of delegate public keys in the algorithm and delegate lists.
- Remove the changes to LIP-0003 from the specifications
- Update reference to functions and links
@IkerAlus IkerAlus requested review from shuse2, janhack and MaximeGagnebin Feb 11, 2020
@IkerAlus IkerAlus self-assigned this Feb 11, 2020
@shuse2
shuse2 approved these changes Feb 12, 2020
proposals/lip-0022.md Outdated Show resolved Hide resolved
proposals/lip-0022.md Outdated Show resolved Hide resolved

Bear in mind that this proposal does not change any other point in the round processing once the list with the 103 forging delegates for the next round is computed.
This LIP also proposes to use the 20 bytes adddress of a delegate as defined in [LIP-0018](https://github.com/LiskHQ/lips/blob/master/proposals/lip-0018.md) for the [delegate lists](https://github.com/LiskHQ/lisk-sdk/blob/5997714fde5181701fa0508eebd7f3c595cbf92c/elements/lisk-dpos/src/delegates_list.ts#L124). This also implies two things:

This comment has been minimized.

Copy link
@janhack

janhack Feb 12, 2020

Contributor

This line and the two next line rather refer to the implementation than the protocol if I understand correctly. I don't understand why here the order in the delegate list is important because the forging order is defined in LIP 0003.

This comment has been minimized.

Copy link
@IkerAlus

IkerAlus Feb 13, 2020

Author Member

With the first bulletpoint I wanted to cover every case in which there is a tie by delegateWeight. Bear in mind that this is not about the ordered output of the function in LIP 0003 but about the input of said function. As said in line 186 here, a list of 103 delegates has to be created where 101 of them are the top 101 delegates by delegateWeight. The bulletpoint here covers the situation where if for example delegate 101 and delegate 102 have the same delegateWeight, the one with the "smaller" address will make it to the next forging round. This is now done by the function createRoundDelegateList, but it is done with the delegate public keys. Perhaps it is clearer if I join both points in the same bullet point at line 186?

This comment has been minimized.

Copy link
@IkerAlus

IkerAlus Feb 13, 2020

Author Member

Regarding the second bulletpoint at line 190, I think it is in the limit of being considered a protocol change. I preferred to state it here to avoid future issues.

This comment has been minimized.

Copy link
@janhack

janhack Feb 14, 2020

Contributor

Good point regarding the tie-breaking. Maybe the "Round Computation and Delegate List" section could be re-structures a bit to make this more clear. One way would be to define the delegate list from scratch (or forger list as used in the documentation) and how it is created (add 101 active delegate by weight with tie-breaking by address plus 2 randomly selected standby delegates). Currently, it rather defines how an object (delegate_list class) in the code needs to be modified.

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

janhack left a comment

Consider the comments in the delegate list section. Possibly, we could even remove this section as it mostly refers to the implementation and does not specify the protocol.

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

janhack left a comment

Consider the remaining open comments in the "Round Computation and Delegate List" section.

@IkerAlus IkerAlus requested a review from janhack Feb 18, 2020
* The round length is now 103 block slots. Currently, this is defined by the constant <code>[ACTIVE_DELEGATES](https://github.com/LiskHQ/lisk-sdk/blob/310283408102ff9f891830225afe8124e833a608/config/default/constants.js#L55)</code>. For clarity, this constant should be replaced for three constants defining the number of the top 101 delegates by `delegateWeight`, the number of standby delegates, i.e., 2, and the length of the round (sum of the two previous constants).
* The change in the length of the round affects the logic to check when a round is finished which is 103 block slots in total.
* `delegate1` and `delegate2` have to be included in the delegate list<sup>3</sup> before it is re-ordered for `round` + 1. Currently, the function <code>[getKeysSortByVote](https://github.com/LiskHQ/lisk-sdk/blob/7c31c9d4b12a72f18a35ead79198b2ba6d4bd091/framework/src/modules/chain/submodules/delegates.js#L32)</code> computes this list before being re-ordered.
Before being [shuffled](https://github.com/LiskHQ/lips/blob/master/proposals/lip-0003.md), the forger list<sup>3</sup> for `round` + 1 is constructed as follows:

This comment has been minimized.

Copy link
@janhack

janhack Feb 19, 2020

Contributor
Suggested change
Before being [shuffled](https://github.com/LiskHQ/lips/blob/master/proposals/lip-0003.md), the forger list<sup>3</sup> for `round` + 1 is constructed as follows:
Before being ordered according to [LIP 0003](https://github.com/LiskHQ/lips/blob/master/proposals/lip-0003.md), the set of 103 forgers for `round + 1` is constructed as follows:

This comment has been minimized.

Copy link
@janhack

janhack Feb 19, 2020

Contributor

For me it would be clearer if there is only one forger list for a round, i.e., the forgers in the order of assigned block slots. What is defined here is rather a set, as the order does not matter for the protocol (only the order in LIP 0003 matters). I find it a bit confusing to call this set forger list and also impose some arbitrary order on it.

This comment has been minimized.

Copy link
@janhack

janhack Feb 19, 2020

Contributor

I also find the foot note confusing as here you specify how this list is computed and it is unclear what computing with a delay of 2 rounds means.

This comment has been minimized.

Copy link
@janhack

janhack Feb 19, 2020

Contributor

You could also define the creation of the forger list here and make the 3rd step, order the delegates according to LIP 0003.

This comment has been minimized.

Copy link
@IkerAlus

IkerAlus Feb 19, 2020

Author Member

Modified the first part as suggested. Also changed the footnote formulation to make it clearer, I dont want to remove so we have a quick link to check where this 2 rounds delay comes from.

This comment has been minimized.

Copy link
@IkerAlus

IkerAlus Feb 19, 2020

Author Member

I also removed every occurrence of forging list as the structure being specified here.


Bear in mind that this proposal does not change any other point in the round processing once the list with the 103 forging delegates for the next round is computed.
* The addresses of the top 101 delegates at the end of `round` - 1 by lexicographic order according to the combined key `(delegateWeight, address)` are included in the forging list.
* The addresses of `delegate1` and `delegate2`, defined in the previous section, are included at the end of the forger list in this order respectively.

This comment has been minimized.

Copy link
@janhack

janhack Feb 19, 2020

Contributor
Suggested change
* The addresses of `delegate1` and `delegate2`, defined in the previous section, are included at the end of the forger list in this order respectively.
* The addresses of `delegate1` and `delegate2`, defined in the previous section, are added to the set of forgers.

This comment has been minimized.

Copy link
@IkerAlus

IkerAlus Feb 19, 2020

Author Member

changed to "forging delegates"


Bear in mind that this proposal does not change any other point in the round processing once the list with the 103 forging delegates for the next round is computed.
* The addresses of the top 101 delegates at the end of `round` - 1 by lexicographic order according to the combined key `(delegateWeight, address)` are included in the forging list.

This comment has been minimized.

Copy link
@janhack

janhack Feb 19, 2020

Contributor
Suggested change
* The addresses of the top 101 delegates at the end of `round` - 1 by lexicographic order according to the combined key `(delegateWeight, address)` are included in the forging list.
* The addresses of the top 101 delegates by delegate weight at the end of `round - 2` are added to the forger set. Ties are broken in favor of smaller address.

This comment has been minimized.

Copy link
@janhack

janhack Feb 19, 2020

Contributor
  • I think it should be the end of round-2. The same weights that are used for the standby delegate selection above.
  • Sorting lexicographically by combined key (delegateWeight, address) is a bit confusing, because the delegate with smallest weight come first and it is unclear what top means then. Did you want to do the tie-breaking by larger address?

This comment has been minimized.

Copy link
@IkerAlus

IkerAlus Feb 19, 2020

Author Member

True, the round was wrong. Changed the whole point as suggested, thanks!

Copy link
Contributor

janhack left a comment

Please consider the comments in the "Round Computation and Forger List" section.

@IkerAlus IkerAlus requested a review from janhack Feb 19, 2020
@janhack janhack requested a review from MaximeGagnebin Feb 19, 2020

Bear in mind that this proposal does not change any other point in the round processing once the list with the 103 forging delegates for the next round is computed.
* The addresses of the top 101 delegates by `delegateWeight` at the end of `round` - 2 are added to the set<sup>3</sup>. Ties are broken in favor of smaller address.

This comment has been minimized.

Copy link
@janhack

janhack Feb 19, 2020

Contributor

To me now the sentence says the same things as the footnote at the end, which I find confusing.

 Also, remove footnote 3 and other formatting
@IkerAlus IkerAlus requested review from janhack and shuse2 Feb 19, 2020
Copy link
Contributor

janhack left a comment

Looks good!

@shuse2
shuse2 approved these changes Feb 24, 2020
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.