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

Optimize multilocation reanchoring logic in XCM #6301

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

tonyalaribe
Copy link
Contributor

This PR improves the algorithm for reanchor - see paritytech/polkadot-sdk#897 for more details.

Approach:

  • We first normalize/prepend target and id with the ancestor
  • We then remove any prefix nodes with both target and id share in common
  • Then we convert all of target to parents, and add that to the parents of id.
    This gives us the path to go from target to id, without any superflous nodes, or the need for post simplification.

Question:
The simplify() method which was used previously is public. Does it make sense to leave it or should it be removed if it's not used anywhere else in the polkadot codebase (except tests)? This also applies with the inverted() method on Multilocation

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Nov 16, 2022
@tonyalaribe tonyalaribe added 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. labels Nov 17, 2022
@muharem
Copy link
Contributor

muharem commented Nov 17, 2022

@tonyalaribe I see two not covered edge cases, here the failing tests #6307

Comment on lines 344 to 346
id.parents = id
.parent_count()
.saturating_add(target.parent_count().saturating_add(target.interior().len() as u8));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
id.parents = id
.parent_count()
.saturating_add(target.parent_count().saturating_add(target.interior().len() as u8));
let final_parents = (id.parent_count() as usize).saturating_add(
(target.parent_count() as usize).saturating_add(target.interior().len()),
);
if final_parents > 255 {
return Err(())
}
id.parents = final_parents as u8;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see. The idea is to return an error if there's an overflow and not just saturate it to fail silently? In that case I've switched to using a checked_add instead of saturating_add. That should give us the behaviour we want.

xcm/src/v1/multilocation.rs Outdated Show resolved Hide resolved
xcm/src/v1/multilocation.rs Outdated Show resolved Hide resolved
@tonyalaribe
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@tonyalaribe
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@tonyalaribe
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Head SHA changed from 59b949d to 36db35e

@tonyalaribe
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@tonyalaribe
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@tonyalaribe
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@dzmitry-lahoda
Copy link

dzmitry-lahoda commented Dec 27, 2022

I took tests from location_conversion.rs and ported to multilocation.rs on master of polkadot

	fn account20() -> Junction {
		AccountKey20 { network: Any, key: Default::default() }
	}

	fn account32() -> Junction {
		AccountId32 { network: Any, id: Default::default() }
	}

	#[test]
	fn inverter_works_in_tree() {
		let ancestry: MultiLocation = X3(Parachain(1), account20(), account20()).into();
		let target = MultiLocation::new(3, X2(Parachain(2), account32()));
		let inverted = ancestry.inverted(&target).unwrap();
		let expected = MultiLocation::new(2, X3(Parachain(1), account20(), account20()));
		assert_eq!(inverted, expected);
	}

	#[test]
	fn inverter_uses_ancestry_as_inverted_location() {
		let ancestry: MultiLocation = X2(account20(), account20()).into();
		let target = MultiLocation::grandparent();
		let inverted = ancestry.inverted(&target).unwrap();
		let expected = X2(account20(), account20()).into();
		assert_eq!(inverted, expected);
	}

	#[test]
	fn inverter_uses_only_child_on_missing_ancestry() {
		let ancestry: MultiLocation = X1(PalletInstance(5)).into();
		let target = MultiLocation::grandparent();
		let inverted = ancestry.inverted(&target).unwrap();
		let expected = X2(PalletInstance(5), OnlyChild).into();
		assert_eq!(inverted, expected);
	}

	#[test]
	fn inverter_errors_when_location_is_too_large() {
		let ancestry: MultiLocation = Here.into();
		let target = MultiLocation { parents: 99, interior: X1(Parachain(88)) };
		let inverted = ancestry.inverted(&target);
		assert_eq!(inverted, Err(()));
	}

Only one test is failing:

---- v1::multilocation::tests::inverter_uses_only_child_on_missing_ancestry stdout ----
thread 'v1::multilocation::tests::inverter_uses_only_child_on_missing_ancestry' panicked at 'assertion failed: `(left == right)`
  left: `MultiLocation { parents: 0, interior: X2(OnlyChild, PalletInstance(5)) }`,
 right: `MultiLocation { parents: 0, interior: X2(PalletInstance(5), OnlyChild) }`', xcm/src/v1/multilocation.rs:913:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

May be useful to ensure that no regression happened.

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. I10-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants