wallet: clarify FundTransaction signature

[S3RK]

Follow up for #28560 (comment)

Pass vector<CTxIn> instead of CMutableTransaction through parameters. Pass locktime and nVersion as part of CoinControl object.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	josibake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #bitcoin-core/gui/733 (Deniability - a tool to automatically improve coin ownership privacy by denavila)
• #29936 (fuzz: wallet: add target for CreateTransaction by brunoerg)
• #29325 (consensus: Store transaction nVersion as uint32_t by achow101)
• #27792 (wallet: Deniability API (Unilateral Transaction Meta-Privacy) by denavila)
• #26732 ([WIP] wallet: tx creation, don't select outputs from txes that are being replaced by furszy)

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.

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/22285255999}

[fanquake]

wallet/test/fuzz/notifications.cpp:169:15: error: no matching function for call to 'FundTransaction'
  169 |         (void)FundTransaction(*wallet, tx, recipients, change_position, /*lockUnspents=*/false, coin_control);
      |               ^~~~~~~~~~~~~~~
./wallet/spend.h:227:40: note: candidate function not viable: no known conversion from 'CMutableTransaction' to 'const std::vector<CTxIn>' for 2nd argument
  227 | util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const std::vector<CTxIn>& inputs, const std::vector<CRecipient>& recipients, std::optional<unsigned int> change_pos, bool lockUnspents, CCoinControl);
      |                                        ^                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

[josibake]

Concept ACK, thanks for taking this as a follow up! Much cleaner than passing the whole transaction. Looks like you need to update the fuzzer code as well, since it is still trying to pass a transaction.

[josibake]

@S3RK I think this change could go in as-is, but IIRC from #28560 , there are places we are creating a CMutableTransaction just so we can pass it to FundTransaction. It would be nice to get rid of those too, since the only time we would actually need to start with a CMutableTransaction is when deserializing a transaction (e.g. fundrawtransaction) from hex and then adding funds. In almost all other cases where FundTransaction is used, we are creating a new transaction. Since CreateTransaction is what actually creates the transaction, creating a transaction just so we can pass the inputs and outputs to FundTransaction seems a bit gross.