Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #19953

sipa · 2020-09-14T04:53:20Z

This is an implementation of the Schnorr/taproot consensus rules proposed by BIPs 340, 341, and 342.

See the list of commits below. No signing or wallet support of any kind is included, as testing is done entirely through the Python test framework.

This is a successor to #17977 (see discussion following this comment), and will have further changes squashed/rebased. The history of this PR can be found in #19997.

sipa · 2020-09-14T05:15:52Z

Here is a categorized list of all the commits:

Refactors (sipa/bitcoin@`f8c099e...450d2b2`) [+50 -39]:

107b57d scripted-diff: put ECDSA in name of signature functions: In preparation for adding Schnorr versions of CheckSig, VerifySignature, and ComputeEntry, give them an ECDSA specific name.
8bd2b4e refactor: rename scriptPubKey in VerifyWitnessProgram to exec_script: The old name is confusing, as it doesn't store a scriptPubKey, but the actually executed script.
5d62e3a refactor: keep spent outputs in PrecomputedTransactionData: A BIP-341 signature message may commit to the scriptPubKeys and amounts of all spent outputs (including other ones than the input being signed for spends), so keep them available to signature hashing code.

BIP340/341/342 consensus rules (sipa/bitcoin@`450d2b2...865d2c3`) [+693 -50]:

9eb5908 Add TaggedHash function (BIP 340): This adds the TaggedHash function as defined by BIP340 to the hash module, which is used in BIP340 and BIP341 to produce domain-separated hashes.
5de246c Implement Taproot signature hashing (BIP 341): This implements the new sighashing scheme from BIP341, with all relevant whole-transaction values precomputed once and cached.
0664f5f Support for Schnorr signatures and integration in SignatureCheckers (BIP 340): This enables the schnorrsig module in libsecp256k1, adds the relevant types and functions to src/pubkey, as well as in higher-level SignatureChecker classes. The (verification side of the) BIP340 test vectors is also added.
8bbed4b Implement Taproot validation (BIP 341): This includes key path spending and script path spending, but not the Tapscript execution implementation (leaf 0xc0 remains unemcumbered in this commit).
330de89 Use ScriptExecutionData to pass through annex hash: Instead of recomputing the annex hash every time a signature is verified, compute it once and cache it in a new ScriptExecutionData structure.
72422ce Implement Tapscript script validation rules (BIP 342): This adds a new SigVersion::TAPSCRIPT, makes the necessary interpreter changes to make it implement BIP342, and uses them for leaf version 0xc0 in Taproot script path spends.

Regtest activation and policy (sipa/bitcoin@`865d2c3...206fb18`) [+98 -8]:

e9a021d Make Taproot spends standard + policy limits: This adds a TxoutType::WITNESS_V1_TAPROOT for P2TR outputs, and permits spending them in standardness rules. No corresponding CTxDestination is added for it, as that isn't needed until we want wallet integration. The taproot validation flags are also enabled for mempool transactions, and standardness rules are added (stack item size limit, no annexes).
d7ff237 Activate Taproot/Tapscript on regtest (BIP 341, BIP 342): Define a versionbits-based activation for the new consensus rules on regtest. No activation or activation mechanism is defined for testnet or mainnet.

Tests (sipa/bitcoin@`206fb18...0e2a5e4`) [+2148 -28]:

3c22663 tests: add BIP340 Schnorr signature support to test framework: Add a pure Python implementation of BIP340 signing and verification, tested against the BIP's test vectors.
f06e6d0 tests: functional tests for Schnorr/Taproot/Tapscript: A large functional test is added that automatically generates random transactions which exercise various aspects of the new rules, and verifies they are accepted into the mempool (when appropriate), and correctly accepted/rejected in (Python-constructed) blocks.
4567ba0 tests: add generic qa-asset-based script verification unit test: This adds a unit test that does generic script verification tests, with positive/negative witnesses/scriptsigs, under various flags. The test data is large (several MB) so it's stored in the qa-assets repo.
0e2a5e4 tests: dumping and minimizing of script assets data: This adds a --dumptests flag to the feature_taproot.py test, to dump all its generated test cases to files, in a format compatible with the script_assets_test unit test. A fuzzer for said format is added as well, whose primary purpose is coverage-based minimization of those dumps.

_{PREV="$(git rev-parse HEAD)"; git log --oneline upstream/master..HEAD | while read C L; do if [ "d${L:0:14}" == "d--- [TAPROOT] " ]; then if [ "d$PREV" != "" ]; then git diff --shortstat $C..$PREV | (read _ _ _ ADD _ DEL _; echo "### ${L:14:-4} (https://github.com/sipa/bitcoin/compare/$C...$PREV) [+$ADD -$DEL]:"); echo; fi; PREV=$C; PREVL=$L; else echo -n " * $C **${L}**: "; git show "$C" --format="%b" -s | awk '/^$/{exit} 1' | tr $'\n' ' '; echo; fi; done | tac}

jnewbery

Concept ACK :)

Perhaps the pure refactor commits can be split into their own PR to reduce the size of this?

configure.ac

sipa · 2020-09-14T08:34:39Z

@jnewbery There are only two of them. The variable name one is very small, and the ECDSA naming one isn't really a standalone useful change. I think that one could be changed into a scripted-diff though.

src/policy/policy.cpp

instagibbs · 2020-09-14T14:14:04Z

concept ACK, just confirming for now this PR is identical to the old PR #17977 at 111be54

sipa · 2020-09-14T18:42:30Z

Reordered commits a bit, replaced the ECDSA naming one with a scripted diff, and organized the commits in sections. The end state is identical to what it was before.

benthecarman · 2020-09-14T20:37:13Z

Could the first 2 commits be done as separate PRs? That should slightly reduce the size of this one

sipa · 2020-09-14T20:39:21Z

@benthecarman That was suggested earlier by @jnewbery. I think splitting off 28 trivial lines of refactors from a 2500-line PR (though 1700 are tests) isn't going to change much. The refactors that were nontrivial and useful as standalone improvements have been split off already and merged (see history of the previous PR).

sipa · 2020-09-16T04:08:07Z

I added a unit test too now, with test vectors that were extracted from 20000 runs of the feature_taproot.py test (with the largest tests removed, and larger groups of inputs per transaction), minimized using libfuzzer's coverage tracking to 105.

The code for doing so is in https://github.com/sipa/bitcoin/commits/taproot-test-creation. I'll clean that code up a bit and integrate some parts of it in the normal Python test, so it's easier to recreate these vectors if improvements to the Python test are made.

jnewbery · 2020-09-16T08:52:07Z

I was surprised to learn that this was a 2500-line PR. By directory:

/test/functional - 1790 lines
/src/test - 134 lines
/src (exc test) - 817 lines

So the majority of code in this PR is tests (which is a good thing!)

I agree with sipa that it's not necessary to split off the first two commits (especially now that they're separated to the top of the branch). Equally no harm in doing so if that'd reduce the workload.

instagibbs · 2020-09-16T11:35:39Z

No code changes aside from unit test commit 9673fd9

DrahtBot · 2020-09-19T13:34:34Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

configure: Allow users to explicitly enable libsecp256k1's GMP bignum support #20121 (configure: Allow users to explicitly enable libsecp256k1's GMP bignum support by luke-jr)
Script: split policy/error consensus codes for CLEANSTACK, MINIMALIF #20100 (Script: split policy/error consensus codes for CLEANSTACK, MINIMALIF by sanket1729)
signet: Fix uninitialized read in validation #20035 (signet: Fix uninitialized read in validation by MarcoFalke)
Move SaltedHashers to separate file and add some new ones #19935 (Move SaltedHashers to separate file and add some new ones by achow101)
rpc: Add dumpcoinstats #19792 (rpc: Add dumpcoinstats by fjahr)
wallet: Migrate legacy wallets to descriptor wallets #19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)
Coinstats Index #19521 (Coinstats Index (without UTXO set hash) by fjahr)
Introduce deploymentstatus #19438 (Introduce deploymentstatus by ajtowns)
tests: Update more tests to work with descriptor wallets #18788 (tests: Update more tests to work with descriptor wallets by achow101)
External signer support - Wallet Box edition #16546 (External signer support - Wallet Box edition by Sjors)
[tests] Reduced number of validations in tx_validationcache_tests #13533 ([tests] Reduced number of validations in tx_validationcache_tests by lucash-dev)
Make script interpreter independent from storage type CScript #13062 (Make script interpreter independent from storage type CScript by sipa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/pubkey.h

src/script/script.cpp

src/script/script_error.cpp

jaybny · 2020-09-22T11:02:40Z

My expectation was to do a code review for taproot, but unfortunately, I cannot ACK the concept or the approach of this PR at this time.

After initial research and deep dive, I ask the following:

Will a majority of bitcoin users benefit from this PR?
for consensus changes this answer should be a 'YES'
Does this PR create new security concerns or attack vectors for the typical bitcoin user?
for consensus changes this answer should be a 'NO'

Below are my concerns with questions regarding this PR, answers and clarifications will hopefully bring me around.

Background

Original motivation to review this PR came from a tweet with a bounty. I am honored to help bitcoin, and am reviewing this PR independant of monetary expectations.

Following documents have been read and reviewed in detail specifically for this review, skipping some code and proofs.

Reason for not a simple "concept NACK" is because of the known information gap on my end. Specifically, I have not read most of bitcoin bips since early segwit.

My knowledge and use of bitcoin has been almost exclusively with P2PKH, including custom coded raw transactions on the utxo level.
Have not yet personally implemented BIP32 - Hierarchical Deterministic Wallets, nor SegWit

Questions

some of these are leading some of these are clarifying

Are my current ECDSA private/public key pairs compatible with Schnorr?
Where are the specific steps of transforming keys to Schnorr public/private key pairs?
is there any difference in key management between ECDSA pairs and Schnorr pairs?
is Schnorr compatible with BIP32?
If we define complexity risk as a measure of required knowledge in relation to security expectations, does this PR add complexity risk?
Can we measure the complexity risk delta of this PR?
Would an expert ECDSA pk pair security manager need more knowledge to become an expert Schnorr pk pair manager?
Would a future expert Schnorr pk pair manger already be an expert ECDSA pk pair manager?
If we define a hard-knowledge-fork as needing more knowledge and a soft-knowledge-fork as needing less knowledge to use bitcoin, is this Taproot PR a hard-knowledge-fork or soft-knowledge-fork?
Can Taproot be implemented with ECDSA? Technical: Pay-to-contract and Sign-to-contract?
Is Taproot available for ECDSA in this PR?

Concerns

MultiSig
I’m concerned that we are explicitly promoting MultiSig as a net positive for securing users private keys, and as far as i can tell, the consensus is that multisig comes down to a single leader in practice.

poll: Is the use of multisig on a bitcoin transaction, a security feature or a false sense of security?

Key Reuse
I’m concerned that we are implicitly assuming that key reuse is wrong, but as far as i can tell, the consensus is that code reuse is fine for most cases.

Highest concern is if the efficiency improvements, like removal of double sha256 hashing and others, are assuming no key reuse, creating a new vector for user error, since a majority of bitcoin users reuse their keys.

similar sentiments:

"While the key holders are expected not to reuse key pair, little could be done to stop payers to reuse an address. Unfortunately, key-pair reuse has been a social and technical norm since the creation of Bitcoin (the first tx made in block 170 reused the previous public key). I don’t see any hope to change this norm any time soon, if possible at all." bitcoin-dev

Overall, we are left with concerns both about the merit of doing Taproot
versus alternatives, as well as the process through which we got to be here.

Is Taproot actually more private than bare MAST and Schnorr separately? What
are the actual anonymity set benefits compared to doing the separately?

Yes, presuming single-pubkey-single-signature remains a common authorisation pattern.
bitcoin-dev

and fud:

Similarly, the SIGHASH_SINGLE bug for example would have been disastrous for this scheme. In general, the Blockchain this is used in must not allow spending more than one output with a single
signature.

red flags:

Homomorphic Payment Addresses and the Pay-to-Contract Protocol

Customer public key “a" in the tx cannot be used twice for same Public-Key of merchant

signaling variation? address(a,c) vs address(P,c) -

(a,c) becomes a private key

other:
DISCOURAGE_FLAG - why the need for this flag? can’t we say - ON_POLICY_CHANGE(Transform the mempool)?

flagged for followup:
BIP340

This complicates the implementation of secure signing protocols in scenarios in which a group of mutually distrusting signers work together to produce a single joint signature

Using the first option would be slightly more efficient for verification (around 10%), but we prioritize compactness, and therefore choose option 3.
batch verification

breaking an X only key - at most a small constant faster

nonce - leaking secret

every public key - has two corresponding secret keys

not using the same private key - across different signing schemes - signatures can leak secret key through nonce reuse

open up new nonce attack vectors

However, if this optimization is used and additionally the signature verification at the end of the signing algorithm is dropped for increased efficiency, signers must ensure the public key is correctly calculated and not taken from untrusted sources.

While recent academic papers claim that they are also possible with ECDSA, consensus support for Schnorr signature verification would significantly simplify the constructions.”

BIP341

Combining all these ideas in a single proposal would be an extensive change, be hard to review, and likely miss new discoveries that otherwise could have been made along the way.

Just like other existing output types, taproot outputs should never reuse keys, for privacy reasons. This does not only apply to the particular leaf that was used to spend an output but to all leaves committed to in the output. If leaves were reused, it could happen that spending a different output would reuse the same Merkle branches in the Merkle proof.

fixes the verification capabilities of offline signing devices by including amount and scriptPubKey in the signature message, avoids unnecessary hashing

Hashes that go into the signature message and the message itself are now computed with a single SHA256 invocation instead of double SHA256. There is no expected security improvement by doubling SHA256 because this only protects against length-extension attacks against SHA256 which are not a concern for signature messages because there is no secret data. Therefore doubling SHA256 is a waste of resources.

The public key is directly included in the output in contrast to typical earlier constructions which store a hash of the public key or script in the output. This has the same cost for senders and is more space efficient overall if the key-based spending path is taken. [2]

BIP342

The inefficient OP_CHECKMULTISIG and OP_CHECKMULTISIGVERIFY opcodes are disabled. Instead, a new opcode OP_CHECKSIGADD is introduced to allow creating the same multisignature policies in a batch-verifiable way.

Disabled script opcodes The following script opcodes are disabled in tapscript: OP_CHECKMULTISIG and OP_CHECKMULTISIGVERIFY[2]. The disabled opcodes behave in the same way as OP_RETURN, by failing and terminating the script immediately when executed, and being ignored when found in unexecuted branch of the script.

This way, new SIGHASH modes can be added, as well as NOINPUT-tagged public keys and a public key constant which is replaced by the taproot internal key for signature validation.

Why is a limit on script size no longer needed? Since there is no scriptCode directly included in the signature hash (only indirectly through a precomputable tapleaf hash), the CPU time spent on a signature check is no longer proportional to the size of the script being executed.

PR Purpose

as stated

privacy, efficiency, and flexibility of Bitcoin's scripting capabilities

This proposal aims to improve privacy, efficiency, and flexibility of Bitcoin's scripting capabilities without adding new security assumptions[1]. Specifically, it seeks to minimize how much information about the spendability conditions of a transaction output is revealed on chain at creation or spending time and to add a number of upgrade mechanisms, while fixing a few minor but long-standing issues

Taproot is an optional feature, and P2PKH key reusers, not concerned with privacy, do not need to us it. Yet, they still get the benefits of smaller multisig transactions.

Is this chart the only benefits of this PR for the majority of users? Is it worth consensus soft-fork? What are the risks?

Taproot

Taproot's advantages become apparent under the assumption that most applications involve outputs that could be spent by all parties agreeing

This use of practical reality built into the bitcoin protocol is exactly what we need more off! Maybe this abstraction can one day be made between alice and bob, because many times alice is really bob in a dress, and bob has no double spend risk on the tx from bob to bob.

defaut spending path
from what i understand, since in practice over 85% of bitcoin transaction are single key, the default path will be basically p2pkh (or the Segwit + Schnorr equivalent)

user story
If Taproot tree path spending is not for the 85% of bitcoin users, then who is it for? actual user stories will go a long way here.

jonatack · 2020-10-13T16:35:44Z

ACK 0e2a5e4 modulo the last four commits (tests) that I plan to finish reviewing tomorrow

achow101

ACK 0e2a5e4

achow101 · 2020-10-13T19:57:56Z

test/functional/test_framework/key.py

@@ -223,7 +242,7 @@ def set(self, data):
                p = SECP256K1.lift_x(x)
                # if the oddness of the y co-ord isn't correct, find the other
                # valid y
-                if (p[1] & 1) != (data[0] & 1):
+                if data[0] & 1:
                    p = SECP256K1.negate(p)


In 3c22663 "tests: add BIP340 Schnorr signature support to test framework"

nit: The code here seems to be entirely unnecessary as lift_x ensures the evenness of y is correct. I commented out these 2 lines and no tests failed.

I think the only thing that's wrong here is the comment: with this change, it's not longer "correcting" the oddness; it's just negating if an odd Y coordinate is desired.

The code is necessary though, but possibly untested. It's what constructs a point from a compressed public key. It's only used for ECDSA (as BIP340 public keys are x-only, not compressed), and unused in the current tests (which only use the signing side of ECDSA).

Going to leave this for a follow-up, as it's not directly related to Taproot testing.

This comment is also being addressed in #20161.

achow101 · 2020-10-13T20:31:52Z

test/functional/test_framework/key.py

+                        sig_actual = sign_schnorr(seckey, msg, aux_rand)
+                        self.assertEqual(sig.hex(), sig_actual.hex(), "BIP340 test vector %i (%s): sig mismatch" % (i, comment))
+                    except RuntimeError as e:
+                        self.assertFalse("BIP340 test vector %i (%s): signing raised exception %s" % (i, comment, e))


In 3c22663 "tests: add BIP340 Schnorr signature support to test framework"

nit: This should be self.fail rather than assertFalse.

Indeed, will fix in a follow-up.

This is being addressed in #20161.

fjahr · 2020-10-13T22:28:06Z

reACK 0e2a5e4

jamesob

Partial review; more soon.

f8c099e --- [TAPROOT] Refactors ---
107b57d scripted-diff: put ECDSA in name of signature functions
8bd2b4e refactor: rename scriptPubKey in VerifyWitnessProgram to exec_script
5d62e3a refactor: keep spent outputs in PrecomputedTransactionData
450d2b2 --- [TAPROOT] BIP340/341/342 consensus rules ---
9eb5908 Add TaggedHash function (BIP 340)
5de246c Implement Taproot signature hashing (BIP 341)

jamesob · 2020-10-14T15:11:02Z

src/validation.cpp

+        for (const auto& txin : tx.vin) {
+            const COutPoint& prevout = txin.prevout;
+            const Coin& coin = inputs.AccessCoin(prevout);
+            assert(!coin.IsSpent());


Note to other reviewers: even though this is a move from existing code, I was still curious about whether this assert is safe. There are three call-sites for CheckInputScripts(); here they are with the various ways they ensure the input coins aren't already spent (and so this assert won't blow up):

MemPoolAccept::PolicyScriptChecks(): only ever called from MemPoolAccept::AcceptSingleTransaction(), which makes a call to Consensus::CheckTxInputs() via MemPoolAccept::PreChecks() beforehand,

CheckInputsFromMempoolAndCache(): only ever called from MemPoolAccept::ConsensusScriptChecks(), which is only ever called from AcceptSingleTransaction() (case above),

CChainState::ConnectBlock(): call to Consensus::CheckTxInputs() beforehand

jamesob · 2020-10-14T17:02:56Z

src/script/interpreter.cpp

+    const uint8_t spend_type = (ext_flag << 1) + (have_annex ? 1 : 0); // The low bit indicates whether an annex is present.
+    ss << spend_type;
+    if (input_type == SIGHASH_ANYONECANPAY) {
+        ss << tx_to.vin[in_pos].prevout;


Note to reviewers: serializes as [hash][out_index] per COutPoint (c.f. BIP).

jamesob · 2020-10-14T17:03:49Z

src/script/interpreter.cpp

+    ss << spend_type;
+    if (input_type == SIGHASH_ANYONECANPAY) {
+        ss << tx_to.vin[in_pos].prevout;
+        ss << cache.m_spent_outputs[in_pos];


Note to reviewers: serializes as [amount i.e. nValue][scriptPubKey] per CTxOut (c.f. BIP).

src/script/interpreter.cpp

ajtowns

ACK 0e2a5e4 - code review, just nits. However signet activation params are missing (should be disabled as per mainnet/testnet)

ajtowns · 2020-10-15T07:01:53Z

src/script/interpreter.cpp

@@ -1679,14 +1682,35 @@ static bool ExecuteWitnessScript(const Span<const valtype>& stack_span, const CS
    return true;
 }

-static bool VerifyWitnessProgram(const CScriptWitness& witness, int witversion, const std::vector<unsigned char>& program, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* serror)
+static bool VerifyTaprootCommitment(const std::vector<unsigned char>& control, const std::vector<unsigned char>& program, const CScript& script)


Would have expected VerifyTaprootCommitment to take script as a const valtype& -- future taproot versions might not look like current script at all.

Without #13062 that's annoying to do, as it means constructing both a valtype and a CScript with the same data. You're right that future version leafs may not want a script at all, but until then, little reason to add this complication.

ajtowns · 2020-10-15T07:05:10Z

src/script/interpreter.cpp

+
+static bool VerifyWitnessProgram(const CScriptWitness& witness, int witversion, const std::vector<unsigned char>& program, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* serror, bool is_p2sh)
+{
+    CScript exec_script; //!< Actually executed script (last stack item in P2WSH; implied P2PKH script in P2WPKH; leaf script in P2TR)


Could move declaration of exec_script closer to its assignment, and make it const CScript exec_script(script_bytes.begin(), script_bytes.end()); for the p2wsh and p2tr cases.

Yeah, not sure that's worth changing without other improvements though.

…t/tapscript) 0e2a5e4 tests: dumping and minimizing of script assets data (Pieter Wuille) 4567ba0 tests: add generic qa-asset-based script verification unit test (Pieter Wuille) f06e6d0 tests: functional tests for Schnorr/Taproot/Tapscript (Pieter Wuille) 3c22663 tests: add BIP340 Schnorr signature support to test framework (Pieter Wuille) 206fb18 --- [TAPROOT] Tests --- (Pieter Wuille) d7ff237 Activate Taproot/Tapscript on regtest (BIP 341, BIP 342) (Pieter Wuille) e9a021d Make Taproot spends standard + policy limits (Pieter Wuille) 865d2c3 --- [TAPROOT] Regtest activation and policy --- (Pieter Wuille) 72422ce Implement Tapscript script validation rules (BIP 342) (Johnson Lau) 330de89 Use ScriptExecutionData to pass through annex hash (Pieter Wuille) 8bbed4b Implement Taproot validation (BIP 341) (Pieter Wuille) 0664f5f Support for Schnorr signatures and integration in SignatureCheckers (BIP 340) (Pieter Wuille) 5de246c Implement Taproot signature hashing (BIP 341) (Johnson Lau) 9eb5908 Add TaggedHash function (BIP 340) (Pieter Wuille) 450d2b2 --- [TAPROOT] BIP340/341/342 consensus rules --- (Pieter Wuille) 5d62e3a refactor: keep spent outputs in PrecomputedTransactionData (Pieter Wuille) 8bd2b4e refactor: rename scriptPubKey in VerifyWitnessProgram to exec_script (Pieter Wuille) 107b57d scripted-diff: put ECDSA in name of signature functions (Pieter Wuille) f8c099e --- [TAPROOT] Refactors --- (Pieter Wuille) Pull request description: This is an implementation of the Schnorr/taproot consensus rules proposed by BIPs [340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki), [341](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki), and [342](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki). See the list of commits [below](bitcoin#19953 (comment)). No signing or wallet support of any kind is included, as testing is done entirely through the Python test framework. This is a successor to bitcoin#17977 (see discussion following [this comment](bitcoin#17977 (comment))), and will have further changes squashed/rebased. The history of this PR can be found in bitcoin#19997. ACKs for top commit: instagibbs: reACK bitcoin@0e2a5e4 benthecarman: reACK 0e2a5e4 kallewoof: reACK 0e2a5e4 jonasnick: ACK 0e2a5e4 almost only looked at bip340/libsecp related code jonatack: ACK 0e2a5e4 modulo the last four commits (tests) that I plan to finish reviewing tomorrow fjahr: reACK 0e2a5e4 achow101: ACK 0e2a5e4 Tree-SHA512: 1b00314450a2938a22bccbb4e177230cf08bd365d72055f9d526891f334b364c997e260c10bc19ca78440b6767712c9feea7faad9a1045dd51a5b96f7ca8146e

hebasto · 2020-10-18T18:03:20Z

This regression introduced in 4567ba0 is fixed in #20180.

ariard

Code Review ACK 0e2a5e4. Mainly reviewed change since my last review in #17977 at 84ec870.

One thing I'm still inquiring is scope of test coverage. I've started to look at it only now and it's still trying to map if every spec object is correctly covered. I'll try if I can come with any comment improvement suggestion.

ariard · 2020-10-19T23:54:53Z

test/functional/test_framework/script.py

+             - a function, which specifies how to compute the hashing partner
+               in function of the hash of whatever it is combined with
+
+    Returns: script (sPK or redeemScript), tweak, {name:(script, leaf version, negation flag, innerkey, merklepath), ...}


I'm not sure how to read this description compared to the effective return. tweak is returned in 4th position, after internal pubkey in 2nd and the negation flag in 3rd. Further it seems leaves are sorted on (script, version, merklebranch) and doesn't rely on negation flag/ innerkey.

See #20207. All of this was just outdated, thanks for noticing.

ariard · 2020-10-20T00:07:45Z

test/functional/feature_taproot.py

+    * failure: a dict of entries to override in the context when intentionally failing to spend (if None, no_fail will be set)
+    * standard: whether the (valid version of) spending is expected to be standard
+    * err_msg: a string with an expected error message for failure (or None, if not cared about)
+    * sigops_weight: the pre-taproot sigops weight consumed by a successful spend


nit: need_vin_vout_mismatch isn't commented

Added in #20207.

ariard · 2020-10-20T01:10:31Z

src/script/interpreter.cpp

 {
-    CScript scriptPubKey;
+    const int path_len = (control.size() - TAPROOT_CONTROL_BASE_SIZE) / TAPROOT_CONTROL_NODE_SIZE;
+    const XOnlyPubKey p{uint256(std::vector<unsigned char>(control.begin() + 1, control.begin() + TAPROOT_CONTROL_BASE_SIZE))};


A code comment to hint about the +1 would be great. Like "Exclude parity bit from internal pubkey"

I added a bunch of comments around this in #20207. The first byte contains both the leaf version and the parity bit, btw.

ariard · 2020-10-20T01:21:09Z

src/pubkey.h

@@ -169,7 +170,7 @@ class CPubKey
    /*
     * Check syntactic correctness.
     *
-     * Note that this is consensus critical as CheckSig() calls it!
+     * Note that this is consensus critical as CheckECDSASignature() calls it!


As a side-note, it could be worthy to document what is meaned here by "syntactic correctness" if it's consensus criticial. AFAICT, pubkey must be superior to 0 ?

Added comments in #20207.

bitcoin/src/pubkey.h

Line 253 in b2f5c38

/** Compute the Taproot tweak as specified in BIP341, with *this as internal

- is the asterisk in *this intentional?

ariard · 2020-10-20T01:23:25Z

src/script/interpreter.cpp

+    success = !sig.empty();
+    if (success) {
+        // Implement the sigops/witnesssize ratio test.
+        // Passing with an upgradable public key version is also counted.


Can future upgradable public key define their own sigops rules without branching inside the if (success) branch ?

They can certainly define their own cost rules, as long as the cost is at least 50 vbytes per check. I'm not sure what you mean by "without branching".

I meaned that we do the sigops/witnesssize ratio test before the pubkey size one. If a future softforked new pubkey type comes with its own new ratio test, maybe the code structure isn't going to be adequate ?

ariard · 2020-10-20T01:24:41Z

src/script/interpreter.cpp

+        /*
+         *  New public key version softforks should be defined before this `else` block.
+         *  Generally, the new code should not do anything but failing the script execution. To avoid
+         *  consensus bugs, it should not modify any existing values (including `success`).


Can we enforce this assign-once property with either some cpp magic or compiler option ? I've no idea.

I'm sure there are ways to solve these softforkability guarantees more generically by encapsulating modifiable properties in an object... but the risks from refactoring consensus code to allow that are probably far bigger than the risk a bug would be missed in future consensus logic (probably a very rare event).

maflcko

Concept ACK 0e2a5e4

left some questions in the test commits

src/policy/policy.cpp

test/functional/test_framework/key.py

test/functional/feature_taproot.py

src/Makefile.test.include

src/test/script_tests.cpp

2d8099c Mention units of MAX_STANDARD_ policy constants (Pieter Wuille) 84e29c7 Mention in validation that IsWitnessStandard tests for P2TR (Pieter Wuille) f867cbc Clean up assets test minimizer LDFLAGS (Pieter Wuille) ea0e786 Document additional IsWitnessStandard behavior (Pieter Wuille) 6040de9 Add comments on CPubKey::IsValid (Pieter Wuille) 8dbb7de Add comments to VerifyTaprootCommitment (Pieter Wuille) cdf900c Document need_vin_vout_mismatch argument to make_spender (Pieter Wuille) 18246ed Fix and improve taproot_construct comments (Pieter Wuille) Pull request description: Addressing some review comments raised here: bitcoin/bitcoin#19953 (review) and bitcoin/bitcoin#19953 (review) ACKs for top commit: jonatack: ACK 2d8099c per `git range-diff 5009159 4f10965 2d8099c` ariard: ACK 2d8099c, only changes are comment light improvements on IsValid/IsWitnessStandard. Tree-SHA512: c4881546c379ea8efc7ef99a43cbf3b9cd3f9dde5fd97a07ee66f2b593c78aef0bd8784853c5c9c737b66c269241a1048bbbdd6c964a3d872efd8ba0ec410b68

2d8099c Mention units of MAX_STANDARD_ policy constants (Pieter Wuille) 84e29c7 Mention in validation that IsWitnessStandard tests for P2TR (Pieter Wuille) f867cbc Clean up assets test minimizer LDFLAGS (Pieter Wuille) ea0e786 Document additional IsWitnessStandard behavior (Pieter Wuille) 6040de9 Add comments on CPubKey::IsValid (Pieter Wuille) 8dbb7de Add comments to VerifyTaprootCommitment (Pieter Wuille) cdf900c Document need_vin_vout_mismatch argument to make_spender (Pieter Wuille) 18246ed Fix and improve taproot_construct comments (Pieter Wuille) Pull request description: Addressing some review comments raised here: bitcoin#19953 (review) and bitcoin#19953 (review) ACKs for top commit: jonatack: ACK 2d8099c per `git range-diff 5009159 4f10965 2d8099c` ariard: ACK 2d8099c, only changes are comment light improvements on IsValid/IsWitnessStandard. Tree-SHA512: c4881546c379ea8efc7ef99a43cbf3b9cd3f9dde5fd97a07ee66f2b593c78aef0bd8784853c5c9c737b66c269241a1048bbbdd6c964a3d872efd8ba0ec410b68

f64adc1 test: remove unused function xor_bytes (Sebastian Falbesoner) Pull request description: The function `xor_bytes` was introduced in commit 3c22663 (#19953, BIP340-342 validation), even [code-reviewed](https://github.com/bitcoin/bitcoin/pull/19953/files#r509383731), but actually never used. The [default signing algorithm in BIP340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#Default_Signing) needs a xor operation, but this step is currently done by a single xor operation on large integer operands: ``` t = (sec ^ int.from_bytes(TaggedHash("BIP0340/aux", aux), 'big')).to_bytes(32, 'big') ``` Alternatively, we could keep the function and as well use it: ```diff --- a/test/functional/test_framework/key.py +++ b/test/functional/test_framework/key.py @@ -492,7 +492,7 @@ def sign_schnorr(key, msg, aux=None, flip_p=False, flip_r=False): P = SECP256K1.affine(SECP256K1.mul([(SECP256K1_G, sec)])) if SECP256K1.has_even_y(P) == flip_p: sec = SECP256K1_ORDER - sec - t = (sec ^ int.from_bytes(TaggedHash("BIP0340/aux", aux), 'big')).to_bytes(32, 'big') + t = xor_bytes(sec.to_bytes(32, 'big'), TaggedHash("BIP0340/aux", aux)) kp = int.from_bytes(TaggedHash("BIP0340/nonce", t + P[0].to_bytes(32, 'big') + msg), 'big') % SECP256K1_ORDER assert kp != 0 R = SECP256K1.affine(SECP256K1.mul([(SECP256K1_G, kp)])) ``` ACKs for top commit: practicalswift: cr ACK f64adc1: untested unused code should be removed Tree-SHA512: e9afae303488f19c6f6f44874d3537ed1c8164a197490e2b4e34853db886b858825b719648fa1a30b95177dcee9cf241f94ee9b835f0a2cae07024ce38a8c090

f64adc1 test: remove unused function xor_bytes (Sebastian Falbesoner) Pull request description: The function `xor_bytes` was introduced in commit 3c22663 (bitcoin#19953, BIP340-342 validation), even [code-reviewed](https://github.com/bitcoin/bitcoin/pull/19953/files#r509383731), but actually never used. The [default signing algorithm in BIP340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#Default_Signing) needs a xor operation, but this step is currently done by a single xor operation on large integer operands: ``` t = (sec ^ int.from_bytes(TaggedHash("BIP0340/aux", aux), 'big')).to_bytes(32, 'big') ``` Alternatively, we could keep the function and as well use it: ```diff --- a/test/functional/test_framework/key.py +++ b/test/functional/test_framework/key.py @@ -492,7 +492,7 @@ def sign_schnorr(key, msg, aux=None, flip_p=False, flip_r=False): P = SECP256K1.affine(SECP256K1.mul([(SECP256K1_G, sec)])) if SECP256K1.has_even_y(P) == flip_p: sec = SECP256K1_ORDER - sec - t = (sec ^ int.from_bytes(TaggedHash("BIP0340/aux", aux), 'big')).to_bytes(32, 'big') + t = xor_bytes(sec.to_bytes(32, 'big'), TaggedHash("BIP0340/aux", aux)) kp = int.from_bytes(TaggedHash("BIP0340/nonce", t + P[0].to_bytes(32, 'big') + msg), 'big') % SECP256K1_ORDER assert kp != 0 R = SECP256K1.affine(SECP256K1.mul([(SECP256K1_G, kp)])) ``` ACKs for top commit: practicalswift: cr ACK f64adc1: untested unused code should be removed Tree-SHA512: e9afae303488f19c6f6f44874d3537ed1c8164a197490e2b4e34853db886b858825b719648fa1a30b95177dcee9cf241f94ee9b835f0a2cae07024ce38a8c090

dgpv · 2021-08-31T15:34:49Z

test/functional/test_framework/script.py

+        if input_index < len(txTo.vout):
+            ss += sha256(txTo.vout[input_index].serialize())
+        else:
+            ss += bytes(0 for _ in range(32))


Post-merge comment/question after stumbling upon this while looking at the test framework code:

Is using 32 zero bytes instead of correct BIP341 behaviour (i.e. failing) intentional here ?

The code does not seem accidental, as it is not copied from legacy sighash function, and if I stick assert 0 here, the test/functional/feature_taproot.py test fails, but it is not clear from the backtrace where exactly, a lot of deep_eval() in that backtrace.

If this is intentional, I think that a comment with explanation was in order here, to avoid confusion for the readers of the code.

If this was not intentional, then even if does not cause problems now, it might result in incorrect test behaviour in the future

@sipa, can you please comment on this ?

I think it's testing that a SIGHASH_SINGLE signature without a corresponding output will fail, even if you generate an otherwise reasonable looking signature. If you change it to bytes(42 for _ in range(32)) rather than an assert 0, the tests will still pass, demonstrating the exact value used isn't important. (If you change the length to something other than 32, you'll get an assertion failure slightly later when the length of hashed data is tested)

See the need_vin_vout_mismatch argument to make_spender(), which is set to True in the "Test SIGHASH_SINGLE behavior in combination with mismatching outputs" section. If you disable those two spenders, then adding the assert 0 when there isn't a corresponding output works fine.

It is very much looks like you are correct, these two tests depend on this behavior of TaprootSignatureHash.

Without any comment explaining this, It will confuse people who read only a part of the code. Some people might copy that code and have problems later. I only cross-referenced this code with C++ implementation in Core while writing my own implementation in python, but I think it is definitely a possibility that someone may just copy the whole TaprootSignatureHash, and maybe even without looking at the comments. Other possibility might be that TaprootSignatureHash is reused for some another purpose inside the test framework without taking this behavior into account.

Might it be better to just do add_spender( ... , failure={"hashtype_actual": hashtype, "sighash": lambda(ctx): b"\x00"*32}, ... ) for these two tests ? It is not that TaprootSignatureHash itself is tested here, AFAIU, and expected behavior is incorrect hash returned, so why not just return it right away ? Alternative might be a special flag in the context "invalid sighash expected instead of failure". Or at least a comment with explanation inside TaprootSignatureHash, to spare readers from confusion.

fanquake added the Consensus label Sep 14, 2020

sipa force-pushed the taproot branch from 6c1edf0 to 111be54 Compare September 14, 2020 05:20

jnewbery reviewed Sep 14, 2020

View reviewed changes

configure.ac Show resolved Hide resolved

flack reviewed Sep 14, 2020

View reviewed changes

src/policy/policy.cpp Show resolved Hide resolved

sipa force-pushed the taproot branch from 111be54 to e21e892 Compare September 14, 2020 18:32

sipa force-pushed the taproot branch 4 times, most recently from f1bbd52 to 687e7ea Compare September 14, 2020 19:45

benthecarman reviewed Sep 19, 2020

View reviewed changes

src/pubkey.h Outdated Show resolved Hide resolved

benthecarman reviewed Sep 20, 2020

View reviewed changes

src/script/script.cpp Outdated Show resolved Hide resolved

src/script/script_error.cpp Outdated Show resolved Hide resolved

achow101 reviewed Oct 13, 2020

View reviewed changes

jamesob reviewed Oct 14, 2020

View reviewed changes

laanwj merged commit 3caee16 into bitcoin:master Oct 15, 2020

ajtowns reviewed Oct 15, 2020

View reviewed changes

dr-orlovsky mentioned this pull request Oct 16, 2020

Tagged hashes for taproot rust-bitcoin/rust-bitcoin#259

Merged

hebasto mentioned this pull request Oct 18, 2020

test: Fix -Wunused-function warnings if configured --without-libs #20180

Merged

ariard reviewed Oct 20, 2020

View reviewed changes

sipa mentioned this pull request Oct 20, 2020

Follow-up extra comments on taproot code and tests #20207

Merged

maflcko reviewed Oct 21, 2020

View reviewed changes

src/test/script_tests.cpp Show resolved Hide resolved

theStack mentioned this pull request Feb 7, 2021

test: remove unused function xor_bytes #21100

Merged

synctext mentioned this pull request Feb 24, 2021

Bounty payout voting and payment execution - self-evolving AI-DAO Tribler/tribler#5986

Closed

devos50 mentioned this pull request Mar 1, 2021

A Mobile and Generic Asset Exchange using Liquidity Pooling [group 2] Tribler/tribler#5996

Closed

Sjors mentioned this pull request May 22, 2021

Basic Taproot signing support for descriptor wallets #21365

Merged

lontivero mentioned this pull request Jun 30, 2021

[WIP] Taproot scripts validation MetacoSA/NBitcoin#1019

Closed

dgpv reviewed Aug 31, 2021

View reviewed changes

kwvg added a commit to kwvg/dash that referenced this pull request Feb 28, 2022

partial bitcoin#19953: Keep spent outputs in PrecomputedTransactionData

94ef082

kwvg added a commit to kwvg/dash that referenced this pull request Feb 28, 2022

partial bitcoin#19953: Keep spent outputs in PrecomputedTransactionData

2f304ab

kwvg added a commit to kwvg/dash that referenced this pull request Feb 28, 2022

partial bitcoin#19953: Keep spent outputs in PrecomputedTransactionData

acf0796

kwvg added a commit to kwvg/dash that referenced this pull request Mar 13, 2022

partial bitcoin#19953: Keep spent outputs in PrecomputedTransactionData

5e5b913

kwvg added a commit to kwvg/dash that referenced this pull request Mar 24, 2022

partial bitcoin#19953: Keep spent outputs in PrecomputedTransactionData

6522e99

bitcoin locked as resolved and limited conversation to collaborators Sep 1, 2022

Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #19953

Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #19953

Conversation

sipa commented Sep 14, 2020 • edited Loading

sipa commented Sep 14, 2020 • edited Loading

Refactors (sipa/bitcoin@f8c099e...450d2b2) [+50 -39]:

BIP340/341/342 consensus rules (sipa/bitcoin@450d2b2...865d2c3) [+693 -50]:

Regtest activation and policy (sipa/bitcoin@865d2c3...206fb18) [+98 -8]:

Tests (sipa/bitcoin@206fb18...0e2a5e4) [+2148 -28]:

jnewbery left a comment

Choose a reason for hiding this comment

sipa commented Sep 14, 2020

instagibbs commented Sep 14, 2020 • edited Loading

sipa commented Sep 14, 2020 • edited Loading

benthecarman commented Sep 14, 2020

sipa commented Sep 14, 2020 • edited Loading

sipa commented Sep 16, 2020 • edited Loading

jnewbery commented Sep 16, 2020

instagibbs commented Sep 16, 2020

DrahtBot commented Sep 19, 2020 • edited Loading

Conflicts

jaybny commented Sep 22, 2020

My expectation was to do a code review for taproot, but unfortunately, I cannot ACK the concept or the approach of this PR at this time.

Background

Questions

Concerns

PR Purpose

Taproot

jonatack commented Oct 13, 2020

achow101 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

fjahr commented Oct 13, 2020

jamesob 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

ajtowns 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

hebasto commented Oct 18, 2020

ariard 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

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

Choose a reason for hiding this comment

maflcko 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

dgpv Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

sipa commented Sep 14, 2020 •

edited

Loading

sipa commented Sep 14, 2020 •

edited

Loading

Refactors (sipa/bitcoin@`f8c099e...450d2b2`) [+50 -39]:

BIP340/341/342 consensus rules (sipa/bitcoin@`450d2b2...865d2c3`) [+693 -50]:

Regtest activation and policy (sipa/bitcoin@`865d2c3...206fb18`) [+98 -8]:

Tests (sipa/bitcoin@`206fb18...0e2a5e4`) [+2148 -28]:

instagibbs commented Sep 14, 2020 •

edited

Loading

sipa commented Sep 14, 2020 •

edited

Loading

sipa commented Sep 14, 2020 •

edited

Loading

sipa commented Sep 16, 2020 •

edited

Loading

DrahtBot commented Sep 19, 2020 •

edited

Loading

dgpv Sep 1, 2021 •

edited

Loading