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

Wallet: require public key for watchonly #97

Merged
merged 3 commits into from
Mar 4, 2019

Conversation

pinheadmz
Copy link
Member

This is a cherry-pick of bcoin-org/bcoin#639

Addresses #636 in which a user creates private keys they could never access due to a wallet being watch-only.

This PR goes along with #637 and bcoin-org/bclient#15, maybe we can decide if we need all three safeties in place or which ones we do or don't need.

In this pull request, we prevent a watch-only wallet from ever generating a master (private) key which could lead to a user seeing addresses they can not spend from.

I think watchOnly should always require an accountKey. The reverse isn't necessarily required, for example in bclient, creating a wallet with accountKey automatically sets watchOnly to true. In bcoin wallet._createAccount(), accountKey is just ignored if watchOnly is false, but at least that doesn't lead to unrecoverable keys!

Note in hsw-cli the params for watch-only and account-key (in bcoin/bwallet-cli) are watch and key (in hsw-cli)

Example:

$ hsw-cli mkwallet --id=watchtest --watch=true
Error: Must add HD public keys to watch only wallet.
    at WalletClient.request (/Users/matthewzipkin/Desktop/work/hs-client/node_modules/bcurl/lib/client.js:213:19)
    at process._tickCallback (internal/process/next_tick.js:68:7)

$ hsw-cli mkwallet --id=watchtest --watch=true --key=tpubDDuC5zidzkFVJSETSQnpzNv5yESw3TKSAJsYmYT1XaLYmr7kZa3bZ7CFKSQGb2E9AmktZf1QMj9FuxmBEDZ6U5TAYNLtgfZcqQ
uLqJhoZ6c
{
  "network": "testnet",
  "wid": 6,
  "id": "watchtest",
  "watchOnly": true,
  "accountDepth": 1,
  "token": "69a02638a83dae00d6583974fc7862a9ec3afc85d258e86601a678cc631f287d",
  "tokenDepth": 0,
  "master": {
    "encrypted": false
  },
  "balance": {
    "account": -1,
    "tx": 0,
    "coin": 0,
    "unconfirmed": 0,
    "confirmed": 0,
    "lockedUnconfirmed": 0,
    "lockedConfirmed": 0
  }
}

CHANGELOG.md Outdated Show resolved Hide resolved
@boymanjor
Copy link
Contributor

LGTM

@boymanjor boymanjor merged commit 8673c57 into handshake-org:master Mar 4, 2019
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.

3 participants