Add dev-listaddrs option

[lvaccaro]

Add dev-listaddrs command to show bip32 addresses list (up to bip32_max_index). From pr #940

help

$ lightning-cli help
...
command=dev-listaddrs
description=Show addresses list up to derivation {index} (default is the last bip32 index)
...

output

$ lightning-cli dev-listaddrs
{ "addresses" : 
        [ 
                { 
                         "keyidx" : 0,
                         "pubkey" : "...",     
                         "p2sh": "...",   
                         "p2sh_redeemscript": "...",
                         "bech32": "...",
                         "bech32_redeemscript": "..." 
                }, 
                .... 
        ]
}

Set optional value {index} to show also future derivations up to {index}. This could be useful for testing and recovery. Default {index} value is bip32_max_index in vars table of sqlite file.

suggestion

If you prefer, I can add an address_type option (bech32 or p2sh, like get_newaddr) to show only one kind of derivation address.
In this case, it shows only 2 fields (address,redeemscript) instead 4 fields (p2sh,p2sh_redeemscript,bech32,bech32_redeemscript) as described at discussion.

[Sjors]

Quite useful, thanks.

Rather than an address_type argument and/or showing both bech32 and p2sh variants, is there a way to know which variant was used?

[Sjors]

I'm missing an address that has funds from THEIR_UNILATERAL/OUTPUT_TO_US, though this may be specific to #1000.

[cdecker]

@Sjors yes, the their_unilateral/to_us is not part of the HD tree derived from the hsm_secret, but it's the only exception.

[ZmnSCPxj]

Mostly indentation changes --- we use 8-wide tabs and align to just after opening (

[ZmnSCPxj]

Not preferred indentation. Should be:

if (!json_get_params(cmd, buffer, params,
                     "?bip32_max_index", &bip32tok,
                     NULL)) {

(the extreme indentation rather makes the newline useless)

[ZmnSCPxj]

Extra newline...

[ZmnSCPxj]

Why fail the command at this point? You are only listing existing addresses, not generating a new one. Maybe better to break out of this loop instead?

[lvaccaro]

dev-listaddrs supports bip32_max_index option to show all existing or not existing addresses up to bip32_max_index for testing. The default value, show only existing addresses according to bip32_max_index value inside vars table in sqlite. It is better break out instead command fail.

[ZmnSCPxj]

Please indent to align just after the opening ( of the call. Tabs are eight spaces.

[ZmnSCPxj]

Why sizeof(struct ripemd160) + 1, why not tal_count(redeemscript)?

[ZmnSCPxj]

ACK fe224b5

[lvaccaro]

Last commit resolves previous discussion, @rustyrussell suggests me to change tal_tmpctx(NULL) into tal_tmpctx(cmd). I have tested it.

[ZmnSCPxj]

Can also remove this tal_free now: command_* resolution functions destroy the cmd, and since tmpctx has cmd as parent, tmpctx is automatically freed by the command_* resolution functions.

[lvaccaro]

Thanks for explanation, in the last commit I move tal_free up to command_success. Now I can remove it.

[ZmnSCPxj]

Also add a test in tests/test_lightningd.py for successful use of dev-listaddrs command, as well as failure cases (invalid argument, out of range argument, etc). You can use l.rpc._call to get direct access to method name and parameters object.

[ZmnSCPxj]

You also need to remove the tal_free(tmpctx) calls in the failure routes. Note that json_get_params itself will call command_fail_detailed if it returns false, and since tmpctx is now rooted from cmd, you should not be calling tal_free(tmpctx) there either. Also holds for your uses of command_fail.

[lvaccaro]

Thanks, I understand and fix.

[ZmnSCPxj]

The CI is pointing out that you do not actually use tmpctx even if you allocate it.... remove it entirely?

[cdecker]

ACK 594ac0a