Four defects in the test vectors: pre-BIP350 bech32, CPubKey('00'), non-canonical VarInt, and a mislabelled block

[fametrano]

Reporting four findings in bitcoin/tests/, all measured against master at
91e334d831fd16c60c932ad7df42c88fd6567c02. They surfaced while
btclib surveyed this repository's
vectors to decide which ones to vendor
(btclib#199,
btclib#239); we took your two
block-validity files unchanged and left these four alone, so this is the
report rather than a pull request.

Three of the four are assertions that pin behaviour Bitcoin Core rejects, so
fixing them means changing library code, not only the vector — which is why we
are asking first. Happy to send patches for any of them if that is welcome.

1. bech32_encode_decode.json predates BIP350

Lines 4-6
list three addresses as valid:

["5128751e76e8199196d454941c45d1b3a323f1433bd6751e76e8199196d454941c45d1b3a323f1433bd6",
 "bc1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7k7grplx"],
["6002751e", "BC1SW50QA3JX3S"],
["5210751e76e8199196d454941c45d1b3a323", "bc1zw508d6qejxtdg4y5r3zarvaryvg6kdaj"]

Those are witness versions 1, 16 and 2 carrying a bech32 checksum. They are
the BIP173 spellings, and
BIP350
replaced them: witness version 1 and above take bech32m. Core implemented
BIP350 in v22.0 (2021). The current spellings are

bc1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7kt5nd6y
BC1SW50QGDZ25J
bc1zw508d6qejxtdg4y5r3zarvaryvaxxpcs

So the file asserts as valid three strings that no BIP350 implementation will
accept, and asserts an encoding of witness v1+ programs that no wallet should
now produce.

bech32_invalid.json shows the same seam from the other side. All ten are
correctly refused, but four are refused for a reason other than the one the
file names, because a bech32m reader answers the checksum before it reaches
the length or the padding:

vector	reason in the file	reason under BIP350
BC13W508D6QEJXTDG4Y5R3ZARVARY0C5XW7KN40WF2	Invalid witness version	invalid checksum
bc1rw5uspcuh	Invalid program length	invalid checksum
bc10w508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7kw5rljs90	Invalid program length	invalid checksum
bc1zw508d6qejxtdg4y5r3zarvaryvqyzf3du	zero padding of more than 4 bits	invalid checksum

2. CPubKey(x('00')).is_fullyvalid is True

test_key.py:27,
with the question already in the file:

T('00', True, True, False) # why is this valid?

A single 0x00 byte is not a public key: it is neither 33 bytes with a 02/03
prefix nor 65 with 04, and Core's CPubKey::IsFullyValid() refuses it on
size() before it ever calls into libsecp256k1. The neighbouring rows agree
with Core — '01' and '02' are is_fullyvalid=False — so the '00' row is
also inconsistent with its own siblings. Answering the comment: it should not be
valid.

3. VarIntSerializer.deserialize accepts non-canonical encodings

test_serialize.py:59-69,
Test_VarIntSerializer.test_non_optimal, asserts that

T(b'fd0000', 0)
T(b'fe00000000', 0)
T(b'ff0000000000000000', 0)

all decode to zero. Core refuses all three: ReadCompactSize() raises
"non-canonical ReadCompactSize()" when a value is encoded in more bytes than
it needs, and feature_block.py has a block-level vector for it. It matters
beyond tidiness — the same transaction with a non-canonical input count is a
second serialization of the same transaction, hence a second txid, so accepting
it on parse is accepting a malleated form.

The other three rows of the same test (fd3412, fe67452301, ffefcdab8967452301)
are canonical for their prefix and are fine; it is the three zeros that pin the
wrong behaviour.

4. checkblock_invalid.json's "Duplicate transaction" is a merkle-root mismatch

The vector at
line 27
carries block 170's header over transactions [coinbase, A, A]. Three leaves
pad to [coinbase, A, A, A], whose root is not the root of [coinbase, A]
that the header commits to — so the block is already refused by its merkle
root, independently of the duplicate.

CheckBlock reaches the duplicate-txid loop before it computes the root, so the
vector passes for the reason the comment gives; Core computes the root first and
answers bad-txnmrklroot. The consequence is that the vector cannot exercise
the CVE-2012-2459 mutation it appears to be about: a vector for that needs the
duplicated pair to be the padding the tree already implies, so that the
mutated block keeps the original root.

---

Points 1-3 are behaviour we would change; point 4 is a vector that could stay as
it is with its comment corrected. All four were verified by running the strings
and bytes above through an independent implementation and against Core's current
sources.