4dfb6eef70d719a79904cabc4519d7a725de130a test: Add DERSIG tests to script_tests (billymcbip)
884978f3894ac7d96f113a00bbcce45c9785d44a test: Fix a STRICTENC test in script_tests (billymcbip)
527e8ca7b54515e129484824e4df66b5dafdb45f test: Remove outdated comment in script_tests (billymcbip)
Pull request description:
1. Remove a comment referencing a file that no longer exists in the codebase: `script_invalid.json`.
2. Fix a test that isn't implemented as intended. The idea is to test execution order by providing a signature that would cause script failure when parsed. An empty signature does not cause script failure in `CHECKMULTISIG`. Use `OP_1` for the second signature instead of `OP_0`.
3. Copy existing `STRICTENC` tests and change the flag to `DERSIG`. `DERSIG` is a consensus flag (unlike `STRICTENC`), so it'd be good to have dedicated test cases.
`script_tests` pass on my end.
ACKs for top commit:
darosior:
reACK 4dfb6eef70d719a79904cabc4519d7a725de130a
sipa:
ACK 4dfb6eef70d719a79904cabc4519d7a725de130a
Tree-SHA512: aea4aa5199530804561e9f597f69d6cffd7a40d4830919f9371fe97e4d04d8f10e8ed29b91e65e007a3f6e3a38e2881f88dff25831e741ad7592a12ed02b801a
6f7b4323cb46687ba9df7073a9cde427985d2dfc test: remove UNKNOWN_ERROR from script_tests (Bruno Garcia)
bd31a92d671610e1173a4e6f0c761d94724441ae script: use SCRIPT_ERR_SCRIPTNUM for CScriptNum errors (Bruno Garcia)
0ca4dcd78665bc5258a2b9cfcef0dcdf971a88c3 script: add SCRIPT_ERR_SCRIPTNUM error (Bruno Garcia)
Pull request description:
When evaluating a script, the current code is bad for analyzing some errors because it returns `SCRIPT_ERR_UNKNOWN_ERROR` for errors that are clearly known.
`CScriptNum` has two well defined errors: number overflow and non-minimally encoded number. However, for both errors we return as unknown. This PR changes it by adding a new ScriptError that is used for any `CScriptNum` error.
ACKs for top commit:
achow101:
ACK 6f7b4323cb46687ba9df7073a9cde427985d2dfc
w0xlt:
ACK 6f7b4323cb
darosior:
ACK 6f7b4323cb46687ba9df7073a9cde427985d2dfc
Tree-SHA512: e656d9992251fbc95d33966fa18ce64bf714179d51ba6a7f429e5a55bc58e7fc08827e4ab71ace0dd385dac7e1feaea621b49503387793a30eae7a7e44aa6b0f
Fix a test that isn't implemented as intended. The idea is to test execution order by providing a signature that would cause script failure when parsed. An empty signature does not cause script failure in CHECKMULTISIG. Use OP_1 for the second signature instead of OP_0.
a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43 Fix 11-year-old mis-categorized error code in OP_IF evaluation (Calin Culianu)
Pull request description:
This was introduced by commit ab9edbd6b6eb3efbca11f16fa467c3c0ef905708.
It appears the original author may have gotten tired and pasted the wrong error code into this 1 place. Every other situation where the value stack lacks the required number of arguments for the op-code, SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here.
This commit fixes the situation.
EDIT: Note this turns out to be a dupe of the abandoned #30359 .
ACKs for top commit:
billymcbip:
tACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
achow101:
ACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
darosior:
utACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
sedited:
ACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
Tree-SHA512: e8c01a3e2448b5d49b76a0cab3f38a2d0249b71beeb7d9d05d5ecc3812bd91d0bd1d0f78b809b6f4ccb73186fa119cb1ed3779a73284b83a67ae219ef378fa6c
Empty public keys in tapscript are rejected by consensus rules, independent of SCRIPT_VERIFY_STRICTENC. Add SCRIPT_ERR_TAPSCRIPT_EMPTY_PUBKEY to distinguish this from STRICTENC policy failures currently reported as SCRIPT_ERR_PUBKEYTYPE.
5fa81e239a39d161a6d5aba7bcc7e1f22a5be777 test: add valid tx test with minimum-sized ECDSA signature (8 bytes DER-encoded) (Sebastian Falbesoner)
Pull request description:
Currently in our tests, all ECDSA signatures passing verification have sizes of 69 bytes and above (that's the DER-encoded size, i.e. counted without the sighash flag byte) [1]. This PR adds test coverage for the minimum-sized valid case of 8 bytes, by taking an interesting testnet transaction that I stumbled upon:
https://mempool.space/testnet/tx/c6c232a36395fa338da458b86ff1327395a9afc28c5d2daa4273e410089fd433
Note that this is a very obscure construction that only works because the public key used isn't contained in the locking script, but calculated and provided later at spending time (see https://bitcointalk.org/index.php?topic=1729534.msg17309060#msg17309060 for an explainer), to match the message (sighash) and picked signature. So this doesn't represent a use-case that really makes sense in practice, but it can still appear in a block (not in mempool though, due to `SCRIPT_VERIFY_CONST_SCRIPTCODE`), and having test-coverage seems useful.
Can be tested with same patch below (tests crash with the condition `>= 9`, but pass with `>= 8`).
[1] this can be verified by applying the following patch and running the tests:
```diff
diff --git a/src/pubkey.cpp b/src/pubkey.cpp
index a4ca9a170a..bee0caa603 100644
--- a/src/pubkey.cpp
+++ b/src/pubkey.cpp
@@ -288,7 +288,9 @@ bool CPubKey::Verify(const uint256 &hash, const std::vector<unsigned char>& vchS
/* libsecp256k1's ECDSA verification requires lower-S signatures, which have
* not historically been enforced in Bitcoin, so normalize them first. */
secp256k1_ecdsa_signature_normalize(secp256k1_context_static, &sig, &sig);
- return secp256k1_ecdsa_verify(secp256k1_context_static, &sig, hash.begin(), &pubkey);
+ bool ret = secp256k1_ecdsa_verify(secp256k1_context_static, &sig, hash.begin(), &pubkey);
+ if (ret) assert(vchSig.size() >= 69);
+ return ret;
}
```
ACKs for top commit:
ajtowns:
ACK 5fa81e239a39d161a6d5aba7bcc7e1f22a5be777 lgtm
fjahr:
tACK 5fa81e239a39d161a6d5aba7bcc7e1f22a5be777
real-or-random:
utACK 5fa81e239a interesting case
Tree-SHA512: d1f0612fdb71c9238ca0420f574f6f246e60dbd11970b23f21d082c759a89ff98a13b12a1f6266f14f20539ec437b7ab79322082278da32984ddfee2d8893356
e3d7533ac954516d87a09b230c2898f5c9b1e213 test: improves tapscript unit tests (Ethan Heilman)
3e167085bad0d5565100d11d4cca3ae3e4277062 test: Ensures test fails if witness is not hex (Ethan Heilman)
Pull request description:
This commit creates new test utilities for future Taproot script tests within script_tests.json. The key features of this commit are the addition of three new tags: `#SCRIPT#`, `#CONTROLBLOCK#`, and `#TAPROOTOUTPUT#`. These tags streamline the test creation process by eliminating the need to manually generate these components outside the test suite.
* `#SCRIPT#`: Parses Tapscript and outputs a byte string of opcodes.
* `#CONTROLBLOCK#`: Automatically generates the control block for a given Taproot output.
* `#TAPROOTOUTPUT#`: Generates the final Taproot scriptPubKey.
This code was originally part of the OP_CAT PR https://github.com/bitcoin/bitcoin/pull/29247 but was pulled out into a separate PR to reduce the rebase treadmill for the OP_CAT PR.
Additionally this PR adds a check to ensure that if the witness data can not be parsed as hex the test fails. Prior to this PR, the test code would fail silently and set the values it couldn't parse as empty stack elements. This fix was suggested by @instagibbs.
## Rationale
While writing JSON script tests (script_tests.json) for https://github.com/bitcoin/bitcoin/pull/29247 we ran into the following problem. The JSON script tests are simple and easy to write for pre-Tapscript scripts, but adding or changing a Tapscript test requires substantial work per test. Consider the following pre-tapscript test:
```
["'aa' 'bb'", "CAT 0x4c 0x02 0xaabb EQUAL", "P2SH,STRICTENC", "DISABLED_OPCODE", "CAT disabled"]
````
whereas a Tapscript test for the same script (annotated with comments for better readability) would look like:
```
[
[
"aa",
"bb",
"7e4c02aabb87", // output script
"c0d6889cb081036e0faefa3a35157ad71086b123b2b144b649798b494c300a961d", // control block
0.00000001
],
"",
"0x51 0x20 0x15048ed3a65748549c27b671936987093cf73a4c9cb18522a74fb9553060ca99", // Tapscript output
"P2SH,WITNESS,TAPROOT",
"OK",
"TAPSCRIPT CATs aa and bb together and checks if EQUAL to aabb"
]
```
Computing the Tapscript output, such as `0x51 0x20 0x15048ed3a65748549c27b671936987093cf73a4c9cb18522a74fb9553060ca99`, requires writing custom code and running it for each test. The same is true for the Tapscript control block, such as `c0d6889cb081036e0faefa3a35157ad71086b123b2b144b649798b494c300a961d`. If a test is changed or updated new outputs and control blocks must be computed. The complexity of doing this is likely the reason that no one has added any Tapscript tests to JSON script tests until this PR.
In this PR we address this issue by adding the following improvements to JSON script tests:
Adding simple macros ("#SCRIPT# and #CONTROLBLOCK#) that allow the script test parser to automatically generate and inject a valid Tapscript output and control block to be computed automatically from the JSON script.
Allowing Tapscript scripts to use the human readable strings like pre-script scripts by marking the location of the script in the witness stack using #SCRIPT#. This transforms the unreadable script 7e4c02aabb87 into #SCRIPT# CAT 0x4c 0x02 0xaabb EQUAL.
This results in the following JSON script test which is far easier to write and easier to read.
```
[
[
"aa",
"bb",
"#SCRIPT# CAT",
"#CONTROLBLOCK#",
0.00000001
],
"",
"0x51 0x20 #TAPROOTOUTPUT#",
"P2SH,WITNESS,TAPROOT,OP_CAT",
"OK",
"TAPSCRIPT Test of OP_CAT flag by calling CAT on two elements. TAPSCRIPT_OP_CAT flag is set so CAT is executed."
],
```
ACKs for top commit:
instagibbs:
reACK e3d7533ac954516d87a09b230c2898f5c9b1e213
sipa:
utACK e3d7533ac954516d87a09b230c2898f5c9b1e213
janb84:
Re ACK [e3d7533](e3d7533ac9)
Tree-SHA512: 948c3ec28a4b2b222c2d77e48918ed19d298b51d64662fc20959073edd9978fc796516a392da9755a7e173f556e3021816dc6ce8eb3ed16bbe0fa6ebc574fd48
This commit creates new test utilities for future Taproot script
tests within script_tests.json. The key features of this commit are the
addition of three new tags: `#SCRIPT#`, `#CONTROLBLOCK#`, and
`#TAPROOTOUTPUT#`. These tags streamline the test creation process by
eliminating the need to manually generate these components outside the
test suite.
* `#SCRIPT#`: Parses Tapscript and outputs a byte string of opcodes.
* `#CONTROLBLOCK#`: Automatically generates the control block for a given
Taproot output.
* `#TAPROOTOUTPUT#`: Generates the final Taproot scriptPubKey.
Update src/test/script_tests.cpp
Co-authored-by: Jan B <608446+janb84@users.noreply.github.com>
This was introduced by commit ab9edbd6b6eb3efbca11f16fa467c3c0ef905708.
It appears the original author may have gotten tired and pasted the wrong
error code into this 1 place. Every other situation where the value stack
lacks the required number of arguments for the op-code,
SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here.
This commit fixes the situation.
Also in this commit:
- Fix script_tests to adjust to the corrected error message
- Fix p2p_invalid_tx functional test to produce the desired error message
In order to ensure that the change of nVersion to a uint32_t in the
previous commit has no effect, rename nVersion to version in this commit
so that reviewers can easily spot if a spot was missed or if there is a
check somewhere whose semantics have changed.
Similar to 19db590d044efe7d474a16720e5b56e7b55db54c, which removed these
for the valid tests. Not removing ones that cause a false/empty stack
error because these tests should fail due to being invalid with CSV/CLTV
Instead of using /16 netgroups to bucket nodes in Addrman for connection
diversification, ASN, which better represents an actor in terms
of network-layer infrastructure, is used.
For testing, asmap.raw is used. It represents a minimal
asmap needed for testing purposes.
This commit adds comments referencing multiple CVEs both in production and test code.
CVEs covered in this commit:
CVE-2010-5137
CVE-2010-5139
CVE-2010-5141
CVE-2012-1909
CVE-2012-2459
CVE-2012-3789
CVE-2018-17144
1e747e3c1e Make segwit failure due to CLEANSTACK violation return a SCRIPT_ERR_CLEANSTACK error code. (Mark Friedenbach)
Pull request description:
If a segwit script terminates with a stack size not equal to one, the current error code is EVAL_FALSE. This is semantically wrong, and prevents explicitly checking CLEANSTACK violations in the unit tests. This PR changes the error code (and affected unit tests) to use SCRIPT_ERROR_CLEANSTACK instead of SCRIPT_ERROR_EVAL_FALSE.
Tree-SHA512: 8f7b1650f7a23a942cde1070e3e56420be456b4a7be42515b237e95557bf2bd5e7ba9aabd213c8092bea28c165dbe73f5a3486300089aeb01e698151b42484b1
1f45e21 scripted-diff: Convert 11 enums into scoped enums (C++11) (practicalswift)
Pull request description:
Rationale (from Bjarne Stroustrup's ["C++11 FAQ"](http://www.stroustrup.com/C++11FAQ.html#enum)):
>
> The enum classes ("new enums", "strong enums") address three problems with traditional C++ enumerations:
>
> * conventional enums implicitly convert to int, causing errors when someone does not want an enumeration to act as an integer.
> * conventional enums export their enumerators to the surrounding scope, causing name clashes.
> * the underlying type of an enum cannot be specified, causing confusion, compatibility problems, and makes forward declaration impossible.
>
> The new enums are "enum class" because they combine aspects of traditional enumerations (names values) with aspects of classes (scoped members and absence of conversions).
Tree-SHA512: 9656e1cf4c3cabd4378c7a38d0c2eaf79e4a54d204a3c5762330840e55ee7e141e188a3efb2b4daf0ef3110bbaff80d8b9253abf2a9b015cdc4d60b49ac2b914
