rpc: parse legacy pubkeys consistently with specific error messages

theStack commented at 3:14 pm on August 24, 2023: contributor

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.

DrahtBot commented at 3:14 pm on August 24, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stratospher, achow101, Eunovo, davidgumberg
Concept ACK	sipa
Stale ACK	kevkevinpal

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

Conflicts

No conflicts as of last run.

DrahtBot added the label RPC/REST/ZMQ on Aug 24, 2023

sipa commented at 3:18 pm on August 24, 2023: member

Concept ACK

in src/rpc/util.cpp:186 in 194b4e6ab7 outdated

178@@ -179,11 +179,14 @@ std::string HelpExampleRpcNamed(const std::string& methodname, const RPCArgList&
179 CPubKey HexToPubKey(const std::string& hex_in)
180 {
181     if (!IsHex(hex_in)) {
182-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + hex_in);
183+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey \"" + hex_in + "\" must be a hex string");
184+    }
185+    if (hex_in.length() != 66 && hex_in.length() != 130) {
186+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey \"" + hex_in + "\" must have a length of either 33 or 65 bytes");

kevkevinpal commented at 3:01 am on August 30, 2023:

nit: not sure how often this HexToPubKey is called but I’m pretty sure hex_in.length() != 66 && hex_in.length() != 130 will fail faster than !IsHex(hex_in) so it would be preferred to do that first

feel free to ignore this comment if that efficiency bump isn’t a concern

theStack commented at 12:16 pm on February 2, 2024:

Sorry for the late reply. I think the exact order of the checks doesn’t really matter here from a performance perspective, since in the typical case (if the passed pubkey is valid) all three of them are executed anyway. Also, if users pass in something that has a different format like e.g. a Bitcoin address, the error message “pubkey has to be in hex” seems to be more useful than “pubkey has the wrong length”.

kevkevinpal commented at 2:34 pm on February 8, 2024:

yup makes sense and I agree!

kevkevinpal commented at 3:08 am on August 30, 2023: contributor

ACK 194b4e6

added one comment but feel free to ignore

DrahtBot added the label CI failed on Sep 3, 2023

DrahtBot removed the label CI failed on Sep 5, 2023

DrahtBot added the label CI failed on Jan 12, 2024

theStack force-pushed on Jan 22, 2024

DrahtBot removed the label CI failed on Jan 22, 2024

theStack commented at 5:06 pm on January 22, 2024: contributor

Rebased on master (to fix CI).

in src/wallet/rpc/addresses.cpp:270 in 0f7a1139b1 outdated

266@@ -267,7 +267,7 @@ RPCHelpMan addmultisigaddress()
267     const UniValue& keys_or_addrs = request.params[1].get_array();
268     std::vector<CPubKey> pubkeys;
269     for (unsigned int i = 0; i < keys_or_addrs.size(); ++i) {
270-        if (IsHex(keys_or_addrs[i].get_str()) && (keys_or_addrs[i].get_str().length() == 66 || keys_or_addrs[i].get_str().length() == 130)) {
271+        if (IsHex(keys_or_addrs[i].get_str())) {

stratospher commented at 5:08 am on February 9, 2024:

b8ee41b: is it impossible to have valid bitcoin addresses which are just hex characters?

theStack commented at 12:43 pm on February 9, 2024:

Good point. For legacy addresses on mainnet, it’s very unlikely, but indeed theoretically possible to have a valid address that IsHex, i.e. consists of an even number of hex characters. One example for this is 111111111111111111126C31DfD9, found with the following Python script using our test framework:

 0#!/usr/bin/env python3
 1from test_framework.address import keyhash_to_p2pkh
 2
 3for i in range(100000):
 4    addr = keyhash_to_p2pkh(i.to_bytes(20, 'big'), main=True)
 5    try:
 6        bytes.fromhex(addr)
 7    except:
 8        continue
 9    print(f"mainnet legacy address in hex format found: {addr}")
10    break

If we only consider pubkey-hashes constructed from valid pubkeys (the example above is not), I’d assume that the chance to get a hex-address is negligible. Still, to be on the safe side and to avoid changing behaviour, I restored the length-check conditions after the IsHex call in addmultisigaddress in the first commit.

stratospher commented at 1:59 pm on February 9, 2024:

interesting script! sounds good to be on safer side.

nit: only if you have to retouch, you could update commit message in 100e8a75 to not mention addmultisigaddress

stratospher commented at 6:21 am on February 9, 2024: contributor

Concept ACK! liked how the error handling is done in a consitent manner now.

rpc: check and throw specific pubkey parsing errors in `HexToPubKey`

In the helper `HexToPubKey`, check for three different causes of legacy
public key parsing errors (in this order):

    - pubkey is not a hex string
    - pubkey doesn't have a valid length (33 or 65 bytes) [NEW]
    - pubkey is cryptographically invalid, i.e. not on curve
      (`IsFullyValid` check)

and throw a specific error message for each one. Note that the error
code is identical for all of them (-5), so this doesn't break RPC API
compatibility.

The helper is currently used for the RPCs `createmultisig` and
`addmultisigaddress`. The length checks can be removed from the
call-sites and error message checks in the functional tests are adapted.

100e8a75bf

rpc: use `HexToPubKey` helper for all legacy pubkey-parsing RPCs

This deduplicates code and leads to more consistent and detailed error
messages. Affected are legacy import RPCs (`importpubkey`,
`importmulti`) and other ones where solving data can be provided
(`fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendall`).

c740b154d1

test: add coverage for parsing cryptographically invalid pubkeys 98570fe29b

theStack force-pushed on Feb 9, 2024

stratospher commented at 1:59 pm on February 9, 2024: contributor

tested ACK 98570fe.

DrahtBot requested review from sipa on Feb 9, 2024

DrahtBot requested review from kevkevinpal on Feb 9, 2024

achow101 removed review request from kevkevinpal on Apr 9, 2024

achow101 requested review from achow101 on Apr 9, 2024

achow101 requested review from josibake on Apr 9, 2024

in src/rpc/util.cpp:183 in 100e8a75bf outdated

179@@ -180,11 +180,14 @@ std::string HelpExampleRpcNamed(const std::string& methodname, const RPCArgList&
180 CPubKey HexToPubKey(const std::string& hex_in)
181 {
182     if (!IsHex(hex_in)) {
183-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + hex_in);
184+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey \"" + hex_in + "\" must be a hex string");

achow101 commented at 5:50 pm on April 15, 2024:

In 100e8a75bf5d8196c005331bd8f2ed42ada6d8d0 “rpc: check and throw specific pubkey parsing errors in HexToPubKey”

nit: The error message reads a little bit weirdly to me. I think it would make more sense to use “is not” rather than “must be”, e.g. “Pubkey abcd is not a hex string”.

achow101 commented at 5:51 pm on April 15, 2024: member

ACK 98570fe29bb08d7edc48011aa6b9731c6ab4ed2e

DrahtBot requested review from kevkevinpal on Apr 15, 2024

in src/rpc/util.cpp:185 in 100e8a75bf outdated

179@@ -180,11 +180,14 @@ std::string HelpExampleRpcNamed(const std::string& methodname, const RPCArgList&
180 CPubKey HexToPubKey(const std::string& hex_in)
181 {
182     if (!IsHex(hex_in)) {
183-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + hex_in);
184+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey \"" + hex_in + "\" must be a hex string");
185+    }
186+    if (hex_in.length() != 66 && hex_in.length() != 130) {

davidgumberg commented at 11:34 pm on May 3, 2024:

Nit: IMO, the length check should happen before the IsHex check, since a string with an odd-numbered length of valid hex characters will fail with the slightly confusing error message that it “must be a hex string.” For example, 67 zeroes:

0$ bitcoin-cli importpubkey 0000000000000000000000000000000000000000000000000000000000000000000
1error code: -5
2error message:
3Pubkey "0000000000000000000000000000000000000000000000000000000000000000000" must be a hex string

in test/functional/rpc_rawtransaction.py:494 in 100e8a75bf outdated

490@@ -491,11 +491,11 @@ def raw_multisig_transaction_legacy_tests(self):
491         addr2Obj = self.nodes[2].getaddressinfo(addr2)
492 
493         # Tests for createmultisig and addmultisigaddress
494-        assert_raises_rpc_error(-5, "Invalid public key", self.nodes[0].createmultisig, 1, ["01020304"])
495+        assert_raises_rpc_error(-5, 'Pubkey "01020304" must have a length of either 33 or 65 bytes', self.nodes[0].createmultisig, 1, ["01020304"])

davidgumberg commented at 11:45 pm on May 3, 2024:

Nit-question: Should there be a test added here that checks for pubkeys that are not valid points as in https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e?

Or, since this PR deduplicates pubkey parsing, is this test of the length check redundant and worthy of being removed?

in test/functional/wallet_fundrawtransaction.py:1059 in c740b154d1 outdated

1054@@ -1055,8 +1055,8 @@ def test_external_inputs(self):
1055         assert_raises_rpc_error(-4, "Not solvable pre-selected input COutPoint(%s, %s)" % (ext_utxo["txid"][0:10], ext_utxo["vout"]), wallet.fundrawtransaction, raw_tx)
1056 
1057         # Error conditions
1058-        assert_raises_rpc_error(-5, "'not a pubkey' is not hex", wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["not a pubkey"]})
1059-        assert_raises_rpc_error(-5, "'01234567890a0b0c0d0e0f' is not a valid public key", wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["01234567890a0b0c0d0e0f"]})
1060+        assert_raises_rpc_error(-5, 'Pubkey "not a pubkey" must be a hex string', wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["not a pubkey"]})
1061+        assert_raises_rpc_error(-5, 'Pubkey "01234567890a0b0c0d0e0f" must have a length of either 33 or 65 bytes', wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["01234567890a0b0c0d0e0f"]})

davidgumberg commented at 11:45 pm on May 3, 2024:

Same nit-question as in https://github.com/bitcoin/bitcoin/pull/28336/commits/100e8a75bf5d8196c005331bd8f2ed42ada6d8d0

davidgumberg commented at 11:59 pm on May 3, 2024: contributor

ACK https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e The deduplication of pubkey parsing here is a big improvement, and granular pubkey parsing error messages make the rpc interface easier to use. As far as I could find, no instances of pubkey parsing in the rpc code were missed here.

I tested this PR manually by compiling with legacy wallet support and passing bad pubkeys with the importpubkey rpc.

Eunovo commented at 3:45 pm on May 4, 2024: none

Tested ACK https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e Verified that all checks are covered in tests

achow101 merged this on May 8, 2024

achow101 closed this on May 8, 2024

theStack deleted the branch on May 8, 2024

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

Code Coverage

Reviews

Conflicts