RPC-API: read gaplimit from config

[AdamISZ]

Prior to this commit, an attempt to change the gap limit used in wallet syncing, by setting gaplimit in the config via configset, would fail, since gaplimit was only being read from command line options. After this commit, the value of gaplimit in the POLICY section of the config, defaulting to 6, will be read and used in Wallet object creation, either via the createwallet or recoverwallet endpoints, or via the wallet opening operation in the unlockwallet endpoint. This can be used in combination with the rescanblockchain endpoint to allow discovery of funds in addresses beyond the default gap.

[AdamISZ]

This addresses the issue identified by @theborakompanioni here.

[AdamISZ]

Perhaps a note can be dropped somewhere that,

in the scenario: you have a seedphrase, you recover, it doesn't show all funds, and you think it's because there are gaps of more than 6 addresses then:

• You try to recover normally (/recoverwallet) and see missing balance
• Without locking the wallet, you call configset with "POLICY", "gaplimit", "N" for some high N.
• Use /rescanblockchain with some appropriately old block height as argument
• /lock the wallet (maybe delete the new file, or choose a new filename)
• /recoverwallet again with new filename, then you should see the extra balance/addresses recognized (after calling /display; but also jmwalletd output, as well as bitcoin debug.log will show the add-to-wallet events)

[theborakompanioni]

tACK! Works as expected. Thanks @AdamISZ for fixing this so quickly and cleanly 🙏

Perhaps a note can be dropped somewhere that,

in the scenario: you have a seedphrase, you recover, it doesn't show all funds, and you think it's because there are gaps of more than 6 addresses then:

* You try to recover normally (`/recoverwallet`) and see missing balance

* Without locking the wallet, you call `configset` with "POLICY", "gaplimit", "N" for some high N.

* Use `/rescanblockchain` with some appropriately old block height as argument

* `/lock` the wallet (maybe delete the new file, or choose a new filename)

* `/recoverwallet` again with new filename, then you should see the extra balance/addresses recognized (after calling `/display`; but also jmwalletd output, as well as bitcoin debug.log will show the add-to-wallet events)

These steps do not work as described, as deleting a wallet is not possible via the API - which is totally fine. Recovering a second time with a different filename leaves the user with two wallets, which should probably also be avoided. So currently (work in progress!), in Jam, the following approach is taken automatically, without further user intervention:

• /recoverwallet
• /configset ("POLICY", "gaplimit", "N")
• /lock
• /unlock
• /rescanblockchain
• /display

This successfully recovered all funds described in the mentioned scenario. Do you think this approach is sound?

Heads up: I first used "YIELDGENERATOR", "gaplimit", "N" instead of "POLICY", "gaplimit", "N" for /configset as the gaplimit arg is in section YIELDGENERATOR in the default config - I will create a PR moving it to the POLICY section, if that is okay for you. Ah.. I just saw now, that you already fixed this here! Wonderful! 🧡

[theborakompanioni]

Would it be reasonable/possible to add the gaplimit to the /recoverwallet endpoint?

[AdamISZ]

Ah.. I just saw now, that you already fixed this here!

Ah yes my bad, although it's referred to indirectly, I should have made that very explicit in the commit comment, as it is a separate edit.

[AdamISZ]

Do you think this approach is sound?

Yes, I think it is. It's a little unobvious how /rescanblockchain fits into the flow, but this way does indeed work; after you /unlock there'll be a Joinmarket wallet sync event, which will import all the extra addresses, and after /rescanblockchain the transactions at those addresses will be seen by Core, and because the WalletService is already running, the monitor loop will pick up the new transactions and add them to the JM wallet.

[AdamISZ]

Would it be reasonable/possible to add the gaplimit to the /recoverwallet endpoint?

It did cross my mind. It's possible. But it does bring into clearer perspective, the slight ugliness we have in the architecture here: how the config is used. As you know, there's a default in-code, then there's a .cfg file that reads in as a delta to that default. Then it's sitting in memory as long as joinmarketd is running, and you can edit it via configset, but that will not persist over restarts of joinmarketd (I know we discussed this before). But even more confusing is, the config setting of gaplimit is global and will persist across wallets, but for some reason the configset endpoint is at .../walletname/configset, i.e. it requires a loaded wallet. That last part doesn't make sense and we can PR to change it in a separate PR.

I guess the reason I didn't decide to include gaplimit in the recover endpoint is as per the above, the gaplimit is a global setting. The awkwardness of having to make sure a wallet's loaded to use it, comes from the above described error in design; we should just fix that, then it's a one-shot instruction to globally change it any time you need to.

[theborakompanioni]

Would it be reasonable/possible to add the gaplimit to the /recoverwallet endpoint?

It did cross my mind. It's possible. But it does bring into clearer perspective, the slight ugliness we have in the architecture here: how the config is used. As you know, there's a default in-code, then there's a .cfg file that reads in as a delta to that default. Then it's sitting in memory as long as joinmarketd is running, and you can edit it via configset, but that will not persist over restarts of joinmarketd (I know we discussed this before).

Yep, we talked about that. I was more or less thinking about an optional parameter which defaults to the config file value if it is not provided in the request. No persistence would be necessary. However, you are right, we should not talk about it here in this PR.

But even more confusing is, the config setting of gaplimit is global and will persist across wallets, but for some reason the configset endpoint is at .../walletname/configset, i.e. it requires a loaded wallet. That last part doesn't make sense and we can PR to change it in a separate PR.

Yes, I guess it has to do with authentication/authorization. So it is not completely arbitrary and "kind of makes sense".

I guess the reason I didn't decide to include gaplimit in the recover endpoint is as per the above, the gaplimit is a global setting. The awkwardness of having to make sure a wallet's loaded to use it, comes from the above described error in design; we should just fix that, then it's a one-shot instruction to globally change it any time you need to.

Totally okay! I do not want to hijack this PR. It works, and that is all that is needed for now. Thank you 🙏

Looking forward to this being merged! 🚀

[AdamISZ]

OK thanks, I will take that as a tACK and merge.

Will open an issue on the /configset issue so we don't forget.