wallet: clarify FundTransaction signature #29564

1. 
   S3RK commented at 9:01 am on March 5, 2024: contributor

   Follow up for #28560 (comment)

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

2. 
   DrahtBot commented at 9:01 am on March 5, 2024: contributor

   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.

3. DrahtBot added the label Wallet on Mar 5, 2024
4. 
   DrahtBot commented at 9:35 am on March 5, 2024: contributor

   🚧 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

5. DrahtBot added the label CI failed on Mar 5, 2024
6. fanquake requested review from josibake on Mar 5, 2024
7. 
   fanquake commented at 9:37 am on March 5, 2024: member

   0wallet/test/fuzz/notifications.cpp:169:15: error: no matching function for call to 'FundTransaction'
   1  169 |         (void)FundTransaction(*wallet, tx, recipients, change_position, /*lockUnspents=*/false, coin_control);
   2      |               ^~~~~~~~~~~~~~~
   3./wallet/spend.h:227:40: note: candidate function not viable: no known conversion from 'CMutableTransaction' to 'const std::vector<CTxIn>' for 2nd argument
   4  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);
   5      |                                        ^                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   61 error generated.
   

8. 
   josibake commented at 3:05 pm on March 5, 2024: member

   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.
9. wallet: clarify FundTransaction signature

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

   e85054efd1
10. S3RK force-pushed on Mar 6, 2024
11. DrahtBot removed the label CI failed on Mar 6, 2024
12. 
    josibake commented at 2:57 pm on March 6, 2024: member

    @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.
13. 
    DrahtBot commented at 9:41 am on June 12, 2024: contributor

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

14. DrahtBot added the label Needs rebase on Jun 12, 2024
15. 
    DrahtBot commented at 1:14 am on September 9, 2024: contributor

    ⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
16. 
    maflcko commented at 4:12 pm on September 12, 2024: member

    @S3RK Are you still working on this?

Contributors
S3RK DrahtBot fanquake josibake maflcko

Labels
Wallet Needs rebase