Merge bitcoin/bitcoin#25664: refactor: Redefine IsSolvable() using descriptors

b16f93cadd script/sign: remove needless IsSolvable() utility (Antoine Poinsot)
c232ef20c0 outputtype: remove redundant check for uncompressed keys in AddAndGetDestinationForScript (Antoine Poinsot)

Pull request description:

  Now that we have descriptors there is no need to try to sign for a scriptPubKey using dummy signatures, and using a mocked verification of this witness against the interpreter, just to make sure we know how to spend such a Script. Just try to infer a solvable descriptor: any scriptPubKey that we can sign for can be inferred as such.

  This came up in #24149 but i think it's worth it on its own.

ACKs for top commit:
  instagibbs:
    ACK b16f93cadd
  achow101:
    re-ACK b16f93cadd
  furszy:
    ACK b16f93ca, only change is the `IsSolvable` helper function removal.

Tree-SHA512: 137068157ce90210b710b1bf9ac3c400e2ff5af1112f892094b69875ea473d6a899f52adb51e5030cb907dee517602059cd1661107808558efa5de842ba12b41

src/outputtype.cpp

@@ -91,8 +91,6 @@ CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore,
     case OutputType::BECH32: {
         CTxDestination witdest = WitnessV0ScriptHash(script);
         CScript witprog = GetScriptForDestination(witdest);
-        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
-        if (!IsSolvable(keystore, witprog)) return ScriptHash(script);
         // Add the redeemscript, so that P2WSH and P2SH-P2WSH outputs are recognized as ours.
         keystore.AddCScript(witprog);
         if (type == OutputType::BECH32) {

src/script/sign.cpp

@@ -632,25 +632,6 @@ public:
 const BaseSignatureCreator& DUMMY_SIGNATURE_CREATOR = DummySignatureCreator(32, 32);
 const BaseSignatureCreator& DUMMY_MAXIMUM_SIGNATURE_CREATOR = DummySignatureCreator(33, 32);
 
-bool IsSolvable(const SigningProvider& provider, const CScript& script)
-{
-    // This check is to make sure that the script we created can actually be solved for and signed by us
-    // if we were to have the private keys. This is just to make sure that the script is valid and that,
-    // if found in a transaction, we would still accept and relay that transaction. In particular,
-    // it will reject witness outputs that require signing with an uncompressed public key.
-    SignatureData sigs;
-    // Make sure that STANDARD_SCRIPT_VERIFY_FLAGS includes SCRIPT_VERIFY_WITNESS_PUBKEYTYPE, the most
-    // important property this function is designed to test for.
-    static_assert(STANDARD_SCRIPT_VERIFY_FLAGS & SCRIPT_VERIFY_WITNESS_PUBKEYTYPE, "IsSolvable requires standard script flags to include WITNESS_PUBKEYTYPE");
-    if (ProduceSignature(provider, DUMMY_SIGNATURE_CREATOR, script, sigs)) {
-        // VerifyScript check is just defensive, and should never fail.
-        bool verified = VerifyScript(sigs.scriptSig, script, &sigs.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, DUMMY_CHECKER);
-        assert(verified);
-        return true;
-    }
-    return false;
-}
-
 bool IsSegWitOutput(const SigningProvider& provider, const CScript& script)
 {
     int version;

src/script/sign.h

@@ -97,12 +97,6 @@ bool SignSignature(const SigningProvider &provider, const CTransaction& txFrom,
 SignatureData DataFromTransaction(const CMutableTransaction& tx, unsigned int nIn, const CTxOut& txout);
 void UpdateInput(CTxIn& input, const SignatureData& data);
 
-/* Check whether we know how to sign for an output like this, assuming we
- * have all private keys. While this function does not need private keys, the passed
- * provider is used to look up public keys and redeemscripts by hash.
- * Solvability is unrelated to whether we consider this output to be ours. */
-bool IsSolvable(const SigningProvider& provider, const CScript& script);
-
 /** Check whether a scriptPubKey is known to be segwit. */
 bool IsSegWitOutput(const SigningProvider& provider, const CScript& script);
 

src/test/descriptor_tests.cpp

@@ -302,7 +302,6 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string&
             // For each of the produced scripts, verify solvability, and when possible, try to sign a transaction spending it.
             for (size_t n = 0; n < spks.size(); ++n) {
                 BOOST_CHECK_EQUAL(ref[n], HexStr(spks[n]));
-                BOOST_CHECK_EQUAL(IsSolvable(Merge(key_provider, script_provider), spks[n]), (flags & UNSOLVABLE) == 0);
 
                 if (flags & SIGNABLE) {
                     CMutableTransaction spend;
@@ -324,7 +323,7 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string&
                 BOOST_CHECK(inferred->Expand(0, provider_inferred, spks_inferred, provider_inferred));
                 BOOST_CHECK_EQUAL(spks_inferred.size(), 1U);
                 BOOST_CHECK(spks_inferred[0] == spks[n]);
-                BOOST_CHECK_EQUAL(IsSolvable(provider_inferred, spks_inferred[0]), !(flags & UNSOLVABLE));
+                BOOST_CHECK_EQUAL(InferDescriptor(spks_inferred[0], provider_inferred)->IsSolvable(), !(flags & UNSOLVABLE));
                 BOOST_CHECK(GetKeyOriginData(provider_inferred, flags) == GetKeyOriginData(script_provider, flags));
             }
 

src/test/fuzz/key.cpp

@@ -138,8 +138,6 @@ FUZZ_TARGET_INIT(key, initialize_key)
         assert(tx_multisig_script.size() == 37);
 
         FillableSigningProvider fillable_signing_provider;
-        assert(IsSolvable(fillable_signing_provider, tx_pubkey_script));
-        assert(IsSolvable(fillable_signing_provider, tx_multisig_script));
         assert(!IsSegWitOutput(fillable_signing_provider, tx_pubkey_script));
         assert(!IsSegWitOutput(fillable_signing_provider, tx_multisig_script));
         assert(fillable_signing_provider.GetKeys().size() == 0);

src/test/fuzz/script.cpp

@@ -89,7 +89,6 @@ FUZZ_TARGET_INIT(script, initialize_script)
     const FlatSigningProvider signing_provider;
     (void)InferDescriptor(script, signing_provider);
     (void)IsSegWitOutput(signing_provider, script);
-    (void)IsSolvable(signing_provider, script);
 
     (void)RecursiveDynamicUsage(script);
 

src/wallet/rpc/addresses.cpp

@@ -578,7 +578,7 @@ RPCHelpMan getaddressinfo()
 
     if (provider) {
         auto inferred = InferDescriptor(scriptPubKey, *provider);
-        bool solvable = inferred->IsSolvable() || IsSolvable(*provider, scriptPubKey);
+        bool solvable = inferred->IsSolvable();
         ret.pushKV("solvable", solvable);
         if (solvable) {
             ret.pushKV("desc", inferred->ToString());

src/wallet/scriptpubkeyman.cpp

@@ -1456,7 +1456,8 @@ void LegacyScriptPubKeyMan::LearnRelatedScripts(const CPubKey& key, OutputType t
         CTxDestination witdest = WitnessV0KeyHash(key.GetID());
         CScript witprog = GetScriptForDestination(witdest);
         // Make sure the resulting program is solvable.
-        assert(IsSolvable(*this, witprog));
+        const auto desc = InferDescriptor(witprog, *this);
+        assert(desc && desc->IsSolvable());
         AddCScript(witprog);
     }
 }