Merge bitcoin/bitcoin#28336: rpc: parse legacy pubkeys consistently with specific error messages

98570fe29bb08d7edc48011aa6b9731c6ab4ed2e test: add coverage for parsing cryptographically invalid pubkeys (Sebastian Falbesoner)
c740b154d193b91ca42f18759098d3fef6eaab05 rpc: use `HexToPubKey` helper for all legacy pubkey-parsing RPCs (Sebastian Falbesoner)
100e8a75bf5d8196c005331bd8f2ed42ada6d8d0 rpc: check and throw specific pubkey parsing errors in `HexToPubKey` (Sebastian Falbesoner)

Pull request description:

  Parsing legacy public keys can fail for three reasons (in this order):
  - pubkey is not in hex
  - pubkey has an invalid length (not 33 or 65 bytes for compressed/uncompressed, respectively)
  - pubkey is crytographically invalid, i.e. is not on curve (`CPubKey.IsFullyValid()` check)

  Many RPCs currently perform these checks manually with different error messages, even though we already have a `HexToPubKey` helper. This PR puts all three checks in this helper (the length check was done on the call-sites before), adds specific error messages for each case, and consequently uses it for all RPCs that parse legacy pubkeys. This leads to deduplicated code and also to more consistent and detailed error messages for the user.

  Affected RPC calls are `createmultisig`, `addmultisigaddress`, `importpubkey`, `importmulti`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send` and `sendall`.

  Note that the error code (-5 a.k.a. `RPC_INVALID_ADDRESS_OR_KEY`) doesn't change in any of the causes, so the changes are not breaking RPC API compatibility. Only the messages are more specific.

  The last commits adds test coverage for the cryptographically invalid (not-on-curve) pubkey case which wasn't exercised before.

ACKs for top commit:
  stratospher:
    tested ACK 98570fe.
  davidgumberg:
    ACK https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
  Eunovo:
    Tested ACK https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
  achow101:
    ACK 98570fe29bb08d7edc48011aa6b9731c6ab4ed2e

Tree-SHA512: cfa474176e95b5b18f3a9da28fdd9e87195cd58994c1331198f2840925fff322fd323a6371feab74a1b32e4b9ea58a6dc732fa751b4cdd45402c1029af609ece

2 files changed, 3 insertions, 24 deletions

diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index c05600484e..8d3eea59ee 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -456,12 +456,7 @@ RPCHelpMan importpubkey()
         throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
     }
 
-    if (!IsHex(request.params[0].get_str()))
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey must be a hex string");
-    std::vector<unsigned char> data(ParseHex(request.params[0].get_str()));
-    CPubKey pubKey(data);
-    if (!pubKey.IsFullyValid())
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey is not a valid public key");
+    CPubKey pubKey = HexToPubKey(request.params[0].get_str());
 
     {
         LOCK(pwallet->cs_wallet);
@@ -983,15 +978,7 @@ static UniValue ProcessImportLegacy(ImportData& import_data, std::map<CKeyID, CP
         import_data.witnessscript = std::make_unique<CScript>(parsed_witnessscript.begin(), parsed_witnessscript.end());
     }
     for (size_t i = 0; i < pubKeys.size(); ++i) {
-        const auto& str = pubKeys[i].get_str();
-        if (!IsHex(str)) {
-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey \"" + str + "\" must be a hex string");
-        }
-        auto parsed_pubkey = ParseHex(str);
-        CPubKey pubkey(parsed_pubkey);
-        if (!pubkey.IsFullyValid()) {
-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey \"" + str + "\" is not a valid public key");
-        }
+        CPubKey pubkey = HexToPubKey(pubKeys[i].get_str());
         pubkey_map.emplace(pubkey.GetID(), pubkey);
         ordered_pubkeys.push_back(pubkey.GetID());
     }
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index 6060f017ce..1a364a75ed 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -627,15 +627,7 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact
         const UniValue solving_data = options["solving_data"].get_obj();
         if (solving_data.exists("pubkeys")) {
             for (const UniValue& pk_univ : solving_data["pubkeys"].get_array().getValues()) {
-                const std::string& pk_str = pk_univ.get_str();
-                if (!IsHex(pk_str)) {
-                    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("'%s' is not hex", pk_str));
-                }
-                const std::vector<unsigned char> data(ParseHex(pk_str));
-                const CPubKey pubkey(data.begin(), data.end());
-                if (!pubkey.IsFullyValid()) {
-                    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("'%s' is not a valid public key", pk_str));
-                }
+                const CPubKey pubkey = HexToPubKey(pk_univ.get_str());
                 coinControl.m_external_provider.pubkeys.emplace(pubkey.GetID(), pubkey);
                 // Add witness script for pubkeys
                 const CScript wit_script = GetScriptForDestination(WitnessV0KeyHash(pubkey));