feat: ledger key manager interface #5644

brianp · 2023-08-16T12:22:06Z

Description

This PR expands the console wallet and key manager interfaces to support hardware wallets. It does not introduce functionality to communicate with the hardware wallet, simply the interface to support it. Another PR will be opened swapping out the placeholder software key manager for the hardware key manager where appropriate.

Motivation and Context

So people can use their most excellent hardware wallets with the MinoTari console wallet.

How Has This Been Tested?

Manually on osx

What process can a PR reviewer use to test or verify this change?

Breaking Changes

None
Requires data directory on base node to be deleted
Requires hard fork
Other - Please specify

github-actions · 2023-08-16T12:38:11Z

Test Results (CI)

3 files 120 suites 34m 16s ⏱️
1 269 tests 1 269 ✅ 0 💤 0 ❌
3 799 runs 3 799 ✅ 0 💤 0 ❌

Results for commit 85d8b85.

♻️ This comment has been updated with latest results.

github-actions · 2023-08-16T12:42:50Z

Test Results (Integration tests)

2 files 11 suites 41m 12s ⏱️
32 tests 31 ✅ 0 💤 1 ❌
34 runs 31 ✅ 0 💤 3 ❌

For more details on these failures, see this check.

Results for commit 85d8b85.

♻️ This comment has been updated with latest results.

brianp · 2023-08-16T13:30:59Z

This PR was previously tracked on #5609 but stopped updating when new commits were pushed. This is the same PR but updated against development and with fixes in the test suite which is now green.

hansieodendaal · 2023-08-16T14:08:08Z

I added a fix for the failing cucumber test - see #5645

SWvheerden · 2023-08-17T10:28:14Z

applications/minotari_console_wallet/Cargo.toml

 chrono = { version = "0.4.19", default-features = false }
 clap = { version = "3.2", features = ["derive", "env"] }
 config = "0.13.0"
 crossterm = { version = "0.25.0" }
 digest = "0.10"
 futures = { version = "^0.3.16", default-features = false, features = ["alloc"] }
-log4rs = { git = "https://github.com/tari-project/log4rs.git", default_features = false, features = ["config_parsing", "threshold_filter", "yaml_format", "console_appender", "rolling_file_appender", "compound_policy", "size_trigger", "fixed_window_roller", "delete_roller"] }
+ledger-transport-hid = { git = "https://github.com/Zondax/ledger-rs" }


lock to a commit

hansieodendaal

Hi @brianp, I can appreciate that this was a lot of work, however, I am not a fan of this approach.

In this PR we create a lot of scaffolding just to be able to create a ledger-specific or software-specific transaction key manager. My recommendations:

Initial startup
- Add a command line switch to force the use of the ledger hardware wallet - if the ledger is not attached and the app not started panic with the error message - if attached and app started ok.
- If the ledger is attached but the wallet is started without the switch, ask the user if this should be a hardware console wallet, if yes follow the logic as if the command line switch was present.
- ~~When started ok, earmark the console wallet as a ledger-only console wallet - consecutive startups without the ledger attached and the app started must panic.~~
Key manager
- We should not differentiate outside of the key manager if we have a ledger or not, so no ledger_key_manager.
- The key manager should be aware if a hardware wallet should be used and if it is a ledger and later on perhaps a trezor if that is added.
- Inside the key manager whenever we have to use a secret key, for example inside inner/get_receiver_partial_metadata_signature(..), the appropriate software or hardware branch is called. There are not that many places where this branching will be necessary. All ledger-specific code can go into key_manager/ledger.rs, and later on we can add key_manager/trezor.rs.
Private keys to be managed
- Both spend and script private keys should be managed by the ledger, but with the difference that the spend private key should be returned from the ledger to be used inside the key manager where needed and the script private key should never leave the ledger.
- ~~This will mean that range-proof bulletproofs can be constructed outside the ledger but not without it, and script signatures will be wholly constructed inside the ledger.~~
- The same index must be used to obtain the matching set of ~~spend and script~~ private keys, as is currently the case with the software key manager.
~~Spending scripts~~
- If both spend and script private keys are managed by the ledger, a hardware wallet will be able to receive an interactive transaction with any type of script, and will not be able to be sussed out as a hardware wallet by penny/poking transactions.

SWvheerden · 2023-08-21T07:18:09Z

The ledger implementation very much follows this RFC: tari-project/rfcs#98
This means spend keys should be managed by the software wallet, not the hardware wallet aka ledger in this first implementation.

As per the RFC, this allows us to receive transactions without the hardware wallet being online, allowing it to be a cold storage wallet.

leet4tari

utACK - scripts/install_ubuntu_dependencies.sh

hansieodendaal

Nice so far, just one comment for now, apart form the CI/clippy errors.

hansieodendaal · 2024-03-13T10:32:36Z

applications/minotari_console_wallet/src/init/mod.rs

+                    Ok(_) => {
+                        println!("Device found.");
+                        let account = prompt_ledger_account().expect("An account value");
+                        Some(WalletType::Ledger(account))


The TRANSPORT lazy static should be assigned a value

Suggested change

Ok(_) => {

println!("Device found.");

let account = prompt_ledger_account().expect("An account value");

Some(WalletType::Ledger(account))

Ok(transport) => {

*TRANSPORT.lock().expect("lock exists") = Some(transport);

println!("Device found.");

let account = prompt_ledger_account().expect("An account value");

Some(WalletType::Ledger(account))

This actually sent me on a wild goose chase trying to find where the code had previously been removed.

But, we don't actually need to assign this here. We check to make sure the ledger is present to see if we should create a ledger backed wallet. But the ledger could be unplugged anytime between this prompt, and performing a transaction. It doesn't need to be plugged in at all times. We need to check to see if it's present again, only during script and meta data sig generation for send transactions.

hansieodendaal

utACK

SWvheerden · 2024-03-14T14:59:46Z

base_layer/core/src/transactions/key_manager/inner.rs

@@ -429,6 +443,40 @@ where TBackend: KeyManagerBackend<PublicKey> + 'static
    // Transaction input section (transactions > transaction_components > transaction_input)
    // -----------------------------------------------------------------------------------------------------------------

+    pub async fn get_script_private_key(&self, script_key_id: &TariKeyId) -> Result<PrivateKey, TransactionError> {


This is fine for now, but in the actual implementation we should ver get this script key out of the ledger, we should ask the ledger to sign the script signature for us and we should get that out

ghpbot-tari-project added the CR-too_long Changes Requested - Your PR is too long label Aug 16, 2023

ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Aug 16, 2023

brianp requested review from SWvheerden and hansieodendaal August 16, 2023 13:29

SWvheerden changed the base branch from development to feat-hardware-support August 17, 2023 11:41

SWvheerden reviewed Aug 17, 2023

View reviewed changes

brianp mentioned this pull request Aug 17, 2023

wip: ledger wallet integration #5648

Closed

4 tasks

hansieodendaal requested changes Aug 18, 2023

View reviewed changes

brianp requested a review from sdbondi August 18, 2023 10:13

brianp force-pushed the ledger-key-manager branch from 46329dc to 4b254d1 Compare August 25, 2023 15:02

brianp force-pushed the ledger-key-manager branch 2 times, most recently from eacace9 to 41934fb Compare March 11, 2024 09:53

brianp requested review from a team as code owners March 11, 2024 09:53

leet4tari reviewed Mar 13, 2024

View reviewed changes

hansieodendaal reviewed Mar 13, 2024

View reviewed changes

brianp force-pushed the ledger-key-manager branch 2 times, most recently from ac183a1 to 2618697 Compare March 13, 2024 15:57

SWvheerden changed the base branch from feat-hardware-support to development March 14, 2024 06:07

hansieodendaal previously approved these changes Mar 14, 2024

View reviewed changes

SWvheerden reviewed Mar 14, 2024

View reviewed changes

brianp dismissed hansieodendaal’s stale review via c5e5a9f March 14, 2024 17:12

brianp force-pushed the ledger-key-manager branch 2 times, most recently from c5e5a9f to 29efb44 Compare March 14, 2024 17:26

brianp force-pushed the ledger-key-manager branch from 29efb44 to b2c5030 Compare March 14, 2024 22:19

brianp and others added 12 commits March 25, 2024 15:11

Support wallet type

992c117

Add libudev-dev for hidapi support

5ca1ed3

Merge fixes for account number

4605815

Recover minitari ledger wallet

55742e8

Simplify the inner manager ledger interface

3228c84

Codesplit for ledger

9bf33dd

Ignore ledger deliverables

4284fbb

Turn off console wallet ledger support

2ee9913

Push the deactivation up a level for easier compat

2490da6

clippy warnings

9513368

Conditionally compiler ledger support

2a61d14

rev lock the ledger crate

85d8b85

brianp force-pushed the ledger-key-manager branch from 50a5805 to 85d8b85 Compare March 25, 2024 14:14

SWvheerden approved these changes Mar 26, 2024

View reviewed changes

SWvheerden merged commit 0d66126 into tari-project:development Mar 26, 2024
14 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ledger key manager interface #5644

feat: ledger key manager interface #5644

brianp commented Aug 16, 2023

github-actions bot commented Aug 16, 2023 •

edited

github-actions bot commented Aug 16, 2023 •

edited

brianp commented Aug 16, 2023

hansieodendaal commented Aug 16, 2023

SWvheerden Aug 17, 2023

hansieodendaal left a comment •

edited

SWvheerden commented Aug 21, 2023

leet4tari left a comment

hansieodendaal left a comment •

edited

hansieodendaal Mar 13, 2024

brianp Mar 14, 2024

hansieodendaal left a comment

SWvheerden Mar 14, 2024

feat: ledger key manager interface #5644

feat: ledger key manager interface #5644

Conversation

brianp commented Aug 16, 2023

Description

Motivation and Context

How Has This Been Tested?

What process can a PR reviewer use to test or verify this change?

Breaking Changes

github-actions bot commented Aug 16, 2023 • edited

Test Results (CI)

github-actions bot commented Aug 16, 2023 • edited

Test Results (Integration tests)

brianp commented Aug 16, 2023

hansieodendaal commented Aug 16, 2023

SWvheerden Aug 17, 2023

Choose a reason for hiding this comment

hansieodendaal left a comment • edited

Choose a reason for hiding this comment

SWvheerden commented Aug 21, 2023

leet4tari left a comment

Choose a reason for hiding this comment

hansieodendaal left a comment • edited

Choose a reason for hiding this comment

hansieodendaal Mar 13, 2024

Choose a reason for hiding this comment

brianp Mar 14, 2024

Choose a reason for hiding this comment

hansieodendaal left a comment

Choose a reason for hiding this comment

SWvheerden Mar 14, 2024

Choose a reason for hiding this comment

github-actions bot commented Aug 16, 2023 •

edited

github-actions bot commented Aug 16, 2023 •

edited

hansieodendaal left a comment •

edited

hansieodendaal left a comment •

edited