Follow-up extra comments on taproot code and tests

sipa commented at 10:35 pm on October 20, 2020: member

Addressing some review comments raised here: #19953#pullrequestreview-512238027 and #19953#pullrequestreview-513499921

DrahtBot added the label Consensus on Oct 20, 2020

ariard approved

ariard commented at 1:29 am on October 21, 2020: member

Code review ACK 51475be, thanks for the clarifications

benthecarman approved

benthecarman commented at 1:48 am on October 21, 2020: contributor

ACK

sipa commented at 8:22 pm on October 21, 2020: member

Added a few more things to address @MarcoFalke’s review comments #19953#pullrequestreview-513499921.

in test/functional/test_framework/key.py:21 in 6dffc759ee outdated

21@@ -22,6 +22,7 @@ def TaggedHash(tag, data):
22     return hashlib.sha256(ss).digest()

MarcoFalke commented at 8:42 pm on October 21, 2020:

0test/functional/test_framework/key.py:13:1: F401 'sys' imported but unused

sipa commented at 8:48 pm on October 21, 2020:

Done.

MarcoFalke commented at 8:43 pm on October 21, 2020: member

ACK

Couldn’t find the doc comment change for #19953 (review) though

sipa force-pushed on Oct 21, 2020

DrahtBot commented at 0:33 am on October 22, 2020: member

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

Conflicts

No conflicts as of last run.

MarcoFalke commented at 7:20 am on October 22, 2020: member

ACK 029d2d9817d5149d75f0b23f1a82acf8834560e3

only changes comments and tests

MarcoFalke commented at 7:57 am on October 25, 2020: member

@ariard Mind to re-ACK?

in src/policy/policy.h:108 in c991fe584c outdated

101@@ -102,7 +102,9 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
102     /**
103      * Check if the transaction is over standard P2WSH resources limit:
104      * 3600bytes witnessScript size, 80bytes per witness stack element, 100 witness stack elements
105-     * These limits are adequate for multi-signature up to n-of-100 using OP_CHECKSIG, OP_ADD, and OP_EQUAL,
106+     * These limits are adequate for multi-signature up to n-of-100 using OP_CHECKSIG, OP_ADD, and OP_EQUAL.
107+     *
108+     * Also enforce a maximum stack size limit and no annexes for P2TR spends.

ariard commented at 2:06 pm on October 28, 2020:

The first half of this comment is confusing. AFAIK, the check enforced L261 is on a stack item size not the stack size itself ?

Few improvements suggestions w.r.t MAX_STANDARD_TAPROOT_STACK_ITEM_SIZE:

normalize naming compared to MAX_SCRIPT_ELEMENT_SIZE, item or element are designating the same thing ?
replace TAPSCRIPT with P2TR, akin to usage of of P2WSH/P2WPKH across the codebase
add the unit (bytes) around variable comment

sipa commented at 10:01 pm on October 30, 2020:

The first half of this comment is confusing. AFAIK, the check enforced L261 is on a stack item size not the stack size itself ?

Indeed, fixed.

normalize naming compared to MAX_SCRIPT_ELEMENT_SIZE, item or element are designating the same thing ?

They’re not exactly the same; MAX_SCRIPT_ELEMENT_SIZE constant is a consensus limit on the size of script pushes, and witness inputs; MAX_*_STACK_ITEM_SIZE is a policy rule that only applies to inputs (and isn’t relevant during execution). They’re indeed confusing, but the issue already exists in the codebase, and I’d rather keep this one analoguous to MAX_STANDARD_P2WSH_STACK_ITEM_SIZE.

replace TAPSCRIPT with P2TR, akin to usage of of P2WSH/P2WPKH across the codebase

They’re not the same thing. P2TR is an output type. TAPSCRIPT refers to BIP342 spends of such outputs (meaning P2TR outputs spent using the script path, with a leaf version 0xc0).

add the unit (bytes) around variable comment

Done.

ariard commented at 2:07 pm on October 28, 2020: member

ACK 029d2d9 pending on fixing the confusion around the new comment on “maximum stack size limit”

sipa force-pushed on Oct 30, 2020

MarcoFalke commented at 9:43 am on November 2, 2020: member

re-ACK 4f1096562d

in test/functional/test_framework/script.py:851 in 4f1096562d outdated

851              - another list of items (with the same structure)
852-             - a function, which specifies how to compute the hashing partner
853-               in function of the hash of whatever it is combined with
854+             - a list of two items; the first of which is an item itself, and the
855+               second is a function. The function takes as input the Merkle root of the
856+               first item, and produces a (ficticious) partner to hash with.

jonatack commented at 11:34 am on November 2, 2020:

975ec514 s/ficticious/fictitious/

sipa commented at 5:28 am on November 18, 2020:

Apparently I didn’t know how to spell that word. Fixed.

in src/pubkey.h:179 in 4f1096562d outdated

174+     * don't match the length of the data), the size is set to 0. Thus,
175+     * by checking size, one can observe whether Set() or deserialization has
176+     * failed.
177+     *
178+     * This does not check for more than that. In particular, it does not verify
179+     * that the coordinates correspond to a point on the curve (see IsFullyValid

jonatack commented at 11:38 am on November 2, 2020:

515a74145 for consistency with referencing Set() in the same comment block

0     * that the coordinates correspond to a point on the curve (see IsFullyValid()

sipa commented at 5:27 am on November 18, 2020:

Done.

in src/policy/policy.h:106 in 4f1096562d outdated

101@@ -102,7 +102,9 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
102     /**
103      * Check if the transaction is over standard P2WSH resources limit:
104      * 3600bytes witnessScript size, 80bytes per witness stack element, 100 witness stack elements
105-     * These limits are adequate for multi-signature up to n-of-100 using OP_CHECKSIG, OP_ADD, and OP_EQUAL,
106+     * These limits are adequate for multi-signature up to n-of-100 using OP_CHECKSIG, OP_ADD, and OP_EQUAL.

jonatack commented at 11:45 am on November 2, 2020:

3d0948d per codebase usage and optech style guide

0     * These limits are adequate for multisignatures up to k-of-100 using OP_CHECKSIG, OP_ADD, and OP_EQUAL.

sipa commented at 5:27 am on November 18, 2020:

Fixed.

jonatack commented at 11:51 am on November 2, 2020: member

ACK modulo minor comments below

ariard commented at 11:51 pm on November 8, 2020: member

ACK 4f10965

sipa force-pushed on Nov 18, 2020

sipa force-pushed on Nov 25, 2020

sipa commented at 8:51 pm on November 25, 2020: member

Rebased.

laanwj added this to the "Blockers" column in a project

jonatack commented at 8:58 pm on November 26, 2020: member

git range-diff 5009159 4f10965 4b8720f LGTM…some of the changes seem to have been lost in the last rebase per git range-diff 5009159 4f10965 0efa4da

Fix and improve taproot_construct comments 18246ed5f0

Document need_vin_vout_mismatch argument to make_spender cdf900cbf2

Add comments to VerifyTaprootCommitment 8dbb7de67c

Add comments on CPubKey::IsValid 6040de9a46

Document additional IsWitnessStandard behavior ea0e78677b

Clean up assets test minimizer LDFLAGS f867cbcc26

Mention in validation that IsWitnessStandard tests for P2TR 84e29c7c01

Mention units of MAX_STANDARD_ policy constants 2d8099c713

sipa commented at 10:56 pm on November 26, 2020: member

@jonatack Thanks for catching that, fixed.

sipa force-pushed on Nov 26, 2020

jonatack commented at 11:48 pm on November 26, 2020: member

ACK 2d8099c per git range-diff 5009159 4f10965 2d8099c

ariard commented at 11:38 am on December 1, 2020: member

ACK 2d8099c, only changes are comment light improvements on IsValid/IsWitnessStandard.

MarcoFalke merged this on Dec 1, 2020

MarcoFalke closed this on Dec 1, 2020

sidhujag referenced this in commit 6f21c27635 on Dec 1, 2020

fanquake removed this from the "Blockers" column in a project

DrahtBot locked this on Feb 15, 2022

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

Conflicts