rpc: Filter inputs by type during CoinSelection #25183

aureleoules commented at 3:44 AM on May 21, 2022: member

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.

DrahtBot added the label RPC/REST/ZMQ on May 21, 2022

DrahtBot added the label Wallet on May 21, 2022

fanquake requested review from TheBlueMatt on May 21, 2022

theStack commented at 8:02 AM on May 21, 2022: contributor

Concept ACK

DrahtBot commented at 4:02 PM on May 21, 2022: contributor

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 commented at 7:53 AM on May 23, 2022: contributor

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

brunoerg commented at 8:58 PM on May 23, 2022: contributor

Concept ACK

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

promag commented at 10:58 PM on May 23, 2022: member

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

aureleoules commented at 1:08 AM on May 24, 2022: member

@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 (https://github.com/bitcoin/bitcoin/issues/25181#issuecomment-1134923482).

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

in src/wallet/rpc/spend.cpp:796 in 6677717fba outdated

 791 | @@ -787,7 +792,8 @@ RPCHelpMan fundrawtransaction()
 792 |                                          "so the maximum DER signatures size of 73 bytes should be used when considering ECDSA signatures."
 793 |                                          "Remember to convert serialized sizes to weight units when necessary."},
 794 |                                  },
 795 | -                             },
 796 | +                            },
 797 | +                            {"inputs_witness_only", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether to only use inputs with witness data for transaction."},

luke-jr commented at 12:16 PM on May 25, 2022:

The name here seems confusing to me. Maybe "segwit_inputs_only"?

aureleoules commented at 2:10 PM on May 25, 2022:

Indeed, I changed it.

aureleoules force-pushed on May 25, 2022

aureleoules renamed this:
~~rpc: Witness-only inputs for fundrawtransaction~~
rpc: Witness-only inputs option for fundrawtransaction
on May 25, 2022

aureleoules renamed this:
~~rpc: Witness-only inputs option for fundrawtransaction~~
rpc: Segwit-only inputs option for fundrawtransaction
on May 25, 2022

luke-jr referenced this in commit bf94eed131 on May 26, 2022

in test/functional/rpc_fundrawtransaction.py:201 in 1c5cfd84b3 outdated

 192 | +        self.log.info("Testing fundrawtxn with witness inputs only")
 193 | +
 194 | +        inputs  = [ ]
 195 | +
 196 | +        # make sure legacy inputs are not accepted in witness only mode if no witness inputs are found
 197 | +        self.generatetoaddress(self.nodes[2], 2, self.nodes[2].getnewaddress(address_type='legacy'))

murchandamus commented at 3:08 PM on May 31, 2022:

This may actually be testing something else than intended: given that the coinbase output is not 100 blocks old yet, I suspect the "Insufficient funds" would be foremost caused by the inputs being immature.

This test could be improved by creating a scenario in which a transaction is first successfully funded by using legacy inputs, but then toggling the 'segwit_inputs_only': true causes a repeated attempt to fund the transaction to fail.

aureleoules commented at 2:05 PM on June 1, 2022:

Thank you for reviewing the code!

This may actually be testing something else than intended: given that the coinbase output is not 100 blocks old yet, I suspect the "Insufficient funds" would be foremost caused by the inputs being immature.

If I increase the amount of blocks to generate in legacy (400+), the "Insufficient funds" error is still raised. There are already blocks generated before the new test here so there should be enough legacy coins. https://github.com/bitcoin/bitcoin/blob/1c5cfd84b3dc14ab886acd47c7891c11eb2457ec/test/functional/rpc_fundrawtransaction.py#L103-L104 Wrong node my bad.

This test could be improved by creating a scenario in which a transaction is first successfully funded by using legacy inputs, but then toggling the 'segwit_inputs_only': true causes a repeated attempt to fund the transaction to fail.

I'll look into that, this seems better.

murchandamus commented at 3:12 PM on May 31, 2022: contributor

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 commented at 5:41 PM on June 5, 2022: contributor

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.

DrahtBot added the label Needs rebase on Jun 17, 2022

aureleoules force-pushed on Jul 6, 2022

DrahtBot removed the label Needs rebase on Jul 6, 2022

aureleoules force-pushed on Jul 6, 2022

aureleoules commented at 3:38 PM on July 6, 2022: member

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

DrahtBot added the label Needs rebase on Jul 6, 2022

aureleoules force-pushed on Jul 18, 2022

DrahtBot removed the label Needs rebase on Jul 18, 2022

aureleoules force-pushed on Jul 18, 2022

aureleoules force-pushed on Jul 19, 2022

S3RK commented at 6:48 AM on July 21, 2022: contributor

But if I understand correctly, this feature might still be useful in cases where you don't control the wallet. @aureleoules Do you have a concrete use-case in mind? I'm not sure, but it seems #25181 supposes that we do control the wallet.

furszy commented at 2:13 PM on July 22, 2022: member

Instead of adding the segwit_inputs_only filter only, what if we add a general input type filter? Something like filter_inputs_by=<OutputType>

So we aren't forced to add a new type*_inputs_only flag in every RPC command every time that a new inputs type filter is required.

aureleoules force-pushed on Jul 29, 2022

aureleoules renamed this:
~~rpc: Segwit-only inputs option for fundrawtransaction~~
rpc: Filter inputs by type during CoinSelection
on Jul 29, 2022

aureleoules force-pushed on Jul 29, 2022

aureleoules commented at 10:46 AM on July 29, 2022: member

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.

in src/wallet/coincontrol.h:15 in 7d1e4a107a outdated

  11 | @@ -12,6 +12,7 @@
  12 |  #include <script/keyorigin.h>
  13 |  #include <script/signingprovider.h>
  14 |  #include <script/standard.h>
  15 | +#include <unordered_set>

fanquake commented at 10:46 AM on July 29, 2022:

This should go with the other std lib includes.

aureleoules commented at 10:47 AM on July 29, 2022:

Fixed, thanks.

aureleoules commented at 10:46 AM on July 29, 2022: member

Instead of adding the segwit_inputs_only filter only, what if we add a general input type filter? Something like filter_inputs_by=<OutputType>

So we aren't forced to add a new type*_inputs_only flag in every RPC command every time that a new inputs type filter is required.

Thanks for the feedback, I've added it.

aureleoules force-pushed on Jul 29, 2022

josibake commented at 2:48 PM on July 29, 2022: member

Concept ACK

This is coming along nicely! I do have a few suggestions:

I think the correct place to do the filtering is in AvailableCoins. This would make it more useful outside of just funding a transaction. I recently opened #25734 which should make this much easier. All we would need is your update to coincontrol.h and a small change to AvailableCoins and then any RPC can filter by just adding filters to m_filter_inputs (like you've done with fundrawtransaction.

I rebased this off #25734 , added my suggested changes, and verified that the functional tests still ran. Feel free to cherry pick:

josibake@e4369e6 (add filter to AvailableCoins)
josibake@2609ab1 (your commit rebased on my changes)

Also, small nit (feel free to ignore): I'd suggest breaking up your commits to make it easier to review.. something like 1) make changes to AvailableCoins 2) make changes to the RPC 3) make changes to the functional test. Makes it much easier from a review perspective, imo

in src/wallet/rpc/spend.cpp:509 in a99b734ec1 outdated

 503 | +    while (ss.good()) {
 504 | +        getline(ss, item, ',');
 505 | +        result.push_back(item);
 506 | +    }
 507 | +    return result;
 508 | +}

josibake commented at 2:53 PM on July 29, 2022:

this function should also handle lists like bech32, bech32m, as most users will probably pass them with spaces. i tested that this currently fails by passing bech32, bech32m in the functional test.

aureleoules commented at 12:18 PM on July 30, 2022:

Thanks, I fixed it

in src/wallet/coinselection.h:151 in a99b734ec1 outdated

 146 | @@ -145,6 +147,8 @@ struct CoinSelectionParams {
 147 |       * associated with the same address. This helps reduce privacy leaks resulting from address
 148 |       * reuse. Dust outputs are not eligible to be added to output groups and thus not considered. */
 149 |      bool m_avoid_partial_spends = false;
 150 | +    /** Filter transaction inputs by type */
 151 | +    std::unordered_set<OutputType> m_filter_inputs;

josibake commented at 2:54 PM on July 29, 2022:

if you do the filtering in AvailableCoins, we don't need to add this to coinselection.h

in src/wallet/spend.cpp:804 in a99b734ec1 outdated

 800 | @@ -776,6 +801,7 @@ static BResult<CreatedTransactionResult> CreateTransactionInternal(
 801 |  
 802 |      CoinSelectionParams coin_selection_params{rng_fast}; // Parameters for coin selection, init with dummy
 803 |      coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
 804 | +    coin_selection_params.m_filter_inputs = coin_control.m_filter_inputs;

josibake commented at 2:55 PM on July 29, 2022:

same as above, this isn't needed if we filter in AvailableCoins

in test/functional/rpc_fundrawtransaction.py:195 in a99b734ec1 outdated

 190 | +            if not info['iswitness']:
 191 | +                return False
 192 | +
 193 | +        return True
 194 | +
 195 | +    def test_witness_only(self):

josibake commented at 2:57 PM on July 29, 2022:

it would be great to test the case where we filter inputs to a particular type (or multiple types) and then verify that the inputs in the funded tx only contain those types. you can look at test/functioanal/wallet_avoid_mixing_output_types.py for an example of checking the inputs to a transaction and verifying they are all of a certain OutputType

aureleoules commented at 3:08 PM on August 1, 2022:

Thanks, I've added these tests. I included some helpers from wallet_avoid_mixing_output_types to the test framework to reuse them.

aureleoules marked this as a draft on Jul 30, 2022

aureleoules force-pushed on Jul 30, 2022

aureleoules commented at 12:19 PM on July 30, 2022: member

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 force-pushed on Aug 1, 2022

aureleoules commented at 3:12 PM on August 1, 2022: member

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 marked this as ready for review on Aug 1, 2022

DrahtBot added the label Needs rebase on Aug 5, 2022

aureleoules force-pushed on Aug 22, 2022

DrahtBot removed the label Needs rebase on Aug 22, 2022

in src/wallet/coincontrol.h:43 in 432310ff0b outdated

  38 | @@ -38,6 +39,8 @@ class CCoinControl
  39 |      //! If true, the selection process can add extra unselected inputs from the wallet
  40 |      //! while requires all selected inputs be used
  41 |      bool m_allow_other_inputs = false;
  42 | +    //! Filter inputs by type
  43 | +    std::unordered_set<OutputType> m_filter_inputs;

furszy commented at 1:33 PM on September 3, 2022:

Small idea, what about using an optional bit-field here? So you later can skip coins with a fast:

if (m_filter_inputs && !(*m_filter_inputs & coin_type)) continue;

aureleoules commented at 9:29 AM on September 5, 2022:

great idea, I pushed it git diff 0a9d345 89ea8fc

aureleoules force-pushed on Sep 5, 2022

in src/wallet/coincontrol.h:20 in 28540bcaca outdated

  16 | @@ -17,6 +17,7 @@
  17 |  #include <algorithm>
  18 |  #include <map>
  19 |  #include <set>
  20 | +#include <unordered_set>

furszy commented at 3:21 PM on September 5, 2022:

can remove this include now.

in src/wallet/rpc/spend.cpp:569 in 28540bcaca outdated

 564 | +            UniValue v = options["filter_inputs_by"];
 565 | +            std::stringstream ss(v.get_str());
 566 | +            const auto& outputTypes = parseStringList(ss);
 567 | +            coinControl.m_filter_inputs = 0;
 568 | +            for (size_t i = 0; i < outputTypes.size(); i++) {
 569 | +                *coinControl.m_filter_inputs |= static_cast<unsigned int>(parseOutputType(outputTypes[i]));

furszy commented at 3:32 PM on September 5, 2022:

No need to implement the function parseStringList, we already have such functionality. Can directly provide the array and read it in the following way:

UniValue array = options["filter_inputs_by"].get_array();
for (size_t i = 0; i < array.size(); i++) {
    const auto& type = parseOutputType(array[I].get_str());
    if (!type) throw exception;
    coinControl->m_filter_inputs |= static_cast<unsigned int>(*type);
}

(remember to change the test to provide an array instead of a string separated by commas as well)

aureleoules commented at 9:21 AM on September 6, 2022:

thanks, i pushed it

wallet: add filter to AvailableCoins

add m_filter_inputs to coinControl, which then
allows us to filter by OutputType in AvailableCoins

a25afbef62

aureleoules force-pushed on Sep 6, 2022

rpc: Filter inputs by type during CoinSelection 9e7fd5c0fe

aureleoules force-pushed on Sep 6, 2022

S3RK commented at 7:22 AM on September 7, 2022: contributor

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:

users could be confused about funding errors despite having enough balance
it doesn't provide a solution how to make all the funds usable
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:

(preferred, descriptors only) create a new wallet and import only segwit descriptors from the old wallet
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 added the label Needs rebase on Sep 21, 2022

DrahtBot commented at 3:42 PM on September 21, 2022: contributor

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

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

aureleoules closed this on Dec 8, 2022

luke-jr referenced this in commit e9026765e5 on Aug 16, 2023

bitcoin locked this on Dec 8, 2023