Explicitly note that existing `AccountIdConversion` is truncating and add fallible `try_into...` #10719

shawntabrizi · 2022-01-23T22:57:05Z

Closes: #8564
closes: #10812

Tired of running into this problem accidentally when writing tests.

The behavior of AccountIdConversion may occasionally lead to duplicate accounts if the seed provided to generate the account is bigger than the encoded AccountId type. In tests, where accounts are often u64, this can happen, and actually lead to hard to debug behaviors with account balances, and stuff like that.

This updates the API so that the existing behavior is explicitly mentioned, and also provides an alternative try_into... function which can handle such a truncation problem, and explicitly return an error when an infallible operation is not needed.

BREAKS API!

Migration is simple:

Whenever you use into_account or into_sub_account, you must now explicitly choose between into_account_truncating or try_into_account, and if you use the latter, handle the situation where we may return None.

Polkadot companion: paritytech/polkadot#4947
cumulus companion: paritytech/cumulus#1262

ggwpez

A test that the truncating works is missing. I can do that later if you are not at it already.

primitives/runtime/src/traits.rs

bkchr

Added some comment and what should be fixed.

I'm not really convinced that this will help in tests, because people will still not get the relation between the input and a short account id. However, it should make the code more correct 👍

bkchr · 2022-02-21T08:43:16Z

primitives/runtime/src/traits.rs

+			if b.len() > T::max_encoded_len() {
+				return None
+			};
+			let account = T::decode(&mut TrailingZeroInput(b))


IMO the check above is wrong, because if you for example have an enum max_encoded_len returns you the length of the biggest enum variant. This is wrong here, just imagine you are decoding the shortest variant and then you would not have consumed all bytes.

If you want to make it correct, you need to extend TrailingZeroInput to be able to check if the given input was consumed completely after having the type decoded.

The AccountId type has constant length, but in general I see that it does not work for all T.
So my proposal of adding a const_encoded_len could also fix this, right?

How do you know that the AccountId has a constant length? AccountId is some generic value that could be anything (one of the reasons for this pr). Just because Polkadot uses AccountId32, it doesn't mean that everyone uses this or is required to use this.

Yes therefore we could add a FixEncodedLen trait analogous to MaxEncodedLen that requires a type to always have the same encoding len.
PS: Probably still better to make it work even for non FixEncodedLen types 😄

+1 using MaxEncodedLen is IMO also wrong. Ideally, you just decode, and check how many bytes where actually used for decoding.

Maybe one trick here could be to decode, then re-encode, and see if we go back to the same bytes. A bit hacky and also assumes reversible encode/decode.

I did something like what @kianenigma suggested. I check the length of the bytes used for the seed, and check that the length of bytes of the account is greater or equal to, otherwise, we know some bytes were truncated.

Thanks. I also tried my luck on this but could not create a test that the function actually returns Some(). The AccountId type is u64, so an overflow is very likely anyway.

You can just create a mock environment where account id is 32 bytes?

stale · 2022-03-23T22:22:05Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

emostov · 2022-04-11T14:25:47Z

This is still useful

primitives/runtime/src/traits.rs

kianenigma

I am a huge fan of this PR and have hit this wall in the past, but I think https://github.com/paritytech/substrate/pull/10719/files#r810890249 should be resolved first.

shawntabrizi · 2022-05-17T20:33:17Z

bot merge

paritytech-processbot · 2022-05-17T20:33:24Z

Error: "Check reviews" status is not passing for paritytech/polkadot#4947

shawntabrizi · 2022-05-17T20:53:32Z

bot merge

paritytech-processbot · 2022-05-17T20:53:39Z

Error: "Check reviews" status is not passing for paritytech/cumulus#1262

shawntabrizi · 2022-05-17T21:02:35Z

bot merge

paritytech-processbot · 2022-05-17T21:02:42Z

Error: Github API says paritytech/polkadot#4947 is not mergeable

shawntabrizi · 2022-05-17T21:03:29Z

bot merge

paritytech-processbot · 2022-05-17T21:03:36Z

Error: "Check reviews" status is not passing for paritytech/polkadot#4947

shawntabrizi · 2022-05-17T21:11:54Z

bot merge

kianenigma · 2022-05-18T08:52:30Z

We should have a DecodeExact like this to achieve what we do in this PR cleaner

https://github.com/paritytech/parity-scale-codec/blob/master/src/decode_all.rs#L22

… add fallible `try_into...` (paritytech#10719) * note truncating, add fallible try_into * fmt * migrate all to `truncating` * typo * uno mas * Update primitives/runtime/src/traits.rs Co-authored-by: Kian Paimani <[email protected]> * check the bytes before and after are sensible * fmt * Update lib.rs * Update primitives/runtime/src/traits.rs Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Kian Paimani <[email protected]> Co-authored-by: Bastian Köcher <[email protected]>

note truncating, add fallible try_into

da8adfb

github-actions bot added the A0-please_review Pull request needs code review. label Jan 23, 2022

shawntabrizi added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D2-breaksapi D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. and removed A0-please_review Pull request needs code review. labels Jan 23, 2022

shawntabrizi added 4 commits January 23, 2022 23:01

fmt

fb51483

migrate all to truncating

d84fcbc

typo

d5f4708

uno mas

bcba8e3

shawntabrizi mentioned this pull request Feb 7, 2022

[BUG] PalletIds not uniquely determined by 8 symbol input #10812

Closed

Merge branch 'master' into shawntabrizi-improve-accountidconversion

e8b3f01

ggwpez self-requested a review February 9, 2022 13:48

ggwpez approved these changes Feb 9, 2022

View reviewed changes

primitives/runtime/src/traits.rs Outdated Show resolved Hide resolved

ggwpez mentioned this pull request Feb 19, 2022

Co #10719: Use into_account_truncating paritytech/polkadot#4947

Merged

bkchr reviewed Feb 21, 2022

View reviewed changes

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 23, 2022

stale bot closed this Apr 6, 2022

ggwpez mentioned this pull request Apr 8, 2022

Nomination Pools #10694

Merged

4 tasks

emostov reopened this Apr 11, 2022

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Apr 11, 2022

kianenigma reviewed May 3, 2022

View reviewed changes

primitives/runtime/src/traits.rs Outdated Show resolved Hide resolved

kianenigma reviewed May 3, 2022

View reviewed changes

paritytech-processbot bot merged commit 2823dc4 into master May 17, 2022

paritytech-processbot bot deleted the shawntabrizi-improve-accountidconversion branch May 17, 2022 21:12

shawntabrizi mentioned this pull request May 19, 2022

Fix Cumulus Compile (into_account_truncating) paritytech/cumulus#1277

Merged

This was referenced Jun 2, 2022

Upgrade to Polkadot 0.9.23 Phala-Network/phala-blockchain#812

Merged

Upgrade to Polkadot 0.9.23 Phala-Network/khala-parachain#116

Merged

gregdhill mentioned this pull request Jun 16, 2022

chore: upgrade to polkadot-v0.9.24 interlay/interbtc#646

Merged

github-actions bot mentioned this pull request Jun 22, 2022

Update substrate/polkadot/cumulus from v0.9.20 to v0.9.23 moonbeam-foundation/moonbeam#1623

Closed

jiguantong added a commit to darwinia-network/darwinia-messages-substrate that referenced this pull request Jun 30, 2022

Companion for paritytech/substrate#10719

c487b02

jiguantong added a commit to darwinia-network/darwinia-messages-substrate that referenced this pull request Jun 30, 2022

Companion for paritytech/substrate#10719

0308ec7

AurevoirXavier mentioned this pull request Jun 30, 2022

Prepare polkadot v0.9.23 for darwinia-parachain darwinia-network/darwinia-messages-substrate#151

Merged

boundless-forest mentioned this pull request Jul 1, 2022

Migrate to new s2s bridge darwinia-network/darwinia-messages-substrate#149

Merged

jiguantong added a commit to darwinia-network/darwinia-parachain that referenced this pull request Jul 1, 2022

Companion for paritytech/substrate#10719

1bdea06

jiguantong added a commit to darwinia-network/darwinia-parachain that referenced this pull request Jul 9, 2022

Companion for paritytech/substrate#10719

8c59d61

ghzlatarev mentioned this pull request Jul 27, 2022

Update deps from v0.9.22 to v0.9.26 Manta-Network/Manta#720

Merged

3 tasks

realnimish mentioned this pull request Aug 23, 2022

Update to polkadot-v0.9.27 Laguna-Chain/laguna-chain#51

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explicitly note that existing `AccountIdConversion` is truncating and add fallible `try_into...` #10719

Explicitly note that existing `AccountIdConversion` is truncating and add fallible `try_into...` #10719

shawntabrizi commented Jan 23, 2022 •

edited

Loading

ggwpez left a comment

bkchr left a comment

bkchr Feb 21, 2022

ggwpez Feb 21, 2022

bkchr Feb 21, 2022

ggwpez Feb 21, 2022 •

edited

Loading

kianenigma May 3, 2022 •

edited

Loading

shawntabrizi May 16, 2022

ggwpez May 16, 2022

shawntabrizi May 16, 2022

stale bot commented Mar 23, 2022

emostov commented Apr 11, 2022 •

edited

Loading

kianenigma left a comment

shawntabrizi commented May 17, 2022

paritytech-processbot bot commented May 17, 2022

shawntabrizi commented May 17, 2022

paritytech-processbot bot commented May 17, 2022

shawntabrizi commented May 17, 2022

paritytech-processbot bot commented May 17, 2022

shawntabrizi commented May 17, 2022

paritytech-processbot bot commented May 17, 2022

shawntabrizi commented May 17, 2022

kianenigma commented May 18, 2022

Explicitly note that existing AccountIdConversion is truncating and add fallible try_into... #10719

Explicitly note that existing AccountIdConversion is truncating and add fallible try_into... #10719

Conversation

shawntabrizi commented Jan 23, 2022 • edited Loading

ggwpez left a comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

bkchr Feb 21, 2022

Choose a reason for hiding this comment

ggwpez Feb 21, 2022

Choose a reason for hiding this comment

bkchr Feb 21, 2022

Choose a reason for hiding this comment

ggwpez Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

kianenigma May 3, 2022 • edited Loading

Choose a reason for hiding this comment

shawntabrizi May 16, 2022

Choose a reason for hiding this comment

ggwpez May 16, 2022

Choose a reason for hiding this comment

shawntabrizi May 16, 2022

Choose a reason for hiding this comment

stale bot commented Mar 23, 2022

emostov commented Apr 11, 2022 • edited Loading

kianenigma left a comment

Choose a reason for hiding this comment

shawntabrizi commented May 17, 2022

paritytech-processbot bot commented May 17, 2022

shawntabrizi commented May 17, 2022

paritytech-processbot bot commented May 17, 2022

shawntabrizi commented May 17, 2022

paritytech-processbot bot commented May 17, 2022

shawntabrizi commented May 17, 2022

paritytech-processbot bot commented May 17, 2022

shawntabrizi commented May 17, 2022

kianenigma commented May 18, 2022

Explicitly note that existing `AccountIdConversion` is truncating and add fallible `try_into...` #10719

Explicitly note that existing `AccountIdConversion` is truncating and add fallible `try_into...` #10719

shawntabrizi commented Jan 23, 2022 •

edited

Loading

ggwpez Feb 21, 2022 •

edited

Loading

kianenigma May 3, 2022 •

edited

Loading

emostov commented Apr 11, 2022 •

edited

Loading