Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 19 commits into from Oct 15, 2020

Conversation

sipa
Copy link
Member

@sipa sipa commented Sep 14, 2020

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
Copy link
Member Author

sipa commented Sep 14, 2020

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

Copy link
Member

@jnewbery jnewbery left a comment

Concept ACK :)

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

configure.ac Show resolved Hide resolved
@sipa
Copy link
Member Author

sipa commented Sep 14, 2020

@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 Show resolved Hide resolved
@instagibbs
Copy link
Member

instagibbs commented Sep 14, 2020

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

@sipa
Copy link
Member Author

sipa commented Sep 14, 2020

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.

@sipa sipa force-pushed the taproot branch 4 times, most recently from f1bbd52 to 687e7ea Compare Sep 14, 2020
@benthecarman
Copy link
Contributor

benthecarman commented Sep 14, 2020

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

@sipa
Copy link
Member Author

sipa commented Sep 14, 2020

@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
Copy link
Member Author

sipa commented Sep 16, 2020

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
Copy link
Member

jnewbery commented Sep 16, 2020

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
Copy link
Member

instagibbs commented Sep 16, 2020

No code changes aside from unit test commit 9673fd9

@laanwj laanwj added this to Blockers in High-priority for review Sep 17, 2020
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 19, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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 Outdated Show resolved Hide resolved
src/script/script.cpp Outdated Show resolved Hide resolved
src/script/script_error.cpp Outdated Show resolved Hide resolved
Copy link
Member

@achow101 achow101 left a comment

ACK 0e2a5e4

@@ -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)
Copy link
Member

@achow101 achow101 Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@sipa sipa Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@fanquake fanquake Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is also being addressed in #20161.

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))
Copy link
Member

@achow101 achow101 Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

@sipa sipa Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, will fix in a follow-up.

Copy link
Member

@fanquake fanquake Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being addressed in #20161.

@fjahr
Copy link
Contributor

fjahr commented Oct 13, 2020

reACK 0e2a5e4

Copy link
Member

@jamesob jamesob left a comment

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)

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

@jamesob jamesob Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;
Copy link
Member

@jamesob jamesob Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@jamesob jamesob Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/script/interpreter.cpp Outdated Show resolved Hide resolved
@laanwj laanwj merged commit 3caee16 into bitcoin:master Oct 15, 2020
2 checks passed
Copy link
Contributor

@ajtowns ajtowns left a comment

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

@@ -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)
Copy link
Contributor

@ajtowns ajtowns Oct 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@sipa sipa Oct 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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)
Copy link
Contributor

@ajtowns ajtowns Oct 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@sipa sipa Oct 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 16, 2020
…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
Copy link
Member

hebasto commented Oct 18, 2020

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

Copy link
Member

@ariard ariard left a comment

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.

- 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), ...}
Copy link
Member

@ariard ariard Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@sipa sipa Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* 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
Copy link
Member

@ariard ariard Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: need_vin_vout_mismatch isn't commented

Copy link
Member Author

@sipa sipa Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in #20207.

{
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))};
Copy link
Member

@ariard ariard Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@sipa sipa Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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!
Copy link
Member

@ariard ariard Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

@sipa sipa Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments in #20207.

Copy link
Contributor

@kiminuo kiminuo Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/** Compute the Taproot tweak as specified in BIP341, with *this as internal
- is the asterisk in *this intentional?

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

@ariard ariard Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@sipa sipa Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member

@ariard ariard Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

/*
* 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`).
Copy link
Member

@ariard ariard Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@sipa sipa Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@MarcoFalke MarcoFalke left a comment

Concept ACK 0e2a5e4

left some questions in the test commits

src/policy/policy.cpp Show resolved Hide resolved
src/policy/policy.cpp Show resolved Hide resolved
test/functional/test_framework/key.py Show resolved Hide resolved
test/functional/test_framework/key.py Show resolved Hide resolved
test/functional/feature_taproot.py Show resolved Hide resolved
test/functional/feature_taproot.py Show resolved Hide resolved
src/Makefile.test.include Show resolved Hide resolved
MarcoFalke added a commit to bitcoin-core/gui that referenced this pull request Dec 1, 2020
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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 1, 2020
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
MarcoFalke added a commit that referenced this pull request Feb 15, 2021
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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 15, 2021
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
if input_index < len(txTo.vout):
ss += sha256(txTo.vout[input_index].serialize())
else:
ss += bytes(0 for _ in range(32))
Copy link
Contributor

@dgpv dgpv Aug 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@dgpv dgpv Aug 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sipa, can you please comment on this ?

Copy link
Contributor

@ajtowns ajtowns Sep 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dgpv dgpv Sep 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

kittywhiskers added a commit to kittywhiskers/dash that referenced this pull request Feb 28, 2022
kittywhiskers added a commit to kittywhiskers/dash that referenced this pull request Feb 28, 2022
kittywhiskers added a commit to kittywhiskers/dash that referenced this pull request Feb 28, 2022
kittywhiskers added a commit to kittywhiskers/dash that referenced this pull request Mar 13, 2022
kittywhiskers added a commit to kittywhiskers/dash that referenced this pull request Mar 24, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.