into_sub_account should check seed length

[shawntabrizi]

Currently, into_sub_account directly encodes the seed data into the generated account id bytes.

We do not check that the seed data is greater than the length of the account id type, and thus do not check that some of the data may be truncated and not used to seed an account. This can obviously be bad since all of the seed information should be used to generate a unique account, and any truncation could lead to unintended account collisions.

into_sub_account should return a result, and only generate an account if the seed data would fit completely into the account id space.

Otherwise, hash_into_sub_account can be introduced which takes a hash of the seed data and uses that to generate as much data as needed for the account.

We must review all uses of into_sub_account to make sure they would not suddenly stop working.

[xlc]

Yeah we had some issues in our test that using u64 as AccountId type and confused why the test isn't working...

[hirschenberger]

I changed into_sub_account to return an Option, (or should it really be a Result, what's the Error then?). Maybe we can rename the function to try_into_sub_account to be consistent and the into_sub_account be the one that's infallible and is hashing the seed.

For the hashing function, what hasher should be used?

And there may also be the case, that the hash value does not fit into the AccountId type. As I understand you correctly, the AccountId should just be filled up with the hashed seed, to reduce the probability of collisions, but may also be left out at all, as in the previous implementation?

[shawntabrizi]

You should be able to pass a generic hashing trait so the end user can pick the hasher.

Yeah, if the hash value is too much, we put as much as we can into the AccountId.

Is there some code I can look at?

[hirschenberger]

@shawntabrizi I opened a PR #8672

[gui1117]

I also see that into_sub_account will always return the same default account in case AccountId can't decode the byte slice.

substrate/primitives/runtime/src/traits.rs

Lines 1146 to 1150 in 37e97cf

	fn into_sub_account<S: Encode>(&self, sub: S) -> T {
	(Id::TYPE_ID, self, sub).using_encoded(|b|
	T::decode(&mut TrailingZeroInput(b))
	).unwrap_or_default()
	}

I think we should create a proper trait to be able to generate different account id safely.

For example we could a new trait to frame_system::Config::AccountId: GenerateAccountFromSeed

trait GenerateAccountFromSeed {
fn from_seed(s: &[u8; 8]) -> Self;
fn pallet_sub_account<P: PalletInfoAccess>(seed: &[u8]) -> Self {
  Self::from_seed(twox64("pllt"++encode(P::index)++Some(seed))))
}
fn pallet_account<P: PalletInfoAccess>(seed: &[u8]) -> Self {
  Self::from_seed(twox64("pllt"++encode(P::index)++None))))
}
}

and implement it for u64 and u128.
Or maybe another trait which allows more flexibility.

[hirschenberger]

This looks like a safe alternative. How about generalizing with the Encode-Trait for the seed?
And does pallet_account need a seed parameter?

trait GenerateAccountFromSeed {
    fn from_seed<S: Encode>(s: S) -> Self;
    fn pallet_sub_account<S: Encode, P: PalletInfoAccess>(seed: S) -> Self {
        Self::from_seed(twox64("pllt"++ encode(P::index) ++ encode(seed)))
    }
    fn pallet_account<P: PalletInfoAccess>() -> Self {
        Self::from_seed(twox64("pllt"++encode(P::index))))
    }
}

The drawback of hashing the whole representation is that it's not reversible/inspectable anymore.

What are the implications on changing the underlying representation for existing accounts or code that maybe rely on that representation? Is there some migration necessary?

[shawntabrizi]

I dont think this is exactly the direction we want to go.

In this case, the account for a pallet will change depending on the order a user puts the pallets in their runtime.

Currently, we have it that the treasury account for all substrate chains are the same account, which makes it easy for exchanges and block explorers to track those well known accounts.

I think the goal here is to keep the same underlying logic about PalletId, but simply make sure that there are appropriate error handling when a user programs too much value into the seed data.

[xlc]

we can explicitly set pallet index so the order shouldn't be an issue.

we should not assume treasury account address on different chains are the same, because there will be exceptions and I don't think that's a good idea anyway.

[gui1117]

I mean treasury doesn't have to use pallet_sub_account. My proposition was in order to cover the case paritytech/polkadot-sdk#324 at the same time

[hirschenberger]

This Issue and PR seems stuck. How can we make progress here?

I see three directions from here:

• Adding the hashed seed without any safety nets that relies on the the user to take care that everything fits. @shawntabrizi
• An implementation that reports missing the addition of at least a part of the (hashed) seed. @hirschenberger
• A version that guarantees the addition of the seed. @thiolliere

[gui1117]

if we have into_account or into_sub_account returning a result, and if they can return Err in case account id is u64 I think this is weird. Like a pallet would stop working depending on how the account id is configured.

Pallets can then write a test in integrity_test ensuring that they are able to generate their account ids.

So if into_sub_account returns a result, the code will look like pallet_id.into_account().expect("ensured by integrity tests");
And every pallet will have to write some integrity test when they make use of pallet id.

I don't mind it but it is not very handy.

Instead I think it is better to have a proper trait bound on frame_system::Config::AccountId which allows to generate those id without issue. using some hash or ensuring no more than 8 byte of into is given, and that AccountId is more than 8 byte.

This issue paritytech/polkadot-sdk#324 is about making pallet id not configurable but hardcoded. The only unique identifier of pallet are currently their name and their index.

In general the pallets should then put those account into metadata if they want third party to interact with it, I think. (if treasury still wants a specific account id then we can still give the final account id in treasury::Config)

[stale]

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.

[hirschenberger]

#8672 is still open for discussion

[gui1117]

Pallet contract generates some account id by bounding T::AccountId: UncheckedFrom<T::Hash> and using the cryptographic hash algorithm T::Hashing to generate a unique account.

Contrary to pallet contract, for now we use this into_sub_account method only to generate account with seed which is not influenced by users. (in pallet bounties we put in the seed the bounty index which comes from an incremented storage).

I see 2 solutions:

Solution 1: sub account is generated from non-cryptographic or cryptographic hash depending on usecase.

For this solution we force frame_system::Config::AccountId to be at least 16 bytes. (e.g. we can use u128 in test, it is as handy as u64).

• if a pallet want to generate an account id with a seed not influenced by users then it uses: twox128("pallet" ++ $pallet_name ++ seed)
  e.g. for the pallet named "Balances" with the seed "abc" twox128(b"palletBalancesabc").
• otherwise if the seed can be influenced by users, then we can do:
  • same but with blake2_128 if it is considered safe enough
  • not support it as it is currently the case
  • require frame_system to give a safe hasher which result in an AccountId, for test this hasher can be not safe, and we can continue using u128.

Solution 2: sub account is from trusted input (not influenced by user) and limited to 4 byte:

For this solution we force frame_system::Config::AccountId to be at least 16 bytes. (e.g. we can use u128 in test, it is as handy as u64).

• we do not support seed that can be influeced by users
• seed must be 4 bytes
• we generate as such: "pllt" ++ twox_64($palle_name) ++ seed.

So it is exactly 16 bytes because we constraint the seed to be 4 bytes.

Notes

both solution are annoying because it uses a hash and thus from the account id it is more difficult to see what is the meaning.

• for solution 2 we can put twox_64($pallet_name) in the metadata so user can easily identify those account
• for solution 1 I don't any exhaustive way to put the accounts in the metadata:
  • for fixed account like in society with the seed "payout" we can put them in extra_constant
  • for the dynamic ones like in bounties we can't do that.

[kianenigma]

cc @emostov we ran into this as well.