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

josibake commented at 10:34 am on May 6, 2024: member

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 https://github.com/bitcoin/bitcoin/pull/28201/commits/797e21c8c1bf393e668eeef8bb7ee9cae1e5e41e)
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 commented at 10:34 am on May 6, 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
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.

in src/wallet/spend.cpp:1150 in b44f00251a outdated

1128@@ -1141,6 +1129,16 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1129            result.GetWaste(),
1130            result.GetSelectedValue());
1131 
1132+    // vouts to the payees
1133+    for (const auto& recipient : vecSend)

paplorinc commented at 12:48 pm on May 12, 2024:

would it make sense to preallocate the vouts before pushing?

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

josibake commented at 8:45 am on May 13, 2024:

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.

paplorinc commented at 9:54 am on May 13, 2024:

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

paplorinc commented at 6:57 pm on June 27, 2024:

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

in src/wallet/spend.cpp:1012 in b44f00251a outdated

993@@ -994,6 +994,8 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
994 
995     // Set the long term feerate estimate to the wallet's consolidate feerate
996     coin_selection_params.m_long_term_feerate = wallet.m_consolidate_feerate;
997+    // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 witness overhead (dummy, flag, stack size)
998+    coin_selection_params.tx_noinputs_size = 10 + GetSizeOfCompactSize(vecSend.size()); // bytes for output count

paplorinc commented at 12:49 pm on May 12, 2024:

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

josibake commented at 8:49 am on May 13, 2024:

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

in src/wallet/spend.cpp:1137 in b44f00251a outdated

1128@@ -1141,6 +1129,16 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1129            result.GetWaste(),
1130            result.GetSelectedValue());
1131 
1132+    // vouts to the payees
1133+    for (const auto& recipient : vecSend)
1134+    {
1135+        CTxOut txout(recipient.nAmount, GetScriptForDestination(recipient.dest));
1136+
1137+        if (IsDust(txout, wallet.chain().relayDustFee())) {

S3RK commented at 7:51 am on June 10, 2024:

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

josibake commented at 8:10 am on June 10, 2024:

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.

josibake commented at 8:28 am on June 10, 2024:

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.

josibake commented at 9:35 am on June 10, 2024:

Updated, thanks @S3RK !

josibake force-pushed on Jun 10, 2024

josibake commented at 9:35 am on June 10, 2024: member

Updated https://github.com/bitcoin/bitcoin/commit/b44f00251a77a3360888eeb10ec903375eb1e0f8 -> https://github.com/bitcoin/bitcoin/commit/f004b1625bcb9b833f36af47b53675e3dd77135b (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 )

in src/wallet/wallet.cpp:4563 in 0aca3dc624 outdated

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

S3RK commented at 7:24 am on June 12, 2024:

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

S3RK commented at 7:28 am on June 12, 2024:

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

josibake commented at 9:40 am on June 12, 2024:

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 https://github.com/bitcoin/bitcoin/pull/28201/commits/797e21c8c1bf393e668eeef8bb7ee9cae1e5e41e.

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.

furszy commented at 2:34 pm on June 12, 2024:

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

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

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

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

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

josibake commented at 6:12 pm on June 17, 2024:

@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 commented at 7:27 am on June 12, 2024: contributor

Code Review ACK f004b1625bcb9b833f36af47b53675e3dd77135b

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.

DrahtBot added the label Needs rebase on Jun 12, 2024

josibake force-pushed on Jun 12, 2024

josibake commented at 10:04 am on June 12, 2024: member

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

DrahtBot removed the label Needs rebase on Jun 12, 2024

in src/wallet/wallet.h:299 in d7608456fd outdated

293@@ -293,6 +294,9 @@ struct CRecipient
294     bool fSubtractFeeFromAmount;
295 };
296 
297+size_t GetSerializeSizeForRecipient(const CRecipient& recipient);
298+bool IsDust(const CRecipient& recipient, const CFeeRate& dustRelayFee);
299+

furszy commented at 1:58 pm on June 12, 2024:

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.

josibake commented at 6:13 pm on June 17, 2024:

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.

in src/wallet/spend.cpp:14 in d7608456fd outdated

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

furszy commented at 2:13 pm on June 12, 2024:

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

josibake commented at 6:14 pm on June 17, 2024:

Fixed.

josibake force-pushed on Jun 17, 2024

wallet: use CRecipient instead of CTxOut

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.

adc6ab25bb

move-only: refactor CreateTransactionInternal

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.

a9c7300135

josibake force-pushed on Jun 17, 2024

josibake commented at 6:30 pm on June 17, 2024: member

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

S3RK commented at 6:51 am on June 19, 2024: contributor

reACK a9c7300135f0188daa5bce5491e2daf2dd8da8ae

Looks even simpler now, thanks for incorporating feedback.

in src/wallet/spend.cpp:1115 in a9c7300135

1118-        coin_selection_params.tx_noinputs_size += GetSerializeSizeForRecipient(recipient);
1119-
1120-        if (IsDust(recipient, wallet.chain().relayDustFee())) {
1121-            return util::Error{_("Transaction amount too small")};
1122-        }
1123-        txNew.vout.push_back(txout);

rkrux commented at 6:44 pm on June 25, 2024:

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.

josibake commented at 11:47 am on June 26, 2024:

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!

rkrux approved

rkrux commented at 6:48 pm on June 25, 2024: none

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.

furszy commented at 8:18 pm on June 26, 2024: member

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 commented at 5:46 pm on June 27, 2024: member

ACK a9c7300135f0188daa5bce5491e2daf2dd8da8ae

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.

DrahtBot requested review from furszy on Jun 27, 2024

achow101 merged this on Jun 27, 2024

achow101 closed this on Jun 27, 2024

Shuhaib07 commented at 10:21 pm on August 6, 2024: none

Approve

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

Code Coverage

Reviews

Conflicts

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