Add BitBox02 Simulator #741

asi345 · 2024-06-05T21:44:34Z

Recently, a simulator for BitBox02 was implemented. Therefore, we also wanted to add this to HWI repository for automated tests and ease of reference in the future.

benma · 2024-06-06T12:42:01Z

fyi @achow101, I reviewed this before @asi345 made the PR

achow101 · 2024-06-24T20:34:40Z

test/test_device.py

@@ -130,11 +129,12 @@ def create(*args, **kwargs):
        return c

 class DeviceTestCase(unittest.TestCase):
-    def __init__(self, bitcoind, emulator=None, interface='library', methodName='runTest'):
+    def __init__(self, bitcoind, emulator=None, interface='library', methodName='runTest', supports_legacy=True):


supports_legacy should be a property of the emulator, not an argument to the test case. See supports_taproot.

Okay, we can use this convention. I'll updating that shortly, but then I will also add the relevant field to the other emulators.

achow101 · 2024-06-24T20:35:23Z

test/test_device.py

-        keypool_desc = self.do_command(self.dev_args + ["getkeypool", "--account", "10", "--addr-type", "legacy", "0", "100"])
+        addr_type = "legacy" if self.supports_legacy else "sh_wit"
+        keypool_desc = self.do_command(self.dev_args + ["getkeypool", "--account", "10", "--addr-type", addr_type, "0", "100"])


It should be fine to just change the address type to sh_wit always instead of using legacy for the other devices.

Okay, changed the types to sh_wit

achow101 · 2024-06-24T20:41:28Z

test/setup_environment.sh

+    docker pull shiftcrypto/firmware_v2:latest
+    # The safe.directory config is so that git commands work. even though the repo folder mounted in
+    # Docker is owned by root, which can be different from the owner on the host.
+    docker run -i --rm --volume ./:/bb02 shiftcrypto/firmware_v2 bash -c "git config --global --add safe.directory /bb02 && cd /bb02 && make -j simulator"


This script needs to work when run inside a docker container as that is how the tests are suggested to be run locally. IIRC Docker within docker doesn't really work, so I don't think this will.

asi345 · 2024-07-17T17:49:30Z

Okay, we also came up with a solution for docker in docker problem for bitbox02 simulator.
Now, a separate build_bitbox02.sh script is ran before running hwi container, to build BitBox02 simulator.
See the comments in ci Dockerfile for reference.

benma · 2024-08-22T11:47:29Z

test/README.md

+Pull the BitBox02 firmware Docker image:
+
+```
+docker pull shiftcrypto/firmware_v2:latest


Replace with

cd bitbox02-firmware make dockerpull

(remove the cd below then)

benma · 2024-08-22T11:49:12Z

test/setup_environment.sh

+    fi
+
+    # Build the simulator. This is cached, but it is also fast
+    docker pull shiftcrypto/firmware_v2:latest


use $(cat .containerversion) instead of latest here too

benma · 2024-08-22T12:03:45Z

@achow101 please approve the workflow and take another look at the changes, would be really nice to have this merged soon. Thanks!

achow101 · 2024-08-30T17:06:57Z

Could you rebase so that CI runs the tests with the simulator?

brunoerg · 2024-08-30T19:10:45Z

test/README.md

+In order to build the BitBox02 simulator, the following packages will need to be installed:
+
+```
+apt install docker.io


Isn't there a way to build it without docker?

Not really, this is how we build and release the BB02 firmware. Replicating the Docker environment outside of it is bound to lead to tons of overhead in maintenance and CI breaks.

podman instead of docker would also work though.

Ok, anyway, I could build and run it. No problem.

brunoerg · 2024-08-30T20:48:13Z

test/test_bitbox02.py

+    parser.add_argument('--interface', help='Which interface to send commands over', choices=['library', 'cli', 'bindist'], default='library')
+    args = parser.parse_args()
+
+    sys.exit(not bitbox02_test_suite(args.simulator, None, None))


I had some problems with this test. It seems it should pass bitcoind and interface in bitbox02_test_suite instead of None, no?

Yes, you're right. Fixed that now.

brunoerg · 2024-08-30T21:23:18Z

test/setup_environment.sh

+    docker pull shiftcrypto/firmware_v2:$CONTAINER_VERSION
+    # The safe.directory config is so that git commands work. even though the repo folder mounted in
+    # Docker is owned by root, which can be different from the owner on the host.
+    docker run -i --rm --volume ./:/bb02 shiftcrypto/firmware_v2:$CONTAINER_VERSION bash -c "git config --global --add safe.directory /bb02 && cd /bb02 && make -j simulator"


Got this error running setup_environment.sh:

+ docker run -i --rm --volume ./:/bb02 shiftcrypto/firmware_v2:42 bash -c 'git config --global --add safe.directory /bb02 && cd /bb02 && make -j simulator' docker: Error response from daemon: create ./: "./" includes invalid characters for a local volume name, only "[a-zA-Z0-9][a-zA-Z0-9_.-]" are allowed. If you intended to pass a host directory, use absolute path.

I think you could use "$(pwd):/bb02" instead of ./:/bb02.

This was not an issue in CI, but in local maybe that created problems. Anyways, I changed to $(pwd) instead of . as you suggested. This should solve the error.

asi345 · 2024-09-04T08:43:21Z

Okay, I rebased this branch on top of master. CI is again waiting for your approval.

However, can you in any chance fix bitcoind build first? This commit breaks bitcoind build in setup_environment.sh, since autogen.sh file is not existent in bitcoin repo anymore : bitcoin/bitcoin@d71ac76

brunoerg · 2024-09-04T09:35:15Z

However, can you in any chance fix bitcoind build first? This commit breaks bitcoind build in setup_environment.sh, since autogen.sh file is not existent in bitcoin repo anymore : bitcoin/bitcoin@d71ac76

See #752

brunoerg · 2024-09-04T20:42:13Z

ci/build_bitbox02.sh

+docker volume create bitbox02_volume
+CONTAINER_VERSION=$(curl https://raw.githubusercontent.com/BitBoxSwiss/bitbox02-firmware/master/.containerversion)
+docker pull shiftcrypto/firmware_v2:$CONTAINER_VERSION
+docker run -i --rm -v bitbox02_volume:/bitbox02-firmware shiftcrypto/firmware_v2:$CONTAINER_VERSIONs bash -c \


Suggested change

docker run -i --rm -v bitbox02_volume:/bitbox02-firmware shiftcrypto/firmware_v2:$CONTAINER_VERSIONs bash -c \

docker run -i --rm -v bitbox02_volume:/bitbox02-firmware shiftcrypto/firmware_v2:$CONTAINER_VERSION bash -c \

brunoerg · 2024-09-04T21:40:15Z

hwilib/devices/bitbox02.py

+    def __init__(self) -> None:
+        self.client_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+        ip, port = SIMULATOR_PATH.split(":")
+        self.client_socket.connect((ip, int(port)))


It throws ConnectionRefusedError if it wasn't able to connect to the BitBox02 simulator, so we won't be able to use any other simulator.

As far as I saw, Trezor or Keepkey also call connect on their simulators without handling any exceptions or connect_ex. Do you have a suggestion how to handle this exactly?

As far as I saw, Trezor or Keepkey also call connect on their simulators without handling any exceptions or connect_ex.

See:

def enumerate(password: Optional[str] = None, expert: bool = False, chain: Chain = Chain.MAIN, allow_emulators: bool = False) -> List[Dict[str, Any]]: results = [] devs = hid.HidTransport.enumerate() devs.extend(webusb.WebUsbTransport.enumerate()) if allow_emulators: devs.extend(udp.UdpTransport.enumerate()) for dev in devs:

When calling enumerate with --simulators, Trezor will call enumerate from UdpTransport. This function will return nothing if it wasn't able to find a device (simulator) so devs is empty.

Maybe you could do something like:

diff --git a/hwilib/devices/bitbox02.py b/hwilib/devices/bitbox02.py index 53497c6..7cd6929 100644 --- a/hwilib/devices/bitbox02.py +++ b/hwilib/devices/bitbox02.py @@ -186,6 +186,8 @@ def enumerate(password: Optional[str] = None, expert: bool = False, chain: Chain devs.append(SIMULATOR_PATH) for path in devs: client = Bitbox02Client(path=path) + if allow_emulators and not client.transport: + continue if path != SIMULATOR_PATH: client.set_noise_config(SilentNoiseConfig()) d_data: Dict[str, object] = {} @@ -267,7 +269,11 @@ class BitBox02Simulator(): def __init__(self) -> None: self.client_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM) ip, port = SIMULATOR_PATH.split(":") - self.client_socket.connect((ip, int(port))) + self.connected = True + try: + self.client_socket.connect((ip, int(port))) + except: + self.connected = False def write(self, data: bytes) -> None: # Messages from client are always prefixed with HID report ID(0x00), which is not expected by the simulator. @@ -301,7 +307,10 @@ class Bitbox02Client(HardwareWalletClient): self.noise_config = CLINoiseConfig() else: simulator = BitBox02Simulator() - self.transport = u2fhid.U2FHid(simulator) + if simulator.connected: + self.transport = u2fhid.U2FHid(simulator) + else: + self.transport = None self.device_path = path # use self.init() to access self.bb02.

Okay, I did a fix based on this(not the same since this does not pass type checks). Now, in my tests, other devices are also passing the tests.

achow101 · 2024-09-12T14:55:20Z

The current implementation is causing all other tests to fail.

Signed-off-by: asi345 <[email protected]>

asi345 · 2024-09-25T14:03:55Z

I updated the commit to handle connection refused on BitBox02 simulator, now it seems that other devices do not fail.

benma · 2024-10-28T18:56:18Z

The current implementation is causing all other tests to fail.

@achow101 please take a look again, @asi345 updated the PR and all checks have passed.

achow101 reviewed Jun 24, 2024

View reviewed changes

benma mentioned this pull request Jul 14, 2024

[Feature request] Simulator BitBoxSwiss/bitbox02-firmware#55

Closed

asi345 force-pushed the bitbox02_simulator branch 2 times, most recently from 5e94111 to cacfcc6 Compare July 17, 2024 17:45

asi345 force-pushed the bitbox02_simulator branch from cacfcc6 to 7a1d0c1 Compare August 22, 2024 11:39

benma reviewed Aug 22, 2024

View reviewed changes

asi345 force-pushed the bitbox02_simulator branch from 7a1d0c1 to b8b679a Compare August 22, 2024 11:55

brunoerg reviewed Aug 30, 2024

View reviewed changes

asi345 force-pushed the bitbox02_simulator branch from b8b679a to 97635bf Compare September 4, 2024 08:21

brunoerg reviewed Sep 4, 2024

View reviewed changes

asi345 force-pushed the bitbox02_simulator branch from 97635bf to 8b76ed6 Compare September 5, 2024 11:17

bitbox02 library: Update to v6.3.0

000e5c5

Signed-off-by: asi345 <[email protected]>

asi345 force-pushed the bitbox02_simulator branch from 8b76ed6 to abc4ac8 Compare September 25, 2024 12:59

bitbox02: add bitbox02 simulator and tests

3c3a185

Signed-off-by: asi345 <[email protected]>

asi345 force-pushed the bitbox02_simulator branch from abc4ac8 to 3c3a185 Compare September 25, 2024 13:32

Thompson1985 approved these changes Oct 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BitBox02 Simulator #741

Add BitBox02 Simulator #741

asi345 commented Jun 5, 2024

benma commented Jun 6, 2024

achow101 Jun 24, 2024

asi345 Jun 26, 2024

achow101 Jun 24, 2024

asi345 Jul 17, 2024

achow101 Jun 24, 2024

asi345 commented Jul 17, 2024

benma Aug 22, 2024

benma Aug 22, 2024

benma commented Aug 22, 2024

achow101 commented Aug 30, 2024

brunoerg Aug 30, 2024

benma Aug 30, 2024

brunoerg Aug 30, 2024

brunoerg Aug 30, 2024 •

edited

Loading

asi345 Sep 4, 2024

brunoerg Aug 30, 2024

asi345 Sep 4, 2024

asi345 commented Sep 4, 2024

brunoerg commented Sep 4, 2024

brunoerg Sep 4, 2024

brunoerg Sep 4, 2024 •

edited

Loading

asi345 Sep 5, 2024

brunoerg Sep 5, 2024

asi345 Sep 25, 2024

achow101 commented Sep 12, 2024

asi345 commented Sep 25, 2024

benma commented Oct 28, 2024 •

edited

Loading

	docker run -i --rm -v bitbox02_volume:/bitbox02-firmware shiftcrypto/firmware_v2:$CONTAINER_VERSIONs bash -c \
	docker run -i --rm -v bitbox02_volume:/bitbox02-firmware shiftcrypto/firmware_v2:$CONTAINER_VERSION bash -c \

Add BitBox02 Simulator #741

Are you sure you want to change the base?

Add BitBox02 Simulator #741

Conversation

asi345 commented Jun 5, 2024

benma commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asi345 commented Jul 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma commented Aug 22, 2024

achow101 commented Aug 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoerg Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asi345 commented Sep 4, 2024

brunoerg commented Sep 4, 2024

Choose a reason for hiding this comment

brunoerg Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Sep 12, 2024

asi345 commented Sep 25, 2024

benma commented Oct 28, 2024 • edited Loading

brunoerg Aug 30, 2024 •

edited

Loading

brunoerg Sep 4, 2024 •

edited

Loading

benma commented Oct 28, 2024 •

edited

Loading