Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Guide: add a diagram for Inclusion Pipeline & Approval Subsystem #1457

Merged
6 commits merged into from
Jul 31, 2020
Merged

Guide: add a diagram for Inclusion Pipeline & Approval Subsystem #1457

6 commits merged into from
Jul 31, 2020

Conversation

ordian
Copy link
Member

@ordian ordian commented Jul 23, 2020 •
edited
Loading

The Availability Distribution step (6) is somewhat hard to display, it was omitted for brevity. Suggestions on how to improve this are welcome!

Also it only shows the happy path as there are too many ways where a failure can happen.

image

@ordian ordian added I7-documentation Documentation needs fixing, improving or augmenting. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 23, 2020
@ordian ordian self-assigned this Jul 23, 2020
@ordian ordian requested a review from riusricardo July 23, 2020 12:12
@github-actions github-actions bot added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jul 23, 2020
@coriolinus
Copy link
Contributor

👍 for including a render!

  • Are Validator 1 and Validator 3 in the Approval Subsystem the same as in the Parachain Validators section? I suspect not, but I'm not sure.
  • Maybe explicitly forward step 6 to a different diagram?

@ordian
Copy link
Member Author

ordian commented Jul 23, 2020 •
edited
Loading

Are Validator 1 and Validator 3 in the Approval Subsystem the same as in the Parachain Validators section? I suspect not, but I'm not sure.

They are the same if my undestanding is correct. I've tried to reuse the vertices, but the generated graph looked really complex. So I resorted to duplicating them and the diagram's vertical axis is time.

@ordian
Copy link
Member Author

ordian commented Jul 24, 2020

Are Validator 1 and Validator 3 in the Approval Subsystem the same as in the Parachain Validators section? I suspect not, but I'm not sure.

They are the same if my undestanding is correct. I've tried to reuse the vertices, but the generated graph looked really complex. So I resorted to duplicating them and the diagram's vertical axis is time.

Actually I was wrong, let me know what you think about the latest version.

@coriolinus
Copy link
Contributor

LGTM; I'm only not approving because I'm not confident that I have the knowledge to spot an error in the actual flow.

@ordian ordian requested a review from rphmeier July 24, 2020 12:10
@ordian ordian marked this pull request as ready for review July 24, 2020 12:10
@ordian ordian added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 24, 2020
@rphmeier
Copy link
Contributor

rphmeier commented Jul 24, 2020 •
edited
Loading

Yeah, the validators at every stage are drawn from the same set. We take care not to mix validator sets from different sessions. Maybe worth tagging the "Parachain Validators" box as "(subset of all)"

@rphmeier
Copy link
Contributor

bot merge

@ghost
Copy link

ghost commented Jul 31, 2020

Trying merge.

@ghost ghost merged commit caaed9a into master Jul 31, 2020
@ghost ghost deleted the ao-guide-inclusion-diagram branch July 31, 2020 14:28
ordian added a commit that referenced this pull request Jul 31, 2020
@ordian
Merge branch 'master' into ao-chain-api-subsystem
b0e21ba 
* master:
  guide: collator networking & subsystems (#1452)
  Guide: add a diagram for Inclusion Pipeline & Approval Subsystem (#1457)
  [CI] Build wasm blob with srtool and include prop hashes and blobs in release notes (#1506)
@burdges
Copy link
Contributor

burdges commented Aug 6, 2020

It looks fine. I'd note when using that this gives only the happy path of course.

I'd maybe replace "subset of all" with "preassigned subset" for the backing phase and "unpredictable subset" for the approvals phase, but that's really the security reasoning, so..

Just fyi, I attempted to make backing, availability, and approvals all fit the same mold in my tikz diagram in https://github.com/w3f/research/tree/master/docs/papers/AnV/slides not that you need to do that here..

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rphmeier rphmeier rphmeier approved these changes

@riusricardo riusricardo Awaiting requested review from riusricardo

Assignees

@ordian ordian

Labels
A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. I7-documentation Documentation needs fixing, improving or augmenting.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ordian @coriolinus @rphmeier @burdges