search

paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.87k stars 683 forks source link

More flexible `decl_test_parachains` macro in XCM emulator for `BlockNumber` type #4428

Closed ntn-x2 closed 5 months ago

ntn-x2 commented 5 months ago

The following line https://github.com/paritytech/polkadot-sdk/blob/00440779d42b754292783612fc0f7e99d7cde2d2/cumulus/xcm/xcm-emulator/src/lib.rs#L660 assumes a parachain block number is a u32. When this assumption is not maintained, XCM emulators-based tests do not compile. We think the BlockNumber definition should come from the runtime implementation of the frame_system pallet and not hardcoded to be u32 as specified in parachains_common.

We are on cumulus 1.0 and trying to migrate to 1.1. The issue still persists today, so I am wondering if we should comment out the integration tests until we upgrade to a version of the XCM emulator that includes the fix?

bkontur commented 5 months ago

@ntn-x2 Would this fix, https://github.com/paritytech/polkadot-sdk/pull/4434, work for you?

bkontur commented 5 months ago

@ntn-x2 And another question: Could you please elaborate further on what you mean exactly by 'cumulus 1.0 and trying to migrate to 1.1'? Are you attempting to update your Polkadot SDK dependencies to which version?

This is just a guess, are you using this?

xcm-emulator = {git = "https://github.com/paritytech/cumulus", default-features = false, branch = "polkadot-v1.0.0"}

What is your desired target version here: branch = "polkadot-vX.Y.Z"?

Another topic, there is also possibility to change branch-ref dependencies to the crates.io-like, e.g.:

xcm-emulator = { version = "0.10.0" }

I mean, if this fix works for you, I could possibly backport it to some older versions, for example: https://crates.io/crates/xcm-emulator/versions, just let me know :)

ntn-x2 commented 5 months ago

Would this fix, https://github.com/paritytech/polkadot-sdk/pull/4434, work for you?

@bkontur it looks like it would, yes 😄 But I need to pull in the changes to verify that.

And another question: Could you please elaborate further on what you mean exactly by 'cumulus 1.0 and trying to migrate to 1.1'? Are you attempting to update your Polkadot SDK dependencies to which version?

Yes, we are migrating from xcm-emulator = {git = "https://github.com/paritytech/cumulus", default-features = false, branch = "polkadot-v1.0.0"} to xcm-emulator = {git = "https://github.com/paritytech/polkadot-sdk", default-features = false, tag = "polkadot-v1.1.0"}.

Another topic, there is also possibility to change branch-ref dependencies to the crates.io-like, e.g.:

This would be a different thing, as we have not yet checked how old branch/tag refs change to crates.io releases.

So the best for us would be to backport the fix basically to all releases of polkadot-sdk, if possible. That would need to be the same anyway even if fixes are to be pushed to crates.io, or not? Not sure how semver works in that case.

ntn-x2 commented 5 months ago

@bkontur any updates on this? Can we hope to see it backported in all releases starting from 1.1?

bkontur commented 5 months ago

@bkontur any updates on this? Can we hope to see it backported in all releases starting from 1.1?

Hey @ntn-x2, sorry for the delayed response. It's no problem to backport stuff, but there's something I'd like to clarify before we proceed. I noticed that you want to stick to tag revisions like tag = "polkadot-v1.1.0", ..., tag = "polkadot-v1.11.0". I'm not entirely sure if this is the best approach, because these tags come from branches like origin/release-polkadot-v1.11.0, which are used for polkadot / polkadot-parachain node binaries.

The issue here is that we don't typically backport pallet fixes to these branches. For runtime or pallet-related code, we usually use origin/release-crates-io-vX.Y.Z, such as origin/release-crates-io-v1.11.0 and we release/patch all crates to crates.io. Therefore, if your runtime relies on these polkadot-vX.Y.Z tags, it's possible to miss important pallet patches or fixes.

That's why I suggested considering the use of polkadot-sdk crates.io releases instead. Because it does not make sense to backport this kind of fixes to node releases :).

Another thing, I notice that you have two types of nodes - parachain and standalone. My suggestion is that for your nodes, you could use those tags tag = "polkadot-vX.Y.Z", and for your runtime, it would be better to switch to the crates.io polkadot-sdk releases. Let's discuss this before moving forward with backporting anything.

We have a tool for managing polkadot-sdk versions: https://github.com/paritytech/psvm, I've never tried it yet, but maybe @patriciobcs could help.

There is also a @kianenigma's omni-node: https://forum.polkadot.network/t/polkadot-parachain-omni-node-gathering-ideas-and-feedback, please take a look, if it could be interesting for you.

ntn-x2 commented 5 months ago

Thanks for the useful insights @bkontur! This is our first PR where we upgrade from the different repo to the monorepo, so we are still on time to switch to the branch that makes the most sense! I'll give time to other people from your side to chip in by Monday, after which we'll make an attempt to switch to the monorepo using a different mechanism. In that case I guess backporting the fix to the crates.io releases would make sense, and perhaps it should be done regardless, right? So that we can see if any other issues arise after that fix lands on the relevant versions.

Cheers!

acatangiu commented 5 months ago

@bkontur any updates on this? Can we hope to see it backported in all releases starting from 1.1?

This is not a critical issue that we can prioritize unfortunately. I think this is the kind of issue that should be driven by the community (you). If you open backport PRs, we can help getting them merged.

ntn-x2 commented 5 months ago

Hi @acatangiu, I understand. Thanks for your input. I opened a backport PR here: https://github.com/paritytech/polkadot-sdk/pull/4493. I hope it can get merged soon as it is resulting in failing to compile our integration tests.

bkontur commented 5 months ago

Hi @acatangiu, I understand. Thanks for your input. I opened a backport PR here: #4493. I hope it can get merged soon as it is resulting in failing to compile our integration tests.

@ntn-x2 I checked you PR, yes, go ahead, and prepare backports for release-crates-io-v1.1.0 to release-crates-io-v1.11.0, when ready I will manage to release patched crates for everything as a one issue (don't want to bother other guys with partial tasks :) )

also my suggestion is to start with release-crates-io-v1.7.0, because live Polkadot/Kusama runtimes are bump to this version, so I suggest to you to start bumping your runtime directly to this version, no need to do lower that this :)

I did this bumping stuff for fellows for 1.7.0, you can also check some of my hints: https://github.com/polkadot-fellows/runtimes/issues/101#issuecomment-2020866066

ntn-x2 commented 5 months ago

also my suggestion is to start with release-crates-io-v1.7.0, because live Polkadot/Kusama runtimes are bump to this version, so I suggest to you to start bumping your runtime directly to this version, no need to do lower that this :)

I am not sure I understand your point? Are you suggesting that we could bump our runtime directly from 1.0 to 1.7?

Will open the other backport PRs right away 🚀

bkontur commented 5 months ago

also my suggestion is to start with release-crates-io-v1.7.0, because live Polkadot/Kusama runtimes are bump to this version, so I suggest to you to start bumping your runtime directly to this version, no need to do lower that this :)

I am not sure I understand your point? Are you suggesting that we could bump our runtime directly from 1.0 to 1.7?

Yes, exactly, from version 1.0 to 1.7. That's how I would do it, you can save a lot of work and time. Because, releasing to crates.io was also incremental, so for older releases, there could be issues maybe, and at least we know that polkadot-sdk@v1.7.0 is pretty stable and is already live on Polkadot/Kusama chains.

So, thats why I think it does not make sense to start backporting before 1.7.0

ntn-x2 commented 5 months ago

Because, releasing to crates.io was also incremental, so for older releases, there could be issues maybe, and at least we know that polkadot-sdk@v1.7.0 is pretty stable and is already live on Polkadot/Kusama chains.

I see. We will check that and perhaps bump directly to 1.7.0 and open backport PRs for those only. I think it's ok to still merge the backport PR to 1.1, which is basically what anyone would get after switching to the monorepo. As for the other intermediate versions, if you are implying there could be other issues as well, then perhaps it's ok to skip those. For the time being I will then open only PRs from 1.7 to 1.11 + 1.1, and open other ones if for some reason we will have to stop to those intermediate versions. Thanks for the guidance!

ntn-x2 commented 5 months ago

@bkontur that was a bit of spam, but:

ntn-x2 commented 5 months ago

@bkontur I am still struggling to find more info about the new way of depending on polkadot-sdk versions. I see few tools being built around crates.io versions since they are apparently hard to manage, but no mentions, not even in the release notes for the 1.1 version, about the change in the branching strategy. All I found out is that releases are still based on the polkadot-vX.Y.Z tag. Can you give me some pointers?

bkontur commented 5 months ago

@bkontur I am still struggling to find more info about the new way of depending on polkadot-sdk versions. I see few tools being built around crates.io versions since they are apparently hard to manage, but no mentions, not even in the release notes for the 1.1 version, about the change in the branching strategy. All I found out is that releases are still based on the polkadot-vX.Y.Z tag. Can you give me some pointers?

I asked others for more materials (I will let you know), but at least check this: https://forum.polkadot.network/t/polkadot-sdk-version-manager/

ntn-x2 commented 5 months ago

@bkontur hey any updates on the release strategy you mentioned? We found out other projects mostly follow the polkadot-vX branches for both client and runtime side.

ntn-x2 commented 5 months ago

As a follow-up point, we noticed that, at least for v1.1.0, release-crates-io-v1.1.0 has a lot of additional commits (bugfixes) compared to release-polkadot-v1.1.0, but is not unfortunately just ahead of it. release-polkadot-v1.1.0 received a bugfix with this PR: https://github.com/paritytech/polkadot-sdk/pull/2214. As far as I could see, it also applies to v1.2.0. Also, we've used psvm, and it actually uses the crates branch also for client-side dependencies. Other projects have done the opposite, by depending on the release-polkadot-* branches, so we're now more confused than ever.

bkontur commented 5 months ago

@ntn-x2 I would definitely go with crates.io dependencies and psvm and forget about the release-polkadot-v* branches :) (they are used for releasing polkadot and polkadot-parachain binaries).

For example, polkadot-fellows (Kusama, Polkadot, and all their system parachains) uses crates.io dependencies: https://github.com/polkadot-fellows/runtimes/blob/main/Cargo.toml. So if crates.io dependencies are good for the relay chain and system parachains, crates.io dependencies should be good for every other parachain :)

ntn-x2 commented 5 months ago

@bkontur yes, we've used the crates.io release branch for our runtime components. My doubt is more on what strategy we should follow for our client side, which is of course not a concern for the runtimes repo, with which we agree for runtime dependencies 😊

bkontur commented 5 months ago

@bkontur yes, we've used the crates.io release branch for our runtime components. My doubt is more on what strategy we should follow for our client side, which is of course not a concern for the runtimes repo, with which we agree for runtime dependencies 😊

really, I would suggest to use just crates.io dependencies everywhere instead of any release branch, we release patches when needed to crates.io :)

E.g. this is just client library: https://crates.io/crates/cumulus-relay-chain-inprocess-interface, everything is there in the crates.io.

So e.g. instead of: https://github.com/KILTprotocol/kilt-node/blob/master/Cargo.toml#L131

frame-support = {git = "https://github.com/paritytech/substrate", default-features = false, branch = "polkadot-v1.0.0"}

I would go everywhere just directly from crates.io:

frame-support = { version = "29.0.2", default-features = false }
acatangiu commented 5 months ago

My doubt is more on what strategy we should follow for our client side

@ntn-x2 Is this for KILT parachain? Do you run any custom client logic in your KILT collators/RPC/nodes?

If not you should be able to simply use the polkadot-parachain binary included with every polkadot-sdk release. Or get the docker images from here: https://hub.docker.com/r/parity/polkadot-parachain/tags?page=1&ordering=last_updated

If you can't use polkadot-parachain binary for your nodes for some reason, I suggest you list your missing prerequisites here https://forum.polkadot.network/t/polkadot-parachain-omni-node-gathering-ideas-and-feedback/7823

ntn-x2 commented 5 months ago

Interesting, we will check this. Thanks @acatangiu!

Morganamilo commented 4 months ago

As a follow-up point, we noticed that, at least for v1.1.0, release-crates-io-v1.1.0 has a lot of additional commits (bugfixes) compared to release-polkadot-v1.1.0, but is not unfortunately just ahead of it. release-polkadot-v1.1.0 received a bugfix with this PR: #2214. As far as I could see, it also applies to v1.2.0. Also, we've used psvm, and it actually uses the crates branch also for client-side dependencies. Other projects have done the opposite, by depending on the release-polkadot-* branches, so we're now more confused than ever.

The history of this is a bit messy because we used to not have any crate releases and git was the recommended way to use polkadot.

The release-polkadot-* branches are branched off from master at release time, some modifications are made to prepare them for release, then the binaires are generated from here. And as we had no releases on crates.io, the only way to use our crates was to add git dependencies pointing to these branches.

Now that we have crates.io releases, we have a similar but separate system to generate the releases. We branch off of the same commit that release-polkadot-* does, apply some changes, and push to crates.io.

These two branches exist because we use them to generate our releases. They are visible because we are open source and they can be useful for hacking/investigation purposes.

I do not recommend any one uses the release-crates-io-* branches in the general case, you should just source your deps from crates.io. The release-polkadot-* branch is useful for building your own binaries that match what we've published but it is not intended to be the source for cargo dependencies.

Ad96el commented 4 months ago

@Morganamilo This situation seems confusing to me because the crates.io releases contain broken crates.

For instance, the Rococo runtime in Polkadot version 1.7.0 corresponds to crates.io version 8.0.0. This crate does not compile, and I noticed that there is a backported fix in the release-crates-io-v1.7.0 branch (fix). However, this fix does not appear to be present in the crates.io version - there the crate is not compiling at all.

Could you please clarify where the releases for the crates.io version are made?

Thank you for your comment. I appreciate your work and don't mean to criticize, but as a developer, it is frustrating when there are inconsistencies and differing instructions.