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

Add an overview of Validity subsystems #1161

Merged
merged 10 commits into from
Jun 4, 2020
Merged

Conversation

coriolinus
Copy link
Contributor

@coriolinus coriolinus commented May 27, 2020

This PR takes over from w3f/research#87.

Enumerate the set of subsystems in the validity module and briefly describe their high-level responsibilities and the graph of messages between them.

As the file at this new location included changes not present in
w3f/research#87, this is effectively a
rebase, applied manually. I believe that I have successfully retained
all of, and only, the intended changes.
@coriolinus coriolinus self-assigned this May 27, 2020
@parity-cla-bot
Copy link

It looks like @coriolinus signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

Copy link
Contributor Author

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Copied some comments and linked some discussions from the previous PR. Didn't take all of them, just the ones which seemed unresolved. Hopefully didn't miss anything critical.

roadmap/implementors-guide/guide.md Outdated Show resolved Hide resolved
roadmap/implementors-guide/guide.md Outdated Show resolved Hide resolved
roadmap/implementors-guide/subsystems-overview.md Outdated Show resolved Hide resolved
roadmap/implementors-guide/subsystems-overview.md Outdated Show resolved Hide resolved
roadmap/implementors-guide/subsystems-overview.md Outdated Show resolved Hide resolved
@gavofyork gavofyork added the A0-please_review Pull request needs code review. label May 29, 2020
@rphmeier
Copy link
Contributor

@coriolinus Did you want this to be pleasereview or in-progress? You will want to change the label to inprogress if so

Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

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

Solid on general direction. I like the subsystem map. What are the next steps?

Also could use a merge with master, the line-endings have been merged elsewhere and then they won't show up in the diff here.

@coriolinus coriolinus 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 Jun 2, 2020
@coriolinus coriolinus 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 Jun 2, 2020
@coriolinus coriolinus marked this pull request as ready for review June 2, 2020 11:30
@coriolinus
Copy link
Contributor Author

I think this is ready for review. I've isolated the changes to the implementer's guide; I'm going to open a new PR shortly which contains those changes, and does not contain the subsystems overview.

@coriolinus coriolinus changed the title Expand on the set of documented subsystems Add an overview of Validity Module subsystems Jun 2, 2020
coriolinus added a commit that referenced this pull request Jun 2, 2020
The subsystems overview in #1161
is a very useful reference for what subsystems need to talk to each other,
when, and why; it helps us design the actual messages.

However, administratively, it belongs in a different PR. This commit
brings in all the changes made there so far as a base for an independent
PR.
@rphmeier rphmeier changed the title Add an overview of Validity Module subsystems Add an overview of Backing subsystems Jun 2, 2020
@rphmeier
Copy link
Contributor

rphmeier commented Jun 2, 2020

(Validity Module is about secondary checking & post-inclusion reporting, this is more complex and will be done later)


## Misbehavior Arbitration

The misbehavior arbitration subsystem kicks in when two validator nodes disagree about a candidate's validity. In this case, _all_ validators, not just those assigned to its parachain, weigh in on the validity of this candidate. The minority is slashed.
Copy link
Contributor

@rphmeier rphmeier Jun 2, 2020

Choose a reason for hiding this comment

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

There seem to be two disparate cases covered here.

  1. Arbitration of blocks that are included in the chain, where disputes are signaled by the chain. These blocks are guaranteed to be available, and arbitration is handled by having validators self-select based on a random process to do further approval checks. This is handled on the runtime side by the Validity module.
  2. Arbitration of blocks that have not been included in the chain. My PR to the validity module Add exception to overseer communication and write up local and remote disputes #1176 discusses the possibility of "remote" disputes, for blocks that haven't been included in this chain. We could allow that any disagreement becomes a remote dispute, but so far I've only considered the possibility of remote disputes where a candidate is potentially backed (i.e. has a majority of support from its validator group).

Either way, I'd classify both of these as beyond scope for this PR and suggest that we not define what to do with conflicting votes at the backing stage of the pipeline and revisit those definitions in its own PR, along with the rest of the self-selection logic. This logic is most likely to span multiple subsystems, including those for availability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't intend to include case 1 in this; the cases I've been thinking of are all for blocks which have not yet been included. That said, I'm fine leaving the behavior of this subsystem mostly unspecified for now pending future work.

@coriolinus coriolinus added the B0-silent Changes should not be mentioned in any release notes label Jun 3, 2020
@coriolinus coriolinus changed the title Add an overview of Backing subsystems Add an overview of Validity subsystems Jun 4, 2020
Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

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

Looks fine as beginning, will need some touch-ups based on our conversation

@rphmeier rphmeier merged commit 0b8a8b9 into master Jun 4, 2020
@rphmeier rphmeier deleted the prgn-subsystems-expansion branch June 4, 2020 16:35
rphmeier added a commit that referenced this pull request Jun 10, 2020
…rview (#1185)

* expand subsystems descriptions independent of subsystems overview

The subsystems overview in #1161
is a very useful reference for what subsystems need to talk to each other,
when, and why; it helps us design the actual messages.

However, administratively, it belongs in a different PR. This commit
brings in all the changes made there so far as a base for an independent
PR.

* Reorder subsystem descriptions, add some messages

Update ordering of subsystem descriptions to rough order of use,
mirroring the order in the overview document.

Added some message types. Added OverseerSignal variants to several
types, such that each subsystem only needs to listen for a single
type.

* add some more message types, Statement Distribution overview

* add more detail on Statement Distribution, Misbehavior Arbitration

* intentionally punt MA details for a future PR

* reduce duplication from overseer signal handling

* reword for clarity

* clarify: other modules and subsystems also get to talk to the network

* finish current work on candidate selection

* update candidate backing subsystem description according to current thought

* update mechanism for candidate backing to report collator misbehavior to candidate selection

* sketch out the peer receipt state machine

* Fix typo in roadmap/implementors-guide/guide.md

Co-authored-by: Robert Habermeier <[email protected]>

* Don't specify 'peer validators', as messages from non-validator peers are ignored regardless

* clarify instancing of peer receipt state machine

* add section on peer knowledge tracking

* fix typo in roadmap/implementors-guide/guide.md

Co-authored-by: Max Inden <[email protected]>

Co-authored-by: Robert Habermeier <[email protected]>
Co-authored-by: Max Inden <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants