XCM: Remote account converter #6662

mustermeiszer · 2023-02-02T11:15:38Z

Based on the discussion in #6612 this PR adds a converter to the codebase that allows to have a stable conversion from a local account (xcm sending location) to a remote account (xcm receiving location).

mustermeiszer · 2023-02-02T16:35:40Z

@xlc @joepetrowski @jak-pan @girazoki WDYT about the converter like this?

Happy to adapt to what is the compromise everybody is happy with.

mustermeiszer · 2023-02-02T16:36:30Z

Also @KiChjang do you already have some remarks?

xlc · 2023-02-03T03:13:01Z

I think a fixed prefix that includes parachain id could be useful. This acts as metadata and namespaced addresses. By only looking at an address, we will know it is an account belong to a certain parachain and impossible for one parachain to access account belong to another parachain

mustermeiszer · 2023-02-03T08:50:34Z

You mean as part of the resulting [u8; 32]?

I am not sure this adds a lot, but I am also not against it.

Security is already derived from the input of the hash to prevent access from other parachains
From UI perspective this won't help, as the ss58 is not helping here.
The raw-bytes will have a clear prefix that signal the "origin"

Happy to add that if we want that to be part of it.

mustermeiszer · 2023-02-06T12:31:23Z

@xlc I thought about adding a fixed prefix to the resulting [u8; 32] and my main thoughts against it are

a short prefix (e.g. *b"P1000") → higher probability to have collisions with "local" accounts
a larger prefix (e.g. *b"ForeignP1000") → the entropy of the bytes is too low

Basically, these are just thoughts of someone who barely knows stuff about cryptography and the probabilities of collisions. But I think if we want a prefix we need to inspect this more thoroughly while we can trust the blake2_256 output.

gavofyork · 2023-02-14T08:55:57Z

xcm/xcm-builder/src/location_conversion.rs

+			MultiLocation {
+				parents: 0,
+				interior: X2(Parachain(para_id), AccountId32 { id, .. }),
+			} => ForeignChainAliasAccount::<AccountId>::from_para_32(para_id, id),


Suggested change

} => ForeignChainAliasAccount::<AccountId>::from_para_32(para_id, id),

} => ForeignChainAliasAccount::<AccountId>::from_relay_32(para_id, id),

This matching arms matches against messages received on the relay chain coming from a para-chain. I would argue that this should use the conversion from the parachain, allowing to have the same alias on each consuming chain of this converter.

Then I would add another item from_child_32, and with its own prefix.

gavofyork · 2023-02-14T08:56:15Z

xcm/xcm-builder/src/location_conversion.rs

+			MultiLocation {
+				parents: 0,
+				interior: X2(Parachain(para_id), AccountKey20 { key, .. }),
+			} => ForeignChainAliasAccount::<AccountId>::from_para_20(para_id, key),


Suggested change

} => ForeignChainAliasAccount::<AccountId>::from_para_20(para_id, key),

} => ForeignChainAliasAccount::<AccountId>::from_relay_20(para_id, key),

This matching arms matches against messages received on the relay chain coming from a para-chain. I would argue that this should use the conversion from the parachain, allowing to have the same alias on each consuming chain of this converter.

Then I would add another item from_child_20, and with its own prefix.

gavofyork · 2023-02-14T08:57:10Z

xcm/xcm-builder/src/location_conversion.rs

+
+impl<AccountId> ForeignChainAliasAccount<AccountId> {
+	fn from_para_32(para_id: &u32, id: &[u8; 32]) -> [u8; 32] {
+		(FOREIGN_CHAIN_PREFIX, para_id, id).using_encoded(blake2_256)


I would suggest different prefixes for each mode.

Why so? Can add that of course, just am curious about the reasoning.

To avoid the possibility of unintended collision.

Okay. I assumed that having a 32 bits difference between from_relay_32 and from_para_32 and a 256 bits difference between the from_para_* methods in the input stream would be enough difference to have sufficient bitflips with blake.

Will adapt then!

Polkadot-Forum · 2023-02-17T10:21:52Z

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

https://forum.polkadot.network/t/multilocation-based-account-derivation/436/10

mustermeiszer · 2023-02-24T15:10:17Z

RE: Methods

My reasoning for having only

from_para_32
from_para_20
from_relay_32

Is that the user will for a given Account A always know that his alias account A' is consistent across all chains using this converter. Making integrations with UI and wallets easier, where as when having the from_child_* this breaks this assumption for the relay chain.

@gavofyork if having this is a blocker, I will adapt the PR.

mustermeiszer · 2023-03-08T07:34:59Z

@gavofyork or @KiChjang a last comment on the above and then we can probably finish this.

gavofyork · 2023-03-14T18:36:38Z

There may be unintended security consequences from having the same account ID used as the alias across multiple chains. I would certainly feel more confident if the aliases were different over each chain. That said, as long as it's strictly only for chains within the same security apparatus (i.e. not bridged locations), then it's probably fine to reuse the account alias.

gavofyork · 2023-03-14T18:50:31Z

To avoid the possibility of unintended collision.

This will break horribly with multi-level relay-chains: Account A on para index P controls account h(A, P) on all sibling, parent and child chains which use these primitives.

If you have multiple sets of parachains, then you'll naturally have index collisions. E.g.

         R
      /     \
    P1        P2
  /   \     /    \
P1.1 P1.2  P2.1 P2.2

Here, P1.2 would be index 2 to P1, P2.2 would be index 2 to P2 and P2 would be index 2 to R. Account h(A, 2) would therefore be controllable on P2 by both A on R, and A on P2.2; this might be very problematic if you trust the logic of R much more than P2.2.

This will never be a problem if you include the relationship within the hashed data.

mustermeiszer · 2023-03-15T14:50:41Z

Account h(A, 2) would therefore be controllable on P2 by both A on R, and A on P2.2; this might be very problematic if you trust the logic of R much more than P2.2.

The indices are wrong, but you are right:

A on R → controls h(A) on P1 and P2
A on P1.2 → controls h(A, 2) on P1 and P1.1
A on P2 → controls h(A, 2) on R and P1

→ account h(A, 2) on P1 is controllable by both P2 and P1.2.

TBH I have not thought about multi-relay-chain setup. ~~What do you mean with relationship in this context, how would that be extracted from the MultiLocation?~~ (edit)

Yes, we could include the parents-value in the conversion. WDYT?

girazoki · 2023-03-15T15:39:26Z

I think we can just use a different conversion for sibling or child parachains (similar to how the sovereign conversion is different since you append the "child" alias for child chains and "sibl" for chains).

A on P1.2 → controls h("child", A, 2) on P1 and h("sibl", A, 2)  P1.1
A on P2 → controls h("child", A, 2)  on R and P1 h("sibl", A, 2)

I think you wouldnt have colisions in this case

gavofyork · 2023-03-18T11:33:36Z

Yes; including the parents value is slightly more general, but either way would be fine.

crystalin · 2023-03-20T10:03:43Z

Hi @mustermeiszer, do you think you can do changes to adapt to the comments in the next days? We are waiting for this one to be merged before releasing a new runtime. If not, let me know and @girazoki can do the modifications
Thank you

mustermeiszer · 2023-03-20T14:26:19Z

@crystalin Its updated. Can be reviewed and merged if all are happy.

crystalin · 2023-03-20T15:49:44Z

@mustermeiszer it seems to fail passing the test: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/2554023#L1732

mustermeiszer · 2023-03-20T15:52:00Z

Docs did not format the tree correctly...

mustermeiszer · 2023-03-20T20:27:55Z

@crystalin the two jobs failing seem not connected to this pr. cc @bkchr

@gavofyork are you otherwise fine with the current state of the pr?

This reverts commit 4617597.

mustermeiszer · 2023-04-04T09:11:24Z

@gavofyork how do you feel about the current state? Can we merge it?

girazoki · 2023-04-12T09:00:28Z

@KiChjang @gavofyork is there anything else missing for this PR to be merged?

gavofyork

Looks good to me.

mustermeiszer · 2023-04-19T15:50:26Z

@KiChjang are you also happy with that? Can we merge that?

KiChjang

Ok for me, but needs an audit. Doesn't prevent this PR from landing however.

mustermeiszer · 2023-04-20T17:51:54Z

Cool. Let me know if you need anything for merging, but seems like it is up to you guys now.

KiChjang · 2023-04-24T13:25:14Z

bot merge

* master: malus: dont panic on missing validation data (#6952) Offences Migration v1: Removes `ReportsByKindIndex` (#7114) Fix stalling dispute coordinator. (#7125) Fix rolling session window (#7126) [ci] Update buildah command and version (#7128) Bump assigned_slots params (#6991) XCM: Remote account converter (#6662) Rework `dispute-coordinator` to use `RuntimeInfo` for obtaining session information instead of `RollingSessionWindow` (#6968) Revert default proof size back to 64 KB (#7115)

* master: (39 commits) malus: dont panic on missing validation data (#6952) Offences Migration v1: Removes `ReportsByKindIndex` (#7114) Fix stalling dispute coordinator. (#7125) Fix rolling session window (#7126) [ci] Update buildah command and version (#7128) Bump assigned_slots params (#6991) XCM: Remote account converter (#6662) Rework `dispute-coordinator` to use `RuntimeInfo` for obtaining session information instead of `RollingSessionWindow` (#6968) Revert default proof size back to 64 KB (#7115) update rocksdb to 0.20.1 (#7113) Reduce base proof size weight component to zero (#7081) PVF: Move PVF workers into separate crate (#7101) Companion for #13923 (#7111) update safe call filter (#7080) PVF: Don't dispute on missing artifact (#7011) XCM: Properly set the pricing for the DMP router (#6843) pvf: Update docs for PVF artifacts (#6551) Bump syn from 2.0.14 to 2.0.15 (#7093) Companion for substrate#13771 (#6983) Added Dwellir Nigeria bootnodes. (#7097) ...

* Draft remote account converter * Tests and doc comments * Adapt naming, tests fix * Try with prefix in account * Revert "Try with prefix in account" This reverts commit 4617597. * Adapt code and test to review suggestions * adapt to take into account parent in conversion * fix: docs * fix: try doc fix v2

Polkadot-Forum · 2023-06-07T16:10:58Z

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

https://forum.polkadot.network/t/multichain-friendly-account-abstraction/1298/19

mustermeiszer mentioned this pull request Feb 2, 2023

XCM Transact should support using multiple accounts on the target chain #6612

Closed

mustermeiszer marked this pull request as ready for review February 6, 2023 12:24

gavofyork reviewed Feb 14, 2023

View reviewed changes

mustermeiszer force-pushed the remote-account-converter branch from 93cabed to 97b3b88 Compare March 20, 2023 15:51

mustermeiszer added 6 commits March 25, 2023 11:17

Draft remote account converter

d80dbbe

Tests and doc comments

1e32ed7

Adapt naming, tests fix

d2b1b6e

Try with prefix in account

e711e7e

Revert "Try with prefix in account"

1536ba7

This reverts commit 4617597.

Adapt code and test to review suggestions

bbe0791

mustermeiszer added 2 commits March 25, 2023 11:17

fix: docs

e3c276f

fix: try doc fix v2

2a5f5b7

mustermeiszer force-pushed the remote-account-converter branch from 0e38b64 to 2a5f5b7 Compare March 25, 2023 10:18

bkontur mentioned this pull request Apr 6, 2023

[xcm] Foreign global consensus parachain LocationToAccountId converter #7016

Merged

gavofyork approved these changes Apr 18, 2023

View reviewed changes

KiChjang approved these changes Apr 20, 2023

View reviewed changes

paritytech-processbot bot merged commit 887e38f into paritytech:master Apr 24, 2023

librelois mentioned this pull request May 25, 2023

XCM: Derive remote accounts according to polkadot standards moonbeam-foundation/moonbeam#2315

Merged

albertov19 mentioned this pull request May 30, 2023

Need Remote Origins Calculator Based on 0.9.42 Standard Moonsong-Labs/xcm-tools#78

Merged

hbulgarini mentioned this pull request May 31, 2023

Configure Origins for Transact in a secure way paritytech/trappist#118

Open

olanod mentioned this pull request Jun 7, 2023

XCM specific configuration. virto-network/virto-node#242

Closed

github-actions bot mentioned this pull request Jun 15, 2023

Update substrate/polkadot/cumulus from v0.9.40 to v0.9.43 moonbeam-foundation/moonbeam#2354

Closed

darkforest0202 mentioned this pull request Jun 15, 2023

Set XCM: Remote account converter virto-network/virto-node#260

Closed

mustermeiszer mentioned this pull request Nov 20, 2023

XCM Absolute Location Account Derivation polkadot-fellows/RFCs#34

Open

imstar15 mentioned this pull request Dec 8, 2023

Adapts XCM remote account derivation to match Polkadot Standard AvaProtocol/OAK-blockchain#484

Merged

albertov19 mentioned this pull request Jul 24, 2024

Descend XCM origin instead of clearing it where possible polkadot-fellows/RFCs#109

Closed

	} => ForeignChainAliasAccount::<AccountId>::from_para_32(para_id, id),
	} => ForeignChainAliasAccount::<AccountId>::from_relay_32(para_id, id),

XCM: Remote account converter #6662

XCM: Remote account converter #6662

Conversation

mustermeiszer commented Feb 2, 2023

mustermeiszer commented Feb 2, 2023

mustermeiszer commented Feb 2, 2023

xlc commented Feb 3, 2023

mustermeiszer commented Feb 3, 2023 • edited Loading

mustermeiszer commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Polkadot-Forum commented Feb 17, 2023

mustermeiszer commented Feb 24, 2023

RE: Methods

mustermeiszer commented Mar 8, 2023

gavofyork commented Mar 14, 2023

gavofyork commented Mar 14, 2023 • edited Loading

mustermeiszer commented Mar 15, 2023 • edited Loading

girazoki commented Mar 15, 2023 • edited Loading

gavofyork commented Mar 18, 2023

crystalin commented Mar 20, 2023

mustermeiszer commented Mar 20, 2023

crystalin commented Mar 20, 2023

mustermeiszer commented Mar 20, 2023

mustermeiszer commented Mar 20, 2023

mustermeiszer commented Apr 4, 2023

girazoki commented Apr 12, 2023

gavofyork left a comment

Choose a reason for hiding this comment

mustermeiszer commented Apr 19, 2023

KiChjang left a comment

Choose a reason for hiding this comment

mustermeiszer commented Apr 20, 2023

KiChjang commented Apr 24, 2023

Polkadot-Forum commented Jun 7, 2023

mustermeiszer commented Feb 3, 2023 •

edited

Loading

gavofyork commented Mar 14, 2023 •

edited

Loading

mustermeiszer commented Mar 15, 2023 •

edited

Loading

girazoki commented Mar 15, 2023 •

edited

Loading