buidl-bitcoin / buidl-python

python3 bitcoin library with no dependencies and extensive test coverage
https://pypi.org/project/buidl/
MIT License
83 stars 26 forks source link

Multiwallet Feature: Generate Seed from Dice Rolls #147

Open tdb3 opened 1 year ago

tdb3 commented 1 year ago

Added capability to generate a mnemonic from dice rolls. Also added a convenience feature to multiwallet, guiding a user through seed creation from dice rolls. The feature can be accessed through the menu option generate_seed_from_dice. The test cases are based on Coinkite's examples (https://coldcard.com/docs/verifying-dice-roll-math).

Happy to tweak as needed.

mflaxman commented 1 year ago

Very cool, thank you for the PR @tdb3! I'm partial to pulling words out of a hat (vs rolling dice) as it's one less step to verify (dice->words->xpub vs just words->xpub), but I like having support for both as long as it's done in a way that matches other implementations.

I think this should be part of the HDPrivateKey class, perhaps a new HDPrivateKey.from_dice() method? That way it's accessible to developers using buidl (not just multiwallet users) and also so it can have bulletproof unit testing (the testing harness for the CLI apps is admittedly a bit janky). What do you think of moving it there?

tdb3 commented 1 year ago

Sounds good. How about something like the following?

Since HDPrivateKey.from_dice() is essentially a specific example of creating a seed from bytes, a more general HDPrivateKey.from_bytes() is created, which from_dice() calls. HDPrivateKey.from_bytes() incorporates similar arguments (e.g., optional password, etc.) as HDPrivateKey.from_mnemonic(), keeps the HMAC SHA512 approach (for integration of the password as salt, and stretching), and returns the HDPrivateKey as well as the associated mnemonic words generated from the bytes provided (for later re-use).

Modified approach adds dice_rolls_to_mnemonic() to mnemonic.

mflaxman commented 1 year ago

I'm not 100% sure I'm following what you're saying, maybe it would be easier if you wrote it as a PR and then it will be easy to comment on the code?

My concern with a HDPrivateKey.from_bytes() is developer ergonomics. buidl is very cautious about making it hard for regular uesrs to footgun, and only experts should go outside commonly accepted standards (i.e. bip39 mnemonics). If there's a need for a from_bytes() method, it should at least be a private method _from_bytes(), although python doesn't enforce that it's at least provides some signaling.

tdb3 commented 9 months ago

Rebased, modified approach to address comments from @mflaxman, force pushed.

tdb3 commented 9 months ago

Force pushed to address linter error.

mflaxman commented 9 months ago

Thanks for the PR @tdb3!

Why support only 24 words? It's the default/best choice, but I know lots of people with 12 word seed phrases that use passphrases for extra entropy. I don't think this is a requirement for merging, I'd just think you'd want to support it since you already did most of the work.

I added a few small comments, overall nicely done!

tdb3 commented 9 months ago

Great comments. Comments addressed inline. Force pushed.

Updated to support 12 through 24 words.

Overall, decided to simplify things. HDPrivateKey.from_dice() isn't an absolute necessity when dice_rolls_to_mnemonic() exists. dice_rolls_to_mnemonic() is sufficient to provide the mnemonic words that can then be used with HDPrivateKey.from_mnemonic(). This results in a smaller code footprint.

Added additional test cases for 12-word (and other) entries, and edge cases (such as only using values 1-6, string provided, minimum entropy checking). These were double-checked against https://iancoleman.io/bip39/ and https://jlopp.github.io/xpub-converter/

Multiwallet does some basic sanity checking (e.g. that the number of mnemonic words being generated from dice rolls is 12 to 24 words, inclusive), and receives raised exceptions from dice_rolls_to_mnemonic() when improper data is entered (e.g. wrong dice values, insufficient entropy). This way other applications can receive the same error handling through dice_rolls_to_mnemonic().

Below is the output from Coinkite's rolls12.py script for comparison (these are baked into the unit tests):

$ python3 rolls12.py 

e3b0c44298fc1c149afbf4c8996fb924

WARNING: Input is empty. This is a known wallet

WARNING: Input is only 0 bits of entropy

   1: together
   2: mail
   3: awful
   4: cradle
   5: scrub
   6: apart
   7: hip
   8: leader
   9: silk
  10: slice
  11: unusual
  12: embark

$ python3 rolls12.py 
123456
8d969eef6ecad3c29a3a629280e686cf

WARNING: Input is only 15 bits of entropy

   1: mirror
   2: reject
   3: rookie
   4: talk
   5: pudding
   6: throw
   7: happy
   8: era
   9: myth
  10: already
  11: payment
  12: owner

$ python3 rolls12.py 
12345612345612345612345612345612345612345612345612
ee72ae915a4e6ea7ccbeb8e5e5eecef2

   1: unveil
   2: nice
   3: picture
   4: region
   5: tragic
   6: fault
   7: cream
   8: strike
   9: tourist
  10: control
  11: recipe
  12: tourist
mflaxman commented 9 months ago

Also, this is a substantial PR! Let's bump the version number in setup.py.