Skip to content

Conversation

aureleoules
Copy link
Contributor

@aureleoules aureleoules commented May 21, 2022

This PR adds a filter to the CoinControl to select only specific utxos by type. It allows the fundrawtransaction and walletcreatefundedpsbt rpc calls to filter inputs by type.

Closes #25181.

@theStack
Copy link
Contributor

Concept ACK

@DrahtBot
Copy link
Contributor

DrahtBot commented May 21, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25933 (wallet: AvailableCoins, simplify output script type acquisition by furszy)
  • #25730 (RPC: listunspent, add "include immature coinbase" flag by furszy)
  • #25291 (test: Refactor rpc_fundrawtransaction.py by akankshakashyap)
  • #25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@S3RK
Copy link
Contributor

S3RK commented May 23, 2022

@aureleoules can we achieve the same result by using descriptors wallet with only segwit descriptors?

Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

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

Concept ACK

I think you should squash the commits since the test won't work without the implementation.

@promag
Copy link
Contributor

promag commented May 23, 2022

Concept ACK.

@brunoerg I think it's fine as it. Should squash if the 1st commit requires the 2nd to pass CI.

@aureleoules
Copy link
Contributor Author

@aureleoules can we achieve the same result by using descriptors wallet with only segwit descriptors?

I am not very familiar with wallet descriptors.
Do you mean using a wallet that does not have legacy descriptors imported?
I think @achow101 answed that yes (#25181 (comment)).

But if I understand correctly, this feature might still be useful in cases where you don't control the wallet.

@aureleoules aureleoules force-pushed the 2022-05-witness-only-fundrawtransaction branch from 6677717 to 1c5cfd8 Compare May 25, 2022 14:08
@aureleoules aureleoules changed the title rpc: Witness-only inputs for fundrawtransaction rpc: Witness-only inputs option for fundrawtransaction May 25, 2022
@aureleoules aureleoules changed the title rpc: Witness-only inputs option for fundrawtransaction rpc: Segwit-only inputs option for fundrawtransaction May 25, 2022
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request May 26, 2022
Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

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

I noticed that this PR changes similar code parts as #24584. Perhaps, the changes to availableCoins in #24584 could be useful in implementing the idea a bit more elegantly. It may be good to coordinate this PR with the author of #24584, perhaps #25183 should build on #24584 or vice versa.

@ishaanam
Copy link
Contributor

ishaanam commented Jun 5, 2022

Would it be useful to add the "segwit_inputs_only" option to walletcreatefundedpsbt as well? They both call FundTransaction so I think you would just have to include it in the walletcreatefundedpsbt options.

@aureleoules
Copy link
Contributor Author

I've rebased my PR on top of #24584 as @xekyo suggested and addressed the comments.
Thanks for the reviews!

@aureleoules aureleoules force-pushed the 2022-05-witness-only-fundrawtransaction branch from a9547f9 to 3efcd5f Compare July 18, 2022 08:34
@aureleoules aureleoules marked this pull request as draft July 30, 2022 09:35
@aureleoules aureleoules force-pushed the 2022-05-witness-only-fundrawtransaction branch from a99b734 to 8f907bb Compare July 30, 2022 12:17
@aureleoules
Copy link
Contributor Author

Thank you @josibake for the review and the commits! I added your commits and fixed the list parsing.
I will address the test a bit later.

@aureleoules
Copy link
Contributor Author

I have rebased the PR on top of @josibake's PR #25734.
The RPC fundrawtransaction and walletcreatefundedpsbt now take an option to filter inputs during the coin selection.
I've added a test case.

@aureleoules aureleoules force-pushed the 2022-05-witness-only-fundrawtransaction branch 2 times, most recently from 89ea8fc to 28540bc Compare September 5, 2022 10:23
add m_filter_inputs to coinControl, which then
allows us to filter by OutputType in AvailableCoins
@aureleoules aureleoules force-pushed the 2022-05-witness-only-fundrawtransaction branch from 28540bc to 03ab6b4 Compare September 6, 2022 09:14
@aureleoules aureleoules force-pushed the 2022-05-witness-only-fundrawtransaction branch from 03ab6b4 to 9e7fd5c Compare September 6, 2022 09:25
@S3RK
Copy link
Contributor

S3RK commented Sep 7, 2022

Do you have a concrete use-case in mind? I'm not sure, but it seems #25181 supposes that we do control the wallet.

I believe if your wallet already has mixed inputs and you want to filter these, this feature may be useful.

I was thinking about this proposal and I'm still not convinced there is a clear understanding how this would be actually used in the real world.

The declared benefit is non-malleability for lightning wallets (#25181). For new lightning wallets this is a non-issue as they shouldn't have non-segwit descriptors in the first place. IIUC the proposed solution for existing wallets is to filter malleable inputs when creating channel opening txs in fundrawtransaction and walletcreatefundedpsbt. This approach renders a portion of wallet funds unusable and have a number of problems:

  1. users could be confused about funding errors despite having enough balance
  2. it doesn't provide a solution how to make all the funds usable
  3. what about other RPCs? what if non-malleable inputs are added by walletprocesspsbt or when bumping psbt?

It seems what you really want is to "unmix" the wallet, i.e. sweep all malleable inputs as a one-time operation.
Today, I think, there are two main approaches users can take to achieve this:

  1. (preferred, descriptors only) create a new wallet and import only segwit descriptors from the old wallet
  2. create a new lighting wallet and fund it with desired amount to one or more addresses

While 2nd approach has some downsides, maybe it's just good enough. I don't know whether there are enough users affected and whether these downsides outweigh the added complexity.

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@aureleoules aureleoules closed this Dec 8, 2022
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Witness-Only Option for fundrawtransaction