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

Reanchor should return canonical location #4470

Merged
merged 13 commits into from
Dec 14, 2021
Merged

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented Dec 6, 2021

skip-cumulus-check

TODO:

  • fn reanchored(mut assets: Assets, dest: &MultiLocation) -> Result<MultiAssets, XcmError> should either fail if an asset cannot be reanchored or possibly return a pair with (reanchored, unreanchorable), with the latter being placed back into Holding by the instruction.

@gavofyork gavofyork added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. labels Dec 6, 2021
@gavofyork gavofyork changed the title Reanchor should return canonical Reanchor should return canonical location Dec 6, 2021
@@ -418,13 +425,17 @@ impl<Config: config::Config> XcmExecutor<Config> {
for asset in assets.assets_iter() {
Config::AssetTransactor::check_out(&dest, &asset);
}
let assets = Self::reanchored(assets, &dest)?;
// Note that we pass `None` as `maybe_failed_bin` and drop any assets which cannot
// be reanchored because we have already checked all assets out.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100% sure on this one -- the docs on TransactAsset::check_out states the following:

	/// Implementation note: In general this will do one of two things: On chains where the asset is native,
	/// it will increase the assets in a special "teleported" account so that a) total-issuance is preserved; and
	/// b) to ensure that no more assets can be teleported in than were teleported out overall (this should not
	/// be needed if the teleporting chains are to be trusted, but better to be safe than sorry). On chains where
	/// the asset is not native then it will generally just be a no-op.

So that leads me to conclude that the native assets that are checked out are put into the holding register in order to "preserve the total issuance". If we choose to drop the assets here by passing in None, that seems to me that it's equivalent to a burn, i.e. it reduces the total issuance of the native asset. Am I conceptualizing this correctly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but it's not a big deal. It's very difficult to burn assets in this way. People are also heavily incentivised not to (accidentally) burn assets (as they lose them). And if they do, then the fact that the total issuance is unaffected is no big issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I thought the special "teleported" account here meant the holding register, because if it did, then the total issuance would have decreased as it was removed from holding, and wasn't ever put back into it after reanchoring had failed.

let context = X3(OnlyChild, Parachain(1000), PalletInstance(42));
let expected = GeneralIndex(69).into();
location.simplify(&context);
assert_eq!(location, expected);
Copy link
Member

@shawntabrizi shawntabrizi Dec 13, 2021

Choose a reason for hiding this comment

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

Should this pass? Because it does.

		let mut location: MultiLocation = (Parent, PalletInstance(42), GeneralIndex(69)).into();
		let context = X2(PalletInstance(30), PalletInstance(42)); // <-- change here
		let expected = GeneralIndex(69).into();
		location.simplify(&context);
		assert_eq!(location, expected);

I am having a really hard time understanding this function's behavior.

Copy link
Member Author

@gavofyork gavofyork Dec 13, 2021

Choose a reason for hiding this comment

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

Yes. All locations within a consensus system must be taken to be within some local context (aka the consensus system's ancestry). The ancestry is essentially just the consensus system's location from the point of view of the most distant ancestor consensus system under which the local consensus sits. For the Polkadot Relay, this is conventionally empty (as it is its own universe) but for a parachain which knows that it exists within a broader consensus system it would be the Parachain junction by which the parent Relay-chain refers to it.

Local locations which begin with Parent should be "escaping" outside of the local consensus system (otherwise there is no need to begin with Parent), however if after the Parent node(s), the rest of the location is such that it descends back into the local consensus then this becomes a non-canonical location. Basically, the Parent items and their corresponding junctions are superfluous because they're already implied by the ancestry.

In order to normalise it, we need to detect and remove this case.

In this case the context is (PalletInstance(30), PalletInstance(42)
and the location is Parent, PalletInstance(42), GeneralIndex(69). The location is therefore non-canonical, because the Parent is being used to descend outside of the local consensus and beyond PalletInstance(42) (the last item of context) and then re-descend immediately back into the very same PalletInstance(42). Therefore this location is exactly equivalent to the simpler (and shorter) GeneralIndex(69).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I understand now. Basically, we use context and the front of the multilocation and cancel out all "redundant" data, and normalize it.

The ancestry is essentially just the consensus system's location from the point of view of the most distant ancestor consensus system under which the local consensus sits.

This was very key. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 just some note here
image

Choose a reason for hiding this comment

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

where I can read about stuff happening here? except code:) I read all papers on xcm I found and ported acala tests. so found reanchor stuff. do not get what it is. actually security of xcm is very unclear.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dzmitry-lahoda Substrate/Polkadot doesn't have a concept of CurrencyId, and it looks to me to be an Acala/Karura concept. Would you kindly raise this issue in their repo instead?

Choose a reason for hiding this comment

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

:) Substrate/Polkadot has the location and general key concept. why would somebody trust these are genuine? Asset with location FooBar. Parachain X xcmed FooBar from X to Z. Why would we trust that FooBar was not just minted?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are not understanding that every consensus system (or blockchain in layman terms) is responsible for interpreting incoming XCM messages correctly. As such, how a chain decides to interpret GeneralKey or GeneralIndex is not up to us to decide.

Thus, would you please raise this issue to Acala/Karura?

Choose a reason for hiding this comment

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

Still not clear. Kusama can have KSM. Kusama can send (if we call its XCM pallet) to send KSM to paracahain. Can someone send KSM back to Kusama? Does Kusama tracks all places it send KSM to be equal? I mean DOT. Polkadot is relay chain. Does DOT sent via XCM has any security/alignment after it was send from Polkadot to Parachain?

So right now I speak about Relay and some abstract Parachain. And some 2 abstract parachains having DOT/KSM. How these could interpret correctly message with asset (1, Here) origin?

Copy link
Contributor

Choose a reason for hiding this comment

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

MultiLocations are relative paths, as explained in Gav's blogpost. A parachain of Kusama/Polkadot will identify KSM/DOT as (1, Here). When they send (1, Here) to the relay chain, their local XCM executor will reanchor the location to be (0, Here).

As I mentioned in my first message, reanchoring happens automatically behind the scenes and doesn't require any user input. If you are wondering about how assets are transferred between parachains, I have an answer on Stack Exchange that describes in detail how it is done, and how the trust issues are resolved.

@shawntabrizi
Copy link
Member

I'm sorry, but I cannot give an approve at the moment. Maybe it is just late for me and some concepts aren't clicking, but not fully understanding all the behaviors of these functions.

I will start looking again tomorrow morning if you are blocked on merge for me.

@gavofyork gavofyork merged commit ead3db4 into master Dec 14, 2021
@gavofyork gavofyork deleted the gav-canonical-reanchor branch December 14, 2021 08:21
@viniul viniul added D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. labels Dec 16, 2021
jasl added a commit to Phala-Network/khala-parachain that referenced this pull request Feb 6, 2022
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. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants