wallet, rpc: `FundTransaction` refactor #28560

josibake · 2023-10-02T16:50:20Z

Motivation

The primary motivation for this PR is to enable FundTransaction to take a vector of CRecipient objects to allow passing BIP352 silent payment addresses to RPCs that use FundTransaction (e.g. send, walletcreatefundedpsbt). To do that, SFFO logic needs to be moved out of FundTransaction so the CRecipient objects with the correct SFFO information can be created and then passed to FundTransaction.

As a secondary motivation, this PR moves the SFFO stuff closer to the caller, making the code cleaner and easier to understand. This is done by having a single function which parses RPC inputs for SFFO and consistently using the set<int> method for communicating SFFO.

I'm also not convinced we need to pass a full CMutableTx object to FundTransaction, but I'm leaving that for a follow-up PR/discussion, as its not a blocker for silent payments.

DrahtBot · 2023-10-02T16:50:22Z

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
ACK	S3RK, achow101
Concept ACK	theStack

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)
#29264 (Add max_tx_weight to transaction funding options by instagibbs)
#28944 (wallet, rpc: add anti-fee-sniping to send and sendall by ishaanam)
#27792 (wallet: Deniability API (Unilateral Transaction Meta-Privacy) by denavila)

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.

S3RK

I'm still trying to fully grasp the existing code (which is a big mess), so bear with me if I don't make sense. Left some comment/questions based on my current understanding.

src/wallet/rpc/spend.cpp

src/wallet/spend.h

src/wallet/rpc/spend.cpp

josibake · 2023-12-20T16:06:58Z

Fixed the has_data logic and replaced all references to tx.vout with recipients in FundTransaction, per feedback from @S3RK

test/functional/wallet_fundrawtransaction.py

test/functional/wallet_sendmany.py

aureleoules

A few nits

src/wallet/rpc/spend.cpp

josibake · 2024-01-06T18:45:06Z

git range-diff 5892014...909c6bb

Addresses review comments from @aureleoules and @achow101 , most notable being now clearing tx.vout and asserting that it is empty inside both FundTransaction functions. This ensures this is no longer used to pass outputs to FundTransaction, now and in future code.

src/rpc/rawtransaction_util.cpp

src/wallet/rpc/spend.cpp

josibake · 2024-01-09T19:57:14Z

Updated 909c6bb -> c856a10 (compare)

Moved CTxDestination, Amount into loop
Simplified the loop inside CreateRecipients
Refactored InterpretSubtractFeeFromAmountInstructions to infer parsing logic based on the instruction type (int, bool, str)

Thanks for the suggestion @achow101 , it looks a lot cleaner now.

S3RK · 2024-01-10T08:26:09Z

src/wallet/rpc/spend.cpp

@@ -78,6 +57,27 @@ static void InterpretFeeEstimationInstructions(const UniValue& conf_target, cons
    }
 }

+std::set<int> InterpretSubtractFeeFromOutputInstructions(const UniValue& sffo_instructions, const std::vector<std::string>& destinations)


in 0b0276b

I don't see a reason to extract this function at this point (could be done later in the PR).
There are two disparate cases covered (bool and vector<string>). And each case is only used in one and only one place, further commits also don't introduce further usage of this code path as they pass vector<int>.

Wouldn't it be better if we keep this logic at the call site?
Benefits of my proposal:

we don't need second parameter for this function

we don't need to pass dummy value in fundrawtransaction RPC code

we can combine NormalizeOutputs and ParseOutputs. they are used always together and the only reason for NormalizeOutputs to exist now is too pass second param to InterpretSubtractFeeFromOutputInstructions in send and walletfundpsbt RPC. But there second parameter is useless as subtract_fee_from_outputs option of those RPCs can't be of the vector<string> type

I prefer having a single function (as opposed to keeping the logic at the call site) for the following reasons:

We avoid duplicating code at the RPC call sites:

The logic for validating SFFO inputs would need to be duplicated at send, fundrawtx and walletcreatefundedpsbt. This is the code that checks that the ints passed are not duplicated and in bounds and previously used to be inside rpc/FundTransaction

We have one place to validate all SFFO inputs:

Currently, sendmany SFFO inputs are not validated (documented in the added functional tests)

Having one function for parsing and validating makes it cleaner to add validation for all inputs in a follow-up PR. The only reason I'm not doing it in this PR is that its a behavior change (it could make calls to sendmany invalid that were previously accepted) and I'd like to keep this strictly a refactor

we don't need second parameter for this function

In the case of sendmany, we do need the array of destinations to lookup their position. In the case of send, walletcreatefundedpsbt, and fundrawtransaction, we need the array of destinations to make sure our ints are in bounds. There are definitely other ways to do it, but it seems fine to use the same vector for lookups and size. More just making the point that the argument is always used.

we can combine NormalizeOutputs and ParseOutputs. they are used always together

Actually, the main goal here was to separate the logic in AddOutputs to get rid of duplicate validation logic in AddOutputs and also the original ParseRecipients function. sendtoaddress and sendtoaddress need the validation logic, but don't need NormalizeOutputs since those RPCs already only allow the user to pass a key-value pair for address and amounts.

The other RPCs (send, fundrawtransaction, walletcreatefundedpsbt) need the logic for parsing how the univalue is passed (NormalizeOutputs) and validating the addresses (ParseOutputs).

S3RK · 2024-01-15T09:13:04Z

Code review ACK c856a10

Nice improvements removing duplication between CreateRecipient and AddOutputs, also embedding SFFO values in CRecipient instead of passing as a separate parameter to FundTransaction.

I'd still prefer InterpretSubtractFeeFromOutputInstructions to work with one type of SFFO option - vector<int> and process others at the call site. SFFO option can be in three different forms and vector<int> is the only form that used multiple times, the other forms are only used once. But that's not blocking as InterpretSubtractFeeFromOutputInstructions and the RPCs are in the same file.

If the serialized transaction passed to `fundrawtransaction` contains duplicates, they will be deserialized and added to the transaction. Add a test to ensure this behavior is not changed during the refactor. A user can pass any number of duplicated and unrelated addresses as an SFFO argument to `sendmany` and the RPC will not throw an error (note, all the rest of the RPCs which take SFFO as an argument will error if the user passes duplicates or specifies outputs not present in the transaction). Add a test to ensure this behavior is not changed during the refactor.

Move the univalue formatting logic out of AddOutputs and into its own function, `NormalizeOutputs`. This allows us to re-use this logic in later commits.

Move the parsing and validation out of `AddOutputs` into its own function, `ParseOutputs`. This allows us to re-use this logic in `ParseRecipients` in a later commit, where the code is currently duplicated. The new `ParseOutputs` function returns a CTxDestination,CAmount tuples. This allows the caller to then translate the validated outputs into either CRecipients or CTxOuts.

Have `ParseRecipients` return a vector of `CRecipients` and rename to `CreateRecipients`.

Move validation logic out of `CreateRecipients` and instead take the already validated outputs from `ParseOutputs` as an input. Move SFFO parsing out of `CreateRecipients` into a new function, `InterpretSubtractFeeFromOutputsInstructions`. This takes the SFFO instructions from `sendmany` and `sendtoaddress` and turns them into a set of integers. In a later commit, we will also move the SFFO parsing logic from `FundTransaction` into this function. Worth noting: a user can pass duplicate addresses and addresses that dont exist in the transaction outputs as SFFO args to `sendmany` and `sendtoaddress` without triggering a warning. This behavior is preserved in to keep this commit strictly a refactor.

Instead turning tx.vout into a vector of `CRecipient`, make `FundTransaction` take a `CRecipient` vector directly. This allows us to remove SFFO logic from the wrapper RPC `FundTransaction` since the `CRecipient` objects have already been created with the correct SFFO values. This also allows us to remove SFFO from both `FundTransaction` function signatures. This sets us up in a future PR to be able to use these RPCs with BIP352 static payment codes.

S3RK · 2024-01-22T08:45:49Z

According to my range-diff nothing changed.
reACK 18ad1b9

theStack · 2024-01-22T18:52:06Z

Concept ACK

josibake · 2024-01-23T15:36:37Z

According to my range-diff nothing changed. reACK 18ad1b9

for context, this was the reason for the rebase: #29218 (comment)

achow101

ACK 18ad1b9

achow101 · 2024-01-23T21:29:53Z

src/rpc/rawtransaction_util.cpp

+            CTxDestination destination{CNoDestination{CScript() << OP_RETURN << data}};
+            CAmount amount{0};
+            parsed_outputs.emplace_back(destination, amount);


In f7384b9 "refactor: move parsing to new function"

nit: Making temp variables is not necessary.

josibake mentioned this pull request Oct 2, 2023

wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction #25273

Merged

josibake marked this pull request as draft October 2, 2023 16:56

josibake mentioned this pull request Oct 2, 2023

BIP352 tracking issue #28536

Open

15 tasks

DrahtBot added the Needs rebase label Oct 16, 2023

josibake force-pushed the fundtransaction-sffo-crecipient-refactor branch 2 times, most recently from b2efb35 to ae53820 Compare December 11, 2023 17:26

josibake marked this pull request as ready for review December 11, 2023 17:29

DrahtBot removed the Needs rebase label Dec 11, 2023

josibake force-pushed the fundtransaction-sffo-crecipient-refactor branch 3 times, most recently from 84f833e to 3dfcb81 Compare December 12, 2023 16:07

S3RK reviewed Dec 20, 2023

View reviewed changes

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

src/wallet/spend.h Show resolved Hide resolved

src/wallet/rpc/spend.cpp Show resolved Hide resolved

josibake force-pushed the fundtransaction-sffo-crecipient-refactor branch from 3dfcb81 to 0ed0406 Compare December 20, 2023 16:04

josibake force-pushed the fundtransaction-sffo-crecipient-refactor branch 2 times, most recently from 2361861 to ebf118f Compare December 22, 2023 17:53

achow101 reviewed Jan 3, 2024

View reviewed changes

test/functional/wallet_fundrawtransaction.py Outdated Show resolved Hide resolved

test/functional/wallet_sendmany.py Show resolved Hide resolved

aureleoules reviewed Jan 4, 2024

View reviewed changes

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jan 5, 2024

refactor(tidy): Use C++20 contains method #29191

Closed

josibake force-pushed the fundtransaction-sffo-crecipient-refactor branch 2 times, most recently from 9ab387e to 909c6bb Compare January 6, 2024 18:38

achow101 reviewed Jan 8, 2024

View reviewed changes

src/rpc/rawtransaction_util.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

josibake force-pushed the fundtransaction-sffo-crecipient-refactor branch 2 times, most recently from 26b4336 to c856a10 Compare January 9, 2024 19:33

S3RK reviewed Jan 10, 2024

View reviewed changes

DrahtBot added the CI failed label Jan 17, 2024

DrahtBot mentioned this pull request Jan 17, 2024

Add max_tx_weight to transaction funding options #29264

Closed

josibake added 6 commits January 19, 2024 15:04

refactor: move normalization to new function

6f569ac

Move the univalue formatting logic out of AddOutputs and into its own function, `NormalizeOutputs`. This allows us to re-use this logic in later commits.

refactor: remove out param from ParseRecipients

47353a6

Have `ParseRecipients` return a vector of `CRecipients` and rename to `CreateRecipients`.

josibake force-pushed the fundtransaction-sffo-crecipient-refactor branch from c856a10 to 18ad1b9 Compare January 19, 2024 14:08

DrahtBot removed the CI failed label Jan 19, 2024

achow101 reviewed Jan 23, 2024

View reviewed changes

DrahtBot requested a review from theStack January 23, 2024 21:31

achow101 merged commit e69796c into bitcoin:master Jan 23, 2024
16 checks passed

josibake deleted the fundtransaction-sffo-crecipient-refactor branch January 26, 2024 10:51

S3RK mentioned this pull request Mar 5, 2024

wallet: clarify FundTransaction signature #29564

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet, rpc: `FundTransaction` refactor #28560

wallet, rpc: `FundTransaction` refactor #28560

josibake commented Oct 2, 2023 •

edited

DrahtBot commented Oct 2, 2023 •

edited

S3RK left a comment

josibake commented Dec 20, 2023

aureleoules left a comment

josibake commented Jan 6, 2024

josibake commented Jan 9, 2024 •

edited

S3RK Jan 10, 2024 •

edited

josibake Jan 10, 2024

S3RK commented Jan 15, 2024

S3RK commented Jan 22, 2024

theStack commented Jan 22, 2024

josibake commented Jan 23, 2024

achow101 left a comment

achow101 Jan 23, 2024

wallet, rpc: FundTransaction refactor #28560

wallet, rpc: FundTransaction refactor #28560

Conversation

josibake commented Oct 2, 2023 • edited

Motivation

DrahtBot commented Oct 2, 2023 • edited

Code Coverage

Reviews

Conflicts

S3RK left a comment

Choose a reason for hiding this comment

josibake commented Dec 20, 2023

aureleoules left a comment

Choose a reason for hiding this comment

josibake commented Jan 6, 2024

josibake commented Jan 9, 2024 • edited

S3RK Jan 10, 2024 • edited

Choose a reason for hiding this comment

josibake Jan 10, 2024

Choose a reason for hiding this comment

S3RK commented Jan 15, 2024

S3RK commented Jan 22, 2024

theStack commented Jan 22, 2024

josibake commented Jan 23, 2024

achow101 left a comment

Choose a reason for hiding this comment

achow101 Jan 23, 2024

Choose a reason for hiding this comment

wallet, rpc: `FundTransaction` refactor #28560

wallet, rpc: `FundTransaction` refactor #28560

josibake commented Oct 2, 2023 •

edited

DrahtBot commented Oct 2, 2023 •

edited

josibake commented Jan 9, 2024 •

edited

S3RK Jan 10, 2024 •

edited