coincontrol can filter for segwit inputs, expose fundraw option #11049

instagibbs commented at 11:27 PM on August 14, 2017: member

This is useful for external signers such as Ledger where only one type of input is allowed for any given transaction to be signed ~~or in the case where you want to explicitly make a non-malleable transaction.~~

promag commented at 11:37 PM on August 14, 2017: member

Drop Desired from enum?

sipa commented at 11:44 PM on August 14, 2017: member

Concept ACK

instagibbs force-pushed on Aug 14, 2017

fanquake added the label Wallet on Aug 14, 2017

in src/wallet/wallet.h:1232 in a372116c9c outdated

1227 | +    LEGACY, //! Only use non-segwit inputs
1228 | +    SEGWIT, //! Only use segwit inputs
1229 | +    ALL,    //! Use any inputs
1230 | +};
1231 | +
1232 | +bool InputTypeFromString(const std::string& mode_string, InputType& input_type);

promag commented at 11:54 PM on August 14, 2017:

const std::string& string.

in src/wallet/wallet.cpp:4367 in a372116c9c outdated

4362 | +    static const std::map<std::string, InputType> input_modes = {
4363 | +    {"LEGACY", InputType::LEGACY},
4364 | +    {"SEGWIT", InputType::SEGWIT},
4365 | +    {"ALL", InputType::ALL},
4366 | +    };
4367 | +    auto mode = input_modes.find(mode_string);

promag commented at 11:55 PM on August 14, 2017:

auto it = .

in src/wallet/wallet.cpp:4362 in a372116c9c outdated

4356 | @@ -4337,3 +4357,15 @@ bool CMerkleTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState&
4357 |  {
4358 |      return ::AcceptToMemoryPool(mempool, state, tx, true, nullptr, nullptr, false, nAbsurdFee);
4359 |  }
4360 | +
4361 | +bool InputTypeFromString(const std::string& mode_string, InputType& input_type) {
4362 | +    static const std::map<std::string, InputType> input_modes = {

promag commented at 11:55 PM on August 14, 2017:

input_types.

in src/wallet/wallet.h:1229 in a372116c9c outdated

1222 | @@ -1223,4 +1223,12 @@ bool CWallet::DummySignTx(CMutableTransaction &txNew, const ContainerType &coins
1223 |      return true;
1224 |  }
1225 |  
1226 | +enum class InputType {
1227 | +    LEGACY, //! Only use non-segwit inputs
1228 | +    SEGWIT, //! Only use segwit inputs
1229 | +    ALL,    //! Use any inputs

promag commented at 12:07 AM on August 15, 2017:

//! Use all input types

instagibbs force-pushed on Aug 15, 2017

promag commented at 12:37 AM on August 15, 2017: member

Concept ACK.

What about unspent_types (instead of input_type) and use txnouttype instead of adding the new InputType enum (which also adds legacy, both not well defined terms).

unspent_types should an array of strings, each parsed to a txnouttype value. I think RPC interface could be as low level as this.

instagibbs commented at 3:40 PM on August 15, 2017: member

@promag I see negative value to the user to have them iterate what type of segwit inputs they'd like, especially as that list grows. What the user will typically care about is whether or not the resulting transaction can be malleated.

I also think "LEGACY" is the best possibly name I've heard, since "NONSEGWIT" is kind of a mouthful. Ideas welcome.

instagibbs force-pushed on Aug 15, 2017

instagibbs commented at 6:07 PM on August 15, 2017: member

travis failure seems unrelated, p2p-leaktest.py

in src/wallet/rpcwallet.cpp:2815 in d91d0e5269 outdated

2810 | @@ -2811,6 +2811,10 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
2811 |                              "         \"UNSET\"\n"
2812 |                              "         \"ECONOMICAL\"\n"
2813 |                              "         \"CONSERVATIVE\"\n"
2814 | +                            "     \"input_type\"             (string, optional, default=ALL) The type of inputs used to fund the transaction, must be one of:\n"
2815 | +                            "         \"ALL\"\n"

jonasschnelli commented at 6:29 PM on August 15, 2017:

Maybe re-use the comments from https://github.com/bitcoin/bitcoin/pull/11049/files#diff-12635a58447c65585f51d32b7e04075bR1227 here?

in src/wallet/wallet.h:1226 in d91d0e5269 outdated

1222 | @@ -1223,4 +1223,12 @@ bool CWallet::DummySignTx(CMutableTransaction &txNew, const ContainerType &coins
1223 |      return true;
1224 |  }
1225 |  
1226 | +enum class InputType {

jonasschnelli commented at 6:30 PM on August 15, 2017:

I think that enum class belong to coincontrol.h

in src/wallet/wallet.h:1232 in d91d0e5269 outdated

1227 | +    LEGACY, //! Only use non-segwit inputs
1228 | +    SEGWIT, //! Only use segwit inputs
1229 | +    ALL,    //! Use all input types
1230 | +};
1231 | +
1232 | +bool InputTypeFromString(const std::string& mode_string, InputType& input_type);

jonasschnelli commented at 6:31 PM on August 15, 2017:

Same here: could be defined and implemented in coincontrol.h/cpp

in src/wallet/wallet.cpp:2221 in d91d0e5269 outdated

2214 | @@ -2215,6 +2215,26 @@ void CWallet::AvailableCoins(std::vector<COutput> &vCoins, bool fOnlySafe, const
2215 |                      continue;
2216 |                  }
2217 |  
2218 | +
2219 | +                if (coinControl) {
2220 | +                    std::vector<std::vector<unsigned char> > vSolutions;
2221 | +                    txnouttype whichType;

jonasschnelli commented at 6:36 PM on August 15, 2017:

Maybe it would be saner to set a default value in case Solver() returns false and hasn't set whichType?

jonasschnelli commented at 6:37 PM on August 15, 2017: contributor

Concept ACK

Leviathn commented at 4:18 AM on August 17, 2017: none

Concept ACK

NicolasDorier commented at 4:39 AM on August 17, 2017: contributor

Concept NACK.

or in the case where you want to explicitly make a non-malleable transaction.

It is untrue.

What you are interested is not SEGWIT output, what you are interested is third party malleabilitySafe utxos. All segwit utxo are not malleabilitySafe. A segwit utxo is malleability safe in one of the following conditions:

The utxo is SEGWIT and confirmed

OR

The utxo is SEGWIT, unconfirmed.
The utxo's transaction have only segwit inputs
All the unconfirmed ancestors have only segwit input (else another non-segwit input can be malleated and invalidate your transaction),
All those unconfirmed transactions disable RBF (else you might invalidate your own tx by mistake),
All those unconfirmed transactions are trusted (if not trusted, then you are weak to third party malleability).

This PR is misleading, people think they can select UTXOs which can't be malleated, but this would be untrue.

A useful parameter would be a malleabilitySafe boolean which select UTXOs based on the above conditions.

Your PR achieves in selecting segwit UTXO which might not be malleability safe. I don't see any use case on why this would be needed, except for signing faster on HW.

instagibbs commented at 4:29 PM on August 17, 2017: member

@NicolasDorier I can remove that reasoning from the PR description, but the first use-case would exist even with a more "complete" anti-malleability setting.

An additional future option could also be added to get only confirmed outputs, which would suffice for the first anti-malleability case.

instagibbs force-pushed on Aug 17, 2017

instagibbs commented at 11:27 PM on August 17, 2017: member

fixed up @jonasschnelli 's concerns

in src/wallet/wallet.cpp:2219 in f7acc0fab9 outdated

2214 | @@ -2215,6 +2215,26 @@ void CWallet::AvailableCoins(std::vector<COutput> &vCoins, bool fOnlySafe, const
2215 |                      continue;
2216 |                  }
2217 |  
2218 | +
2219 | +                if (coinControl) {

promag commented at 12:05 AM on August 18, 2017:

Correct me if I'm wrong but this block will incur in a substantial performance penalty? What about:

if (coinControl && coinControler->m_input_type != InputType::ALL) {

in src/wallet/coincontrol.cpp:9 in f7acc0fab9 outdated

   0 | @@ -0,0 +1,17 @@
   1 | +// Copyright (c) 2011-2017 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#include "coincontrol.h"
   6 | +
   7 | +bool InputTypeFromString(const std::string& input_string, InputType& input_type) {
   8 | +    static const std::map<std::string, InputType> input_types = {
   9 | +    {"LEGACY", InputType::LEGACY},

promag commented at 12:05 AM on August 18, 2017:

Indent.

promag commented at 12:19 AM on August 18, 2017: member

IMO just be able to filter the unspent type is great. Something like:

fundrawtransaction(rawtx, {"unspent_type":"*"});            # default (all)
fundrawtransaction(rawtx, {"unspent_type":"segwit_v0_*"});  # witness_v0_keyhash and
                                                            # witness_v0_scripthash
fundrawtransaction(rawtx, {"unspent_type":"multisig"});

See https://github.com/bitcoin/bitcoin/blame/master/src/script/standard.cpp#L25-L32.

So, no new type, no new "legacy", scalable and really "raw" as the call states it is.

In any case, some comments.

instagibbs force-pushed on Aug 18, 2017

instagibbs commented at 5:36 PM on August 18, 2017: member

@promag I'd really rather not have regex-y inputs presented to users unless there's a compelling use-case. Open to suggestions of course, I likely missed some.

Fixed comments.

in src/wallet/rpcwallet.cpp:2918 in 659a0b205d outdated

2912 | @@ -2907,6 +2913,11 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
2913 |                  throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter");
2914 |              }
2915 |          }
2916 | +        if (options.exists("input_type")) {
2917 | +            if (!InputTypeFromString(options["input_type"].get_str(), coinControl.m_input_type)) {
2918 | +                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid input_type parameter");

promag commented at 11:59 PM on August 18, 2017:

Missing test for this error.

NicolasDorier commented at 12:44 AM on August 19, 2017: contributor

@instagibbs I don't see why someone would want to filter by type on fundraw?

Is it so that the ultimate signer don't have to know how to use legacy signing? But all HW does right now.

coincontrol can filter for segwit inputs, expose fundraw option 2cca90c9c6

instagibbs force-pushed on Aug 19, 2017

instagibbs commented at 10:31 PM on August 22, 2017: member

Closing for now, as for my use-case it's actually not needed: At least for Ledger one can sign by simply "signing twice", segwit inputs then non-segwit inputs. No need to special case coin selection. h/t @greenaddress

instagibbs closed this on Aug 22, 2017

DrahtBot locked this on Sep 8, 2021