Improve in using multiple instance of balances #6531

IAliceBobI · 2020-06-29T04:32:46Z

Add a INDEX field to the generated trait Instance to make pallets with multiple instants more easy to be used.

For example, If I want to make system module aware of which instance of balances module currently used,

Lines 989 to 991 in 4be954a

    
           let allow_death = existence_requirement == ExistenceRequirement::AllowDeath; 
        
           let allow_death = allow_death && system::Module::<T>::allow_death(transactor); 
        
           ensure!(allow_death || from_account.free >= ed, Error::<T, I>::KeepAlive);

maybe I can write the code:
system::Module::<T>::allow_death(I::INDEX, transactor);

parity-cla-bot · 2020-06-29T04:32:49Z

It looks like @chenwei767 signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

bkchr · 2020-06-29T07:29:08Z

I think you want to use this: https://github.com/paritytech/substrate/blob/master/frame/balances/src/tests_local.rs#L110-L115

IAliceBobI · 2020-06-29T08:55:27Z

Yes, I have used the altered version of:

	type AccountStore = StorageMapShim<
		super::Account<Test>,
		system::CallOnCreatedAccount<Test>,
		system::CallKillAccount<Test>,
		u64, super::AccountData<u64>
	>;

Currently all instances including the default instance share one system module, that is to say all instances share the same AccountInfo::refcount.

I am not sure whether sharing refcount is intentional.

substrate/frame/system/src/lib.rs

Lines 348 to 357 in 4be954a

    
           pub struct AccountInfo<Index, AccountData> { 
        
           	/// The number of transactions this account has sent. 
        
           	pub nonce: Index, 
        
           	/// The number of other modules that currently depend on this account's existence. The account 
        
           	/// cannot be reaped until this is zero. 
        
           	pub refcount: RefCount, 
        
           	/// The additional data that belongs to this account. Used to store the balance(s) in a lot of 
        
           	/// chains. 
        
           	pub data: AccountData, 
        
           }

I think Instance0~Instance15 should decouple from frame_system::AccountInfo or make system module aware of multiple instances of balances.

gui1117 · 2020-06-29T09:44:57Z

do you mean multiple instance use AccountStore: frame_system::Module<Runtime> ?
And system AccountData is pallet_balances::AccountData<Balance> ?

This can't work you basically put all kind of balances at the same place, all balances are thus mixed together, if you add funds to balances::instance1 to some account, this account will have the same amount of found in balances::instance2. This is not correct.

Each instance must store their accountData at their own places, maybe having system::AccoundData being a tuple or similar mecanism can works.

A simple way to test it is, to have one balance use u128 for currency and another one use u64, then compilation must fail.

gui1117

maybe I can write the code:
system::Module::<T>::allow_death(I::INDEX, transactor);

I don't see how instance index helps here, if system want to allow death to some modules only then it can use the type ModuleToIndex.

About the PR itself, maybe it can be used in some other context

bkchr · 2020-06-29T11:04:31Z

Currently all instances including the default instance share one system module, that is to say all instances share the same AccountInfo::refcount.

That is totally fine, why should the account be repeated if he still has funds?

IAliceBobI · 2020-06-29T11:04:31Z

do you mean multiple instance use AccountStore: frame_system::Module<Runtime> ?

I do not want multiple instance to use the same AccountStore: frame_system::Module<Runtime>.

And system AccountData is pallet_balances::AccountData<Balance> ?

Yes, it is.

I already have the flowing code:

impl balances::Trait<balances::Instance0> for Runtime {
	type Balance = Balance;
	type DustRemoval = ();
	type Event = Event;
	type ExistentialDeposit = OtherExistentialDeposit;
	type AccountStore = frame_support::traits::StorageMapShim<
		balances::Account<Self, balances::Instance0>,
		(),
		(),
		Self::AccountId,
		balances::AccountData<Self::Balance>,
	>;
}
impl balances::Trait<balances::DefaultInstance> for Runtime {
	type Balance = Balance;
	type DustRemoval = ();
	type Event = Event;
	type ExistentialDeposit = ExistentialDeposit;
	type AccountStore = System;
}

It works fine if I set OtherExistentialDeposit=0 and I do not want to kill dust account for Instance0

Suppose the refcount in system module of Alice is one due to a lock operation on DefaultInstance
and we set OtherExistentialDeposit=500 for Instance0.

If the balance of Alice in Instance0 falls below ED during a transfer operation
then the statement let allow_death = allow_death && system::Module::<T>::allow_death(transactor);
in balances module will stop Alice's account from being removed as dust from Instance0.

substrate/frame/balances/src/lib.rs

Lines 989 to 991 in 4be954a

    
           let allow_death = existence_requirement == ExistenceRequirement::AllowDeath; 
        
           let allow_death = allow_death && system::Module::<T>::allow_death(transactor); 
        
           ensure!(allow_death || from_account.free >= ed, Error::<T, I>::KeepAlive);

So, I want to add the INDEX to work around this problem:

In balances pallet:
let allow_death = allow_death && system::Module::<T>::allow_death(I::INDEX as InstanceIndex, transactor);

In system pallet:

trait Store for Module<T: Trait> as System {
    pub Account get(fn account):
    map hasher(blake2_128_concat) (InstanceIndex, T::AccountId) => AccountInfo<T::Index, T::AccountData>;
    ...
}

pub fn inc_ref(i: InstanceIndex, who: &T::AccountId) {
    Account::<T>::mutate((i, who.clone()), |a| a.refcount = a.refcount.saturating_add(1));
}

pub fn allow_death(i: InstanceIndex, who: &T::AccountId) -> bool {
    Account::<T>::get((i, who.clone())).refcount == 0
}

IAliceBobI · 2020-06-29T11:25:46Z

That is totally fine, why should the account be repeated if he still has funds?

I basically do not want all instances to share one AccountInfo::refcount.

bkchr · 2020-06-29T17:42:58Z

Why not?

What do you try to achieve?

IAliceBobI · 2020-06-30T03:06:52Z

Why not?

What do you try to achieve?

impl balances::Trait<balances::Instance0> for Runtime {
	type Balance = Balance;
	type DustRemoval = ();
	type Event = Event;
	type ExistentialDeposit = OtherExistentialDeposit;
	type AccountStore = frame_support::traits::StorageMapShim<
		balances::Account<Self, balances::Instance0>,
		(),
		(),
		Self::AccountId,
		balances::AccountData<Self::Balance>,
	>;
}
impl balances::Trait<balances::DefaultInstance> for Runtime {
	type Balance = Balance;
	type DustRemoval = ();
	type Event = Event;
	type ExistentialDeposit = ExistentialDeposit;
	type AccountStore = System;
}
construct_runtime!(
		Balances: balances::<DefaultInstance>::{Module, Call, Storage, Config<T>, Event<T>},
		Balances0 : balances::<Instance0 >::{Module, Call, Storage, Config<T>, Event<T>},
);

When using two instances simultaneously, the locks on DefaultInstance
should not affect Instance0.

substrate/frame/balances/src/lib.rs

Lines 989 to 991 in 4be954a

    
           let allow_death = existence_requirement == ExistenceRequirement::AllowDeath; 
        
           let allow_death = allow_death && system::Module::<T>::allow_death(transactor); 
        
           ensure!(allow_death || from_account.free >= ed, Error::<T, I>::KeepAlive);

A simple way to achieve this is we change the above code to the following:

let mut allow_death = existence_requirement == ExistenceRequirement::AllowDeath; 
if I::INDEX == T::SystemInstance::get() { 
    allow_death = allow_death && system::Module::<T>::allow_death(transactor); 
}
ensure!(allow_death || from_account.free >= ed, Error::<T, I>::KeepAlive);

and the fn update_locks in balances::Module needs to be modified accordingly.

xlc · 2020-06-30T03:47:01Z

Why not just set ExistentialDeposit to zero?

IAliceBobI · 2020-06-30T04:05:09Z

Why not just set ExistentialDeposit to zero?

I actually have already set ExistentialDeposit
to zero in my project and do not remove
dust accounts for Instance0 for now.

If we want to clean up the dust accounts,
this PR will be useful.

And maybe this PR can be used in some other context.

For example, If I write A multiple instance module A
and a single instance module B and a function in
A calls a function in B, then I::INDEX will be useful.
In A I can pass I::INDEX to B.

bkchr · 2020-06-30T08:04:20Z

All the other changes you propose here will require more changes in balances etc. But with the current changes we can not be sure that everything will work out as you expect and we maybe need other changes or whatever. I would be nice to have a simple POC impl that proofs your statements.

IAliceBobI · 2020-06-30T09:05:14Z

Good idea. I will think it over.

gavofyork · 2020-07-01T07:26:55Z

That is totally fine, why should the account be repeated if he still has funds?

I basically do not want all instances to share one AccountInfo::refcount.

This is fundamentally nonsensical. AccountInfo::refcount is coupled with the account as stored in the System module (i.e. the account nonce). Its definition is independent of Balances and the semantics of ED are subordinate to refcount/allow_death. (The allow_death refers to the death of the system AccountInfo entry, and not necessarily anything to do with anything in the balances module.)

However, I believe what you ultimately want to do is reasonable. Namely:

Two instances of balances.
One should be stored in system::AccountInfo and enforce/imply an ED.
The other should be stored privately (in its instance of balances pallet storage).

The question is what should happen for low balances of the latter instance. There are four options:

It should have no ED and not keep a reference. This is insecure unless there are some other restrictions that prevent account spam.
It should have some ED but not keep a reference. This is secure.
It should have no ED but keep a reference.
It should have some ED and keep a reference.

Both of the last two models are secure and make sense, but imply that any account with a balance in the latter instance would have a balance >= ED in the former instance, which the balances pallet doesn't currently support enforcement of.

IAliceBobI · 2020-07-02T09:41:07Z

It should have some ED and keep a reference.

Both of the last two models are secure and make sense, but imply that any account with a balance in the latter instance would have a balance >= ED in the former instance, which the balances pallet doesn't currently support enforcement of.

This is a very good analysis and I am going to implement the fourth option.

IAliceBobI · 2020-07-03T10:36:15Z

type AccountId = u64;
impl frame_system::Trait for Test {
	type AccountData = balances::AccountData<AccountId>;
}
impl Trait for Test {
	type ExistentialDeposit = ExistentialDeposit;
	type AccountStore = system::Module<Test>;
	type AccountRef = balances::SystemAccountRef<Test>;
}
impl Trait<balances::Instance0> for Test {
	type ExistentialDeposit = ExistentialDeposit;
	type AccountStore = StorageMapShim<
		balances::Account<Test, balances::Instance0>,
		balances::CallOnCreatedAccount<Test, balances::Instance0>,
		balances::CallKillAccount<Test, balances::Instance0>,
		AccountId,
		balances::AccountData<AccountId>
	>;
	type AccountRef = balances::LocalAccountRef<Test, A, balances::Instance0>;
}
type A = balances::Module<Test>;
type B = balances::Module<Test, balances::Instance0>;

For two instances of balances, A and B, configuration,
A is stored in system::AccountInfo and B is stored in local.

A have some ED and keep references in System.
B have some ED and keep references in local.
When create an account in B, the ref of A add one and
deposit an event NewAccount from B.
any account with a balance in B would have
a balance >= ED in A and ref >= 1 in storage of A.

gavofyork · 2020-07-04T10:05:17Z

I'm not quite convinced of the need for multiple separate reference counting. "Option 4" certainly doesn't require multiple reference counters. I would also prefer to avoid introducing additional types into system::Trait because of the huge codebase overhead it implies.

What use-case could you imagine where the system reference-count and the balances::Instance0 reference-count need to be independent?

The point of the reference count is to ensure that some ED exists in deposit for an account as long as a reference is held. With "Option 4", an ED in the secondary balances implies a reference in System and therefore an ED in the first balances. There is no need for a second, independent reference-counting system.

IAliceBobI · 2020-07-04T12:06:12Z

I'm not quite convinced of the need for multiple separate reference counting. "Option 4" certainly doesn't require multiple reference counters. I would also prefer to avoid introducing additional types into system::Trait because of the huge codebase overhead it implies.

Thanks for your advice. To avoid the overhead, maybe I can add a new method to Trait frame_support::traits::StoredMap to indicate that whether a balances instance is using the system storage or not.

What use-case could you imagine where the system reference-count and the balances::Instance0 reference-count need to be independent?

The point of the reference count is to ensure that some ED exists in deposit for an account as long as a reference is held. With "Option 4", an ED in the secondary balances implies a reference in System and therefore an ED in the first balances. There is no need for a second, independent reference-counting system.

I think you are right. One reference-counting system is enough. Setting a lock on a secondary account should not change the system reference-count or the local one. This was an oversight on my part.

IAliceBobI · 2020-07-06T02:35:13Z

type AccountId = u64;
impl frame_system::Trait for Test { type AccountData = super::AccountData<AccountId>; }
impl Trait for Test {
	type ExistentialDeposit = ExistentialDeposit;
	type AccountStore = system::Module<Test>;
}
type A = crate::Module<Test>;

impl Trait<crate::Instance0> for Test {
	type ExistentialDeposit = ExistentialDeposit;
	type AccountStore = StorageMapShim<
		super::Account<Test, crate::Instance0>,
		crate::CallOnCreatedAccount<Test, crate::Instance0>,
		crate::CallKillAccount<Test, crate::Instance0>,
		AccountId, super::AccountData<AccountId>>;
}
type B = crate::Module<Test, crate::Instance0>;

parameter_types! { pub const UsingSystemAccount: bool = true; }
impl Trait for Test {
	type ExistentialDeposit = ExistentialDeposit;
	type AccountStore = StorageMapShim<
		super::Account<Test>,
		system::CallOnCreatedAccount<Test>,
		system::CallKillAccount<Test>,
		AccountId, super::AccountData<AccountId>,
		UsingSystemAccount>;
}
type C = crate::Module<Test>;

Can not configure both A and C.
Can not use both B and C.
Use only A: do not need to change your code.
Use only B: you can not create any account, even in genesis config.
Use only C: need to define a new type in parameter_types! additionally. (very few cases)
Use A and B: "Option 4". (There is only one reference-counting system)

IAliceBobI marked this pull request as ready for review June 29, 2020 04:33

gui1117 reviewed Jun 29, 2020

View reviewed changes

IAliceBobI marked this pull request as draft June 30, 2020 09:01

gavofyork added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jul 1, 2020

IAliceBobI changed the title ~~Add INDEX to Instance~~ Allow balances local refcount Jul 3, 2020

IAliceBobI marked this pull request as ready for review July 3, 2020 10:38

IAliceBobI requested review from kianenigma, andresilva and pepyakin as code owners July 3, 2020 14:35

This was referenced Jul 4, 2020

Allow balances local refcount paritytech/polkadot#1358

Closed

Allow balances local refcount paritytech/polkadot#1359

Closed

IAliceBobI marked this pull request as draft July 4, 2020 12:17

IAliceBobI changed the title ~~Allow balances local refcount~~ Improve in using multiple instance of balances Jul 5, 2020

IAliceBobI marked this pull request as ready for review July 5, 2020 05:49

Improve in using multiple instance of balances

8525e88

IAliceBobI closed this Aug 2, 2020

IAliceBobI deleted the multi-instants branch September 24, 2020 02:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve in using multiple instance of balances #6531

Improve in using multiple instance of balances #6531

IAliceBobI commented Jun 29, 2020 •

edited

Loading

parity-cla-bot commented Jun 29, 2020

bkchr commented Jun 29, 2020

IAliceBobI commented Jun 29, 2020 •

edited

Loading

gui1117 commented Jun 29, 2020

gui1117 left a comment

bkchr commented Jun 29, 2020

IAliceBobI commented Jun 29, 2020 •

edited

Loading

IAliceBobI commented Jun 29, 2020

bkchr commented Jun 29, 2020

IAliceBobI commented Jun 30, 2020

xlc commented Jun 30, 2020

IAliceBobI commented Jun 30, 2020 •

edited

Loading

bkchr commented Jun 30, 2020

IAliceBobI commented Jun 30, 2020

gavofyork commented Jul 1, 2020 •

edited

Loading

IAliceBobI commented Jul 2, 2020

IAliceBobI commented Jul 3, 2020

gavofyork commented Jul 4, 2020 •

edited

Loading

IAliceBobI commented Jul 4, 2020 •

edited

Loading

IAliceBobI commented Jul 6, 2020 •

edited

Loading

	let allow_death = existence_requirement == ExistenceRequirement::AllowDeath;
	let allow_death = allow_death && system::Module::<T>::allow_death(transactor);
	ensure!(allow_death \|\| from_account.free >= ed, Error::<T, I>::KeepAlive);

Improve in using multiple instance of balances #6531

Improve in using multiple instance of balances #6531

Conversation

IAliceBobI commented Jun 29, 2020 • edited Loading

parity-cla-bot commented Jun 29, 2020

bkchr commented Jun 29, 2020

IAliceBobI commented Jun 29, 2020 • edited Loading

gui1117 commented Jun 29, 2020

gui1117 left a comment

Choose a reason for hiding this comment

bkchr commented Jun 29, 2020

IAliceBobI commented Jun 29, 2020 • edited Loading

IAliceBobI commented Jun 29, 2020

bkchr commented Jun 29, 2020

IAliceBobI commented Jun 30, 2020

xlc commented Jun 30, 2020

IAliceBobI commented Jun 30, 2020 • edited Loading

bkchr commented Jun 30, 2020

IAliceBobI commented Jun 30, 2020

gavofyork commented Jul 1, 2020 • edited Loading

IAliceBobI commented Jul 2, 2020

IAliceBobI commented Jul 3, 2020

gavofyork commented Jul 4, 2020 • edited Loading

IAliceBobI commented Jul 4, 2020 • edited Loading

IAliceBobI commented Jul 6, 2020 • edited Loading

IAliceBobI commented Jun 29, 2020 •

edited

Loading

IAliceBobI commented Jun 29, 2020 •

edited

Loading

IAliceBobI commented Jun 29, 2020 •

edited

Loading

IAliceBobI commented Jun 30, 2020 •

edited

Loading

gavofyork commented Jul 1, 2020 •

edited

Loading

gavofyork commented Jul 4, 2020 •

edited

Loading

IAliceBobI commented Jul 4, 2020 •

edited

Loading

IAliceBobI commented Jul 6, 2020 •

edited

Loading