Lnd: Feature Request :Allow Bitcoin and Litecoin to be active together

[offerm]

Background

As of today, the code does not allow Litecoin and Bitcoin to be active together.
If you try you get an error message:

loadConfig: Currently both Bitcoin and Litecoin cannot be active together

I would like to suggest a PR that allows both networks to be active together. I plan to create it in a way that will have no impact on the user and/or configuration in case a single network is used (so fully backward compatible). In case the 2 networks are active the user will have an option, as part of the cli/rpc calls to provide the network to be used with the call.

I plan to implement a similar solution as done by Conner Fromknecht here, as part of the swapz branch

So, for example you will be able to:

lncli addinvoice 40000 --ticker=LTC

Any help is appreciated. If you have any design conclusions from the swapz experience, it will be great if you could share them.

Thanks

Offer

Your environment

• lnd version 0.4.2-beta commit=d98d4523e10426c4ce097ecc7ec1ba199a331c60
• Darwin admins-MacBook-Pro-2.local 16.7.0 Darwin Kernel Version 16.7.0: Tue Jan 30 11:27:06 PST 2018; root:xnu-3789.73.11~1/RELEASE_X86_64 x86_64
• btcd version 0.12.0-beta
• ltcd version 0.12.0-beta

Steps to reproduce

lnd --bitcoin.active --litecoin.active

Expected behaviour

Allow both to be active at the same time.

Actual behaviour

loadConfig: Currently both Bitcoin and Litecoin cannot be active together

[Roasbeef]

This is no small feat. Also it isn't a priority at all atm, there are much, much more important things that are prioritized over features like this.

[offerm]

Hi Roasbeef,

Thanks for the prompt response.

I'm well aware that this is not a small feature. Yet I find it an interesting use case which can ease the management and monitoring pains involved with running LNDs for both Litecoin and Bitcoin. I also understand your position that there are more important features in queue.
Still, for me this is an important feature which I believe I can develop on my own (based on work done by @cfromknecht) .

While the feature is not small, there are many parts of it which can be done by newcomers and can ease the learning curve of the system.

I believe it would take me a few weeks to provide a working branch and a PR for the master branch.

Thanks

Offer

[offerm]

Hi Again,

So, I have started to work on this feature which is, as @Roasbeef wrote above, challenging and touching "everything". In addition to the server changes that should be done, this feature also brings changes to the request and response formats of many CLI/RPC calls.

For example
lncli addinvoice 123
Should accept another parameter specifying if this is a BTC or LTC amount.
Similar, walletbalance can't aggregate the balance of BTC and LTC together

$ lncli walletbalance
{
"total_balance": "9995250",
"confirmed_balance": "9995250",
"unconfirmed_balance": "0"
}

I created a plan aiming to reduce the complexity and allowing this feature to be done in stages. Here are the phases I target:

1. Go over all relevant commands and make sure that a) the rpc response includes, when relevant, the name of the chain, and b) that all chain dependent data is shown under a chain section. When needed introduce a "chains" element that includes a list of chain elements. (see below more information).
   This change prepares the system for multi-chain but has no impact as long a single chain is being used.

2. Go over all relevant commands and allow the user to provide a "--ticker" option. This option is optional. The user may leave it empty or provide a value. When the system is using a single chain and the user didn't provide value for this option, the system will opt to the chain in use. If the system is using Bitcoin and the user provide --ticker-LTC he will face an error. If the system has multi-chain the parameter is mandatory. This change prepares the system for multi-chain but has no impact as long a single chain is being used and users are not providing the --ticker option.

3. Fix some issue detected in phases 1 and 2 above. For example Transaction does not properly record the chain, in payinvoice channel hops - the chain is not set, payments does not include chain information, queryroutes not ready for multi-chain, etc. These issues have no impact when using single chain but should be solved for multi-chain

4. Internal changes to support multi-chain. This will probably be broken for additional stages.

Here are some examples for changes of phase 1 (using 2 servers so don't compare data):

$ lncli walletbalance
{
    "total_balance": "9995250",
    "confirmed_balance": "9995250",
    "unconfirmed_balance": "0"
}

Vs

$ lncli walletbalance
{
    "chains": [
        {
            "chain": "litecoin",
            "total_balance": "721980300",
            "confirmed_balance": "721980300",
            "unconfirmed_balance": "0"
        }
    ]
}

$ lncli getinfo
{
    "identity_pubkey": "02a5b850fba0298e50f76ecd948c7959979889245274fac9e2850c8c3b8c341236",
    "alias": "[email protected]",
    "num_pending_channels": 0,
    "num_active_channels": 4,
    "num_peers": 1,
    "block_height": 605968,
    "block_hash": "561faa19ba8ea95442344d4601f5f10bb75571f3523ea51568b1c836daf2688e",
    "synced_to_chain": true,
    "testnet": true,
    "chains": [
        "litecoin"
    ],
    "uris": [
        "02a5b850fba0298e50f76ecd948c7959979889245274fac9e2850c8c3b8c341236@52.175.8.58:9735"
    ],
    "best_header_timestamp": "1528464271",
    "version": "0.4.2-beta commit=4bde4c1c262ddae5171240d47fcbd0b5ea1f1eae"
}

vs

$ lncli getinfo
{
    "identity_pubkey": "036924b0efdd03aa6ede5e13c3aa8483f038fcbd3d7204cc50ab3055fe1000c5c6",
    "alias": "Offer",
    "num_peers": 1,
    "chains": [
        {
            "chain": "litecoin",
            "num_pending_channels": 0,
            "num_active_channels": 4,
            "block_height": 605969,
            "block_hash": "85861231030f2f6286f74a1d50fb921a6c2a6b09a27c7aa1457c8e9ac3b6a455",
            "synced_to_chain": true,
            "testnet": true,
            "best_header_timestamp": "1528464573"
        }
    ],
    "uris": [
        "036924b0efdd03aa6ede5e13c3aa8483f038fcbd3d7204cc50ab3055fe1000c5c6@84.111.139.147:9735"
    ],
    "version": "0.4.2-beta commit="
}

listpayments identify the chain:

$ lncli listpayments
{
    "payments": [
        {
            "payment_hash": "9fdde71a03e8ec3e31932e9a88ae882931c87c6b88117ff96109a14a941bcf14",
            "value": "1234",
            "creation_date": "1528457100",
            "path": [
                "02a5b850fba0298e50f76ecd948c7959979889245274fac9e2850c8c3b8c341236"
            ],
            "fee": "0",
            "payment_preimage": "01083d2f11cb3973ab0ad428bfc18443f07d425222a155200b065607c43fd6cc",
            "chain": "litecoin"
        },
        {
            "payment_hash": "9fdde71a03e8ec3e31932e9a88ae882931c87c6b88117ff96109a14a941bcf14",
            "value": "1234",
            "creation_date": "1528457409",
            "path": [
                "02a5b850fba0298e50f76ecd948c7959979889245274fac9e2850c8c3b8c341236"
            ],
            "fee": "0",
            "payment_preimage": "01083d2f11cb3973ab0ad428bfc18443f07d425222a155200b065607c43fd6cc",
            "chain": "litecoin"
        },
    ]
}

decodepayrequest show the chain

$ lncli decodepayreq lntltc12340n1pd35euppp5nlw7wxsrarkruvvn96dg3t5g9ycuslrt3qghl7tppxs549qmeu2qdqqcqzjqh75gp57fjxrjpva8s5cjc3zna7jt3n98mz8pqz92puu736rwm40qq0uaujveezja2u9aytqxw73gct5d65jtqnkuqx7ne0uymftydwqqcs3e3j
{
    "destination": "02a5b850fba0298e50f76ecd948c7959979889245274fac9e2850c8c3b8c341236",
    "payment_hash": "9fdde71a03e8ec3e31932e9a88ae882931c87c6b88117ff96109a14a941bcf14",
    "num_satoshis": "1234",
    "timestamp": "1528457089",
    "expiry": "3600",
    "description": "",
    "description_hash": "",
    "fallback_addr": "",
    "cltv_expiry": "576",
    "route_hints": [
    ],
    "chain": "litecoin"
}

Please let me know if you have any comments/suggestions/etc. I plan to upload a PR later today or tomorrow.

Thanks

Offer

[Roasbeef]

I wouldn't rank this in the top 100 things that need to be done within lnd atm. As a result, we won't prioritize review of this vanity feature in lieu of bug fixes, substantial improvements, and issues that trend towards our next release.

[offerm]

@Roasbeef, I value your position and opinion. 0.5 list also looks impressive.

If I may, I would like to give you another point of view regarding phase 1 above. If you believe that sometime in the future LND will support multi chain you must agree that the current output of many system commands/RPCs is not ready for that. This implies that there will be an API level change in the future which will not be backward compatible with current versions.

The number of 3rd party projects which adopt LND as their lightning server is growing and is expected to grow. For the benefit of these projects we better make these API changes now, reducing the impact this will have when we come to deal with it in the future.

The risk in this change is minor.

I will soon upload a PR for your review. Hope you decide to consider it and add it to the 0.5 list.

Thanks

Offer

[robtex]

i agree on importance of making API multi chain asap, even if it is "faked" information.
where you just add the "chain" : "bitcoin" it is already compatible and dead easy to implement.
for total compatibility, we could in some cases add a new api and keep the old one as it is. for instance we could make walletbalancechains while the walletbalance return as before. there are already 3rd party products that will break if we change the api now.
or, probably a bad idea, add both result formats

$ lncli walletbalance
{
    "total_balance": "721980300",
    "confirmed_balance": "721980300",
    "unconfirmed_balance": "0"
    "chains": [
        {
            "chain": "litecoin",
            "total_balance": "721980300",
            "confirmed_balance": "721980300",
            "unconfirmed_balance": "0"
        }
    ]
}

[offerm]

@robtex, @Roasbeef I actually prepared the change as you suggested and kept the original field in place.

But I do think it is better to go for the right version and not leave these in as if people use them it will be harder to take them out later, and when we finally have multi-chain support these top level field will not have any meaning (since you can't just sum BTC and LTC amounts and present as "total_balance").

But if you like to consider a PR with these fields in, just let me know and I restore them.

The impact is only for getinfo, getnetworkinfo, listpeers, getnodeinfo, walletbalance. Se we can also be selective and consider for example to leave these fields for getinfo and walletbalance but take them out from the rest.

@Roasbeef Let me know please

Offer

[offerm]

Providing that #1820 is accepted, for me, this is not really needed anymore or priority can be reduced to P5 :-)
@Roasbeef