Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enumeratable accounts #195

Merged
merged 85 commits into from Jun 18, 2018

Conversation

@gavofyork
Copy link
Member

commented Jun 1, 2018

Based on #192
Closes #185 , closes #209

Makes all accounts enumerable. Introduces a short-hand form for encoding addresses and the notion of an existential deposit which is the minimum amount that must be held in an account for it to be considered "alive"; if the (free) balance ever falls below this, the account is deleted. It also introduces fees for account creation, contract-creation, transfer and a rebate for account-creation with a valid or non-existent hint.

Note: This will change the Polkadot binary extrinsic format. Before it was:

[ account-id (32-bytes), index (4-bytes), call (dynamic-length), signature on first three fields (64 bytes) ]

Now, it is:

[ address (1/3/5/9/33-bytes, dependent on first byte), index (4-bytes), call (dynamic-length), signature on *original* fields (64 bytes) ]

Note that the "original fields" refer to the [ account-id (32-bytes), index (4-bytes), call (dynamic-length) ] serialisation, not the first three fields of this extrinsic.

The specific format for the new address type is one of 5 sub-formats, switched on the first byte:

  • 0xff, 32-byte account id
  • 0xfe, 8-byte account index
  • 0xfd, 4-byte account index
  • 0xfc, 2-byte account index
  • [0xf0...0xfb] (invalid, reserved for future use)
  • [0x00...0xef] 1-byte account index (less than 0xf0)

The account index variants are significantly smaller but require a lookup in the state. To avoid a transaction replay attack when an index changes its account, the signature is signed not with the first field as the index, but rather as the account id, thereby invalidating all previous signatures once the index is used to lookup a different id.

In addition to the sender field, any parameters to Call/Propose (PrivCall) fields that were AccountId (32-bytes) are now Address (1/3/5/9/33-bytes).

There is currently a small "easter egg" which allows an index to be reclaimed after the account to which it refers expires. The reason it's done this way rather than an explicit new dispatch call is to allow for incidental account creation (e.g. a smart contract doing the creating, or being created) to also be able to make claims.

Still todo:

  • Basic implementation
  • Lookup call to convert to AccountIndex from AccountId

Be hygienic:

  • Disallow existence of accounts with value below existential_deposit and their indexes
  • Allow reusing dead index values...
  • ...for a lower tx fee

Utilise indexes:

  • Introduce enum Address { Id(AccountId), Index(AccountIndex) } (this could be more efficient since AccountId could account for all AccountIndex values within its dead state)
  • Efficient serialisation of Address (either 33 bytes or 1-8 bytes)
  • Switch out AccountId for Address for extrinsic's sender
  • Substitute AccountId for Address for extrinsic params

And of course...

  • More tests around account reclaiming, particularly FreeBalance and ReservedBalance interplay
  • Integrate other balance transformations into the new model
  • Finalise semantics for ReservedBalance
  • Useful runtime hooks for the transaction pool
  • Reserve some extra initial bytes of address for future format changes
  • Test for when nonce gets reset
  • Updated transaction pool

@gavofyork gavofyork added this to To Do in Runtime via automation Jun 1, 2018

gavofyork added some commits Jun 1, 2018

Fail(&'static str),
}
}
/*

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 13, 2018

Member

commented out code

Payment: MakePayment<System::AccountId>,
Finalisation: Executable,
>(PhantomData<(System, Block, Lookup, Payment, Finalisation)>) where
Block::Extrinsic: Checkable<AccountId=System::AccountId> + Slicable,

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 13, 2018

Member

style: bounds should be on implementation and not on definition

This comment has been minimized.

Copy link
@gavofyork

gavofyork Jun 14, 2018

Author Member

that was the commented-out-code in the previous comment :-P

#[derive(Eq, PartialEq, Clone, Copy)]
#[cfg_attr(feature = "std", derive(Debug, Serialize))]
#[repr(u8)]
pub enum ApplyOutcome {

This comment has been minimized.

Copy link
@rphmeier
#[derive(Eq, PartialEq, Clone, Copy)]
#[cfg_attr(feature = "std", derive(Debug, Serialize))]
#[repr(u8)]
pub enum ApplyError {

This comment has been minimized.

Copy link
@rphmeier
/// Type to lookup into.
type Target;
/// Attempt a lookup.
fn lookup(s: Self::Source) -> result::Result<Self::Target, &'static str>;

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 13, 2018

Member

I don't think this should be implicitly global. taking &self is strictly more powerful because it's easy to us a ZST that uses globals internally.

This comment has been minimized.

Copy link
@gavofyork

gavofyork Jun 14, 2018

Author Member

that means passing through an instance, which would be difficult to make work with the rest of the runtime code. perhaps another PR, though it should probably be delayed until/unless we actually need it, and if so i'd probably take a much deeper look at how to rework the runtime.

@@ -86,18 +102,23 @@ impl<Hashing, AccountId> ContractAddressFor<AccountId> for Hashing where
}
}

// MaybeSerializeDebug is workaround for https://github.com/rust-lang/rust/issues/26925 . Remove when sorted.

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 13, 2018

Member

doesn't seem like it's actually used here

type DetermineContractAddress: ContractAddressFor<Self::AccountId>;
/// Type used for storing an account's index; implies the maximum number of accounts the system
/// can hold.
type AccountIndex: Parameter + Member + Slicable + SimpleArithmetic + As<u8> + As<u16> + As<u32> + As<usize> + Copy;

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 13, 2018

Member

this is fairly dangerous because it can truncate

This comment has been minimized.

Copy link
@gavofyork

gavofyork Jun 14, 2018

Author Member

?

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 14, 2018

Member

the As trait is being used without accounting for overflow

This comment has been minimized.

Copy link
@gavofyork

gavofyork Jun 15, 2018

Author Member

where?

<FreeBalance<T>>::remove(who);
<Bondage<T>>::remove(who);
<CodeOf<T>>::remove(who);
// TODO: <StorageOf<T>>::remove_prefix(address.clone());

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 13, 2018

Member

is there any storage in particular that this would target? IMO it seems dangerous to have the staking module arbitrarily clear storage from any other modules, whatever they may be

This comment has been minimized.

Copy link
@gavofyork

gavofyork Jun 14, 2018

Author Member

It's used only for smart contracts which the staking module has sole ownership and operation rights over.

/// Kill an account's reserved portion.
fn on_reserved_too_low(who: &T::AccountId) {
<ReservedBalance<T>>::remove(who);
if Self::free_balance(who).is_zero() {

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 13, 2018

Member

should it not check if the balance < existential deposit and call on_free_too_low?

This comment has been minimized.

Copy link
@gavofyork

gavofyork Jun 14, 2018

Author Member

no; FreeBalance can never be >0 && <ExistentialDeposit, so if it's ripe for deletion, FreeBalance will already have been collapsed to zero.

let mut set = Self::enum_set(T::AccountIndex::sa(usize_index / ENUM_SET_SIZE));
let i = usize_index % ENUM_SET_SIZE;
if i < set.len() {
Some(set.swap_remove(i))

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 13, 2018

Member

this whole if/else could be set.get(i).map(|x| x.clone())

would be cheaper also since it doesn't need to swap

This comment has been minimized.

Copy link
@gavofyork

gavofyork Jun 14, 2018

Author Member

but it would clone; swap_remove doesn't.

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 15, 2018

Member

swap_remove is more expensive as long as AccountId is Copy.

let try_index: usize = index.as_();
let try_set = Self::enum_set(T::AccountIndex::sa(try_index / ENUM_SET_SIZE));
let item_index = try_index % ENUM_SET_SIZE;
item_index < try_set.len() && Self::voting_balance(&try_set[item_index]).is_zero()

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 13, 2018

Member

.is_zero() or < existential deposit?

This comment has been minimized.

Copy link
@gavofyork

gavofyork Jun 14, 2018

Author Member

VotingBalance should not be compared directly to ExistentialDeposit. VotingBalance == FreeBalance + ReservedBalance, neither of which may ever be allowed to be set s.t. >0 && <ExistentialDeposit. By extension, VotingBalance is never < ExistentialDeposit, so .is_zero() works fine.

/// Refund some balance.
pub fn refund(who: &T::AccountId, value: T::Balance) {
<FreeBalance<T>>::insert(who, Self::free_balance(who) + value)
/// Set the reserved balance of an account to some new value. Will enforce ExistentialDeposit

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 13, 2018

Member

comment is wrong (this is fn set_free_balance)

gavofyork added some commits Jun 14, 2018

let mut set = Self::enum_set(T::AccountIndex::sa(usize_index / ENUM_SET_SIZE));
let i = usize_index % ENUM_SET_SIZE;
if i < set.len() {
Some(set.swap_remove(i))

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 15, 2018

Member

swap_remove is more expensive as long as AccountId is Copy.


/// `true` if the account `index` is ready for reclaim.
pub fn can_reclaim(index: T::AccountIndex) -> bool {
let try_index: usize = index.as_();

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 15, 2018

Member

if this overflows it could cause issues


// A little easter-egg for reclaiming dead indexes..
let ret = {
let next_set_index: usize = next_set_index.as_();

This comment has been minimized.

Copy link
@rphmeier

rphmeier Jun 15, 2018

Member

or here

@@ -251,8 +329,9 @@ impl TransactionPool {
}
}

// TODO: remove. This is pointless - just use `submit()` directly.

This comment has been minimized.

Copy link
@tomusdrw

tomusdrw Jun 15, 2018

Contributor

We have 2 places were we submit runtime::UncheckedExtrinsic directly, so we don't have to go through serialization:

polkadot/service/src/lib.rs
131:			match self.pool.import_unchecked_extrinsic(uxt) {

polkadot/consensus/src/lib.rs
552:			self.transaction_pool.import_unchecked_extrinsic(uxt)

This comment has been minimized.

Copy link
@gavofyork

gavofyork Jun 15, 2018

Author Member

the implementation shows it's just a shim for the underlying pool's api:

self.inner.submit(vec![uxt]).map(|mut v| v.swap_remove(0))

if it's really just meant to be a convenience method for submitting a single transaction then fair enough, but in that case it should be named submit_typed or some such. "import" suggests it uses different logic to "submit" which (aside from the largely superficial typing) it doesn't.

@gavofyork gavofyork referenced this pull request Jun 16, 2018
7 of 7 tasks complete
@rphmeier

This comment has been minimized.

Copy link
Member

commented Jun 18, 2018

LGTM modulo sync tests being ignored

@gavofyork gavofyork merged commit f191535 into master Jun 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Runtime automation moved this from To Do to Done Jun 18, 2018

@gavofyork gavofyork deleted the gav-enum-accounts branch Jun 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.