Rename tapscript descriptors

[jachiang]

As discussed in #123

1) Tapscripts updated to use hash160 as a hashlock

2) Tapscripts updated to have the following descriptors:

• pk
• pk_delay
• pk_hashlock
• pk_hashlock_delay
• csa
• csa_delay
• csa_hashlock
• csa_hashlock_delay

3) Constructor methods have been updated accordingly...

• e.g. TapLeaf().construct_csa_hashlock_delay(keys, hash, delay)

[jnewbery]

Thanks for this, @jachiang ! The changes look mostly good, but you can help reviewers a lot by:

1. Using good, consistent commit logs (see the link I sent you here: https://github.com/bitcoin/bitcoin/pull/17288#pullrequestreview-308807117 for pointers)
2. don't break things between commits. For example, in this PR, you update the test_framework library in one commit and then update the notebooks in a subsequent commit. Between those commits, the notebooks wouldn't work. (This is especially important when a repo has automated tests because it would break git bisect)
3. don't mix up random clean-ups (eg spelling corrections, whitespace removal) in the same commit as functional changes.

Those things are slightly less important in this repo because there are only a couple of us working on it, we don't have automated tests, but it's best to get into good habits for when you're contributing to Bitcoin Core or other projects. It's also just good practice to make things as easy as possible for your reviewers.

[jnewbery]

Can you restructure this PR as follows:

1. squash the commits Changed tapscript hashlock: ripemd160 -> hash160. and Updated hash160 hashlock in chapter 2.3. (with the review comments addressed)
2. Have one commit for renaming the descriptors. I think you can do this quite easily with a script:

sed -i -E -e 's/pkhasholder\b/pk_hashlock_delay/g' $(git grep -l pkhasholder)
sed -i -E -e 's/pkhash\b/pk_hashlock/g' $(git grep -l pkhash)

etc. Run the script and then paste it into the commit log so reviewers can verify the script.

3. Split the spelling/whitespace corrections into their own commit. This could potentially go into its own PR since it's not really related to the other changes in this PR.
4. Split the tapleaf constructor methods into their own commit. Again, this could go into its own PR.

[jachiang]

Those things are slightly less important in this repo because there are only a couple of us working on it, we don't have automated tests, but it's best to get into good habits for when you're contributing to Bitcoin Core or other projects. It's also just good practice to make things as easy as possible for your reviewers.

@jnewbery Many thanks for the tips, much appreciated and will happily apply them here and in future PR's!

[jachiang]

@jnewbery Thanks for the review. I have applied most of the suggested fixes. Typos are split into #140 and this PR now builds on top of that. It was easier to fix the typos first, since they affect the subsitution of tapscript descriptors.

Remaining TODO:

4. Split the tapleaf constructor methods into their own commit. Again, this could go into its own PR.

Do you mean dedicating a PR to the renaming of the constructors to match the descriptor update? The tapscript-specific constructor methods should be renamed together with the descriptors, or I am missing something?

[jachiang]

Rebased after merging of #140.

[jnewbery]

Split the tapleaf constructor methods into their own commit. Again, this could go into its own PR.

Do you mean dedicating a PR to the renaming of the constructors to match the descriptor update? The tapscript-specific constructor methods should be renamed together with the descriptors, or I am missing something?

Sorry. I wasn't clear. Your original branch had changes like this, which constructed the tapleaf objects on a single line:

-    "tapscript_2a = TapLeaf()\n",
-    "tapscript_2b = TapLeaf()\n",
-    "tapscript_2c = TapLeaf()\n",
-    "tapscript_2d = TapLeaf()\n",
-    "tapscript_2e = TapLeaf()\n",
-    "tapscript_2f = TapLeaf()\n",
     "delay = 3*24*6\n",
-    "tapscript_2a.construct_csaolder(3, [main_pubkeyA, main_pubkeyB, backup_pubkeyD], delay)\n",
-    "tapscript_2b.construct_csaolder(3, [main_pubkeyA, main_pubkeyC, backup_pubkeyD], delay)\n",
-    "tapscript_2c.construct_csaolder(3, [main_pubkeyB, main_pubkeyC, backup_pubkeyD], delay)\n",
-    "tapscript_2d.construct_csaolder(3, [main_pubkeyA, main_pubkeyB, backup_pubkeyE], delay)\n",
-    "tapscript_2e.construct_csaolder(3, [main_pubkeyA, main_pubkeyC, backup_pubkeyE], delay)\n",
-    "tapscript_2f.construct_csaolder(3, [main_pubkeyB, main_pubkeyC, backup_pubkeyE], delay)\n",
+    "tapscript_2a = TapLeaf().construct_csa_delay(3, [main_pubkeyA, main_pubkeyB, backup_pubkeyD], delay)\n",
+    "tapscript_2b = TapLeaf().construct_csa_delay(3, [main_pubkeyA, main_pubkeyC, backup_pubkeyD], delay)\n",
+    "tapscript_2c = TapLeaf().construct_csa_delay(3, [main_pubkeyB, main_pubkeyC, backup_pubkeyD], delay)\n",
+    "tapscript_2d = TapLeaf().construct_csa_delay(3, [main_pubkeyA, main_pubkeyB, backup_pubkeyE], delay)\n",
+    "tapscript_2e = TapLeaf().construct_csa_delay(3, [main_pubkeyA, main_pubkeyC, backup_pubkeyE], delay)\n",

which I was trying to say should be in their own commit or PR.

[jnewbery]

sorry. Needs a rebase (at least the scripted name change should be easy!)

[jachiang]

Oops. Didn't mean to close this. Spotty Internet.

[jachiang]

Ok, I have addressed nits, added back the init/constructor chaining as separate commit and rebased on master :)

[jnewbery]

Tested ACK ced902d5856661cdf6fa9ddfb34d05f1e86fe4f5

Great stuff. Thanks James!