Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bound CandidateDescriptor to core and not para_id #2984

Open
bkchr opened this issue Jan 18, 2024 · 9 comments
Open

Bound CandidateDescriptor to core and not para_id #2984

bkchr opened this issue Jan 18, 2024 · 9 comments
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I4-refactor Code needs refactoring.

Comments

@bkchr
Copy link
Member

bkchr commented Jan 18, 2024

Currently the CandidateDescriptor is "bound" to a ParaId:

/// The ID of the para this is a candidate for.
pub para_id: Id,

We should change this to be a CoreIndex. This will make the type more future proof, to support things like elastic/static scaling when a parachain has more than one core assigned.

@bkchr
Copy link
Member Author

bkchr commented Jan 18, 2024

@eskimor I couldn't find an issue for this. If there exists one, please close and link to it.

@bkchr bkchr added D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I4-refactor Code needs refactoring. labels Jan 18, 2024
@dharjeezy
Copy link
Contributor

Can i pick up this issue as i will like to work on it? @bkchr

@bkchr
Copy link
Member Author

bkchr commented Jan 18, 2024

Maybe first finish your other pr? This issue will also require a lot of work and what needs to be done is not really specified.

@dharjeezy
Copy link
Contributor

Maybe first finish your other pr? This issue will also require a lot of work and what needs to be done is not really specified.

Sure. I already made the changes as requested in the other PR. And, yes, I don't mind the lot of work it requires.

@eskimor
Copy link
Member

eskimor commented Jan 19, 2024

Can't find an issue either right now. Rob created a forum thread at some point collecting more changes to candidate receipt, so we can do all of them at once. In any case, this is precisely the change I was talking about, why Q1 will definitely not fly, even Q2 will be tough.

@dharjeezy The design for this is still ongoing: It might be the simple replacement as suggested by @bkchr or something similar and it will very likely go together with other changes as well. The tricky part is also not the code itself, but the upgrade path.

The discussion I was referring to: https://forum.polkadot.network/t/pre-rfc-discussion-candidate-receipt-format-v2/3738

@dharjeezy
Copy link
Contributor

Oh.. I see. @eskimor Let me know when is the right time to pick this up.

@burdges
Copy link

burdges commented Jan 20, 2024

I'd expect this means adding a core_index, and then using core_index instead of para_id in all the right places, not still using para_id in a few places, which makes this a bit harder to debug.

As an aside.. We've discussed proposals in which we split up some of the roles of the core index, primarily Rob H's idea that relay chain block producers would dynamically collect backed candidates under core indexes, based upon how much resources each candidate claimed it required. At present, these have all fallen out of fashion, but they'd some nice benefits, so who knows. This possibility does not impact this PR, more this PR makes doing that idea slightly easier, if desired again in future.

@dharjeezy
Copy link
Contributor

Is this still desired to be worked upon? i will like to work on this.

@eskimor
Copy link
Member

eskimor commented Feb 13, 2024

We will combine this with a couple of other changes. Design is in the works, but not yet ready unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I4-refactor Code needs refactoring.
Projects
Status: Backlog
Development

No branches or pull requests

4 participants