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

Support spend_unconfirmed in EstimateFee and FundPsbt #4905

Merged

Conversation

mrfelton
Copy link
Contributor

@mrfelton mrfelton commented Jan 9, 2021

This is a rebased version of #4845. I'm not sure what happened to that original PR but it seems the repo where it came from is no longer online.

FundPsbt and EstimateFee now support spending from the underlying wallet using unconfirmed inputs or inputs with a specific number of minumum confirmations; this is made possible by adding the params min_confs and spend_unconfirmed to the underlying proto messages, and implementing checks in their function calls.

It changes the interface of WalletController slightly; it adds the param minConfs to CreateSimpleTx inside the WalletController interface, thus allowing the underlying implementation of the function to pass minConfs to its underlying wallet's createsimpletx implementation (if possible and/or desired).

@mrfelton mrfelton force-pushed the feat/spend-unconfirmed-estimate branch 3 times, most recently from fd8ffb5 to 28f7d72 Compare January 9, 2021 20:02
@guggero guggero mentioned this pull request Jan 11, 2021
15 tasks
@Roasbeef Roasbeef added this to the 0.13.0 milestone Jan 28, 2021
@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour P2 should be fixed if one has time wallet The wallet (lnwallet) which LND uses labels Jan 28, 2021
@mrfelton mrfelton force-pushed the feat/spend-unconfirmed-estimate branch 2 times, most recently from c90afb7 to 84caf9d Compare January 28, 2021 11:02
@mrfelton mrfelton force-pushed the feat/spend-unconfirmed-estimate branch from 84caf9d to 00bbb15 Compare February 5, 2021 18:23
@guggero guggero self-requested a review February 8, 2021 08:30
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for picking this up!
Did a first pass, looks pretty good.
Main comments are about splitting the commit and reusing existing code.

lnrpc/rpc.proto Show resolved Hide resolved
lnrpc/walletrpc/walletkit_server.go Outdated Show resolved Hide resolved
lnwallet/btcwallet/btcwallet.go Show resolved Hide resolved
lnwallet/interface.go Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
@guggero guggero requested a review from wpaulino February 9, 2021 15:11
@alexbosworth
Copy link
Contributor

I'd love to see this change, it would help me doing CPFPs a lot

@wpaulino
Copy link
Contributor

wpaulino commented Apr 5, 2021

Ping @mrfelton. If you're still available to work on this, we can try to get it merged for v0.13.

@mrfelton
Copy link
Contributor Author

mrfelton commented Apr 6, 2021

Sure, I'll take a look. Missed the last batch of updates on this one somehow. Have been running with this patch for some months though.

@mrfelton mrfelton force-pushed the feat/spend-unconfirmed-estimate branch from 00bbb15 to f57c98b Compare April 22, 2021 17:28
@mrfelton mrfelton force-pushed the feat/spend-unconfirmed-estimate branch from f57c98b to 2f80283 Compare April 22, 2021 18:36
@mrfelton mrfelton requested a review from guggero April 22, 2021 18:46
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very useful feature, thank you. LGTM 💯

@wpaulino wpaulino enabled auto-merge April 23, 2021 20:51
@Roasbeef Roasbeef disabled auto-merge April 26, 2021 22:48
@Roasbeef Roasbeef merged commit bfcaf02 into lightningnetwork:master Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to existing features / behaviour P2 should be fixed if one has time wallet The wallet (lnwallet) which LND uses
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants