[Wallet] FundRawTransaction can fund a transaction with preset inputs found in the CoinView #10202

pull NicolasDorier wants to merge 5 commits into bitcoin:master from NicolasDorier:fundrawtransaction changing 5 files +122 −20

NicolasDorier commented at 1:48 PM on April 13, 2017: contributor

This PR makes it possible to call FundRawTransaction with pre filled inputs not belonging to the wallet. This is very useful for AnyOneCanPay scenario, where one of a third party only cover part of a transaction.

The necessary information to complete the transaction is taken from of the mempool and coinview.

A typical example would involves Alice and Bob wanting to fund 0.5 each to the payment channel. Alice would give her input, and Bob would be able to complete the missing amount via FundRawTransaction.

A follow up PR will allow the client fundrawtransaction to pass previous TxOuts corresponding to the inputs. This is necessary for filling transaction in some 2nd layer protocols, where the the inputs's of the transaction to fund have not yet been broadcasted.

This supersede my previous attempt at #10068 which I closed, as it was harder to review.

The first two commits are strict refactoring. The third is the one checking information in the coinview. Rest is about tests.
fanquake added the label RPC/REST/ZMQ on Apr 14, 2017
fanquake added the label Wallet on Apr 14, 2017

in src/wallet/wallet.cpp:2242 in 2f2e25ddc0 outdated

2248 | -                return false;
2249 | -            nValueFromPresetInputs += pcoin->tx->vout[outpoint.n].nValue;
2250 | -            setPresetCoins.insert(CInputCoin(pcoin, outpoint.n));
2251 | -        } else
2252 | -            return false; // TODO: Allow non-wallet inputs
2253 | +        auto foundCoin = coinControl->FindKnownCoin(outpoint);

TheBlueMatt commented at 7:45 PM on April 17, 2017:

Wont this break Qt coin control as-is? I believe it either needs a similar AddKnownCoins call or the old code needs to remain and just get a second option.

TheBlueMatt commented at 8:28 PM on April 17, 2017: member

Concept ACK.

Its really quite unfortunate we cant properly calculate fee when using most outputs here. Maybe its possible to assume, at a minimum, a compressed pubkey and standard signature size for "normal" things like P2PH? Alternatively, could force the user to provide some additional data, like a set of pubkeys needed.

Also, Windows line endings in a few commits :p
NicolasDorier force-pushed on Apr 18, 2017
NicolasDorier commented at 2:23 AM on April 18, 2017: contributor

Fixed the end-of-line and fixed wallet.cpp to not break QT coin control as suggested by @TheBlueMatt . In a follow up PR I will clean QT coin control dialog to use AddKnownCoin. I briefly tried to do that, it simplified quite a lot. I prefer keeping it in another PR though.
in src/wallet/coincontrol.h:85 in f3595b6ceb outdated
```
  76 | @@ -76,8 +77,24 @@ class CCoinControl
  77 |          vOutpoints.assign(setSelected.begin(), setSelected.end());
  78 |      }
  79 |  
  80 | +    void AddKnownCoins(const CInputCoin& coin)
  81 | +    {
  82 | +        knownCoins.insert(std::make_pair(coin.outpoint, coin));
  83 | +    }
  84 | +
  85 | +    boost::optional<CInputCoin> FindKnownCoin(const COutPoint& outpoint) const
```
jonasschnelli commented at 9:41 AM on April 19, 2017:

Can we avoid boost::optional? Maybe std::shared_ptr<CInputCoin> should do it? Maybe switch the map keys to std::pair<uint256, int>?

NicolasDorier commented at 11:29 AM on April 19, 2017:

why? We are already using boost::optional from https://github.com/bitcoin/bitcoin/pull/10165/files#diff-b2bb174788c7409b671c46ccc86034bdR2088 . This will be implemented in std:: in C++0x17 as well. I was kind of against it on #10165, but it turns out to be very handy.

NicolasDorier commented at 11:44 AM on April 19, 2017:

Another solution is

bool FindKnownCoin(coinst COutPoint&, CInputCoin& output)
in src/wallet/coincontrol.h:97 in f3595b6ceb outdated
```
  92 | +    }
  93 | +
  94 |  private:
  95 |      std::set<COutPoint> setSelected;
  96 | +    //! A map of known UTXO
  97 | +    std::map<COutPoint, CInputCoin> knownCoins;
```
jonasschnelli commented at 9:43 AM on April 19, 2017:

Why not use setSelected (and maybe extend/migrate it to fit your use-case), AFAIK they serve the same purpose (selecting an desired input over the GUI).

I think this is not something we should fix in a follow-up PR because it kinda messes up the design.

NicolasDorier commented at 11:31 AM on April 19, 2017:

I briefly tried, but it changes lot's of stuff in the QT Code. I prefer doing it in a separate PR, as I fear the changes will be too hard to review.

The design of this PR will not be greatly impacted once I switch setSelected to use CInputCoin everywhere. Basically only this block https://github.com/bitcoin/bitcoin/pull/10202/files#diff-b2bb174788c7409b671c46ccc86034bdR2247 will need to be removed.
jonasschnelli commented at 10:10 AM on April 19, 2017: contributor

Concept ACK. I think we should not have two code parts for handling pre-selected-inputs and I think we should avoid using boost::optional (if reasonable possible).
kanzure commented at 5:13 PM on May 12, 2017: contributor

Its really quite unfortunate we cant properly calculate fee when using most outputs here. Maybe its possible to assume, at a minimum, a compressed pubkey and standard signature size for "normal" things like P2PH? Alternatively, could force the user to provide some additional data, like a set of pubkeys needed.

How about user gives an estimated final transaction size (minus whatever fundrawtransaction adds) when calling fundrawtransaction? Then ignore all "unsolvable" inputs or whatever.

Alternatively, what about allowing fundrawtransaction feeRate to be 0 even for "unsolvable" inputs etc in the transaction, and user sets another parameter like ignoreFee--- because perhaps the user has arranged for fee to come from somewhere else, or maybe the user plans to delete one of the outputs to convert the output to fee.

For my particular problem (discussed in IRC) I believe the following workaround makes sense: createrawtransaction with a change output set to the final fee value that I would like, fundrawtransaction with a reasonable feeRate set, then using the fee value in the fundrawtransaction output dictionary it is easy to switch the change output from my original "fee" to the new fee (important to account for difference or else funds could be lost here). This would move any extra fee from "fundrawtransaction" to the change output, and my original change output "fee" has now become the real fee.
NicolasDorier commented at 8:23 AM on May 17, 2017: contributor

@kanzure I think nothing has to be done to calculate fee correctly. Currently, the user can just fill dummy input of the right size into the scriptSig, this would correctly evaluate the fees.
[Wallet] Pass CCoinControl as parameter of CWallet::FundTransaction 114b79c829
NicolasDorier force-pushed on May 17, 2017
NicolasDorier commented at 8:56 AM on May 17, 2017: contributor

I rebased, and rewrote this part https://github.com/bitcoin/bitcoin/pull/10202/files#diff-b2bb174788c7409b671c46ccc86034bdL2259 to be more diff friendly so you can see it is trivially right.
NicolasDorier commented at 8:58 AM on May 17, 2017: contributor

@jonasschnelli I did not managed to find good alternative to boost::optional. I tried bool FindKnownCoin(coinst COutPoint&, CInputCoin& output) but since CInputCoin can't be in uninitialized state, it does not work.

I don't think it is such a big deal, we use boost::optional elsewhere and std::optional will eventually exist.
NicolasDorier force-pushed on May 17, 2017
[Wallet] Fetch CInputCoins from CWallet before calling FundTransaction afb40b40e7
[Wallet] Fetch CInputCoins from CCoinView before calling FundTransaction 60ceea8824
[Wallet] FundRawTransaction returns unknown-input error on invalid input 1dd91edf89
[Wallet] Test FundRawTransaction with inputs from the CoinView aa3a812759
NicolasDorier force-pushed on May 17, 2017
jonasschnelli commented at 7:43 PM on August 15, 2017: contributor

Needs rebase.
NicolasDorier commented at 5:12 PM on March 6, 2018: contributor

I workedaround this need for this.
NicolasDorier closed this on Mar 6, 2018
domob1812 commented at 3:58 PM on February 5, 2019: contributor

This would indeed be very useful. Was there a particular reason for closing this, or was it simply closed because it was no longer needed for you? Would it make sense for me to take up the patch, rebase it and resubmit? Or are there any technical objections to the code / approach itself?
NicolasDorier commented at 11:00 AM on February 9, 2019: contributor

@domob1812 I closed it because for my projects I decided to stop using RPC's wallet API completely, and did my own wallet. It was the fastest way to reach my goal, so I did not wanted to spend time rebasing this over and over. Feel free to clone this PR and take care of it.
domob1812 commented at 10:18 AM on February 10, 2019: contributor

@NicolasDorier: Thanks for the answer, that makes sense. I also have other, more important things to work on for now, but then I'll consider taking over this PR when I run into the need to do this on my own projects.
JeremyRand commented at 6:26 PM on October 3, 2021: contributor

Am I correct in inferring that if #17211 gets merged, that would eliminate the need for this PR? Or does this PR do something that that PR doesn't do?
DrahtBot locked this on Oct 30, 2022

Contributors

Labels

Linked (view graph)

#10068 [Wallet] FundRawTransaction accepts preset non-wallet inputs #10216 [Wallet] FundRawTransaction can accept pre-set inputs whose parent is not yet broadcasted