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

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

Closed
ntn-x2 opened this issue May 10, 2024 · 24 comments · Fixed by #4434
Closed

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

ntn-x2 opened this issue May 10, 2024 · 24 comments · Fixed by #4434
Labels
I10-unconfirmed Issue might be valid, but it's not yet known.

Comments

@ntn-x2
Copy link

ntn-x2 commented May 10, 2024

The following line

<<Self as Parachain>::ParachainSystem as Hooks<$crate::BlockNumber>>::on_initialize(block_number);
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?

@github-actions github-actions bot added the I10-unconfirmed Issue might be valid, but it's not yet known. label May 10, 2024
@ntn-x2 ntn-x2 changed the title More flexible decl_test_parachains macro in XCM emulator More flexible decl_test_parachains macro in XCM emulator for type BlockNumber type May 10, 2024
@ntn-x2 ntn-x2 changed the title More flexible decl_test_parachains macro in XCM emulator for type BlockNumber type More flexible decl_test_parachains macro in XCM emulator for BlockNumber type May 10, 2024
@bkontur
Copy link
Contributor

bkontur commented May 10, 2024

@ntn-x2 Would this fix, #4434, work for you?

@bkontur
Copy link
Contributor

bkontur commented May 10, 2024

@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
Copy link
Author

ntn-x2 commented May 13, 2024

Would this fix, #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
Copy link
Author

ntn-x2 commented May 16, 2024

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

@bkontur
Copy link
Contributor

bkontur commented May 16, 2024

@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
Copy link
Author

ntn-x2 commented May 16, 2024

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
Copy link
Contributor

@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 pushed a commit to KILTprotocol/polkadot-sdk that referenced this issue May 17, 2024
@ntn-x2
Copy link
Author

ntn-x2 commented May 17, 2024

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.

@bkontur
Copy link
Contributor

bkontur commented May 17, 2024

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: polkadot-fellows/runtimes#101 (comment)

@ntn-x2
Copy link
Author

ntn-x2 commented May 17, 2024

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
Copy link
Contributor

bkontur commented May 17, 2024

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 [email protected] 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
Copy link
Author

ntn-x2 commented May 17, 2024

Because, releasing to crates.io was also incremental, so for older releases, there could be issues maybe, and at least we know that [email protected] 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!

acatangiu pushed a commit that referenced this issue May 17, 2024
…kNumber=u32` (#4434) (#4493)

Cherry-picked from `master` (#4434). Context:
#4428.

I don't see any other way than opening other PRs for each of the
released branches with this fix. If you guys have an alternative
proposal, I am all ears. Otherwise, I'll go ahead and open the other
ones once this gets merged.

Co-authored-by: Branislav Kontur <[email protected]>
ntn-x2 pushed a commit to KILTprotocol/polkadot-sdk that referenced this issue May 17, 2024
ntn-x2 pushed a commit to KILTprotocol/polkadot-sdk that referenced this issue May 17, 2024
ntn-x2 pushed a commit to KILTprotocol/polkadot-sdk that referenced this issue May 17, 2024
ntn-x2 pushed a commit to KILTprotocol/polkadot-sdk that referenced this issue May 17, 2024
@ntn-x2
Copy link
Author

ntn-x2 commented May 17, 2024

@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?

acatangiu pushed a commit that referenced this issue May 17, 2024
…kNumber=u32` (#4434) (#4496)

Context: #4428 and
#4493.

Co-authored-by: Branislav Kontur <[email protected]>
acatangiu pushed a commit that referenced this issue May 17, 2024
…kNumber=u32` (#4434) (#4497)

Context: #4428 and
#4493.

Co-authored-by: Branislav Kontur <[email protected]>
acatangiu pushed a commit that referenced this issue May 17, 2024
…kNumber=u32` (#4434) (#4498)

Context: #4428 and
#4493.

Co-authored-by: Branislav Kontur <[email protected]>
acatangiu pushed a commit that referenced this issue May 17, 2024
…kNumber=u32` (#4434) (#4499)

Context: #4428 and
#4493.

Co-authored-by: Branislav Kontur <[email protected]>
acatangiu pushed a commit that referenced this issue May 17, 2024
…kNumber=u32` (#4434) (#4500)

Context: #4428 and
#4493.

Co-authored-by: Branislav Kontur <[email protected]>
@bkontur
Copy link
Contributor

bkontur commented May 17, 2024

@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
Copy link
Author

ntn-x2 commented May 27, 2024

@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
Copy link
Author

ntn-x2 commented May 28, 2024

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.

@bkontur
Copy link
Contributor

bkontur commented May 30, 2024

@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
Copy link
Author

ntn-x2 commented May 31, 2024

@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
Copy link
Contributor

bkontur commented May 31, 2024

@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
Copy link
Contributor

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
Copy link
Author

ntn-x2 commented May 31, 2024

Interesting, we will check this. Thanks @acatangiu!

hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this issue Jun 5, 2024
@Morganamilo
Copy link
Contributor

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
Copy link

Ad96el commented Jun 12, 2024

@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.

liuchengxu pushed a commit to liuchengxu/polkadot-sdk that referenced this issue Jun 19, 2024
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this issue Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I10-unconfirmed Issue might be valid, but it's not yet known.
Projects
None yet
5 participants