script_standard_tests
#31340
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31340.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | instagibbs, 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);
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
0diff --git a/src/addresstype.h b/src/addresstype.h
1index 93cdf66c5b..09f3063c61 100644
2--- a/src/addresstype.h
3+++ b/src/addresstype.h
4@@ -115,14 +115,17 @@ public:
5 if (w1.GetWitnessVersion() > w2.GetWitnessVersion()) return false;
6 return w1.GetWitnessProgram() < w2.GetWitnessProgram();
7 }
8 };
9
10+/** Witness program for Pay-to-Anchor output script type */
11+static const std::vector<unsigned char> anchor_bytes{0x4e, 0x73};
12+
13 struct PayToAnchor : public WitnessUnknown
14 {
15- PayToAnchor() : WitnessUnknown(1, {0x4e, 0x73}) {
16- Assume(CScript::IsPayToAnchor(1, {0x4e, 0x73}));
17+ PayToAnchor() : WitnessUnknown(1, anchor_bytes) {
18+ Assume(CScript::IsPayToAnchor(1, anchor_bytes));
19 };
20 };
21
22 /**
23 * A txout script categorized into standard templates.
24diff --git a/src/test/script_standard_tests.cpp b/src/test/script_standard_tests.cpp
25index 74920b0fbd..67f91f43fd 100644
26--- a/src/test/script_standard_tests.cpp
27+++ b/src/test/script_standard_tests.cpp
28@@ -2,16 +2,16 @@
29 // Distributed under the MIT software license, see the accompanying
30 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
31
32 #include <test/data/bip341_wallet_vectors.json.h>
33
34+#include <addresstype.h>
35 #include <key.h>
36 #include <key_io.h>
37 #include <script/script.h>
38 #include <script/signingprovider.h>
39 #include <script/solver.h>
40-#include <test/util/script.h>
41 #include <test/util/setup_common.h>
42 #include <util/strencodings.h>
43
44 #include <boost/test/unit_test.hpp>
45
46diff --git a/src/test/util/script.h b/src/test/util/script.h
47index c046bcab66..96c4d55e5a 100644
48--- a/src/test/util/script.h
49+++ b/src/test/util/script.h
50@@ -28,12 +28,9 @@ static const CScript P2WSH_EMPTY{
51 return hash;
52 }())};
53 static const std::vector<std::vector<uint8_t>> P2WSH_EMPTY_TRUE_STACK{{static_cast<uint8_t>(OP_TRUE)}, {}};
54 static const std::vector<std::vector<uint8_t>> P2WSH_EMPTY_TWO_STACK{{static_cast<uint8_t>(OP_2)}, {}};
55
56-/** Witness program for Pay-to-Anchor output script type */
57-static const std::vector<unsigned char> anchor_bytes{0x4e, 0x73};
58-
59 /** Flags that are not forbidden by an assert in script validation */
60 bool IsValidFlagCombination(unsigned flags);
61
62 #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?
Co-authored-by: Gregory Sanders <gsanders87@gmail.com>
how about
0...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()). 0struct PayToAnchor : public WitnessUnknown
1{
2 /** Witness program for output script */
3 static constexpr std::array<unsigned char, 2> PROGRAM{0x4e, 0x73};
4
5 static constexpr unsigned int VERSION = 1;
6
7 PayToAnchor() : WitnessUnknown{VERSION, PROGRAM} {
8 Assume(CScript::IsPayToAnchor(GetWitnessVersion(), GetWitnessProgram()));
9 };
10};
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.
ACK a3a4d199e298a76725d0c0424195ae54c7e95fe0
Nice catch/updates. Left a couple of non-blocking minor comments.
Induced failures locally by:
bad_variant_access occur with ExtractDestination()
0- s << OP_1 << ToByteVector(xpk);
1+ s << OP_0 << ToByteVector(scripthash);
0+ CKey wrong_key = GenerateRandomKey();
1+ CPubKey wrong_pubkey = wrong_key.GetPubKey();
2+ auto wrong_xpk = XOnlyPubKey(wrong_pubkey);
3 auto xpk = XOnlyPubKey(pubkey);
4 s << OP_1 << ToByteVector(xpk);
5 BOOST_CHECK(ExtractDestination(s, address));
6- BOOST_CHECK(std::get<WitnessV1Taproot>(address) == WitnessV1Taproot(xpk));
7+ 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)
Labels
Tests