From 38a1be26e0064b33cfd870cc5fef8a5da7c3c405 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Mon, 7 Apr 2025 15:27:33 +0200 Subject: [PATCH 01/16] add another fixme for different pubkey size in electrum balance display --- mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs b/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs index 84e39997e4..c6ca1e089e 100644 --- a/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs +++ b/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs @@ -935,6 +935,7 @@ impl UtxoRpcClientOps for ElectrumClient { // If the plain pubkey is available, fetch the balance found in P2PK output as well (if any). if let Some(pubkey) = address.pubkey() { + // FIXME: This is only a single version of the pubkey (either 33- or 65-bytes), you need to add the other version. let p2pk_output_script = output_script_p2pk(pubkey); hashes.push(hex::encode(electrum_script_hash(&p2pk_output_script))); } From 1477355d859b5e02678267c84fba772a54dc2f6f Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Mon, 7 Apr 2025 15:49:46 +0200 Subject: [PATCH 02/16] add more fixmes regarding p2pk pubkey size --- mm2src/coins/utxo_signer/src/with_key_pair.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mm2src/coins/utxo_signer/src/with_key_pair.rs b/mm2src/coins/utxo_signer/src/with_key_pair.rs index b62fa695f2..9422c86d9e 100644 --- a/mm2src/coins/utxo_signer/src/with_key_pair.rs +++ b/mm2src/coins/utxo_signer/src/with_key_pair.rs @@ -78,6 +78,8 @@ pub fn p2pk_spend( ) -> UtxoSignWithKeyPairResult { let unsigned_input = get_input(signer, input_index)?; + // FIXME: You should try the compresses and uncompressed versions of the public key and choose + // the one that matches unsigned_input.prev_script (if any). let script = Builder::build_p2pk(key_pair.public()); if script != unsigned_input.prev_script { return MmError::err(UtxoSignWithKeyPairError::MismatchScript { From 7b180f2a5d65cabb5c40bd95f86be7c9f52741ce Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Tue, 8 Apr 2025 19:24:51 +0200 Subject: [PATCH 03/16] add to_secp256k1_pubkey() to Public --- mm2src/mm2_bitcoin/keys/src/public.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/mm2src/mm2_bitcoin/keys/src/public.rs b/mm2src/mm2_bitcoin/keys/src/public.rs index cb801585d8..5e5620bdd0 100644 --- a/mm2src/mm2_bitcoin/keys/src/public.rs +++ b/mm2src/mm2_bitcoin/keys/src/public.rs @@ -3,7 +3,7 @@ use crypto::dhash160; use hash::{H160, H264, H520}; use hex::ToHex; use secp256k1::{recovery::{RecoverableSignature, RecoveryId}, - Message as SecpMessage, PublicKey, Signature as SecpSignature}; + Error as SecpError, Message as SecpMessage, PublicKey, Signature as SecpSignature}; use std::{fmt, ops}; use {CompactSignature, Error, Message, Signature}; @@ -80,6 +80,13 @@ impl Public { Public::Normal(_) => None, } } + + pub fn to_secp256k1_pubkey(&self) -> Result { + match self { + Public::Compressed(public) => PublicKey::from_slice(&**public), + Public::Normal(public) => PublicKey::from_slice(&**public), + } + } } impl ops::Deref for Public { From a125a0af3c2ef2fb92aca64030b3427049284fb4 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Wed, 9 Apr 2025 09:26:42 +0200 Subject: [PATCH 04/16] display balances in p2pks corresponding to both compressed and uncompressed 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) --- .../utxo/rpc_clients/electrum_rpc/client.rs | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs b/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs index c6ca1e089e..1ea25c4899 100644 --- a/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs +++ b/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs @@ -934,10 +934,24 @@ impl UtxoRpcClientOps for ElectrumClient { let mut hashes = vec![hex::encode(electrum_script_hash(&output_script))]; // If the plain pubkey is available, fetch the balance found in P2PK output as well (if any). + // The output script (scriptPubkey) of such utxos may have the full pubkey length of 65 bytes or the compressed + // version of it (33 bytes). Both are valid and accepted by the blockchain. if let Some(pubkey) = address.pubkey() { - // FIXME: This is only a single version of the pubkey (either 33- or 65-bytes), you need to add the other version. - let p2pk_output_script = output_script_p2pk(pubkey); - hashes.push(hex::encode(electrum_script_hash(&p2pk_output_script))); + let pubkey = try_f!(pubkey.to_secp256k1_pubkey().map_err(|err| { + JsonRpcError::new( + UtxoJsonRpcClientInfo::client_info(self), + rpc_req!(self, "blockchain.scripthash.get_balance").into(), + JsonRpcErrorType::Internal(err.to_string()), + ) + })); + let compressed_pubkey = keys::Public::Compressed(pubkey.serialize().into()); + hashes.push(hex::encode(electrum_script_hash(&output_script_p2pk( + &compressed_pubkey, + )))); + let uncompressed_pubkey = keys::Public::Normal(pubkey.serialize_uncompressed().into()); + hashes.push(hex::encode(electrum_script_hash(&output_script_p2pk( + &uncompressed_pubkey, + )))); } let this = self.clone(); From 296b904fc5977d8ad2215ac430abaeaed9ba8064 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Tue, 8 Apr 2025 19:59:33 +0200 Subject: [PATCH 05/16] sign p2pk inputs correctly allow both compressed and uncompressed pubkeys in scriptPubkey of the utxo being spent --- mm2src/coins/utxo_signer/src/lib.rs | 7 +--- mm2src/coins/utxo_signer/src/with_key_pair.rs | 36 +++++++++++-------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/mm2src/coins/utxo_signer/src/lib.rs b/mm2src/coins/utxo_signer/src/lib.rs index 8032e9acf7..bc8a87ece8 100644 --- a/mm2src/coins/utxo_signer/src/lib.rs +++ b/mm2src/coins/utxo_signer/src/lib.rs @@ -55,11 +55,6 @@ pub enum UtxoSignTxError { script: Script, prev_script: Script, }, - #[display( - fmt = "Can't spend the UTXO with script = '{}'. This script format isn't supported", - script - )] - UnspendableUTXO { script: Script }, #[display(fmt = "Transport error: {}", _0)] Transport(String), #[display(fmt = "Internal error: {}", _0)] @@ -87,8 +82,8 @@ impl From for UtxoSignTxError { // that are expected to be checked by [`sign_common::UtxoSignTxParamsBuilder::build`] already. // So if this error happens, it's our internal error. UtxoSignWithKeyPairError::InputIndexOutOfBound { .. } => UtxoSignTxError::Internal(error), - UtxoSignWithKeyPairError::UnspendableUTXO { script } => UtxoSignTxError::UnspendableUTXO { script }, UtxoSignWithKeyPairError::ErrorSigning(sign) => UtxoSignTxError::ErrorSigning(sign), + UtxoSignWithKeyPairError::InternalError(internal) => UtxoSignTxError::Internal(internal), } } } diff --git a/mm2src/coins/utxo_signer/src/with_key_pair.rs b/mm2src/coins/utxo_signer/src/with_key_pair.rs index 9422c86d9e..605a246f20 100644 --- a/mm2src/coins/utxo_signer/src/with_key_pair.rs +++ b/mm2src/coins/utxo_signer/src/with_key_pair.rs @@ -30,13 +30,10 @@ pub enum UtxoSignWithKeyPairError { }, #[display(fmt = "Input index '{}' is out of bound. Total length = {}", index, len)] InputIndexOutOfBound { len: usize, index: usize }, - #[display( - fmt = "Can't spend the UTXO with script = '{}'. This script format isn't supported", - script - )] - UnspendableUTXO { script: Script }, #[display(fmt = "Error signing using a private key")] ErrorSigning(keys::Error), + #[display(fmt = "{}", _0)] + InternalError(String), } impl From for UtxoSignWithKeyPairError { @@ -59,9 +56,10 @@ pub fn sign_tx( ScriptType::PubKeyHash => p2pkh_spend(&unsigned, i, key_pair, signature_version, fork_id), // Allow spending legacy P2PK utxos. ScriptType::PubKey => p2pk_spend(&unsigned, i, key_pair, signature_version, fork_id), - _ => MmError::err(UtxoSignWithKeyPairError::UnspendableUTXO { - script: input.prev_script.clone(), - }), + _ => MmError::err(UtxoSignWithKeyPairError::InternalError(format!( + "Can't spend the UTXO with script = '{}'. This script format isn't supported", + input.prev_script + ))), } }) .collect::>()?; @@ -77,14 +75,22 @@ pub fn p2pk_spend( fork_id: u32, ) -> UtxoSignWithKeyPairResult { let unsigned_input = get_input(signer, input_index)?; - - // FIXME: You should try the compresses and uncompressed versions of the public key and choose - // the one that matches unsigned_input.prev_script (if any). - let script = Builder::build_p2pk(key_pair.public()); - if script != unsigned_input.prev_script { + // P2PK UTXOs can have either compressed or uncompressed public keys in the scriptPubkey. + // We need to check that one of them matches the prev_script of the input being spent. + 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())), + ]; + // Check that one of the scriptPubkeys matches the prev_script of the input being spent. + if !possible_script_pubkeys.contains(&unsigned_input.prev_script) { return MmError::err(UtxoSignWithKeyPairError::MismatchScript { script_type: "P2PK".to_owned(), - script, + // Safe unwrapping since the array has exactly 2 elements. + script: possible_script_pubkeys.get(0).unwrap().clone(), prev_script: unsigned_input.prev_script.clone(), }); } @@ -92,7 +98,7 @@ pub fn p2pk_spend( let signature = calc_and_sign_sighash( signer, input_index, - &script, + &unsigned_input.prev_script, key_pair, signature_version, SIGHASH_ALL, From 736624020a41b1777355eb4b6058485ba1218a87 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Wed, 9 Apr 2025 08:58:36 +0200 Subject: [PATCH 06/16] allow listing p2pk utxo with uncompressed pubkeys in scriptPub --- mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs b/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs index 1ea25c4899..1cfc67d3ed 100644 --- a/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs +++ b/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs @@ -804,8 +804,13 @@ impl UtxoRpcClientOps for ElectrumClient { // If the plain pubkey is available, fetch the UTXOs found in P2PK outputs as well (if any). if let Some(pubkey) = address.pubkey() { - let p2pk_output_script = output_script_p2pk(pubkey); - output_scripts.push(p2pk_output_script); + let pubkey = try_f!(pubkey + .to_secp256k1_pubkey() + .map_err(|e| UtxoRpcError::Internal(format!("Couldn't get secp256k1 pubkey from public key: {}", e)))); + let compressed_pubkey = keys::Public::Compressed(pubkey.serialize().into()); + output_scripts.push(output_script_p2pk(&compressed_pubkey)); + let uncompressed_pubkey = keys::Public::Normal(pubkey.serialize_uncompressed().into()); + output_scripts.push(output_script_p2pk(&uncompressed_pubkey)); } let this = self.clone(); From 4d5523cfccb5b9ac020fc95ca358ef058e505f0b Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Thu, 17 Apr 2025 12:09:22 +0200 Subject: [PATCH 07/16] add a fixme - to be amended --- mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs b/mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs index 5e702371b1..8e66a38ab3 100644 --- a/mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs +++ b/mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs @@ -255,6 +255,8 @@ fn test_my_balance() { } // TODO: Add a p2pk spending test in the docker tests when electrum nodes are available (also try to invoke the utxo cache by spending in rapid succession). +// FIXME: This address has a legacy 33-byte pubkey utxo, add another 65-byte pubkey utxo to test that those appear as well. +// Also test `ElectrumClient::list_unspent` returns the said 2 utxos. #[test] fn test_p2pk_my_balance() { // PK of the P2PK balance: 03f8f8fa2062590ba9a0a7a86f937de22f540c015864aad35a2a9f6766de906265 From d3d4ab2e4d07e7f18b0f2e4b8e2350cd496eab71 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Thu, 17 Apr 2025 12:48:40 +0200 Subject: [PATCH 08/16] review(onur): more idiomatic indexing than `get(x).unwrap()` 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/coins/utxo_signer/src/with_key_pair.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mm2src/coins/utxo_signer/src/with_key_pair.rs b/mm2src/coins/utxo_signer/src/with_key_pair.rs index 605a246f20..91b8e59665 100644 --- a/mm2src/coins/utxo_signer/src/with_key_pair.rs +++ b/mm2src/coins/utxo_signer/src/with_key_pair.rs @@ -89,8 +89,8 @@ pub fn p2pk_spend( if !possible_script_pubkeys.contains(&unsigned_input.prev_script) { return MmError::err(UtxoSignWithKeyPairError::MismatchScript { script_type: "P2PK".to_owned(), - // Safe unwrapping since the array has exactly 2 elements. - script: possible_script_pubkeys.get(0).unwrap().clone(), + // Safe indexing since the array has exactly 2 elements. + script: (&possible_script_pubkeys[0]).clone(), prev_script: unsigned_input.prev_script.clone(), }); } From f86696955fa410eec26dc0e19bed169302ae84f8 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Thu, 17 Apr 2025 13:40:33 +0200 Subject: [PATCH 09/16] no need for the ref - `.clone()` won't consume the value --- mm2src/coins/utxo_signer/src/with_key_pair.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm2src/coins/utxo_signer/src/with_key_pair.rs b/mm2src/coins/utxo_signer/src/with_key_pair.rs index 91b8e59665..fe57148c8d 100644 --- a/mm2src/coins/utxo_signer/src/with_key_pair.rs +++ b/mm2src/coins/utxo_signer/src/with_key_pair.rs @@ -90,7 +90,7 @@ pub fn p2pk_spend( return MmError::err(UtxoSignWithKeyPairError::MismatchScript { script_type: "P2PK".to_owned(), // Safe indexing since the array has exactly 2 elements. - script: (&possible_script_pubkeys[0]).clone(), + script: possible_script_pubkeys[0].clone(), prev_script: unsigned_input.prev_script.clone(), }); } From 4645f9b63610b6e05f4555334bc3a5fdc6413122 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Mon, 21 Apr 2025 12:27:48 +0200 Subject: [PATCH 10/16] review(dimxy): produce the compressed and uncompressed p2pk scripts from a single function --- mm2src/coins/utxo.rs | 13 ++++++++-- .../utxo/rpc_clients/electrum_rpc/client.rs | 24 +++++-------------- mm2src/coins/utxo/utxo_tests.rs | 20 +++++++++------- 3 files changed, 28 insertions(+), 29 deletions(-) diff --git a/mm2src/coins/utxo.rs b/mm2src/coins/utxo.rs index 35e7281d4e..974e0425c3 100644 --- a/mm2src/coins/utxo.rs +++ b/mm2src/coins/utxo.rs @@ -1798,8 +1798,17 @@ pub fn output_script(address: &Address) -> Result { } } -/// Builds transaction output script for a legacy P2PK address -pub fn output_script_p2pk(pubkey: &Public) -> Script { Builder::build_p2pk(pubkey) } +/// Builds transaction output script for a legacy P2PK address. +/// +/// This actually builds two scripts, the first is for the P2PK constructed using a compressed 33-byte pubkey and the second for the +/// P2PK constructed using a 65-byte uncompressed pubkey. Both of these scripts are accepted by the blockchain and can contain funds. +pub fn output_scripts_p2pk(pubkey: &Public) -> Result<[Script; 2], keys::Error> { + let secp_pubkey = pubkey.to_secp256k1_pubkey()?; + Ok([ + Builder::build_p2pk(&Public::Compressed(secp_pubkey.serialize().into())), + Builder::build_p2pk(&Public::Normal(secp_pubkey.serialize_uncompressed().into())), + ]) +} pub fn address_by_conf_and_pubkey_str( coin: &str, diff --git a/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs b/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs index 1cfc67d3ed..d6a6be1ae7 100644 --- a/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs +++ b/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs @@ -10,7 +10,7 @@ use super::rpc_responses::*; use crate::utxo::rpc_clients::ConcurrentRequestMap; use crate::utxo::utxo_block_header_storage::BlockHeaderStorage; -use crate::utxo::{output_script, output_script_p2pk, GetBlockHeaderError, GetConfirmedTxError, GetTxHeightError, +use crate::utxo::{output_script, output_scripts_p2pk, GetBlockHeaderError, GetConfirmedTxError, GetTxHeightError, ScripthashNotification}; use crate::RpcTransportEventHandler; use crate::SharableRpcTransportEventHandler; @@ -804,13 +804,9 @@ impl UtxoRpcClientOps for ElectrumClient { // If the plain pubkey is available, fetch the UTXOs found in P2PK outputs as well (if any). if let Some(pubkey) = address.pubkey() { - let pubkey = try_f!(pubkey - .to_secp256k1_pubkey() + let p2pk_scripts = try_f!(output_scripts_p2pk(pubkey) .map_err(|e| UtxoRpcError::Internal(format!("Couldn't get secp256k1 pubkey from public key: {}", e)))); - let compressed_pubkey = keys::Public::Compressed(pubkey.serialize().into()); - output_scripts.push(output_script_p2pk(&compressed_pubkey)); - let uncompressed_pubkey = keys::Public::Normal(pubkey.serialize_uncompressed().into()); - output_scripts.push(output_script_p2pk(&uncompressed_pubkey)); + output_scripts.extend(p2pk_scripts); } let this = self.clone(); @@ -939,24 +935,16 @@ impl UtxoRpcClientOps for ElectrumClient { let mut hashes = vec![hex::encode(electrum_script_hash(&output_script))]; // If the plain pubkey is available, fetch the balance found in P2PK output as well (if any). - // The output script (scriptPubkey) of such utxos may have the full pubkey length of 65 bytes or the compressed - // version of it (33 bytes). Both are valid and accepted by the blockchain. if let Some(pubkey) = address.pubkey() { - let pubkey = try_f!(pubkey.to_secp256k1_pubkey().map_err(|err| { + let p2pk_scripts = try_f!(output_scripts_p2pk(pubkey).map_err(|err| { JsonRpcError::new( UtxoJsonRpcClientInfo::client_info(self), rpc_req!(self, "blockchain.scripthash.get_balance").into(), JsonRpcErrorType::Internal(err.to_string()), ) })); - let compressed_pubkey = keys::Public::Compressed(pubkey.serialize().into()); - hashes.push(hex::encode(electrum_script_hash(&output_script_p2pk( - &compressed_pubkey, - )))); - let uncompressed_pubkey = keys::Public::Normal(pubkey.serialize_uncompressed().into()); - hashes.push(hex::encode(electrum_script_hash(&output_script_p2pk( - &uncompressed_pubkey, - )))); + let p2pk_hashes = p2pk_scripts.iter().map(|s| hex::encode(electrum_script_hash(s))); + hashes.extend(p2pk_hashes); } let this = self.clone(); diff --git a/mm2src/coins/utxo/utxo_tests.rs b/mm2src/coins/utxo/utxo_tests.rs index afbe38fea6..970f7bb92d 100644 --- a/mm2src/coins/utxo/utxo_tests.rs +++ b/mm2src/coins/utxo/utxo_tests.rs @@ -3435,6 +3435,16 @@ fn test_withdraw_p2pk_balance() { UtxoStandardCoin::get_unspent_ordered_list.mock_safe(|coin, _| { let fut = async move { let cache = coin.as_ref().recently_spent_outpoints.lock().await; + let [compressed_p2pk_script, _uncompressed_p2pk_script] = output_scripts_p2pk( + &coin + .as_ref() + .derivation_method + .unwrap_single_addr() + .await + .pubkey() + .unwrap(), + ) + .unwrap(); let unspents = vec![UnspentInfo { outpoint: OutPoint { hash: 1.into(), @@ -3443,15 +3453,7 @@ fn test_withdraw_p2pk_balance() { value: 1000000000, height: Default::default(), // Use a p2pk output script for this UTXO - script: output_script_p2pk( - &coin - .as_ref() - .derivation_method - .unwrap_single_addr() - .await - .pubkey() - .unwrap(), - ), + script: compressed_p2pk_script, }]; Ok((unspents, cache)) }; From 2d055ecddcd066a6f4152d9f3aeb60f0f133fa15 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Mon, 21 Apr 2025 12:45:39 +0200 Subject: [PATCH 11/16] test withdraw/signing with 65-byte p2pk output present --- mm2src/coins/utxo/utxo_tests.rs | 39 +++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/mm2src/coins/utxo/utxo_tests.rs b/mm2src/coins/utxo/utxo_tests.rs index 970f7bb92d..ecaf748853 100644 --- a/mm2src/coins/utxo/utxo_tests.rs +++ b/mm2src/coins/utxo/utxo_tests.rs @@ -3435,7 +3435,7 @@ fn test_withdraw_p2pk_balance() { UtxoStandardCoin::get_unspent_ordered_list.mock_safe(|coin, _| { let fut = async move { let cache = coin.as_ref().recently_spent_outpoints.lock().await; - let [compressed_p2pk_script, _uncompressed_p2pk_script] = output_scripts_p2pk( + let [compressed_p2pk_script, uncompressed_p2pk_script] = output_scripts_p2pk( &coin .as_ref() .derivation_method @@ -3445,16 +3445,31 @@ fn test_withdraw_p2pk_balance() { .unwrap(), ) .unwrap(); - let unspents = vec![UnspentInfo { - outpoint: OutPoint { - hash: 1.into(), - index: 0, + let unspents = vec![ + UnspentInfo { + outpoint: OutPoint { + hash: 1.into(), + index: 0, + }, + // 1 BTC to spend. Next utxo has another 1 BTC. + value: 100000000 + + 1000 // To cover the fees. + + 5000, // To get back as change. + height: Default::default(), + // Use a p2pk output (using a 33-byte pubkey) script for this UTXO + script: compressed_p2pk_script, }, - value: 1000000000, - height: Default::default(), - // Use a p2pk output script for this UTXO - script: compressed_p2pk_script, - }]; + UnspentInfo { + outpoint: OutPoint { + hash: 1.into(), + index: 0, + }, + value: 100000000, + height: Default::default(), + // Use a p2pk output (using a 65-byte pubkey) script for this UTXO + script: uncompressed_p2pk_script, + }, + ]; Ok((unspents, cache)) }; MockResult::Return(fut.boxed()) @@ -3464,7 +3479,7 @@ fn test_withdraw_p2pk_balance() { let my_p2pkh_address = block_on(coin.as_ref().derivation_method.unwrap_single_addr()); let withdraw_req = WithdrawRequest { - amount: 1.into(), + amount: 2.into(), to: my_p2pkh_address.to_string(), coin: TEST_COIN_NAME.into(), ..Default::default() @@ -3478,7 +3493,7 @@ fn test_withdraw_p2pk_balance() { assert_eq!(output_script, expected_script); // And it should have this value (p2pk balance - amount sent - fees). - assert_eq!(transaction.outputs[1].value, 899999000); + assert_eq!(transaction.outputs[1].value, 5000); } /// `UtxoStandardCoin` has to check UTXO maturity if `check_utxo_maturity` is `true`. From e65a9f042bc097f0f11838ca507a52c37cc45344 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Mon, 21 Apr 2025 21:01:20 +0200 Subject: [PATCH 12/16] fix list_unspent p2pk script getter error message --- mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs b/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs index d6a6be1ae7..cbb91f24ab 100644 --- a/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs +++ b/mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs @@ -805,7 +805,7 @@ impl UtxoRpcClientOps for ElectrumClient { // If the plain pubkey is available, fetch the UTXOs found in P2PK outputs as well (if any). if let Some(pubkey) = address.pubkey() { let p2pk_scripts = try_f!(output_scripts_p2pk(pubkey) - .map_err(|e| UtxoRpcError::Internal(format!("Couldn't get secp256k1 pubkey from public key: {}", e)))); + .map_err(|e| UtxoRpcError::Internal(format!("Couldn't get p2pk output scripts: {}", e)))); output_scripts.extend(p2pk_scripts); } From 4a1188e350362c6a10efd584345815d02dec751d Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Tue, 22 Apr 2025 13:32:15 +0200 Subject: [PATCH 13/16] add balance and list_unspent tests for uncompressed pubkey --- mm2src/coins/utxo/utxo_tests.rs | 47 +++++++++++++++++++ .../tests/mm2_tests/mm2_tests_inner.rs | 11 +++-- 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/mm2src/coins/utxo/utxo_tests.rs b/mm2src/coins/utxo/utxo_tests.rs index ecaf748853..a3bd4c61c8 100644 --- a/mm2src/coins/utxo/utxo_tests.rs +++ b/mm2src/coins/utxo/utxo_tests.rs @@ -3496,6 +3496,53 @@ fn test_withdraw_p2pk_balance() { assert_eq!(transaction.outputs[1].value, 5000); } +#[test] +#[cfg(not(target_arch = "wasm32"))] +fn test_list_unspent_lists_p2pk_utxos() { + use mm2_test_helpers::{electrums::tbtc_electrums, for_tests::tbtc_conf}; + + let conf = tbtc_conf(); + let req = json!({ + "method": "electrum", + "servers": tbtc_electrums(), + }); + + let ctx = MmCtxBuilder::new().into_mm_arc(); + let params = UtxoActivationParams::from_legacy_req(&req).unwrap(); + + // This is the private key for two p2pk utxos (one created using comressed pubkey and the other using uncompressed pubkey). + // PK of the P2PK balance: 03f8f8fa2062590ba9a0a7a86f937de22f540c015864aad35a2a9f6766de906265 (owned 0.00076 tBTC - https://mempool.space/testnet/address/03f8f8fa2062590ba9a0a7a86f937de22f540c015864aad35a2a9f6766de906265) + // Uncompressed form: 04f8f8fa2062590ba9a0a7a86f937de22f540c015864aad35a2a9f6766de9062655d6a466270be272069ec95b7b83724a6ec24238395980ea7efd69fd9155d2a6d (owns 0.00003 tBTC - https://mempool.space/testnet/address/04f8f8fa2062590ba9a0a7a86f937de22f540c015864aad35a2a9f6766de9062655d6a466270be272069ec95b7b83724a6ec24238395980ea7efd69fd9155d2a6d) + let priv_key = Private::from_str("18GYrxNbvwoBi5Xwe4o92tDVoSf49ybs78BiQYDPKUkFp7qVn2wZ") + .unwrap() + .secret; + let coin = block_on(utxo_standard_coin_with_priv_key(&ctx, "tBTC", &conf, ¶ms, priv_key)).unwrap(); + + let my_address = block_on(coin.as_ref().derivation_method.single_addr_or_err()).unwrap(); + let (unspents, _recently_spent) = block_on_f01(coin.get_unspent_ordered_list(&my_address).compat()).unwrap(); + let expected_unspents = vec![ + UnspentInfo { + outpoint: OutPoint { + hash: H256::from_reversed_str("8085b7a738f711a00a44ba291265453f52b3af7b71586a3e5166612d2438d355"), + index: 0, + }, + value: 3000, + height: Some(4296371), + script: "4104f8f8fa2062590ba9a0a7a86f937de22f540c015864aad35a2a9f6766de9062655d6a466270be272069ec95b7b83724a6ec24238395980ea7efd69fd9155d2a6dac".into(), + }, + UnspentInfo { + outpoint: OutPoint { + hash: H256::from_reversed_str("0e440b23e8c12e8ba32b1ff0cdd7d0fa6a58f8b594f7813f073eee4834f15b4e"), + index: 0, + }, + value: 76000, + height: Some(2575577), + script: "2103f8f8fa2062590ba9a0a7a86f937de22f540c015864aad35a2a9f6766de906265ac".into(), + }, + ]; + assert_eq!(unspents, expected_unspents); +} + /// `UtxoStandardCoin` has to check UTXO maturity if `check_utxo_maturity` is `true`. /// https://github.com/KomodoPlatform/atomicDEX-API/issues/1181 #[test] diff --git a/mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs b/mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs index 8e66a38ab3..7db8c32472 100644 --- a/mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs +++ b/mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs @@ -255,11 +255,10 @@ fn test_my_balance() { } // TODO: Add a p2pk spending test in the docker tests when electrum nodes are available (also try to invoke the utxo cache by spending in rapid succession). -// FIXME: This address has a legacy 33-byte pubkey utxo, add another 65-byte pubkey utxo to test that those appear as well. -// Also test `ElectrumClient::list_unspent` returns the said 2 utxos. #[test] fn test_p2pk_my_balance() { - // PK of the P2PK balance: 03f8f8fa2062590ba9a0a7a86f937de22f540c015864aad35a2a9f6766de906265 + // PK of the P2PK balance: 03f8f8fa2062590ba9a0a7a86f937de22f540c015864aad35a2a9f6766de906265 (owned 0.00076 tBTC - https://mempool.space/testnet/address/03f8f8fa2062590ba9a0a7a86f937de22f540c015864aad35a2a9f6766de906265) + // Uncompressed form: 04f8f8fa2062590ba9a0a7a86f937de22f540c015864aad35a2a9f6766de9062655d6a466270be272069ec95b7b83724a6ec24238395980ea7efd69fd9155d2a6d (owns 0.00003 tBTC - https://mempool.space/testnet/address/04f8f8fa2062590ba9a0a7a86f937de22f540c015864aad35a2a9f6766de9062655d6a466270be272069ec95b7b83724a6ec24238395980ea7efd69fd9155d2a6d) let seed = "salmon angle cushion sauce accuse earth volume until zone youth emerge favorite"; let coins = json!([tbtc_conf()]); let conf = Mm2TestConf::seednode(seed, &coins); @@ -271,9 +270,11 @@ fn test_p2pk_my_balance() { block_on(enable_electrum(&mm, "tBTC", false, TBTC_ELECTRUMS)); let my_balance = block_on(my_balance(&mm, "tBTC")); - assert_eq!(my_balance.balance, "0.00076".parse().unwrap()); + // The total balance is the sum of the balances in the p2pkh, compressed-pubkey p2pk, uncompressed-pubkey p2pk addresses. + // In this case, we only have the compressed-pubkey p2pk (0.00076 tBTC) and uncompressed-pubkey p2pk (0.00003 tBTC) addresses. + assert_eq!(my_balance.balance, "0.00079".parse().unwrap()); assert_eq!(my_balance.unspendable_balance, BigDecimal::from(0)); - // Even though the address is a P2PK, it's formatted as P2PKH like most explorers do. + // Even though the addresses containing the coins are P2PK, it's formatted as P2PKH like most explorers do. assert_eq!(my_balance.address, "mgrM9w49Q7vqtroLKGekLTqCVFye5u6G3v"); } From 1eaf3e806abed6966ca265ed3fb4cfe28e4bccda Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Tue, 22 Apr 2025 16:51:38 +0200 Subject: [PATCH 14/16] support listing tx-history for p2pk outputs --- mm2src/coins/utxo/utxo_common.rs | 51 ++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/mm2src/coins/utxo/utxo_common.rs b/mm2src/coins/utxo/utxo_common.rs index db7ad1f65a..590ac662eb 100644 --- a/mm2src/coins/utxo/utxo_common.rs +++ b/mm2src/coins/utxo/utxo_common.rs @@ -3563,20 +3563,20 @@ pub async fn request_tx_history(coin: &T, metrics: MetricsArc) -> RequestTxHi where T: UtxoCommonOps + MmCoin + MarketCoinOps, { - let my_address = match coin.my_address() { - Ok(addr) => addr, - Err(e) => { - return RequestTxHistoryResult::CriticalError(ERRL!( - "Error on getting self address: {}. Stop tx history", - e - )) - }, - }; - let tx_ids = match &coin.as_ref().rpc_client { UtxoRpcClientEnum::Native(client) => { let mut from = 0; let mut all_transactions = vec![]; + let my_address = match coin.my_address() { + Ok(addr) => addr, + Err(e) => { + return RequestTxHistoryResult::CriticalError(ERRL!( + "Error on getting self address: {}. Stop tx history", + e + )) + }, + }; + loop { mm_counter!(metrics, "tx.history.request.count", 1, "coin" => coin.as_ref().conf.ticker.clone(), "client" => "native", "method" => "listtransactions"); @@ -3619,16 +3619,27 @@ where Ok(my_address) => my_address, Err(e) => return RequestTxHistoryResult::CriticalError(e.to_string()), }; - let script = match output_script(&my_address) { - Ok(script) => script, + let mut output_scripts = match output_script(&my_address) { + Ok(script) => vec![script], Err(err) => return RequestTxHistoryResult::CriticalError(err.to_string()), }; - let script_hash = electrum_script_hash(&script); - + // We support P2PK addresses for Iguana (i.e. non-HD Wallet) mode. + if coin.as_ref().derivation_method.hd_wallet().is_none() && !my_address.addr_format().is_segwit() { + if let Some(pubkey) = my_address.pubkey() { + let p2pk_scripts = match output_scripts_p2pk(pubkey) { + Ok(scripts) => scripts, + Err(e) => return RequestTxHistoryResult::CriticalError(e.to_string()), + }; + output_scripts.extend(p2pk_scripts); + } + } mm_counter!(metrics, "tx.history.request.count", 1, "coin" => coin.as_ref().conf.ticker.clone(), "client" => "electrum", "method" => "blockchain.scripthash.get_history"); - let electrum_history = match client.scripthash_get_history(&hex::encode(script_hash)).compat().await { + let script_hashes = output_scripts + .iter() + .map(|script| hex::encode(electrum_script_hash(script))); + let electrum_history = match client.scripthash_get_history_batch(script_hashes).compat().await { Ok(value) => value, Err(e) => match &e.error { JsonRpcErrorType::InvalidRequest(e) @@ -3656,15 +3667,11 @@ where mm_counter!(metrics, "tx.history.response.total_length", electrum_history.len() as u64, "coin" => coin.as_ref().conf.ticker.clone(), "client" => "electrum", "method" => "blockchain.scripthash.get_history"); - // electrum returns the most recent transactions in the end but we need to - // process them first so rev is required electrum_history .into_iter() - .rev() - .map(|item| { - let height = if item.height < 0 { 0 } else { item.height as u64 }; - (item.tx_hash, height) - }) + .flatten() + .map(|item| (item.tx_hash, item.height.max(0) as u64)) + .sorted_by(|(_, h1), (_, h2)| h2.cmp(h1)) .collect() }, }; From c5c62d2aff235648a8f0a7b2b83321d8d43f84ee Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Tue, 22 Apr 2025 17:39:45 +0200 Subject: [PATCH 15/16] produce correct address hash for 65-byte p2pk addresses 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) --- mm2src/mm2_bitcoin/keys/src/public.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/mm2src/mm2_bitcoin/keys/src/public.rs b/mm2src/mm2_bitcoin/keys/src/public.rs index 5e5620bdd0..6ec098faf2 100644 --- a/mm2src/mm2_bitcoin/keys/src/public.rs +++ b/mm2src/mm2_bitcoin/keys/src/public.rs @@ -37,7 +37,18 @@ impl Public { } } - pub fn address_hash(&self) -> H160 { dhash160(self) } + 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 verify(&self, message: &Message, signature: &Signature) -> Result { let public = match self { From 5e697997585e70a8bbc01dbc756d330f7b1e5f68 Mon Sep 17 00:00:00 2001 From: Omer Yacine Date: Fri, 16 May 2025 17:40:30 +0200 Subject: [PATCH 16/16] review(dimxy/sami): avoid using internal error --- mm2src/coins/utxo_signer/src/lib.rs | 10 +++++++- mm2src/coins/utxo_signer/src/with_key_pair.rs | 23 +++++++++++-------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/mm2src/coins/utxo_signer/src/lib.rs b/mm2src/coins/utxo_signer/src/lib.rs index bc8a87ece8..a2085e7a18 100644 --- a/mm2src/coins/utxo_signer/src/lib.rs +++ b/mm2src/coins/utxo_signer/src/lib.rs @@ -55,6 +55,13 @@ pub enum UtxoSignTxError { script: Script, prev_script: Script, }, + #[display( + fmt = "Can't spend the UTXO with script = '{}'. This script format isn't supported", + script + )] + UnspendableUTXO { script: Script }, + #[display(fmt = "Couldn't get secp256k1 pubkey from keypair: {}", error)] + BadPublicKey { error: String }, #[display(fmt = "Transport error: {}", _0)] Transport(String), #[display(fmt = "Internal error: {}", _0)] @@ -83,7 +90,8 @@ impl From for UtxoSignTxError { // So if this error happens, it's our internal error. UtxoSignWithKeyPairError::InputIndexOutOfBound { .. } => UtxoSignTxError::Internal(error), UtxoSignWithKeyPairError::ErrorSigning(sign) => UtxoSignTxError::ErrorSigning(sign), - UtxoSignWithKeyPairError::InternalError(internal) => UtxoSignTxError::Internal(internal), + UtxoSignWithKeyPairError::UnspendableUTXO { script } => UtxoSignTxError::UnspendableUTXO { script }, + UtxoSignWithKeyPairError::BadPublicKey { error } => UtxoSignTxError::BadPublicKey { error }, } } } diff --git a/mm2src/coins/utxo_signer/src/with_key_pair.rs b/mm2src/coins/utxo_signer/src/with_key_pair.rs index fe57148c8d..49795eca5d 100644 --- a/mm2src/coins/utxo_signer/src/with_key_pair.rs +++ b/mm2src/coins/utxo_signer/src/with_key_pair.rs @@ -30,10 +30,15 @@ pub enum UtxoSignWithKeyPairError { }, #[display(fmt = "Input index '{}' is out of bound. Total length = {}", index, len)] InputIndexOutOfBound { len: usize, index: usize }, + #[display( + fmt = "Can't spend the UTXO with script = '{}'. This script format isn't supported", + script + )] + UnspendableUTXO { script: Script }, + #[display(fmt = "Couldn't get secp256k1 pubkey from keypair: {}", error)] + BadPublicKey { error: String }, #[display(fmt = "Error signing using a private key")] ErrorSigning(keys::Error), - #[display(fmt = "{}", _0)] - InternalError(String), } impl From for UtxoSignWithKeyPairError { @@ -56,10 +61,9 @@ pub fn sign_tx( ScriptType::PubKeyHash => p2pkh_spend(&unsigned, i, key_pair, signature_version, fork_id), // Allow spending legacy P2PK utxos. ScriptType::PubKey => p2pk_spend(&unsigned, i, key_pair, signature_version, fork_id), - _ => MmError::err(UtxoSignWithKeyPairError::InternalError(format!( - "Can't spend the UTXO with script = '{}'. This script format isn't supported", - input.prev_script - ))), + _ => MmError::err(UtxoSignWithKeyPairError::UnspendableUTXO { + script: input.prev_script.clone(), + }), } }) .collect::>()?; @@ -77,9 +81,10 @@ pub fn p2pk_spend( let unsigned_input = get_input(signer, input_index)?; // P2PK UTXOs can have either compressed or uncompressed public keys in the scriptPubkey. // We need to check that one of them matches the prev_script of the input being spent. - let pubkey = key_pair.public().to_secp256k1_pubkey().map_err(|e| { - UtxoSignWithKeyPairError::InternalError(format!("Couldn't get secp256k1 pubkey from keypair: {}", e)) - })?; + let pubkey = key_pair + .public() + .to_secp256k1_pubkey() + .map_err(|e| UtxoSignWithKeyPairError::BadPublicKey { error: e.to_string() })?; // Build the scriptPubKey for both compressed and uncompressed public keys. let possible_script_pubkeys = vec![ Builder::build_p2pk(&keys::Public::Compressed(pubkey.serialize().into())),