Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add uncompressed public key recover #2003

Merged
merged 2 commits into from
Nov 15, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions radix-common/src/crypto/signature_validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,28 @@ pub fn verify_and_recover_secp256k1(
None
}

#[cfg(feature = "secp256k1_sign_and_validate")]
pub fn verify_and_recover_secp256k1_uncompressed(
signed_hash: &Hash,
signature: &Secp256k1Signature,
) -> Option<[u8; secp256k1::constants::UNCOMPRESSED_PUBLIC_KEY_SIZE]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth us adding an explicit new type for this, e.g. ExpandedSecp256k1PublicKey - which we can use here and in scrypto? (And can be a semantic transparent wrapper around a pub [u8; 65])

Copy link
Contributor

Choose a reason for hiding this comment

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

And maybe adding to the Rust doc for Secp256k1PublicKey that it's the compressed public key, which is the default format used in the Radix stack?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add the wrapper.

let recovery_id = signature.0[0];
let signature_data = &signature.0[1..];
if let Ok(id) = ::secp256k1::ecdsa::RecoveryId::from_i32(recovery_id.into()) {
if let Ok(sig) = ::secp256k1::ecdsa::RecoverableSignature::from_compact(signature_data, id)
{
let msg = ::secp256k1::Message::from_digest_slice(&signed_hash.0)
.expect("Hash is always a valid message");

// The recover method also verifies the signature as part of the recovery process
if let Ok(pk) = SECP256K1_CTX.recover_ecdsa(&msg, &sig) {
return Some(pk.serialize_uncompressed());
}
}
}
None
}

#[cfg(feature = "secp256k1_sign_and_validate")]
pub fn verify_secp256k1(
signed_hash: &Hash,
Expand Down
1 change: 1 addition & 0 deletions radix-engine-tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ paste = { workspace = true }
hex = { workspace = true }
trybuild = { workspace = true }
extend = { workspace = true }
secp256k1 = { workspace = true }

[build-dependencies]
walkdir = { workspace = true, optional = true }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,5 +68,12 @@ mod component_module {
) -> Secp256k1PublicKey {
CryptoUtils::secp256k1_ecdsa_verify_and_key_recover(&hash, &signature)
}

pub fn secp256k1_ecdsa_verify_and_key_recover_uncompressed(
hash: Hash,
signature: Secp256k1Signature,
) -> [u8; 65] {
CryptoUtils::secp256k1_ecdsa_verify_and_key_recover_uncompressed(&hash, &signature)
}
}
}
28 changes: 25 additions & 3 deletions radix-engine-tests/tests/system/crypto_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -576,14 +576,19 @@ fn crypto_scrypto_secp256k1_ecdsa_verify_and_key_recover(
package_address: PackageAddress,
hash: Hash,
signature: Secp256k1Signature,
compressed: bool,
) -> TransactionReceipt {
runner.execute_manifest(
ManifestBuilder::new()
.lock_fee(runner.faucet_component(), 500u32)
.call_function(
package_address,
"CryptoScrypto",
"secp256k1_ecdsa_verify_and_key_recover",
if compressed {
"secp256k1_ecdsa_verify_and_key_recover"
} else {
"secp256k1_ecdsa_verify_and_key_recover_uncompressed"
},
manifest_args!(hash, signature),
)
.build(),
Expand Down Expand Up @@ -688,16 +693,32 @@ fn test_crypto_scrypto_key_recover_secp256k1_ecdsa() {
let hash1_signature = Secp256k1Signature::from_str(hash1_signature).unwrap();

// Act
let pk_recovered: Secp256k1PublicKey =
let pk_recovered1: Secp256k1PublicKey =
get_output!(crypto_scrypto_secp256k1_ecdsa_verify_and_key_recover(
&mut ledger,
package_address,
hash1,
hash1_signature,
true
));
let pk_recovered2: Vec<u8> =
get_output!(crypto_scrypto_secp256k1_ecdsa_verify_and_key_recover(
&mut ledger,
package_address,
hash1,
hash1_signature,
false
));

// Assert
assert_eq!(pk, pk_recovered);
assert_eq!(pk, pk_recovered1);
assert_eq!(
secp256k1::PublicKey::from_slice(pk.as_ref())
.unwrap()
.serialize_uncompressed()
.to_vec(),
pk_recovered2
);

// Test for key recovery error
let invalid_signature = "01cd8dcd5bb841430dd0a6f45565a1b8bdb4a204eb868832cd006f963a89a662813ab844a542fcdbfda4086a83fbbde516214113051b9c8e42a206c98d564d7122";
Expand All @@ -708,6 +729,7 @@ fn test_crypto_scrypto_key_recover_secp256k1_ecdsa() {
package_address,
hash1,
invalid_signature,
true
));

// Assert
Expand Down
2 changes: 2 additions & 0 deletions radix-engine/src/vm/wasm/constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ pub const CRYPTO_UTILS_SECP256K1_ECDSA_VERIFY_FUNCTION_NAME: &str =
"crypto_utils_secp256k1_ecdsa_verify";
pub const CRYPTO_UTILS_SECP256K1_ECDSA_VERIFY_AND_KEY_RECOVER_FUNCTION_NAME: &str =
"crypto_utils_secp256k1_ecdsa_verify_and_key_recover";
pub const CRYPTO_UTILS_SECP256K1_ECDSA_VERIFY_AND_KEY_RECOVER_UNCOMPRESSED_FUNCTION_NAME: &str =
"crypto_utils_secp256k1_ecdsa_verify_and_key_recover_uncompressed";

//=================
// WASM Shim
Expand Down
27 changes: 27 additions & 0 deletions radix-engine/src/vm/wasm/prepare.rs
Original file line number Diff line number Diff line change
Expand Up @@ -965,6 +965,32 @@ impl WasmModule {
));
}
}
CRYPTO_UTILS_SECP256K1_ECDSA_VERIFY_AND_KEY_RECOVER_UNCOMPRESSED_FUNCTION_NAME =>
{
if version < ScryptoVmVersion::crypto_utils_v2() {
return Err(PrepareError::InvalidImport(
InvalidImport::ProtocolVersionMismatch {
name: entry.name.to_string(),
current_version: version.into(),
expected_version: ScryptoVmVersion::crypto_utils_v2().into(),
},
));
}

if let TypeRef::Func(type_index) = entry.ty {
if Self::function_type_matches(
&self.module,
type_index,
vec![ValType::I32, ValType::I32, ValType::I32, ValType::I32],
vec![ValType::I64],
) {
continue;
}
return Err(PrepareError::InvalidImport(
InvalidImport::InvalidFunctionType(entry.name.to_string()),
));
}
}
// Crypto Utils v2 end
_ => {}
};
Expand Down Expand Up @@ -1553,6 +1579,7 @@ mod tests {
CRYPTO_UTILS_ED25519_VERIFY_FUNCTION_NAME,
CRYPTO_UTILS_SECP256K1_ECDSA_VERIFY_FUNCTION_NAME,
CRYPTO_UTILS_SECP256K1_ECDSA_VERIFY_AND_KEY_RECOVER_FUNCTION_NAME,
CRYPTO_UTILS_SECP256K1_ECDSA_VERIFY_AND_KEY_RECOVER_UNCOMPRESSED_FUNCTION_NAME,
],
),
] {
Expand Down
6 changes: 6 additions & 0 deletions radix-engine/src/vm/wasm/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,12 @@ pub trait WasmRuntime {
message: Vec<u8>,
signature: Vec<u8>,
) -> Result<Buffer, InvokeError<WasmRuntimeError>>;

fn crypto_utils_secp256k1_ecdsa_verify_and_key_recover_uncompressed(
&mut self,
message: Vec<u8>,
signature: Vec<u8>,
) -> Result<Buffer, InvokeError<WasmRuntimeError>>;
}

/// Represents an instantiated, invocable Scrypto module.
Expand Down
46 changes: 46 additions & 0 deletions radix-engine/src/vm/wasm/wasmi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,29 @@ fn secp256k1_ecdsa_verify_and_key_recover(
.map(|buffer| buffer.0)
}

fn secp256k1_ecdsa_verify_and_key_recover_uncompressed(
mut caller: Caller<'_, HostState>,
message_ptr: u32,
message_len: u32,
signature_ptr: u32,
signature_len: u32,
) -> Result<u64, InvokeError<WasmRuntimeError>> {
let runtime = grab_runtime!(caller);
let memory = grab_memory!(caller);

let message = read_memory(caller.as_context_mut(), memory, message_ptr, message_len)?;
let signature = read_memory(
caller.as_context_mut(),
memory,
signature_ptr,
signature_len,
)?;

runtime
.crypto_utils_secp256k1_ecdsa_verify_and_key_recover_uncompressed(message, signature)
.map(|buffer| buffer.0)
Copy link

@CyonAlexRDX CyonAlexRDX Nov 15, 2024

Choose a reason for hiding this comment

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

what does this method do? returns u64 and not a public key? why do we call buffer.0? should we not return the full buffer and update the return type to be Result<[u8: 64], _>? (or 65, depending on representation)

Copy link
Member Author

Choose a reason for hiding this comment

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

We can not pass the array raw to wasm as a function return, and have to wrap it into a buffer (which will be read by wasm later).

The wasmi linker requires a u32/u64 return type, so we had to unwrap the Buffer.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the Engine side of the Engine <=> WASM boundary. It effectively returns a pointer to the buffer in WASM's memory space

Choose a reason for hiding this comment

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

so the caller of this function reads out the 65 bytes how? via mut caller? I lack the WASM knowledge to understand this :D but just looked wrong. I assume it is some WASM vodoo magic making this work...

Choose a reason for hiding this comment

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

wrote simultaneously... thx makes sense

}

#[cfg(feature = "radix_engine_tests")]
fn test_host_read_memory(
mut caller: Caller<'_, HostState>,
Expand Down Expand Up @@ -1623,6 +1646,24 @@ impl WasmiModule {
.map_err(|e| e.into())
},
);
let host_secp2561k1_ecdsa_verify_and_key_recover_uncompressed = Func::wrap(
store.as_context_mut(),
|caller: Caller<'_, HostState>,
message_ptr: u32,
message_len: u32,
signature_ptr: u32,
signature_len: u32|
-> Result<u64, Trap> {
secp256k1_ecdsa_verify_and_key_recover_uncompressed(
caller,
message_ptr,
message_len,
signature_ptr,
signature_len,
)
.map_err(|e| e.into())
},
);

let mut linker = <Linker<HostState>>::new();

Expand Down Expand Up @@ -1826,6 +1867,11 @@ impl WasmiModule {
CRYPTO_UTILS_SECP256K1_ECDSA_VERIFY_AND_KEY_RECOVER_FUNCTION_NAME,
host_secp2561k1_ecdsa_verify_and_key_recover
);
linker_define!(
linker,
CRYPTO_UTILS_SECP256K1_ECDSA_VERIFY_AND_KEY_RECOVER_UNCOMPRESSED_FUNCTION_NAME,
host_secp2561k1_ecdsa_verify_and_key_recover_uncompressed
);

#[cfg(feature = "radix_engine_tests")]
{
Expand Down
8 changes: 8 additions & 0 deletions radix-engine/src/vm/wasm_runtime/no_op_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,4 +378,12 @@ impl<'a> WasmRuntime for NoOpWasmRuntime<'a> {
) -> Result<Buffer, InvokeError<WasmRuntimeError>> {
Err(InvokeError::SelfError(WasmRuntimeError::NotImplemented))
}

fn crypto_utils_secp256k1_ecdsa_verify_and_key_recover_uncompressed(
&mut self,
message: Vec<u8>,
signature: Vec<u8>,
) -> Result<Buffer, InvokeError<WasmRuntimeError>> {
Err(InvokeError::SelfError(WasmRuntimeError::NotImplemented))
}
}
21 changes: 21 additions & 0 deletions radix-engine/src/vm/wasm_runtime/scrypto_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -794,4 +794,25 @@ impl<'y, Y: SystemApi<RuntimeError>> WasmRuntime for ScryptoRuntime<'y, Y> {

self.allocate_buffer(key.to_vec())
}

/// This method is only available to packages uploaded after "Cuttlefish"
/// protocol update due to checks in [`ScryptoV1WasmValidator::validate`].
#[trace_resources]
fn crypto_utils_secp256k1_ecdsa_verify_and_key_recover_uncompressed(
&mut self,
message: Vec<u8>,
signature: Vec<u8>,
) -> Result<Buffer, InvokeError<WasmRuntimeError>> {
let hash = Hash::try_from(message.as_slice()).map_err(WasmRuntimeError::InvalidHash)?;
let signature = Secp256k1Signature::try_from(signature.as_ref())
.map_err(WasmRuntimeError::InvalidSecp256k1Signature)?;

self.api
.consume_cost_units(ClientCostingEntry::Secp256k1EcdsaKeyRecover)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this Secp256k1EcdsaKeyRecover variant used here deliberately?
I guess the costs for uncompressed variant are similar, so this probably makes sense.

Copy link
Member Author

@iamyulong iamyulong Nov 15, 2024

Choose a reason for hiding this comment

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

I noticed this too. The uncompressed version should be cheaper, but didn't bother changing it for now.


let key = verify_and_recover_secp256k1_uncompressed(&hash, &signature)
.ok_or(WasmRuntimeError::Secp256k1KeyRecoveryError)?;

self.allocate_buffer(key.to_vec())
}
}
15 changes: 15 additions & 0 deletions scrypto/src/crypto_utils/crypto_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,4 +156,19 @@ impl CryptoUtils {
});
Secp256k1PublicKey(key.try_into().unwrap())
}

pub fn secp256k1_ecdsa_verify_and_key_recover_uncompressed(
message_hash: impl AsRef<Hash>,
signature: impl AsRef<Secp256k1Signature>,
) -> [u8; 65] {
let key = copy_buffer(unsafe {
crypto_utils::crypto_utils_secp256k1_ecdsa_verify_and_key_recover_uncompressed(
message_hash.as_ref().0.as_ptr(),
message_hash.as_ref().0.len(),
signature.as_ref().0.as_ptr(),
signature.as_ref().0.len(),
)
});
key.try_into().unwrap()
}
}
6 changes: 6 additions & 0 deletions scrypto/src/engine/wasm_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,12 @@ pub mod crypto_utils {
message_len: usize,
signature_ptr: *const u8,
signature_len: usize) -> Buffer;

pub fn crypto_utils_secp256k1_ecdsa_verify_and_key_recover_uncompressed(
message_ptr: *const u8,
message_len: usize,
signature_ptr: *const u8,
signature_len: usize) -> Buffer;
}
}

Expand Down
Loading