Rename tapscript descriptors by jachiang · Pull Request #138 · bitcoinops/taproot-workshop

jachiang · 2019-10-30T09:24:44Z

As discussed in #123

Tapscripts updated to use hash160 as a hashlock
Tapscripts updated to have the following descriptors:

pk
pk_delay
pk_hashlock
pk_hashlock_delay
csa
csa_delay
csa_hashlock
csa_hashlock_delay

Constructor methods have been updated accordingly...

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

jnewbery · 2019-10-31T17:50:21Z

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

Using good, consistent commit logs (see the link I sent you here: Added TestShell class for interactive Python environments. bitcoin/bitcoin#17288 (review) for pointers)
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)
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 · 2019-10-31T18:20:16Z

+    "Construct a `csa_hashlock_delay` tapscript with the following locking conditions:\n",
    "\n",
    "* 2-of-2 public keys\n",
    "* Hashlock with the preimage `sha256(b'secret')`\n",


This is strange. You change the comment in the first commit and then revert it in a later commit.

Do you use the preimage sha256(b'secret') to enforce that the preimage provided in the witness is 32 bytes? If so, is that explained somewhere?

Do you use the preimage sha256(b'secret') to enforce that the preimage provided in the witness is 32 bytes? If so, is that explained somewhere?

Yes there's a 32B size guard in the output script - I will add explainer that the hashlock does this to protect against unfeasibly large pre-images at spending time (e.g. atomic swap between two chains with different data push size limits, where correlating pre-image size is only valid on one-chain).

jnewbery · 2019-10-31T18:21:29Z

    "\n",
    "# Method: 32B preimage - sha256(bytes)\n",
-    "# Method: 20B digest - hashlib.new('ripemd160', bytes).digest()\n",
+    "# Method: 20B digest - hashlib.new('ripemd160', sha256(bytes)).digest()\n",


Just use hash160 from script.py and then remove the direct import of hashlib.

jnewbery · 2019-10-31T18:33:17Z

Can you restructure this PR as follows:

squash the commits Changed tapscript hashlock: ripemd160 -> hash160. and Updated hash160 hashlock in chapter 2.3. (with the review comments addressed)
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 · 2019-10-31T19:41:12Z

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 · 2019-11-01T13:44:37Z

@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:

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 · 2019-11-01T14:01:26Z

Rebased after merging of #140.

jnewbery

Tested ACK 412196b

Three nits inline

jnewbery · 2019-11-01T13:57:46Z

+    "\n",
+    "**A hashlock consumes a 32B preimage and checks the hash digest for correctness:**\n",
+    "\n",
+    "1. A 20B size guard checks that the spending preimage size is exactly 20B.\n",


These should be 32B, not 20B

jnewbery · 2019-11-01T14:00:34Z

-    "* `ts(csahasholder(k, [key0, key1, ...], hash, time))`\n",
-    "    * Witness: `[signature], [signature], ..., [32B pre-image], [delay]`"
+    "* `ts(csa_hashlock_delay(k, [key0, key1, ...], hash, time))`\n",
+    "    * Witness: `[signature], [signature], ..., [32B pre-image], [delay]`\n",


I don't think the witness includes [delay]

Fixed in #141.

jnewbery · 2019-11-01T14:06:09Z

    "from test_framework.messages import CTxInWitness, ser_string, sha256\n",
    "from test_framework.musig import generate_musig_key\n",
-    "from test_framework.script import TapLeaf, TapTree, TaprootSignatureHash, SIGHASH_ALL_TAPROOT"
+    "from test_framework.script import TapLeaf, TapTree, TaprootSignatureHash, SIGHASH_ALL_TAPROOT, hash160"


nit: sort import names alphabetically

jnewbery · 2019-11-01T14:08:28Z

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 · 2019-11-01T14:42:10Z

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

Changed hashlock to hash160 function. Also added usage of convenience method hash160 to chapter 2.3.

Ran the following shell script to subsitute old/new descriptors in repository. Order of sed invocations matter. sed -i -e 's/pkhasholder/pk_hashlock_delay/g' $(git grep -l pkhasholder) sed -i -e 's/pkhash/pk_hashlock/g' $(git grep -l pkhash) sed -i -e 's/pkolder/pk_delay/g' $(git grep -l pkolder) sed -i -e 's/csahasholder/csa_hashlock_delay/g' $(git grep -l \ csahasholder) sed -i -e 's/csahash/csa_hashlock/g' $(git grep -l csahash) sed -i -e 's/csaolder/csa_delay/g' $(git grep -l csaolder)

jachiang · 2019-11-01T16:10:35Z

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

Modified code to chain tapscript constructor to initializer in chapters 2.3, 2.4 and 3.1.

jachiang · 2019-11-01T16:19:01Z

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

jnewbery · 2019-11-01T16:42:08Z

Tested ACK ced902d

Great stuff. Thanks James!

jachiang requested a review from jnewbery October 30, 2019 09:24

jnewbery reviewed Oct 31, 2019

View reviewed changes

jachiang force-pushed the 2019-10-ts-desc-update branch from 4a5d1f7 to 412196b Compare November 1, 2019 13:37

jachiang force-pushed the 2019-10-ts-desc-update branch from 412196b to ae0b2eb Compare November 1, 2019 14:00

jnewbery reviewed Nov 1, 2019

View reviewed changes

jnewbery changed the title ~~Updated tapscript descriptors.~~ Rename tapscript descriptors Nov 1, 2019

jachiang added 3 commits November 1, 2019 16:28

Update tapscript hashlock: ripemd160 -> hash160

a476429

Changed hashlock to hash160 function. Also added usage of convenience method hash160 to chapter 2.3.

Add hashlock and delay explainer to chapter 2.3

b634e25

jachiang force-pushed the 2019-10-ts-desc-update branch from ae0b2eb to 92c783f Compare November 1, 2019 15:39

jachiang closed this Nov 1, 2019

jachiang reopened this Nov 1, 2019

Chain tapscript constructor to initializer

ced902d

Modified code to chain tapscript constructor to initializer in chapters 2.3, 2.4 and 3.1.

jachiang force-pushed the 2019-10-ts-desc-update branch from ba1399f to ced902d Compare November 1, 2019 16:17

jnewbery merged commit fb2f231 into bitcoinops:master Nov 1, 2019

jnewbery mentioned this pull request Nov 1, 2019

Changes to taproot descriptors #123

Closed

Conversation

jachiang commented Oct 30, 2019

Uh oh!

jnewbery commented Oct 31, 2019

Uh oh!

jnewbery Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

jachiang Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 31, 2019

Uh oh!

jachiang commented Oct 31, 2019

Uh oh!

jachiang commented Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jachiang commented Nov 1, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Nov 1, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery Nov 1, 2019

Choose a reason for hiding this comment

Uh oh!

jachiang Nov 1, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery Nov 1, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Nov 1, 2019

Uh oh!

jnewbery commented Nov 1, 2019

Uh oh!

jachiang commented Nov 1, 2019

Uh oh!

jachiang commented Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Nov 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jachiang commented Nov 1, 2019 •

edited

Loading

jachiang commented Nov 1, 2019 •

edited

Loading