refactor, wallet: get serialized size of CRecipients directly
#30050
Conversation
Now that a CRecipient holds a CTxDestination, we can get the serialized size using a visitor. This doesnt change any current behavior, but provides a nice generalization that can be used to apply special logic to a CTxDestination serialization in the future.
Move the output serialization size 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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
| @@ -1141,6 +1129,16 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( | |||
| result.GetWaste(), | |||
| result.GetSelectedValue()); | |||
|
|
|||
| // vouts to the payees | |||
| for (const auto& recipient : vecSend) | |||
There was a problem hiding this comment.
would it make sense to preallocate the vouts before pushing?
txNew.vout.reserve(txNew.vout.size() + vecSend.size());There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Split it out to a new PR so that we don't forget it: https://github.com/bitcoin/bitcoin/pull/30093/files
| @@ -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 | |||
There was a problem hiding this comment.
nit: found it a bit confusing that we have two different style comments for the same line
There was a problem hiding this comment.
Would prefer to not change for now to keep this a move-only commit.
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
CreateTransactionInternalto:CRecipient: this sets us up in a future PR to calculate the serialized size of silent paymentCTxDestinations(see 797e21c)GetSerializeSizeForRecipientto 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 selectedAside 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