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

Fixed saving keys in wallet #1613

Merged
merged 3 commits into from Mar 4, 2019

Conversation

Projects
5 participants
@pixelplex
Copy link

commented Feb 25, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

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

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

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 pixelplex changed the base branch from master to develop Feb 25, 2019

@designsters designsters force-pushed the pixelplex:fixed-saving-keys-wallet branch from 6a90c97 to 1b140d2 Feb 25, 2019

@pixelplex

This comment has been minimized.

Copy link
Author

commented Feb 25, 2019

@oxarbitrage I have changed the base branch

@abitmore

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Another related issue is #71

@abitmore

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

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

@pixelplex

This comment has been minimized.

Copy link
Author

commented Feb 27, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

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
Copy link
Contributor

left a comment

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 !

@abitmore abitmore merged commit 3cd75ae into bitshares:develop Mar 4, 2019

1 of 2 checks passed

ci/dockercloud Your tests are pending in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@abitmore abitmore added this to In development in Feature Release (3.1.0) via automation Mar 4, 2019

@pmconrad pmconrad moved this from In development to Done in Feature Release (3.1.0) Mar 5, 2019

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