fix(p2pk): allow listing and spending p2pk utxos created with uncompressed pubkeys by mariocynicys · Pull Request #2410 · GLEECBTC/komodo-defi-framework

mariocynicys · 2025-04-09T07:09:59Z

A p2pk utxo could have the scriptPub created with either a compressed 33-byte pubkey or an uncompressed 65-byte pubkey.

We store pubkeys internally in compressed format, so when we query electrum for unspent utxos, we construct the output script using that compressed 33-byte pubkey. But a p2pk output script could very well be constructed using an uncompressed 65-byte pubkey (e.g.). Both are valid and accepted by the blockchain.

Also while spending, we need to construct the output script as it's used in the sig_hash calcuation. We used to always construct the p2pk output script as OP_PUSHBYTES_33 33-BYTE-PUBKEY OP_CHECKSIG but now we also construct the OP_PUSHBYTES_65 65-BYTE-PUBKEY OP_CHECKSIG as well, and check which of them matches the previous script and that's what's used to sign the transaction.

p.s. lingo might be confusing, output_script = scriptPub = scriptPubkey = locking_script

…ressed pubkeys previously we did only one of them depending on the stored Public object (which apparently always seem to be compressed based on how we initialize it)

allow both compressed and uncompressed pubkeys in scriptPubkey of the utxo being spent

onur-ozkan

Would you mind to add some test coverage on this logic if possible?

mm2src/coins/utxo_signer/src/with_key_pair.rs

mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs

there seem to be no way to consume the vec to only take one value out of it. this way looks the cleanst. indexing the first element by ref and cloning it.

mm2src/mm2_bitcoin/keys/src/public.rs

borngraced

lgtm 🚀

mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs

…rom a single function

mariocynicys · 2025-04-22T11:35:06Z

@onur-ozkan added some tests for balance and list unspents that interact with the uncompressed pubkey.

the address hash should dhash160 the compressed pubkey and not the uncompressed one. without this, 65-byte p2pk outputs show incorrect addresses in tx_history (address for a p2pk is just the p2pkh of the same pubkey, to get the hash, one should hash the compressed pubkey rather than the full 65-byte uncompressed one)

…-pubkey

dimxy · 2025-04-24T08:10:53Z

mm2src/coins/utxo_signer/src/lib.rs

            UtxoSignWithKeyPairError::InputIndexOutOfBound { .. } => UtxoSignTxError::Internal(error),
-            UtxoSignWithKeyPairError::UnspendableUTXO { script } => UtxoSignTxError::UnspendableUTXO { script },
            UtxoSignWithKeyPairError::ErrorSigning(sign) => UtxoSignTxError::ErrorSigning(sign),
+            UtxoSignWithKeyPairError::InternalError(internal) => UtxoSignTxError::Internal(internal),


I think as a general approach we should try to avoid using such general and unspecific errors like InternalError, with attached long string messages (which bloats code and are harder to document), in favour of specific error valiants which can be converted into documented top-level RPC errors.

agree. the prompt for me to compress the unspendableutxo into internal error though was the fact that it is only spitted out once in a specific place. and with the addition of pubkey conversion, another error would be spitted out once in another specific place. maybe 2 specific errors are OK, but i feel if we keep adding such small errors variants that aren't generic and used in only one or two places, we will end up with large error structs, and subsequently large error conversion code (which will compress everything to internal error at the later stages :/, as they are too specific to have their own error variants on user-facing/RPC level - though we could have error variants containing other errors, that's more clean and information preserving).

borngraced

some notes

borngraced · 2025-05-15T08:47:30Z

mm2src/coins/utxo_signer/src/with_key_pair.rs

+                _ => MmError::err(UtxoSignWithKeyPairError::InternalError(format!(
+                    "Can't spend the UTXO with script = '{}'. This script format isn't supported",
+                    input.prev_script
+                ))),


not an InternalError more like UnspendableUTXO or UnsupportedScript

~~ummmm, none of these are error variants 🤔~~

oh i guess u meant it was removed like this #2410 (comment)

ok, will return it back 👍

borngraced · 2025-05-15T08:48:37Z

mm2src/coins/utxo_signer/src/with_key_pair.rs

+    let pubkey = key_pair.public().to_secp256k1_pubkey().map_err(|e| {
+        UtxoSignWithKeyPairError::InternalError(format!("Couldn't get secp256k1 pubkey from keypair: {}", e))
+    })?;
+    // Build the scriptPubKey for both compressed and uncompressed public keys.
+    let possible_script_pubkeys = vec![
+        Builder::build_p2pk(&keys::Public::Compressed(pubkey.serialize().into())),
+        Builder::build_p2pk(&keys::Public::Normal(pubkey.serialize_uncompressed().into())),
+    ];


why can't output_scripts_p2pk be used here?

because it's in another crate the depends on this one. but i prefer the verbosity here anyway tbh.

borngraced · 2025-05-15T09:00:47Z

mm2src/mm2_bitcoin/keys/src/public.rs

+    pub fn to_secp256k1_pubkey(&self) -> Result<PublicKey, SecpError> {
+        match self {
+            Public::Compressed(public) => PublicKey::from_slice(&**public),
+            Public::Normal(public) => PublicKey::from_slice(&**public),
+        }
+    }


Suggested change

pub fn to_secp256k1_pubkey(&self) -> Result<PublicKey, SecpError> {

match self {

Public::Compressed(public) => PublicKey::from_slice(&**public),

Public::Normal(public) => PublicKey::from_slice(&**public),

}

}

pub fn to_secp256k1_pubkey(&self) -> Result<PublicKey, SecpError> { PublicKey::from_slice(self) }

pretty sure new rustc will complain and suggest similar code

#2410 (comment)

we can let this throw for now and the other PR will fix it. but i can fix it here if u want too.

borngraced · 2025-05-15T09:01:23Z

mm2src/mm2_bitcoin/keys/src/public.rs

+    pub fn address_hash(&self) -> H160 {
+        match self {
+            Public::Compressed(public) => dhash160(public.as_slice()),
+            // If the public key isn't compressed, we wanna compress it then get the hash.
+            // No body uses the uncompressed form to get an address hash.
+            Public::Normal(public) => match PublicKey::from_slice(public.as_slice()) {
+                Ok(public) => dhash160(&public.serialize()),
+                // This should never happen, as then the public key would be invalid. If so, return a dummy value.
+                Err(_) => H160::default(),
+            },
+        }
+    }


Suggested change

pub fn address_hash(&self) -> H160 {

match self {

Public::Compressed(public) => dhash160(public.as_slice()),

// If the public key isn't compressed, we wanna compress it then get the hash.

// No body uses the uncompressed form to get an address hash.

Public::Normal(public) => match PublicKey::from_slice(public.as_slice()) {

Ok(public) => dhash160(&public.serialize()),

// This should never happen, as then the public key would be invalid. If so, return a dummy value.

Err(_) => H160::default(),

},

}

}

pub fn address_hash(&self) -> H160 {

match self {

Public::Compressed(public) => dhash160(public.as_slice()),

// If the public key isn't compressed, we wanna compress it then get the hash.

// No body uses the uncompressed form to get an address hash.

Public::Normal(public) => PublicKey::from_slice(public.as_slice())

.map(|public| dhash160(&public.serialize()))

.unwrap_or_default(),

}

}

borngraced · 2025-05-15T09:01:46Z

mm2src/mm2_bitcoin/keys/src/public.rs

+            // If the public key isn't compressed, we wanna compress it then get the hash.
+            // No body uses the uncompressed form to get an address hash.


It's better to have this as doc comment i guess.

i think it's gonna be confusing, esp that this is a very legacy edge case that doesn't happen in day to day.

onur-ozkan · 2025-05-27T08:26:24Z

mm2src/mm2_bitcoin/keys/src/public.rs

+                // This should never happen, as then the public key would be invalid. If so, return a dummy value.
+                Err(_) => H160::default(),


This looks fishy. If this should never happen then put unreachable!() here. Let's say it did happen, with the current approach we will most likely miss it. If it never happens, then unreachable will not be harmful anyway.

it indeed looks fishy 😂

i just prefer errors over panics, but this one is a utility func that's used everywhere and hard to be made fallible.
returning such invalid/dummy data tho will screw whatever operation being done down the call stack.

there is only this place https://github.com/KomodoPlatform/komodo-defi-framework/blob/5e697997585e70a8bbc01dbc756d330f7b1e5f68/mm2src/mm2_bitcoin/script/src/script.rs#L415
where a public key is provided not by us, but from the blockchain (or a lying rpc server), so it's not completely safe to panic given that there is an exterior entity that could provide that pubkey. otherwise, all pubkeys delt with within mm2 should be always valid.

On KDF I do prefer errors over panics as well, but the comment says this should never happen, so either the comment is wrong or the code needs to change.

I would create 2 functions instead of trying to handle infallible/fallible calls from a single function: address_hash_unchecked that panics and address_hash with Result (or address_hash that panics try_address_hash with Result.).

seams reasonable 👍

mariocynicys added 2 commits April 9, 2025 08:42

add another fixme for different pubkey size in electrum balance display

38a1be2

add more fixmes regarding p2pk pubkey size

1477355

mariocynicys added status: pending review improvement: wallet priority: medium Moderately important tasks that should be completed but are not urgent. labels Apr 9, 2025

mariocynicys added 4 commits April 9, 2025 09:23

add to_secp256k1_pubkey() to Public

7b180f2

display balances in p2pks corresponding to both compressed and uncomp…

a125a0a

…ressed pubkeys previously we did only one of them depending on the stored Public object (which apparently always seem to be compressed based on how we initialize it)

sign p2pk inputs correctly

296b904

allow both compressed and uncompressed pubkeys in scriptPubkey of the utxo being spent

allow listing p2pk utxo with uncompressed pubkeys in scriptPub

7366240

mariocynicys force-pushed the p2pk-with-uncompressed-pubkey branch from 5e309e2 to 7366240 Compare April 9, 2025 07:27

mariocynicys mentioned this pull request Apr 9, 2025

fix(p2pk): validate expected pubkey correctly for p2pk inputs #2408

Merged

shamardy requested a review from dimxy April 14, 2025 23:03

shamardy added the 2.5.0-beta label Apr 14, 2025

onur-ozkan reviewed Apr 17, 2025

View reviewed changes

mm2src/coins/utxo_signer/src/with_key_pair.rs Outdated Show resolved Hide resolved

mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs Outdated Show resolved Hide resolved

mariocynicys added 2 commits April 17, 2025 12:09

add a fixme - to be amended

4d5523c

review(onur): more idiomatic indexing than get(x).unwrap()

d3d4ab2

there seem to be no way to consume the vec to only take one value out of it. this way looks the cleanst. indexing the first element by ref and cloning it.

borngraced reviewed Apr 17, 2025

View reviewed changes

mm2src/mm2_bitcoin/keys/src/public.rs Show resolved Hide resolved

no need for the ref - .clone() won't consume the value

f866969

borngraced previously approved these changes Apr 17, 2025

View reviewed changes

dimxy reviewed Apr 18, 2025

View reviewed changes

mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs Outdated Show resolved Hide resolved

review(dimxy): produce the compressed and uncompressed p2pk scripts f…

4645f9b

…rom a single function

mariocynicys dismissed borngraced’s stale review via 4645f9b April 21, 2025 10:27

mariocynicys added 3 commits April 21, 2025 12:45

test withdraw/signing with 65-byte p2pk output present

2d055ec

fix list_unspent p2pk script getter error message

e65a9f0

add balance and list_unspent tests for uncompressed pubkey

4a1188e

mariocynicys added 3 commits April 22, 2025 16:51

support listing tx-history for p2pk outputs

1eaf3e8

Merge remote-tracking branch 'origin/dev' into p2pk-with-uncompressed…

93d0376

…-pubkey

dimxy reviewed Apr 24, 2025

View reviewed changes

mariocynicys requested review from borngraced, dimxy, onur-ozkan and shamardy May 13, 2025 09:02

borngraced suggested changes May 15, 2025

View reviewed changes

review(dimxy/sami): avoid using internal error

5e69799

onur-ozkan reviewed May 27, 2025

View reviewed changes

shamardy marked this pull request as draft June 11, 2025 03:02

shamardy removed the 2.5.0-beta label Jun 16, 2025

merge with origin/dev

c2ec918

shamardy removed the request for review from dimxy January 3, 2026 20:52

		// If the public key isn't compressed, we wanna compress it then get the hash.
		// No body uses the uncompressed form to get an address hash.

		// This should never happen, as then the public key would be invalid. If so, return a dummy value.
		Err(_) => H160::default(),

Conversation

mariocynicys commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onur-ozkan left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

borngraced left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mariocynicys commented Apr 22, 2025

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!

borngraced left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariocynicys May 16, 2025 • 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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mariocynicys commented Apr 9, 2025 •

edited

Loading

onur-ozkan left a comment •

edited

Loading

mariocynicys May 16, 2025 •

edited

Loading