From 27f08fec3a5251ddc5eb6d0e28a193023fafaf84 Mon Sep 17 00:00:00 2001
From: Shawn Tabrizi <shawntabrizi@gmail.com>
Date: Tue, 17 May 2022 17:12:02 -0400
Subject: [PATCH] Explicitly note that existing `AccountIdConversion` is
truncating and add fallible `try_into...` (#10719)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* 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 <5588131+kianenigma@users.noreply.github.com>
* check the bytes before and after are sensible
* fmt
* Update lib.rs
* Update primitives/runtime/src/traits.rs
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
---
substrate/frame/bounties/src/lib.rs | 4 +-
substrate/frame/child-bounties/src/lib.rs | 2 +-
substrate/frame/lottery/src/lib.rs | 2 +-
substrate/frame/nomination-pools/src/lib.rs | 4 +-
substrate/frame/society/src/lib.rs | 4 +-
substrate/frame/tips/src/lib.rs | 2 +-
substrate/frame/treasury/src/lib.rs | 2 +-
substrate/primitives/runtime/src/traits.rs | 89 ++++++++++++++++---
.../frame-utilities-cli/src/pallet_id.rs | 2 +-
9 files changed, 87 insertions(+), 24 deletions(-)
diff --git a/substrate/frame/bounties/src/lib.rs b/substrate/frame/bounties/src/lib.rs
index dfeef36a1fa..b01a36b430a 100644
--- a/substrate/frame/bounties/src/lib.rs
+++ b/substrate/frame/bounties/src/lib.rs
@@ -799,14 +799,14 @@ impl<T: Config> Pallet<T> {
/// This actually does computation. If you need to keep using it, then make sure you cache the
/// value and only call this once.
pub fn account_id() -> T::AccountId {
- T::PalletId::get().into_account()
+ T::PalletId::get().into_account_truncating()
}
/// The account ID of a bounty account
pub fn bounty_account_id(id: BountyIndex) -> T::AccountId {
// only use two byte prefix to support 16 byte account id (used by test)
// "modl" ++ "py/trsry" ++ "bt" is 14 bytes, and two bytes remaining for bounty index
- T::PalletId::get().into_sub_account(("bt", id))
+ T::PalletId::get().into_sub_account_truncating(("bt", id))
}
fn create_bounty(
diff --git a/substrate/frame/child-bounties/src/lib.rs b/substrate/frame/child-bounties/src/lib.rs
index a8496d4e7af..4f25fdcf890 100644
--- a/substrate/frame/child-bounties/src/lib.rs
+++ b/substrate/frame/child-bounties/src/lib.rs
@@ -786,7 +786,7 @@ impl<T: Config> Pallet<T> {
// This function is taken from the parent (bounties) pallet, but the
// prefix is changed to have different AccountId when the index of
// parent and child is same.
- T::PalletId::get().into_sub_account(("cb", id))
+ T::PalletId::get().into_sub_account_truncating(("cb", id))
}
fn create_child_bounty(
diff --git a/substrate/frame/lottery/src/lib.rs b/substrate/frame/lottery/src/lib.rs
index bc96a029a42..02df65a3336 100644
--- a/substrate/frame/lottery/src/lib.rs
+++ b/substrate/frame/lottery/src/lib.rs
@@ -389,7 +389,7 @@ impl<T: Config> Pallet<T> {
/// This actually does computation. If you need to keep using it, then make sure you cache the
/// value and only call this once.
pub fn account_id() -> T::AccountId {
- T::PalletId::get().into_account()
+ T::PalletId::get().into_account_truncating()
}
/// Return the pot account and amount of money in the pot.
diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs
index cdc3bdaf343..ff77949cf69 100644
--- a/substrate/frame/nomination-pools/src/lib.rs
+++ b/substrate/frame/nomination-pools/src/lib.rs
@@ -2018,14 +2018,14 @@ impl<T: Config> Pallet<T> {
/// Create the main, bonded account of a pool with the given id.
pub fn create_bonded_account(id: PoolId) -> T::AccountId {
- T::PalletId::get().into_sub_account((AccountType::Bonded, id))
+ T::PalletId::get().into_sub_account_truncating((AccountType::Bonded, id))
}
/// Create the reward account of a pool with the given id.
pub fn create_reward_account(id: PoolId) -> T::AccountId {
// NOTE: in order to have a distinction in the test account id type (u128), we put
// account_type first so it does not get truncated out.
- T::PalletId::get().into_sub_account((AccountType::Reward, id))
+ T::PalletId::get().into_sub_account_truncating((AccountType::Reward, id))
}
/// Get the member with their associated bonded and reward pool.
diff --git a/substrate/frame/society/src/lib.rs b/substrate/frame/society/src/lib.rs
index 645a0b8ba17..2973ecd6809 100644
--- a/substrate/frame/society/src/lib.rs
+++ b/substrate/frame/society/src/lib.rs
@@ -1726,7 +1726,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
/// This actually does computation. If you need to keep using it, then make sure you cache the
/// value and only call this once.
pub fn account_id() -> T::AccountId {
- T::PalletId::get().into_account()
+ T::PalletId::get().into_account_truncating()
}
/// The account ID of the payouts pot. This is where payouts are made from.
@@ -1734,7 +1734,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
/// This actually does computation. If you need to keep using it, then make sure you cache the
/// value and only call this once.
pub fn payouts() -> T::AccountId {
- T::PalletId::get().into_sub_account(b"payouts")
+ T::PalletId::get().into_sub_account_truncating(b"payouts")
}
/// Return the duration of the lock, in blocks, with the given number of members.
diff --git a/substrate/frame/tips/src/lib.rs b/substrate/frame/tips/src/lib.rs
index f43545e2b4f..e1c7b5e77c0 100644
--- a/substrate/frame/tips/src/lib.rs
+++ b/substrate/frame/tips/src/lib.rs
@@ -467,7 +467,7 @@ impl<T: Config> Pallet<T> {
/// This actually does computation. If you need to keep using it, then make sure you cache the
/// value and only call this once.
pub fn account_id() -> T::AccountId {
- T::PalletId::get().into_account()
+ T::PalletId::get().into_account_truncating()
}
/// Given a mutable reference to an `OpenTip`, insert the tip into it and check whether it
diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs
index d080d346ba3..419970ed18a 100644
--- a/substrate/frame/treasury/src/lib.rs
+++ b/substrate/frame/treasury/src/lib.rs
@@ -437,7 +437,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
/// This actually does computation. If you need to keep using it, then make sure you cache the
/// value and only call this once.
pub fn account_id() -> T::AccountId {
- T::PalletId::get().into_account()
+ T::PalletId::get().into_account_truncating()
}
/// The needed bond for a proposal whose spend is `value`.
diff --git a/substrate/primitives/runtime/src/traits.rs b/substrate/primitives/runtime/src/traits.rs
index 393de086f3c..1b21e7c65dd 100644
--- a/substrate/primitives/runtime/src/traits.rs
+++ b/substrate/primitives/runtime/src/traits.rs
@@ -1243,9 +1243,16 @@ impl<'a> codec::Input for TrailingZeroInput<'a> {
/// This type can be converted into and possibly from an AccountId (which itself is generic).
pub trait AccountIdConversion<AccountId>: Sized {
- /// Convert into an account ID. This is infallible.
- fn into_account(&self) -> AccountId {
- self.into_sub_account(&())
+ /// Convert into an account ID. This is infallible, and may truncate bytes to provide a result.
+ /// This may lead to duplicate accounts if the size of `AccountId` is less than the seed.
+ fn into_account_truncating(&self) -> AccountId {
+ self.into_sub_account_truncating(&())
+ }
+
+ /// Convert into an account ID, checking that all bytes of the seed are being used in the final
+ /// `AccountId` generated. If any bytes are dropped, this returns `None`.
+ fn try_into_account(&self) -> Option<AccountId> {
+ self.try_into_sub_account(&())
}
/// Try to convert an account ID into this type. Might not succeed.
@@ -1253,8 +1260,8 @@ pub trait AccountIdConversion<AccountId>: Sized {
Self::try_from_sub_account::<()>(a).map(|x| x.0)
}
- /// Convert this value amalgamated with the a secondary "sub" value into an account ID. This is
- /// infallible.
+ /// Convert this value amalgamated with the a secondary "sub" value into an account ID,
+ /// truncating any unused bytes. This is infallible.
///
/// NOTE: The account IDs from this and from `into_account` are *not* guaranteed to be distinct
/// for any given value of `self`, nor are different invocations to this with different types
@@ -1262,19 +1269,45 @@ pub trait AccountIdConversion<AccountId>: Sized {
/// - `self.into_sub_account(0u32)`
/// - `self.into_sub_account(vec![0u8; 0])`
/// - `self.into_account()`
- fn into_sub_account<S: Encode>(&self, sub: S) -> AccountId;
+ ///
+ /// Also, if the seed provided to this function is greater than the number of bytes which fit
+ /// into this `AccountId` type, then it will lead to truncation of the seed, and potentially
+ /// non-unique accounts.
+ fn into_sub_account_truncating<S: Encode>(&self, sub: S) -> AccountId;
+
+ /// Same as `into_sub_account_truncating`, but ensuring that all bytes of the account's seed are
+ /// used when generating an account. This can help guarantee that different accounts are unique,
+ /// besides types which encode the same as noted above.
+ fn try_into_sub_account<S: Encode>(&self, sub: S) -> Option<AccountId>;
/// Try to convert an account ID into this type. Might not succeed.
fn try_from_sub_account<S: Decode>(x: &AccountId) -> Option<(Self, S)>;
}
-/// Format is TYPE_ID ++ encode(parachain ID) ++ 00.... where 00... is indefinite trailing zeroes to
+/// Format is TYPE_ID ++ encode(sub-seed) ++ 00.... where 00... is indefinite trailing zeroes to
/// fill AccountId.
impl<T: Encode + Decode, Id: Encode + Decode + TypeId> AccountIdConversion<T> for Id {
- fn into_sub_account<S: Encode>(&self, sub: S) -> T {
+ // Take the `sub` seed, and put as much of it as possible into the generated account, but
+ // allowing truncation of the seed if it would not fit into the account id in full. This can
+ // lead to two different `sub` seeds with the same account generated.
+ fn into_sub_account_truncating<S: Encode>(&self, sub: S) -> T {
(Id::TYPE_ID, self, sub)
.using_encoded(|b| T::decode(&mut TrailingZeroInput(b)))
- .expect("`AccountId` type is never greater than 32 bytes; qed")
+ .expect("All byte sequences are valid `AccountIds`; qed")
+ }
+
+ // Same as `into_sub_account_truncating`, but returns `None` if any bytes would be truncated.
+ fn try_into_sub_account<S: Encode>(&self, sub: S) -> Option<T> {
+ let encoded_seed = (Id::TYPE_ID, self, sub).encode();
+ let account = T::decode(&mut TrailingZeroInput(&encoded_seed))
+ .expect("All byte sequences are valid `AccountIds`; qed");
+ // If the `account` generated has less bytes than the `encoded_seed`, then we know that
+ // bytes were truncated, and we return `None`.
+ if encoded_seed.len() <= account.encoded_size() {
+ Some(account)
+ } else {
+ None
+ }
}
fn try_from_sub_account<S: Decode>(x: &T) -> Option<(Self, S)> {
@@ -1652,6 +1685,13 @@ mod tests {
let _ = s.verify(&[0u8; 100][..], &Public::unchecked_from([0; 32]));
}
+ #[derive(Encode, Decode, Default, PartialEq, Debug)]
+ struct U128Value(u128);
+ impl super::TypeId for U128Value {
+ const TYPE_ID: [u8; 4] = [0x0d, 0xf0, 0x0d, 0xf0];
+ }
+ // f00df00d
+
#[derive(Encode, Decode, Default, PartialEq, Debug)]
struct U32Value(u32);
impl super::TypeId for U32Value {
@@ -1669,9 +1709,19 @@ mod tests {
type AccountId = u64;
#[test]
- fn into_account_should_work() {
- let r: AccountId = U32Value::into_account(&U32Value(0xdeadbeef));
+ fn into_account_truncating_should_work() {
+ let r: AccountId = U32Value::into_account_truncating(&U32Value(0xdeadbeef));
+ assert_eq!(r, 0x_deadbeef_cafef00d);
+ }
+
+ #[test]
+ fn try_into_account_should_work() {
+ let r: AccountId = U32Value::try_into_account(&U32Value(0xdeadbeef)).unwrap();
assert_eq!(r, 0x_deadbeef_cafef00d);
+
+ // u128 is bigger than u64 would fit
+ let maybe: Option<AccountId> = U128Value::try_into_account(&U128Value(u128::MAX));
+ assert!(maybe.is_none());
}
#[test]
@@ -1681,9 +1731,22 @@ mod tests {
}
#[test]
- fn into_account_with_fill_should_work() {
- let r: AccountId = U16Value::into_account(&U16Value(0xc0da));
+ fn into_account_truncating_with_fill_should_work() {
+ let r: AccountId = U16Value::into_account_truncating(&U16Value(0xc0da));
+ assert_eq!(r, 0x_0000_c0da_f00dcafe);
+ }
+
+ #[test]
+ fn try_into_sub_account_should_work() {
+ let r: AccountId = U16Value::try_into_account(&U16Value(0xc0da)).unwrap();
assert_eq!(r, 0x_0000_c0da_f00dcafe);
+
+ let maybe: Option<AccountId> = U16Value::try_into_sub_account(
+ &U16Value(0xc0da),
+ "a really large amount of additional encoded information which will certainly overflow the account id type ;)"
+ );
+
+ assert!(maybe.is_none())
}
#[test]
diff --git a/substrate/utils/frame/frame-utilities-cli/src/pallet_id.rs b/substrate/utils/frame/frame-utilities-cli/src/pallet_id.rs
index c0a221d4bcd..4ad82a01c24 100644
--- a/substrate/utils/frame/frame-utilities-cli/src/pallet_id.rs
+++ b/substrate/utils/frame/frame-utilities-cli/src/pallet_id.rs
@@ -72,7 +72,7 @@ impl PalletIdCmd {
"Cannot convert argument to palletid: argument should be 8-character string"
})?;
- let account_id: R::AccountId = PalletId(id_fixed_array).into_account();
+ let account_id: R::AccountId = PalletId(id_fixed_array).into_account_truncating();
with_crypto_scheme!(
self.crypto_scheme.scheme,
--
GitLab