refactor (hardware wallet) : reduce the number of threads by niklasad1 · Pull Request #9644 · openethereum/parity-ethereum

niklasad1 · 2018-09-25T08:56:58Z

Attempt to close #9622 along with fixing some minor formatting things!

It introduces some boiler-plate code though such as:

Some errors are duplicated in trezor and ledger but I think these makes sense because it indicate the errors more clearly
Device arrived and left methods
Checking the device is valid
Fixed the Ledger logs which was faulty (added inline comments for more info)

Tested successfully both on Trezor and Ledger on Linux

Proof that number of threads have been reduced:

# This branch
$  ps hH p 19919 | wc -l
57
# master
$ ps hH p 23329 | wc -l 
58

niklasad1 · 2018-09-25T08:59:11Z

-	pub fn new(hidapi: Arc<Mutex<hidapi::HidApi>>, exiting: Arc<AtomicBool>) -> Result<Arc<Self>, libusb::Error> {
-		let manager = Arc::new(Self {
-			usb: hidapi,
+	pub fn new(usb: Arc<Mutex<hidapi::HidApi>>) -> Arc<Self> {


doesn't make sense that this function returns Result anymore because it can't fail anymore

niklasad1 · 2018-09-25T08:59:54Z

+		})
 	}

 	// Transport Protocol:


fix formatting!!!

5chdn · 2018-10-26T11:27:10Z

@niklasad1 what's the status of this? Do you want feedback from @dvdplm @debris ?

niklasad1 · 2018-10-26T11:30:21Z

@5chdn Need grab to hardware wallets and test this first. I will change the label when it is good for reviewing hopefully later today

sorpaas

LGTM!

5chdn · 2018-12-05T09:37:56Z

👍 needs a 2nd review

dvdplm

A few nits, but overall looks good.

dvdplm · 2018-12-14T13:55:35Z

+/// Hardware wallet event handler
+///
+/// Note, that this run to completion and race-conditions can't occur but this can
+/// therefore starve other events for being process with a spinlock or similar


Suggested change

/// therefore starve other events for being process with a spinlock or similar

/// stop other events from being processed with a spinlock or similar.

(but I'm not sure what you mean by "with spinlock or similar" tbh)

Spinlock is a bit blurry in this context, should probably say spinning instead.

I mean that if the thread is in an infinite loop or looping for a condition it will prevent other events from being processed. I can rephrase it but not really introduced in this PR

dvdplm · 2018-12-14T14:00:53Z

+impl libusb::Hotplug for EventHandler {
+	fn device_arrived(&mut self, device: libusb::Device) {
+		// Upgrade reference to an Arc
+		if let (Some(ledger), Some(trezor)) = (self.ledger.upgrade(), self.trezor.upgrade()) {


q: is it impossible to have one upgrade call return Some and the other None? Wouldn't that cause the whole block to be skipped? In other words, are both upgrade() calls going to return Some if one of them does?

Good question, because the HardwareWalletManager keeps an Arc for both Ledger and Trezor for the entire lifetime of Parity unless it is disabled by the cli arg --no-hardware-wallet! upgrade()will always succeed as long theHardwareWalletManageris enabled (the strong count will be at least 1). So, either bothTrezorandLedger` is enabled or none is enabled.

However, I admit it is a bit sloppy. Theoretically, interleaving "could" happen if one of the hw-wallets has been dropped and the other has not been dropped but this is matter in granularity in terms of microseconds and I don't worry too much about this.

I can change it if you like? :P

dvdplm · 2018-12-14T14:11:11Z

+
+	fn device_left(&mut self, device: libusb::Device) {
+		// Upgrade reference to an Arc
+		if let (Some(ledger), Some(trezor)) = (self.ledger.upgrade(), self.trezor.upgrade()) {


Same question here.

5chdn · 2019-01-02T10:17:38Z

Please resolve conflicts :)

This changes parity to use one thread instead of two to subscribe USB events via the hardware-wallets.

Co-Authored-By: niklasad1 <niklasadolfsson1@gmail.com>

* Clarify bad explaination of `run-to-completion` * Replace magic numbers with constants

niklasad1 added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Sep 25, 2018

niklasad1 commented Sep 25, 2018

View reviewed changes

Comment thread hw/src/ledger.rs

})

}

// Transport Protocol:

Copy link
Copy Markdown

Collaborator Author

niklasad1 Sep 25, 2018

Choose a reason for hiding this comment

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

fix formatting!!!

5chdn added this to the 2.2 milestone Sep 25, 2018

5chdn added the M4-core ⛓ Core client code / Rust. label Sep 25, 2018

5chdn requested review from debris and dvdplm October 26, 2018 11:26

5chdn added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 26, 2018

niklasad1 force-pushed the refactor/hardware-wallet-one-thread branch from 013abd3 to aacb057 Compare October 27, 2018 09:25

niklasad1 added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. labels Oct 28, 2018

niklasad1 force-pushed the refactor/hardware-wallet-one-thread branch from 334c6fb to c26a70f Compare October 28, 2018 20:34

5chdn modified the milestones: 2.2, 2.3 Oct 29, 2018

sorpaas approved these changes Nov 25, 2018

View reviewed changes

Comment thread hw/src/ledger.rs Outdated

Comment thread hw/src/lib.rs Outdated

sorpaas added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 25, 2018

niklasad1 force-pushed the refactor/hardware-wallet-one-thread branch from c26a70f to 79ba32d Compare November 25, 2018 21:16

dvdplm approved these changes Dec 14, 2018

View reviewed changes

niklasad1 force-pushed the refactor/hardware-wallet-one-thread branch 3 times, most recently from 09649d6 to fa2324a Compare December 16, 2018 20:13

niklasad1 removed the request for review from debris December 16, 2018 20:14

niklasad1 force-pushed the refactor/hardware-wallet-one-thread branch from fa2324a to f099f3d Compare December 17, 2018 09:39

niklasad1 added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Dec 17, 2018

niklasad1 and others added 7 commits January 2, 2019 14:18

refactor(hardware-wallet) - use only one thread

b455fcf

This changes parity to use one thread instead of two to subscribe USB events via the hardware-wallets.

Fix logs and tests

1f16984

fix(grumbles) : formatting and spelling nits

2b20a2b

Update hw/src/lib.rs

0b6b7ae

Co-Authored-By: niklasad1 <niklasadolfsson1@gmail.com>

Update hw/src/lib.rs

75f87f8

Co-Authored-By: niklasad1 <niklasadolfsson1@gmail.com>

Update hw/src/ledger.rs

8f152b9

Co-Authored-By: niklasad1 <niklasadolfsson1@gmail.com>

fix(grumbles): clarify docs and use constants

0329802

* Clarify bad explaination of `run-to-completion` * Replace magic numbers with constants

niklasad1 force-pushed the refactor/hardware-wallet-one-thread branch from f099f3d to 0329802 Compare January 2, 2019 13:19

5chdn merged commit 2bb7961 into master Jan 2, 2019

5chdn deleted the refactor/hardware-wallet-one-thread branch January 2, 2019 15:49

	/// therefore starve other events for being process with a spinlock or similar
	/// stop other events from being processed with a spinlock or similar.

Conversation

niklasad1 commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niklasad1 Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasad1 Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

5chdn commented Oct 26, 2018

Uh oh!

niklasad1 commented Oct 26, 2018

Uh oh!

sorpaas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

5chdn commented Dec 5, 2018

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dvdplm Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

niklasad1 Dec 15, 2018

Choose a reason for hiding this comment

Uh oh!

dvdplm Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

niklasad1 Dec 15, 2018

Choose a reason for hiding this comment

Uh oh!

dvdplm Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

5chdn commented Jan 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

niklasad1 commented Sep 25, 2018 •

edited

Loading

niklasad1 Sep 25, 2018 •

edited

Loading