Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Composite accounts #4820

Merged
merged 64 commits into from
Feb 14, 2020
Merged

Composite accounts #4820

merged 64 commits into from
Feb 14, 2020

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented Feb 4, 2020

This brings in the final element of my balance refactorings, namely to avoid the (pessimising) spreading of typically co-accessed account data between multiple trie keys. This is particularly annoying for nonce and balance, which for all normal transactions are always mutated and yet are split between two separate trie items.

This provides an extensible framework, via a trait StoredMap, which allows an item to be stored somewhere, without specifying whether it's stored in its own storage item or in a shared composite storage item with other (probably related) values. It is used by the system module to allow for arbitrary data to be stored alongside the account nonce.

Concretely, this merges the balances of an account with the nonce, reducing the number of trie accesses for a typical transfer from 3 to 2.

Until we know the exact tradeoff between bulking up a single key with lots of data that may possibly not tend to be co-accessed versus having multiple, smaller, trie keys, then we'll keep it lean, but benchmarking typical use-cases with "fat account items" might lead to further optimisations.

TODO

  • Indices should not use chunking.
  • Indices should hold a deposit.
  • Reclaiming an index should not be possible before removal.
  • Migration code.
  • Remove index lookup (in Kusama/Polkadot).
  • Test migration on live Kusama state.

Additionally...

In addition to this, it makes the system of account indices opt-in; this means that accounts need not have indexes despite being funded. Two calls for registering indices are provided, as well as a third call for giving an index up and a forth for administrating indices by root.

The upshot to this is that there no longer needs to be a separate AccountCreationFee. All transfers work in the same way.

^^^ CC @jacogr @pepyakin

Indices were originally brought in to provide for a means of enumerating all accounts (e.g. for migrations). This is no longer needed since we improved the storage interface. Their other usage was for minimising the size of a transaction, at the cost of an additional storage lookup. This additional looking is in reality an extra cost that must be borne out through conditional weighting, increasing code complexity and attack surface. The reality is that the extra bytes fee for putting an normal address is likely smaller than the fee for the extra lookup, meaning it will never be used. Its third usage is in terms of usability, allowing users to pass around smaller addresses. This is slightly dangerous in the previous usage, due to address reclaiming, and needs careful UI provisions. The cost, as it has been implemented, is an ever-growing state or significant fees.

This is mitigated in the current PR by having a single storage entry per index and a deposit, where the deposit can be reclaimed.

Upgrading Pallets & Runtimes

If you're using decl_event in your tests, you'll now need to include system<T> as an item, so
decl_event!(my_pallet<T>); would become decl_event!(system<T>, my_pallet<T>);.

system::Trait is now the place to put any OnNewAccount or OnReapAccount hooks; these will need moving from balances::Trait. You should include at least the Balances handler in the reap account, so:

impl frame_system::Trait for Runtime {
    ...
    type OnNewAccount = (...);
    type OnReapAccount = (Balances, ...);
    ...
}

Additionally, there's a new item in system::Trait called AccountData and a new item in balances::Trait called AccountStore. These can be wired together in order to let system store your account balances, which will be more efficient than balances storing them itself:

impl frame_system::Trait for Runtime {
    ...
    type AccountData = pallet_balances::AccountData<Balance>; // Or whatever your balance type is (e.g. u64 in tests)
    ...
}
impl pallet_balances::Trait for Runtime {
    ...
    type AccountStore = System;
    ...
}

Several config trait items are gone, they can be removed:

  • pallet_balances::Trait::OnNewAccount (now in frame_system)
  • pallet_balances::Trait::OnReapAccount (now in frame_system)
  • pallet_balances::Trait::OnFreeBalanceZero (just use OnReapAccount)
  • pallet_balances::Trait::CreationFee (removed)
  • pallet_balances::Trait::TransferFee (removed)

@gavofyork gavofyork added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 4, 2020
frame/society/src/mock.rs Outdated Show resolved Hide resolved
@gavofyork
Copy link
Member Author

logic errors fixed.

Copy link
Contributor

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy with this other than the final comments.

@gavofyork gavofyork merged commit 29454c3 into master Feb 14, 2020
@gavofyork gavofyork added this to Done in Runtime via automation Feb 14, 2020
@gavofyork gavofyork deleted the gav-composite-account branch February 14, 2020 00:47
General-Beck pushed a commit to General-Beck/substrate that referenced this pull request Feb 18, 2020
* Basic account composition.

* Add try_mutate_exists

* De-duplicate

* Refactor away the UpdateBalanceOutcome

* Expunge final UpdateBalanceOutcome refs

* Refactor transfer

* Refactor reservable currency stuff.

* Test with the alternative setup.

* Fixes

* Test with both setups.

* Fixes

* Fix

* Fix macros

* Make indices opt-in

* Remove CreationFee, and make indices opt-in.

* Fix construct_runtime

* Fix last few bits

* Fix tests

* Update trait impls

* Don't hardcode the system event

* Make tests build and fix some stuff.

* Pointlessly bump runtime version

* Fix benchmark

* Another fix

* Whitespace

* Make indices module economically safe

* Migrations for indices.

* Fix

* Whilespace

* Trim defunct migrations

* Remove unused storage item

* More contains_key fixes

* Docs.

* Bump runtime

* Remove unneeded code

* Fix test

* Fix test

* Update frame/balances/src/lib.rs

Co-Authored-By: Shawn Tabrizi <shawntabrizi@gmail.com>

* Fix ED logic

* Repatriate reserved logic

* Typo

* Fix typo

* Update frame/system/src/lib.rs

Co-Authored-By: Shawn Tabrizi <shawntabrizi@gmail.com>

* Update frame/system/src/lib.rs

Co-Authored-By: Shawn Tabrizi <shawntabrizi@gmail.com>

* Last few fixes

* Another fix

* Build fix

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Jaco Greeff <jacogr@gmail.com>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
@@ -761,9 +760,6 @@ decl_storage! {

/// The earliest era for which we have a pending, unapplied slash.
EarliestUnappliedSlash: Option<EraIndex>;

/// The version of storage for upgrade.
StorageVersion: u32;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the storage version hasn't been removed from storage in kusama

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if I query on kusama the key twox128("Staking") ++ twox128("StorageVersion") == 0x5f3e4907f716ac89b6347d15ececedca308ce9615de0775a82f8a94dc3d285a1 then I get no value, and I can't understand why yet.

EDIT: I got my answer about Kusama, migration feature was off

lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020
* Amalgamate pieces of balance module

* Fixes for vesting split

* Refactoring for vesting/balances split

* Build fixes

* Remove on_free_balance_zero and some docs.

* Indentation.

* Revert branch

* Fix.

* Update substrate: fixes after CLI refactoring

* Reverting removal of exit

* Removed too much again

* Update Cargo.lock

* Cargo.lock

* Update Substrate, ready for paritytech#4820

* Fixes

* Update to latest substrate master

* Fix network tests

* Update lock

* Fix tests

* Update futures to get bug fixes

* Fix tests for new balances/vesting logic

* Cargo fix

* Another fix

Co-authored-by: Cecile Tonglet <cecile.tonglet@cecton.com>
Co-authored-by: Robert Habermeier <rphmeier@gmail.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants