Chapter 2.2 update: Added commitment unsealing to 2.2.3/2.24 & Mini case-study #132

jachiang · 2019-10-28T13:18:17Z

This update adds the following updates to taptweak chapter 2.2 :

Contract commitment unsealing to examples 2.2.3 and 2.2.4 (Partially addresses Updates to chapter 2.2 #93)
Updates chapter image ("pay-to-contract")
Mini case-study: OP_RETURN vs. pay-to-contract (Partially address Add mini case-studies to the 2.x chapters #86)

jachiang · 2019-10-28T15:16:09Z

Builds on #131

jnewbery · 2019-10-28T18:45:05Z

I've had a quick skim and this looks good. I'll do a detailed review soon. Thanks!

jachiang · 2019-10-28T19:06:27Z

I've had a quick skim and this looks good. I'll do a detailed review soon. Thanks!

Cheers @jnewbery

bitschmidty

Thanks for taking the lead on this, @jachiang

I was able to successfully complete the notebook using the solutions.

2.2-taptweak.ipynb

bitschmidty · 2019-10-29T17:59:58Z

2.2-taptweak.ipynb

+    "\n",
+    "First, we commit a contract between Alice and Bob and then demonstrate how this unsafe commitment can be changed.\n",
+    "\n",
+    "* The initial committed contract is: `Alice pays 10 BTC to Bob`"


I think it is confusing to have the message sound like a BTC transaction

How about "Alice agrees to pay 10 BTC to Bob"?

I think that’s better, @jachiang

2.2-taptweak.ipynb

bitschmidty · 2019-10-29T18:21:54Z

2.2-taptweak.ipynb

+    "coinbase_address = node.getnewaddress(address_type=\"bech32\")\n",
+    "node.generatetoaddress(101, coinbase_address)\n",
+    "balance = node.getbalance()\n",
+    "assert balance > 1\n",


Perhaps generate_and_send_coins() here. Also not sure we need the balance var or assert statement

Ok, I will open a separate PR to modify generate_and_send_coins() to accept multiple outputs and both addresses and data (op_return).

Ok, this is now tracked here #143.

2.2-taptweak.ipynb

jachiang · 2019-10-31T16:00:26Z

@bitschmidty - Many thanks for your review! I will apply your fixes and suggestions as soon as I can today.

jachiang · 2019-11-02T00:13:01Z

@bitschmidty Many thanks for your review. All your nits have been addressed.

This PR now builds on the following:

Add default stack value for CTxInWitness initializer. #142: Fix - Default stack value for CTxInWitness() initializer
Make generate_and_send_coins send rewards to bech32 address. #131: Bech32 address usage in generate_and_send_coins()
Optional op_return output for generate_and_send_coins #143: OP_RETURN support for generate_and_send_coins()

2.2-taptweak.ipynb

bitschmidty · 2019-11-06T17:21:04Z

@jachiang Tested again based on the latest. I resolved the conversion for my items. Thanks for addressing those. Left a couple more comments and also left the unresolved items as well.

jachiang · 2019-11-06T19:07:04Z

@jachiang Tested again based on the latest. I resolved the conversion for my items. Thanks for addressing those. Left a couple more comments and also left the unresolved items as well.

Thanks @bitschmidty for your review. Sorry, what are you referring to as unresolved items?

bitschmidty · 2019-11-06T19:23:19Z

Chapter 2.2 update: Added commitment unsealing to 2.2.3/2.24 & Mini case-study #132 (comment)
Chapter 2.2 update: Added commitment unsealing to 2.2.3/2.24 & Mini case-study #132 (comment)

jachiang · 2019-11-06T19:43:00Z

* [#132 (comment)](https://github.com/bitcoinops/taproot-workshop/pull/132#discussion_r341773228)

* [#132 (comment)](https://github.com/bitcoinops/taproot-workshop/pull/132#discussion_r341222960)

I have changed the contract string according to comments above. Now: "Alice agrees to pay 10 BTC to Bob".
Enabling generate_and_send_coins(..,data) is pending Optional op_return output for generate_and_send_coins #143.

jnewbery

Really good stuff @jachiang . Thanks for doing this, and apologies that it's taken me a while to get to it.

I've rebased on master after merging a couple of the earlier PRs, and also added a commit with some suggested nits. Take a look and let me know what you think.

I really like the new stuff in part 1 about pay-to-contract and verifying the commitments. It's a great practical example of a broken commitment scheme and making it secure.

For the case study, I think it'd be more effective if the student built the OP_RETURN and pay-to-contract transactions themselves. Adding a commitment in the transaction is the point of the exercise, so we should bring that to the center of the student's attention. We should move functionality into the background in the framework convenience functions when it's something we don't want the student to pay attention to.

jnewbery · 2019-11-08T22:32:50Z

util.py

@@ -149,7 +149,7 @@ def __getattr__(self, name):
    def __setattr__(self, name):
        return setattr(self.instance, name)

-def generate_and_send_coins(node, address):
+def generate_and_send_coins(node, address, data=None):


I'd prefer to construct the OP_RETURN transaction in the notebook itself rather than have it hidden in the util file. Having the transaction constructed in the notebook is more illustrative of the differences between using OP_RETURN and committing to a value in a pubkey.

jnewbery · 2019-11-08T22:35:55Z

2.2-taptweak.ipynb

+    "print(\"Transaction weight with OP_RETURN: {}\\n\".format(node.decoderawtransaction(op_return_tx_hex)['weight']))"
+   ]
+  },
+  {


I suggest removing this exercise Verify that the contract between Alice and Bob is committed correctly and the final example Construct a CTransaction to spend the pay-to-contract public key. Both of those things have been done before, so I don't think it provides much extra value to the student to go through them again. I think this could just be replaced with a text cell saying that the commitment can be verified like we did in part 1, and that the output to a tweaked pubkey is spendable, like in part 2.

jnewbery · 2019-11-26T18:26:15Z

@jachiang - are you still working on this? What are your thoughts about moving the OP_RETURN construction into the notebook so it's more visible to the student?

jachiang · 2019-11-27T07:53:53Z

@jnewbery - apologies for the delay. I agree with constructing OP_RETURN out in the open (it's a simple script too), and will prioritize this for an update this week.

jachiang · 2019-11-28T22:50:56Z

@jnewbery - I have included the OP_RETURN construction in part 3) with createrawtransaction. Alternatively, it could be constructed with CTransaction, by setting vouts individually, resulting in more (boilerplate) code. What do you think is better?

Edit: I have added commit "Rewrite 2.2.11 and 2.2.12 with CTransaction" so the reviewer can compare the alternative OP_RETURN construction with CTransaction.

Other updates: Commit review fixups broke example 2.2.5. at "Alice tweaks her key pair". This has now been fixed.

bitschmidty · 2019-12-05T20:45:25Z

@jachiang let me know if/when youd like me to test through the applicable notebooks on this PR

jnewbery · 2020-01-03T22:22:18Z

Rebased on master and added a commit with a few small changes.

@bitschmidty - Chapter 2.2 should be ready for final review.

bitschmidty

tACK 407792b

jachiang requested review from jnewbery and bitschmidty October 28, 2019 13:18

bitschmidty reviewed Oct 29, 2019

View reviewed changes

jachiang mentioned this pull request Nov 1, 2019

Optional op_return output for generate_and_send_coins #143

Open

jachiang force-pushed the 2019-10-2.2-update-minicasestudy branch from b3cade7 to 0ca2e9e Compare November 2, 2019 00:09

bitschmidty reviewed Nov 6, 2019

View reviewed changes

2.2-taptweak.ipynb Outdated Show resolved Hide resolved

bitschmidty reviewed Nov 6, 2019

View reviewed changes

2.2-taptweak.ipynb Outdated Show resolved Hide resolved

jachiang force-pushed the 2019-10-2.2-update-minicasestudy branch from 0ca2e9e to b259f1d Compare November 6, 2019 19:05

jachiang force-pushed the 2019-10-2.2-update-minicasestudy branch from b259f1d to f538aa8 Compare November 6, 2019 19:39

jnewbery force-pushed the 2019-10-2.2-update-minicasestudy branch from f538aa8 to 1b9dc08 Compare November 8, 2019 22:24

jnewbery reviewed Nov 8, 2019

View reviewed changes

jachiang force-pushed the 2019-10-2.2-update-minicasestudy branch from 1b9dc08 to 835225d Compare November 28, 2019 22:36

jachiang force-pushed the 2019-10-2.2-update-minicasestudy branch from 835225d to 008ba57 Compare November 28, 2019 22:57

jachiang and others added 4 commits January 3, 2020 16:18

Add contract commitment unsealing to 2.2.3, 2.2.4

33af341

Update taptweak image with pay-to-contract

a39c2cc

Add mini-case study: OP_RETURN vs pay-to-contract

984d796

review fixups

3299c98

jachiang and others added 2 commits January 3, 2020 16:20

Rewrite 2.2.11 and 2.2.12 with CTransaction

9010e18

Review markups for chapter 2.2

407792b

jnewbery force-pushed the 2019-10-2.2-update-minicasestudy branch from 8424485 to 407792b Compare January 3, 2020 22:21

bitschmidty reviewed Jan 6, 2020

View reviewed changes

jnewbery merged commit 22066f5 into bitcoinops:master Jan 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chapter 2.2 update: Added commitment unsealing to 2.2.3/2.24 & Mini case-study #132

Chapter 2.2 update: Added commitment unsealing to 2.2.3/2.24 & Mini case-study #132

jachiang commented Oct 28, 2019

jachiang commented Oct 28, 2019

jnewbery commented Oct 28, 2019

jachiang commented Oct 28, 2019

bitschmidty left a comment

bitschmidty Oct 29, 2019

jachiang Oct 31, 2019

bitschmidty Nov 6, 2019

bitschmidty Oct 29, 2019

jachiang Oct 31, 2019

jachiang Nov 1, 2019

jachiang commented Oct 31, 2019

jachiang commented Nov 2, 2019

bitschmidty commented Nov 6, 2019

jachiang commented Nov 6, 2019

bitschmidty commented Nov 6, 2019

jachiang commented Nov 6, 2019

jnewbery left a comment

jnewbery Nov 8, 2019

jnewbery Nov 8, 2019

jnewbery commented Nov 26, 2019

jachiang commented Nov 27, 2019 •

edited

Loading

jachiang commented Nov 28, 2019 •

edited

Loading

bitschmidty commented Dec 5, 2019

jnewbery commented Jan 3, 2020

bitschmidty left a comment

Chapter 2.2 update: Added commitment unsealing to 2.2.3/2.24 & Mini case-study #132

Chapter 2.2 update: Added commitment unsealing to 2.2.3/2.24 & Mini case-study #132

Conversation

jachiang commented Oct 28, 2019

jachiang commented Oct 28, 2019

jnewbery commented Oct 28, 2019

jachiang commented Oct 28, 2019

bitschmidty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jachiang commented Oct 31, 2019

jachiang commented Nov 2, 2019

bitschmidty commented Nov 6, 2019

jachiang commented Nov 6, 2019

bitschmidty commented Nov 6, 2019

jachiang commented Nov 6, 2019

jnewbery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Nov 26, 2019

jachiang commented Nov 27, 2019 • edited Loading

jachiang commented Nov 28, 2019 • edited Loading

bitschmidty commented Dec 5, 2019

jnewbery commented Jan 3, 2020

bitschmidty left a comment

Choose a reason for hiding this comment

jachiang commented Nov 27, 2019 •

edited

Loading

jachiang commented Nov 28, 2019 •

edited

Loading