btc: fix requesting previous transactions when not needed

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.pyi

@@ -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
 

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

@@ -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 _,
             }
         }
 
@@ -1659,7 +1675,7 @@ mod tests {
     }
 
     #[test]
-    pub fn test_sign_init_fail() {
+    fn test_sign_init_fail() {
         *crate::hww::MOCK_NEXT_REQUEST.0.borrow_mut() = None;
 
         let init_req_valid = pb::BtcSignInitRequest {
@@ -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)
@@ -1895,7 +1913,7 @@ mod tests {
     }
 
     #[test]
-    pub fn test_process() {
+    fn test_process() {
         static mut UI_COUNTER: u32 = 0;
         static mut PREVTX_REQUESTED: u32 = 0;
 
@@ -2042,7 +2060,7 @@ mod tests {
 
     /// Test that receiving an unexpected message from the host results in an invalid state error.
     #[test]
-    pub fn test_invalid_state() {
+    fn test_invalid_state() {
         let transaction =
             alloc::rc::Rc::new(core::cell::RefCell::new(Transaction::new(pb::BtcCoin::Btc)));
         mock_unlocked();
@@ -2066,7 +2084,7 @@ mod tests {
 
     /// Test signing if all inputs are of type P2WPKH-P2SH.
     #[test]
-    pub fn test_script_type_p2wpkh_p2sh() {
+    fn test_script_type_p2wpkh_p2sh() {
         let transaction =
             alloc::rc::Rc::new(core::cell::RefCell::new(Transaction::new(pb::BtcCoin::Btc)));
         for input in transaction.borrow_mut().inputs.iter_mut() {
@@ -2101,7 +2119,7 @@ mod tests {
 
     /// Test signing if all inputs are of type P2TR.
     #[test]
-    pub fn test_script_type_p2tr() {
+    fn test_script_type_p2tr() {
         let transaction =
             alloc::rc::Rc::new(core::cell::RefCell::new(Transaction::new(pb::BtcCoin::Btc)));
         for input in transaction.borrow_mut().inputs.iter_mut() {
@@ -2147,10 +2165,61 @@ 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]
-    pub fn test_script_type_p2tr_mixed() {
+    fn test_script_type_p2tr_mixed() {
         let transaction =
             alloc::rc::Rc::new(core::cell::RefCell::new(Transaction::new(pb::BtcCoin::Btc)));
         transaction.borrow_mut().inputs[0].input.script_config_index = 1;
@@ -2180,18 +2249,38 @@ 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
     /// receive addresses at these indices (to mitigate ransom attacks), we should still be able to
     /// spend them.
     #[test]
-    pub fn test_spend_high_address_index() {
+    fn test_spend_high_address_index() {
         let transaction =
             alloc::rc::Rc::new(core::cell::RefCell::new(Transaction::new(pb::BtcCoin::Btc)));
         transaction.borrow_mut().inputs[0].input.keypath[4] = 100000;
@@ -2208,7 +2297,7 @@ mod tests {
 
     /// Test invalid input cases.
     #[test]
-    pub fn test_invalid_input() {
+    fn test_invalid_input() {
         enum TestCase {
             // all inputs should be the same coin type.
             WrongCoinInput,
@@ -2319,7 +2408,7 @@ mod tests {
 
     /// Test signing with mixed input types.
     #[test]
-    pub fn test_mixed_inputs() {
+    fn test_mixed_inputs() {
         let transaction =
             alloc::rc::Rc::new(core::cell::RefCell::new(Transaction::new(pb::BtcCoin::Btc)));
         transaction.borrow_mut().inputs[0].input.script_config_index = 1;
@@ -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));
@@ -3588,7 +3681,7 @@ mod tests {
     }
 
     #[test]
-    pub fn test_payment_request() {
+    fn test_payment_request() {
         let transaction =
             alloc::rc::Rc::new(core::cell::RefCell::new(Transaction::new(pb::BtcCoin::Btc)));