Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Make AssetId and MultiLocation non-Copy; make Instruction 92% smaller #7236

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

koute
Copy link
Contributor

@koute koute commented May 16, 2023

This PR makes the following changes:

  1. AssetId, MultiLocation, Junctions are not Copy anymore and need to be explicitly clone()'d.

  2. The sizes of XCM types are now drastically reduced.

    Before:

    v2::Instruction - 1208 bytes
    v3::Instruction - 1472 bytes
    v2::MultiAsset - 624 bytes
    v3::MultiAsset - 752 bytes
    v2::MultiLocation - 584 bytes
    v3::MultiLocation - 712 bytes
    v2::AssetId - 584 bytes
    v3::AssetId - 712 bytes
    v2::Junctions - 576 bytes
    v3::Junctions - 704 bytes
    

    After:

    v2::Instruction - 96 bytes
    v3::Instruction - 112 bytes
    v2::MultiAsset - 72 bytes
    v3::MultiAsset - 80 bytes
    v2::MultiLocation - 24 bytes
    v3::MultiLocation - 24 bytes
    v2::AssetId - 32 bytes
    v3::AssetId - 40 bytes
    v2::Junctions - 16 bytes
    v3::Junctions - 16 bytes
    
  3. The Junctions type is now internally boxed, or more specifically Arc'd, and acts in a copy-on-write fashion. This makes it cheaper to clone (now it only has to bump the refcount instead of copying over half a kilobyte).

  4. The canonical way of constructing a Junctions is now through a conversion from an array.

    Before: let xs = Junctions::X2(OnlyChild, OnlyChild)
    After: let xs: Junctions = [OnlyChild, OnlyChild].into()

  5. Since Rust doesn't (yet) support pattern matching of boxed values this makes it not possible to directly match on them, however this can be worked around by converting the internal Junctions into a slice and matching on that, for example:

    Before:

    match location {
         MultiLocation { parents: 1, interior: X1(AccountId32 { id, .. }) } => { ... }
    }
    

    After:

    match location.unpack() {
        (1, [AccountId32 { id, .. }]) => { ... }
    }
    
  6. A few of some of the previously const functions and statics were made not-const since an Arc cannot be constructed in a const context.

Potential open questions

  1. Do we want to make the Junctions type completely opaque? There might not be much of a point in keeping it as an enum now, since you can't directly match on it anyway. (Although this could potentially change in the future when Rust will support matching of boxed values.)
  2. Do we want to keep Junctions::unpack returning a tuple, or perhaps introduce a JunctionsRef type so that the parameters are named when matching?

(This still needs a Cumulus companion, which I will add once paritytech/substrate#14158 is merged.)

@koute koute added I8-refactor Code needs refactoring. I9-footprint An enhancement to provide a smaller (system load, memory, network or disk) footprint. A0-please_review Pull request needs code review. I10-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. T6-XCM This PR/Issue is related to XCM. labels May 16, 2023
@koute koute requested review from gavofyork, KiChjang, ggwpez and a team May 16, 2023 07:05
@paritytech-ci paritytech-ci requested review from a team May 16, 2023 07:06
@muharem muharem self-requested a review May 16, 2023 11:34
}
}

impl IntoIterator for Junctions {
type Item = Junction;
type IntoIter = JunctionsIterator;
fn into_iter(self) -> Self::IntoIter {
JunctionsIterator(self)
JunctionsIterator { range: 0..self.len(), junctions: self }
Copy link
Contributor

@gilescope gilescope May 17, 2023

Choose a reason for hiding this comment

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

would be nice if arg order was switched: junctions, range so it's the same as JunctionsRefIterator above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is deliberate. The arg order must be like this because self is moved into JunctionsIterator, and once you move it you can't call self.len().

Copy link
Contributor

@gilescope gilescope left a comment

Choose a reason for hiding this comment

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

On the whole I really like this.

Comment on lines +147 to +161
pub struct OnlyParachains;
impl Contains<MultiLocation> for OnlyParachains {
fn contains(loc: &MultiLocation) -> bool {
matches!(loc.unpack(), (0, [Parachain(_)]))
}
}

pub struct CollectivesOrFellows;
impl Contains<MultiLocation> for CollectivesOrFellows {
fn contains(loc: &MultiLocation) -> bool {
matches!(loc.unpack(), |(0, [Parachain(COLLECTIVES_ID)])| (
0,
[Parachain(COLLECTIVES_ID), Plurality { id: BodyId::Technical, .. }]
))
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to keep the declarative syntax. As a rule, top-level runtime files should not contain procedural code.

@paritytech-ci paritytech-ci requested review from a team May 22, 2023 11:22
Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

Generally looks like a reasonable direction (modulo the comment).

This should probably go in XCMv4 though since it's a major API breakage.

@paritytech-ci paritytech-ci requested review from a team May 22, 2023 11:28
@koute
Copy link
Contributor Author

koute commented May 22, 2023

This should probably go in XCMv4 though since it's a major API breakage.

@gavofyork Hm... well, although it is a source-level API breakage the ABI will stay the same (Encode and Decode behave the same as they did) so on-chain this shouldn't break anything. It's somewhat unfortunate to forever leave the V2 and V3 with those huge types just because of source-level API incompatibility while in other places we break source-level API compatibility all the time.

I'm not saying we shouldn't care about API-level backwards compatibility, we definitely should, however in my opinion fixing this is worth the API breakage. Having such huge types is a potential performance trap and risks triggering bugs like these not only in our code but in our users' code.

So personally I would suggest landing this and, if we're worried about how it will affect downstream users, I could maybe write a migration guide explaining how to update the code to the new API (which is fairly easy, albeit maybe somewhat tedious) and we'd attach that to the release notes?

But it is up to you; if you don't want to break the API then I can update this PR to only include non-breaking changes in preparation for V4, but obviously this won't fix the underlying issue, just make it easier to do this when V4 lands.

@gilescope
Copy link
Contributor

I note that we currently need to box Multilocations when we want to use them in Call enums because the Call memory size is enforced (by a test) to be max 1kb.

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-43/3158/3

@KiChjang
Copy link
Contributor

KiChjang commented Aug 3, 2023

I think it's best to do this on XCMv4. While it's true the binary interface didn't change, the programming interface did, and it does require some effort by XCM developers to change to the new format.

@gavofyork
Copy link
Member

We might reasonably make XCMv4 an API-only change (for this) and deliver NOW, rather than waiting around indefinitely for the rest of v4 to materialize. The new instructions can be done as a v5.

@KiChjang
Copy link
Contributor

KiChjang commented Aug 11, 2023

Okay, so just to clarify: release v4 now containing this change, and retarget all existing RFCs to v5? Sounds like we should also pair it with some renaming changes.

In any case, this means that this PR needs to create a new folder for v4 and make the changes there, rather than modifying existing v2 and v3 type definitions.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. I8-refactor Code needs refactoring. I9-footprint An enhancement to provide a smaller (system load, memory, network or disk) footprint. I10-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants