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

Moare fixes for parachains#1911

Merged
bkchr merged 3 commits intomasterfrom
bkchr-fix-provisioner-bitfields
Nov 3, 2020
Merged

Moare fixes for parachains#1911
bkchr merged 3 commits intomasterfrom
bkchr-fix-provisioner-bitfields

Conversation

@bkchr
Copy link
Member

@bkchr bkchr commented Nov 3, 2020

  • Sending data to a job should always contain a relay parent. Done this
    for the provisioner
  • Fixed the select_availability_bitfields function. It was assuming we
    have one core per validator, while we only have one core per parachain.
  • Drive by async "rewrite" in proposer

- Sending data to a job should always contain a relay parent. Done this
for the provisioner
- Fixed the `select_availability_bitfields` function. It was assuming we
have one core per validator, while we only have one core per parachain.
- Drive by async "rewrite" in proposer
@bkchr bkchr added 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 Nov 3, 2020
self.provisionable_data_channels.push(sender)
}
ToJob::Provisioner(ProvisionableData(data)) => {
ToJob::Provisioner(ProvisionableData(_, data)) => {
Copy link
Contributor

@coriolinus coriolinus Nov 3, 2020

Choose a reason for hiding this comment

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

Why add the relay parent to ProvisionableData if we just ignore it? I see the statement above that sending data to a job should always include that, but it's not obvious why you said that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is that not obvious? How do you want to know to which job the message needs to be send?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm, you're right; missed the change in messages.rs.

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
Self::RequestBlockAuthorshipData(hash, _) => Some(*hash),
Self::RequestInherentData(hash, _) => Some(*hash),
Self::ProvisionableData(_) => None,
Self::ProvisionableData(hash, _) => Some(*hash),
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that most variants of ProvisionableData include a Hash, might it make more sense to just delegate to that hash instead of adding a new one to this enum?

pub enum ProvisionableData {
/// This bitfield indicates the availability of various candidate blocks.
Bitfield(Hash, SignedAvailabilityBitfield),
/// The Candidate Backing subsystem believes that this candidate is valid, pending availability.
BackedCandidate(BackedCandidate),
/// Misbehavior reports are self-contained proofs of validator misbehavior.
MisbehaviorReport(Hash, MisbehaviorReport),
/// Disputes trigger a broad dispute resolution process.
Dispute(Hash, ValidatorSignature),
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I thought the same, but I wanted to make it more explicit here. I can also change it

@bkchr bkchr merged commit f82c61b into master Nov 3, 2020
@bkchr bkchr deleted the bkchr-fix-provisioner-bitfields branch November 3, 2020 17:16
ordian added a commit that referenced this pull request Nov 4, 2020
* master:
  remove stale migrations (#1914)
  Moare fixes for parachains (#1911)
  Update Proxy Filters (#1890)
  New Weights v0.8.26 (#1889)
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 C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants