RPC-API: add ability to recover wallet

[AdamISZ]

Fixes #1082.
This commit allows recovery of a wallet from a seedphrase with a new endpoint wallet/recover. 4 parameters are passed in, the same three as for wallet/create but also a bip39 seedphrase as the fourth argument.

[AdamISZ]

A couple of initial comments:

I'm marking this as a solution to #1082 but I have not yet written another endpoint for blockchain rescan, I will add that in a new commit.

This is still draft because I have only done one primitive test, it will need tests added into CI as well.

Also, the response format for this new /recover endpoint is identical to the /create Response type, so it redundantly sends back the seedphrase as well as the cookie. Didn't seem like a big deal.

[AdamISZ]

Before I actually write a second commit for rescanblockchain I want to raise the points that I'm unsure about, please chime in if you have an opinion:

• The need to do a rescan after recovering will be dependent on whether you are recovering into the same Bitcoin Core instance with the same wallet file.
• So, if a person is doing the above, they should not need any rescan action at all
• If they are not doing the above, then they will likely see a zero balance and need to call the bitcoind rpc command rescanblockchain with an argument of a block height that is before the first usage of the wallet; and then, wait until that action completes before doing anything else.

The last of those three is of course the tricky one, but also tricky is how we distinguish the former from the latter.

The reason for setting up the websocket element was to address cases like this where the jmwalletd backend fires off an event elsewhere (here, a rescan on the bitcoind process) and has to notify a client when it completes. So that would be a natural part of how to address this, but .. I would appreciate any input on how you think it should work, for a client like JAM.

[AdamISZ]

I'm almost ready to add a /rescanblockchain route but I currently need a way to check that the rescan is completed by using the bitcoind rpc; does anyone know the easiest way to do that? (I seem to remember at least one rpc call will tell you 'rescan in progress' but I can't remember which one).

[kristapsk]

There is getrpcinfo. It will return rescanblockchain under active_commands while it is running. But not sure how reliable this is and aren't there better ways.

[AdamISZ]

Calling rescanblockchain in a running process is proving to be a much more difficult task than I had anticipated (or more accurately, I'd just forgotten the nuances here):

• The rescanblockchain RPC call is blocking. You are expected to just do it in another thread if you want to use the RPC otherwise
• Our jsonrpc module uses a single connection and is not asynchronous, either. Even if I put the rescanblockchain RPC in a separate thread, our monitoring RPC calls in the WalletService start failing from the main thread. (To be clear, we have tried very hard to remove all threading from the application anyway, so it wouldn't be desirable to go this route).

Not addressing this at all is hardly desirable either: as long as the rescan operation is continuing, the entire jmwalletd process is blocked and can't respond to client queries (and for mainnet, this could be an hour plus...).

[AdamISZ]

There is getrpcinfo. It will return rescanblockchain under active_commands while it is running. But not sure how reliable this is and aren't there better ways.

Right, thanks, I found getwalletinfo which works very well (it has a 'scanning' field, true/false). But see previous comment as to why this still isn't enough.

[AdamISZ]

Marked this as ready for review. In rudimentary testing with POSTMAN on signet, the following work:

• Use /recover to recover with a seedphrase
• Start a rescan from a specific blockheight with /rescanblockchain
• Check the status of rescanning is True/False using a call to /session

My resolution to the above difficulties was to start a daemon-type subthread for this one specific call, and create a new JsonRPC object to handle that request. It seems to work.

This needs review, but also I should try to add some kind of test to the test suite.

[AdamISZ]

Added some basic testing into test_wallet_rpc module.

[AdamISZ]

OK, squashed and did a few more manual tests. I'm now confident that this is correct. Wil address @kristapsk 's syntax comments and then wait for any testing feedback.

[AdamISZ]

OK, squashed and did a few more manual tests. I'm now confident that this is correct. Wil address @kristapsk 's syntax comments and then wait for any testing feedback.

Done

[kristapsk]

@AdamISZ What tools do you use to test this? I remember there was some simple JM RPC client, but can't find it now, don't remember how it was called or who made it.

[AdamISZ]

@AdamISZ What tools do you use to test this? I remember there was some simple JM RPC client, but can't find it now, don't remember how it was called or who made it.

I use POSTMAN. Here's a screenshot:

You can enter the Body of POST requests using json formatted text.

[AdamISZ]

But you're right, someone wrote some python code to test it, I have that memory too .. who was it? I don't think it was @PulpCattel ?

[PulpCattel]

Yes, I made this repo to test the RPC interface when it was introduced. I will eventually update it.

[kristapsk]

• Check the status of rescanning is True/False using a call to /session

Looking at getwalletinfo help page. Wouldn't it be useful (and simple) to also return duration and progress if rescanning is in progress?

  "scanning" : {                          (json object) current scanning details, or false if no scan is in progress
    "duration" : n,                       (numeric) elapsed seconds since scan start
    "progress" : n                        (numeric) scanning progress percentage [0.0, 1.0]
  },

[AdamISZ]

• Check the status of rescanning is True/False using a call to /session

Looking at getwalletinfo help page. Wouldn't it be useful (and simple) to also return duration and progress if rescanning is in progress?

  "scanning" : {                          (json object) current scanning details, or false if no scan is in progress
    "duration" : n,                       (numeric) elapsed seconds since scan start
    "progress" : n                        (numeric) scanning progress percentage [0.0, 1.0]
  },

Yes, I noticed this also when running the tests. I think it would be a really nice augmentation to simply rescanning=true/false, but it can be done in a later update.

A related point occurred to me today: almost everyone running JAM is doing so on a packaged node which is providing other services as well as Joinmarket. Is it OK for us to just decide to do a rescan at any time? Might that not affect other running services? @openoms any thoughts?

[openoms]

In my experience a rescan can add load, but finishes within an hour in the worst case even on a RasperryPi and bitcoind remains functional. I am monitoring 20+ wallets with Specter and could rescan the wallets on demand without problems.

It is ok to trigger them as needed.

[kristapsk]

I think it would be a really nice augmentation to simply rescanning=true/false, but it can be done in a later update.

Agree, that can be added later.

[AdamISZ]

In my experience a rescan can add load, but finishes within an hour in the worst case even on a RasperryPi and bitcoind remains functional. I am monitoring 20+ wallets with Specter and could rescan the wallets on demand without problems.

It is ok to trigger them as needed.

That's good to know, thanks. The main reason I was asking, though, is that I was concerned if maybe some RPC calls might be disabled during rescan; thinking about it, they're probably wallet RPCs, and I guess that most applications are not using bitcoin core's wallet feature?

[kristapsk]

I guess that most applications are not using bitcoin core's wallet feature?

That's true, RaspiBolt at one point even had disabled wallet feature in Core by default, which was a problem for JoinMarket guide.

[AdamISZ]

Perhaps a little impatient, but it's been a while and this seems to be highly desired, so I'm going to go ahead and merge without further test or review (we've had a little).

[theborakompanioni]

Hey @AdamISZ,

I have a question regarding how to correctly use this from a client perspective.
Testing this by trying to recover a wallet that has some funds on the following addresses:

• 1st address of Account 0 (m/84'/1'/0'/0/0)
• 201st change address of Account 4 (m/84'/1'/4'/1/201)

After importing, the funds on address m/84'/1'/0'/0/0 are present - great.
But how do I make sure the UTXO on m/84'/1'/4'/1/201 is found?
I tried increasing the gap limit to 205 before rescanning, locking/unlock the wallet, etc. - without success.
First idea was to just generate some addresses manually (in all accounts) before rescanning, but since the second UTXO is in the "change" account, I am not able to do that.

I know this is a theoretical scenario, but even if the funds are on other addresses (beyond the gap limit), I was not able to recover them successfully.

Now before I try to be too clever, I just thought I'd ask what to do.
What is your suggestion on how to import the wallet described above and recover the whole balance?

[AdamISZ]

Hey @AdamISZ,

I have a question regarding how to correctly use this from a client perspective. Testing this by trying to recover a wallet that has some funds on the following addresses:

* 1st address of Account 0 (`m/84'/1'/0'/0/0`)

* 201st change address of Account 4 (`m/84'/1'/4'/1/201`)

After importing, the funds on address m/84'/1'/0'/0/0 are present - great. But how do I make sure the UTXO on m/84'/1'/4'/1/201 is found? I tried increasing the gap limit to 205 before rescanning, locking/unlock the wallet, etc. - without success. First idea was to just generate some addresses manually (in all accounts) before rescanning, but since the second UTXO is in the "change" account, I am not able to do that.

I know this is a theoretical scenario, but even if the funds are on other addresses (beyond the gap limit), I was not able to recover them successfully.

Now before I try to be too clever, I just thought I'd ask what to do. What is your suggestion on how to import the wallet described above and recover the whole balance?

I was able to replicate this effect on regtest, for now outside the API (because I want to understand it "raw" first, before considering API commands) (also, I will document exactly what I did later, but for now, just an outline comment):

I find that if I do the following: (1) run wallet-tool with gap limit set to 202 (whatever is higher than your funding), (2) run rescanblockchain N with N sufficient to find the txs and then (3) run wallet-tool with gap limit set to 202 again, then it works.

While awkward the logic makes sense, right: the first run of wallet tool display is going to import as many addresses into the backend Core wallet as it thinks it needs; and with -g 202 you are telling it to import all that way up. Now, after the imports have occurred, the rescanblockchain command is looking for all those addresses (if you do it first, before using -g, it doesn't have any knowledge of the index 201 addresses), so it finds all the transactions. The third step, of running wallet-tool display with still -g 202 set, will pick up the new transactions. I guess at this point there should be no further funds-finding issues since after that you have the wallet_index set in the JM wallet file, so it persists. I need to check a few more things.

But anyway just a quick initial report on what I'm seeing. I'll look at the same sequence of events via API next.

[AdamISZ]

@theborakompanioni ok I'm reasonably sure I understand what's going on now, after trying via the API. I can indeed reproduce what you're saying.

So the problem is how we're handling gap limit. The method to open the wallet (open_wallet, open_test_wallet_maybe) has to explicitly pass in the gap_limit value to the BaseWallet constructor, if it wants to be anything other than default 6; we do this explicitly in a few places in the codebase, but not in the calls that come via the RPC. Moreover, this gap limit setting is only coming from command line options, via the optparse parser, not from a setting which exists (wrongly placed? I'll check later) in the config file, so doing configset in the API won't matter anyway.
So what I'll do is re-do my API calls setup with the gaplimit setting being taken from the config file, so that you can edit it with configset, and if that works, I'll PR.

[AdamISZ]

See #1525 . I think it should do what is needed. Sorry for this screw up, I can imagine quite a few people had problems because of it!

[theborakompanioni]

• Check the status of rescanning is True/False using a call to /session

Looking at getwalletinfo help page. Wouldn't it be useful (and simple) to also return duration and progress if rescanning is in progress?

  "scanning" : {                          (json object) current scanning details, or false if no scan is in progress
    "duration" : n,                       (numeric) elapsed seconds since scan start
    "progress" : n                        (numeric) scanning progress percentage [0.0, 1.0]
  },

Yes, I noticed this also when running the tests. I think it would be a really nice augmentation to simply rescanning=true/false, but it can be done in a later update.

Related joinmarket-webui/jam#743
Any further thoughts on this? Ability to show a progress percentage would be nice (e.g. "Rescanning in progress (21%)")

May I open a ticket?

[kristapsk]

May I open a ticket?

Yes, of course.