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

Implement wallet protection rpc methods #4198

Closed
6 tasks done
ghubstan opened this issue Apr 27, 2020 · 5 comments · Fixed by #4214
Closed
6 tasks done

Implement wallet protection rpc methods #4198

ghubstan opened this issue Apr 27, 2020 · 5 comments · Fixed by #4214
Assignees

Comments

@ghubstan
Copy link
Member

ghubstan commented Apr 27, 2020

After implementing gRPC authentication in 4189, it makes sense to implement wallet encryption / decryption endpoints in the next development phase.

A new PR will will close this issue after the following tasks are completed.

  • Add error handling to getbalance method.
    Balances are not available while :daemon is initializing or when a wallet is locked. Currently, the user sees Error: UNKNOWN for any server-side error. Tell users to wait or how to unlock their wallet.

  • Implement setwalletpassword method: setwalletpassword "password" "newpassword" with error handling. There are two use cases:
    * A single "password" argument is provided to encrypt an unencrypted wallet.
    * An optional second "newpassword" argument is is provided to change the password on an encrypted wallet.
    This is the analog to bitcoin core's encryptwallet and walletpassphrasechange methods, combined into one.

  • Implement removewalletpassword method: removewalletpassword "password" with error handling.
    Automated testing with (bats) is a necessity for :cli, and despite the lack of a use case for a removewalletpassword method in Bisq, and the security risks it exposes, a removewalletpassword method will allow devs to quickly run the test suite without having to wait for :daemon's full initialization to complete before any wallet related operations can be performed. This method should not be mentioned in help text.

  • Implement unlockwallet "password" timeout method, where timeout is in seconds.
    This is the analog to bitcoin core's walletpassphrase method.

  • Implement lockwallet method.
    This is the analog to bitcoin core's walletlock method. After calling this method, users will need to call unlockwallet again before being able to call any methods which require the wallet to be unlocked.

  • Port the expect based test.sh script in the cli module to bats (easier to read and write).

@cbeams cbeams changed the title CLI needs wallet encryption password methods Implement wallet protection rpc methods Apr 27, 2020
@cbeams
Copy link
Member

cbeams commented Apr 27, 2020

Thanks, @ghubstan. I've updated the title to better reflect what needs to happen here. The core task is implementing wallet protection rpc methods, and as the rpc reference client (and hopefully as a useful tool), we always support new rpc methods in the cli.

I also removed the Description section from the description above, as it's redundant (I know it's part of the issue template, but is intended more for bug reporting, not a feature request issue like this one. Likewise, I removed the Version section as well, as its intent is for reporting the version of Bisq affected by a given bug being reported. Whether or not this change will make it into v1.3.3 remains to be seen.

I'll see about getting our previous PR merged asap, so that I can push my changes on the rpc server side. You said in another chat that you're going to work on the bats conversion first, that's will buy a little time (and should be its own PR separate from the one dealing with implementing these new methods). But if you do start working on the server side implementation of these new methods, please be aware that we'll have merge conflicts if you don't build on top of my changes. Again, they're in my refactor-rpc-server branch. I'll get them pushed up to a PR soon.

@ghubstan
Copy link
Member Author

@cbeams , I have server side impls of some of these new methods in another branch, but I've been waiting to modify/add them to your changes in a new branch after you merge PR 4189 -- to avoid those merge conflicts and extra work to fix them.

ghubstan added a commit to ghubstan/bisq that referenced this issue Apr 28, 2020
This reverts commit ac87c23.

This change has to be in another PR, as per
bisq-network#4198 (comment)
@ghubstan
Copy link
Member Author

There is also a getbalance error handling bug that needs fixing: Error: null is printed to the :cli console when the daemon is losing peers (SOCKET_TIMEOUT).

@ghubstan
Copy link
Member Author

ghubstan commented May 6, 2020

  • despite the lack of a use case for a removewalletpassword method in Bisq

@cbeams , There is a removewalletpassword function in the UI, in the same view where you set your password.

@cbeams cbeams added this to In progress in Ship Bisq Daemon and API May 8, 2020
@cd2357
Copy link
Contributor

cd2357 commented May 13, 2020

There is also a getbalance error handling bug that needs fixing: Error: null is printed to the :cli console when the daemon is losing peers (SOCKET_TIMEOUT).

@ghubstan seems this is related to the Tor issue you mentioned, which sometimes affects BitcoinJ.

Two solutions come to mind. When exposing this API, Bisq could:

  1. require a local bitcoin node
    • can be on localhost, or any IP/hostname; only requirement is that peerbloomfilters=1
  2. use the default bitcoin nodes, but not use Tor when connecting to them
    • via Bisq UI: Settings > Network info > uncheck "Use Tor for Bitcoin network"
    • via Bisq command line args: --useTorForBtc=false (looks promising, but never tried / used it)

In both those cases, BitcoinJ would entirely bypass Tor to do basic wallet operations (calculate balance, etc).

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

Successfully merging a pull request may close this issue.

3 participants