Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bitcoin: display account name when sending to self #1360

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ customers cannot upgrade their bootloader, its changes are recorded separately.
- Ethereum: remove deprecated Goerli network
- SD card: solve backup bug when sd card is re-inserted
- Cardano: allow serialization using 258-tagged sets
- Bitcoin: identify outputs belonging to different accounts of the same keystore

### 9.21.0
- Bitcoin: add support for sending to silent payment (BIP-352) addresses
Expand Down
12 changes: 11 additions & 1 deletion messages/btc.proto
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ message BTCSignInitRequest {
}
FormatUnit format_unit = 8;
bool contains_silent_payment_outputs = 9;
// 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;
}

message BTCSignNextResponse {
Expand Down Expand Up @@ -174,12 +177,19 @@ message BTCSignOutputRequest {
uint64 value = 3;
bytes payload = 4; // if ours is false. Renamed from `hash`.
repeated uint32 keypath = 5; // if ours is true
// If ours is true. References a script config from BTCSignInitRequest
// If ours is true and `output_script_config_index` is absent. References a script config from
// BTCSignInitRequest. This allows change output identification and allows us to identify
// non-change outputs to the same account, so we can display this info to the user.
uint32 script_config_index = 6;
optional uint32 payment_request_index = 7;
// If provided, `type` and `payload` is ignored. The generated output pkScript is returned in
// BTCSignNextResponse. `contains_silent_payment_outputs` in the init request must be true.
SilentPayment silent_payment = 8;
// If ours is true. If set, `script_config_index` is ignored. References an output script config
// from BTCSignInitRequest. This enables verification that an output belongs to the same keystore,
// even if it is from a different account than we spend from, allowing us to display this info to
// the user.
optional uint32 output_script_config_index = 9;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wondering it this should just be optional BTCScriptConfigWithKeypath output_script_config instead of a reference into a vector, to simplify the code and API of client libraries.

The script_configs vector for inputs/changes is there so

  • we can validate that multisig/singlesig inputs are not mixed and that different accounts are not mixed, and
  • that we can confirm multisig details at the beginning of a transaction
  • and to have a speedup to not repeat validation for each input

All that is not necessary for the output script infos, and the validation performance is not so important there as the user is anyway manually confirming the output 🤔

}

message BTCScriptConfigRegistration {
Expand Down
7 changes: 3 additions & 4 deletions py/bitbox02/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@

## [Unreleased]

# 8.0.0
- SD card: Remove API to prompt removal of the microSD card from the device
- Add support for regtest (supported from firmware version v9.21.0)

# 7.0.0
- get_info: add optional device initialized boolean to returned tuple
- eth_sign: add address_case field, which should be initialized by the client
- SD card: Remove API to prompt removal of the microSD card from the device
- Add support for regtest (supported from firmware version v9.21.0)
- btc_sign: allow identifying outputs belonging to different accounts of the same keystore

# 6.3.0
- Allow infering product and version via API call instead of via USB descriptor
Expand Down
2 changes: 1 addition & 1 deletion py/bitbox02/bitbox02/bitbox02/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from __future__ import print_function
import sys

__version__ = "8.0.0"
__version__ = "7.0.0"

if sys.version_info.major != 3 or sys.version_info.minor < 6:
print(
Expand Down
23 changes: 21 additions & 2 deletions py/bitbox02/bitbox02/bitbox02/bitbox02.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,20 @@ class BTCInputType(TypedDict):
class BTCOutputInternal:
# TODO: Use NamedTuple, but not playing well with protobuf types.

def __init__(self, keypath: Sequence[int], value: int, script_config_index: int):
def __init__(
self,
keypath: Sequence[int],
value: int,
script_config_index: int,
output_script_config_index: Optional[int] = None,
):
"""
keypath: keypath to the change output.
"""
self.keypath = keypath
self.value = value
self.script_config_index = script_config_index
self.output_script_config_index = output_script_config_index


class BTCOutputExternal:
Expand Down Expand Up @@ -390,13 +397,14 @@ def btc_sign(
version: int = 1,
locktime: int = 0,
format_unit: "btc.BTCSignInitRequest.FormatUnit.V" = btc.BTCSignInitRequest.FormatUnit.DEFAULT,
output_script_configs: Optional[Sequence[btc.BTCScriptConfigWithKeypath]] = None,
) -> Sequence[Tuple[int, bytes]]:
"""
coin: the first element of all provided keypaths must match the coin:
- BTC: 0 + HARDENED
- Testnets: 1 + HARDENED
- LTC: 2 + HARDENED
script_configs: types of all inputs and change outputs. The first element of all provided
script_configs: types of all inputs and outputs belonging to the same account (change or non-change). The first element of all provided
keypaths must match this type:
- SCRIPT_P2PKH: 44 + HARDENED
- SCRIPT_P2WPKH_P2SH: 49 + HARDENED
Expand All @@ -407,6 +415,8 @@ def btc_sign(
outputs: transaction outputs. Can be an external output
(BTCOutputExternal) or an internal output for change (BTCOutputInternal).
version, locktime: reserved for future use.
format_unit: defines in which unit amounts will be displayed
output_script_configs: script types for outputs belonging to the same keystore
Returns: list of (input index, signature) tuples.
Raises Bitbox02Exception with ERR_USER_ABORT on user abort.
"""
Expand All @@ -418,6 +428,10 @@ def btc_sign(
if any(map(is_taproot, script_configs)):
self._require_atleast(semver.VersionInfo(9, 10, 0))

if output_script_configs:
# Attaching output info supported since v9.22.0.
self._require_atleast(semver.VersionInfo(9, 22, 0))

supports_antiklepto = self.version >= semver.VersionInfo(9, 4, 0)

sigs: List[Tuple[int, bytes]] = []
Expand All @@ -433,6 +447,7 @@ def btc_sign(
num_outputs=len(outputs),
locktime=locktime,
format_unit=format_unit,
output_script_configs=output_script_configs,
)
)
next_response = self._msg_query(request, expected_response="btc_sign_next").btc_sign_next
Expand Down Expand Up @@ -552,12 +567,16 @@ def btc_sign(

request = hww.Request()
if isinstance(tx_output, BTCOutputInternal):
if tx_output.output_script_config_index is not None:
# Attaching output info supported since v9.22.0.
self._require_atleast(semver.VersionInfo(9, 22, 0))
request.btc_sign_output.CopyFrom(
btc.BTCSignOutputRequest(
ours=True,
value=tx_output.value,
keypath=tx_output.keypath,
script_config_index=tx_output.script_config_index,
output_script_config_index=tx_output.output_script_config_index,
)
)
elif isinstance(tx_output, BTCOutputExternal):
Expand Down
100 changes: 50 additions & 50 deletions py/bitbox02/bitbox02/communication/generated/btc_pb2.py

Large diffs are not rendered by default.

31 changes: 27 additions & 4 deletions py/bitbox02/bitbox02/communication/generated/btc_pb2.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ class BTCSignInitRequest(google.protobuf.message.Message):
LOCKTIME_FIELD_NUMBER: builtins.int
FORMAT_UNIT_FIELD_NUMBER: builtins.int
CONTAINS_SILENT_PAYMENT_OUTPUTS_FIELD_NUMBER: builtins.int
OUTPUT_SCRIPT_CONFIGS_FIELD_NUMBER: builtins.int
coin: global___BTCCoin.ValueType
@property
def script_configs(self) -> google.protobuf.internal.containers.RepeatedCompositeFieldContainer[global___BTCScriptConfigWithKeypath]:
Expand All @@ -308,6 +309,12 @@ class BTCSignInitRequest(google.protobuf.message.Message):

format_unit: global___BTCSignInitRequest.FormatUnit.ValueType
contains_silent_payment_outputs: builtins.bool
@property
def output_script_configs(self) -> google.protobuf.internal.containers.RepeatedCompositeFieldContainer[global___BTCScriptConfigWithKeypath]:
"""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).
"""
pass
def __init__(self,
*,
coin: global___BTCCoin.ValueType = ...,
Expand All @@ -318,8 +325,9 @@ class BTCSignInitRequest(google.protobuf.message.Message):
locktime: builtins.int = ...,
format_unit: global___BTCSignInitRequest.FormatUnit.ValueType = ...,
contains_silent_payment_outputs: builtins.bool = ...,
output_script_configs: typing.Optional[typing.Iterable[global___BTCScriptConfigWithKeypath]] = ...,
) -> None: ...
def ClearField(self, field_name: typing_extensions.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","script_configs",b"script_configs","version",b"version"]) -> None: ...
def ClearField(self, field_name: typing_extensions.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: ...
global___BTCSignInitRequest = BTCSignInitRequest

class BTCSignNextResponse(google.protobuf.message.Message):
Expand Down Expand Up @@ -454,6 +462,7 @@ class BTCSignOutputRequest(google.protobuf.message.Message):
SCRIPT_CONFIG_INDEX_FIELD_NUMBER: builtins.int
PAYMENT_REQUEST_INDEX_FIELD_NUMBER: builtins.int
SILENT_PAYMENT_FIELD_NUMBER: builtins.int
OUTPUT_SCRIPT_CONFIG_INDEX_FIELD_NUMBER: builtins.int
ours: builtins.bool
type: global___BTCOutputType.ValueType
"""if ours is false"""
Expand All @@ -469,7 +478,10 @@ class BTCSignOutputRequest(google.protobuf.message.Message):
"""if ours is true"""
pass
script_config_index: builtins.int
"""If ours is true. References a script config from BTCSignInitRequest"""
"""If ours is true and `output_script_config_index` is absent. References a script config from
BTCSignInitRequest. This allows change output identification and allows us to identify
non-change outputs to the same account, so we can display this info to the user.
"""

payment_request_index: builtins.int
@property
Expand All @@ -478,6 +490,13 @@ class BTCSignOutputRequest(google.protobuf.message.Message):
BTCSignNextResponse. `contains_silent_payment_outputs` in the init request must be true.
"""
pass
output_script_config_index: builtins.int
"""If ours is true. If set, `script_config_index` is ignored. References an output script config
from BTCSignInitRequest. This enables verification that an output belongs to the same keystore,
even if it is from a different account than we spend from, allowing us to display this info to
the user.
"""

def __init__(self,
*,
ours: builtins.bool = ...,
Expand All @@ -488,9 +507,13 @@ class BTCSignOutputRequest(google.protobuf.message.Message):
script_config_index: builtins.int = ...,
payment_request_index: typing.Optional[builtins.int] = ...,
silent_payment: typing.Optional[global___BTCSignOutputRequest.SilentPayment] = ...,
output_script_config_index: typing.Optional[builtins.int] = ...,
) -> None: ...
def HasField(self, field_name: typing_extensions.Literal["_payment_request_index",b"_payment_request_index","payment_request_index",b"payment_request_index","silent_payment",b"silent_payment"]) -> builtins.bool: ...
def ClearField(self, field_name: typing_extensions.Literal["_payment_request_index",b"_payment_request_index","keypath",b"keypath","ours",b"ours","payload",b"payload","payment_request_index",b"payment_request_index","script_config_index",b"script_config_index","silent_payment",b"silent_payment","type",b"type","value",b"value"]) -> None: ...
def HasField(self, field_name: typing_extensions.Literal["_output_script_config_index",b"_output_script_config_index","_payment_request_index",b"_payment_request_index","output_script_config_index",b"output_script_config_index","payment_request_index",b"payment_request_index","silent_payment",b"silent_payment"]) -> builtins.bool: ...
def ClearField(self, field_name: typing_extensions.Literal["_output_script_config_index",b"_output_script_config_index","_payment_request_index",b"_payment_request_index","keypath",b"keypath","ours",b"ours","output_script_config_index",b"output_script_config_index","payload",b"payload","payment_request_index",b"payment_request_index","script_config_index",b"script_config_index","silent_payment",b"silent_payment","type",b"type","value",b"value"]) -> None: ...
@typing.overload
def WhichOneof(self, oneof_group: typing_extensions.Literal["_output_script_config_index",b"_output_script_config_index"]) -> typing.Optional[typing_extensions.Literal["output_script_config_index"]]: ...
@typing.overload
def WhichOneof(self, oneof_group: typing_extensions.Literal["_payment_request_index",b"_payment_request_index"]) -> typing.Optional[typing_extensions.Literal["payment_request_index"]]: ...
global___BTCSignOutputRequest = BTCSignOutputRequest

Expand Down
4 changes: 4 additions & 0 deletions py/bitbox02/bitbox02/communication/generated/cardano_pb2.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,10 @@ class CardanoSignTransactionRequest(google.protobuf.message.Message):
"""include ttl even if it is zero"""

tag_cbor_sets: builtins.bool
"""Tag arrays in the transaction serialization with the 258 tag.
See https://github.com/IntersectMBO/cardano-ledger/blob/6e2d37cc0f47bd02e89b4ce9f78b59c35c958e96/eras/conway/impl/cddl-files/extra.cddl#L5
"""

def __init__(self,
*,
network: global___CardanoNetwork.ValueType = ...,
Expand Down
49 changes: 47 additions & 2 deletions py/send_message.py
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ def _sign_btc_normal(
for input_index, sig in sigs:
print("Signature for input {}: {}".format(input_index, sig.hex()))

def _sign_btc_send_to_self(
def _sign_btc_send_to_self_same_account(
self,
format_unit: "bitbox02.btc.BTCSignInitRequest.FormatUnit.V" = bitbox02.btc.BTCSignInitRequest.FormatUnit.DEFAULT,
) -> None:
Expand Down Expand Up @@ -561,6 +561,50 @@ def _sign_btc_send_to_self(
for input_index, sig in sigs:
print("Signature for input {}: {}".format(input_index, sig.hex()))

def _sign_btc_send_to_self_different_account(
self,
format_unit: "bitbox02.btc.BTCSignInitRequest.FormatUnit.V" = bitbox02.btc.BTCSignInitRequest.FormatUnit.DEFAULT,
) -> None:
# pylint: disable=no-member
bip44_account: int = 0 + HARDENED
inputs, outputs = _btc_demo_inputs_outputs(bip44_account)
outputs[1] = bitbox02.BTCOutputInternal(
keypath=[84 + HARDENED, 0 + HARDENED, 1 + HARDENED, 0, 0],
value=int(1e8 * 0.2),
script_config_index=0,
output_script_config_index=0,
)
sigs = self._device.btc_sign(
bitbox02.btc.BTC,
[
bitbox02.btc.BTCScriptConfigWithKeypath(
script_config=bitbox02.btc.BTCScriptConfig(
simple_type=bitbox02.btc.BTCScriptConfig.P2WPKH
),
keypath=[84 + HARDENED, 0 + HARDENED, bip44_account],
),
bitbox02.btc.BTCScriptConfigWithKeypath(
script_config=bitbox02.btc.BTCScriptConfig(
simple_type=bitbox02.btc.BTCScriptConfig.P2WPKH_P2SH
),
keypath=[49 + HARDENED, 0 + HARDENED, bip44_account],
),
],
inputs=inputs,
outputs=outputs,
format_unit=format_unit,
output_script_configs=[
bitbox02.btc.BTCScriptConfigWithKeypath(
script_config=bitbox02.btc.BTCScriptConfig(
simple_type=bitbox02.btc.BTCScriptConfig.P2WPKH
),
keypath=[84 + HARDENED, 0 + HARDENED, 1 + HARDENED],
),
],
)
for input_index, sig in sigs:
print("Signature for input {}: {}".format(input_index, sig.hex()))

def _sign_btc_high_fee(self) -> None:
# pylint: disable=no-member
bip44_account: int = 0 + HARDENED
Expand Down Expand Up @@ -832,7 +876,8 @@ def _sign_btc_tx(self) -> None:
format_unit=bitbox02.btc.BTCSignInitRequest.FormatUnit.SAT
),
),
("Send to self", self._sign_btc_send_to_self),
("Send to self (same account)", self._sign_btc_send_to_self_same_account),
("Send to self (different account)", self._sign_btc_send_to_self_different_account),
("High fee warning", self._sign_btc_high_fee),
("Multiple change outputs", self._sign_btc_multiple_changes),
("Locktime/RBF", self._sign_btc_locktime_rbf),
Expand Down
5 changes: 1 addition & 4 deletions src/rust/bitbox02-rust/src/hww/api/bitcoin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,7 @@ async fn address_policy(

let parsed = policies::parse(policy, coin)?;

let name = match policies::get_name(coin, policy)? {
Some(name) => name,
None => return Err(Error::InvalidInput),
};
let name = parsed.name(coin_params)?.ok_or(Error::InvalidInput)?;

let title = "Receive to";

Expand Down
6 changes: 4 additions & 2 deletions src/rust/bitbox02-rust/src/hww/api/bitcoin/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,13 +191,15 @@ impl Payload {
ValidatedScriptConfig::SimpleType(simple_type) => {
Self::from_simple(xpub_cache, params, *simple_type, keypath)
}
ValidatedScriptConfig::Multisig(multisig) => Self::from_multisig(
ValidatedScriptConfig::Multisig { multisig, .. } => Self::from_multisig(
params,
multisig,
keypath[keypath.len() - 2],
keypath[keypath.len() - 1],
),
ValidatedScriptConfig::Policy(policy) => Self::from_policy(params, policy, keypath),
ValidatedScriptConfig::Policy { parsed_policy, .. } => {
Self::from_policy(params, parsed_policy, keypath)
}
}
}

Expand Down
9 changes: 8 additions & 1 deletion src/rust/bitbox02-rust/src/hww/api/bitcoin/policies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,13 @@ pub struct ParsedPolicy<'a> {
}

impl ParsedPolicy<'_> {
/// Get the name of a registered policy account.
///
/// Returns the name of the registered policy account if it exists or None otherwise.
pub fn name(&self, params: &Params) -> Result<Option<String>, ()> {
get_name(params.coin, self.policy)
}

/// Iterates over the placeholder keys in this descriptor. For tr() descriptors, this covers the
/// internal key and every key in every leaf script.
/// This iterates the keys "left-to-right" in the descriptor.
Expand Down Expand Up @@ -739,7 +746,7 @@ pub fn get_hash(coin: BtcCoin, policy: &Policy) -> Result<Vec<u8>, ()> {
Ok(hasher.finalize().as_slice().into())
}

/// Get the name of a registered policy account. The poliy is not validated, it must be
/// Get the name of a registered policy account. The policy is not validated, it must be
/// pre-validated!
///
/// Returns the name of the registered policy account if it exists or None otherwise.
Expand Down
Loading