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

Vendor bitbox02 library #683

Merged
merged 8 commits into from Jul 18, 2023
Merged

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Jul 6, 2023

This brings the bitbox02 implementation in line with the implementations of the other hardware wallets as we include copies of their respective libraries in our source tree rather than just including them as a dependency. Alternative to #677

Also an issue with bitbox02 in the GUI is fixed.

Fixes #675

semver is required by jadepy and bitbox02
noiseprotocol and protobuf are required by bitbox02
Bring in a copy of the bitbox02 python library. Not yet used.
Modify bitbox02's vendored library to use relative imports.

Also modify to use our own _base58 instead of an external module.
Use the vendored package instead installing it as an external
dependency.
@achow101
Copy link
Member Author

achow101 commented Jul 6, 2023

cc @benma

@benma
Copy link
Contributor

benma commented Jul 6, 2023

Thanks. Looking at this now, I see you need to add the deps of the bitbox02 deps manually. This is an obvious disadvantage - if the bitbox02 lib gets new deps or bumps its dep versions to fix issues, this would have to be done here as well.

What is the problem exactly with using a regular dep? The linked issue is not convincing to me:

For a user that does not have a bitbox device, this is an extra dependency and extra code that is not needed.

But the code is still there, just vendored.

In the end I think a regular dep is preferable for the above reason, and should also be easier to manage. If you have other reasons to vendor, please let me know and proceed anyway. In that case it would be helpful to add the commands you used to vendor in the commit msg (or in the README) so that updating it to newer versions is easier.

@achow101
Copy link
Member Author

achow101 commented Jul 7, 2023

The vendoring of device libraries is done for two reasons: 1) to remove unnecessary dependencies and code, and 2) to lock in the exact API that we are using. We have had issues in the past where differing library versions on a user's system have resulted in incompatibiliies and unexpected behavior. Vendoring the device library lets us ensure that the version of the library that ends up being used is the one that we have tested against, and it gives us better control over dependencies.

We mainly want to avoid letting external dependencies pull in even more dependencies that completely blow up our dependency stack which makes it hard to actually know what code is being run and if any of it is malicious.

Honestly, I'm not particularly thrilled by the dependencies that supporting bitbox02 and some other recent devices have forced us to add.

@benma
Copy link
Contributor

benma commented Jul 8, 2023

Sounds reasonable 👍 Thanks for elaborating.

If it helps: noiseprotocol is used to encrypt the communication (see http://www.noiseprotocol.org/), protobuf is used to encode/decode the API messages to/from the BitBox02. semver is used to enable/disable support for features depending on the firmware version.

@achow101 achow101 merged commit d209333 into bitcoin-core:master Jul 18, 2023
7 of 135 checks passed
@prusnak
Copy link
Collaborator

prusnak commented Jul 18, 2023

Honestly, I'm not particularly thrilled by the dependencies that supporting bitbox02 and some other recent devices have forced us to add.

Btw, you can drop the protobuf dependency if you rewrite the bitbox02 code to use https://github.com/bitcoin-core/HWI/blob/master/hwilib/devices/trezorlib/protobuf.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bitbox02 should not be a hard dependency
3 participants