Skip to content

BSIP65: Fix Locked Accounts #149

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.

Sign up for GitHub

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

BSIP65: Fix Locked Accounts #149

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6ba1f10
Merge pull request #1 from bitshares/master
OpenLedgerApp Dec 18, 2018
7c0363d
Merge branch 'master-bs'
OpenLedgerApp Feb 22, 2019
5655920
fix locked accounts
OpenLedgerApp Feb 22, 2019
3c99c15
formatting changed
OpenLedgerApp Feb 22, 2019
6cc3a57
update specification: add some terms, describe manual unlock approach
OpenLedgerApp Feb 28, 2019
44b4712
update rational and specifications
OpenLedgerApp Mar 1, 2019
0ef008e
update abstract, motivation, summary and see also
OpenLedgerApp Mar 1, 2019
a52f2d1
update header, remove README modifications
OpenLedgerApp Mar 1, 2019
7579612
fixed spaces formatting
OpenLedgerApp Mar 1, 2019
9571127
fixed lists formatting
OpenLedgerApp Mar 1, 2019
b242bee
Update Summary for Shareholders
OpenLedgerApp Mar 25, 2019
dce5b2e
sign_state extension: provide more information
OpenLedgerApp Mar 28, 2019
5d807b1
Update 'fix locked accounts on demand'
OpenLedgerApp Mar 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,4 @@ Number | Title |
[58](bsip-0058.md) | Global Settlement Protection Through Price Feeding | Jerry Liu | Protocol | Draft
[59](https://github.com/bitshares/bsips/issues/140) | Adjustment of MSSR and MCR Through Voting | Jerry Liu | Informational | Draft
[60](https://github.com/bitshares/bsips/issues/131) | BitShares URI scheme | John Titor, Stefan Schießl, Abit More | Informational | Draft
[61](https://github.com/bitshares/bsips/pull/) | Fix locked accounts with circular dependencies | OpenLedgerApp | Protocol | Draft
53 changes: 53 additions & 0 deletions bsip-0061.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
BSIP: 0061
Title: Fix locked accounts with circular dependencies
Authors: OpenLedgerApp <https://github.com/OpenLedgerApp>
Status: Draft
Type: Protocol
Created: 2019-22-02
Updated:
Discussion: https://github.com/bitshares/bsips/issues/94
Worker:


# Abstract
BitShares allow to change permissions of account A to account A. 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

# Motivation
We offer to develop additional functionality "Prevent to create cycled accounts". This functionality will avoid to create or update account authority with circular dependencies in the future.

# Rationale
According to our investigation, BitShares has about 384 locked accounts with circular dependencies. Most of these accounts are locked by mistake.

# Specifications

## Prevent circular dependencies
We must extend create and update account evaluators adding code that detect cycled authorities and throws exception in do_evaluate() method if cycle detected.
To avoid any misunderstanding of existing authorization mechanism we propose to reuse actual code of sign_state::check_authority(). Make template class traverse_authorities_state and parametrize it with approve() method. By default approve = signed_by(), an existing method of sign_state class. We add keys_available() method to resolve signing path. So, check_authority() returns true, if signing path fully ends with existing keys.

## Fix locked accounts
There are two approaches here:
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is:

    1. Resolving the locked accounts is unconditional for the impacted accounts (no fees, no ability to opt-out).
    1. The hardfork will revert the most recent account update operation by performing a new account update operation to set the the account authority to be in the previous (unlocked) state.
    1. The hardfork will also contain logic that prevents locked accounts in the future (to a depth TBD).

I don't see a need for the Manual (on-demand) section. I am open to further discussion/explanation of its utility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See details in updated version. You are welcome to discussion.


1) Automatic (unconditional)
- Detect all locked accounts in first maintenance after hard-fork.
- 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.

2) Manual (on-demand)
- Check if account is locked.
- Find latest operation ("lock"-operation) in cycle.
- Undo "lock"-operation.

# Discussion
Should we identify authority as locked in case of cycle only? Bitshares has *max_authority_depth* parameter. So, authorities with delegetion depth > *max_authority_depth* are potentially locked. Should we prevent this?
Copy link
Contributor

Choose a reason for hiding this comment

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

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 now removed everything about GRAPHENE_MAX_AUTHORITY_DEPTH from the document. But according to the rule 'verify_cycled_authority ()', accounts in which the authority delegation level exceeds max_authority_depth are considered blocked. Correspondingly, the create_account_operation and update_account_operation operations will not pass evaluation. Thoughts?


## Examples
Account A has key. Account B delegates it's authority to A - ok. C -> B - ok. D -> C - fail.
Transaction can never be signed by D. D exceed depth limit. Current depth limit in Bitshares = 2.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please demonstrate where this depth limit is defined in the code today. I assume something like GRAPHENE_MAX_AUTHORITY_DEPTH or something. Is this parameter allowed to be changed by the Committee at this time, or hard coded and would require a distinct BSIP to change?

The example is lacking clarity for me. We have to consider both Owner and Active authorities and how they may interact, at what depths. We need to provide and example of what we can unlock (and cannot, why) and what we can prevent from being locked (and cannot, why).

Please use valid formed account names such as "alice" "bob" "charlie" etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above.

Copy link
Contributor

Choose a reason for hiding this comment

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

In answer to your question, @ryanRfox, that limit is found in the committee-controlled parameters here and is used when verifying transaction authorization here. It is defaulted to 2 in the static configuration here, although that default is irrelevant by now: the committee controls the value.


# Summary for Shareholders

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 •
edited
Loading

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