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

BSIP65: Fix Locked Accounts #149

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
182 changes: 182 additions & 0 deletions bsip-0061.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
BSIP: TBD
Title: Fix locked accounts with circular dependencies
Authors: OpenLedgerApp <https://github.com/OpenLedgerApp>
Status: Draft
Type: Protocol
Created: 2019-02-22
Updated: 2019-03-01
Discussion: https://github.com/bitshares/bsips/issues/94
Worker:


# Abstract
Every account has two authorities assigned: owner and active.

An authority is a set of keys and/or accounts, each of which is assigned a weight.
Each authority has a weight threshold that must be crossed before an action requiring that authority may be performed.
The owner authority is designed for cold-storage, and its primary role is to update the active authority or to change the owner authority.
The active authority is meant to be a hot key and can perform any action except changing the owner authority.

BitShares has locked accounts with circular dependencies. Some of them are locked by mistake. So, locked accounts couldn't buy/sell their assets, make transfer and e. c.

The community has had extensive discussions about how to resolve this issue:

1. To add additional functionality "avoid circular dependencies"to the nearest hard fork;
2. To find all locked accounts with circular dependencies and undo the last account_update operation.

There are some cases that led to locked account:

- Alice updates, via account_update_operation, her active and owner authority to herself;
- Bob updates, via account_update_operation, his active and owner authority to a locked account;
- Jill updates, via account_update_operation, her active and owner authority threshold greater than a sum of authorities weights.

This proposal is to implement a hard-fork that would prevent it in the future and to make posible unlock locked accounts.
New operation unlock_account will be added, account_update_evaluator and account_create_evaluator will be modified.
pmconrad marked this conversation as resolved.
Show resolved Hide resolved

# Motivation
OpenLedgerApp marked this conversation as resolved.
Show resolved Hide resolved
Current BitShares implementation has no mechanism to unlock accounts and doesn't prevent the appearance of locked.

There are two important objectives this proposal aims to achieve:

- to allow users to unlock their accounts;
- to prevent circular dependencies.

This solution allows:

- to resolve user complaints;
- to increase confidence in BitShares.

# Rationale
## Prevent circular dependencies
To prevent circular dependencies account evaluators should contain verificaton code that detects cycled authorities. The new function `verify_cycled_authority()` traverses down the hierarchy of accounts and checks if the transaction can be potentially signed with this account. To avoid any misunderstanding of existing authorization mechanism the `sign_state` class will be reused. Parametrizing this class with `verify_only`-flag helps to do code of `verify_cycled_authority()` simple, readable and reliable.

## "Lock"-operation
"Lock"-operation is the last *account_update_operation* that led the account to a locked state. No more operations possible on behalf of this account.

## Unlocking account
To unlock account we have to find previous *account_create_operation* (or *account_update_operation*) that changed owner or active authority. This operation will be reapplied to undo "lock"-operation if possible. See Discussion.

## Fix locked accounts
At the beginning find all locked accounts in first maintenance after hard-fork. Maybe locked accounts should be marked for unlock. Then fix them. There are two approaches here.

First - automatic (unconditional)

- Find latest operation ("lock"-operation) among operations of cycled accounts. Do it for each cycle.
- Undo all "lock"-operations. Find previous account update/create operation that changes authorities and redo it.

Second - manual (on-demand). Unlocking can be performed only by the account that performed the operation that led to the blocking, using the credentials immediately prior to blocking. That is, not all the keys from the history of this account fit. Thus, you can unlock an account that has been blocked only by mistake and only if the necessary credentials are available. Here we will avoid illegal behavior.

- New type of operation defined: unlock_account_operation { account_to_unlock : account_id_type }.
- Initiator (potentially *account_to_unlock*) signs the transaction using its previous authority preceding the locked state.
- Initiator (potentially *account_to_unlock*) pays fee for this operation.
- *account_to_unlock* should have sufficient balances to perform this operation.
- Check if *account_to_unlock* is locked in unlock_account_evaluator.
- Check if *account_to_unlock* is authorized to perform the operation (transaction signed with previous authority).
- Find previous account update/create operation that changes authorities - restore point.
- Undo "lock"-operation: extract owner and active authorities from restore point, update account with extracted data.
- Push corresponding virtual account_update_operation.

# Specifications
## Prevent circular dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

The specification is incomplete. It does not specify the unlocking process.

Also, please keep in mind that the target audience of a BSIP consists largely of non-programmers. Code as specification is insufficient. Pseudocode is welcome for data structures etc., but logic should be describe in a human-readable way. (Some of the "Rationale" can be included in the specification.)

```
account_create_evaluator::do_evaluate( )
{ ...
if( head_block_time() < HARDFORK_PREVENT_CYCLES_TIME )
{
graphene::chain::verify_cycled_authority( account );
}
...
}

account_update_evaluator::do_evaluate( )
{ ...
if( head_block_time() < HARDFORK_PREVENT_CYCLES_TIME )
{
graphene::chain::verify_cycled_authority( account );
}
...
}
```
```
verify_cycled_authority( account_id )
{
sign_state state( initial_parameters, verify_only=true );

GRAPHENE_ASSERT( state.check_authority(account_id), "Missing Authority" );
}
```
```
class sign_state
{
sign_state( old_parameters, bool verify_only = false )

//
// New method added to parametrize class behaviour
// 'verify_only'-option helps to traverse down the hierarchy of accounts
// emulating that all requested signatures are available
//
bool is_key_available(const public_key_type &key)
OpenLedgerApp marked this conversation as resolved.
Show resolved Hide resolved
{
if (verify_only)
{
return true
}

auto pk = available_keys.find(key);
return (pk != available_keys.end());
}

//
// There is a small refactoring here:
// allocate checking key/signature availability in a separate method
//
bool signed_by( const public_key_type& k )
{
auto itr = provided_signatures.find(k);
if( itr == provided_signatures.end() )
{
-- auto pk = available_keys.find(k);
-- if( pk != available_keys.end() )

++ if( is_key_available(k) )
return provided_signatures[k] = true;

return false;
}
return itr->second = true;
}

bool signed_by( const address& a )...

...
bool verify_only
}
```

# Discussion
## Unable to unlock account
Use case:

1) Account "Alice" delegates its authority to "Bob" - here: prior state
2) Account "Charlie" delegates its authority to "Alice"
3) Account "Alice" delegates its authority to "Charlie" (transaction was signed with Bob's private key) - here: locked state
4) Account "Bob" delegates its authority to "Charlie" - here: locked state

To unlock "Alice" we need to reapply update operation at step 1. But "Bob" also locked, we have double lock here.
pmconrad marked this conversation as resolved.
Show resolved Hide resolved

# Summary for Shareholders
OpenLedgerApp marked this conversation as resolved.
Show resolved Hide resolved
We propose to develop a solution for BitShares users.
This solution will help to avoid accidental account blocking by setting circular dependencies.
There are similar precedents on the network, and unlocking accounts without hardfork is impossible. Thus, the account with all its balances may be blocked for a long period. Also, blocking of a lifetime member account incurs direct financial losses to the owner.
Moreover, the solution will allow unlocking accounts that are blocked by mistake.
The most important goal is to prevent irreversible blocking of accounts and help to regain access to the already blocked accounts and their balances.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a cost/benefit discussion.
How many such locked accounts exist today, how much are the locked balances worth, and what would it cost to implement this proposal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

cost/benefit discussion?

Copy link
Contributor Author

@OpenLedgerApp OpenLedgerApp Mar 29, 2019

Choose a reason for hiding this comment

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

Could you please most specific what is required here - the cost/benefit? We have put the benefits previously. Finally, it does not matter how does it costs if it influences on the Bitshares major functionality and many users faced with this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

How many such locked accounts exist today, how much are the locked balances worth, and what would it cost to implement this proposal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We decided to change the search algorithm according to the comments - #94

As a result, the number of locked accounts has decreased.

List of the locked balances - https://docs.google.com/document/d/1Dmcr9QzSWnCbSKcpaN08iENJyn6AWogbKjZ_iHH4OM0/edit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have summarised the most liquid currencies of locked accounts.

Here the list:
BTS - 1,641,712.7494
USD - 14,546.7176
BTC - 6.86072047
CNY - 16,396.3406
ETH - 0.278826
LTC - 8.60401198
DASH - 0.23722333
ICOO - 348.1812
OBITS - 3,102.1454

Total locked budget is 144,619.841 bitUSD

As you can see the total budget is huge. We offer to unlock these accounts. 

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. That is indeed significant.

How about taking 10% from each account as payment for the implementation? (Seriously. The accounts are locked which means a 100% loss for the owner. We're offering to return 90%.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about taking 10% from each account as payment for the implementation? (Seriously. The accounts are locked which means a 100% loss for the owner. We're offering to return 90%.)

It makes sense.
@pmconrad could you please share your idea of how this can be implemented in a technical way?

Copy link
Contributor

Choose a reason for hiding this comment

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

At unlock time, walk through the account balances and auto-transfer 10% from each non-zero balance to a specific account, e. g. committee-account. That's pretty straightforward.

The non-technical side may be more tricky. :-/

# Copyright
This document is placed in the public domain.

# See Also
https://github.com/bitshares/bitshares-core/issues/269

See *Cycles* paragraph of https://bitshares.org/technology/dynamic-account-permissions for details.

More information can be found here: https://steemit.com/blockchain/@hipster/sad-story-how-i-lost-bitshares-account