descriptors: do not return top-level only funcs as sub descriptors

furszy commented at 3:53 pm on July 11, 2023: member

Linked to #28057.

Currently, the InferScript function returns an invalid descriptor when it tries to infer a p2sh-p2pkh script whose pubkey is not known by the wallet.

This behavior occurs because the inference process bypasses the pkh subscript when the pubkey is not contained by the wallet (no pubkey provider), interpreting it as a sh(addr(ADDR)) descriptor. Then, the failure arises because the addr() function is restricted to being used only at the top level.

For reviewers, would recommend to start by examining the functional test to understand the context and the circumstances on which this can result in a fatal error (e.g. during the migration process).

wallet: loading, log descriptor parsing error details

The `UNKNOWN_DESCRIPTOR` error comes from the
`WalletDescriptor::DeserializeDescriptor` std::ios_base
exception, which contains further information about the
parsing error.

286e0c7d5e

DrahtBot commented at 3:54 pm on July 11, 2023: 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	darosior, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

maflcko commented at 4:04 pm on July 11, 2023: member

For reviewers, would recommend to start by examining the functional test to understand the context and the circumstances on which this can result in a fatal error (e.g. during the migration process).

The added functional test doesn’t crash, does it?

furszy commented at 4:25 pm on July 11, 2023: member

For reviewers, would recommend to start by examining the functional test to understand the context and the circumstances on which this can result in a fatal error (e.g. during the migration process).

The added functional test doesn’t crash, does it?

Yeah. Without the fix commit, the migration process in the test will pass “successfully” and then, at the wallet restart verification, the loading process will throw the “Unrecognized descriptor found” error. Which denotes that the migrated wallet is in an unusable state. With the fix commit, the migration and the restart verification will pass successfully.

For your issue crash cause, I think that I’m getting closer to it. But yeah, it shouldn’t be fixed by this PR. Still, try it please. The first commit adds logging that might give us some useful information.

sipa commented at 4:29 pm on July 11, 2023: member

This is clearly an issue; we should not be inferring descriptors that the code itself doesn’t accept back.

On the other hand, maybe we should just permit addr() and raw() inside sh/wsh/tr. There is discussion about that in #24114 too.

in src/script/descriptor.cpp:1660 in 185f8a33b1 outdated

1656@@ -1657,6 +1657,7 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
1657         if (provider.GetCScript(scriptid, subscript)) {
1658             auto sub = InferScript(subscript, ParseScriptContext::P2SH, provider);
1659             if (sub) return std::make_unique<SHDescriptor>(std::move(sub));
1660+            else return std::make_unique<RawDescriptor>(script);

sipa commented at 4:48 pm on July 11, 2023:

Coding style nit: if you have more than a single line if statement, you must use braces and indentation.

furszy commented at 7:08 pm on July 14, 2023:

fixed

furszy commented at 5:32 pm on July 11, 2023: member

On the other hand, maybe we should just permit addr() and raw() inside sh/wsh/tr. There is discussion about that in #24114 too.

Nice, I was asking myself the same question. I actually started implementing this as a custom, sort of dummy, pubkey provider which had no knowledge of its keys, only containing the key ids, inside the PKHDescriptor. So it mapped to the original sh(pkh(key_id)). But.. I ended up preferring this approach for the “controversy” that the other one could had (a pubkey provider with no pubkeys wouldn’t be a pubkey provider anymore..). What I liked from that approach was that it allowed us to update the descriptor whenever the wallet gained knowledge of the script pubkey(s). Still, this last point can be achieved differently too.

So, great. Will explore #24114 further tomorrow and maybe cook something for it to see how it would look like.

furszy commented at 8:24 pm on July 12, 2023: member

@sipa, how the allowance of internal raw() subscripts would play out with previous releases?.

I’m thinking on the current wallet migration process, which is broken for pubkey unknown sh(pkh) scripts (also for wsh(pkh)).

We might want this PR to fix the bug and be backported. Then, after having all the pertinent discussions etc. We could implement the new feature (compat breaking change?) for the next release.

sipa commented at 3:53 pm on July 13, 2023: member

@furszy I think there is only a downgrade issue, no? If someone were to import a sh(raw(...)) as watch-only address in a descriptor wallet after allowing that, and then downgrades to older software which does not support it.

Ping @achow101.

achow101 commented at 4:31 pm on July 13, 2023: member

Allowing new types of descriptors only disallows downgrading of wallets that have imported them. Downgrade protection is automatic for such wallets since descriptor wallets were introduced.

furszy commented at 7:05 pm on July 13, 2023: member

@furszy I think there is only a downgrade issue, no? If someone were to import a sh(raw(…)) as watch-only address in a descriptor wallet after allowing that, and then downgrades to older software which does not support it.

Yeah good @sipa and @achow101. We are sync then.

My point was more about agreeing that the bug fix should go first, so it can be backported for users of v24 and v25. Preventing them from entering an unrecoverable state after migration.

And then we can move forward implementing the “raw() subscript” allowance new feature. Which is downgrade incompatible. (happy to work on it).

Agree?

furszy force-pushed on Jul 14, 2023

furszy renamed this:
~~[WIP] descriptors: do not return top-level only funcs as sub descriptors~~
descriptors: do not return top-level only funcs as sub descriptors
on Jul 14, 2023

DrahtBot added the label Descriptors on Jul 14, 2023

furszy force-pushed on Jul 14, 2023

achow101 commented at 7:52 pm on July 14, 2023: member

ACK 47abfe1525cc2700699ba0605747f1ad36a34a1b

furszy force-pushed on Jul 14, 2023

furszy commented at 9:44 pm on July 14, 2023: member

Little push to update the missing todo in the test. Tiny diff.

DrahtBot added the label CI failed on Jul 14, 2023

furszy commented at 2:53 pm on July 15, 2023: member

Failing CI task isn’t related. Failure is on feature_fee_estimation.py, in the periodical flush case.

in src/script/descriptor.cpp:1664 in 3b483e72be outdated

1660+            if (sub) {
1661+                return std::make_unique<SHDescriptor>(std::move(sub));
1662+            } else {
1663+                // When the sh subscript is unknown or unsolvable, parse it as a raw descriptor
1664+                return std::make_unique<RawDescriptor>(script);
1665+            }

darosior commented at 12:53 pm on July 20, 2023:

This is unnecessary? It would be returned as such at the end of the function. And not as raw() as in your test, but as an actual address.

furszy commented at 2:21 pm on July 20, 2023:

I initially did it without the early return. But IIRC, the inference of a non-standard script as an addr() instead of a raw() made me have a second thought.

Still, all good. Removed the early return.

in src/script/descriptor.cpp:1677 in 3b483e72be outdated

1674+            if (sub) {
1675+                return std::make_unique<WSHDescriptor>(std::move(sub));
1676+            } else {
1677+                // When the wsh subscript is unknown or unsolvable, parse it as a raw descriptor
1678+                return std::make_unique<RawDescriptor>(script);
1679+            }

darosior commented at 12:54 pm on July 20, 2023:

Same here but it’s also incorrect because we’re not necessarily at top level in this branch. This could create a sh(raw()) which would be invalid.

furszy commented at 2:11 pm on July 20, 2023:

upps, true. Fixed.

darosior changes_requested

darosior commented at 1:11 pm on July 20, 2023: member

Concept ACK. Good catch. However i think you reintroduce the same bug in the second commit by returning early as raw() when in the first case it should be an address descriptor and the second it should fail if not at top-level.

 0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
 1index 9e4f775f41..09ded5fc61 100644
 2--- a/src/script/descriptor.cpp
 3+++ b/src/script/descriptor.cpp
 4@@ -1662,12 +1662,7 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
 5         CScript subscript;
 6         if (provider.GetCScript(scriptid, subscript)) {
 7             auto sub = InferScript(subscript, ParseScriptContext::P2SH, provider);
 8-            if (sub) {
 9-                return std::make_unique<SHDescriptor>(std::move(sub));
10-            } else {
11-                // When the sh subscript is unknown or unsolvable, parse it as a raw descriptor
12-                return std::make_unique<RawDescriptor>(script);
13-            }
14+            if (sub) return std::make_unique<SHDescriptor>(std::move(sub));
15         }
16     }
17     if (txntype == TxoutType::WITNESS_V0_SCRIPTHASH && (ctx == ParseScriptContext::TOP || ctx == ParseScriptContext::P2SH)) {
18@@ -1675,12 +1670,7 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
19         CScript subscript;
20         if (provider.GetCScript(scriptid, subscript)) {
21             auto sub = InferScript(subscript, ParseScriptContext::P2WSH, provider);
22-            if (sub) {
23-                return std::make_unique<WSHDescriptor>(std::move(sub));
24-            } else {
25-                // When the wsh subscript is unknown or unsolvable, parse it as a raw descriptor
26-                return std::make_unique<RawDescriptor>(script);
27-            }
28+            if (sub) return std::make_unique<WSHDescriptor>(std::move(sub));
29         }
30     }
31     if (txntype == TxoutType::WITNESS_V1_TAPROOT && ctx == ParseScriptContext::TOP) {
32diff --git a/test/functional/data/rpc_decodescript.json b/test/functional/data/rpc_decodescript.json
33index fd0f30d86e..4a15ae8792 100644
34--- a/test/functional/data/rpc_decodescript.json
35+++ b/test/functional/data/rpc_decodescript.json
36@@ -69,7 +69,7 @@
37             "p2sh": "2N34iiGoUUkVSPiaaTFpJjB1FR9TXQu3PGM",
38             "segwit": {
39                 "asm": "0 96c2368fc30514a438a8bd909f93c49a1549d77198ccbdb792043b666cb24f42",
40-                "desc": "raw(002096c2368fc30514a438a8bd909f93c49a1549d77198ccbdb792043b666cb24f42)#33khjalh",
41+                "desc": "addr(bcrt1qjmprdr7rq522gw9ghkgfly7yng25n4m3nrxtmdujqsakvm9jfapqk795l5)#5akkdska",
42                 "hex": "002096c2368fc30514a438a8bd909f93c49a1549d77198ccbdb792043b666cb24f42",
43                 "address": "bcrt1qjmprdr7rq522gw9ghkgfly7yng25n4m3nrxtmdujqsakvm9jfapqk795l5",
44                 "type": "witness_v0_scripthash",
45diff --git a/test/functional/rpc_decodescript.py b/test/functional/rpc_decodescript.py
46index c8390a66b3..854363ac68 100755
47--- a/test/functional/rpc_decodescript.py
48+++ b/test/functional/rpc_decodescript.py
49@@ -278,7 +278,7 @@ class DecodeScriptTest(BitcoinTestFramework):
50         res = self.nodes[0].decodescript("82012088a914ffffffffffffffffffffffffffffffffffffffff882102ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffacb2")
51         # Check v0 wsh. Output will be OP_0 + sha256(script)
52         script_pubkey = CScript([OP_0, sha256(bytes.fromhex("82012088a914ffffffffffffffffffffffffffffffffffffffff882102ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffacb2"))])
53-        assert res["segwit"]["desc"] == descsum_create(f"raw({script_pubkey.hex()})")
54+        assert res["segwit"]["desc"] == descsum_create(f"addr(bcrt1q73qyfypp47hvgnkjqnav0j3k2lq3v76wg22dk8tmwuz5sfgv66xsvxg6uu)")
55         # Miniscript-compatible multisig bigger than 520 byte P2SH limit.
56         res = self.nodes[0].decodescript("5b21020e0338c96a8870479f2396c373cc7696ba124e8635d41b0ea581112b678172612102675333a4e4b8fb51d9d4e22fa5a8eaced3fdac8a8cbf9be8c030f75712e6af992102896807d54bc55c24981f24a453c60ad3e8993d693732288068a23df3d9f50d4821029e51a5ef5db3137051de8323b001749932f2ff0d34c82e96a2c2461de96ae56c2102a4e1a9638d46923272c266631d94d36bdb03a64ee0e14c7518e49d2f29bc401021031c41fdbcebe17bec8d49816e00ca1b5ac34766b91c9f2ac37d39c63e5e008afb2103079e252e85abffd3c401a69b087e590a9b86f33f574f08129ccbd3521ecf516b2103111cf405b627e22135b3b3733a4a34aa5723fb0f58379a16d32861bf576b0ec2210318f331b3e5d38156da6633b31929c5b220349859cc9ca3d33fb4e68aa08401742103230dae6b4ac93480aeab26d000841298e3b8f6157028e47b0897c1e025165de121035abff4281ff00660f99ab27bb53e6b33689c2cd8dcd364bc3c90ca5aea0d71a62103bd45cddfacf2083b14310ae4a84e25de61e451637346325222747b157446614c2103cc297026b06c71cbfa52089149157b5ff23de027ac5ab781800a578192d175462103d3bde5d63bdb3a6379b461be64dad45eabff42f758543a9645afd42f6d4248282103ed1e8d5109c9ed66f7941bc53cc71137baa76d50d274bda8d5e8ffbd6e61fe9a5fae736402c00fb269522103aab896d53a8e7d6433137bbba940f9c521e085dd07e60994579b64a6d992cf79210291b7d0b1b692f8f524516ed950872e5da10fb1b808b5a526dedc6fed1cf29807210386aa9372fbab374593466bc5451dc59954e90787f08060964d95c87ef34ca5bb53ae68")
57         assert_equal(res["segwit"]["desc"], "wsh(or_d(multi(11,020e0338c96a8870479f2396c373cc7696ba124e8635d41b0ea581112b67817261,02675333a4e4b8fb51d9d4e22fa5a8eaced3fdac8a8cbf9be8c030f75712e6af99,02896807d54bc55c24981f24a453c60ad3e8993d693732288068a23df3d9f50d48,029e51a5ef5db3137051de8323b001749932f2ff0d34c82e96a2c2461de96ae56c,02a4e1a9638d46923272c266631d94d36bdb03a64ee0e14c7518e49d2f29bc4010,031c41fdbcebe17bec8d49816e00ca1b5ac34766b91c9f2ac37d39c63e5e008afb,03079e252e85abffd3c401a69b087e590a9b86f33f574f08129ccbd3521ecf516b,03111cf405b627e22135b3b3733a4a34aa5723fb0f58379a16d32861bf576b0ec2,0318f331b3e5d38156da6633b31929c5b220349859cc9ca3d33fb4e68aa0840174,03230dae6b4ac93480aeab26d000841298e3b8f6157028e47b0897c1e025165de1,035abff4281ff00660f99ab27bb53e6b33689c2cd8dcd364bc3c90ca5aea0d71a6,03bd45cddfacf2083b14310ae4a84e25de61e451637346325222747b157446614c,03cc297026b06c71cbfa52089149157b5ff23de027ac5ab781800a578192d17546,03d3bde5d63bdb3a6379b461be64dad45eabff42f758543a9645afd42f6d424828,03ed1e8d5109c9ed66f7941bc53cc71137baa76d50d274bda8d5e8ffbd6e61fe9a),and_v(v:older(4032),multi(2,03aab896d53a8e7d6433137bbba940f9c521e085dd07e60994579b64a6d992cf79,0291b7d0b1b692f8f524516ed950872e5da10fb1b808b5a526dedc6fed1cf29807,0386aa9372fbab374593466bc5451dc59954e90787f08060964d95c87ef34ca5bb))))#7jwwklk4")

descriptor: InferScript, do not return top-level only func as sub descriptor

e.g. sh(addr(ADDR)) or sh(raw(HEX)) are invalid descriptors.

Making sh and wsh top level functions to return addr/raw descriptors when
the subscript inference fails.

cc781a2180

test: wallet, add coverage for watch-only raw sh script migration dd9633b516

furszy force-pushed on Jul 20, 2023

furszy commented at 2:22 pm on July 20, 2023: member

Updated per feedback. Thanks @darosior!.

darosior approved

darosior commented at 2:52 pm on July 20, 2023: member

utACK dd9633b516d6936ac4e23a40f9b0bea120117d35

DrahtBot requested review from achow101 on Jul 20, 2023

achow101 commented at 3:09 pm on July 20, 2023: member

ACK dd9633b516d6936ac4e23a40f9b0bea120117d35

DrahtBot removed review request from achow101 on Jul 20, 2023

achow101 merged this on Jul 20, 2023

achow101 closed this on Jul 20, 2023

fanquake added the label Needs backport (25.x) on Jul 20, 2023

sidhujag referenced this in commit bdf0d1f139 on Jul 21, 2023

fanquake referenced this in commit 6d5a510dcd on Jul 21, 2023

fanquake referenced this in commit 513ca0a711 on Jul 21, 2023

fanquake commented at 8:54 am on July 21, 2023: member

Backported 2/3 of the commits here in #https://github.com/bitcoin/bitcoin/pull/28067.

fanquake removed the label Needs backport (25.x) on Jul 21, 2023

furszy deleted the branch on Jul 21, 2023

fanquake referenced this in commit ecc74cd4f3 on Sep 6, 2023

bitcoin locked this on Jul 21, 2024

descriptors: do not return top-level only funcs as sub descriptors #28067

Reviews