wallet: clarify FundTransaction signature

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

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

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.

DrahtBot added the label Wallet on Mar 5, 2024

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

DrahtBot added the label CI failed on Mar 5, 2024

fanquake requested review from josibake on Mar 5, 2024

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.

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.

wallet: clarify FundTransaction signature

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

e85054efd1

S3RK force-pushed on Mar 6, 2024

DrahtBot removed the label CI failed on Mar 6, 2024

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.

DrahtBot commented at 9:41 am on June 12, 2024: contributor

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

DrahtBot added the label Needs rebase on Jun 12, 2024

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.

maflcko commented at 4:12 pm on September 12, 2024: member

@S3RK Are you still working on this?

DrahtBot commented at 1:04 am on December 10, 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.

maflcko closed this on Dec 10, 2024

wallet: clarify FundTransaction signature #29564

Code Coverage

Reviews

Conflicts