descriptor, rpc, wallet: do not echo key material in error messages #36648

For wpkh(KEY)x, the error is now 'wpkh(...' is not a valid descriptor function. wpkh is a valid function; the real problem is the trailing x. The old message echoed the full text, so the mistake was visible.

you can make it skip the key inside the function print the chars that is connected to ).

somethiing like that:

Invalid descriptor expression 'wpkh(...)x'

suggested diff:

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 3ae2da81d..29dbfa3a7 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -2828,15 +2828,29 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
         error = "A function is needed within P2WSH";
         return {};
     }
-    // Quote the function name only if it is 16 characters or fewer: every key is longer, and a bare key has no '('
+    // Show the expression as "name(...)rest", where the arguments are never quoted, the name
+    // only if it is 16 characters or fewer (every key is longer, and a bare key has no '('),
+    // and the text after the matching ')' only if it is no longer than MAX_QUOTED_TOKEN_CHARS.
     constexpr size_t MAX_FUNCTION_NAME_CHARS{16};
     const std::string_view expr_text{expr.begin(), expr.end()};
-    const size_t paren{expr_text.find('(')};
-    if (paren == std::string_view::npos || paren == 0 || paren > MAX_FUNCTION_NAME_CHARS) {
+    const size_t open{expr_text.find('(')};
+    if (open == std::string_view::npos || open == 0 || open > MAX_FUNCTION_NAME_CHARS) {
         error = "Not a valid descriptor function";
-    } else {
-        error = strprintf("'%s(...' is not a valid descriptor function", expr_text.substr(0, paren));
+        return {};
+    }
+    std::string shown{strprintf("%s(...", expr_text.substr(0, open))};
+    int depth{0};
+    for (size_t i{open}; i < expr_text.size(); ++i) {
+        if (expr_text[i] == '(') {
+            ++depth;
+        } else if (expr_text[i] == ')' && --depth == 0) {
+            const std::string_view rest{expr_text.substr(i + 1)};
+            shown += ')';
+            shown += rest.size() <= MAX_QUOTED_TOKEN_CHARS ? rest : std::string_view{"..."};
+            break;
+        }
     }
+    error = strprintf("Invalid descriptor expression '%s'", shown);
     return {};
 }

In a descriptor with many keys, the error doesn't say which one failed. For wsh(multi(2,KEY1,KEY2,KEY3)) with a typo in KEY3, the error is only:

Multi: key is not valid

With 15 keys, the user has to check each one by hand.

You can print the key's position. key_exp_index already counts the keys parsed before the failing one, so no key text is needed.

Something like that:

Multi: Key [#3](/bitcoin-bitcoin/3/) is not valid

Note: the count includes each musig() aggregate key, so a key after a musig() gets a number one higher than a user would count. The test expectations for these messages still need updating.

In a descriptor with many keys, the error doesn't say which one failed. For wsh(multi(2,KEY1,KEY2,KEY3)) with a typo in KEY3, the error is only:

Multi: key is not valid

With 15 keys, the user has to check each one by hand.

You can print the key's position. key_exp_index already counts the keys parsed before the failing one, so no key text is needed.

Something like that:

Multi: Key [#3](/bitcoin-bitcoin/3/) is not valid

Note: the count includes each musig() aggregate key, so a key after a musig() gets a number one higher than a user would count. The test expectations for these messages still need updating.

similar to the key index solving_data.descriptors is a list, now that the descriptor is no longer printed, the error doesn't say which one failed:

Unable to parse descriptor: Missing checksum

You can print its index in the list.

should be:

Unable to parse descriptor at index 1: Missing checksum

we could do something like that instead:

diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index 383f752dc..0ab874793 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -622,14 +622,15 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact
         }
 
         if (solving_data.exists("descriptors")) {
-            for (const UniValue& desc_univ : solving_data["descriptors"].get_array().getValues()) {
-                const std::string& desc_str  = desc_univ.get_str();
+            const auto& desc_values{solving_data["descriptors"].get_array().getValues()};
+            for (size_t desc_index{0}; desc_index < desc_values.size(); ++desc_index) {
+                const std::string& desc_str{desc_values[desc_index].get_str()};
                 FlatSigningProvider desc_out;
                 std::string error;
                 std::vector<CScript> scripts_temp;
                 auto descs = Parse(desc_str, desc_out, error, true);
                 if (descs.empty()) {
-                    throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Unable to parse descriptor: %s", error));
+                    throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Unable to parse descriptor at index %u: %s", desc_index, error));
                 }
                 for (auto& desc : descs) {
                     desc->Expand(0, desc_out, scripts_temp, desc_out);

i see you are using the same value of MAX_QUOTED_CHARS in https://github.com/bitcoin/bitcoin/pull/36648/changes#diff-dcb25ae6b5207f65ae6a1cd364c9e62e5a9df69a1cd21b9ed79849cc99c2150bR1880

you can define it here and reuse it inside descriptor.cpp