Skip to content
This repository has been archived by the owner on Feb 21, 2019. It is now read-only.

Make all trx one-time-keys derive from account key not wallet master key #769

Closed
xeroc opened this issue Sep 12, 2014 · 13 comments
Closed
Assignees
Milestone

Comments

@xeroc
Copy link
Member

xeroc commented Sep 12, 2014

I had a very long support thread over at bitsharestalk.org and couldn't fix this ..
sth. wrong when recovering transactions from the market:

full thread:
https://bitsharestalk.org/index.php?topic=8371.0

errors:

2014-09-12T18:29:40 433157    kisa0145            UNKNOWN     12,250.00000 BTSX     7b893f41
2014-09-12T18:29:40 433172    kisa0145            UNKNOWN     13,000.00000 BTSX     b6e4dbd1
2014-09-12T18:29:40 433186    kisa0145            UNKNOWN     13,500.00000 BTSX     d37cf834
2014-09-12T18:29:40 433192    kisa0145            UNKNOWN     13,625.00000 BTSX     86bbd08d
2014-09-12T18:29:40 433246    kisa0145            UNKNOWN     12,500.00000 BTSX     44f1cb99
2014-09-12T18:29:40 433261    kisa0145            UNKNOWN     12,925.00000 BTSX     f39e8978
2014-09-12T18:29:40 433494    kisa0145            UNKNOWN     12,858.00000 BTSX     fec7cc45
2014-09-12T18:29:40 433552    kisa0145            UNKNOWN     12,857.60999 BTSX     3bcea812
2014-09-12T18:29:40 433603    kisa0145            UNKNOWN     13,200.00000 BTSX     9b771d45
2014-09-12T18:29:40 433618    kisa0145            UNKNOWN     13,445.00000 BTSX     b29df527

>> wallet_recover_transaction 7b893f41 (each of the above IDs yields the same output):

10 assert_exception: Assert Exception
has_deposit:
{}
bitshares  wallet.cpp:3315 bts::wallet::wallet::recover_transaction

{}
bitshares  wallet.cpp:3374 bts::wallet::wallet::recover_transaction

{}
bitshares  common_api_client.cpp:1763 bts::rpc_stubs::common_api_client::wallet_recover_transaction

{"command":"wallet_recover_transaction"}
bitshares  cli.cpp:537 bts::cli::detail::cli_impl::execute_command
@vikramrajkumar vikramrajkumar self-assigned this Sep 12, 2014
@vikramrajkumar
Copy link
Contributor

wallet_recover_transaction only supports normal transfers for now, and not market operations. I will look into this thread when I have time.

@xeroc
Copy link
Member Author

xeroc commented Sep 13, 2014

The issue is: How to recover bitUSD from the market then?

@xeroc
Copy link
Member Author

xeroc commented Sep 23, 2014

I can easily reproduce this issue simply by

  1. create and register a new name
  2. fund the new name with .. say 100 BTSX
  3. move the private key to a newly created wallet
  4. make some market operations on the new wallet (single account in there)
  5. go back to your original wallet that also includes that account name
  6. history 'name' will not show all transaction (especially 'matching' transactions)
  7. balances will not include all funds
  8. wallet_recover_transaction will fail with assertion "has_deposits"

How can I recover the funds in the main wallet?

@vikramrajkumar
Copy link
Contributor

wallet_recover_transaction is not for recovering funds, only recovering transaction details. wallet_regenerate_keys is for recovering funds. There are other strange problems with missing funds though--see #712--which we are tracking down.

@xeroc
Copy link
Member Author

xeroc commented Sep 23, 2014

'wallet_regenerate_keys' also doesn't work with the above described scenario :(

@vikramrajkumar
Copy link
Contributor

Actually see this post: https://bitsharestalk.org/index.php?topic=8935.msg119781#msg119781

I overlooked that market transactions are not TITAN and cannot be regenerated using the account private key alone. Keys generated for market transactions are derived from your account address and your wallet master private key. To successfully regenerate the keys for market transactions, you need to have a version of the same wallet that the transactions were originally created with.

@xeroc
Copy link
Member Author

xeroc commented Sep 24, 2014

oh .. That changes alot for my work-flow ..

Is there no way to use the account private key?

@drltc
Copy link
Contributor

drltc commented Oct 23, 2014

My understanding of this ticket is as follows: Claiming the funds from a successfully executed market order requires the master private key of the wallet used to create the order.

I'd suggest fixing this as follows: When executed, market orders should send their proceeds to the same key used to fund the market order, instead of the master private key. That way whoever controls the account or address used to fund the order.

Before implementing my suggestion, we should consider: What was the reason for using the wallet master private key instead of the account key in the first place?

@drltc drltc changed the title Assert Exception has_deposit: Make all trx one-time-keys derive from account key not wallet master key Oct 23, 2014
@vikramrajkumar
Copy link
Contributor

The problem leftover in the originally linked thread is that kisa still cannot recover his/her funds using the same wallet. I am handling this separately as part of the greater #712 issue.

The new focus of this issue is that we want to deterministically derive any new keys from the relevant account's active key. I've already made this change and will close this issue when the update gets merged into develop.

@nathanielhourt
Copy link
Contributor

It's not that the market orders are deposited to the master private key (the market engine doesn't know what that key is); it's that if you lose the private key to claim a market order, you need the master private key to regenerate it.

The solution here is to make the market order keys derive solely from the active key, so given the active key, I can recover all market orders I made with it.

@xeroc
Copy link
Member Author

xeroc commented Oct 24, 2014

I cannot completely follow your talk as I am not familiar with the code or the market transactions, but your proposals would still keep market participants private/anonymous, wouldn't it?

I'd just like to ensure that market transactions cannot be linked to account names ..

@nathanielhourt
Copy link
Contributor

Yes, of course. Market transactions would lose no anonymity as a result of
this change.

@vikramrajkumar vikramrajkumar modified the milestone: v0.4.24 Oct 27, 2014
@vikramrajkumar
Copy link
Contributor

bbdc03a

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

No branches or pull requests

4 participants