rust: add better error messages

The protobuf Error response has a message field, but it is hardcoded
to generic strings depending on the error type, e.g. "generic",
"invalid input", etc. When a user/developer reports this error, better
information is useful.

This patch adds error messages based on the error condition.

Unfortunately, for now we still have to stick to static strings with
no runtime information, e.g. "invalid keypath" over "keypath invalid:
{}" cotaining the actual keypath. I experimented with dynamic strings,
but this immediatelly added another ~7kB in binary bloat due the usage
of String and formatting. Only changing the type from `&'static str`
to `String` adds a few kB.

Alternatives considered:

- use String+format!() to be able to return runtime info (e.g. actual
  keypaths used), as well as chain context strings together, but that
  resulted in too many kilobytes of additional binary bloat.
- error deps like anyhow, snafu, etc. They all add a lot of binary and
code bloat.
- using only numeric error codes instead of static strings to save
binary space. Can still do in the future if needed.
- using `ufmt` crate hoping it produces smaller binaries than
`format!()` for dynamic strings. I tried it and it was about the same.

Author: benma

src/rust/bitbox02-rust/src/hww.rs

@@ -18,6 +18,8 @@ pub mod noise;
 extern crate alloc;
 use alloc::vec::Vec;
 
+use api::error::{Error, ErrorKind};
+
 const OP_UNLOCK: u8 = b'u';
 const OP_ATTESTATION: u8 = b'a';
 
@@ -30,17 +32,20 @@ const OP_STATUS_FAILURE_UNINITIALIZED: u8 = 2;
 /// message, `Err(Error::InvalidInput)` is returned.
 pub async fn next_request(
     response: crate::pb::response::Response,
-) -> Result<crate::pb::request::Request, api::error::Error> {
+) -> Result<crate::pb::request::Request, Error> {
     let mut out = [OP_STATUS_SUCCESS].to_vec();
-    noise::encrypt(&api::encode(response), &mut out).or(Err(api::error::Error::NoiseEncrypt))?;
+    noise::encrypt(&api::encode(response), &mut out).map_err(Error::err_noise_encrypt)?;
     let request = crate::async_usb::next_request(out).await;
     match request.split_first() {
         Some((&noise::OP_NOISE_MSG, encrypted_request)) => {
             let decrypted_request =
-                noise::decrypt(&encrypted_request).or(Err(api::error::Error::NoiseDecrypt))?;
+                noise::decrypt(&encrypted_request).map_err(Error::err_noise_decrypt)?;
             api::decode(&decrypted_request[..])
         }
-        _ => Err(api::error::Error::InvalidInput),
+        _ => Err(Error {
+            msg: None,
+            kind: ErrorKind::InvalidInput,
+        }),
     }
 }
 

src/rust/bitbox02-rust/src/hww/api.rs

@@ -37,7 +37,7 @@ mod system;
 
 use alloc::vec::Vec;
 
-use error::{make_error, Error};
+use error::{make_error, Error, ErrorKind};
 use pb::request::Request;
 use pb::response::Response;
 use prost::Message;
@@ -58,7 +58,14 @@ pub fn decode(input: &[u8]) -> Result<Request, Error> {
         Ok(pb::Request {
             request: Some(request),
         }) => Ok(request),
-        _ => Err(Error::InvalidInput),
+        Ok(pb::Request { request: None }) => Err(Error {
+            msg: Some("request missing".into()),
+            kind: ErrorKind::InvalidInput,
+        }),
+        Err(_) => Err(Error {
+            msg: Some("protobuf decode error".into()),
+            kind: ErrorKind::InvalidInput,
+        }),
     }
 }
 
@@ -110,7 +117,10 @@ async fn process_api_btc(request: &Request) -> Option<Result<Response, Error>> {
 
 #[cfg(not(any(feature = "app-bitcoin", feature = "app-litecoin")))]
 async fn process_api_btc(_request: &Request) -> Option<Result<Response, Error>> {
-    Some(Err(Error::Disabled))
+    Some(Err(Error {
+        msg: None,
+        kind: ErrorKind::Disabled,
+    }))
 }
 
 /// Handle a protobuf api call.
@@ -164,7 +174,10 @@ pub async fn process(input: Vec<u8>) -> Vec<u8> {
         Err(err) => return encode(make_error(err)),
     };
     if !bitbox02::commander::states_can_call(request_tag(&request) as u16) {
-        return encode(make_error(Error::InvalidState));
+        return encode(make_error(Error {
+            msg: None,
+            kind: ErrorKind::InvalidState,
+        }));
     }
 
     // Since we will process the call now, so can clear the 'force next' info.

src/rust/bitbox02-rust/src/hww/api/backup.rs

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use super::Error;
+use super::error::{Context, Error, ErrorKind};
 use crate::pb;
 
 use pb::response::Response;
@@ -24,7 +24,10 @@ pub async fn check(
     &pb::CheckBackupRequest { silent }: &pb::CheckBackupRequest,
 ) -> Result<Response, Error> {
     if !bitbox02::sdcard_inserted() {
-        return Err(Error::InvalidInput);
+        return Err(Error {
+            msg: Some("sdcard not inserted".into()),
+            kind: ErrorKind::InvalidInput,
+        });
     }
     match backup::check() {
         Ok(backup::CheckData { id, name, .. }) => {
@@ -55,12 +58,18 @@ pub async fn check(
             if !silent {
                 status::status("Backup missing\nor invalid", false).await;
             }
-            Err(Error::Generic)
+            Err(Error {
+                msg: Some("backup missing or invalid".into()),
+                kind: ErrorKind::Generic,
+            })
         }
         Err(err) => {
             let msg = format!("Could not check\nbackup\n{:?}", err).replace("BACKUP_ERR_", "");
             status::status(&msg, false).await;
-            Err(Error::Generic)
+            Err(Error {
+                msg: None,
+                kind: ErrorKind::Generic,
+            })
         }
     }
 }
@@ -83,7 +92,10 @@ pub async fn create(
     const MAX_WEST_UTC_OFFSET: i32 = -43200; // 12 hours in seconds
 
     if timezone_offset < MAX_WEST_UTC_OFFSET || timezone_offset > MAX_EAST_UTC_OFFSET {
-        return Err(Error::InvalidInput);
+        return Err(Error {
+            msg: Some("invalid timezone_offset".into()),
+            kind: ErrorKind::InvalidInput,
+        });
     }
 
     // Wait for sd card
@@ -95,7 +107,10 @@ pub async fn create(
     let is_initialized = bitbox02::memory::is_initialized();
 
     if is_initialized {
-        unlock::unlock_keystore("Unlock device", unlock::CanCancel::Yes).await?;
+        unlock::unlock_keystore("Unlock device", unlock::CanCancel::Yes)
+            .await
+            .map_err(Error::err)
+            .context("unlock_keystore failed")?;
     }
 
     let seed_birthdate = if !is_initialized {
@@ -106,9 +121,7 @@ pub async fn create(
             ..Default::default()
         };
         confirm::confirm(&params).await?;
-        if bitbox02::memory::set_seed_birthdate(timestamp).is_err() {
-            return Err(Error::Memory);
-        }
+        bitbox02::memory::set_seed_birthdate(timestamp).map_err(Error::err_memory)?;
         timestamp
     } else if let Ok(backup::CheckData { birthdate, .. }) = backup::check() {
         // If adding new backup after initialized, we do not know the seed birthdate.
@@ -133,7 +146,10 @@ pub async fn create(
             let msg = format!("Backup not created\nPlease contact\nsupport ({:?})", err)
                 .replace("BACKUP_ERR_", "");
             status::status(&msg, false).await;
-            Err(Error::Generic)
+            Err(Error {
+                msg: None,
+                kind: ErrorKind::Generic,
+            })
         }
     }
 }

src/rust/bitbox02-rust/src/hww/api/bitcoin.rs

@@ -19,8 +19,8 @@ mod params;
 mod script;
 pub mod signmsg;
 
+use super::error::{Context, Error, ErrorKind};
 use super::pb;
-use super::Error;
 
 use crate::apps::bitcoin;
 use crate::workflow::confirm;
@@ -48,7 +48,10 @@ pub async fn next_request(response: pb::btc_response::Response) -> Result<Reques
         pb::request::Request::Btc(pb::BtcRequest {
             request: Some(request),
         }) => Ok(request),
-        _ => Err(Error::InvalidState),
+        _ => Err(Error {
+            msg: Some("expected Btc request, got".into()),
+            kind: ErrorKind::InvalidState,
+        }),
     }
 }
 
@@ -68,9 +71,13 @@ pub async fn antiklepto_get_host_nonce(
             Ok(host_nonce
                 .as_slice()
                 .try_into()
-                .or(Err(Error::InvalidInput))?)
+                .map_err(Error::err_invalid_input)
+                .context("could not parse host nonce")?)
         }
-        _ => Err(Error::InvalidState),
+        _ => Err(Error {
+            msg: Some("expected AntikleptoSignature".into()),
+            kind: ErrorKind::InvalidState,
+        }),
     }
 }
 
@@ -85,7 +92,10 @@ fn coin_enabled(coin: pb::BtcCoin) -> Result<(), Error> {
     if let Ltc | Tltc = coin {
         return Ok(());
     }
-    Err(Error::Disabled)
+    Err(Error {
+        msg: None,
+        kind: ErrorKind::Disabled,
+    })
 }
 
 /// Processes an xpub api call.
@@ -96,7 +106,9 @@ async fn xpub(
     display: bool,
 ) -> Result<Response, Error> {
     let params = params::get(coin);
-    bitcoin::keypath::validate_xpub(keypath, params.bip44_coin)?;
+    bitcoin::keypath::validate_xpub(keypath, params.bip44_coin)
+        .map_err(Error::err_generic)
+        .context("invalid keypath")?;
     let xpub_type = match xpub_type {
         XPubType::Tpub => xpub_type_t::TPUB,
         XPubType::Xpub => xpub_type_t::XPUB,
@@ -109,7 +121,9 @@ async fn xpub(
         XPubType::CapitalUpub => xpub_type_t::CAPITAL_UPUB,
         XPubType::CapitalYpub => xpub_type_t::CAPITAL_YPUB,
     };
-    let xpub = encode_xpub_at_keypath(keypath, xpub_type).or(Err(Error::InvalidInput))?;
+    let xpub = encode_xpub_at_keypath(keypath, xpub_type)
+        .map_err(Error::err_invalid_input)
+        .context("encode_xpub_at_keypath failed")?;
     if display {
         let title = format!("{}\naccount #{}", params.name, keypath[2] - HARDENED + 1,);
         let confirm_params = confirm::Params {
@@ -130,7 +144,9 @@ async fn address_simple(
     keypath: &[u32],
     display: bool,
 ) -> Result<Response, Error> {
-    let address = bitbox02::app_btc::address_simple(coin as _, simple_type as _, keypath)?;
+    let address = bitbox02::app_btc::address_simple(coin as _, simple_type as _, keypath)
+        .map_err(Error::err)
+        .context("address_simple failed")?;
     if display {
         let confirm_params = confirm::Params {
             title: params::get(coin).name,
@@ -150,17 +166,30 @@ async fn address_simple(
 pub async fn process_pub(request: &pb::BtcPubRequest) -> Option<Result<Response, Error>> {
     let coin = match BtcCoin::from_i32(request.coin) {
         Some(coin) => coin,
-        None => return Some(Err(Error::InvalidInput)),
+        None => {
+            return Some(Err(Error {
+                msg: Some("invalid coin".into()),
+                kind: ErrorKind::InvalidInput,
+            }))
+        }
     };
     if let Err(err) = coin_enabled(coin) {
         return Some(Err(err));
     }
     match request.output {
-        None => Some(Err(Error::InvalidInput)),
+        None => Some(Err(Error {
+            msg: Some("request.output missing".into()),
+            kind: ErrorKind::InvalidInput,
+        })),
         Some(Output::XpubType(xpub_type)) => {
             let xpub_type = match XPubType::from_i32(xpub_type) {
                 Some(xpub_type) => xpub_type,
-                None => return Some(Err(Error::InvalidInput)),
+                None => {
+                    return Some(Err(Error {
+                        msg: Some("invalid xpub_type".into()),
+                        kind: ErrorKind::InvalidInput,
+                    }))
+                }
             };
             Some(xpub(coin, xpub_type, &request.keypath, request.display).await)
         }
@@ -169,7 +198,12 @@ pub async fn process_pub(request: &pb::BtcPubRequest) -> Option<Result<Response,
         })) => {
             let simple_type = match SimpleType::from_i32(simple_type) {
                 Some(simple_type) => simple_type,
-                None => return Some(Err(Error::InvalidInput)),
+                None => {
+                    return Some(Err(Error {
+                        msg: Some("invalid simple_type".into()),
+                        kind: ErrorKind::InvalidInput,
+                    }))
+                }
             };
             Some(address_simple(coin, simple_type, &request.keypath, request.display).await)
         }