Hardware-wallet/usb-subscribe-refactor #7860

niklasad1 · 2018-02-11T21:38:53Z

Hi,

This PR changes how events from libusb/Hotplug are subscribed in more fine-grained manner i.e, by manufacturer_id, product_id and USB class.

By doing this it fixes #7516 & #7793 it seems to be Windows only issue because we tested it earlier on both Linux/Ubuntu and OSX

parity-cla-bot · 2018-02-11T21:38:55Z

It looks like @niklasad1 hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, plesae reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

niklasad1 · 2018-02-11T21:45:05Z

[clabot:check].

parity-cla-bot · 2018-02-11T21:45:06Z

It looks like @niklasad1 signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

rphmeier · 2018-02-12T17:33:00Z

hw/src/ledger.rs

-const LEDGER_PIDS: [u16; 2] = [0x0000, 0x0001]; // Nano S and Blue
+// Expose these to the callback configuration
+pub const LEDGER_VID: u16 = 0x2c97;
+pub const LEDGER_PIDS: [u16; 2] = [0x0000, 0x0001]; // Nano S and Blue


needs documentation for public items

rphmeier · 2018-02-12T17:33:38Z

hw/src/ledger.rs

-			::std::thread::sleep(Duration::from_millis(200));
+		match f() {
+			Ok(handle) => Ok(handle),
+			Err(e) => Err(From::from(e)),


f().map_err(Into::into)

rphmeier · 2018-02-12T17:33:55Z

hw/src/ledger.rs

@@ -335,6 +344,53 @@ impl Manager {
 	}
 }

+pub struct EventHandler {


rphmeier · 2018-02-12T17:34:08Z

hw/src/ledger.rs

+}
+
+impl EventHandler {
+	pub fn new(ledger: Weak<Manager>) -> Self {


rphmeier · 2018-02-12T17:34:17Z

hw/src/lib.rs

 use std::sync::atomic;
 use std::sync::atomic::AtomicBool;
 use std::thread;
 use std::time::Duration;
 use ethereum_types::U256;

+const USB_DEVICE_CLASS_DEVICE: u8 = 0;
+
+


redundant whitespace

rphmeier · 2018-02-12T17:35:54Z

hw/src/lib.rs

-		let usb_context = Arc::new(libusb::Context::new()?);
+		let usb_context_trezor = Arc::new(libusb::Context::new()?);
+		let usb_context_ledger = Arc::new(libusb::Context::new()?);
+		let mut active_threads: Vec<Option<thread::JoinHandle<()>>> = Vec::with_capacity(10);


why bother setting the capacity to 10? (vec internally uses initial of 4 AFAIK)

rphmeier · 2018-02-12T17:37:17Z

hw/src/lib.rs

-			.name("hw_wallet".to_string())
+
+		// Ledger event thread
+		// FIXME: check if we can move it to ledger.rs (lifetime issue)


rphmeier · 2018-02-12T17:37:40Z

hw/src/lib.rs

 			.spawn(move || {
 				if let Err(e) = l.update_devices() {
 					debug!("Error updating ledger devices: {}", e);
 				}
+				loop {
+					usb_context_ledger.handle_events(Some(Duration::from_millis(500)))
+					           .unwrap_or_else(|e| debug!("Error processing USB events: {}", e));


provide a target for log events

rphmeier · 2018-02-12T17:38:40Z

hw/src/trezor.rs

@@ -407,6 +404,41 @@ impl Manager {
 	}
 }

+pub struct EventHandler {


rphmeier · 2018-02-12T17:38:50Z

hw/src/trezor.rs

+				debug!("TREZOR Connect Error: {:?}", e);
+			}
+			println!("unlocked devices: {:?}", t.list_devices());
+			println!("locked devices: {:?}", t.list_locked_devices());


stray printlns in this file

rphmeier · 2018-02-12T17:39:44Z

hw/src/trezor.rs

+		println!("trezor arrived");
+		if let Some(t) = self.trezor.upgrade() {
+			// Wait for the device to boot-up
+			thread::sleep(Duration::from_millis(1000));


this seems arbitrary and prone to race conditions. is there a better way?

Hrm, only a physical device TREZOR device can generate this and the update_devices() iterates through the devices and read/write to them after acquiring a RwLock. Thus, the risk is pretty low that somebody plugs in two TREZOR keys at the same time

But still Weak::upgrade() -> Rc is not atomic, so it that what you refer to? That could probably be solved by another lock (before line 420)

Thoughts/suggestions?

rphmeier · 2018-02-12T17:40:08Z

hw/src/ledger.rs

+impl libusb::Hotplug for EventHandler {
+	fn device_arrived(&mut self, device: libusb::Device) {
+		println!("ledger arrived");
+		Duration::from_millis(1000);


this line does nothing, stray println above

Oops, should be thread::sleep(Duration::from_millis(1000)) ofc

rphmeier · 2018-02-12T17:45:32Z

hw/src/lib.rs

@@ -128,84 +131,81 @@ impl From<libusb::Error> for Error {

 /// Hardware wallet management interface.
 pub struct HardwareWalletManager {
-	update_thread: Option<thread::JoinHandle<()>>,
+	active_threads: Vec<Option<thread::JoinHandle<()>>>,


i'm curious about the motivation to have a separate thread for each HW wallet -- I am mildly worried about the overhead of waking these threads up fairly often to poll for exit and incurring context switches during regular import.

The other downside to joining the threads manually is that bugs in libusb or drivers can cause Parity's exit to completely hang -- this is something we want to avoid because the HardwareWalletManager cleanup might be blocking e.g. rocksdb cleanup and the inevitable forced shutdown could break things.

This is a separate issue, but we have a StopGuard utility that currently wraps the Arc<AtomicBool> logic, with the idea that there is a single one shared by all the utilities in the program (with only the main thread capable of writing to it). I don't think we do this quite yet, but it would be nice to have the main thread signal stop and wait for a few seconds or for the ref-count to go to zero (whichever comes first).

@rphmeier
Great observations, the main motivation is to check whether ÙSB Hotplug callbacks are the reason of the crashes along with removing dummy for loops. By splitting it into two threads makes it possible the control the callbacks in fine-grained manner instead of subscribing to all devices and the code becomes easier to understand IMO.

It's possible to do it with one thread and some additional checks though, but let's wait for the test on the Windows machine take it from there (I would be very surprised if it still fails... 🔨 )

Ok, didn't know that I just did similar to the previous implementation

rphmeier · 2018-02-12T17:46:25Z

Approach generally looks good, major point of question is the shutdown logic.

niklasad1 · 2018-02-13T15:22:13Z

provide a target for log events

The logs looks as follows:

2018-02-13 16:00:25  hw_wallet_trezor DEBUG hardware_wallet  Trezor startup
2018-02-13 16:00:29  hw_wallet_trezor DEBUG hardware_wallet::trezor  Trezor V1 left
2018-02-13 16:00:35  hw_wallet_trezor DEBUG hardware_wallet::trezor  Trezor V1 arrived
2018-02-13 16:00:40  hw_wallet_trezor DEBUG hardware_wallet::trezor  Trezor V1 left

But I don't really understand your comment, can you clarify what you mean?

EDIT:
I use to the following syntax to enable a target for the debug prints now:

debug!(target: "hw", "some debug text");

niklasad1 · 2018-02-22T12:45:52Z

Update: It's now confirmed that this fixes #7516 and is it sufficient change the shutdown logic or move back to one thread to get this merged?

ping @rphmeier @debris

rphmeier · 2018-02-22T16:26:57Z

hw/src/lib.rs

-		if let Some(thread) = self.update_thread.take() {
-			thread.thread().unpark();
-			thread.join().ok();
+		for thread in self.active_threads.iter_mut() {


better to kill the source of a potential hang for now. what are the repercussions if the process exits without joining the threads?

If the main thread just terminates the two other threads will run and listen for new events and try to connect/disconnect to trezors and ledgers. But I think that is ok since these threads as suppose to live as long as parity does!

Thoughts?

EDIT: But still AtomicBool is shared between the threads and should save us in this context and the threads should terminate once this propagated.

I think signalling that Parity has gone down with an atomic bool or similar is fine. Just that I don't think blocking while waiting for them to join is necessarily a good idea.

* More fine-grained initilization of callbacks by vendorID, productID and usb class * Each device manufacturer gets a seperate handle thread each * Replaced "dummy for loop" with a delay to wait for the device to boot-up properly * Haven't been very carefully with checking dependencies cycles etc * Inline comments explaining where shortcuts have been taken * Need to test this on Windows machine and with Ledger (both models) Signed-off-by: niklasad1 <niklasadolfsson1@gmail.com>

* Remove thread joining in HardwareWalletManager * Remove thread handlers in HardwareWalletManager because this makes them unused

niklasad1 · 2018-02-22T22:26:01Z

FYI, I think the next step should be to remove HardwareWalletManager struct and replace it with a trait that all Wallets implement instead

rphmeier · 2018-02-27T15:36:32Z

The thread:sleep stuff on startup isn't great, would prefer it to be polled, but I guess it works for now.

* Hardware-wallet fix * More fine-grained initilization of callbacks by vendorID, productID and usb class * Each device manufacturer gets a seperate handle thread each * Replaced "dummy for loop" with a delay to wait for the device to boot-up properly * Haven't been very carefully with checking dependencies cycles etc * Inline comments explaining where shortcuts have been taken * Need to test this on Windows machine and with Ledger (both models) Signed-off-by: niklasad1 <niklasadolfsson1@gmail.com> * Validate product_id of detected ledger devices * closed_device => unlocked_device * address comments * add target in debug * Address feedback * Remove thread joining in HardwareWalletManager * Remove thread handlers in HardwareWalletManager because this makes them unused

niklasad1 · 2018-02-28T13:16:46Z

@rphmeier
To clarify it is only the hardware event threads that are sleeping and shouldn't have big impact on parity but the user-experience might be affected while waiting for the sleep

Polling should be simple to introduce by something like:

        let start_time = Instant::now();
        loop {
            if let Ok(e) = self.update_devices() {
                break;
            }
            if start_time.elapsed() >= Duration::from_millis(1000) {
                 // ooops failed with USB while trying for 1 second  
                debug!(".....");
                break;
            }
        }

Let me know this should be easy to fix

* Hardware-wallet/usb-subscribe-refactor (#7860) * Hardware-wallet fix * More fine-grained initilization of callbacks by vendorID, productID and usb class * Each device manufacturer gets a seperate handle thread each * Replaced "dummy for loop" with a delay to wait for the device to boot-up properly * Haven't been very carefully with checking dependencies cycles etc * Inline comments explaining where shortcuts have been taken * Need to test this on Windows machine and with Ledger (both models) Signed-off-by: niklasad1 <niklasadolfsson1@gmail.com> * Validate product_id of detected ledger devices * closed_device => unlocked_device * address comments * add target in debug * Address feedback * Remove thread joining in HardwareWalletManager * Remove thread handlers in HardwareWalletManager because this makes them unused * fixed broken logs (#7934) * fixed broken logs * bring back old lock order * removed bloom groups from blockchain * revert unrelated changes * simplify blockchain_block_blooms * Bump WS (#7952) * Calculate proper keccak256/sha3 using parity. (#7953) * Increase max download limit to 128MB (#7965) * fetch: increase max download limit to 64MB * parity: increase download size limit for updater service * Detect too large packets in snapshot sync. (#7977) * fix traces, removed bloomchain crate, closes #7228, closes #7167 (#7979) * Remvoe generator.rs * Make block generator easier to use (#7888) * Make block generator easier to use * applied review suggestions * rename BlockMetadata -> BlockOptions * removed redundant uses of blockchain generator and genereator.next().unwrap() calls

5chdn added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Feb 12, 2018

5chdn added this to the 1.10 milestone Feb 12, 2018

rphmeier reviewed Feb 12, 2018

View reviewed changes

hw/src/ledger.rs Outdated

@@ -335,6 +344,53 @@ impl Manager {

}

}

pub struct EventHandler {

Copy link

Contributor

rphmeier Feb 12, 2018

Choose a reason for hiding this comment

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

needs docs

rphmeier reviewed Feb 12, 2018

View reviewed changes

hw/src/ledger.rs Outdated

}

impl EventHandler {

pub fn new(ledger: Weak<Manager>) -> Self {

Copy link

Contributor

rphmeier Feb 12, 2018

Choose a reason for hiding this comment

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

and here

rphmeier reviewed Feb 12, 2018

View reviewed changes

hw/src/trezor.rs Outdated

@@ -407,6 +404,41 @@ impl Manager {

}

}

pub struct EventHandler {

Copy link

Contributor

rphmeier Feb 12, 2018

Choose a reason for hiding this comment

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

docs

rphmeier reviewed Feb 12, 2018

View reviewed changes

niklasad1 force-pushed the hardware-wallet/usb_fix branch 2 times, most recently from b720882 to fd41215 Compare February 13, 2018 15:15

niklasad1 force-pushed the hardware-wallet/usb_fix branch from fd41215 to 3317ba3 Compare February 14, 2018 16:13

niklasad1 changed the title ~~[WIP] Hardware-wallet/usb-subscribe-refactor~~ Hardware-wallet/usb-subscribe-refactor Feb 14, 2018

5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Feb 22, 2018

rphmeier reviewed Feb 22, 2018

View reviewed changes

niklasad1 added 5 commits February 22, 2018 22:59

Validate product_id of detected ledger devices

cdd58ad

closed_device => unlocked_device

76baa27

address comments

984065c

add target in debug

cbad867

Address feedback

aaff934

* Remove thread joining in HardwareWalletManager * Remove thread handlers in HardwareWalletManager because this makes them unused

niklasad1 force-pushed the hardware-wallet/usb_fix branch from 3317ba3 to aaff934 Compare February 22, 2018 22:23

niklasad1 mentioned this pull request Feb 27, 2018

investigate if thread::sleep is really needed in hw module #7793

Closed

rphmeier approved these changes Feb 27, 2018

View reviewed changes

rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 27, 2018

debris merged commit 6445f12 into openethereum:master Feb 27, 2018

debris added B0-patch B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. labels Feb 27, 2018

tomusdrw mentioned this pull request Feb 27, 2018

[beta] Backports #8011

Merged

10 tasks

niklasad1 deleted the hardware-wallet/usb_fix branch February 28, 2018 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hardware-wallet/usb-subscribe-refactor #7860

Hardware-wallet/usb-subscribe-refactor #7860

niklasad1 commented Feb 11, 2018 •

edited

Loading

parity-cla-bot commented Feb 11, 2018

niklasad1 commented Feb 11, 2018

parity-cla-bot commented Feb 11, 2018

rphmeier Feb 12, 2018

rphmeier Feb 12, 2018

rphmeier Feb 12, 2018

rphmeier Feb 12, 2018

rphmeier Feb 12, 2018

rphmeier Feb 12, 2018

rphmeier Feb 12, 2018

rphmeier Feb 12, 2018

rphmeier Feb 12, 2018

rphmeier Feb 12, 2018

rphmeier Feb 12, 2018

niklasad1 Feb 12, 2018 •

edited

Loading

rphmeier Feb 12, 2018

niklasad1 Feb 12, 2018

rphmeier Feb 12, 2018

niklasad1 Feb 12, 2018 •

edited

Loading

rphmeier commented Feb 12, 2018

niklasad1 commented Feb 13, 2018 •

edited

Loading

niklasad1 commented Feb 22, 2018

rphmeier Feb 22, 2018

niklasad1 Feb 22, 2018 •

edited

Loading

rphmeier Feb 22, 2018

niklasad1 commented Feb 22, 2018

rphmeier commented Feb 27, 2018

niklasad1 commented Feb 28, 2018 •

edited

Loading

Hardware-wallet/usb-subscribe-refactor #7860

Hardware-wallet/usb-subscribe-refactor #7860

Conversation

niklasad1 commented Feb 11, 2018 • edited Loading

parity-cla-bot commented Feb 11, 2018

niklasad1 commented Feb 11, 2018

parity-cla-bot commented Feb 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

rphmeier commented Feb 12, 2018

niklasad1 commented Feb 13, 2018 • edited Loading

niklasad1 commented Feb 22, 2018

Choose a reason for hiding this comment

niklasad1 Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 commented Feb 22, 2018

rphmeier commented Feb 27, 2018

niklasad1 commented Feb 28, 2018 • edited Loading

niklasad1 commented Feb 11, 2018 •

edited

Loading

niklasad1 Feb 12, 2018 •

edited

Loading

niklasad1 Feb 12, 2018 •

edited

Loading

niklasad1 commented Feb 13, 2018 •

edited

Loading

niklasad1 Feb 22, 2018 •

edited

Loading

niklasad1 commented Feb 28, 2018 •

edited

Loading