btc: fix requesting previous transactions when not needed

The previous transaction of every input in a Bitcoin transaction needs
to be streamed to the BitBox, unless all inputs are Taproot
inputs. They are needed for the BitBox to verify the input
amount. Since Taproot, that is not necessary because all input amounts
are part of the sighash.

The firmware currently inspects the "scriptConfigs" to determine if
all inputs are taproot.

Bug: the scriptConfigs is also used to indicate outputs belonging to
the same account (either change outputs or outputs verified as "This
BitBox (same account): …". As a result, if all inputs are Taproot, but
the change output is not Taproot (or coins are sent to a non-Taproot
address of the same account), then previous transactions are
requested/streamed even if they don't need to be.

Fix: we let the client libraries set the flag if prevtxs are required
or not, and reject the transaction if they set it to "not required"
even if there is a non-Taproot input present.

The previous way is preserved for backwards compatibility with client
libraries that do not set this new flag.

Author: benma

CHANGELOG.md

@@ -14,6 +14,8 @@ customers cannot upgrade their bootloader, its changes are recorded separately.
 - simulator: simulate a Nova device
 - Add API call to fetch multiple xpubs at once
 - Add the option for the simulator to write its memory to file.
+- Bitcoin: do not request previous transactions if all inputs are Taproot, even if an output paying
+  to the same account is not Taproot
 
 ### 9.23.1
 - EVM: add HyperEVM (HYPE) and SONIC (S) to known networks

messages/btc.proto

@@ -129,6 +129,19 @@ message BTCSignInitRequest {
   // used script configs for outputs that send to an address of the same keystore, but not
   // necessarily the same account (as defined by `script_configs` above).
   repeated BTCScriptConfigWithKeypath output_script_configs = 10;
+
+  enum PrevTxs {
+    PREV_TXS_DEFAULT = 0;
+    PREV_TXS_REQUIRED = 1;
+    PREV_TXS_NOT_REQUIRED = 2;
+  }
+  // Set this to:
+  // - `PREV_TXS_DEFAULT` for compatibility mode, the value will be determined using `script_configs`.
+  //  This is not robust, as non-Taproot change outputs are included there and falsely leads to
+  //  previous transactions being required.
+  // - `PREV_TXS_REQUIRED` if not all transaction inputs are Taproot.
+  // - `PREV_TXS_NOT_REQUIRED` if all transaction inputs are Taproot.
+  PrevTxs prev_txs = 11;
 }
 
 message BTCSignNextResponse {

py/bitbox02/bitbox02/bitbox02/bitbox02.py

@@ -471,6 +471,9 @@ def btc_sign(
 
         sigs: List[Tuple[int, bytes]] = []
 
+        all_inputs_are_taproot = all(
+            is_taproot(script_configs[inp["script_config_index"]]) for inp in inputs
+        )
         # Init request
         request = hww.Request()
         request.btc_sign_init.CopyFrom(
@@ -483,6 +486,11 @@ def btc_sign(
                 locktime=locktime,
                 format_unit=format_unit,
                 output_script_configs=output_script_configs,
+                prev_txs=(
+                    btc.BTCSignInitRequest.PrevTxs.PREV_TXS_NOT_REQUIRED
+                    if all_inputs_are_taproot
+                    else btc.BTCSignInitRequest.PrevTxs.PREV_TXS_REQUIRED
+                ),
             )
         )
         next_response = self._msg_query(request, expected_response="btc_sign_next").btc_sign_next

py/bitbox02/bitbox02/communication/generated/btc_pb2.py

@@ -342,6 +342,21 @@ class BTCSignInitRequest(google.protobuf.message.Message):
     SAT: BTCSignInitRequest.FormatUnit.ValueType  # 1
     """Only valid for BTC/TBTC, formats as "sat"/"tsat"."""
 
+    class _PrevTxs:
+        ValueType = typing.NewType("ValueType", builtins.int)
+        V: typing_extensions.TypeAlias = ValueType
+
+    class _PrevTxsEnumTypeWrapper(google.protobuf.internal.enum_type_wrapper._EnumTypeWrapper[BTCSignInitRequest._PrevTxs.ValueType], builtins.type):
+        DESCRIPTOR: google.protobuf.descriptor.EnumDescriptor
+        PREV_TXS_DEFAULT: BTCSignInitRequest._PrevTxs.ValueType  # 0
+        PREV_TXS_REQUIRED: BTCSignInitRequest._PrevTxs.ValueType  # 1
+        PREV_TXS_NOT_REQUIRED: BTCSignInitRequest._PrevTxs.ValueType  # 2
+
+    class PrevTxs(_PrevTxs, metaclass=_PrevTxsEnumTypeWrapper): ...
+    PREV_TXS_DEFAULT: BTCSignInitRequest.PrevTxs.ValueType  # 0
+    PREV_TXS_REQUIRED: BTCSignInitRequest.PrevTxs.ValueType  # 1
+    PREV_TXS_NOT_REQUIRED: BTCSignInitRequest.PrevTxs.ValueType  # 2
+
     COIN_FIELD_NUMBER: builtins.int
     SCRIPT_CONFIGS_FIELD_NUMBER: builtins.int
     VERSION_FIELD_NUMBER: builtins.int
@@ -351,6 +366,7 @@ class BTCSignInitRequest(google.protobuf.message.Message):
     FORMAT_UNIT_FIELD_NUMBER: builtins.int
     CONTAINS_SILENT_PAYMENT_OUTPUTS_FIELD_NUMBER: builtins.int
     OUTPUT_SCRIPT_CONFIGS_FIELD_NUMBER: builtins.int
+    PREV_TXS_FIELD_NUMBER: builtins.int
     coin: global___BTCCoin.ValueType
     version: builtins.int
     """must be 1 or 2"""
@@ -360,6 +376,14 @@ class BTCSignInitRequest(google.protobuf.message.Message):
     """must be <500000000"""
     format_unit: global___BTCSignInitRequest.FormatUnit.ValueType
     contains_silent_payment_outputs: builtins.bool
+    prev_txs: global___BTCSignInitRequest.PrevTxs.ValueType
+    """Set this to:
+    - `PREV_TXS_DEFAULT` for compatibility mode, the value will be determined using `script_configs`.
+     This is not robust, as non-Taproot change outputs are included there and falsely leads to
+     previous transactions being required.
+    - `PREV_TXS_REQUIRED` if not all transaction inputs are Taproot.
+    - `PREV_TXS_NOT_REQUIRED` if all transaction inputs are Taproot.
+    """
     @property
     def script_configs(self) -> google.protobuf.internal.containers.RepeatedCompositeFieldContainer[global___BTCScriptConfigWithKeypath]:
         """used script configs in inputs and changes"""
@@ -382,8 +406,9 @@ class BTCSignInitRequest(google.protobuf.message.Message):
         format_unit: global___BTCSignInitRequest.FormatUnit.ValueType = ...,
         contains_silent_payment_outputs: builtins.bool = ...,
         output_script_configs: collections.abc.Iterable[global___BTCScriptConfigWithKeypath] | None = ...,
+        prev_txs: global___BTCSignInitRequest.PrevTxs.ValueType = ...,
     ) -> None: ...
-    def ClearField(self, field_name: typing.Literal["coin", b"coin", "contains_silent_payment_outputs", b"contains_silent_payment_outputs", "format_unit", b"format_unit", "locktime", b"locktime", "num_inputs", b"num_inputs", "num_outputs", b"num_outputs", "output_script_configs", b"output_script_configs", "script_configs", b"script_configs", "version", b"version"]) -> None: ...
+    def ClearField(self, field_name: typing.Literal["coin", b"coin", "contains_silent_payment_outputs", b"contains_silent_payment_outputs", "format_unit", b"format_unit", "locktime", b"locktime", "num_inputs", b"num_inputs", "num_outputs", b"num_outputs", "output_script_configs", b"output_script_configs", "prev_txs", b"prev_txs", "script_configs", b"script_configs", "version", b"version"]) -> None: ...
 
 global___BTCSignInitRequest = BTCSignInitRequest
 

py/bitbox02/bitbox02/communication/generated/btc_pb2.pyi

@@ -717,8 +717,20 @@ async fn _process(
     let mut hasher_amounts = Sha256::new();
     let mut hasher_scriptpubkeys = Sha256::new();
 
-    // Are all inputs taproot?
-    let taproot_only = validated_script_configs.iter().all(is_taproot);
+    let prev_txs = pb::btc_sign_init_request::PrevTxs::try_from(request.prev_txs)?;
+
+    // We will request to stream previous transactions if not all inputs are Taproot.
+    let prevtxs_required: bool = match prev_txs {
+        // For backwards compatibility for client's that don't provide the `prev_txs` field: handle
+        // it like before `prev_txs` was introduced by inspecting the script configs and seeing if
+        // all of them are taproot. This is not robust, as non-Taproot change outputs are included
+        // there and falsely leads to previous transactions being required.
+        pb::btc_sign_init_request::PrevTxs::Default => {
+            !validated_script_configs.iter().all(is_taproot)
+        }
+        pb::btc_sign_init_request::PrevTxs::Required => true,
+        pb::btc_sign_init_request::PrevTxs::NotRequired => false,
+    };
 
     let mut silent_payment = if request.contains_silent_payment_outputs {
         Some(SilentPayment::new(SECP256K1, coin.try_into()?))
@@ -775,7 +787,7 @@ async fn _process(
         hasher_scriptpubkeys.update(serialize_varint(pk_script.len() as u64).as_slice());
         hasher_scriptpubkeys.update(pk_script.as_slice());
 
-        if !taproot_only {
+        if prevtxs_required {
             handle_prevtx(
                 input_index,
                 &tx_input,
@@ -784,6 +796,8 @@ async fn _process(
                 &mut next_response,
             )
             .await?;
+        } else if !is_taproot(script_config_account) {
+            return Err(Error::InvalidInput);
         }
 
         if let Some(ref mut silent_payment) = silent_payment {
@@ -1572,6 +1586,7 @@ mod tests {
                     .outputs
                     .iter()
                     .any(|output| output.silent_payment.is_some()),
+                prev_txs: pb::btc_sign_init_request::PrevTxs::Default as _,
             }
         }
 
@@ -1595,6 +1610,7 @@ mod tests {
                 locktime: self.locktime,
                 format_unit: FormatUnit::Default as _,
                 contains_silent_payment_outputs: false,
+                prev_txs: pb::btc_sign_init_request::PrevTxs::Default as _,
             }
         }
 
@@ -1679,6 +1695,7 @@ mod tests {
             locktime: 0,
             format_unit: FormatUnit::Default as _,
             contains_silent_payment_outputs: false,
+            prev_txs: pb::btc_sign_init_request::PrevTxs::Default as _,
         };
 
         {
@@ -1871,6 +1888,7 @@ mod tests {
                         locktime: 0,
                         format_unit: FormatUnit::Default as _,
                         contains_silent_payment_outputs: false,
+                        prev_txs: pb::btc_sign_init_request::PrevTxs::Default as _,
                     }
                 )),
                 Err(Error::InvalidInput)
@@ -2147,6 +2165,57 @@ mod tests {
         assert!(unsafe { !PREVTX_REQUESTED });
     }
 
+    /// Test signing if all inputs are of type P2TR, but change is not of type P2TR.
+    /// Previous transactions are requested for backwards compatibility when
+    #[test]
+    fn test_script_type_p2tr_different_change() {
+        let transaction =
+            alloc::rc::Rc::new(core::cell::RefCell::new(Transaction::new(pb::BtcCoin::Btc)));
+        for input in transaction.borrow_mut().inputs.iter_mut() {
+            input.input.keypath[0] = 86 + HARDENED;
+            input.input.script_config_index = 1;
+        }
+
+        let tx = transaction.clone();
+        // Check that previous transactions are not streamed, as all inputs are taproot.
+        static mut PREVTX_REQUESTED: bool = false;
+        *crate::hww::MOCK_NEXT_REQUEST.0.borrow_mut() =
+            Some(Box::new(move |response: Response| {
+                let next = extract_next(&response);
+                if NextType::try_from(next.r#type).unwrap() == NextType::PrevtxInit {
+                    unsafe { PREVTX_REQUESTED = true }
+                }
+                Ok(tx.borrow().make_host_request(response))
+            }));
+
+        mock_unlocked();
+        bitbox02::random::fake_reset();
+        let mut init_request = transaction.borrow().init_request();
+        init_request
+            .script_configs
+            .push(pb::BtcScriptConfigWithKeypath {
+                script_config: Some(pb::BtcScriptConfig {
+                    config: Some(pb::btc_script_config::Config::SimpleType(
+                        SimpleType::P2tr as _,
+                    )),
+                }),
+                keypath: vec![86 + HARDENED, 0 + HARDENED, 10 + HARDENED],
+            });
+
+        init_request.prev_txs = pb::btc_sign_init_request::PrevTxs::NotRequired as _;
+        assert!(block_on(process(&mut TestingHal::new(), &init_request)).is_ok());
+        assert!(unsafe { !PREVTX_REQUESTED });
+
+        // Also test compatibility mode - before the introduction of `prev_txs`, the previous
+        // transactions were wrongly requested in this case.
+        unsafe {
+            PREVTX_REQUESTED = false;
+        }
+        init_request.prev_txs = pb::btc_sign_init_request::PrevTxs::Default as _;
+        assert!(block_on(process(&mut TestingHal::new(), &init_request)).is_ok());
+        assert!(unsafe { PREVTX_REQUESTED });
+    }
+
     /// Test signing if with mixed inputs, one of them being taproot. Previous transactions of all
     /// inputs should be streamed in this case.
     #[test]
@@ -2180,11 +2249,31 @@ mod tests {
                 }),
                 keypath: vec![86 + HARDENED, 0 + HARDENED, 10 + HARDENED],
             });
+
+        // In compatibility mode, prevtxs are correctly requested.
+        init_request.prev_txs = pb::btc_sign_init_request::PrevTxs::Default as _;
+        unsafe { PREVTX_REQUESTED = 0 };
         assert!(block_on(process(&mut TestingHal::new(), &init_request)).is_ok());
         assert_eq!(
             unsafe { PREVTX_REQUESTED },
             transaction.borrow().inputs.len() as _
         );
+
+        // Same as when the client explicitly states it.
+        init_request.prev_txs = pb::btc_sign_init_request::PrevTxs::Required as _;
+        unsafe { PREVTX_REQUESTED = 0 };
+        assert!(block_on(process(&mut TestingHal::new(), &init_request)).is_ok());
+        assert_eq!(
+            unsafe { PREVTX_REQUESTED },
+            transaction.borrow().inputs.len() as _
+        );
+
+        // Client can't lie about it.
+        init_request.prev_txs = pb::btc_sign_init_request::PrevTxs::NotRequired as _;
+        assert_eq!(
+            block_on(process(&mut TestingHal::new(), &init_request)),
+            Err(Error::InvalidInput)
+        );
     }
 
     /// Test signing UTXOs with high keypath address indices. Even though we don't support verifying
@@ -2881,6 +2970,7 @@ mod tests {
                 locktime: tx.locktime,
                 format_unit: FormatUnit::Default as _,
                 contains_silent_payment_outputs: false,
+                prev_txs: pb::btc_sign_init_request::PrevTxs::Default as _,
             }
         };
 
@@ -2976,6 +3066,7 @@ mod tests {
                 locktime: tx.locktime,
                 format_unit: FormatUnit::Default as _,
                 contains_silent_payment_outputs: false,
+                prev_txs: pb::btc_sign_init_request::PrevTxs::Default as _,
             }
         };
         assert_eq!(
@@ -3047,6 +3138,7 @@ mod tests {
                 locktime: tx.locktime,
                 format_unit: FormatUnit::Default as _,
                 contains_silent_payment_outputs: false,
+                prev_txs: pb::btc_sign_init_request::PrevTxs::Default as _,
             }
         };
         let result = block_on(process(&mut TestingHal::new(), &init_request));
@@ -3124,6 +3216,7 @@ mod tests {
                 locktime: tx.locktime,
                 format_unit: FormatUnit::Default as _,
                 contains_silent_payment_outputs: false,
+                prev_txs: pb::btc_sign_init_request::PrevTxs::Default as _,
             }
         };
         let result = block_on(process(&mut TestingHal::new(), &init_request));

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

@@ -599,6 +599,14 @@ pub struct BtcSignInitRequest {
     /// necessarily the same account (as defined by `script_configs` above).
     #[prost(message, repeated, tag = "10")]
     pub output_script_configs: ::prost::alloc::vec::Vec<BtcScriptConfigWithKeypath>,
+    /// Set this to:
+    /// - `PREV_TXS_DEFAULT` for compatibility mode, the value will be determined using `script_configs`.
+    ///   This is not robust, as non-Taproot change outputs are included there and falsely leads to
+    ///   previous transactions being required.
+    /// - `PREV_TXS_REQUIRED` if not all transaction inputs are Taproot.
+    /// - `PREV_TXS_NOT_REQUIRED` if all transaction inputs are Taproot.
+    #[prost(enumeration = "btc_sign_init_request::PrevTxs", tag = "11")]
+    pub prev_txs: i32,
 }
 /// Nested message and enum types in `BTCSignInitRequest`.
 pub mod btc_sign_init_request {
@@ -640,6 +648,45 @@ pub mod btc_sign_init_request {
             }
         }
     }
+    #[derive(
+        Clone,
+        Copy,
+        Debug,
+        PartialEq,
+        Eq,
+        Hash,
+        PartialOrd,
+        Ord,
+        ::prost::Enumeration
+    )]
+    #[repr(i32)]
+    pub enum PrevTxs {
+        Default = 0,
+        Required = 1,
+        NotRequired = 2,
+    }
+    impl PrevTxs {
+        /// String value of the enum field names used in the ProtoBuf definition.
+        ///
+        /// The values are not transformed in any way and thus are considered stable
+        /// (if the ProtoBuf definition does not change) and safe for programmatic use.
+        pub fn as_str_name(&self) -> &'static str {
+            match self {
+                PrevTxs::Default => "PREV_TXS_DEFAULT",
+                PrevTxs::Required => "PREV_TXS_REQUIRED",
+                PrevTxs::NotRequired => "PREV_TXS_NOT_REQUIRED",
+            }
+        }
+        /// Creates an enum from field names used in the ProtoBuf definition.
+        pub fn from_str_name(value: &str) -> ::core::option::Option<Self> {
+            match value {
+                "PREV_TXS_DEFAULT" => Some(Self::Default),
+                "PREV_TXS_REQUIRED" => Some(Self::Required),
+                "PREV_TXS_NOT_REQUIRED" => Some(Self::NotRequired),
+                _ => None,
+            }
+        }
+    }
 }
 #[allow(clippy::derive_partial_eq_without_eq)]
 #[derive(Clone, PartialEq, ::prost::Message)]