This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
implement bitfield signing subsystem #1364
implement bitfield signing subsystem #1364
Changes from 6 commits
170efd3
ed778a4
a8d312c
6d3081a
29cc1c3
5807799
00ef611
7fc7780
684038f
f14ffe5
89ff07b
51dbee5
c1b8b54
4410c87
528cf9b
7779405
333d076
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the
CoreOccupied
type should be used. It shouldn't even be exposed from the runtime, but @montekki added it for candidate backing. Now that there are new runtime APIs, those should be used instead.Except #1419 hasn't been done yet. That's why I recommended stubbing out an
async fn availability_cores { unimplemented!() }
in a separate chat. The implementation of that function would be replaced by the actual runtime API in the future.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to account for parachains and parathreads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we only care about availability as it relates to parachains, but I'm not certain, and the guide doesn't say (AFAICT).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What in the guide made you think that parathreads don't have to worry about availability? They are occupying availability cores just like parachains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, this whole branch is a byproduct of using the wrong type / runtime API. This should be using the
CoreState
type, not theCoreOccupied
type, which shouldn't even be exposed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a hard time understanding this code. It makes a validator groups request and the
rx
receives aSchedulerRoster
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(anyway, this is getting the wrong info, as explained in other comments)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
move
flag here is the reason for the explicit references we construct above, and this appears to work, but it's not a pattern I've seen elsewhere in the codebase. Is there an elegant way to avoid explicit references?