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

Conversation

@rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Jul 4, 2020

Closes #1327

This ensures that the validator-set who is responsible for parachain-related items included in a block can be deterministically derived from the relay-parent hash.

@rphmeier rphmeier added the A0-please_review Pull request needs code review. label Jul 4, 2020
@rphmeier rphmeier requested a review from drahnr July 4, 2020 00:07
@rphmeier rphmeier added 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 4, 2020
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

A few small nitpicks.
For next and previous, I'd be in favor of introducing free floating single char variables n to clarify sequence relations i.e. for blocks or session indices, it makes reading lighter.

// buffered session changes along with the block number at which they should be applied.
//
// typically this will be empty or one element long. ordered ascending by BlockNumber and insertion
// order.
Copy link
Contributor

Choose a reason for hiding this comment

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

what could cause this to be longer than 1 element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Multiple session-change notifications within a single block or a session change notification after initialization in block X and before initialization in child(X). The latter is more likely in practice, if you have a runtime upgrade that changes module order, but both cases would require a session that is only one block long (very very unlikely in practice)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh and Governance as mentioned elsewhere

@rphmeier
Copy link
Contributor Author

rphmeier commented Jul 6, 2020

I'm not sure why CI is failing in a different crate than I've even touched. Hopefully merging master will fix that.

Copy link
Contributor

@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.

Seems perfectly plausible to me.

validators: I,
queued: Option<I>,
)
where I: Iterator<Item=(&'a T::AccountId, ValidatorId)>
Copy link
Contributor

Choose a reason for hiding this comment

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

It may not matter, but IntoIterator bounds are sometimes more convenient than Iterator bounds.

@rphmeier rphmeier added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Jul 9, 2020
@rphmeier rphmeier merged commit da23767 into master Jul 9, 2020
@rphmeier rphmeier deleted the rh-delay-session branch July 9, 2020 13:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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.

Runtime APIs don't account for slot number

4 participants