Currently we have two segwitv1 output script types that are considered standard:
TxoutType::WITNESS_V1_TAPROOT (P2TR): witness program has size 32 (introduced with taproot soft-fork)TxoutType::ANCHOR (P2A): witness program is {0x4e, 0x7e} (introduced with #30352)This PR adds them to the script standardness unit tests where missing, i.e. for using them with the ExtractDestination and GetScriptForDestination functions.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31340.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | rkrux, instagibbs, hodlinator |
| Stale ACK | tdb3 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
200 | @@ -201,6 +201,9 @@ BOOST_AUTO_TEST_CASE(script_standard_Solver_failure) 201 | s.clear(); 202 | s << OP_0 << std::vector<unsigned char>(19, 0x01); 203 | BOOST_CHECK_EQUAL(Solver(s, solutions), TxoutType::NONSTANDARD); 204 | + s.clear(); 205 | + s << OP_1 << std::vector<unsigned char>(33, 0x01);
nit: this would be "undefined" rather than "incorrect" like the segwitv0 case
Reworked the comments to match the pattern used in previous cases, which is
// TxoutType::INTENDED_TYPE with <some minor change that leads to different solver outcome>
(the comment "WITNESS_UNKNOWN with incorrect program size" didn't make any sense imho)
I kept the "incorrect" term, but wrote the outcome explanation in parantheses. Open for suggestions.
how about
diff --git a/src/addresstype.h b/src/addresstype.h
index 93cdf66c5b..09f3063c61 100644
--- a/src/addresstype.h
+++ b/src/addresstype.h
@@ -115,14 +115,17 @@ public:
if (w1.GetWitnessVersion() > w2.GetWitnessVersion()) return false;
return w1.GetWitnessProgram() < w2.GetWitnessProgram();
}
};
+/** Witness program for Pay-to-Anchor output script type */
+static const std::vector<unsigned char> anchor_bytes{0x4e, 0x73};
+
struct PayToAnchor : public WitnessUnknown
{
- PayToAnchor() : WitnessUnknown(1, {0x4e, 0x73}) {
- Assume(CScript::IsPayToAnchor(1, {0x4e, 0x73}));
+ PayToAnchor() : WitnessUnknown(1, anchor_bytes) {
+ Assume(CScript::IsPayToAnchor(1, anchor_bytes));
};
};
/**
* A txout script categorized into standard templates.
diff --git a/src/test/script_standard_tests.cpp b/src/test/script_standard_tests.cpp
index 74920b0fbd..67f91f43fd 100644
--- a/src/test/script_standard_tests.cpp
+++ b/src/test/script_standard_tests.cpp
@@ -2,16 +2,16 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <test/data/bip341_wallet_vectors.json.h>
+#include <addresstype.h>
#include <key.h>
#include <key_io.h>
#include <script/script.h>
#include <script/signingprovider.h>
#include <script/solver.h>
-#include <test/util/script.h>
#include <test/util/setup_common.h>
#include <util/strencodings.h>
#include <boost/test/unit_test.hpp>
diff --git a/src/test/util/script.h b/src/test/util/script.h
index c046bcab66..96c4d55e5a 100644
--- a/src/test/util/script.h
+++ b/src/test/util/script.h
@@ -28,12 +28,9 @@ static const CScript P2WSH_EMPTY{
return hash;
}())};
static const std::vector<std::vector<uint8_t>> P2WSH_EMPTY_TRUE_STACK{{static_cast<uint8_t>(OP_TRUE)}, {}};
static const std::vector<std::vector<uint8_t>> P2WSH_EMPTY_TWO_STACK{{static_cast<uint8_t>(OP_2)}, {}};
-/** Witness program for Pay-to-Anchor output script type */
-static const std::vector<unsigned char> anchor_bytes{0x4e, 0x73};
-
/** Flags that are not forbidden by an assert in script validation */
bool IsValidFlagCombination(unsigned flags);
#endif // BITCOIN_TEST_UTIL_SCRIPT_H
which should get rid of all instances of 0x4e, 0x73?
Could also move it into script/script.h and make it a static member of CScript?
how about
...which should get rid of all instances of 0x4e, 0x73? @instagibbs: Thanks, taken!
Could also move it into script/script.h and make it a static member of CScript?
Kept as-is for minimal diff, but happy to also do that change if others feel strongly about it.
116 | @@ -117,10 +117,13 @@ struct WitnessUnknown 117 | } 118 | }; 119 | 120 | +/** Witness program for Pay-to-Anchor output script type */ 121 | +static const std::vector<unsigned char> anchor_bytes{0x4e, 0x73};
nit: Should this be ANCHOR_BYTES instead?
https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
Would prefer:
struct PayToAnchor.constexpr & array to minimize dynamic memory use.Assume()).struct PayToAnchor : public WitnessUnknown
{
/** Witness program for output script */
static constexpr std::array<unsigned char, 2> PROGRAM{0x4e, 0x73};
static constexpr unsigned int VERSION = 1;
PayToAnchor() : WitnessUnknown{VERSION, PROGRAM} {
Assume(CScript::IsPayToAnchor(GetWitnessVersion(), GetWitnessProgram()));
};
};
196 | @@ -197,14 +197,18 @@ BOOST_AUTO_TEST_CASE(script_standard_Solver_failure) 197 | s << OP_RETURN << std::vector<unsigned char>({75}) << OP_ADD; 198 | BOOST_CHECK_EQUAL(Solver(s, solutions), TxoutType::NONSTANDARD); 199 | 200 | - // TxoutType::WITNESS_UNKNOWN with incorrect program size 201 | + // TxoutType::WITNESS_V0_{KEY,SCRIPT}HASH with incorrect program size (-> consensus-invalid, i.e. non-standard) 202 | s.clear(); 203 | s << OP_0 << std::vector<unsigned char>(19, 0x01); 204 | BOOST_CHECK_EQUAL(Solver(s, solutions), TxoutType::NONSTANDARD); 205 | + // TxoutType::WITNESS_V1_TAPROOT with incorrect program size (-> undefined, but still policy-valid)
Probably overkill, but could add a test for undersized v1 SPK s << OP_1 << std::vector<unsigned char>(31, 0x01); as well.
pico nit: If updating the file again, could add a blank line before this comment to be consistent with other checks in the test.
Done.
ACK a3a4d199e298a76725d0c0424195ae54c7e95fe0
Nice catch/updates. Left a couple of non-blocking minor comments.
Induced failures locally by:
bad_variant_access occur with ExtractDestination()- s << OP_1 << ToByteVector(xpk);
+ s << OP_0 << ToByteVector(scripthash);
+ CKey wrong_key = GenerateRandomKey();
+ CPubKey wrong_pubkey = wrong_key.GetPubKey();
+ auto wrong_xpk = XOnlyPubKey(wrong_pubkey);
auto xpk = XOnlyPubKey(pubkey);
s << OP_1 << ToByteVector(xpk);
BOOST_CHECK(ExtractDestination(s, address));
- BOOST_CHECK(std::get<WitnessV1Taproot>(address) == WitnessV1Taproot(xpk));
+ BOOST_CHECK(std::get<WitnessV1Taproot>(address) == WitnessV1Taproot(wrong_xpk));
Code review a3a4d199e298a76725d0c0424195ae54c7e95fe0
Great to test more aspects of the protocol!
Why is this being removed/changed? https://github.com/bitcoin/bitcoin/blob/22ef95dbe3e467039e6cd18988e66557d94041d1/src/test/script_standard_tests.cpp#L271-L276
As I understand it we were creating a taproot output (v1) using a compressed pubkey with the type-prefix prepended, meaning it was 33 bytes instead of the standard 32. Maybe you could add it back with a clearer comment?
Put some C++-style suggestions in https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:pr/31340_array_span
span input parameters over vector in the same vein as the change to CScript::operator<< in cac846c2fbf6fc69bfc288fd387aa3f68d84d584.PayToAnchor with WitnessUnknown (long-shot for inclusion in this PR)
Concept ACK a3a4d199e298a76725d0c0424195ae54c7e95fe0
I'm in favour of adding these tests. I've not reviewed the PR in detail but only glanced as of now. I'd be quick to complete the review once few of the outstanding feedback is addressed. Specially this one: #31340 (review), this #31340 (review) has a few interesting ideas as well.
Re: #31340#pullrequestreview-2461686975
Why is this being removed/changed? @hodlinator: IIUC, this change is done to conform with the "unknown version" portion of the corresponding comment here: https://github.com/bitcoin/bitcoin/blob/199d47d9629b603d5a23b96c5260c68add00ccb3/src/test/script_standard_tests.cpp#L271
Only 0 and 1 are "known" witness versions as of now, hence, the usage of 2 in the test instead of 1. Though I'd prefer to add the new test instead of replacing the older one because the older test does cover the second branch of the condition on line 165 here: https://github.com/bitcoin/bitcoin/blob/199d47d9629b603d5a23b96c5260c68add00ccb3/src/script/solver.cpp#L156-L177
This^ is non blocking because there might be some other test covering that branch as well that Idk about.
Co-authored-by: Gregory Sanders <gsanders87@gmail.com>
@tdb3 @hodlinator @rkrux: Thanks for your detailed reviews, and sorry for the late reply.
Why is this being removed/changed?
As I understand it we were creating a taproot output (v1) using a compressed pubkey with the type-prefix prepended, meaning it was 33 bytes instead of the standard 32. Maybe you could add it back with a clearer comment?
@hodlinator: IIUC, this change is done to conform with the "unknown version" portion of the corresponding comment here:
Only 0 and 1 are "known" witness versions as of now, hence, the usage of 2 in the test instead of 1. Though I'd prefer to add the new test instead of replacing the older one because the older test does cover the second branch of the condition on line 165 here:
Good point, restored the segwitv1 test and added the segwitv2 one (in order to avoid reducing test coverage), and added corresponding comments. From the other suggestions, I only addressed #31340 (review) and capitalizing the constant. The scoping of the constant could be done in two places (CScript #31340#pullrequestreview-2451932627 vs. PayToAnchor #31340 (review)), and it's not clear to me which one makes more sense. Maybe something to discuss in a follow-up, that could also include the other C++ style suggestions?
123 | struct PayToAnchor : public WitnessUnknown
124 | {
125 | - PayToAnchor() : WitnessUnknown(1, {0x4e, 0x73}) {
126 | - Assume(CScript::IsPayToAnchor(1, {0x4e, 0x73}));
127 | + PayToAnchor() : WitnessUnknown(1, ANCHOR_BYTES) {
128 | + Assume(CScript::IsPayToAnchor(1, ANCHOR_BYTES));
What's exactly the point of having Assume here? We pass hardcoded values right in the call here and immediately test the correctness of those hardcoded values?
exactly, in case somehow the IsPayToAnchor definition diverged. It's a quick eye-ball to ensure correctness.
Hmm interesting, Assume is growing on me.
Would have been nice if IsPayToAnchor was constexpr and provably side-effect-free so that compilers could optimize the call away (#32100).
Re: #31340 (comment)
I'm ok with changing the scoping of ANCHOR_BYTES in a follow-up. IMHO it can be added in CScript that can be further be used by struct PayToAnchor as this file addresstype.h already uses CScript, otherwise in order to deduplicate 4e73, ANCHOR_BYTES from struct PayToAnchor would need to be used in CScript::IsPayToAnchor that would create a circular dependency.
ACK 8284229a28c09c585356dcf7e4bddbc8f2a23755
reACK 8284229a28c09c585356dcf7e4bddbc8f2a23755
196 | @@ -197,14 +197,22 @@ BOOST_AUTO_TEST_CASE(script_standard_Solver_failure) 197 | s << OP_RETURN << std::vector<unsigned char>({75}) << OP_ADD; 198 | BOOST_CHECK_EQUAL(Solver(s, solutions), TxoutType::NONSTANDARD); 199 | 200 | - // TxoutType::WITNESS_UNKNOWN with incorrect program size 201 | + // TxoutType::WITNESS_V0_{KEY,SCRIPT}HASH with incorrect program size (-> consensus-invalid, i.e. non-standard)
Should this be:
// TxoutType::WITNESS_V0_{KEY,SCRIPT}HASH with incorrect program size (-> policy-invalid, i.e. non-standard)
?
Seems like the terminology could be out of sync with the comment below (mentioning "policy" instead of "consensus"). Or is it that segwit v0 program sizes are stricter and v1 program sizes are more flexible, consensus-wise?
For segwitv0, program sizes other than 20 or 32 are indeed consensus-invalid (see function VerifyWitnessProgram), so the comment is correct. And yes, for segwitv1+, not-yet-defined programs are only non-standard, but consensus-valid: https://github.com/bitcoin/bitcoin/blob/a0d737cd7a7fafe2c743360c4ac7b2e72664e5c5/src/script/interpreter.cpp#L1952-L1953
Thread #31340 (review):
Thanks for the references! I'm still unfamiliar with that part. So for MANDATORY_SCRIPT_VERIFY_FLAGS we reach the return true there for Taproot. :+1:
288 | + s << OP_1 << ANCHOR_BYTES; 289 | + BOOST_CHECK(ExtractDestination(s, address)); 290 | + BOOST_CHECK(std::get<PayToAnchor>(address) == PayToAnchor()); 291 | + 292 | // TxoutType::WITNESS_UNKNOWN with unknown version 293 | + // -> segwit version 1 with an undefined program size (33 bytes in this test case)
nit: This might be more precise & helpful?
It took me a few minutes to re-grok the difference between the bytes derived from XOnlyPubKey vs compressed CPubKey.
// -> segwit version 1 with an non-standard program size
// (CPubKey::COMPRESSED_SIZE = 33 bytes in this test case)
Will apply if I have to retouch.
Code review 8284229a28c09c585356dcf7e4bddbc8f2a23755
Thanks for restoring the former test!
Only semi-blocker for me is the consensus/policy question (see inline comment).
Code Review ACK 8284229a28c09c585356dcf7e4bddbc8f2a23755
Adds basic tests for WitnessV1Taproot & (WitnessV1)PayToAnchor outputs.
Only non-test change is replacing array literal with std::vector (which sort of makes sense considering struct PayToAnchor (ab)uses WitnessUnknown as a base (predating this PR) which takes vector as a constructor input).