Migrates PrivateKey and PublicKey implementation to Rust #4347

gupnik · 2025-04-01T12:37:45Z

Description

This PR migrates the implementation in PrivateKey.cpp and PublicKey.cpp to use Rust instead of TrezorCrypto.

How to test

Existing tests continue to work

Types of changes

Refactor

Checklist

Create pull request as draft initially, unless its complete.
Add tests to cover changes as needed.
Update documentation as needed.
If there is a related Issue, mention it in the description.

If you're adding a new blockchain

I have read the guidelines for adding a new blockchain.

github-actions · 2025-04-02T10:58:31Z

Binary size comparison

➡️ aarch64-apple-ios:

- 13.93 MB
+ 13.96 MB 	 +40 KB

➡️ aarch64-apple-ios-sim:

- 13.93 MB
+ 13.97 MB 	 +40 KB

➡️ aarch64-linux-android:

- 18.35 MB
+ 18.41 MB 	 +70 KB

➡️ armv7-linux-androideabi:

- 15.36 MB
+ 15.41 MB 	 +47 KB

➡️ wasm32-unknown-emscripten:

- 13.06 MB
+ 13.11 MB 	 +53 KB

satoshiotomakan

Please also add extensive tests to tw_keypair/src/zilliqa_schnorr/mod.rs. Take a look at other curve implementations, it's highly important to cover it well as it's critical section for us

rust/tw_keypair/src/ecdsa/signature.rs

rust/tw_keypair/src/tw/mod.rs

rust/tw_keypair/src/tw/private.rs

rust/tw_keypair/src/tw/public.rs

satoshiotomakan · 2025-04-02T12:19:11Z

src/PrivateKey.cpp

+PublicKey PrivateKey::getPublicKey(TWPublicKeyType type) const { 
+    auto* pubkey = Rust::tw_private_key_get_public_key_by_type(_impl.get(), static_cast<uint32_t>(type));
+    if (pubkey == nullptr) {
+        return PublicKey(Data(), type);


It's not good to return a PublicKey initialized with an empty data in case of an error, even if PublicKey(Data(), type) constructor throws an exception.
Please throw an exception directly from this function

It's not good to return a PublicKey initialized with an empty data in case of an error, even if PublicKey(Data(), type) constructor throws an exception. Please throw an exception directly from this function

For backward compatibility:

wallet-core/src/PrivateKey.cpp

Line 164 in 4b63b63

PublicKey PrivateKey::getPublicKey(TWPublicKeyType type) const {

I think it's legacy, and we should improve it. If either PrivateKey or PublicKey instances created, it means that they are valid, and can be used later in the code.
I really hope this line is never reached in prod

wallet-core/src/PrivateKey.cpp

Line 222 in 4b63b63

return PublicKey(result, type);

satoshiotomakan · 2025-04-02T12:21:44Z

src/PrivateKey.cpp

    // graphene adds 31 to the recovery id
    result[0] += 31;


Let's move this logic to Rust

satoshiotomakan · 2025-04-02T12:25:27Z

src/PublicKey.cpp

+PublicKey::PublicKey(std::shared_ptr<TW::Rust::TWPublicKey> _impl)
+    : _impl(_impl) {
+    auto pubkeyData = Rust::tw_public_key_data(_impl.get());
+    bytes = Data(pubkeyData.data, pubkeyData.data + pubkeyData.size);


I think bytes filed should be replaced with bytes() method to avoid confusion as the pubkey bytes also stored in Rust's PublicKey

I think bytes filed should be replaced with bytes() method to avoid confusion as the pubkey bytes also stored in Rust's PublicKey

This is for consistency to avoid breaking the API further.

satoshiotomakan · 2025-04-02T12:34:00Z

src/PublicKey.cpp

    if (messageDigest.size() < PrivateKey::_size) {
        throw std::invalid_argument("digest too short");
    }


Let's move all these conditions above to Rust

satoshiotomakan · 2025-04-02T12:39:47Z

tests/chains/Ontology/TWAnySignerTests.cpp

-
-    // TODO uncomment when nist256p1 Rust implementation is enabled.
-    // EXPECT_EQ("00d134120000c40900000000000050c3000000000000fbacc8214765d457c8e3f2b5a1d3c4981a2e9d2a4d02e9001496f688657b95be51c11a87b51adfda4ab69e9cbb1457e9d1a61f9aafa798b6c7fbeae35639681d7df653c1087472616e736665726733def739225d0f93dd2aed457d7b1fd074ec31ff00024140bd2923854d7b84b97a107bb3cddf18c8e3dddd2f36b41a1f5f5b23366484daa2d78e3046e66dc020e1634e1612e9455d0c8acac2305ae0563293d39bfa9d3bec232103d9fd62df332403d9114f3fa3da0d5aec9dfa42948c2f50738d52470469a1a1eeac41406d638653597774ce45812ea2653250806b657b32b7c6ad3e027ddeba91e9a9dab44a2531dc2504589734ce4534c74b58bdc0f3457cd53267331ec5211b0a4e842321031bec1250aa8f78275f99a6663688f31085848d0ed92f1203e447125f927b7486ac", rawTxHex);
+    EXPECT_EQ("00d134120000c40900000000000050c3000000000000fbacc8214765d457c8e3f2b5a1d3c4981a2e9d2a4d02e9001496f688657b95be51c11a87b51adfda4ab69e9cbb1457e9d1a61f9aafa798b6c7fbeae35639681d7df653c1087472616e736665726733def739225d0f93dd2aed457d7b1fd074ec31ff00024140bd2923854d7b84b97a107bb3cddf18c8e3dddd2f36b41a1f5f5b23366484daa2d78e3046e66dc020e1634e1612e9455d0c8acac2305ae0563293d39bfa9d3bec232103d9fd62df332403d9114f3fa3da0d5aec9dfa42948c2f50738d52470469a1a1eeac41406d638653597774ce45812ea2653250806b657b32b7c6ad3e027ddeba91e9a9dab44a2531dc2504589734ce4534c74b58bdc0f3457cd53267331ec5211b0a4e842321031bec1250aa8f78275f99a6663688f31085848d0ed92f1203e447125f927b7486ac", rawTxHex);


Expected 00d134120000c40900000000000050c3000000000000fbacc8214765d457c8e3f2b5a1d3c4981a2e9d2a4d02e9001496f688657b95be51c11a87b51adfda4ab69e9cbb1457e9d1a61f9aafa798b6c7fbeae35639681d7df653c1087472616e736665726733def739225d0f93dd2aed457d7b1fd074ec31ff00024140bd2923854d7b84b97a107bb3cddf18c8e3dddd2f36b41a1f5f5b23366484daa2d78e3046e66dc020e1634e1612e9455d0c8acac2305ae0563293d39bfa9d3bec232103d9fd62df332403d9114f3fa3da0d5aec9dfa42948c2f50738d52470469a1a1eeac41406d638653597774ce45812ea2653250806b657b32b7c6ad3e027ddeba91e9a9dab44a2531dc2504589734ce4534c74b58bdc0f3457cd53267331ec5211b0a4e842321031bec1250aa8f78275f99a6663688f31085848d0ed92f1203e447125f927b7486ac instead. Can you please check if all correct?
I could be wrong with the comment by a mistake, but still better to validate

…verifying APIs for zillqa

satoshiotomakan

A couple of changes please

satoshiotomakan · 2025-04-03T08:07:54Z

src/PublicKey.cpp

 }

+PublicKey::PublicKey(std::shared_ptr<TW::Rust::TWPublicKey> _impl)
+    : _impl(_impl) {


std::move(_impl)

rust/tw_keypair/src/tw/public.rs

satoshiotomakan · 2025-04-03T11:05:37Z

rust/tw_keypair/src/tw/public.rs

+        }
+    }
+
+    pub fn recover_from_signature(


secp256k1SignatureSize is the same as nist256p1 signature length.
For the reference, we have nist256p1::PublicKey::recover function, so I think we can expose it to FFI too

satoshiotomakan · 2025-04-03T11:18:14Z

rust/tw_keypair/src/zilliqa_schnorr/private.rs

+    type SigningMessage = Vec<u8>;
+    type Signature = Signature;
+
+    fn sign(&self, message: Self::SigningMessage) -> KeyPairResult<Self::Signature> {


No I mean a link to zilliqa_rs crate

satoshiotomakan · 2025-04-03T11:36:58Z

src/PrivateKey.cpp

    bytes = data;
    _curve = curve;
+    auto* privkey = Rust::tw_private_key_create_with_data(data.data(), data.size(), static_cast<uint32_t>(curve));
+    _impl = privkey ? Rust::wrapTWPrivateKey(privkey) : nullptr;


Not fixed yet. Let's throw an exception if privkey is null

satoshiotomakan · 2025-04-03T11:37:10Z

src/PrivateKey.cpp

    append(bytes, chainCode2);
    _curve = curve;
+    auto* privkey = Rust::tw_private_key_create_with_data(bytes.data(), bytes.size(), static_cast<uint32_t>(curve));
+    _impl = privkey ? Rust::wrapTWPrivateKey(privkey) : nullptr;


Same, let's throw an exception

satoshiotomakan · 2025-04-03T11:39:48Z

src/PrivateKey.cpp

+PublicKey PrivateKey::getPublicKey(TWPublicKeyType type) const { 
+    auto* pubkey = Rust::tw_private_key_get_public_key_by_type(_impl.get(), static_cast<uint32_t>(type));
+    if (pubkey == nullptr) {
+        return PublicKey(Data(), type);


I think it's legacy, and we should improve it. If either PrivateKey or PublicKey instances created, it means that they are valid, and can be used later in the code.
I really hope this line is never reached in prod

wallet-core/src/PrivateKey.cpp

Line 222 in 4b63b63

return PublicKey(result, type);

src/PrivateKey.cpp

satoshiotomakan · 2025-04-03T11:43:20Z

src/PublicKey.cpp

+    auto* pubkey = Rust::tw_public_key_create_with_data(data.data(), data.size(), static_cast<uint32_t>(type));
+    if (pubkey != nullptr) {
+        _impl = Rust::wrapTWPublicKey(pubkey);
+        Rust::CByteArrayWrapper pubkeyData = Rust::tw_public_key_data(_impl.get());
+        bytes = pubkeyData.data;
+    } else {
+        _impl = nullptr;
+        bytes = data;


Please throw an exception if couldn't create the PublicKey

satoshiotomakan

One more important change

rust/tw_keypair/src/zilliqa_schnorr/private.rs

satoshiotomakan · 2025-04-04T08:33:47Z

rust/tw_keypair/src/tw/private.rs

+                let bytes = if bytes.len() == Self::SIZE {
+                    // For a regular private key, replicate it 6 times to match the Cardano extended format
+                    let mut extended_bytes = Vec::with_capacity(Self::CARDANO_SIZE);
+                    for _ in 0..6 {
+                        extended_bytes.extend_from_slice(&bytes[Self::KEY_RANGE]);
+                    }
+                    extended_bytes
+                } else {
+                    bytes.to_vec()
+                };


Fix as discussed DM

…an optional

satoshiotomakan

LGTM!

satoshiotomakan · 2025-04-04T12:03:12Z

tests/chains/Cardano/AddressTests.cpp

-            dummyKey, dummyKey, dummyKey, TWCurveED25519);
-        auto publicKey = privateKey.getPublicKey(TWPublicKeyTypeED25519);
-        EXPECT_ANY_THROW(new AddressV3(publicKey));
+            dummyKey, dummyKey, dummyKey, TWCurveED25519));


Would this test pass if we replace TWCurveED25519ExtendedCardano?

satoshiotomakan · 2025-04-04T12:03:19Z

tests/chains/Cardano/AddressTests.cpp

-            dummyKey, dummyKey, dummyKey, TWCurveED25519);
-        auto publicKey = privateKey.getPublicKey(TWPublicKeyTypeED25519);
-        EXPECT_ANY_THROW(new AddressV2(publicKey));
+            dummyKey, dummyKey, dummyKey, TWCurveED25519));


Same question

gupnik added 4 commits April 1, 2025 18:06

Migrates PrivateKey and PublicKey implementation to Rust

ac6ef20

Migrates PublicKey zilliqa schnorr to rust

804c170

Migrates Zillqa Private Key to rust

9ccd8cd

Removes todo

562999d

gupnik marked this pull request as ready for review April 2, 2025 10:36

gupnik requested a review from satoshiotomakan as a code owner April 2, 2025 10:36

gupnik and others added 3 commits April 2, 2025 16:06

Merge branch 'dev' into gupnik/ffi-rust-migration

92478bf

Makes clippy happy

3b6767e

Minor fix

15e6ff8

gupnik added 2 commits April 2, 2025 18:05

Adds Rust Tests

7be83cd

Fixes address santization issue

6dc84a9

satoshiotomakan requested changes Apr 2, 2025

View reviewed changes

gupnik added 9 commits April 2, 2025 18:58

Fixes doctests

f366a7e

Adds Zilliqa Schnorr as a separate curve and remove separate signing/…

2561a47

…verifying APIs for zillqa

FMT

050ced7

Addresses review comments

8799c46

Fixes C++ Tests

b3b42ec

Derive ZeroizeOnDrop trait on Zillqa Private Key

00f8093

Cleanup public key data

0c3943b

Adds Tests

e129cb4

FMT

d80d41d

trustwallet deleted a comment from Ruyeduardo Apr 3, 2025

Use CByteArrayWrapper

d4aa6d2

satoshiotomakan requested changes Apr 3, 2025

View reviewed changes

Addresses review comments

8086ddb

satoshiotomakan requested changes Apr 3, 2025

View reviewed changes

rust/tw_keypair/src/zilliqa_schnorr/private.rs Outdated Show resolved Hide resolved

gupnik added 2 commits April 3, 2025 20:05

Addresses review comment

8819f24

Removes doc test

2262924

satoshiotomakan requested changes Apr 4, 2025

View reviewed changes

Only allow valid Cardano keys and make TWHDWalletGetMasterKey return …

e788c57

…an optional

satoshiotomakan approved these changes Apr 4, 2025

View reviewed changes

satoshiotomakan merged commit 6b46c47 into dev Apr 4, 2025
14 checks passed

satoshiotomakan deleted the gupnik/ffi-rust-migration branch April 4, 2025 12:04

gupnik mentioned this pull request Apr 4, 2025

[Dev]: Dev<->Master #4315

Open

8 tasks

Migrates PrivateKey and PublicKey implementation to Rust #4347

Migrates PrivateKey and PublicKey implementation to Rust #4347

Uh oh!

Conversation

gupnik commented Apr 1, 2025

Description

How to test

Types of changes

Checklist

Uh oh!

github-actions bot commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary size comparison

Uh oh!

satoshiotomakan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

satoshiotomakan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satoshiotomakan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satoshiotomakan left a comment

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Apr 2, 2025 •

edited

Loading