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

Conversation

iamyulong
Copy link
Member

No description provided.

Copy link

github-actions bot commented Nov 15, 2024

Docker tags
docker.io/radixdlt/private-scrypto-builder:e438eec4fe

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

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.


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

@iamyulong iamyulong force-pushed the feature/uncompressed-public-key branch from 7570a13 to fdb2ad3 Compare November 15, 2024 13:58
@iamyulong iamyulong merged commit ba329d3 into release/cuttlefish Nov 15, 2024
31 checks passed
Copy link

Benchmark for e438eec

Click to view benchmark
Test Base PR %
costing::bench_prepare_wasm 48.2±0.14ms 48.6±0.11ms +0.83%
costing::decode_encoded_i8_array_to_manifest_raw_value 19.3±0.02ms 19.6±0.01ms +1.55%
costing::decode_encoded_i8_array_to_manifest_value 42.2±0.07ms 41.4±0.07ms -1.90%
costing::decode_encoded_tuple_array_to_manifest_raw_value 72.2±0.12ms 71.2±0.12ms -1.39%
costing::decode_encoded_tuple_array_to_manifest_value 98.8±0.13ms 98.5±0.19ms -0.30%
costing::decode_encoded_u8_array_to_manifest_raw_value 32.2±0.12µs 32.2±0.11µs 0.00%
costing::decode_encoded_u8_array_to_manifest_value 41.9±0.05ms 41.6±0.06ms -0.72%
costing::decode_rpd_to_manifest_raw_value 14.5±0.03µs 14.6±0.03µs +0.69%
costing::decode_rpd_to_manifest_value 11.0±0.05µs 10.9±0.10µs -0.91%
costing::deserialize_wasm 1205.2±3.69µs 1228.1±3.52µs +1.90%
costing::execute_transaction_creating_big_vec_substates 707.8±10.40ms 694.2±9.64ms -1.92%
costing::execute_transaction_reading_big_vec_substates 582.9±1.77ms 594.7±1.00ms +2.02%
costing::instantiate_flash_loan 3.5±0.07ms 3.5±0.04ms 0.00%
costing::instantiate_radiswap 3.5±0.06ms 3.6±0.04ms +2.86%
costing::scrypto_malloc 928.5±3.23ms 877.2±1.01ms -5.53%
costing::scrypto_sbor_decode 1018.5±7.06ms 1053.1±2.79ms +3.40%
costing::scrypto_sha256 964.8±2.09ms 989.4±1.24ms +2.55%
costing::spin_loop_v1 657.7±1.20ms 652.9±0.71ms -0.73%
costing::spin_loop_v2 696.0±1.10ms 691.4±1.56ms -0.66%
costing::validate_sbor_payload 30.9±0.10µs 29.3±0.05µs -5.18%
costing::validate_sbor_payload_bytes 258.2±0.60ns 249.3±0.53ns -3.45%
costing::validate_secp256k1 76.7±0.30µs 76.7±0.33µs 0.00%
costing::validate_wasm 36.5±0.03ms 36.5±0.03ms 0.00%
decimal::add/0 8.4±0.00ns 8.4±0.00ns 0.00%
decimal::add/rust-native 9.8±0.01ns 9.8±0.01ns 0.00%
decimal::add/wasmi 714.6±5.92ns 740.0±1.36ns +3.55%
decimal::add/wasmi-call-native 6.0±0.02µs 6.0±0.02µs 0.00%
decimal::div/0 168.0±0.19ns 169.0±0.28ns +0.60%
decimal::from_string/0 155.8±0.28ns 154.7±0.22ns -0.71%
decimal::mul/0 129.2±0.23ns 129.2±0.05ns 0.00%
decimal::mul/rust-native 127.6±0.14ns 127.5±0.07ns -0.08%
decimal::mul/wasmi 49.3±0.11µs 49.2±0.35µs -0.20%
decimal::mul/wasmi-call-native 6.1±0.01µs 6.1±0.01µs 0.00%
decimal::pow/0 598.4±0.74ns 596.3±0.23ns -0.35%
decimal::pow/rust-native 586.4±0.58ns 586.4±0.29ns 0.00%
decimal::pow/wasmi 236.9±1.03µs 234.7±0.33µs -0.93%
decimal::pow/wasmi-call-native 10.1±0.04µs 9.9±0.05µs -1.98%
decimal::root/0 8.1±0.01µs 8.2±0.00µs +1.23%
decimal::sub/0 8.1±0.00ns 8.1±0.00ns 0.00%
decimal::to_string/0 443.6±1.93ns 436.9±0.38ns -1.51%
large_transaction_processing::prepare 2.5±0.00ms 2.5±0.00ms 0.00%
large_transaction_processing::prepare_and_decompile 6.3±0.03ms 6.3±0.02ms 0.00%
large_transaction_processing::prepare_and_decompile_and_recompile 26.7±2.23ms 24.6±0.21ms -7.87%
metadata_validation::validate_urls 5.5±0.05µs 4.8±0.02µs -12.73%
precise_decimal::add/0 8.9±0.08ns 8.9±0.04ns 0.00%
precise_decimal::add/rust-native 10.8±0.00ns 10.6±0.00ns -1.85%
precise_decimal::add/wasmi 845.5±4.89ns 934.4±3.73ns +10.51%
precise_decimal::add/wasmi-call-native 7.7±0.02µs 7.5±0.01µs -2.60%
precise_decimal::div/0 298.3±1.11ns 314.3±0.82ns +5.36%
precise_decimal::from_string/0 201.1±0.13ns 203.0±0.25ns +0.94%
precise_decimal::mul/0 339.3±1.58ns 336.5±0.62ns -0.83%
precise_decimal::mul/rust-native 283.2±0.58ns 284.5±0.55ns +0.46%
precise_decimal::mul/wasmi 123.5±0.23µs 121.7±0.20µs -1.46%
precise_decimal::mul/wasmi-call-native 8.2±0.06µs 7.9±0.04µs -3.66%
precise_decimal::pow/0 1731.4±3.74ns 1734.7±4.86ns +0.19%
precise_decimal::pow/rust-native 1352.9±0.75ns 1357.7±1.46ns +0.35%
precise_decimal::pow/wasmi 593.3±1.42µs 590.7±1.39µs -0.44%
precise_decimal::pow/wasmi-call-native 15.3±0.09µs 15.4±0.10µs +0.65%
precise_decimal::root/0 58.5±0.03µs 59.1±0.03µs +1.03%
precise_decimal::sub/0 8.9±0.06ns 8.9±0.05ns 0.00%
precise_decimal::to_string/0 701.3±1.83ns 685.0±0.31ns -2.32%
schema::validate_payload 382.1±0.69µs 439.6±0.71µs +15.05%
transaction::radiswap 5.4±0.03ms 5.5±0.02ms +1.85%
transaction::transfer 1824.9±4.84µs 1809.5±2.19µs -0.84%
transaction_validation::validate_manifest 43.3±0.14µs 43.3±0.02µs 0.00%
transaction_validation::verify_bls_2KB 1058.0±28.13µs 1006.4±13.27µs -4.88%
transaction_validation::verify_bls_32B 1004.1±12.02µs 1035.3±40.68µs +3.11%
transaction_validation::verify_ecdsa 74.6±0.23µs 74.6±0.09µs 0.00%
transaction_validation::verify_ed25519 45.3±0.05µs 45.5±0.03µs +0.44%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants