contracts: Don't rely on reserved balances keeping an account alive

[athei]

Motivation

Up until now we relied on automatic storage deposits to contracts to keep a contract's account from being reaped. Those storage deposit were reserved in order to prevent the contract from spending it. A nice side effect was that we could use it to guarantee the existence of a contract's account as long as the contract itself existed.

With #12951 reserved balance does no longer contribute to the existential deposit. We need to make modifications to stay compatible.

High Level Changes

The high level changes we made need to address the following needs.

Keep the contract from spending the deposit

• The storage deposit will not be send to the contracts account but to a different account. Since this account is not controlled by anyone it is safe to leave the balance as free. Separating the accounts will also make sure that no slashing bugs in other pallets will break our assumptions.
• We add a consumer reference to this new deposit account. This is not strictly necessary as no one can spend those funds , though.

Make sure the contract's account does not get reaped

• On contract instantiation we always collect the ed from origin (as part of the storage deposit) and use it to pull the contract's account into existence.
• We take a consumer reference to the contract's account. This makes sure that any attempt by the contract to send the ed away will fail the transaction in question. Even slashing will not remove the account and leave the ed. Not sure if any security risks can arise from this.

Why the huge diff?

The diff seems to be quite big for something that seems like a small change. The reasons are the following.

Test changes

Always sending the ed to a new contract broke a lost of tests and created a lot of changes. Especially since it affected the emitted events.

Refactoring of storage.rs

Since I needed to do some changes to the file which had rather old code I took the opportunity to move some code around. I also made all the fields private when this didn't require even more refactoring. In the future they should all become private.

Moved address derivation into its own module

This began to clutter lib.rs too much.

[gavofyork]

The consumer reference on the deposit account means that it cannot have its balance reduced below ED (until the consumer reference is lifted). Is this acceptable/handled properly?

Unless there are no production chains using this pallet (and I believe there are) then there will need to be migration code to transfer the existing storage deposit into the new account. Instead of doing it all at once, it could be that it gets transferred lazily, either when the contract is used or when the contract account is pinged by a "upgrade-this-account" extrinsic with Pays::No if the account was able to be upgraded.

[athei]

The consumer reference on the deposit account means that it cannot have its balance reduced below ED (until the consumer reference is lifted). Is this acceptable/handled properly?

Yes. The base deposit (just for the contract itself) is > ed and is only removed when the contract self destructs. The account can't be used for any slashable activity so there is no way it can be reduced below the ed without the contract being removed (which also removes the reference).

Unless there are no production chains using this pallet (and I believe there are) then there will need to be migration code to transfer the existing storage deposit into the new account.

There is shiden (astar kusma parachain) which is relying on my migrations for some time. Also Astar and Azero are scheduled to launch the pallet on their mainnet with the latest polkadot release.

Instead of doing it all at once, it could be that it gets transferred lazily, either when the contract is used or when the contract account is pinged by a "upgrade-this-account" extrinsic with Pays::No if the account was able to be upgraded.

This "upgraded-this-account" logic is only available after #12951 is merged, right?

[agryaznov]

Some docs need an update, like e.g. here:

substrate/frame/contracts/primitives/src/lib.rs

Lines 50 to 54 in 50ab7fb

	/// How much balance was deposited and reserved during execution in order to pay for storage.
	///
	/// The storage deposit is never actually charged from the caller in case of [`Self::result`]
	/// is `Err`. This is because on error all storage changes are rolled back.
	pub storage_deposit: StorageDeposit<Balance>,

and here:

substrate/frame/contracts/primitives/src/lib.rs

Lines 158 to 169 in 50ab7fb

	pub enum StorageDeposit<Balance> {
	/// The transaction reduced storage consumption.
	///
	/// This means that the specified amount of balance was transferred from the involved
	/// contracts to the call origin.
	Refund(Balance),
	/// The transaction increased overall storage usage.
	///
	/// This means that the specified amount of balance was transferred from the call origin
	/// to the contracts involved.
	Charge(Balance),
	}

Also, this PR does not affect the way amount is reserved (and unreserved) on the contract code uploader account:

substrate/frame/contracts/src/wasm/code_cache.rs

Line 91 in 50ab7fb

T::Currency::reserve(&owner_info.owner, owner_info.deposit)

and

substrate/frame/contracts/src/wasm/code_cache.rs

Line 139 in 50ab7fb

T::Currency::unreserve(&owner_info.owner, owner_info.deposit);

Just to double-check if that's ok. Per my understanding, once #12951 is merged, we probably can't leave it as is:

There are no equivalents for ReservableCurrency::reserve or ::unreserve: an identifier must always be used.

[agryaznov]

needs doc comments update

[agryaznov]

this lil niggle is not addressed

[athei]

You sure? I think I did.

[gavofyork]

This "upgraded-this-account" logic is only available after #12951 is merged, right?

Surely this logic can be merged without breaking 12951, no? I don't think anything in the current pre-12951 logic would break these changes. So then you'd write this migration extrinsic in the contracts pallet, and ideally provide a script which could be run to migrate all accounts over lazily.

The general idea is implemented for migration in 12951, but obviously this migration logic would be specific to the contracts pallet, so you'd need to introduce the extrinsic yourself in this PR.

[athei]

Ahh okay I get it this statement is about the general approach to lazy migrations. In general, yes I want all contract migrations to be lazy as they tend to be heavy.

My confusion is about the order of migrations: What if the migration in #12951 is applied before the contracts migration?

In my mind (correct me if I am wrong) it makes only sense in this order:

1. Merge this PR including the to be written lazy migration for all accounts.
2. Merge Deprecate Currency; introduce holds and freezing into fungible traits #12951.
3. Port all code away from Currency (including contracts).
4. Only then apply the migration in Deprecate Currency; introduce holds and freezing into fungible traits #12951.

[gavofyork]

Merging #12951 and beginning lazy migration of #12951 is really the same action. The logic cannot be merged without requiring any accounts touched to be bought up to date with the new logic.

I'd feel better if we did:

1. Merge this PR including the to be written lazy migration for all accounts.
2. (For chains actively using Contracts pallet): Migrate all contracts to the new logic.
3. Apply Deprecate Currency; introduce holds and freezing into fungible traits #12951, and allow migration to proceed lazily.
4. Deprecate Currency and migrate all pallets to use fungible, migrating lazily.

[athei]

After some discussion in the FRAME channel we agreed that we can merge this without the storage migration in order to get #12951 in quickly. Reason is that no chain that uses pallet-contracts actively depends on master.

Plan is:

1. Merge this.
2. Merge Deprecate Currency; introduce holds and freezing into fungible traits #12951
3. Create lazy/multi block migrations for pallet-contracts

The added benefit is that I can then assume that accounts are already upgraded and exist without reserved balance (when I touch the account in my migration it will be lazily upgraded). This assumptions makes my migration easier: I can just move over all the reserved balance to the deposit account without destroying the contract's account.

All of that will be included in the same release which is then safe to use for existing pallet-contracts deployments.

[athei]

Also, this PR does not affect the way amount is reserved (and unreserved) on the contract code uploader account:
Just to double-check if that's ok. Per my understanding, once #12951 is merged, we probably can't leave it as is:

There are no equivalents for ReservableCurrency::reserve or ::unreserve: an identifier must always be used.

#12951 does not remove ReservableCurrency, yet. The quote is from the part where Gav talks about how to eventually port away from (Reserveable)Currency. But this will happen in a later PR (after #12951). Right now we just need to make sure that the existing reserve logic makes sense with the changes in #12951 so it can be merged.

We have the assumption (before this PR) that reserved balance contributes to the existential deposit. This was true so far. This changes with #12951 because it makes other things easier and more consistent. The solution to this involves moving away from reserves but this is not because ReservableCurrency is removed with #12951. It will still be there for some time.

We only had this assumption for the storage deposits that go to contracts. We don't have this assumption when reserving for the code. Hence we don't need to change this part (yet).

[athei]

bot bench -v PIPELINE_SCRIPTS_REF=bm-fallback $ pallet dev pallet_contracts

[command-bot]

@athei https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2402302 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 34-7325575e-e1b2-4ad3-bf81-c85638c05db0 to cancel this command or bot cancel to cancel all commands in this pull request.

[command-bot]

@athei Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2402302 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2402302/artifacts/download.

[agryaznov]

can complain nothing but some typos in documentation

[agryaznov]

this lil niggle is not addressed

[athei]

bot merge

[paritytech-processbot]

Waiting for commit status.