Skip to content

Conversation

darosior
Copy link
Member

@darosior darosior commented Nov 24, 2022

The wallet currently estimates the size of a signed input by doing a dry run of the signing logic. This is unnecessary since all outputs we can sign for can be represented by a descriptor, and we can derive the size of a satisfaction ("signature") directly from the descriptor itself.
In addition, the current approach does not generalize well: dry runs of the signing logic are only possible for the most basic scripts. See for instance the discussion in #24149 around that.

This introduces a method to get the maximum size of a satisfaction from a descriptor, and makes the wallet use that instead of the dry-run.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 24, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK sipa, 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:

  • #28345 (Bugfix: Package relay / bytespersigop checks by luke-jr)
  • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
  • #27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
  • #27255 (MiniTapscript: port Miniscript to Tapscript by darosior)
  • #26728 (wallet: Have the wallet store the key for automatically generated descriptors by achow101)
  • #25907 (wallet: rpc to add automatically generated descriptors by achow101)
  • #25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)
  • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
  • #22341 (rpc: add getxpub by Sjors)

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.

@darosior
Copy link
Member Author

Pushed a commit to accurately compute the serialized size of the witness stack size, cleanup the commits a bit, and add the newly introduced methods in the descriptors to the descriptor_parse fuzz target. This is now ready for review.

@darosior darosior changed the title Wallet: estimate the size of a signed inputs using descriptors Wallet: estimate the size of signed inputs using descriptors Nov 25, 2022
Copy link
Member

@achow101 achow101 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

@darosior darosior force-pushed the input_size_with_descriptors branch from 052edf8 to 594c33e Compare February 5, 2023 17:42
@darosior
Copy link
Member Author

darosior commented Feb 5, 2023

Thanks for the review. Addressed your comments and rebased on master.

@darosior
Copy link
Member Author

Rebased after #28244.

@darosior
Copy link
Member Author

Hey i keep rebasing this, i felt it was quite close during the last reviews from @achow101. Would be nice to have others look at this though.

@Sjors and @furszy would you mind having a look here? I think you'd be the best candidates for the job.

Copy link
Member

@achow101 achow101 left a comment

Choose a reason for hiding this comment

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

ACK 932556c

Copy link
Member

@furszy furszy 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

Added to my queue. Will start reviewing next week.

It was taking into account the P2WSH script push in the number of stack
elements.
Similarly to how we compute the maximum stack size.

Also note how it would be quite expensive to recompute it recursively
by accounting for different ECDSA signature sizes. So we just assume
high-R everywhere. It's only a trivial difference anyways.
@darosior darosior force-pushed the input_size_with_descriptors branch from 932556c to 617b579 Compare August 25, 2023 10:19
In the wallet code, we are currently estimating the size of a signed
input by doing a dry run of the signing logic. This is unnecessary as
all outputs we are able to sign for can be represented by a descriptor,
and we can derive the size of a satisfaction ("signature") from the
descriptor itself directly.
In addition, this approach does not scale: getting the size of a
satisfaction through a dry run of the signing logic is only possible for
the most basic scripts.

This commit introduces the computation of the size of satisfaction per
descriptor. It's a bit intricate for 2 main reasons:
- We want to conserve the behaviour of the current dry-run logic used by
  the wallet that sometimes assumes ECDSA signatures will be low-r,
  sometimes not (when we don't create them).
- We need to account for the witness discount. A single descriptor may
  sometimes benefit of it, sometimes not (for instance `pk()` if used as
  top-level versus if used inside `wsh()`).
It is sometimes useful to interface with multiple signing providers at
once. For instance when inferring a descriptor with solving information
being provided from multiple sources (see next commit).

Instead of inneficiently copying the information from one provider into
the other, introduce a new signing provider that takes a list of
pointers to existing providers.
Instead of using the dummysigner to compute a placeholder satisfaction,
infer a descriptor on the scriptPubKey of the coin being spent and use
the estimation of the satisfaction size given by the descriptor
directly.

Note this (almost, see next paragraph) exactly conserves the previous
behaviour. For instance CalculateMaximumSignedInputSize was previously
assuming the input to be spent in a transaction that spends at least one
Segwit coin, since it was always accounting for the serialization of the
number of witness elements.

In this commit we use a placeholder for the size of the serialization of
the witness stack size (1 byte). Since the logic in this commit is
already tricky enough to review, and that it is only a very tiny
approximation not observable through the existing tests, it is addressed
in the next commit.
When estimating the maximum size of an input, we were assuming the
number of elements on the witness stack could be encode in a single
byte. This is a valid approximation for all the descriptors we support
(including P2WSH Miniscript ones), but may not hold anymore once we
support Miniscript within Taproot descriptors (since the max standard
witness stack size of 100 gets lifted).

It's a low-hanging fruit to account for it correctly, so just do it now.
@sipa
Copy link
Member

sipa commented Aug 28, 2023

utACK 10546a5

@DrahtBot DrahtBot requested a review from achow101 August 28, 2023 14:26
@fanquake fanquake added this to the 26.0 milestone Aug 28, 2023
@achow101
Copy link
Member

achow101 commented Sep 6, 2023

re-ACK 10546a5

@DrahtBot DrahtBot removed the request for review from achow101 September 6, 2023 17:22
@achow101 achow101 merged commit d2ccca2 into bitcoin:master Sep 6, 2023
@darosior darosior deleted the input_size_with_descriptors branch September 7, 2023 08:03
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023
fanquake added a commit that referenced this pull request Sep 9, 2023
8d6228f consensus/validation.h: remove needless GetTransactionOutputWeight helper (Antoine Poinsot)

Pull request description:

  Introduced in #26567. My bad. Thanks AJ for noticing.

ACKs for top commit:
  ajtowns:
    utACK 8d6228f

Tree-SHA512: cf13647b4aac82fb6a54ae0338e3928e9bdf226ed4f5e91d529996328471744132db2bee9676e0b3f40a8bbe0e0ca51a9e5f91560a84e0f33597290551a1ee18
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2023
8d6228f consensus/validation.h: remove needless GetTransactionOutputWeight helper (Antoine Poinsot)

Pull request description:

  Introduced in bitcoin#26567. My bad. Thanks AJ for noticing.

ACKs for top commit:
  ajtowns:
    utACK 8d6228f

Tree-SHA512: cf13647b4aac82fb6a54ae0338e3928e9bdf226ed4f5e91d529996328471744132db2bee9676e0b3f40a8bbe0e0ca51a9e5f91560a84e0f33597290551a1ee18
@bitcoin bitcoin locked and limited conversation to collaborators Nov 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants