simulator: implement a simulator for bitbox02 device #1167

asi345 · 2024-01-25T16:29:56Z

HWI is thinking of updating its support policy such that supported wallets must implement a simulator/emulator. See bitcoin-core/HWI#685. That's why a simulator is implemented for bitbox02, supporting functionalities of its API.

This first version of the simulator is capable of nearly every functionality of a normal Bitbox02 device, without promising any security or production use. Its main aim is to be able to run unit tests for features and test the API.

In addition, it will be configured to run automated tests in CI, which helps both us and HWI integration.

Right now, the simulator has 3 different ways to communicate with a client: giving inputs/getting output from CLI, using pipes or opening sockets. Socket is the most convenient and reliable choice in this version. It expects the clients to open a socket on port 15432, which is selected intentionally to avoid possible conflicts.

The simulator resides with C unit-tests since it uses same mocks, therefore it can be built by make unit-test.

Lastly, Python client implemented in py/send_message.py is updated to support communicating with simulator with the socket configuration mentioned above. Client can be started up with ./py/send_message.py --simulator command. To run the simulator, build-build/bin/test_simulator command is sufficient.

benma · 2024-01-31T10:06:59Z

src/rust/bitbox02/src/ui/ui_stub_c.rs

@@ -0,0 +1,163 @@
+// Copyright 2020 Shift Crypto AG


benma · 2024-01-31T10:07:27Z

src/rust/bitbox02/src/ui/ui_stub_c.rs

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Stubs for testing.


elaborate that this is for the C unit tests and the simulator in particular.

benma · 2024-01-31T10:08:01Z

src/rust/bitbox02/src/ui.rs

@@ -16,6 +16,7 @@
 mod types;

 #[cfg_attr(feature = "testing", path = "ui/ui_stub.rs")]
+#[cfg_attr(feature = "c-unit-testing", path = "ui/ui_stub_c.rs")]


rename to sth clearer like ui_stub_c_unit_tests.rs

benma · 2024-01-31T10:09:22Z

test/unit-test/framework/mock_securechip.c

@@ -23,6 +23,8 @@
 #include <string.h>
 #include <wally_crypto.h>

+#define COUNTER_MAX_VALUE ((uint32_t)2097151)


where is this value coming from? you can just return any arbitrary value in securechip_monotonic_increments_remaining()

benma · 2024-01-31T10:09:38Z

test/unit-test/test_simulator.c

@@ -0,0 +1,368 @@
+// Copyright 2023 Shift Crypto AG


benma · 2024-01-31T10:15:01Z

test/unit-test/CMakeLists.txt

@@ -240,6 +240,8 @@ set(TEST_LIST
   "-Wl,--wrap=screen_process"
   ugui
   ""
+   simulator


as it is in the the list of tests, the cmocka lib will be linked to it, but is cmocka used? ideally it should not be needed for the simulator. can you try linking the simulator without cmocka?

benma · 2024-01-31T11:00:47Z

test/unit-test/test_simulator.c

+    printf("Sd card setup %s\n", sd_success ? "success" : "failed");
+    mock_memory_factoryreset();
+    memory_interface_functions_t ifs = {
+        .random_32_bytes = _memory_setup_rand_mock,


= random_32_bytes_mcu and #include<random.h> at the top, which uses C's rand() to get pseudorandom numbers, no need for the fixtures.

benma · 2024-01-31T11:01:05Z

test/unit-test/test_simulator.c

+        .random_32_bytes = _memory_setup_rand_mock,
+    };
+    memory_success = memory_setup(&ifs);
+    printf("Memory setup %s\n", memory_success ? "success" : "failed");


properly abort/fail on errors

benma · 2024-01-31T14:56:57Z

src/rust/bitbox02-rust/src/workflow/mnemonic.rs


-use sha2::{Digest, Sha256};
+cfg_if::cfg_if! {


i think with that many changes, it's better to add a new file mnemonic_stub_c_unit_tests.rs, so the real firmware code does not become obfuscated by all this

benma

looking nice, just a few nits!

benma · 2024-02-01T08:49:44Z

src/rust/bitbox02-rust/src/workflow/mnemonic_c_unit_tests.rs

+use alloc::string::ToString;
+
+pub async fn show_and_confirm_mnemonic(words: &[&str]) -> Result<(), CancelError> {
+    let _ = confirm::confirm(&confirm::Params {


not sure what the purpose is of the contents of this function - it could just be empty right?

benma · 2024-02-01T08:51:38Z

test/unit-test/test_simulator.c

+#include <sys/socket.h>
+
+#define BUFFER_SIZE 1024
+#define SOCKET


benma · 2024-02-01T08:52:29Z

test/unit-test/test_simulator.c

+    int (*get_message)(char*, uint8_t*);
+    void (*send_message)(void);


not needed? can just use get_usb_message_socket and send_usb_message_socket directly

benma · 2024-02-01T08:54:26Z

test/unit-test/test_simulator.c

+    if (data != NULL) {
+        data_len = 256 * (int)data[5] + (int)data[6];
+        for (int i = 0; i < USB_HID_REPORT_OUT_SIZE; i++) {
+            out_buffer[i] = (char)data[i];


what's the point of this var? why nt just data in the next line in write()?

benma · 2024-02-01T08:56:47Z

test/unit-test/test_simulator.c

+    }
+}
+
+void simulate_firmware_execution(uint8_t* input)


const uint8_t*

benma · 2024-02-01T08:57:04Z

test/unit-test/test_simulator.c

+
+void simulate_firmware_execution(uint8_t* input)
+{
+    memcpy(_out_report, input, sizeof(_out_report));


no need for _out_report and this copy? can just use input on the next line

benma · 2024-02-01T08:57:59Z

test/unit-test/test_simulator.c

+    bool memory_success, sd_success;
+    int temp_len;


no need to declare the vars at the top, code is imo nicer when the vars are just declared on first use, since we don't care much about stack use. bool memory_success = memory_setup(...) etc.

benma · 2024-02-01T08:58:34Z

test/unit-test/test_simulator.c

+    int portno;
+    struct sockaddr_in serv_addr;
+    struct hostent* server;
+    portno = 15423;


same, one line is better than two lines ;) int portno = 15423. could also prefix with const since it's not changing.

benma

tACK - very nice work!

benma · 2024-02-07T08:34:07Z

test/unit-test/test_simulator.c

+#define BUFFER_SIZE 1024
+
+int data_len;
+int sockfd;


make sockfd* an argument to the get_usb_message_socket function, then you don't need a global sockfd variable and it can be local.

looks like we can't do much about data_len being global for now, that seems it would require some refactoring of the usb processing functions, which we don't need to go into now 😄

benma · 2024-02-07T08:36:01Z

test/unit-test/test_simulator.c

+    int temp_len;
+    while (1) {
+        if (!get_usb_message_socket(input)) break;
+        simulate_firmware_execution(input);


some comments here explaining how long packets are read and send would be helpful for the future, it's not very obvious because the usb stack is so convoluted.

HWI is thinking of updating its support policy such that supported wallets must implement a simulator/emulator. See bitcoin-core/HWI#685. That's why a simulator is implemented for bitbox02, supporting functionalities of its API. This first version of the simulator is capable of nearly every functionality of a normal Bitbox02 device, without promising any security or production use. Its main aim is to be able to run unit tests for features and test the API. In addition, it will be configured to run automated tests in CI, which helps both us and HWI integration. Right now, the simulator has 3 different ways to communicate with a client: giving inputs/getting output from CLI, using pipes or opening sockets. Socket is the most convenient and reliable choice in this version. It expects the clients to open a socket on port 15432, which is selected intentionally to avoid possible conflicts. The simulator resides with C unit-tests since it uses same mocks, therefore it can be built by `make unit-test`. Lastly, Python client implemented in `py/send_message.py` is updated to support communicating with simulator with the socket configuration mentioned above. Client can be started up with `./py/send_message.py --simulator` command. To run the simulator, `build-build/bin/test_simulator` command is sufficient. Signed-off-by: asi345 <[email protected]>

benma reviewed Jan 31, 2024

View reviewed changes

src/rust/bitbox02/src/ui/ui_stub_c.rs Outdated

@@ -0,0 +1,163 @@

// Copyright 2020 Shift Crypto AG

Copy link

Collaborator

benma Jan 31, 2024

Choose a reason for hiding this comment

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

wrong year

benma reviewed Jan 31, 2024

View reviewed changes

asi345 force-pushed the simulator branch 4 times, most recently from 5bb774a to 96e1d59 Compare January 31, 2024 14:30

benma reviewed Jan 31, 2024

View reviewed changes

asi345 force-pushed the simulator branch 2 times, most recently from df29892 to 49f512c Compare January 31, 2024 15:40

benma requested changes Feb 1, 2024

View reviewed changes

asi345 force-pushed the simulator branch from 49f512c to 689b85a Compare February 1, 2024 10:29

benma approved these changes Feb 7, 2024

View reviewed changes

asi345 force-pushed the simulator branch from 689b85a to a1c553b Compare February 7, 2024 08:59

asi345 merged commit b8ec18e into BitBoxSwiss:master Feb 7, 2024
1 check passed

schmitmd mentioned this pull request Jul 14, 2024

[Feature request] Simulator #55

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simulator: implement a simulator for bitbox02 device #1167

simulator: implement a simulator for bitbox02 device #1167

asi345 commented Jan 25, 2024

benma Jan 31, 2024

benma Jan 31, 2024

benma Jan 31, 2024

benma Jan 31, 2024

benma Jan 31, 2024

benma Jan 31, 2024

benma Jan 31, 2024

benma Jan 31, 2024

benma Jan 31, 2024

benma left a comment

benma Feb 1, 2024

benma Feb 1, 2024

benma Feb 1, 2024

benma Feb 1, 2024

benma Feb 1, 2024

benma Feb 1, 2024

benma Feb 1, 2024

benma Feb 1, 2024

benma left a comment

benma Feb 7, 2024

benma Feb 7, 2024

		int (get_message)(char, uint8_t*);
		void (*send_message)(void);

simulator: implement a simulator for bitbox02 device #1167

simulator: implement a simulator for bitbox02 device #1167

Conversation

asi345 commented Jan 25, 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

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

benma left a 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

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

benma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment