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

[furszy]

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

[DrahtBot]

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]

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]

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]

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.

[furszy]

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]

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

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

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]

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

[achow101]

ACK 47abfe1

[furszy]

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

[furszy]

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

[darosior]

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.

This diff fixes it:

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 9e4f775f41..09ded5fc61 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1662,12 +1662,7 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
         CScript subscript;
         if (provider.GetCScript(scriptid, subscript)) {
             auto sub = InferScript(subscript, ParseScriptContext::P2SH, provider);
-            if (sub) {
-                return std::make_unique<SHDescriptor>(std::move(sub));
-            } else {
-                // When the sh subscript is unknown or unsolvable, parse it as a raw descriptor
-                return std::make_unique<RawDescriptor>(script);
-            }
+            if (sub) return std::make_unique<SHDescriptor>(std::move(sub));
         }
     }
     if (txntype == TxoutType::WITNESS_V0_SCRIPTHASH && (ctx == ParseScriptContext::TOP || ctx == ParseScriptContext::P2SH)) {
@@ -1675,12 +1670,7 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
         CScript subscript;
         if (provider.GetCScript(scriptid, subscript)) {
             auto sub = InferScript(subscript, ParseScriptContext::P2WSH, provider);
-            if (sub) {
-                return std::make_unique<WSHDescriptor>(std::move(sub));
-            } else {
-                // When the wsh subscript is unknown or unsolvable, parse it as a raw descriptor
-                return std::make_unique<RawDescriptor>(script);
-            }
+            if (sub) return std::make_unique<WSHDescriptor>(std::move(sub));
         }
     }
     if (txntype == TxoutType::WITNESS_V1_TAPROOT && ctx == ParseScriptContext::TOP) {
diff --git a/test/functional/data/rpc_decodescript.json b/test/functional/data/rpc_decodescript.json
index fd0f30d86e..4a15ae8792 100644
--- a/test/functional/data/rpc_decodescript.json
+++ b/test/functional/data/rpc_decodescript.json
@@ -69,7 +69,7 @@
             "p2sh": "2N34iiGoUUkVSPiaaTFpJjB1FR9TXQu3PGM",
             "segwit": {
                 "asm": "0 96c2368fc30514a438a8bd909f93c49a1549d77198ccbdb792043b666cb24f42",
-                "desc": "raw(002096c2368fc30514a438a8bd909f93c49a1549d77198ccbdb792043b666cb24f42)#33khjalh",
+                "desc": "addr(bcrt1qjmprdr7rq522gw9ghkgfly7yng25n4m3nrxtmdujqsakvm9jfapqk795l5)#5akkdska",
                 "hex": "002096c2368fc30514a438a8bd909f93c49a1549d77198ccbdb792043b666cb24f42",
                 "address": "bcrt1qjmprdr7rq522gw9ghkgfly7yng25n4m3nrxtmdujqsakvm9jfapqk795l5",
                 "type": "witness_v0_scripthash",
diff --git a/test/functional/rpc_decodescript.py b/test/functional/rpc_decodescript.py
index c8390a66b3..854363ac68 100755
--- a/test/functional/rpc_decodescript.py
+++ b/test/functional/rpc_decodescript.py
@@ -278,7 +278,7 @@ class DecodeScriptTest(BitcoinTestFramework):
         res = self.nodes[0].decodescript("82012088a914ffffffffffffffffffffffffffffffffffffffff882102ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffacb2")
         # Check v0 wsh. Output will be OP_0 + sha256(script)
         script_pubkey = CScript([OP_0, sha256(bytes.fromhex("82012088a914ffffffffffffffffffffffffffffffffffffffff882102ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffacb2"))])
-        assert res["segwit"]["desc"] == descsum_create(f"raw({script_pubkey.hex()})")
+        assert res["segwit"]["desc"] == descsum_create(f"addr(bcrt1q73qyfypp47hvgnkjqnav0j3k2lq3v76wg22dk8tmwuz5sfgv66xsvxg6uu)")
         # Miniscript-compatible multisig bigger than 520 byte P2SH limit.
         res = self.nodes[0].decodescript("5b21020e0338c96a8870479f2396c373cc7696ba124e8635d41b0ea581112b678172612102675333a4e4b8fb51d9d4e22fa5a8eaced3fdac8a8cbf9be8c030f75712e6af992102896807d54bc55c24981f24a453c60ad3e8993d693732288068a23df3d9f50d4821029e51a5ef5db3137051de8323b001749932f2ff0d34c82e96a2c2461de96ae56c2102a4e1a9638d46923272c266631d94d36bdb03a64ee0e14c7518e49d2f29bc401021031c41fdbcebe17bec8d49816e00ca1b5ac34766b91c9f2ac37d39c63e5e008afb2103079e252e85abffd3c401a69b087e590a9b86f33f574f08129ccbd3521ecf516b2103111cf405b627e22135b3b3733a4a34aa5723fb0f58379a16d32861bf576b0ec2210318f331b3e5d38156da6633b31929c5b220349859cc9ca3d33fb4e68aa08401742103230dae6b4ac93480aeab26d000841298e3b8f6157028e47b0897c1e025165de121035abff4281ff00660f99ab27bb53e6b33689c2cd8dcd364bc3c90ca5aea0d71a62103bd45cddfacf2083b14310ae4a84e25de61e451637346325222747b157446614c2103cc297026b06c71cbfa52089149157b5ff23de027ac5ab781800a578192d175462103d3bde5d63bdb3a6379b461be64dad45eabff42f758543a9645afd42f6d4248282103ed1e8d5109c9ed66f7941bc53cc71137baa76d50d274bda8d5e8ffbd6e61fe9a5fae736402c00fb269522103aab896d53a8e7d6433137bbba940f9c521e085dd07e60994579b64a6d992cf79210291b7d0b1b692f8f524516ed950872e5da10fb1b808b5a526dedc6fed1cf29807210386aa9372fbab374593466bc5451dc59954e90787f08060964d95c87ef34ca5bb53ae68")
         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")

[furszy]

Updated per feedback. Thanks @darosior!.

[darosior]

utACK dd9633b

[achow101]

ACK dd9633b

[fanquake]

Backported 2/3 of the commits here in ##28067.