refactor, wallet: get serialized size of `CRecipient`s directly #30050

josibake · 2024-05-06T10:34:47Z

Broken out from #28201

In order to estimate fees properly, we need to know what the final serialized transaction size will be. This PR refactors CreateTransactionInternal to:

Get the serialized size directly from the CRecipient: this sets us up in a future PR to calculate the serialized size of silent payment CTxDestinations (see 797e21c)
Use the new GetSerializeSizeForRecipient to move the serialize size calculation to before coin selection and the output creation to after coin selection: this also sets us up for silent payments sending in a future PR in that silent payments outputs cannot be created until after the inputs to the transaction have been selected

Aside from the silent payments use case, I think this structure logically makes more sense. As a reminder, move-only commits are best reviewed with something like git diff -w --color-moved=dimmed-zebra

DrahtBot · 2024-05-06T10:34:50Z

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, rkrux, achow101
Concept ACK	furszy

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:

#30093 (optimization: reserve memory allocation for transaction inputs/outputs by paplorinc)
#29523 (Wallet: Add max_tx_weight to transaction funding options (take 2) by ismaelsadeeq)

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.

l0rinc · 2024-05-12T12:48:05Z

src/wallet/spend.cpp

@@ -1141,6 +1129,16 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
           result.GetWaste(),
           result.GetSelectedValue());

+    // vouts to the payees
+    for (const auto& recipient : vecSend)


would it make sense to preallocate the vouts before pushing?

txNew.vout.reserve(txNew.vout.size() + vecSend.size());

Trying to keep this a move-only refactor, so would prefer to leave it for now. I end up touching this code again in #28201; will make a note of this for that PR.

Split it out to a new PR so that we don't forget it: https://github.com/bitcoin/bitcoin/pull/30093/files

Rebased #30093 after this merge - it became trivial after your changes

l0rinc · 2024-05-12T12:49:29Z

src/wallet/spend.cpp

@@ -994,6 +994,8 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(

    // Set the long term feerate estimate to the wallet's consolidate feerate
    coin_selection_params.m_long_term_feerate = wallet.m_consolidate_feerate;
+    // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 witness overhead (dummy, flag, stack size)
+    coin_selection_params.tx_noinputs_size = 10 + GetSizeOfCompactSize(vecSend.size()); // bytes for output count


nit: found it a bit confusing that we have two different style comments for the same line

Would prefer to not change for now to keep this a move-only commit.

S3RK · 2024-06-10T07:51:38Z

src/wallet/spend.cpp

+    {
+        CTxOut txout(recipient.nAmount, GetScriptForDestination(recipient.dest));
+
+        if (IsDust(txout, wallet.chain().relayDustFee())) {


nit: it makes sense to keep this check before coin selection to return early and avoid wasting time

Yeah, I had considered this but it would require refactoring IsDust in a non-trivial way, since IsDust is used outside of the wallet and as such doesn't really know what a CRecipient is.

It's definitely possible, since all you really need to check for dust is the serialized size of the output and the value, but doing the refactor here blows up the scope of this PR quite a bit. I think it would be better to handle this in a separate PR, i.e. refactor IsDust so it can be used for both CRecipients and CTxOuts. Happy to open a follow-up PR, or at the very least leave a TODO comment here.

Actually, I just realized I was over complicating this: you don't need to refactor IsDust, you just need to wrap it with an IsDust function that turns a CRecipient into a CTxOut, same as GetSerializeSizeForRecipient. Will update.

Updated, thanks @S3RK !

josibake · 2024-06-10T09:35:34Z

Updated b44f002 -> f004b16 (serialize-size-for-crecipient-00 -> serialize-size-for-crecipient-01, compare)

Added method for checking if CRecipient is dust directly
This allows for checking the outputs are dust before running coin selection (h/t @S3RK )

S3RK · 2024-06-12T07:24:52Z

src/wallet/wallet.cpp

@@ -4559,4 +4559,18 @@ std::optional<CKey> CWallet::GetKey(const CKeyID& keyid) const
    }
    return std::nullopt;
 }
+
+size_t GetSerializeSizeForRecipient(const CRecipient& recipient)


nit: I think the visitor pattern in GetSerializeSizeForRecipient and IsDust in this case makes the code less clear, but I could be missing something

p.s. I don't see how the callable will be applied to more than one destination, so in my mind it's just easier to inline the callable and drop the std::visit

Regarding the visitor pattern, CTxDestination is a std::variant and we need the visitor to know to use WitnessV1Taproot when calculating the serialized size for both GetSerializeSizeForRecipient and IsDust when the CTxDestination is a V0SilentPayments destination. See 797e21c.

However, you're correct this isn't really clear in this PR, so I'll update the commit message to explain this.

Regarding making the functions callable, I also wasn't sure about this. I noticed a few places where IsDust is used in the RPC calls, which is why I decided to make it callable here, but we can always remove them from the header if we end up only using it in CreateTransactionInternal.

I must also be missing something here because you should be able to write this as:

size_t GetSerializeSizeForRecipient(const CRecipient& recipient) { return ::GetSerializeSize(CTxOut(recipient.nAmount, GetScriptForDestination(recipient.dest))); }

And the follow-up PR should also compile this code:

size_t GetSerializeSizeForRecipient(const CRecipient& recipient) { // A Silent Payements address is instructions on how to create a WitnessV1Taproot output // Luckily, we know exactly how big a single WitnessV1Taproot output is, so return the serialization for that // For everything else, convert it to a CTxOut and get the serialized size if (std::get_if<V0SilentPaymentDestination>(&recipient.dest) != nullptr) { return ::GetSerializeSize(CTxOut(recipient.nAmount, GetScriptForDestination(WitnessV1Taproot()))); } else { return ::GetSerializeSize(CTxOut(recipient.nAmount, GetScriptForDestination(recipient.dest))); } }

Still, I'm also not sure about this decoupling commit.

@S3RK , @furszy you're right: get_if works here, as well. For some reason I was thinking std::get_if doesn't check if the type exists in the std::variant at compile time, hence using a visitor was better. But std::get_if does check that the type is included in the variant; its just that the visitor can actually do compile time checks to ensure all the cases are covered. But in the silent payments example, we don't need exhaustive coverage so the visitor is likely overkill. I've updated the functions to remove the visitor.

I also moved these into spend.cpp for now. After looking around a bit, I don't think I will need them anywhere else and if they are, I can move them to a spend_util.h/cpp file, per @furszy 's suggestion.

S3RK · 2024-06-12T07:27:01Z

Code Review ACK f004b16

The PR doesn't modify the existing behavior, but prepares the code for Silent Payments (see #28201)
The code pushed down in the function doesn't affect the logic, because txnew.vout is not used between the old and the new location.

josibake · 2024-06-12T10:04:36Z

Rebased to fix merge conflict and update commit message to better explain the motivation for using the visitor patter, per @S3RK 's feedback

furszy · 2024-06-12T13:58:28Z

src/wallet/wallet.h

+size_t GetSerializeSizeForRecipient(const CRecipient& recipient);
+bool IsDust(const CRecipient& recipient, const CFeeRate& dustRelayFee);
+


Structural topic (not really for this PR):

These two functions, and also CRecipient seem to fit better on a new spend_util.h/cpp file rather than here.

Also TransactionChangeType seem to fit better inside spend.h/cpp rather than here.

IIRC, I tried to move CRecipient out of wallet.h awhile back and it ended up being more complicated than I expected? But in general I agree.

furszy · 2024-06-12T14:13:05Z

src/wallet/spend.cpp

@@ -9,7 +9,6 @@
 #include <consensus/validation.h>
 #include <interfaces/chain.h>
 #include <numeric>
-#include <policy/policy.h>


This include should still be needed. The CreateTransactionInternal still calls GetDustThreshold.

furszy · 2024-06-12T14:34:32Z

src/wallet/wallet.cpp

@@ -4559,4 +4559,18 @@ std::optional<CKey> CWallet::GetKey(const CKeyID& keyid) const
    }
    return std::nullopt;
 }
+
+size_t GetSerializeSizeForRecipient(const CRecipient& recipient)


I must also be missing something here because you should be able to write this as:

size_t GetSerializeSizeForRecipient(const CRecipient& recipient) { return ::GetSerializeSize(CTxOut(recipient.nAmount, GetScriptForDestination(recipient.dest))); }

And the follow-up PR should also compile this code:

size_t GetSerializeSizeForRecipient(const CRecipient& recipient) { // A Silent Payements address is instructions on how to create a WitnessV1Taproot output // Luckily, we know exactly how big a single WitnessV1Taproot output is, so return the serialization for that // For everything else, convert it to a CTxOut and get the serialized size if (std::get_if<V0SilentPaymentDestination>(&recipient.dest) != nullptr) { return ::GetSerializeSize(CTxOut(recipient.nAmount, GetScriptForDestination(WitnessV1Taproot()))); } else { return ::GetSerializeSize(CTxOut(recipient.nAmount, GetScriptForDestination(recipient.dest))); } }

Still, I'm also not sure about this decoupling commit.

Now that a CRecipient holds a CTxDestination, we can get the serialized size and determine if the output is dust using the CRecipient directly. This does not change any current behavior, but provides a nice generalization that can be used to apply special logic to a CTxDestination serialization and dust calculations in the future. Specifically, in a later PR when support for `V0SilentPayment` destinations is added, we need to use `WitnessV1Taproot` as the scriptPubKey for serialized size calcuations whenever the `CRecipient` destination is a `V0SilentPayment` destination.

Move the output serialization size and dust calculation into the loop where the outputs are iterated over to calculate the total sum. Move the code for adding the the txoutputs to the transaction to after coin selection. While this code structure generally follows a more logical flow, the primary motivation for moving the code for adding outputs to the transaction sets us up nicely for silent payments (in a future PR): we need to know the input set before generating the final output scriptPubKeys.

josibake · 2024-06-17T18:30:55Z

Updated to remove the visitor per @S3RK and @furszy 's feedback

S3RK · 2024-06-19T06:51:39Z

reACK a9c7300

Looks even simpler now, thanks for incorporating feedback.

rkrux

tACK a9c7300

make, test/functional are successful.

From what I understand about Silent Payments, the calculation of the recipient address is dependent on the input hash for which it makes sense to push down the txoutputs addition after coin selection.

Left a suggestion regarding commits but not a blocker.

rkrux · 2024-06-25T18:44:21Z

src/wallet/spend.cpp

-        CTxOut txout(recipient.nAmount, GetScriptForDestination(recipient.dest));
-
-        // Include the fee cost for outputs.
-        coin_selection_params.tx_noinputs_size += GetSerializeSizeForRecipient(recipient);
-
-        if (IsDust(recipient, wallet.chain().relayDustFee())) {
-            return util::Error{_("Transaction amount too small")};
-        }
-        txNew.vout.push_back(txout);


Although being a bit pedantic, but as this commit is supposed to be move-only, the removal of CTxOut txout... in line 1107 and the replacement of push_back with emplace_back in line 1115 could have been part of the previous commit to avoid any confusion for future readers.

Will leave for now so as to not require reACKs for a house keeping change, but will reorder if I end up needing retouch. Thanks!

furszy

ACK functionality wise. Not blocking if others are happy with the current code.

Have to admit that I'm not fan of the multiple CTxOut creations and GetScriptForDestination calls when we could cache the CTxOut early in the process, leaving the silent payment destinations empty, just so they can be modified later on. The first commit GetSerializeSizeForRecipient and IsDust decouplings seem more like an unneeded indirection rather than something worth doing to me atm. But as said above, not blocking if others are happy with it.

achow101 · 2024-06-27T17:46:17Z

ACK a9c7300

Have to admit that I'm not fan of the multiple CTxOut creations and GetScriptForDestination calls when we could cache the CTxOut early in the process, leaving the silent payment destinations empty, just so they can be modified later on

I don't like that since it overloads the meaning of an empty scriptPubKey. This would also not work as well for future silent payments versions since information about the version.

josibake mentioned this pull request May 6, 2024

Silent Payments: sending #28201

Draft

This was referenced May 6, 2024

consensus: Store transaction nVersion as uint32_t #29325

Merged

wallet: Track no-longer-spendable TXOs separately #27865

Open

wallet: Keep track of the wallet's own transaction outputs in memory #27286

Merged

josibake mentioned this pull request May 8, 2024

BIP352 tracking issue #28536

Open

15 tasks

l0rinc reviewed May 12, 2024

View reviewed changes

This was referenced May 23, 2024

optimization: reserve memory allocation for transaction inputs/outputs #30093

Merged

wallet: add coin selection parameter add_excess_to_recipient_position for changeless txs with excess that would be added to fees #30080

Closed

DrahtBot mentioned this pull request Jun 4, 2024

wallet: Ensure best block matches wallet scan state #30221

Merged

S3RK reviewed Jun 10, 2024

View reviewed changes

josibake force-pushed the serialize-size-for-crecipient branch from b44f002 to f004b16 Compare June 10, 2024 09:24

S3RK reviewed Jun 12, 2024

View reviewed changes

DrahtBot added the Needs rebase label Jun 12, 2024

josibake force-pushed the serialize-size-for-crecipient branch from f004b16 to 4e1ccb3 Compare June 12, 2024 10:02

DrahtBot removed the Needs rebase label Jun 12, 2024

furszy reviewed Jun 12, 2024

View reviewed changes

josibake force-pushed the serialize-size-for-crecipient branch from 4e1ccb3 to ad55985 Compare June 17, 2024 18:23

josibake added 2 commits June 17, 2024 20:25

josibake force-pushed the serialize-size-for-crecipient branch from ad55985 to a9c7300 Compare June 17, 2024 18:26

DrahtBot mentioned this pull request Jun 22, 2024

Wallet: Add max_tx_weight to transaction funding options (take 2) #29523

Merged

rkrux approved these changes Jun 25, 2024

View reviewed changes

furszy reviewed Jun 26, 2024

View reviewed changes

DrahtBot requested a review from furszy June 27, 2024 17:46

achow101 merged commit f0745d0 into bitcoin:master Jun 27, 2024

bitcoin locked and limited conversation to collaborators Aug 6, 2025

		size_t GetSerializeSizeForRecipient(const CRecipient& recipient);
		bool IsDust(const CRecipient& recipient, const CFeeRate& dustRelayFee);

refactor, wallet: get serialized size of CRecipients directly #30050

refactor, wallet: get serialized size of CRecipients directly #30050

Uh oh!

Conversation

josibake commented May 6, 2024

Uh oh!

DrahtBot commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc May 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josibake Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josibake commented Jun 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy Jun 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

S3RK commented Jun 12, 2024

Uh oh!

josibake commented Jun 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy Jun 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josibake commented Jun 17, 2024

Uh oh!

S3RK commented Jun 19, 2024

Uh oh!

rkrux left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkrux Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refactor, wallet: get serialized size of `CRecipient`s directly #30050

refactor, wallet: get serialized size of `CRecipient`s directly #30050

DrahtBot commented May 6, 2024 •

edited

Loading

l0rinc May 12, 2024 •

edited

Loading

josibake Jun 10, 2024 •

edited

Loading

furszy Jun 12, 2024 •

edited

Loading

furszy Jun 12, 2024 •

edited

Loading

rkrux left a comment •

edited

Loading

rkrux Jun 25, 2024 •

edited

Loading