Skip to content

wallet: clarify FundTransaction signature #29564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

wallet: clarify FundTransaction signature #29564

wants to merge 1 commit into from

Conversation

S3RK
Copy link
Contributor

@S3RK S3RK commented Mar 5, 2024 •
edited
Loading

Follow up for #28560 (comment)

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 5, 2024 •
edited
Loading

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 DrahtBot added the Wallet label Mar 5, 2024
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 5, 2024

🚧 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 fanquake requested a review from josibake March 5, 2024 09:37
@fanquake
Copy link
Member

fanquake commented Mar 5, 2024 

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.

@DrahtBot DrahtBot mentioned this pull request Mar 5, 2024
Closed
josibake
josibake reviewed Mar 5, 2024
View reviewed changes
Copy link
Member

@josibake josibake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This was referenced Mar 5, 2024
Merged
Closed
@S3RK
wallet: clarify FundTransaction signature
e85054e 
Pass vector<CTxIn> intstead of CMutableTransaction through
parameters. Pass locktime and nVersion as part of CoinControl object.
@S3RK S3RK force-pushed the fundtx_refactor branch from 98616ff to e85054e Compare March 6, 2024 07:53
@DrahtBot DrahtBot removed the CI failed label Mar 6, 2024
@josibake
Copy link
Member

josibake commented Mar 6, 2024

@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 DrahtBot mentioned this pull request Mar 31, 2024
Closed
@DrahtBot DrahtBot mentioned this pull request Apr 24, 2024
Merged
@DrahtBot
Copy link
Contributor

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 9, 2024

⌛ 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
Copy link
Member

maflcko commented Sep 12, 2024

@S3RK Are you still working on this?

@DrahtBot
Copy link
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 maflcko closed this Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@josibake josibake josibake left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Needs rebase Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@S3RK @DrahtBot @fanquake @josibake @maflcko