Fixed saving keys in wallet

[pixelplex]

When a new account is created, its private keys must be added to a wallet. Key addition must happen only after the account is created on the blockchain.

Problem:

After an account is created, its keys are put into a temporary data structure (pending_account_registrations), stored in the wallet. After this, the wallet (in JSON format) is recorded into a file (wallet.json). Once a new block is added, a check is performed on whether the account was created on the blockchain. If it was created, its keys are put into _keys data structure, which is stored in the wallet. However, these keys are not re-recorded into the wallet file (wallet.json) in the encoded format (variable cipher_keys) and are not removed from the temporary data structure (pending_account_registrations). Because of this, if the wallet is stopped and restarted, new keys are not added to data structure _keys, stored in the wallet. Which leads to the new account not being able to sign transactions.
The fix is presented in the request.

[oxarbitrage]

Thanks! Can be possible for you to make a test case in cli_tests for this? https://github.com/bitshares/bitshares-core/blob/master/tests/cli/main.cpp .

[oxarbitrage]

In another note, this pull request should point to the develop branch. We never merge to master directly. Thanks.

[pmconrad]

Oh. My. God.

IMO what the wallet does here is completely broken. (Not your fault, @pixelplex ).

• create_account_with_brain_key hashes the given brain key and hands it over to create_account_with_private_key.
• create_account_with_private_key derives active and memo keys from the given private key, while using the private key itself as owner key. It puts active and memo keys into _wallet.pending_account_registrations.
• Then it saves _wallet into a file, including the plaintext keys.
• Then it broadcasts the create_account operation.
• Later, when a new block has been received, it checks if the accounts listed in _wallet.pending_account_registrations exists, and if so, it tries to import the keys it has stored for the account. This also checks if the stored keys match the on-chain keys.
• The keys are deleted from pending_account_registrations afterwards, i. e. if the account is created successfully on a fork that becomes the main fork later, the keys will be lost.

It doesn't store the owner key anywhere. Makes some sense, assuming that the owner key is stored externally (e. g. in the form of the brain key). This should be documented.
It completely disregards the possibility of forking. Related: #151 .

The fix proposed in this PR does not help with any of this AFAICS. IMO these problems need to be resolved at another level, perhaps in the context of #151.

[pixelplex]

@oxarbitrage I have changed the base branch

[abitmore]

I guess we have worked on a similar issue in the past. See #235.

At least the wallet doesn't generate a key from nowhere then lose it.

• If a user tried to create an account with create_account_with_brain_key, the user would have saved the brain key elsewhere already, in this case, owner keys can be derived from the brain key.
• If a user tried to create an account with create_account_with_private_key, the user would have saved the private key elsewhere, in this case, the user can import the keys later.

I agree with @pmconrad that there are issues related to accounts in cli_wallet (#151). Is there any typical use case that relies on those features?

[abitmore]

Another related issue is #71

[abitmore]

The develop branch has a different FC version, please rebase/correct it.

[pixelplex]

Unfortunately, cli_fixture does not allow for proper test coverage. Extensive functionality re-write would be required for proper testing, leading to significant amount of time required for it.

@pmconrad , could you please describe the problem in detail? So far the only obvious bottleneck is this one:
account_id serves as key in the variable extra_keys. Should a fork occur, same accounts can have different ids on different chains of the fork, which would cause problems when a fork is resolved. This can be solved by replacing account_id with name in extra_keys.

@abitmore your documentation states that owner_key is represented as a cold-stored key (which means it is not stored on the device, but rather on an external storage device, i.e. usb flash-drive). By this logic it must not be imported into a wallet in the first place. active_key is used for interaction with the blockchain. active_key and owner_key are not saved on the wallet. Therefore we can not expect to interact with the blockchain after the node is shut down and restarted.

About the FC version - okay, we will update it a little bit later

[pmconrad]

Problem 1: keys must not be saved to disk in plaintext.
Problem 2: keys can be lost if account creation fails (because someone else registers an account with the same name) but the chain switches to a different fork later where account is created with our keys.
Problem 3: the wallet may associate keys with a wrong account id, again due to forking.
Problem 4: the wallet is not saved after the account has been detected in a block. This is your original problem and seems to be fixed by your change.

Problem 2 is probably somewhat academic.

[pmconrad]

IMO we should merge this because it is an improvement over the current behaviour. The key handling still needs a more general overhaul, hopefully in the context of #151 .

Thanks @pixelplex !