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

Spec for ordering block parent CIDs by ticket #289

Merged
merged 1 commit into from
Jun 12, 2019
Merged

Conversation

anorth
Copy link
Member

@anorth anorth commented May 23, 2019

As discussed in slack. If this validation proves onerous in implementation we may elect to declare the ordering irrelevant to tipset equivalence, and order lexicographically in the block header instead (at the cost of having two distinct concepts of parent set).

Closes #285

CC @gnunicorn.

@anorth anorth requested a review from whyrusleeping May 23, 2019 04:34
validation.md Outdated
A syntactically valid block:

- must include a well-formed miner address
- must include at least one well-formed ticket
Copy link
Member

Choose a reason for hiding this comment

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

the chain of tickets should be checkable at this point.

@anorth
Copy link
Member Author

anorth commented May 24, 2019

Hold off on landing this for me: see discussion in #285

@@ -321,6 +321,8 @@ All valid blocks generated in a round form a `TipSet` that participants will att

The first condition implies that all blocks in a TipSet were mined at the same height (remember that height refers to block height as opposed to ticket round). This rule is key to helping ensure that EC converges over time. While multiple new blocks can be mined in a round, subsequent blocks all mine off of a TipSet bringing these blocks together. The second rule means blocks in a TipSet are mined in a same round.

The blocks in a tipset are canonically ordered by ticket.
Copy link
Member

Choose a reason for hiding this comment

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

I had thought we settled on making the order not matter at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

This line refers to the logical ordering, the order of processing in consensus state transitions, rather than the "syntactic" requirement (or not) for ordering in the block header. Can I clarify that in the text more?

For the block header, I was waiting for your call in #285 whether to order by CID or not at all.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, okay. I think that what we want to say is that there is no ordering enforced for tipsets, but during state computation, we apply the transitions of blocks in a tipset ordered by their tickets.

@anorth anorth force-pushed the anorth/tipset branch 2 times, most recently from 7f3b72e to ce8dabb Compare June 3, 2019 01:13
@anorth
Copy link
Member Author

anorth commented Jun 3, 2019

@whyrusleeping ready to land

@@ -303,6 +301,9 @@ To create a block, the eligible miner must compute a few fields:
- `ReceiptsRoot` - To compute this:
- Apply the set of messages selected above to the parent state, collecting invocation receipts as this happens.
- Insert them into a Merkle Tree and take its root.
- `Timestamp` - A Unix Timestamp generated at block creation. We use an unsigned integer to represent a UTC timestamp (in seconds). The Timestamp in the newly created block must satisfy the following conditions:
Copy link
Member

Choose a reason for hiding this comment

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

curious why the timestamp was moved. It's not a big deal, but it feels a little odd now being between the message stuff, and the signature aggregation (Which is kinda part of the messages flow). So this now reads "apply all the messages, collect their receipts, GO SET A TIMESTAMP, then take all the signatures from those messages and aggregate them together"

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved it to align with the field order in the block header. I agree its a bit awkward but was aiming for consistency. I'm happy to undo this here, but opine that it should be re-ordered in the block header to come earlier.

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

one small question, then LGTM. Thanks!

@anorth
Copy link
Member Author

anorth commented Jun 4, 2019

(P.S. I'm not authorized to merge, so do land it for me if ok, or push over it)

@anorth
Copy link
Member Author

anorth commented Jun 12, 2019

Please merge me @whyrusleeping

@whyrusleeping
Copy link
Member

Sorry for the delay, was a busy week last week

@whyrusleeping whyrusleeping merged commit 6841949 into master Jun 12, 2019
@sternhenri sternhenri deleted the anorth/tipset branch November 29, 2019 19:40
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.

Define ordering of parent CIDs in block header
2 participants