test: Improve code coverage for pubkey checks

billymcbip commented at 1:05 pm on December 12, 2025: contributor

Cover these branches in IsCompressedOrUncompressedPubKey and IsCompressedPubKey:

Non-canonical public key: invalid length for uncompressed key
Non-canonical public key: invalid length for compressed key
Non-canonical public key: invalid prefix for compressed key

See the missed branches here: https://maflcko.github.io/b-c-cov/total.coverage/src/script/interpreter.cpp.gcov.html

script_tests succeed on my end.

DrahtBot added the label Tests on Dec 12, 2025

DrahtBot commented at 1:05 pm on December 12, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34058.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

test: Improve code coverage for pubkey checks a00332ffb3

billymcbip force-pushed on Dec 12, 2025

in src/test/data/script_tests.json:2281 in a00332ffb3

2276+        "0479be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798",
2277+        0.00000001
2278+    ],
2279+    "",
2280+    "0 0x14 0xfe78b0052557b3d67248d04e4f7b99c361167432",
2281+    "P2SH,WITNESS,WITNESS_PUBKEYTYPE",

rkrux commented at 10:41 am on December 15, 2025:

I don’t suppose P2SH flag is needed for a P2WPKH script?

 0diff --git a/src/test/data/script_tests.json b/src/test/data/script_tests.json
 1index 6c47245139..bd276011df 100644
 2--- a/src/test/data/script_tests.json
 3+++ b/src/test/data/script_tests.json
 4@@ -2278,7 +2278,7 @@
 5     ],
 6     "",
 7     "0 0x14 0xfe78b0052557b3d67248d04e4f7b99c361167432",
 8-    "P2SH,WITNESS,WITNESS_PUBKEYTYPE",
 9+    "WITNESS,WITNESS_PUBKEYTYPE",
10     "WITNESS_PUBKEYTYPE",
11     "P2WPKH with invalid prefix for compressed key"
12 ],
13(END)

billymcbip commented at 12:16 pm on December 15, 2025:

If WITNESS is enabled, then P2SH must be enabled as well. See: https://github.com/bitcoin/bitcoin/blob/2210feb4466eff1455468f0a25045fce4b89c55d/src/script/interpreter.cpp#L2114

rkrux commented at 12:43 pm on December 15, 2025:

Huh, you are correct. This test passed when I tried it without the P2SH flag - must be because of it being an erroneous test that would have early returned before hitting this^ statement. I tried removing the flag on a test that doesn’t expect an error and it failed, showing the necessity of the P2SH flag.

 0diff --git a/src/test/data/script_tests.json b/src/test/data/script_tests.json
 1index 6c47245139..7c8ea7387d 100644
 2--- a/src/test/data/script_tests.json
 3+++ b/src/test/data/script_tests.json
 4@@ -2503,7 +2503,7 @@
 5     ],
 6     "",
 7     "0 0x20 0x230828ed48871f0f362ce9432aa52f620f442cc8d9ce7a8b5e798365595a38bb",
 8-    "P2SH,WITNESS",
 9+    "WITNESS",
10     "OK",
11     "P2WSH CHECKMULTISIG with second key uncompressed and signing with the second key"
12 ],

billymcbip commented at 1:06 pm on December 15, 2025:

Also the P2SH exception height is lower than the witness activation height. I think we should only test flag combinations that make practical sense.

rkrux commented at 3:33 pm on December 15, 2025:

Looked a bit into this^, it appears there’s some history here regarding the script verification flags.

rkrux commented at 10:45 am on December 15, 2025: contributor

Code review a00332ffb3b57b36b6cdaf2627a856c8dfab6ea7

Few conditions in the mentioned functions indeed don’t seem to be covered in the tests.

The coverage report seems to be not available in corecheck for some reason: https://corecheck.dev/bitcoin/bitcoin/pulls/34058

Coverage report is currently being generated, please come back later

rkrux commented at 2:49 pm on December 15, 2025: contributor

tACK a00332ffb3b57b36b6cdaf2627a856c8dfab6ea7

billymcbip closed this on Dec 18, 2025

billymcbip commented at 12:36 pm on December 18, 2025: contributor

@rkrux I created a new PR with the exact same changes: #34099. I hope the code coverage job will run correctly on that PR.

test: Improve code coverage for pubkey checks #34058

Code Coverage & Benchmarks

Reviews