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

theStack · 2023-08-24T15:14:14Z

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 · 2023-08-24T15:14:22Z

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.

sipa · 2023-08-24T15:18:49Z

Concept ACK

kevkevinpal

ACK 194b4e6

added one comment but feel free to ignore

kevkevinpal · 2023-08-30T03:01:06Z

src/rpc/util.cpp

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

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

yup makes sense and I agree!

theStack · 2024-01-22T17:06:58Z

Rebased on master (to fix CI).

stratospher

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

stratospher · 2024-02-09T05:08:02Z

src/wallet/rpc/addresses.cpp

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

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:

#!/usr/bin/env python3 from test_framework.address import keyhash_to_p2pkh for i in range(100000): addr = keyhash_to_p2pkh(i.to_bytes(20, 'big'), main=True) try: bytes.fromhex(addr) except: continue print(f"mainnet legacy address in hex format found: {addr}") 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.

interesting script! sounds good to be on safer side.

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

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.

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

stratospher · 2024-02-09T13:59:59Z

tested ACK 98570fe.

achow101

ACK 98570fe

achow101 · 2024-04-15T17:50:38Z

src/rpc/util.cpp

 {
    if (!IsHex(hex_in)) {
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + hex_in);
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey \"" + hex_in + "\" must be a hex string");


In 100e8a7 "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".

davidgumberg · 2024-05-03T23:34:37Z

src/rpc/util.cpp

-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + hex_in);
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey \"" + hex_in + "\" must be a hex string");
+    }
+    if (hex_in.length() != 66 && hex_in.length() != 130) {


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:

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

davidgumberg · 2024-05-03T23:45:18Z

test/functional/rpc_rawtransaction.py


        # Tests for createmultisig and addmultisigaddress
-        assert_raises_rpc_error(-5, "Invalid public key", self.nodes[0].createmultisig, 1, ["01020304"])
+        assert_raises_rpc_error(-5, 'Pubkey "01020304" must have a length of either 33 or 65 bytes', self.nodes[0].createmultisig, 1, ["01020304"])


Nit-question: Should there be a test added here that checks for pubkeys that are not valid points as in 98570fe?

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

davidgumberg · 2024-05-03T23:45:58Z

test/functional/wallet_fundrawtransaction.py

-        assert_raises_rpc_error(-5, "'not a pubkey' is not hex", wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["not a pubkey"]})
-        assert_raises_rpc_error(-5, "'01234567890a0b0c0d0e0f' is not a valid public key", wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["01234567890a0b0c0d0e0f"]})
+        assert_raises_rpc_error(-5, 'Pubkey "not a pubkey" must be a hex string', wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["not a pubkey"]})
+        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"]})


Same nit-question as in 100e8a7

davidgumberg · 2024-05-03T23:59:41Z

ACK 98570fe
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 · 2024-05-04T15:45:06Z

Tested ACK 98570fe
Verified that all checks are covered in tests

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

DrahtBot mentioned this pull request Aug 24, 2023

refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc #26840

Closed

kevkevinpal reviewed Aug 30, 2023

View reviewed changes

DrahtBot added CI failed and removed CI failed labels Sep 3, 2023

DrahtBot mentioned this pull request Oct 24, 2023

Remove the legacy wallet and BDB dependency #28710

Merged

DrahtBot added the CI failed label Jan 12, 2024

theStack force-pushed the 202308-rpc-consistent_legacy_pubkey_parsing_errors branch from 194b4e6 to 0f7a113 Compare January 22, 2024 12:41

DrahtBot removed the CI failed label Jan 22, 2024

rajarshimaitra mentioned this pull request Feb 8, 2024

Together we review series Bitshala/BitcoinCore-PR-Review-Club#49

Closed

stratospher reviewed Feb 9, 2024

View reviewed changes

theStack added 3 commits February 9, 2024 13:35

test: add coverage for parsing cryptographically invalid pubkeys

98570fe

theStack force-pushed the 202308-rpc-consistent_legacy_pubkey_parsing_errors branch from 0f7a113 to 98570fe Compare February 9, 2024 12:43

DrahtBot requested review from kevkevinpal and sipa February 9, 2024 14:00

achow101 requested review from achow101 and josibake and removed request for kevkevinpal April 9, 2024 15:46

achow101 reviewed Apr 15, 2024

View reviewed changes

DrahtBot requested a review from kevkevinpal April 15, 2024 17:51

davidgumberg reviewed May 3, 2024

View reviewed changes

achow101 merged commit 4ff4276 into bitcoin:master May 8, 2024

theStack deleted the 202308-rpc-consistent_legacy_pubkey_parsing_errors branch May 8, 2024 22:57

bitcoin locked and limited conversation to collaborators May 8, 2025

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

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

Uh oh!

Conversation

theStack commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

sipa commented Aug 24, 2023

Uh oh!

kevkevinpal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theStack commented Jan 22, 2024

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stratospher commented Feb 9, 2024

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidgumberg May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidgumberg commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eunovo commented May 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

theStack commented Aug 24, 2023 •

edited

Loading

DrahtBot commented Aug 24, 2023 •

edited

Loading

davidgumberg May 3, 2024 •

edited

Loading

davidgumberg commented May 3, 2024 •

edited

Loading