Have Trezor dialog work even if wallet unpaired

Required cleanup of handler logic.  Now every client
is constructed with a handler, so there is never a
question of not having one.

lib/plugins.py

@@ -218,102 +218,117 @@ class BasePlugin(PrintError):
 
 class DeviceMgr(PrintError):
     '''Manages hardware clients.  A client communicates over a hardware
-    channel with the device.  A client is a pair: a device ID (serial
-    number) and hardware port.  If either change then a different
-    client is instantiated.
+    channel with the device.
 
-    In addition to tracking device IDs, the device manager tracks
-    hardware wallets and manages wallet pairing.  A device ID may be
+    In addition to tracking device HID IDs, the device manager tracks
+    hardware wallets and manages wallet pairing.  A HID ID may be
     paired with a wallet when it is confirmed that the hardware device
     matches the wallet, i.e. they have the same master public key.  A
-    device ID can be unpaired if e.g. it is wiped.
+    HID ID can be unpaired if e.g. it is wiped.
 
     Because of hotplugging, a wallet must request its client
     dynamically each time it is required, rather than caching it
     itself.
 
     The device manager is shared across plugins, so just one place
-    does hardware scans when needed.  By tracking device serial
-    numbers the number of necessary hardware scans is reduced, e.g. if
-    a device is plugged into a different port the wallet is
-    automatically re-paired.
+    does hardware scans when needed.  By tracking HID IDs, if a device
+    is plugged into a different port the wallet is automatically
+    re-paired.
 
     Wallets are informed on connect / disconnect events.  It must
     implement connected(), disconnected() callbacks.  Being connected
     implies a pairing.  Callbacks can happen in any thread context,
     and we do them without holding the lock.
 
-    This plugin is thread-safe.  Currently only USB is implemented.'''
+    Confusingly, the HID ID (serial number) reported by the HID system
+    doesn't match the device ID reported by the device itself.  We use
+    the HID IDs.
 
-    # Client lookup types.  CACHED will look up in our client cache
-    # only.  PRESENT will do a scan if there is no client in the cache.
-    # PAIRED will try and pair the wallet, which will involve requesting
-    # a PIN and passphrase if they are enabled
-    (CACHED, PRESENT, PAIRED) = range(3)
+    This plugin is thread-safe.  Currently only devices supported by
+    hidapi are implemented.
+
+    '''
 
     def __init__(self):
         super(DeviceMgr, self).__init__()
-        # Keyed by wallet.  The value is the device_id if the wallet
-        # has been paired, and None otherwise.
+        # Keyed by wallet.  The value is the hid_id if the wallet has
+        # been paired, and None otherwise.
         self.wallets = {}
         # A list of clients.  We create a client for every device present
         # that is of a registered hardware type
         self.clients = []
         # What we recognise.  Keyed by (vendor_id, product_id) pairs,
-        # the value is a handler for those devices.  The handler must
-        # implement
+        # the value is a callback to create a client for those devices
         self.recognised_hardware = {}
         # For synchronization
         self.lock = threading.RLock()
 
-    def register_devices(self, handler, device_pairs):
+    def register_devices(self, device_pairs, create_client):
         for pair in device_pairs:
-            self.recognised_hardware[pair] = handler
+            self.recognised_hardware[pair] = create_client
+
+    def unpair(self, hid_id):
+        with self.lock:
+            wallet = self.wallet_by_hid_id(hid_id)
+            if wallet:
+                self.wallets[wallet] = None
 
     def close_client(self, client):
         with self.lock:
             if client in self.clients:
                 self.clients.remove(client)
+        if client:
             client.close()
 
     def close_wallet(self, wallet):
         # Remove the wallet from our list; close any client
         with self.lock:
-            device_id = self.wallets.pop(wallet, None)
-            self.close_client(self.client_by_device_id(device_id))
+            hid_id = self.wallets.pop(wallet, None)
+            self.close_client(self.client_by_hid_id(hid_id))
 
-    def clients_of_type(self, classinfo):
+    def unpaired_clients(self, handler, classinfo):
+        '''Returns all unpaired clients of the given type.'''
+        self.scan_devices(handler)
         with self.lock:
             return [client for client in self.clients
-                    if isinstance(client, classinfo)]
+                    if isinstance(client, classinfo)
+                    and not self.wallet_by_hid_id(client.hid_id())]
 
-    def client_by_device_id(self, device_id):
+    def client_by_hid_id(self, hid_id, handler=None):
+        '''Like get_client() but when we don't care about wallet pairing.  If
+        a device is wiped or in bootloader mode pairing is impossible;
+        in such cases we communicate by device ID and not wallet.'''
+        if handler:
+            self.scan_devices(handler)
         with self.lock:
             for client in self.clients:
-                if client.device_id() == device_id:
+                if client.hid_id() == hid_id:
                     return client
             return None
 
-    def wallet_by_device_id(self, device_id):
+    def wallet_hid_id(self, wallet):
+        with self.lock:
+            return self.wallets.get(wallet)
+
+    def wallet_by_hid_id(self, hid_id):
         with self.lock:
-            for wallet, wallet_device_id in self.wallets.items():
-                if wallet_device_id == device_id:
+            for wallet, wallet_hid_id in self.wallets.items():
+                if wallet_hid_id == hid_id:
                     return wallet
             return None
 
     def paired_wallets(self):
         with self.lock:
-            return [wallet for (wallet, device_id) in self.wallets.items()
-                    if device_id is not None]
+            return [wallet for (wallet, hid_id) in self.wallets.items()
+                    if hid_id is not None]
 
     def pair_wallet(self, wallet, client):
         assert client in self.clients
         self.print_error("paired:", wallet, client)
-        self.wallets[wallet] = client.device_id()
-        client.pair_wallet(wallet)
+        self.wallets[wallet] = client.hid_id()
         wallet.connected()
 
-    def scan_devices(self):
+    def scan_devices(self, handler):
         # All currently supported hardware libraries use hid, so we
         # assume it here.  This can be easily abstracted if necessary.
         # Note this import must be local so those without hardware
@@ -326,29 +341,26 @@ class DeviceMgr(PrintError):
         devices = {}
         for d in hid.enumerate(0, 0):
             product_key = (d['vendor_id'], d['product_id'])
-            device_id = d['serial_number']
-            path = d['path']
-
-            handler = self.recognised_hardware.get(product_key)
-            if handler:
-                devices[device_id] = (handler, path, product_key)
+            create_client = self.recognised_hardware.get(product_key)
+            if create_client:
+                devices[d['serial_number']] = (create_client, d['path'])
 
         # Now find out what was disconnected
         with self.lock:
             disconnected = [client for client in self.clients
-                            if not client.device_id() in devices]
+                            if not client.hid_id() in devices]
 
         # Close disconnected clients after informing their wallets
         for client in disconnected:
-            wallet = self.wallet_by_device_id(client.device_id())
+            wallet = self.wallet_by_hid_id(client.hid_id())
             if wallet:
                 wallet.disconnected()
             self.close_client(client)
 
         # Now see if any new devices are present.
-        for device_id, (handler, path, product_key) in devices.items():
+        for hid_id, (create_client, path) in devices.items():
             try:
-                client = handler.create_client(path, product_key)
+                client = create_client(path, handler, hid_id)
             except BaseException as e:
                 self.print_error("could not create client", str(e))
                 client = None
@@ -357,21 +369,26 @@ class DeviceMgr(PrintError):
                 with self.lock:
                     self.clients.append(client)
                 # Inform re-paired wallet
-                wallet = self.wallet_by_device_id(device_id)
+                wallet = self.wallet_by_hid_id(hid_id)
                 if wallet:
                     self.pair_wallet(wallet, client)
 
-    def get_client(self, wallet, lookup=PAIRED):
-        '''Returns a client for the wallet, or None if one could not be
-        found.'''
-        with self.lock:
-            device_id = self.wallets.get(wallet)
-            client = self.client_by_device_id(device_id)
-            if client:
-                return client
-
-        if lookup == DeviceMgr.CACHED:
-            return None
+    def get_client(self, wallet, force_pair=True):
+        '''Returns a client for the wallet, or None if one could not be found.
+        If force_pair is False then if an already paired client cannot
+        be found None is returned rather than requiring user
+        interaction.'''
+        # We must scan devices to get an up-to-date idea of which
+        # devices are present.  Operating on a client when its device
+        # has been removed can cause the process to hang.
+        # Unfortunately there is no plugged / unplugged notification
+        # system.
+        self.scan_devices(wallet.handler)
+
+        # Previously paired wallets only need look for matching HID IDs
+        hid_id = self.wallet_hid_id(wallet)
+        if hid_id:
+            return self.client_by_hid_id(hid_id)
 
         first_address, derivation = wallet.first_address()
         # Wallets don't have a first address in the install wizard
@@ -380,29 +397,15 @@ class DeviceMgr(PrintError):
             self.print_error("no first address for ", wallet)
             return None
 
-        # We didn't find it, so scan for new devices.  We scan as
-        # little as possible: some people report a USB scan is slow on
-        # Linux when a Trezor is plugged in
-        self.scan_devices()
-
         with self.lock:
-            # Maybe the scan found it?  If the wallet has a device_id
-            # from a prior pairing, we can determine success now.
-            if device_id:
-                return self.client_by_device_id(device_id)
-
-            # Stop here if no wake and we couldn't find it.
-            if lookup == DeviceMgr.PRESENT:
-                return None
-
             # The wallet has not been previously paired, so get the
             # first address of all unpaired clients and compare.
             for client in self.clients:
                 # If already paired skip it
-                if self.wallet_by_device_id(client.device_id()):
+                if self.wallet_by_hid_id(client.hid_id()):
                     continue
                 # This will trigger a PIN/passphrase entry request
-                if client.first_address(wallet, derivation) == first_address:
+                if client.first_address(derivation) == first_address:
                     self.pair_wallet(wallet, client)
                     return client
 

plugins/trezor/client.py

@@ -29,7 +29,7 @@ class GuiMixin(object):
         else:
             cancel_callback = None
 
-        self.handler().show_message(message % self.device, cancel_callback)
+        self.handler.show_message(message % self.device, cancel_callback)
         return self.proto.ButtonAck()
 
     def callback_PinMatrixRequest(self, msg):
@@ -42,21 +42,21 @@ class GuiMixin(object):
                      "Note the numbers have been shuffled!"))
         else:
             msg = _("Please enter %s PIN")
-        pin = self.handler().get_pin(msg % self.device)
+        pin = self.handler.get_pin(msg % self.device)
         if not pin:
             return self.proto.Cancel()
         return self.proto.PinMatrixAck(pin=pin)
 
     def callback_PassphraseRequest(self, req):
         msg = _("Please enter your %s passphrase")
-        passphrase = self.handler().get_passphrase(msg % self.device)
+        passphrase = self.handler.get_passphrase(msg % self.device)
         if passphrase is None:
             return self.proto.Cancel()
         return self.proto.PassphraseAck(passphrase=passphrase)
 
     def callback_WordRequest(self, msg):
         msg = _("Enter seed word as explained on your %s") % self.device
-        word = self.handler().get_word(msg)
+        word = self.handler.get_word(msg)
         if word is None:
             return self.proto.Cancel()
         return self.proto.WordAck(word=word)
@@ -67,39 +67,31 @@ def trezor_client_class(protocol_mixin, base_client, proto):
 
     class TrezorClient(protocol_mixin, GuiMixin, base_client, PrintError):
 
-        def __init__(self, transport, path, plugin):
+        def __init__(self, transport, handler, plugin, hid_id):
             base_client.__init__(self, transport)
             protocol_mixin.__init__(self, transport)
             self.proto = proto
             self.device = plugin.device
-            self.path = path
-            self.wallet = None
-            self.plugin = plugin
+            self.handler = handler
+            self.hid_id_ = hid_id
             self.tx_api = plugin
             self.msg_code_override = None
 
         def __str__(self):
-            return "%s/%s/%s" % (self.label(), self.device_id(), self.path)
+            return "%s/%s" % (self.label(), self.hid_id())
 
         def label(self):
             '''The name given by the user to the device.'''
             return self.features.label
 
-        def device_id(self):
-            '''The device serial number.'''
-            return self.features.device_id
+        def hid_id(self):
+            '''The HID ID of the device.'''
+            return self.hid_id_
 
         def is_initialized(self):
             '''True if initialized, False if wiped.'''
             return self.features.initialized
 
-        def pair_wallet(self, wallet):
-            self.wallet = wallet
-
-        def handler(self):
-            assert self.wallet and self.wallet.handler
-            return self.wallet.handler
-
         # Copied from trezorlib/client.py as there it is not static, sigh
         @staticmethod
         def expand_path(n):
@@ -116,14 +108,8 @@ def trezor_client_class(protocol_mixin, base_client, proto):
                 path.append(abs(int(x)) | prime)
             return path
 
-        def first_address(self, wallet, derivation):
-            assert not self.wallet
-            # Assign the wallet so we have a handler
-            self.wallet = wallet
-            try:
+        def first_address(self, derivation):
             return self.address_from_derivation(derivation)
-            finally:
-                self.wallet = None
 
         def address_from_derivation(self, derivation):
             return self.get_address('Bitcoin', self.expand_path(derivation))
@@ -188,14 +174,13 @@ def trezor_client_class(protocol_mixin, base_client, proto):
         any dialog box it opened.'''
 
         def wrapped(self, *args, **kwargs):
-            handler = self.handler()
             try:
                 return func(self, *args, **kwargs)
             except BaseException as e:
-                handler.show_error(str(e))
+                self.handler.show_error(str(e))
                 raise e
             finally:
-                handler.finished()
+                self.handler.finished()
 
         return wrapped
 

plugins/trezor/plugin.py

@@ -35,15 +35,13 @@ class TrezorCompatibleWallet(BIP44_Wallet):
 
     def __init__(self, storage):
         BIP44_Wallet.__init__(self, storage)
-        # This is set when paired with a device, and used to re-pair
-        # a device that is disconnected and re-connected
-        self.device_id = None
         # After timeout seconds we clear the device session
         self.session_timeout = storage.get('session_timeout', 180)
         # Errors and other user interaction is done through the wallet's
         # handler.  The handler is per-window and preserved across
         # device reconnects
         self.handler = None
+        self.force_watching_only = True
 
     def set_session_timeout(self, seconds):
         self.print_error("setting session timeout to %d seconds" % seconds)
@@ -54,12 +52,14 @@ class TrezorCompatibleWallet(BIP44_Wallet):
         '''A device paired with the wallet was diconnected.  Note this is
         called in the context of the Plugins thread.'''
         self.print_error("disconnected")
+        self.force_watching_only = True
         self.handler.watching_only_changed()
 
     def connected(self):
         '''A device paired with the wallet was (re-)connected.  Note this
         is called in the context of the Plugins thread.'''
         self.print_error("connected")
+        self.force_watching_only = False
         self.handler.watching_only_changed()
 
     def timeout(self):
@@ -77,17 +77,15 @@ class TrezorCompatibleWallet(BIP44_Wallet):
         return False
 
     def is_watching_only(self):
-        '''The wallet is watching-only if its trezor device is not connected,
-        or if it is connected but uninitialized.'''
+        '''The wallet is watching-only if its trezor device is unpaired.'''
         assert not self.has_seed()
-        client = self.get_client(DeviceMgr.CACHED)
-        return not (client and client.is_initialized())
+        return self.force_watching_only
 
     def can_change_password(self):
         return False
 
-    def get_client(self, lookup=DeviceMgr.PAIRED):
-        return self.plugin.get_client(self, lookup)
+    def get_client(self, force_pair=True):
+        return self.plugin.get_client(self, force_pair)
 
     def first_address(self):
         '''Used to check a hardware wallet matches a software wallet'''
@@ -170,7 +168,8 @@ class TrezorCompatiblePlugin(BasePlugin, ThreadJob):
         self.wallet_class.plugin = self
         self.prevent_timeout = time.time() + 3600 * 24 * 365
         if self.libraries_available:
-            self.device_manager().register_devices(self, self.DEVICE_IDS)
+            self.device_manager().register_devices(
+                self.DEVICE_IDS, self.create_client)
 
     def is_enabled(self):
         return self.libraries_available
@@ -190,13 +189,13 @@ class TrezorCompatiblePlugin(BasePlugin, ThreadJob):
             if (isinstance(wallet, self.wallet_class)
                     and hasattr(wallet, 'last_operation')
                     and now > wallet.last_operation + wallet.session_timeout):
-                client = self.get_client(wallet, DeviceMgr.CACHED)
+                client = self.get_client(wallet, force_pair=False)
                 if client:
-                    wallet.last_operation = self.prevent_timeout
                     client.clear_session()
+                    wallet.last_operation = self.prevent_timeout
                     wallet.timeout()
 
-    def create_client(self, path, product_key):
+    def create_client(self, path, handler, hid_id):
         pair = ((None, path) if self.HidTransport._detect_debuglink(path)
                 else (path, None))
         try:
@@ -206,14 +205,14 @@ class TrezorCompatiblePlugin(BasePlugin, ThreadJob):
             self.print_error("cannot connect at", path, str(e))
             return None
         self.print_error("connected to device at", path)
-        return self.client_class(transport, path, self)
+        return self.client_class(transport, handler, self, hid_id)
 
-    def get_client(self, wallet, lookup=DeviceMgr.PAIRED, check_firmware=True):
-        '''check_firmware is ignored unless doing a PAIRED lookup.'''
-        client = self.device_manager().get_client(wallet, lookup)
+    def get_client(self, wallet, force_pair=True, check_firmware=True):
+        '''check_firmware is ignored unless force_pair is True.'''
+        client = self.device_manager().get_client(wallet, force_pair)
 
-        # Try a ping if doing at least a PRESENT lookup
-        if client and lookup != DeviceMgr.CACHED:
+        # Try a ping for device sanity
+        if client:
             self.print_error("set last_operation")
             wallet.last_operation = time.time()
             try:
@@ -224,7 +223,7 @@ class TrezorCompatiblePlugin(BasePlugin, ThreadJob):
                 self.device_manager().close_client(client)
                 client = None
 
-        if lookup == DeviceMgr.PAIRED:
+        if force_pair:
             assert wallet.handler
             if not client:
                 msg = (_('Could not connect to your %s.  Verify the '
@@ -295,19 +294,25 @@ class TrezorCompatiblePlugin(BasePlugin, ThreadJob):
             client.load_device_by_xprv(item, pin, passphrase_protection,
                                        label, language)
 
+    def unpaired_clients(self, handler):
+        '''Returns all connected, unpaired devices as a list of clients and a
+        list of descriptions.'''
+        devmgr = self.device_manager()
+        clients = devmgr.unpaired_clients(handler, self.client_class)
+        states = [_("wiped"), _("initialized")]
+        def client_desc(client):
+            label = client.label() or _("An unnamed device")
+            state = states[client.is_initialized()]
+            return ("%s: serial number %s (%s)"
+                    % (label, client.hid_id(), state))
+        return clients, list(map(client_desc, clients))
+
     def select_device(self, wallet):
         '''Called when creating a new wallet.  Select the device to use.  If
         the device is uninitialized, go through the intialization
         process.'''
-        self.device_manager().scan_devices()
-        clients = self.device_manager().clients_of_type(self.client_class)
-        suffixes = [_(" (wiped)"), _(" (initialized)")]
-        def client_desc(client):
-            label = client.label() or _("An unnamed device")
-            return label + suffixes[client.is_initialized()]
-        labels = list(map(client_desc, clients))
-
         msg = _("Please select which %s device to use:") % self.device
+        clients, labels = self.unpaired_clients(wallet.handler)
         client = clients[wallet.handler.query_choice(msg, labels)]
         self.device_manager().pair_wallet(wallet, client)
         if not client.is_initialized():

plugins/trezor/qt_generic.py

@@ -261,29 +261,69 @@ def qt_plugin_class(base_plugin_class):
                            lambda: self.show_address(wallet, addrs[0]))
 
     def settings_dialog(self, window):
-        dialog = SettingsDialog(window, self)
+        hid_id = self.choose_device(window)
+        if hid_id:
+            dialog = SettingsDialog(window, self, hid_id)
             window.wallet.handler.exec_dialog(dialog)
 
+    def choose_device(self, window):
+        '''This dialog box should be usable even if the user has
+        forgotten their PIN or it is in bootloader mode.'''
+        handler = window.wallet.handler
+        hid_id = self.device_manager().wallet_hid_id(window.wallet)
+        if not hid_id:
+            clients, labels = self.unpaired_clients(handler)
+            if clients:
+                msg = _("Select a %s device:") % self.device
+                choice = self.query_choice(window, msg, labels)
+                if choice is not None:
+                    hid_id = clients[choice].hid_id()
+            else:
+                handler.show_error(_("No devices found"))
+        return hid_id
+
+    def query_choice(self, window, msg, choices):
+        dialog = WindowModalDialog(window)
+        clayout = ChoicesLayout(msg, choices)
+        layout = clayout.layout()
+        layout.addStretch(1)
+        layout.addLayout(Buttons(CancelButton(dialog), OkButton(dialog)))
+        dialog.setLayout(layout)
+        if not dialog.exec_():
+            return None
+        return clayout.selected_index()
+
+
   return QtPlugin
 
 
 class SettingsDialog(WindowModalDialog):
+    '''This dialog doesn't require a device be paired with a wallet.
+    We want users to be able to wipe a device even if they've forgotten
+    their PIN.'''
 
-    def __init__(self, window, plugin):
-        self.plugin = plugin
-        self.window = window  # The main electrum window
+    def __init__(self, window, plugin, hid_id):
         title = _("%s Settings") % plugin.device
         super(SettingsDialog, self).__init__(window, title)
         self.setMaximumWidth(540)
+
+        devmgr = plugin.device_manager()
+        handler = window.wallet.handler
+        # wallet can be None, needn't be window.wallet
+        wallet = devmgr.wallet_by_hid_id(hid_id)
         hs_rows, hs_cols = (64, 128)
 
-        def get_client(lookup=DeviceMgr.PAIRED):
-            return self.plugin.get_client(wallet, lookup)
+        def get_client():
+            client = devmgr.client_by_hid_id(hid_id, handler)
+            if not client:
+                self.show_error("Device not connected!")
+                raise RuntimeError("Device not connected")
+            return client
 
         def update():
-            features = get_client(DeviceMgr.PAIRED).features
-            self.features = features
-            # The above was for outer scopes.  Now the real logic.
+            # self.features for outer scopes
+            client = get_client()
+            features = self.features = client.features
             set_label_enabled()
             bl_hash = features.bootloader_hash.encode('hex')
             bl_hash = "\n".join([bl_hash[:32], bl_hash[32:]])
@@ -301,6 +341,7 @@ class SettingsDialog(WindowModalDialog):
             bl_hash_label.setText(bl_hash)
             label_edit.setText(features.label)
             device_id_label.setText(features.device_id)
+            serial_number_label.setText(client.hid_id())
             initialized_label.setText(noyes[features.initialized])
             version_label.setText(version)
             coins_label.setText(coins)
@@ -309,7 +350,6 @@ class SettingsDialog(WindowModalDialog):
             pin_button.setText(setchange[features.pin_protection])
             pin_msg.setVisible(not features.pin_protection)
             passphrase_button.setText(endis[features.passphrase_protection])
-
             language_label.setText(features.language)
 
         def set_label_enabled():
@@ -331,7 +371,7 @@ class SettingsDialog(WindowModalDialog):
             if not self.question(msg, title=title):
                 return
             get_client().toggle_passphrase()
-            self.device_manager().close_wallet(wallet)  # Unpair
+            devmgr.unpair(hid_id)
             update()
 
         def change_homescreen():
@@ -362,27 +402,25 @@ class SettingsDialog(WindowModalDialog):
             set_pin(remove=True)
 
         def wipe_device():
-            # FIXME: cannot yet wipe a device that is only plugged in
-            if sum(wallet.get_balance()):
+            if wallet and sum(wallet.get_balance()):
                 title = _("Confirm Device Wipe")
                 msg = _("Are you SURE you want to wipe the device?\n"
                         "Your wallet still has bitcoins in it!")
                 if not self.question(msg, title=title,
                                      icon=QMessageBox.Critical):
                     return
-            # Note: we use PRESENT so that a user who has forgotten
-            # their PIN is not prevented from wiping their device
-            get_client(DeviceMgr.PRESENT).wipe_device()
-            self.device_manager().close_wallet(wallet)
+            get_client().wipe_device()
+            devmgr.unpair(hid_id)
             update()
 
         def slider_moved():
             mins = timeout_slider.sliderPosition()
             timeout_minutes.setText(_("%2d minutes") % mins)
 
-        wallet = window.wallet
-        handler = wallet.handler
-        device = plugin.device
+        def slider_released():
+            seconds = timeout_slider.sliderPosition() * 60
+            wallet.set_session_timeout(seconds)
+
         dialog_vbox = QVBoxLayout(self)
 
         # Information tab
@@ -394,6 +432,7 @@ class SettingsDialog(WindowModalDialog):
         pin_set_label = QLabel()
         version_label = QLabel()
         device_id_label = QLabel()
+        serial_number_label = QLabel()
         bl_hash_label = QLabel()
         bl_hash_label.setWordWrap(True)
         coins_label = QLabel()
@@ -404,7 +443,8 @@ class SettingsDialog(WindowModalDialog):
             (_("Device Label"), device_label),
             (_("PIN set"), pin_set_label),
             (_("Firmware Version"), version_label),
-            (_("Serial Number"), device_id_label),
+            (_("Device ID"), device_id_label),
+            (_("Serial Number"), serial_number_label),
             (_("Bootloader Hash"), bl_hash_label),
             (_("Supported Coins"), coins_label),
             (_("Language"), language_label),
@@ -419,7 +459,6 @@ class SettingsDialog(WindowModalDialog):
         settings_tab = QWidget()
         settings_layout = QVBoxLayout(settings_tab)
         settings_glayout = QGridLayout()
-        #settings_glayout.setColumnStretch(3, 1)
 
         # Settings tab - Label
         label_msg = QLabel(_("Name this %s.  If you have mutiple devices "
@@ -429,7 +468,7 @@ class SettingsDialog(WindowModalDialog):
         label_label = QLabel(_("Device Label"))
         label_edit = QLineEdit()
         label_edit.setMinimumWidth(150)
-        label_edit.setMaxLength(self.plugin.MAX_LABEL_LEN)
+        label_edit.setMaxLength(plugin.MAX_LABEL_LEN)
         label_apply = QPushButton(_("Apply"))
         label_apply.clicked.connect(rename)
         label_edit.textChanged.connect(set_label_enabled)
@@ -451,7 +490,6 @@ class SettingsDialog(WindowModalDialog):
         pin_msg.setWordWrap(True)
         pin_msg.setStyleSheet("color: red")
         settings_glayout.addWidget(pin_msg, 3, 1, 1, -1)
-        settings_layout.addLayout(settings_glayout)
 
         # Settings tab - Homescreen
         homescreen_layout = QHBoxLayout()
@@ -471,25 +509,31 @@ class SettingsDialog(WindowModalDialog):
         settings_glayout.addWidget(homescreen_msg, 5, 1, 1, -1)
 
         # Settings tab - Session Timeout
+        if wallet:
             timeout_label = QLabel(_("Session Timeout"))
             timeout_minutes = QLabel()
-        timeout_slider = self.slider = QSlider(Qt.Horizontal)
+            timeout_slider = QSlider(Qt.Horizontal)
             timeout_slider.setRange(1, 60)
             timeout_slider.setSingleStep(1)
-        timeout_slider.setSliderPosition(wallet.session_timeout // 60)
             timeout_slider.setTickInterval(5)
             timeout_slider.setTickPosition(QSlider.TicksBelow)
             timeout_slider.setTracking(True)
-        timeout_slider.valueChanged.connect(slider_moved)
-        timeout_msg = QLabel(_("Clear the session after the specified period "
+            timeout_msg = QLabel(
+                _("Clear the session after the specified period "
                   "of inactivity.  Once a session has timed out, "
                   "your PIN and passphrase (if enabled) must be "
                   "re-entered to use the device."))
             timeout_msg.setWordWrap(True)
+            timeout_slider.setSliderPosition(wallet.session_timeout // 60)
+            slider_moved()
+            timeout_slider.valueChanged.connect(slider_moved)
+            timeout_slider.sliderReleased.connect(slider_released)
             settings_glayout.addWidget(timeout_label, 6, 0)
             settings_glayout.addWidget(timeout_slider, 6, 1, 1, 3)
             settings_glayout.addWidget(timeout_minutes, 6, 4)
             settings_glayout.addWidget(timeout_msg, 7, 1, 1, -1)
+        settings_layout.addLayout(settings_glayout)
+        settings_layout.addStretch(1)
 
         # Advanced tab
         advanced_tab = QWidget()
@@ -499,9 +543,9 @@ class SettingsDialog(WindowModalDialog):
         # Advanced tab - clear PIN
         clear_pin_button = QPushButton(_("Disable PIN"))
         clear_pin_button.clicked.connect(clear_pin)
-        clear_pin_warning = QLabel(_("If you disable your PIN, anyone with "
-                                     "physical access to your %s device can "
-                                     "spend your bitcoins.") % plugin.device)
+        clear_pin_warning = QLabel(
+            _("If you disable your PIN, anyone with physical access to your "
+              "%s device can spend your bitcoins.") % plugin.device)
         clear_pin_warning.setWordWrap(True)
         clear_pin_warning.setStyleSheet("color: red")
         advanced_glayout.addWidget(clear_pin_button, 0, 2)
@@ -552,14 +596,7 @@ class SettingsDialog(WindowModalDialog):
         tabs.addTab(settings_tab, _("Settings"))
         tabs.addTab(advanced_tab, _("Advanced"))
 
-        # Update information and then connect change slots
+        # Update information
         update()
-        slider_moved()
-
         dialog_vbox.addWidget(tabs)
         dialog_vbox.addLayout(Buttons(CloseButton(self)))
-
-    def closeEvent(self, event):
-        seconds = self.slider.sliderPosition() * 60
-        self.window.wallet.set_session_timeout(seconds)
-        event.accept()