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

Polkadot v0.9.37 #1241

Merged
merged 62 commits into from Apr 7, 2023
Merged

Polkadot v0.9.37 #1241

merged 62 commits into from Apr 7, 2023

Conversation

NunoAlexandre
Copy link
Contributor

@NunoAlexandre NunoAlexandre commented Mar 9, 2023

Update to Polkadot v0.9.37

A small but noteworthy change is that, from now on, extrinsic are explicitly indexed with #[pallet::call_index(_)], something we need to start doing for new extrinsic in the future and be mindful of not changing indexes, as doing so may break some integration (mostly an XCM concern but still worth keeping it in mind)

❗ Blocked by #1233 (review)

@mikiquantum
Copy link
Contributor

@NunoAlexandre Some clean up is needed and another pass over the command & service on monday. But it compiles now.

@NunoAlexandre
Copy link
Contributor Author

@NunoAlexandre Some clean up is needed and another pass over the command & service on monday. But it compiles now.

You rule buddy, thanks ✌🏻

@branan
Copy link
Contributor

branan commented Apr 5, 2023

From what I'm seeing in #1252, I think that the lint, doc, and integration failures here are all legit.@NunoAlexandre If you can get those cleaned up I think we can land this tomorrow? Everything we need to cut the release has landed in main at this point, so we can move forward.

@NunoAlexandre NunoAlexandre marked this pull request as ready for review April 5, 2023 14:59
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

There are a lot of changes to the service here. I would like to understand them better before we merge that.

.github/workflows/build-nix.yml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
pallets/bridge-mapping/Cargo.toml Outdated Show resolved Hide resolved
pallets/connectors/Cargo.toml Show resolved Hide resolved
@@ -1246,7 +1251,10 @@ impl pallet_xcm_transactor::Config for Runtime {
type CurrencyId = CurrencyId;
type CurrencyIdToMultiLocation = xcm::CurrencyIdConvert;
type DerivativeAddressRegistrationOrigin = EnsureRoot<AccountId>;
type HrmpEncoder = moonbeam_relay_encoder::westend::WestendEncoder;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we use moonbeam here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the HrmpEncoder param type is a type of Moonbeam's pallet_xcm_transactor::Config and it requires knowledge about the way that, in this case, Westend encodes the Hrmp calls; We can also create a custom impl that just returns error with a NotImplement kind of error for example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

mmh. Can you make a note. Just so that we know when we move something of this to production runtimes.

Comment on lines +481 to +484
if !collator_options.relay_chain_rpc_urls.is_empty() && !cli.relaychain_args.is_empty() {
warn!("Detected relay chain node arguments together with --relay-chain-rpc-urls. This command starts a minimal Polkadot node that only uses a network-related subset of all relay chain CLI options.");
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this from?

Copy link
Contributor

Choose a reason for hiding this comment

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

This enables Parachain nodes to run a minimized relay node such that they do not have to fully sync with the relay chain to work, so in general they will use fewer system resources. For details, see Cumulus repo: https://github.com/paritytech/cumulus/blob/master/README.md#external-relay-chain-node

src/command.rs Show resolved Hide resolved
Comment on lines +356 to +359
let _overseer_handle = relay_chain_interface
.overseer_handle()
.map_err(|e| sc_service::Error::Application(Box::new(e)))?;

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this unused handle for? The overseer handle usual is needed on the cumulus side of things but we never pass it down to the import queues or so.

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 am not sure. Maybe I copied it over from another project while trying to follow the way they have adapted to these changes in the service or maybe Miguel wrote it when he took over the work around the service. I feel very foreign to the whole service side of the chain; is this something you feel comfortable with? Cause maybe it's a good idea if you get yours hands on it and/or educate me along

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I would like to get more info on is why we need to add a specific ParachainBlockImport everywhere now. What does the relay in this case pass down? What are the checks the parachain import does differently compared to the relay.

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'm pretty sure that's the stuff Miguel worked on, I don't know the answer without digging further into this. If you got some time and will please do 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

The ParachainBlockImport handles some of the logic around preventing clients from considering forks that haven't been backed by the relay chain. I'm not sure when it was introduced, but cumulus was updated in paritytech/cumulus#1559 to make it mandatory through the use of a marker trait.

Copy link
Contributor

@thea-leake thea-leake left a comment

Choose a reason for hiding this comment

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

LGTM!

ci/script.sh Show resolved Hide resolved
src/service.rs Show resolved Hide resolved
src/service.rs Show resolved Hide resolved
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

@NunoAlexandre I have no time to dig into the service and am out the next 8 days, so I am approving this one to not letting it linger around for too long.

Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

Re-approve

@NunoAlexandre NunoAlexandre enabled auto-merge (squash) April 7, 2023 06:21
@hieronx hieronx disabled auto-merge April 7, 2023 06:46
@hieronx hieronx merged commit d36b067 into main Apr 7, 2023
11 checks passed
@NunoAlexandre NunoAlexandre deleted the polkadot-v0.9.37 branch April 7, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants