Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LND: coin control backwards compatibility #2533

Merged

Conversation

kaloudis
Copy link
Contributor

@kaloudis kaloudis commented Nov 14, 2024

Approach 1

Originally, this PR bumped supported LND version for on-chain coin control to v0.18.3, detailed below:

In #2416 we added ability for users to spend full UTXOs on-chain, by leveraging the new
send_all and outpoints params on the SendCoins endpoint. Previously, we would use the PSBT endpoints to make these types of transactions (albeit, without the send max functionality). Users of LND below v0.18.3 now hit unknown field: outpoints when sending an on-chain tx with a UTXO selected. See below:

image

Instead of adding complexity to the code base in the above PR and other changes made to our PSBT flow, we require LND users to update to v0.18.3 to use coin controls.


Approach 2: limit external accounts support to LND v0.18.3+

  • Includes checks in handleAnything for supportsAccounts
  • Maintains backwards compatible for on-chain support without max send and without external accounts for LND versions before v0.18.3

Approach 3

  • Full backwards compat
  • defaultAccount flag
  • Includes checks in handleAnything for supportsAccounts

This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Configuration change
  • Locales update
  • Quality assurance
  • Other

Checklist

  • I’ve run yarn run tsc and made sure my code compiles correctly
  • I’ve run yarn run lint and made sure my code didn’t contain any problematic patterns
  • I’ve run yarn run prettier and made sure my code is formatted correctly
  • I’ve run yarn run test and made sure all of the tests pass

Testing

If you modified or added a utility file, did you add new unit tests?

  • No, I’m a fool
  • Yes
  • N/A

I have tested this PR on the following platforms (please specify OS version and phone model/VM):

  • Android
  • iOS

I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):

  • Embedded LND
  • LND (REST)
  • LND (Lightning Node Connect)
  • Core Lightning (CLNRest)
  • LndHub
  • [DEPRECATED] Core Lightning (c-lightning-REST)
  • [DEPRECATED] Core Lightning (Spark)
  • [DEPRECATED] Eclair

Locales

  • I’ve added new locale text that requires translations
  • I’m aware that new translations should be made on the ZEUS Transfix page and not directly to this repo

Third Party Dependencies and Packages

  • Contributors will need to run yarn after this PR is merged in
  • 3rd party dependencies have been modified:
    • verify that package.json and yarn.lock have been properly updated
    • verify that dependencies are installed for both iOS and Android platforms

Other:

  • Changes were made that require an update to the README
  • Changes were made that require an update to onboarding

@kaloudis kaloudis added LND Issues specific to LND nodes Coin control labels Nov 14, 2024
@kaloudis
Copy link
Contributor Author

Attempted adding backwards compatibility by adding a check for BackendUtils.supportsOnchainSendMax() on TransactionStore:368:

        if (
            BackendUtils.isLNDBased() &&
            transactionRequest.utxos &&
            transactionRequest.utxos.length > 0 &&
            transactionRequest.account === 'default' &&
            // only hit this block if on LND v0.18.3+
            BackendUtils.supportsOnchainSendMax()
        ) {
            transactionRequest.utxos.forEach((input) => {
                const [txid_str, output_index] = input.split(':');
                const inputs = [];
                inputs.push({ txid_str, output_index: Number(output_index) });
                transactionRequest.outpoints = inputs;
            });
        } else if (
            (BackendUtils.isLNDBased() &&
                transactionRequest.utxos &&
                transactionRequest.utxos.length > 0) ||
            (transactionRequest?.additional_outputs?.length &&
                transactionRequest?.additional_outputs?.length > 0)
        ) {
            return this.sendCoinsLNDCoinControl(transactionRequest);
        }

but hit the following error on a send: Can not finalize taproot input #0. No tapleaf script signature provided.

@kaloudis kaloudis added this to the v0.9.3 milestone Nov 14, 2024
@kaloudis kaloudis force-pushed the lnd-bump-supports-version-onchain-coin-control branch from a1595ce to f4800cd Compare November 14, 2024 02:36
@kaloudis
Copy link
Contributor Author

New approach detailed in OP

@kaloudis kaloudis changed the title LND: bumped supported version for on-chain coin control LND: coin control backwards compatibility Nov 14, 2024
@kaloudis kaloudis marked this pull request as ready for review November 14, 2024 02:47
@kaloudis kaloudis force-pushed the lnd-bump-supports-version-onchain-coin-control branch from 146e440 to 5f55a48 Compare November 14, 2024 02:50
@kaloudis kaloudis added the Bug Something isn't working label Nov 14, 2024
@kaloudis kaloudis merged commit 7dfbc48 into ZeusLN:master Nov 15, 2024
4 checks passed
@kaloudis kaloudis deleted the lnd-bump-supports-version-onchain-coin-control branch November 15, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Coin control LND Issues specific to LND nodes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant