Schnorr & Taproot extension of Bitcoin Core TestFramework library

[jachiang]

This is the Schnorr & Taproot library extension of the Bitcoin Core Python TestFramework I have been working on during my residency. Although the TestFramework is intended to provide functional test coverage for the Bitcoin Core project, it can also serve as a convenient Schnorr/Taproot prototyping tool for developers.

The goal of this project was to provide a toolkit for users to quickly prototype new Bitcoin contracting proposals with Schnorr & Taproot, so that the community can quickly get familiar and provide constructive feedback.

Schnorr extensions

• Musig signature aggregation
• Adaptor Signature generation (Removed from this PR, out of scope for Optech)
• Discrete Log Contract key generation (Removed, out of scope for Optech)

Taproot output classes (Segwit V1)

• Taptree & constructor methods (huffman construtor with weighted tapleafs)
• Tapscript descriptors (see below)

Minscript classes (can be composed to (correctly) express tapscript descriptors)
For example:

• pkolder(key, time) = and_v(v:pk(key),older(time))
• csahash(n, keys, hash) = and_v(v:thresh_csa,ripemd160)
• csahasholder(n,keys,hash,time) = and_v(v:thresh_csa ,and_v(v:ripemd160(hash), older(time)))

Utility Classes/Methods

• TestWrapper class wraps functionality of BitcoinTestFramework for external usage.
• Refactor BitcoinTestFramework to encapsulate code in main into setup and shutdown, in order to prevent current code duplication in TestWrapper. I will also create a PR in master for this once completed.

Future work

• Complete miniscript language support.
• Policy to Miniscript compiler for Taproot outputs (Policy language can provide flexible composition of spending conditions for taproot outputs. The compiler will separate policy sub-expressions in different tapleafs for user-defined privacy.)

Your feedback and review is greatly appreciated.

I am currently documenting the library in form of interactive jupyter notebook files in bitcoinops/taproot.

[jnewbery]

Hi James. Thanks for removing the adaptor signature/DLC stuff. There are still a couple of things more to do before this is ready for review:

• squash the 'fixup' commits. (eg remove the Removed Whitespaces. Edited var name & comments. and Min nVersion corrected to 1 in tests. commits)
• make the framework changes in the same (or consecutive) commits to the tests in which they're first used.

Could you also open a PR just for the Add TestFramework reset required by TestWrapper. and Reorganization of BitcoinTestFramework main method. commits please (since we'd want to get those merged into bitcoin/bitcoin).

[jachiang]

• squash the 'fixup' commits. (eg remove the Removed Whitespaces. Edited var name & comments. and Min nVersion corrected to 1 in tests. commits)
• make the framework changes in the same (or consecutive) commits to the tests in which they're first used.

Could you also open a PR just for the Add TestFramework reset required by TestWrapper. and Reorganization of BitcoinTestFramework main method. commits please (since we'd want to get those merged into bitcoin/bitcoin).

Hi @jnewbery - Many thanks for the preliminary review. I have addressed your comments as suggested.

[jnewbery]

I've force pushed a change to fix some general best practices:

• wrapped commit logs at 120 chars
• removed unused python imports and sorted imports according to PEP 8.

original branch is here: https://github.com/jnewbery/bitcoin/tree/2019-09-optech-taproot-master-original

[jnewbery]

force-pushed some more updates:

• changed file permissions so test files are runnable (chmod 755)
• renamed test files to be feature_ instead of framework_
• added boilerplate licence text and doc strings
• fixed all flake8 warnings

[jnewbery]

I've reviewed all the changes in key.py. I haven't reviewed script.py or the individual test scripts.

[jnewbery]

nit: I know this already occurs throughout this module, but in Python, assert is a statement, not a function, so this should be assert self.valid. No need to change it here, but this would be better without the parens.

[jnewbery]

Add a docstring commenting this function (ie that this multiplies a public key by a scalar).

[jachiang]

Done.

[jnewbery]

Add a docstring commenting this function (ie that this adds two pubkeys)

[jachiang]

Done.

[jnewbery]

Did you consider using the __add__ method so users can use the more natural notation:

a = ECPubKey()
b = ECPubKey()
c = a + b

than:

a = ECPubKey()
b = ECPubKey()
c = a.add(b)

(same comment for other add() and mul() functions in this commit)

[jachiang]

Done. I kept add/mul for key operations for scalar bytes (e.g. tweaking a key with hash digest), but these are just wrappers for +/- to minimise code duplication.

[jnewbery]

I don't think the variable name secret is appropriate here. Perhaps summand?

[jnewbery]

nit: consider using a map-reduce here:

from functools import reduce
R_agg = reduce(lambda x, y: SECP256K1.add(x, y), map(lambda x: x.p, Rv))

actually, since you've already defined a way to add ECPubKeys, you don't even need the map:

R_agg = reduce(lambda x, y: x.add(y), Rv).p

If you define __add__ for ECPubKey, then this just becomes:

R_agg = sum(Rv).p

edit: since you're returning a ECPubKey object, just set R_agg as a ECPubKey:

R_agg = reduce(lambda x, y: x.add(y), Rv)

[jachiang]

Very nice!

[jnewbery]

Again, I suggest using a reduce pattern here.

[jachiang]

Done.

[jnewbery]

This comment could be improved by documenting what is being returned.

[jnewbery]

len(s) is guaranteed to be 32 by construction. It would be better to assert that len(sig) == 64 at the top.

[jnewbery]

Use sum or reduce here (and for the r aggregation below)

[jachiang]

@jnewbery - Many thanks for your detailed review. I have incorporated all of your feedback in the latest push.

[jnewbery]

I've force-pushed a branch that squashes the latest changes into the appropriate commits, and also adds the tests to test_runner.py

[jnewbery]

I've pushed three commits containing nits for the add/mul commit, the schnorr commit and the musig commit.

@jachiang - can you review those commits and squash them into the preceding commits if you're happy with them?

I've also left a couple of small comments on musig.py

[jnewbery]

This is duplicating code from sign_schnorr in key.py. Can you just call through to that method to avoid the duplication?

[jnewbery]

Can you document what the arguments are in this docstring?

[jnewbery]

@jachiang - this code is still undergoing active development in the https://github.com/bitcoinops/taproot-workshop repo. I suggest we close this PR until that's settled down, and then perhaps you can re-open against https://github.com/bitcoin/bitcoin when there's a schnorr/taproot PR?

[jachiang]

@jachiang - this code is still undergoing active development in the https://github.com/bitcoinops/taproot-workshop repo. I suggest we close this PR until that's settled down, and then perhaps you can re-open against https://github.com/bitcoin/bitcoin when there's a schnorr/taproot PR?

Sounds good @jnewbery! Many thanks again for your extensive review on this PR.