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

reword the 'round' definition #351

Merged
merged 7 commits into from
Jul 26, 2019
Merged

reword the 'round' definition #351

merged 7 commits into from
Jul 26, 2019

Conversation

laser
Copy link
Contributor

@laser laser commented Jun 28, 2019

Why is this PR needed?

The previous wording was confusing. It was not clear (to me) that the definition of "round" applied to a period of time in which leader election occurred but no leader was found.

What's in this PR?

I've added more words to the "round" definition in hopes of reducing ambiguity.

@laser laser force-pushed the round-definition branch from 4974a9b to b8fa3f7 Compare June 28, 2019 11:33
@laser laser requested a review from whyrusleeping June 28, 2019 11:33
@whyrusleeping whyrusleeping requested a review from sternhenri July 2, 2019 09:54
Copy link
Contributor

@sternhenri sternhenri left a comment

Choose a reason for hiding this comment

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

I think this is generally good. We could be more specific still.

The 4 key elements imo:

  • a round is the period over which a leader election decision is made
  • a round may yield no decision or multiple
  • we move on to the next round after some time has elapsed or once a decision is made
  • we move to the next height once a decision is made

We have the first 2, missing last 2. I would add them.

My inspiration is here. I think they’ve done a great job w it.

@dignifiedquire
Copy link
Contributor

@laser want to take a stab at improving this further?

@sternhenri
Copy link
Contributor

May take a stab if you're ok with it @laser @dignifiedquire given conflicting requests in #357 (cc @whyrusleeping @pooja)

@pooja pooja mentioned this pull request Jul 11, 2019
19 tasks
@laser
Copy link
Contributor Author

laser commented Jul 11, 2019

@sternhenri - Please do! I would very much appreciate that.

@sternhenri
Copy link
Contributor

  • Made round and height synonymous.
    • Did not remove round from spec entirely as it is easier to use in some sentences (e.g. miner runs another round vs. miner tries again at the next height --> means same thing, first less awkward)
  • removed mentioned of epochs (in consensus context)
  • likewise removed mentions of null blocks (nonsensical as they stood half-unused) in favor of unified 'losing tickets' solution.

Ready to merge from my end.

definitions.md Outdated
`Height` refers to the number of `TipSets` that have passed between this `TipSet` and the genesis block (which starts at block height 0). If a `TipSet` contains multiple blocks, each block in the TipSet will have the same `height`.
`Height` and `round` are synonymous and used interchangeably in this spec.

`Height` refers to the number of tickets generated between this `TipSet` and the genesis block (height 0), counting only a single ticket per TipSet.
Copy link
Contributor

Choose a reason for hiding this comment

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

the last part of this sentence is still confusing at best, wrong at worst.

@sternhenri sternhenri changed the base branch from patch/losing-tickets to master July 26, 2019 14:23
mining.md Outdated
@@ -335,6 +334,7 @@ Heaviest tipset at H-1 is {B0}
- New Round with M mining atop B4

Anytime a miner receives new blocks, it should evaluate which is the heaviest TipSet it knows about and mine atop it.
>>>>>>> ca3f93b3497b870b513332d3cfee381b7590e993
Copy link
Contributor

Choose a reason for hiding this comment

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

merge conflict detected

@dignifiedquire dignifiedquire merged commit 3e7fc29 into master Jul 26, 2019
@dignifiedquire dignifiedquire deleted the round-definition branch July 26, 2019 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants