Added Jupyter Chapter 1.2 (Musig Introduction) - #10
Conversation
|
rebased on master |
jnewbery
left a comment
There was a problem hiding this comment.
This is looking good. Lots of nits inline.
Some more general points:
- what tool do you use to make the images/diagrams? Can you commit the source file so the diagrams can be updated later?
- For future chapters, I suggest you just add the solution file for initial review. That way you won't need to apply nits in more than one place after the first review.
- Are you happy for me to just add commits that address my nits? That might speed up the review cycle.
| "source": [ | ||
| "import util\n", | ||
| "\n", | ||
| "from test_framework.test_framework import BitcoinTestFramework\n", |
| "import util\n", | ||
| "\n", | ||
| "from test_framework.test_framework import BitcoinTestFramework\n", | ||
| "from test_framework.key import ECKey, ECPubKey, generate_musig_key, generate_schnorr_nonce\n", |
| "from test_framework.test_framework import BitcoinTestFramework\n", | ||
| "from test_framework.key import ECKey, ECPubKey, generate_musig_key, generate_schnorr_nonce\n", | ||
| "from test_framework.key import aggregate_schnorr_nonces, aggregate_musig_signatures\n", | ||
| "import binascii\n", |
| "from test_framework.key import ECKey, ECPubKey, generate_musig_key, generate_schnorr_nonce\n", | ||
| "from test_framework.key import aggregate_schnorr_nonces, aggregate_musig_signatures\n", | ||
| "import binascii\n", | ||
| "import hashlib" |
There was a problem hiding this comment.
Move standard library imports above local library imports.
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "# 1.2 Introduction to n-of-n Musig\n", |
There was a problem hiding this comment.
style nit: use MuSig, not Musig
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "### 3.1 Programming exercise: Compute aggregated musig signature.\n", |
There was a problem hiding this comment.
It would be good if you could show why the aggregate signature is valid for the aggregated pubkey
| "for pk, c in c_map.items():\n", | ||
| " print(\"Compressed Pubkey: \", pk.get_bytes().hex())\n", | ||
| " print(\"Challenge: \", c.hex(), '\\n')\n", | ||
| " \n", |
There was a problem hiding this comment.
It'd be good to print the aggregated public key here.
| "msg = hashlib.sha256(b'transaction').digest()\n", | ||
| "\n", | ||
| "# Generate individual signatures.\n", | ||
| "sig0 = sec0_c.sign_musig(k0, negated, R_agg, pk_musig, msg)\n", |
There was a problem hiding this comment.
I'd suggest removing the negated argument here. Just negate the k values in the previous cell.
| "outputs": [], | ||
| "source": [ | ||
| "# Compute key pairs.\n", | ||
| "sec0 = ECKey()\n", |
There was a problem hiding this comment.
I think the name privkey0, etc would be a bit clearer than sec0.
| "sig2 = sec2_c.sign_musig(k2, negated, R_agg, pk_musig, msg)\n", | ||
| "\n", | ||
| "# Aggregate signatures.\n", | ||
| "sig_agg = aggregate_musig_signatures([sig0, sig1, sig2])\n", |
There was a problem hiding this comment.
It'd be nice if you could make it much more explicit that the signature generated using the MuSig validates correctly.
|
Feedback has been incorporated. Imports (updated) methods from separate musig module. Uses bitcoin (taproot) branch from here. |
Incorporated feedback and separate musig module methods.
|
rebased on master |
|
added a commit with lots of nitty changes |
Previous chapter feedback which has been incorporated in this PR.