Skip to content
Draft
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
38a1be2
add another fixme for different pubkey size in electrum balance display
mariocynicys Apr 7, 2025
1477355
add more fixmes regarding p2pk pubkey size
mariocynicys Apr 7, 2025
7b180f2
add to_secp256k1_pubkey() to Public
mariocynicys Apr 8, 2025
a125a0a
display balances in p2pks corresponding to both compressed and uncomp…
mariocynicys Apr 9, 2025
296b904
sign p2pk inputs correctly
mariocynicys Apr 8, 2025
7366240
allow listing p2pk utxo with uncompressed pubkeys in scriptPub
mariocynicys Apr 9, 2025
4d5523c
add a fixme - to be amended
mariocynicys Apr 17, 2025
d3d4ab2
review(onur): more idiomatic indexing than `get(x).unwrap()`
mariocynicys Apr 17, 2025
f866969
no need for the ref - `.clone()` won't consume the value
mariocynicys Apr 17, 2025
4645f9b
review(dimxy): produce the compressed and uncompressed p2pk scripts f…
mariocynicys Apr 21, 2025
2d055ec
test withdraw/signing with 65-byte p2pk output present
mariocynicys Apr 21, 2025
e65a9f0
fix list_unspent p2pk script getter error message
mariocynicys Apr 21, 2025
4a1188e
add balance and list_unspent tests for uncompressed pubkey
mariocynicys Apr 22, 2025
1eaf3e8
support listing tx-history for p2pk outputs
mariocynicys Apr 22, 2025
c5c62d2
produce correct address hash for 65-byte p2pk addresses
mariocynicys Apr 22, 2025
93d0376
Merge remote-tracking branch 'origin/dev' into p2pk-with-uncompressed…
mariocynicys Apr 24, 2025
5e69799
review(dimxy/sami): avoid using internal error
mariocynicys May 16, 2025
c2ec918
merge with origin/dev
shamardy Jul 24, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions mm2src/coins/utxo/rpc_clients/electrum_rpc/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -934,9 +939,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() {
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();
Expand Down
7 changes: 1 addition & 6 deletions mm2src/coins/utxo_signer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -87,8 +82,8 @@ impl From<UtxoSignWithKeyPairError> 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),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
}
}
Expand Down
34 changes: 21 additions & 13 deletions mm2src/coins/utxo_signer/src/with_key_pair.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<keys::Error> for UtxoSignWithKeyPairError {
Expand All @@ -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
))),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not an InternalError more like UnspendableUTXO or UnsupportedScript

Copy link
Collaborator Author

@mariocynicys mariocynicys May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ummmm, none of these are error variants 🤔

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

ok, will return it back 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
})
.collect::<UtxoSignWithKeyPairResult<_>>()?;
Expand All @@ -77,20 +75,30 @@ pub fn p2pk_spend(
fork_id: u32,
) -> UtxoSignWithKeyPairResult<TransactionInput> {
let unsigned_input = get_input(signer, input_index)?;

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())),
];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't output_scripts_p2pk be used here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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 indexing since the array has exactly 2 elements.
script: possible_script_pubkeys[0].clone(),
prev_script: unsigned_input.prev_script.clone(),
});
}

let signature = calc_and_sign_sighash(
signer,
input_index,
&script,
&unsigned_input.prev_script,
key_pair,
signature_version,
SIGHASH_ALL,
Expand Down
9 changes: 8 additions & 1 deletion mm2src/mm2_bitcoin/keys/src/public.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -80,6 +80,13 @@ impl Public {
Public::Normal(_) => None,
}
}

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),
}
}
Comment on lines +96 to +101

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty sure new rustc will complain and suggest similar code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

impl ops::Deref for Public {
Expand Down
2 changes: 2 additions & 0 deletions mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading