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

[NicolasDorier]

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.

[TheBlueMatt]

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

[TheBlueMatt]

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.

[NicolasDorier]

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.

[jonasschnelli]

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

[NicolasDorier]

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]

Another solution is

bool FindKnownCoin(coinst COutPoint&, CInputCoin& output)

[jonasschnelli]

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]

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]

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]

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]

@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.

[NicolasDorier]

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]

@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.

[jonasschnelli]

Needs rebase.

[NicolasDorier]

I workedaround this need for this.

[domob1812]

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]

@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]

@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]

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?