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

Conversation

OpenLedgerApp
Copy link
Contributor

@OpenLedgerApp OpenLedgerApp commented Feb 22, 2019

For #94.

Copy link
Contributor

@ryanRfox ryanRfox left a comment

Choose a reason for hiding this comment

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

Overall, this BSIP draft is lacking. It does not meet the expectations defined in BSIP1 - BSIP Purpose and Guidelines. As a Community Member I am unable to understand from this proposal what the problem statement is and what the proposed solution(s) will do to remedy it. This needs much more definition in all sections.

README.md Outdated Show resolved Hide resolved
bsip-0061.md Outdated Show resolved Hide resolved
bsip-0061.md Outdated Show resolved Hide resolved
bsip-0061.md Outdated
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.

bsip-0061.md Show resolved Hide resolved
bsip-0061.md Outdated

## 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.

bsip-0061.md Outdated Show resolved Hide resolved
bsip-0061.md Show resolved Hide resolved
bsip-0061.md Outdated Show resolved Hide resolved
bsip-0061.md Outdated Show resolved Hide resolved
@ryanRfox ryanRfox changed the title Bsip fix locked accounts BSIP Draft: Fix Locked Accounts Feb 23, 2019
bsip-0061.md Show resolved Hide resolved
bsip-0061.md Show resolved Hide resolved
bsip-0061.md Outdated Show resolved Hide resolved
bsip-0061.md Show resolved Hide resolved
We offer to develop a solution for BitShares users.
This solution allows unlocking accounts, which are locked by mistake.
The most important goal is to return access to the wallets and unlock their money.

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. :-/

- 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.)

@OpenLedgerApp
Copy link
Contributor Author

We need to determine the way how to fix locked accounts.
There are two ways:

  1. Automatic (unconditional)

    • Detect all locked accounts in first maintenance after hardfork.
    • Find latest operation ("lock"-operation) among operations of cycled accounts. Do it for each cycle.
    • Undo all "lock"-operations. Find previous account update/create an 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.

Let's decide it will be automatic or manual way.

@OpenLedgerApp
Copy link
Contributor Author

Due to the fact that the algorithm for finding and unlocking accounts is quite complex and will require significant costs, we suggest considering another opportunity to unlock in the near HF:

  1. add functionality - prevent creation cycled authority
  2. compile a list of blocked accounts, find blocking operations and previous account states
  3. perform undo blocking operations and return the account to the previous state (hardcoded operations list)

After HF, we check if there are still blocked accounts. They may appear after adding the unlock code, but before the HF. If there are any, then add them hardcoded to the next HF.

@pmconrad
Copy link
Contributor

pmconrad commented May 6, 2019

IMO the algorithm for finding locked accounts is exactly as simple as detecting the lock in the first place.
Since the detection logic will have to be implemented anyway, you can simply apply it to all account_update operations:

  • Before the hardfork, if you detect a lock, put the account and its previous authorities into a list.
  • At hardfork time you can simply take the list and restore account authorities to the saved state, which unlocks them.
    *After the hardfork, if you detect a lock, fail the transaction.

@OpenLedgerApp
Copy link
Contributor Author

The logic of finding a blocking operation is not as simple as determining whether an account has been blocked. In addition to the fact that the account is blocked, we must find the last operation in the chain of blocked accounts that led to this state. If you perform such calculations in bitshares-core in maintenance, then you should also take into account that the history plugin may be inactive and the nodes must come to consensus even without it. Without history plugin, this is problematic. You need to build a cache of recent authority change operations during replay, which will require a large amount of additional memory.

@pmconrad
Copy link
Contributor

The logic of finding a blocking operation is not as simple as determining whether an account has been blocked.

Yes it is. An account that is blocked cannot be updated. So a blocking operation is an account_update after which the account has been blocked.

Note that it is not necessary to find the complete original account_update that caused the lock. It is sufficient to remember the owner authority as it was before the lock, and restore that.

@abitmore
Copy link
Member

abitmore commented Jun 9, 2019

@ryanRfox please assign BSIP number.

@ryanRfox ryanRfox changed the title BSIP Draft: Fix Locked Accounts BSIP65: Fix Locked Accounts Jun 9, 2019
@sschiessl-bcp
Copy link
Collaborator

Massive amount of hours went into this. What is the status @OpenLedgerApp ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants