General feedback

[josibake]

Hey! Reading through these over the next couple of days and I'll be adding my feedback here. Also happy to open pull requests, if that is easier.

First off, I'd suggest putting the notebooks into chapter-specific folders. I found it a bit confusing to have chapters laid out in the README but then a single folder with all the notebooks and no way to tell which notebook belonged to which chapter without referencing the readme. I'd suggest something like ch-01/notebooks.ipynb or chapter-01/. Alternatively, you could prepend the notebook names with ch-01-blabla.ipynb.

I'd also suggest a consistent naming convention for the notebooks. I tend to prefer all lowercase, hyphenated names: my-really-cool-notebook.ipynb. I don't think the style of naming matters, just that it is consistent.

Will leave content-specific feedback on this issue as I dig in more!

[jarolrod]

yes chapters seems like a requirement, I want to to be told when to start and to know how to progress through it. The notebooks currently display in alphabetical order, so it's not intuitive as to where to begin.

[DariusParvin]

Thanks @josibake ! Yeah that makes sense. I'll move them into folders and also give them a more consistent naming convention. And yeah, any PRs would be appreciated :)

@jarolrod the readme was meant to provide the ordering, but like Josie said - it would be better to not have to rely on it.

Todo:

• move chapters into folders
• use a consistent naming convention

[josibake]

unfortunately, it looks like moving notebooks into sub-folders broke the functions import :(

it's been a while since I've messed with python imports, but I'm sure there is a way to make everything from functions importable even with the notebooks in sub-directories.

[josibake]

Another thing I ran into while going through chapter1-legacy/p2pkh.ipynb was hashlib failing due to ripemd160 not being supported. I followed the instructions from https://stackoverflow.com/questions/72409563/unsupported-hash-type-ripemd160-with-hashlib-in-python to modify my openssl config to get it working.

If there is a better python library we can use to avoid this, that would be great. Otherwise, it might be worth mentioning in the README that you will need to modify your openssl config if you are running a newer version of openssl

[josibake]

Similar to the taproot workbooks, I think we could also use TestShell here instead of relying on a locally installed bitcoin binary. The advantages of approach are:

• The user only needs to have the repo cloned and compiled
• TestShell uses the test framework, so it only runs on regtest in temporary folders
• Guarantees no interaction with the users locally installed bitcoin setup
• Doesn't require the user to install bitcoind/bitcoin-cli to use the tutorials

I wrote an example notebook to demonstrate what it would look like here: https://github.com/josibake/bitcoin-tx-tutorial/blob/main/p2pkh-testshell.ipynb

[DariusParvin]

Thanks for finding these issues. That's a bit annoying about hashlib. I agree that the better solution is to move towards using the TestShell, even for the other benefits alone. The example notebook is awesome! I'll try running it locally and move things over to TestShell