Wallet: estimate the size of signed inputs using descriptors

darosior commented at 5:15 pm on November 24, 2022: member

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 commented at 5:15 pm on November 24, 2022: contributor

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.

DrahtBot added the label Wallet on Nov 24, 2022

darosior force-pushed on Nov 24, 2022

darosior force-pushed on Nov 25, 2022

darosior commented at 10:49 am on November 25, 2022: member

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 renamed this:
~~Wallet: estimate the size of a signed inputs using descriptors~~
Wallet: estimate the size of signed inputs using descriptors
on Nov 25, 2022

in src/script/descriptor.cpp:1066 in 052edf8aa4 outdated

1057@@ -931,6 +1058,20 @@ class TRDescriptor final : public DescriptorImpl
1058     }
1059     std::optional<OutputType> GetOutputType() const override { return OutputType::BECH32M; }
1060     bool IsSingleType() const final { return true; }
1061+
1062+    std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 32; }
1063+
1064+    std::optional<int64_t> MaxSatisfactionWeight(bool) const override {
1065+        // FIXME: We assume keypath spend, which can lead to very large underestimations.
1066+        return 1 + 64;

darosior commented at 4:48 pm on November 25, 2022:

Since we want the maximum size, we should probably account for the sighash byte and make it 65 instead.

darosior commented at 5:45 pm on February 5, 2023:

Done.

DrahtBot added the label Needs rebase on Dec 5, 2022

in src/script/descriptor.cpp:759 in c8b0aeb534 outdated

754@@ -740,6 +755,17 @@ class PKDescriptor final : public DescriptorImpl
755 public:
756     PKDescriptor(std::unique_ptr<PubkeyProvider> prov, bool xonly = false) : DescriptorImpl(Vector(std::move(prov)), "pk"), m_xonly(xonly) {}
757     bool IsSingleType() const final { return true; }
758+
759+    std::optional<int64_t> ScriptSize() const override { return 1 + (m_xonly ? 33 : 32) + 1; }

achow101 commented at 11:21 pm on February 3, 2023:

In c8b0aeb534edce08366176275d5577c1e309eb6f “descriptor: introduce a method to get the satisfaction size”

This is backwards, should be 32 for m_xonly.

0    std::optional<int64_t> ScriptSize() const override { return 1 + (m_xonly ? 32 : 33) + 1; }

darosior commented at 5:07 pm on February 5, 2023:

Woops. Thanks.

in src/script/descriptor.cpp:763 in c8b0aeb534 outdated

754@@ -740,6 +755,17 @@ class PKDescriptor final : public DescriptorImpl
755 public:
756     PKDescriptor(std::unique_ptr<PubkeyProvider> prov, bool xonly = false) : DescriptorImpl(Vector(std::move(prov)), "pk"), m_xonly(xonly) {}
757     bool IsSingleType() const final { return true; }
758+
759+    std::optional<int64_t> ScriptSize() const override { return 1 + (m_xonly ? 33 : 32) + 1; }
760+
761+    std::optional<int64_t> MaxSatSize(bool use_max_sig) const override {
762+        const auto ecdsa_sig_size = use_max_sig ? 72 : 71;
763+        return 1 + (m_xonly ? ecdsa_sig_size : 64);

achow101 commented at 11:23 pm on February 3, 2023:

In c8b0aeb534edce08366176275d5577c1e309eb6f “descriptor: introduce a method to get the satisfaction size”

This is backwards, should be 64 for m_xonly.

0        return 1 + (m_xonly ? 64 : ecdsa_sig_size);

in src/script/descriptor.cpp:790 in c8b0aeb534 outdated

781@@ -756,6 +782,17 @@ class PKHDescriptor final : public DescriptorImpl
782     PKHDescriptor(std::unique_ptr<PubkeyProvider> prov) : DescriptorImpl(Vector(std::move(prov)), "pkh") {}
783     std::optional<OutputType> GetOutputType() const override { return OutputType::LEGACY; }
784     bool IsSingleType() const final { return true; }
785+
786+    std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 1 + 20 + 1 + 1; }
787+
788+    std::optional<int64_t> MaxSatSize(bool use_max_sig) const override {
789+        const auto sig_size = use_max_sig ? 72 : 71;
790+        return 1 + sig_size + 1 + 33;

achow101 commented at 11:26 pm on February 3, 2023:

In c8b0aeb534edce08366176275d5577c1e309eb6f “descriptor: introduce a method to get the satisfaction size”

We could have an uncompressed pubkey, so we should get the size from the PubkeyProvider.

darosior commented at 5:14 pm on February 5, 2023:

Ah good catch! Done, thanks.

in src/script/descriptor.cpp:892 in c8b0aeb534 outdated

863@@ -816,6 +864,20 @@ class MultisigDescriptor final : public DescriptorImpl
864 public:
865     MultisigDescriptor(int threshold, std::vector<std::unique_ptr<PubkeyProvider>> providers, bool sorted = false) : DescriptorImpl(std::move(providers), sorted ? "sortedmulti" : "multi"), m_threshold(threshold), m_sorted(sorted) {}
866     bool IsSingleType() const final { return true; }
867+
868+    std::optional<int64_t> ScriptSize() const override {
869+        const auto n_keys = m_pubkey_args.size();
870+        return 1 + BuildScript(n_keys).size() + BuildScript(m_threshold).size() + 34 * n_keys;

achow101 commented at 11:28 pm on February 3, 2023:

In c8b0aeb534edce08366176275d5577c1e309eb6f “descriptor: introduce a method to get the satisfaction size”

Same uncompressed pubkey issue as above.

achow101 commented at 7:31 pm on June 23, 2023:

This wasn’t changed?

darosior commented at 2:30 pm on June 27, 2023:

Oops. Done now.

achow101 commented at 11:59 pm on February 3, 2023: member

Concept ACK

darosior force-pushed on Feb 5, 2023

darosior commented at 5:56 pm on February 5, 2023: member

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

darosior force-pushed on Feb 5, 2023

DrahtBot removed the label Needs rebase on Feb 5, 2023

in src/wallet/spend.cpp:110 in 896a560abe outdated

 97+    if (const auto provider = wallet->GetSolvingProvider(script_pubkey)) {
 98+        return InferDescriptor(script_pubkey, *provider);
 99+    }
100+    if (coin_control) {
101+        return InferDescriptor(script_pubkey, coin_control->m_external_provider);
102+    }

achow101 commented at 4:11 pm on February 8, 2023:

In 896a560abef52f41dbc12e3fedcd86ab4a657b06 “wallet: use descriptor satisfaction size to estimate inputs size”

I’m not sure if this is actually correct. I think there are some situations where this would return the wrong descriptor.

Consider a watchonly wallet where the user has imported an addr(). When creating a transaction, that user has provided the full wpkh() for that addr() on the command line. In this scenario, I believe GetSolvingProvider would return a SigningProvider, but it wouldn’t contain anything. This would then result in InferDescriptor making us an addr() that cannot be used to retrieve the input weight.

I think this could be solved by combining the retrieved solving provider with the external provider and doing a single InferDescriptor. This would also help in cases where the imported descriptor contains incomplete information, e.g. once we can have tr() with partial branches.

darosior commented at 3:34 pm on February 11, 2023:

Hmm good point. Unfortunately this is a bit clumsy for the LegacyScriptPubKeyMan: since it’s not a FlatSigningProvider it involves copying all its data in order to merge both providers. I’ve pushed the change in a separate commit to be squashed if it’s not too unreasonable. What do you think?

fanquake commented at 3:26 pm on February 24, 2023:

What do you think? @achow101 followup thoughts here?

darosior commented at 11:39 am on April 28, 2023:

Pushed a much cleaner version of this: introducing a MultiSigningProvider to wrap around both the wallet and external providers. Still as a commit on top, let me know what you think @achow101.

DrahtBot added the label Needs rebase on Feb 16, 2023

fanquake requested review from instagibbs on Feb 24, 2023

fanquake requested review from Sjors on Feb 24, 2023

darosior force-pushed on Apr 23, 2023

darosior force-pushed on Apr 28, 2023

DrahtBot removed the label Needs rebase on Apr 28, 2023

DrahtBot added the label CI failed on Apr 28, 2023

darosior force-pushed on Apr 29, 2023

DrahtBot removed the label CI failed on Apr 29, 2023

darosior commented at 9:10 pm on April 29, 2023: member

Rebased, and added Miniscript support since signing support was merged.

DrahtBot added the label Needs rebase on May 3, 2023

in src/script/descriptor.cpp:955 in 9e8df2de17 outdated

913+        return (1 + 32 + 1) * n_keys + BuildScript(m_threshold).size() + 1;
914+    }
915+
916+    std::optional<int64_t> MaxSatSize(bool use_max_sig) const override {
917+        return (1 + 65) * m_threshold + (m_pubkey_args.size() - m_threshold);
918+    }

achow101 commented at 8:08 pm on May 3, 2023:

In 9e8df2de1750b823492c18a924fefbd7d474819e “descriptor: introduce a method to get the satisfaction size”

Is MultiADescriptor supposed to be missing a MaxSatisfactionWeight?

darosior commented at 9:22 am on June 16, 2023:

Yes. It would never be used as MaxSatisfactionWeight is only called on top-level descriptors.

achow101 commented at 8:10 pm on May 3, 2023: member

The MultiSigningProvider seems like a fine approach.

darosior force-pushed on Jun 16, 2023

darosior commented at 9:54 am on June 16, 2023: member

Rebased, added more test coverage (we must be able to estimate the satisfaction size for any solvable top-level descriptor), properly integrated the MultiSigningProvider and removed the MaxSatisfactionWeight for MiniscriptDescriptor (which would never be called as they are never top-level).

DrahtBot removed the label Needs rebase on Jun 16, 2023

in src/script/miniscript.h:886 in 3e3521bf95 outdated

881+                const auto dsat{subs[0]->ws.dsat + subs[1]->ws.dsat};
882+                return {sat, dsat};
883+            }
884+            case Fragment::OR_C: return {subs[0]->ws.sat | (subs[0]->ws.dsat + subs[1]->ws.sat), {}};
885+            case Fragment::OR_D: return {subs[0]->ws.sat | (subs[0]->ws.dsat + subs[1]->ws.sat), subs[0]->ws.dsat + subs[1]->ws.dsat};
886+            case Fragment::OR_I: return {(subs[0]->ws.sat + 1 + 1) | (subs[1]->ws.sat + 1), (subs[0]->ws.dsat + 1 + 1) | (subs[1]->ws.dsat + 1)};

achow101 commented at 7:24 pm on June 23, 2023:

In 3e3521bf956c4387abe4b6ce12ce55224a9e97ba “miniscript: introduce a helper to get the maximum witness size”

I don’t quite follow why subs[0] needs an additional + 1 that subs[1] doesn’t have. AFAICT, subs[0] just has a 1 in the witness, while subs[1] has a 0, and both should be the same size.

darosior commented at 11:43 am on June 27, 2023:

Because serializing a 1 as a witness stack element needs to specify a 1-byte long element and then the value of this byte (serialized 0x0101) whereas encoding a 0 is simply the empty witness (serialized 0x00).

achow101 commented at 2:55 pm on June 27, 2023:

I thought 1 was OP_1?

darosior commented at 2:56 pm on June 27, 2023:

Not in witnesses. :)

achow101 commented at 3:11 pm on June 27, 2023:

Oh right, duh.

darosior force-pushed on Jun 27, 2023

DrahtBot added the label Needs rebase on Jul 4, 2023

darosior force-pushed on Jul 19, 2023

DrahtBot removed the label Needs rebase on Jul 19, 2023

darosior commented at 12:20 pm on July 19, 2023: member

Rebased.

darosior requested review from achow101 on Jul 19, 2023

darosior force-pushed on Jul 19, 2023

in src/script/descriptor.cpp:1192 in 037835f084 outdated

1160+    std::optional<int64_t> ScriptSize() const override { return m_node->ScriptSize(); }
1161+
1162+    std::optional<int64_t> MaxSatSize(bool) const override {
1163+        // We only ever parse satisfiable Miniscripts. Also note for Miniscript we always assume
1164+        // high-R ECDSA signatures.
1165+        return CHECK_NONFATAL(m_node->GetWitnessSize());

darosior commented at 1:32 pm on July 20, 2023:

This is not true if the Miniscript descriptor was parsed from Script. I’ll update this.

darosior commented at 9:17 am on July 21, 2023:

Done.

in src/script/miniscript.h:1216 in ecdee0c3f8 outdated

1151@@ -1152,12 +1152,12 @@ struct Node {
1152      * the script push. */
1153     std::optional<uint32_t> GetStackSize() const {
1154         if (!ss.sat.valid) return {};
1155-        return ss.sat.value + 1;
1156+        return ss.sat.value;

achow101 commented at 5:48 pm on July 20, 2023:

In ecdee0c3f802c6308c358a5ee501d2e031c8dcf2 “miniscript: make GetStackSize independent of P2WSH context”

The comment above this function is outdated.

darosior commented at 8:57 am on July 21, 2023:

Updated the comment.

darosior force-pushed on Jul 21, 2023

DrahtBot added the label Needs rebase on Jul 27, 2023

darosior force-pushed on Jul 31, 2023

darosior commented at 10:32 pm on July 31, 2023: member

Rebased after #27888.

DrahtBot removed the label Needs rebase on Aug 1, 2023

darosior requested review from achow101 on Aug 3, 2023

DrahtBot added the label Needs rebase on Aug 17, 2023

darosior force-pushed on Aug 18, 2023

darosior commented at 12:40 pm on August 18, 2023: member

Rebased after #28244.

DrahtBot removed the label Needs rebase on Aug 18, 2023

darosior commented at 8:28 pm on August 23, 2023: member

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.

in src/script/miniscript.h:1213 in a7db3f0503 outdated

1208@@ -1148,8 +1209,8 @@ struct Node {
1209         return true;
1210     }
1211 
1212-    /** Return the maximum number of stack elements needed to satisfy this script non-malleably, including
1213-     * the script push. */
1214+    /** Return the maximum number of stack elements needed to satisfy this script non-malleably.
1215+     * This does not account for the P2WSH script push. */

achow101 commented at 7:13 pm on August 24, 2023:

In a7db3f0503626c3671482720b22fedef7cd9e23c “miniscript: introduce a helper to get the maximum witness size”

nit: These comments should be changed in the previous commit.

darosior commented at 10:40 am on August 25, 2023:

Done.

in src/wallet/spend.cpp:126 in 310a738245 outdated

121+    }
122+
123+    // Otherwise, use the maximum satisfaction size provided by the descriptor.
124+    std::unique_ptr<Descriptor> desc{GetDescriptor(wallet, coin_control, txo.scriptPubKey)};
125+    if (desc) {
126+        if (const auto weight = MaxInputWeight(*desc, {txin}, coin_control, tx_is_segwit, can_grind_r)) return *weight;

achow101 commented at 7:13 pm on August 24, 2023:

In 310a7382454be01262b3fb2b089372a71a8b4482 “wallet: use descriptor satisfaction size to estimate inputs size”

nit: Could just return the result of MaxInputWeight directly.

darosior commented at 10:40 am on August 25, 2023:

Done.

achow101 commented at 7:20 pm on August 24, 2023: member

ACK 932556c87f939ffcdf1d591075d4c54bbd9bcaea

in src/test/descriptor_tests.cpp:153 in 07f457bb4d outdated

149@@ -150,6 +150,13 @@ void DoCheck(std::string prv, std::string pub, const std::string& norm_pub, int
150     parse_pub = Parse(pub, keys_pub, error);
151     BOOST_CHECK_MESSAGE(parse_pub, error);
152 
153+    // We must be able to estimate the max satisfaction size for any solvable descriptor top descriptor (but combo).

sipa commented at 7:20 pm on August 24, 2023:

Would a test to verify that the descriptor’s string length matches ScriptSize() make sense?

darosior commented at 10:40 am on August 25, 2023:

Good call, done.

in src/test/fuzz/descriptor_parse.cpp:134 in 07f457bb4d outdated

131@@ -132,6 +132,13 @@ static void TestDescriptor(const Descriptor& desc, FlatSigningProvider& sig_prov
132     if (!out_scripts.empty()) {
133         assert(InferDescriptor(out_scripts.back(), sig_provider));
134     }
135+

sipa commented at 7:22 pm on August 24, 2023:

Here too, test that ScriptSize() equals the actually generated script’s size?

darosior commented at 10:40 am on August 25, 2023:

Done.

in src/wallet/spend.cpp:137 in 310a738245 outdated

138-    int64_t vsize = GetVirtualTransactionSize(ctx);
139-    int64_t weight = GetTransactionWeight(ctx);
140-    return TxSize{vsize, weight};
141+    // nVersion + nLockTime + input count + output count
142+    int64_t weight = (4 + 4 + GetSizeOfCompactSize(tx.vin.size()) + GetSizeOfCompactSize(tx.vout.size())) * WITNESS_SCALE_FACTOR;
143+    // Whether any input spend a witness program. Necessary to run before the next loop over the

sipa commented at 7:33 pm on August 24, 2023:

Nit typo: spend -> spends

darosior commented at 10:40 am on August 25, 2023:

Done.

in src/wallet/spend.cpp:61 in 310a738245 outdated

56+ * @param can_grind_r Whether the signer will be able to grind the R of the signature.
57+ */
58+static std::optional<int64_t> MaxInputWeight(const Descriptor& desc, const std::optional<CTxIn>& txin,
59+                                             const CCoinControl* coin_control, const bool tx_is_segwit,
60+                                             const bool can_grind_r) {
61+    if (const auto sat_weight = desc.MaxSatisfactionWeight(can_grind_r && UseMaxSig(txin, coin_control))) {

sipa commented at 7:40 pm on August 24, 2023:

Is this can_grind_r && UseMaxSig(txin, coin_control) condition correct? The first operand sounds like something causing low-r, while the second sounds like something causing high-r.

darosior commented at 10:41 am on August 25, 2023:

Good catch, fixed. I was a bit concerned that this wouldn’t make any test fail, but it’s simply because those use wallets without external signer (so can_grind_r is always true). Actually getting the condition backward:

 0diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
 1index 750b6c100b..45f225b18f 100644
 2--- a/src/wallet/spend.cpp
 3+++ b/src/wallet/spend.cpp
 4@@ -58,7 +58,7 @@ static bool UseMaxSig(const std::optional<CTxIn>& txin, const CCoinControl* coin
 5 static std::optional<int64_t> MaxInputWeight(const Descriptor& desc, const std::optional<CTxIn>& txin,
 6                                              const CCoinControl* coin_control, const bool tx_is_segwit,
 7                                              const bool can_grind_r) {
 8-    if (const auto sat_weight = desc.MaxSatisfactionWeight(!can_grind_r || UseMaxSig(txin, coin_control))) {
 9+    if (const auto sat_weight = desc.MaxSatisfactionWeight(can_grind_r && !UseMaxSig(txin, coin_control))) {
10         if (const auto elems_count = desc.MaxSatisfactionElems()) {
11             const bool is_segwit = IsSegwit(desc);
12             // Account for the size of the scriptsig and the number of elements on the witness stack. Note

would make the tests for this feature fail.

in src/script/descriptor.cpp:1025 in 932556c87f outdated

1020+    std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 32; }
1021+
1022+    std::optional<int64_t> MaxSatSize(bool use_max_sig) const override {
1023+        if (const auto sat_size = m_subdescriptor_args[0]->MaxSatSize(use_max_sig)) {
1024+            if (const auto subscript_size = m_subdescriptor_args[0]->ScriptSize()) {
1025+                return 1 + *subscript_size + *sat_size;

sipa commented at 7:45 pm on August 24, 2023:

Does the 1 here come from push of the subscript? If so, should it not use the push size of the subscript size?

darosior commented at 10:42 am on August 25, 2023:

Thanks, done. This one slipped through my adapting this to consider Miniscript descriptors.

furszy commented at 8:01 pm on August 24, 2023: member

Concept ACK

Added to my queue. Will start reviewing next week.

miniscript: make GetStackSize independent of P2WSH context

It was taking into account the P2WSH script push in the number of stack
elements.

4ab382c2cd

miniscript: introduce a helper to get the maximum witness size

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.

bdba7667d2

darosior force-pushed on Aug 25, 2023

descriptor: introduce a method to get the satisfaction size

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()`).

fa7c46b503

script/signingprovider: introduce a MultiSigningProvider

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.

8d870a9873

wallet: use descriptor satisfaction size to estimate inputs size

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.

9b7ec393b8

wallet: accurately account for the size of the witness stack

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.

10546a569c

darosior force-pushed on Aug 25, 2023

in src/script/descriptor.cpp:1075 in fa7c46b503 outdated

1066@@ -958,6 +1067,13 @@ class TRDescriptor final : public DescriptorImpl
1067     }
1068     std::optional<OutputType> GetOutputType() const override { return OutputType::BECH32M; }
1069     bool IsSingleType() const final { return true; }
1070+
1071+    std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 32; }
1072+
1073+    std::optional<int64_t> MaxSatisfactionWeight(bool) const override {
1074+        // FIXME: We assume keypath spend, which can lead to very large underestimations.
1075+        return 1 + 65;

sipa commented at 1:42 pm on August 28, 2023:

Can we assume SIGHASH_DEFAULT? If so, 1 + 64 I think.

darosior commented at 2:07 pm on August 28, 2023:

Since this is estimating the maximum, i figured we shouldn’t assume SIGHASH_DEFAULT. Do you think we should?

sipa commented at 2:13 pm on August 28, 2023:

Yeah, that makes sense. It’d be nice if there was a way to pass down information about whether sighash_default signing is possible, but there is lower hanging fruit (like not always assuming key path spending…).

in src/script/descriptor.cpp:1190 in fa7c46b503 outdated

1181@@ -1059,6 +1182,13 @@ class RawTRDescriptor final : public DescriptorImpl
1182     RawTRDescriptor(std::unique_ptr<PubkeyProvider> output_key) : DescriptorImpl(Vector(std::move(output_key)), "rawtr") {}
1183     std::optional<OutputType> GetOutputType() const override { return OutputType::BECH32M; }
1184     bool IsSingleType() const final { return true; }
1185+
1186+    std::optional<int64_t> ScriptSize() const override { return 1 + 1 + 32; }
1187+
1188+    std::optional<int64_t> MaxSatisfactionWeight(bool) const override {
1189+        // We can't know whether there is a script path, so assume key path spend.
1190+        return 1 + 65;

sipa commented at 1:43 pm on August 28, 2023:

Here too, if SIGHASH_DEFAULT, 1 + 64?

darosior commented at 2:21 pm on August 28, 2023:

Resolving for the same reason as #26567 (review).

in src/wallet/spend.cpp:84 in 9b7ec393b8 outdated

83-    txn.vin.push_back(CTxIn(outpoint));
84-    if (!provider || !DummySignInput(*provider, txn.vin[0], txout, can_grind_r, coin_control)) {
85-        return -1;
86+    if (!provider) return -1;
87+
88+    if (const auto desc = InferDescriptor(txout.scriptPubKey, *provider)) {

sipa commented at 1:56 pm on August 28, 2023:

Without #24114, should we worry that this may miss cases like a P2PKH for which we don’t have the full public key, in legacy wallets? We’re technically able to estimate the input size for a spend of those, but (for now) can’t infer a better descriptor than addr.

darosior commented at 2:20 pm on August 28, 2023:

This wouldn’t miss any case that were handled previously as far as i can tell. DummySignInput would previously return false on a P2PKH for which we don’t know the public key. However there may be ways to improve upon the current situation, for instance we could set MaxInputWeight for an addr()/raw() descriptor that represents a P2(W)PKH, since we know it? But i’d prefer to first get this switch to using descriptors done with no regression and leave improvements for a follow-up, if reviewers don’t mind.

sipa commented at 2:24 pm on August 28, 2023:

Good point, I guess the observation is that right now, the satisfaction size estimation already only works for anything which can be represented as a (non addr/raw) descriptor, so doing through an inferred descriptor shouldn’t cause any regression. Making it work for more things can only improve the situation.

sipa commented at 2:26 pm on August 28, 2023: member

utACK 10546a569c6c96a5ec1b9708abf9ff5c8644f669

DrahtBot requested review from achow101 on Aug 28, 2023

fanquake added this to the milestone 26.0 on Aug 28, 2023

achow101 commented at 5:22 pm on September 6, 2023: member

re-ACK 10546a569c6c96a5ec1b9708abf9ff5c8644f669

DrahtBot removed review request from achow101 on Sep 6, 2023

achow101 merged this on Sep 6, 2023

achow101 closed this on Sep 6, 2023

darosior deleted the branch on Sep 7, 2023

Frank-GER referenced this in commit 899210cbcc on Sep 8, 2023

fanquake referenced this in commit e25af11225 on Sep 9, 2023

Frank-GER referenced this in commit 673fe98da9 on Sep 19, 2023

Wallet: estimate the size of signed inputs using descriptors #26567

Reviews

Conflicts