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

Switch node side to v2 candidate receipts #5679

Open
wants to merge 137 commits into
base: master
Choose a base branch
from

Conversation

sandreim
Copy link
Contributor

@sandreim sandreim commented Sep 11, 2024

on top of #5423

This PR implements the plumbing work required for #5047 . I also added additional helper methods gated by feature "test" in primitives.

TODO:

  • PRDoc

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
sandreim and others added 14 commits September 19, 2024 12:42
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
…aritytech/polkadot-sdk into sandreim/node_v2_descriptors
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
…aritytech/polkadot-sdk into sandreim/node_v2_descriptors

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim sandreim marked this pull request as ready for review September 26, 2024 14:51
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Copy link
Contributor

@alindima alindima left a comment

Choose a reason for hiding this comment

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

Man was this a tough one to review 😄

Thanks for pushing through the tedious work!

Why is this stacked on top of #5423?
From a quick look it could be merged independently

a
}

impl<H: Copy> From<CandidateDescriptor<H>> for CandidateDescriptorV2<H> {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference between this and the previous implementation?
is it just for performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, initially for testing it was good with encode/decode. At this point I am not sure if it is faster, but in theory should be.

@@ -368,7 +471,7 @@ pub enum CandidateReceiptError {

macro_rules! impl_getter {
($field:ident, $type:ident) => {
/// Returns the value of $field field.
/// Returns the value of `$field`` field.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Returns the value of `$field`` field.
/// Returns the value of `$field` field.

fn set_para_head(&mut self, para_head: Hash);
}

#[cfg(feature = "test")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather directly add these methods onto the CandidateDescriptorV2 and guard each of them with #[cfg(test)], which would save us the import of MutateDescriptorV2. But fine either way really

polkadot/runtime/parachains/src/runtime_api_impl/v10.rs Outdated Show resolved Hide resolved
- audience: Node Dev
description: |
This change is just plumbing work and updates all crate interfaces to use the new primitives.
Id doesn't alter any functionality and is required before implementing RFC103 on the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Id doesn't alter any functionality and is required before implementing RFC103 on the
It doesn't alter any functionality and is required before implementing RFC103 on the

Comment on lines +15 to +18
- name: cumulus-client-consensus-common
bump: major
- name: cumulus-client-pov-recovery
bump: major
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these two major? I think they should be none

actually, why is everything a major bump? :D

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've switched the subsystem messages and cumulus traits to new primtives so this breaks things.

Also I'm missing many crates here and the check fails.

Base automatically changed from sandreim/runtime_v2_descriptor_support to master October 7, 2024 07:22
@sandreim
Copy link
Contributor Author

/cmd prdoc --audience node_dev,runtime_dev --bump major

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
…reim/node_v2_descriptors

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim
Copy link
Contributor Author

/cmd prdoc --audience node_dev,runtime_dev --bump major

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

2 participants