Ensure all displayed addresses are imported

[AdamISZ]

Fixes #1143 (and possibly others). Before this commit,
(index plus gap limit) addresses are imported on sync,
and addresses used by maker/taker in coinjoin are imported,
but when a deposit occurred, bumping the index, further
addresses were not imported. The effect was that it was
possible, if doing a series of deposits to multiple
external addresses in a Qt session, to end up depositing
to an address that was not yet imported. And this results
in the user needing to rescan for Core+JM to recognize the
coins.
After this commit, we ensure all 'gap limit forwards'
addresses, which are displayed as potential deposit addresses
in Joinmarket-Qt, are imported before the display.

[AdamISZ]

(the slight changes in wallet_service and ygrunner are tiny fixes/cleanups, i saw no reason to remove them)

[AdamISZ]

I want to give a bit more background as it really wasn't easy to debug this, and so recording some points I understood might prove helpful.

Importing of addresses into Core occurs: when we sync, and when we call get_internal_addr or get_external_addr in coinjoins/transactions. But in a long running application displaying coin balances and addresses (and this could apply to the RPC daemon as well as to the Qt app; because in the rpc we have /wallet/walletname/display which likewise calls the utility function wallet_utils.wallet_display, although generally not because people use a separate endpoint to get a deposit address, which calls get_external_addr, and as discussed in the commit comment above, that function does handle import correctly, already) we have another scenario where new addresses are being generated for usage:

In wallet_utils.wallet_display there is an initial step which checks utxos:

joinmarket-clientserver/jmclient/jmclient/wallet_utils.py

Line 480 in 693ce5e

utxos = wallet_service.get_utxos_by_mixdepth(include_disabled=True)

But under the hood that function extracts scripts for any coins that the wallet owns, using get_script_from_path, and that function, if we reach the current wallet index, will bump the index (this is our underlying mechanism that prevents address use). The upshot of this rather complex procedure, is that when new deposits arrive, they will bump the index via the wallet display function itself - which is correct, because we want to show the user 6 fresh addresses (6 past the current index, for default gap limit of 6), but it misses a crucial step: we need to make sure that every address displayed, is already imported, to avoid having to rescan to discover the coins in future.

The fix here is to just import the entire "gap" that is going to get displayed. See the comments:

joinmarket-clientserver/jmclient/jmclient/wallet_utils.py

Lines 515 to 523 in 693ce5e

	# ensure that we never display un-imported addresses (this will generally duplicate
	# the import of each new address gap limit times, but one importmulti call
	# per mixdepth is cheap enough.
	# This only applies to the external branch, because it only applies to addresses
	# displayed for user deposit.
	# It also does not apply to fidelity bond addresses which are created manually.
	if address_type == BaseWallet.ADDRESS_TYPE_EXTERNAL:
	wallet_service.bci.import_addresses([addr for addr in gap_addrs],
	wallet_service.get_wallet_name())

It may seem logical to be a bit more 'paranoid': to ensure that every call to get_script_and_update_map (which is always where we go to add a new script to our wallet's 'known scripts' list) is accompanied by an import. I played with that but decided against it; it's overkill and will result in a lot of extra RPC calls. After this PR we are ensuring that the two scenarios where new addresses matter: new addresses displayed to the user for deposit, and new addresses fetched for coinjoins, are imported.

(Side note: I've realised the fidelity bond section of wallet_display is taking multiple seconds per display refresh. this can be fixed, but it's separate from the topic of this PR).

[PulpCattel]

Test suite is passing.

Tested against master with 2 QT, sending from one to the other.
With master, when I deposit to a new displayed address (i.e., an address that appears after one previous address has received some funds, and that therefore wasn't there since the start) the balance shown is not updated (same behavior as explained here). I wasn't able to reproduce observe the error after a restart, i.e., a restart or a refresh always fixed the balance, but I didn't try very hard.

With this PR the balance is updated ~immediately, as it should.

[AdamISZ]

I wasn't able to reproduce the error after a restart, i.e., a restart or a refresh always fixed the balance, but I didn't try very hard.

I think I get what you're saying. It can be very tricky in test setups - one gotcha is that, if you do multiple wallets on regtest, they are working against the same Core instance, so transactions can get processed even when an address was not imported, because the transaction is registered by Core because the other wallet had an imported address associated with it.

[PulpCattel]

Yep, it was almost surely that or something close to that.