From 21c7fd60c1f87f57e74772ebe52e2dba01c470b7 Mon Sep 17 00:00:00 2001 From: Ferdinando Ametrano Date: Thu, 13 Aug 2026 18:54:25 +0200 Subject: [PATCH 1/3] Check every width a sig_hash preimage writes, and every index `int.to_bytes` answers a field too wide for it with an OverflowError, an ArithmeticError that no `except BTClibValueError` catches; a list index out of range is an IndexError, a LookupError, outside it too. #724 made the version and lock-time checks unconditional in `Tx.serialize`, which closed those two fields for `legacy` -- the one sig_hash routed through it. `segwit_v0` and `taproot` assemble their preimage from their own `to_bytes` calls and never reach it, and `TxIn.serialize` and `TxOut.serialize` check nothing when told not to, so the same leak survived on the sequence, on the output value and on the outpoint's vout. The checks go in the serializations rather than in the callers -- `_serialized_4_byte_field`, `_serialized_camount`, `_serialized_out_point`, `_serialized_output`, `_serialized_spend_type` -- so `PrecomputedTxData` and both `sha_`/`hash_` families get them for free. `legacy` checks what its branches leave behind, NONE dropping the outputs it does not commit to. `vin_i` is bounded in `legacy`, `segwit_v0`, `from_tx` and `taproot_annex_and_ext` as it is in `taproot`; `from_tx` also refuses a prevout list of a different length from the vin, with the message `PrecomputedTxData` gives for the same mismatch. `ext_flag` is seven bits, and `message_extension` takes Octets like every other octets parameter here. `_assert_valid_4_byte_field` is imported from `tx.tx` rather than written a second time, and `_serialized_spend_type` is an extraction and not only a check: `taproot` was one branch under C901's ten. The third slice of #744. --- CHANGELOG.md | 38 +++++++ btclib/script/sig_hash.py | 142 +++++++++++++++++++------ tests/script/sig_hash_legacy_test.py | 59 +++++++++- tests/script/sig_hash_segwitv0_test.py | 55 ++++++++++ tests/script/sig_hash_taproot_test.py | 129 +++++++++++++++++++++- 5 files changed, 390 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d9daa5d21..b716992c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1740,6 +1740,44 @@ documented at release-notes length in the first place, and are still in ### The public API and the module layout +- **Every field a sig_hash preimage writes is checked as a width, and + every `vin_i` as an index** (issue #744). `int.to_bytes` answers a + field too wide for it with an `OverflowError`, which is an + `ArithmeticError` and so outside the `except BTClibValueError` this + library invites; a list index out of range is an `IndexError`, a + `LookupError`, and outside it too. + + #724 made the version and lock-time checks unconditional in + `Tx.serialize`, which closed those two fields for `legacy` -- the one + sig_hash routed through it. `segwit_v0` and `taproot` assemble their + preimage from their own `to_bytes` calls and never reach it, and + `TxIn.serialize` and `TxOut.serialize` check nothing when told not to, + so the same leak survived on the sequence, on the output value and on + the outpoint's vout. + + The checks now live in the serializations rather than in the callers: + `_serialized_4_byte_field`, `_serialized_camount`, + `_serialized_out_point` and `_serialized_output` are the writes with + the check in front, so `PrecomputedTxData` and the two `sha_`/`hash_` + families get them for free and no future caller has to remember. + `legacy` checks what is left after its branches -- NONE drops the + outputs, so a value no CAmount can hold is refused by the hash types + that commit to it and hashed by the one that does not. + + `vin_i` is bounded in `legacy`, `segwit_v0`, `from_tx` and + `taproot_annex_and_ext` as it is in `taproot`, and a non-integer is a + `BTClibTypeError`. `from_tx` also refuses a prevout list of a + different length from the vin, which `PrecomputedTxData` refuses with + the same message and `script_engine.verify_transaction` before it: a + short list hashes one transaction's amounts into another's sig_hash. + + Two more of `taproot`'s parameters: `ext_flag` is BIP341's spend-type + byte less the annex bit, so seven bits and no wider, where + `to_bytes(1)` answered an `OverflowError`; and `message_extension` + takes `Octets` like every other octets parameter of this library, + being concatenated raw where the annex goes through `var_bytes` -- a + hex string met `b"".join` and answered a `TypeError` about the join. + - **Twelve places answered a malformed argument instead of refusing it** (issue #744): no exception of the wrong class, no exception at all -- a hash, an address, an entropy, a weight, a residue, handed back for an diff --git a/btclib/script/sig_hash.py b/btclib/script/sig_hash.py index 1297e1720..e6730cd1f 100644 --- a/btclib/script/sig_hash.py +++ b/btclib/script/sig_hash.py @@ -36,7 +36,13 @@ is_p2wsh, type_and_payload, ) -from btclib.tx import Tx, TxIn, TxOut +from btclib.tx import OutPoint, Tx, TxIn, TxOut + +# the width check `Tx.serialize` runs on its own two fields regardless of +# check_validity, imported rather than written again: it is the same +# field width, and a second spelling of it would be a second place to +# read before believing the two agree +from btclib.tx.tx import _assert_valid_4_byte_field from btclib.utils import bytes_from_octets, is_integer __all__ = [ @@ -195,6 +201,8 @@ def taproot_annex_and_ext(tx: Tx, vin_i: int) -> tuple[bytes, bytes]: A signer past a separator computes its own extension; the caller's transaction is never rewritten. """ + _assert_valid_vin_i(tx, vin_i) + # a local name, never assigned back: computing a hash must not rewrite # the caller's Tx, and the annex is dropped by rebinding it below stack = tx.vin[vin_i].script_witness.stack @@ -297,6 +305,59 @@ def _assert_valid_camount(amount: int, name: str) -> None: raise BTClibValueError(f"invalid {name}: {amount}") +# the two field widths every preimage here is made of, each as the write +# that needs the check rather than as a check a writer has to remember: +# `int.to_bytes` answers a field too wide for it with an OverflowError, +# an ArithmeticError that no `except BTClibValueError` written against +# this library catches (issue #690). `Tx.serialize` checks its own two +# the same way and unconditionally, which is what closed them for +# `legacy` alone -- segwit_v0 and taproot assemble their preimage from +# their own calls and never reach it +def _serialized_4_byte_field(name: str, value: int) -> bytes: + _assert_valid_4_byte_field(name, value) + return value.to_bytes(4, byteorder="little", signed=False) + + +def _serialized_camount(amount: int, name: str) -> bytes: + # signed, as TxOut.serialize is and for the same reason: this is the + # same CAmount field, Core's `ss << txout.nValue`, so the two must + # agree on which integers the eight bytes stand for (issue #388) + _assert_valid_camount(amount, name) + return amount.to_bytes(8, byteorder="little", signed=True) + + +def _serialized_out_point(out_point: OutPoint) -> bytes: + """Serialize an outpoint, its 4-byte vout checked as a width. + + `check_validity=False` throughout, as everywhere in this module: a + preimage is computed over transactions the caller has not asked to + be judged whole. The width is the one part of that judgement + serializing four bytes cannot skip. + """ + _assert_valid_4_byte_field("vout", out_point.vout) + return out_point.serialize(check_validity=False) + + +def _serialized_output(tx_out: TxOut) -> bytes: + """Serialize an output, its CAmount checked as a width.""" + _assert_valid_camount(tx_out.value, "output value") + return tx_out.serialize(check_validity=False) + + +def _serialized_spend_type(ext_flag: int, annex_present: int) -> bytes: + """Return BIP341's spend type: `2 * ext_flag + annex_present`. + + One byte, so the extension flag is seven bits: 0 for a key path, 1 + for BIP342's tapscript, and nothing wider has anywhere to go -- + where `to_bytes(1)` answered an OverflowError. + """ + if not is_integer(ext_flag): + raise BTClibTypeError(f"invalid extension flag type: {type(ext_flag).__name__}") + if not 0 <= ext_flag <= 0x7F: + raise BTClibValueError(f"invalid extension flag: {ext_flag}") + return (2 * ext_flag + annex_present).to_bytes(1, "little") + + def _assert_valid_prevouts(prevouts: list[TxOut]) -> None: """Ask every prevout what a preimage committing to all of them needs. @@ -347,6 +408,7 @@ def legacy(script_code: Octets, tx: Tx, vin_i: int, hash_type: int) -> bytes: # SINGLE bug's early return, which answers with the constant 1 and # never reaches the serialization at the end serialized_hash_type = _serialized_hash_type(hash_type) + _assert_valid_vin_i(tx, vin_i) # the legacy preimage commits to the script code with its # OP_CODESEPARATORs elided, and Core does that here rather than to the @@ -379,6 +441,17 @@ def legacy(script_code: Octets, tx: Tx, vin_i: int, hash_type: int) -> bytes: if hash_type & 0x80: new_tx.vin = [new_tx.vin[vin_i]] + # the widths of what is left, and only of what is left: the copy above + # is what `Tx.serialize` is about to write, and NONE and SINGLE have + # already dropped the outputs they do not commit to. `Tx.serialize` + # checks the version and the lock time itself, unconditionally, and + # these are the fields it hands to TxIn and TxOut, which do not + for txin in new_tx.vin: + _assert_valid_4_byte_field("sequence", txin.sequence) + _assert_valid_4_byte_field("vout", txin.prev_out.vout) + for txout in new_tx.vout: + _assert_valid_camount(txout.value, "output value") + preimage = new_tx.serialize(include_witness=False, check_validity=False) preimage += serialized_hash_type @@ -392,28 +465,22 @@ def legacy(script_code: Octets, tx: Tx, vin_i: int, hash_type: int) -> bytes: # to be checked against each other. Private, PrecomputedTxData below being # the supported way to compute them once for a whole transaction def _serialized_prevouts(tx: Tx) -> bytes: - return b"".join([vin.prev_out.serialize(check_validity=False) for vin in tx.vin]) + return b"".join([_serialized_out_point(vin.prev_out) for vin in tx.vin]) def _serialized_sequences(tx: Tx) -> bytes: return b"".join( - [vin.sequence.to_bytes(4, byteorder="little", signed=False) for vin in tx.vin] + [_serialized_4_byte_field("sequence", vin.sequence) for vin in tx.vin] ) def _serialized_outputs(tx: Tx) -> bytes: - return b"".join([vout.serialize(check_validity=False) for vout in tx.vout]) + return b"".join([_serialized_output(vout) for vout in tx.vout]) def _serialized_amounts(prevouts: list[TxOut]) -> bytes: - # signed, as TxOut.serialize is and for the same reason: this is the - # same CAmount field, Core's `ss << txout.nValue`, so the two must - # agree on which integers the eight bytes stand for (issue #388) return b"".join( - [ - prevout.value.to_bytes(8, byteorder="little", signed=True) - for prevout in prevouts - ] + [_serialized_camount(prevout.value, "spent amount") for prevout in prevouts] ) @@ -516,6 +583,7 @@ def segwit_v0( # bytes can stand for is what leaked an OverflowError out of the # serialization, where the contract promises a BTClibValueError _assert_valid_camount(amount, "amount") + _assert_valid_vin_i(tx, vin_i) script_code = bytes_from_octets(script_code) @@ -554,22 +622,21 @@ def segwit_v0( elif (hash_type & 0x1F) == SINGLE and vin_i < len(tx.vout): # this one commits to the signed output alone, so it is per input # by definition and no precomputation can serve it - hash_outputs = hash256(tx.vout[vin_i].serialize(check_validity=False)) + hash_outputs = hash256(_serialized_output(tx.vout[vin_i])) preimage = b"".join( [ - tx.version.to_bytes(4, byteorder="little", signed=False), + _serialized_4_byte_field("version", tx.version), hash_prev_outs, hash_seqs, - tx.vin[vin_i].prev_out.serialize(check_validity=False), + _serialized_out_point(tx.vin[vin_i].prev_out), var_bytes.serialize(script_code), - # a CAmount, signed as TxOut's is: BIP143's `amount` is the - # spent output's value, and Core writes it with the same - # serializer (issue #388) - amount.to_bytes(8, byteorder="little", signed=True), # value - tx.vin[vin_i].sequence.to_bytes(4, byteorder="little", signed=False), + # BIP143's `amount` is the spent output's value, and Core + # writes it with the same serializer TxOut uses (issue #388) + _serialized_camount(amount, "amount"), + _serialized_4_byte_field("sequence", tx.vin[vin_i].sequence), hash_outputs, - tx.lock_time.to_bytes(4, byteorder="little", signed=False), + _serialized_4_byte_field("lock time", tx.lock_time), # an int32_t as Core's nHashType is, so that -1 is the # `ffffffff` Core writes rather than an OverflowError (#405) _serialized_hash_type(hash_type), @@ -584,8 +651,8 @@ def taproot( prevouts: list[TxOut], hashtype: int, ext_flag: int, - annex: bytes, - message_extension: bytes, + annex: Octets, + message_extension: Octets, precomputed: PrecomputedTxData | None = None, ) -> bytes: """Return the BIP341 hash one taproot input's signature commits to. @@ -605,6 +672,10 @@ def taproot( if hashtype & 0x03 == SINGLE and input_index >= len(transaction.vout): raise BTClibValueError("Sighash single without a corresponding output") + # the message extension is concatenated raw, so it is the one octets + # parameter here that no serializer coerces on its way in + message_extension = bytes_from_octets(message_extension) + anyone_can_pay = hashtype & 0x80 == ANYONECANPAY all_outputs = hashtype & 0x03 not in {NONE, SINGLE} annex_present = int(bool(annex)) @@ -614,8 +685,8 @@ def taproot( parts = [ b"\x00", hashtype.to_bytes(1, "little"), - transaction.nVersion.to_bytes(4, "little"), - transaction.nLockTime.to_bytes(4, "little"), + _serialized_4_byte_field("version", transaction.nVersion), + _serialized_4_byte_field("lock time", transaction.nLockTime), ] # the transaction-wide hashes, and only if this hash type commits to @@ -635,7 +706,7 @@ def taproot( if all_outputs: parts.append(precomputed.sha_outputs) - parts.append((2 * ext_flag + annex_present).to_bytes(1, "little")) + parts.append(_serialized_spend_type(ext_flag, annex_present)) if anyone_can_pay: # check_validity=False, as segwit_v0 and the rest of the library do @@ -643,10 +714,12 @@ def taproot( # the transaction once per input is the same waste in miniature prevout = prevouts[input_index] parts += [ - transaction.vin[input_index].prev_out.serialize(check_validity=False), - prevout.value.to_bytes(8, "little", signed=True), # a CAmount + _serialized_out_point(transaction.vin[input_index].prev_out), + _serialized_camount(prevout.value, "spent amount"), var_bytes.serialize(prevout.script_pub_key.script), - transaction.vin[input_index].nSequence.to_bytes(4, "little"), + _serialized_4_byte_field( + "sequence", transaction.vin[input_index].nSequence + ), ] else: parts.append(input_index.to_bytes(4, "little")) @@ -655,9 +728,7 @@ def taproot( parts.append(sha256(var_bytes.serialize(annex))) if hashtype & 0x03 == SINGLE: - parts.append( - sha256(transaction.vout[input_index].serialize(check_validity=False)) - ) + parts.append(sha256(_serialized_output(transaction.vout[input_index]))) parts.append(message_extension) @@ -735,6 +806,15 @@ def from_tx( problem — the interpreter advances Core's `pbegincodehash` as it goes. """ _assert_valid_prevouts(prevouts) + _assert_valid_vin_i(tx, vin_i) + # both lists are indexed at vin_i below, and one prevout per input is + # what a segwit preimage commits to: the message PrecomputedTxData + # gives for the same mismatch, and script_engine.verify_transaction + # before it + if len(prevouts) != len(tx.vin): + raise BTClibValueError( + f"{len(prevouts)} prevouts for {len(tx.vin)} transaction inputs" + ) script = prevouts[vin_i].script_pub_key.script diff --git a/tests/script/sig_hash_legacy_test.py b/tests/script/sig_hash_legacy_test.py index a680f79ac..0d1afeb38 100644 --- a/tests/script/sig_hash_legacy_test.py +++ b/tests/script/sig_hash_legacy_test.py @@ -13,7 +13,7 @@ import pytest from btclib.ecc import dsa -from btclib.exceptions import BTClibValueError +from btclib.exceptions import BTClibTypeError, BTClibValueError from btclib.hashes import hash160, hash256 from btclib.script import serialize, sig_hash from btclib.script.engine import verify_transaction @@ -359,3 +359,60 @@ def test_a_version_too_wide_for_its_four_bytes_is_refused_too() -> None: tx.version = 2**32 with pytest.raises(BTClibValueError, match="invalid version: "): sig_hash.legacy(_SCRIPT_CODE, tx, 0, sig_hash.ALL) + + +def test_the_other_two_widths_of_the_copy_are_refused_as_well() -> None: + """#724 closed the version and the lock time; these are the rest. + + `Tx.serialize` checks its own two fields regardless of + `check_validity`, which is what the test above asserts -- and it hands + the sequence to `TxIn.serialize` and the value to `TxOut.serialize`, + neither of which checks anything when told not to. So the same + `OverflowError` survived on a different field, and on the vout of the + outpoint each input names. + + What is left after the branches, and only that: NONE drops every + output, so a value no CAmount can hold is refused for the hash types + that commit to it and hashed by the one that does not. + """ + tx = _two_in_one_out() + tx.vin[0].sequence = 2**32 + with pytest.raises(BTClibValueError, match="invalid sequence: "): + sig_hash.legacy(_SCRIPT_CODE, tx, 0, sig_hash.ALL) + + tx = _two_in_one_out() + # built rather than assigned into, OutPoint being frozen: what + # `check_validity=False` lets past the constructor is what the + # serialization is then handed + bad_out_point = OutPoint(b"\x01" * 32, 2**32, check_validity=False) + tx.vin[0] = TxIn(bad_out_point, b"", 0xFFFFFFFE, check_validity=False) + with pytest.raises(BTClibValueError, match="invalid vout: "): + sig_hash.legacy(_SCRIPT_CODE, tx, 0, sig_hash.ALL) + + tx = _two_in_one_out() + tx.vout[0] = TxOut(2**63, _SCRIPT_CODE, check_validity=False) + with pytest.raises(BTClibValueError, match="invalid output value: "): + sig_hash.legacy(_SCRIPT_CODE, tx, 0, sig_hash.ALL) + # NONE commits to no output at all, so this one is nothing the + # preimage carries and nothing to refuse + assert len(sig_hash.legacy(_SCRIPT_CODE, tx, 0, sig_hash.NONE)) == 32 + + +def test_the_input_index_names_an_input_that_exists() -> None: + """`new_tx.vin[vin_i]` was an IndexError, which is a LookupError. + + So no `except BTClibValueError` written against this library caught + it. A negative index would have blanked and signed the input at the + other end, which the SINGLE bug's early return reaches too: `vin_i >= + len(new_tx.vout)` is False for -5, and the copy then rebuilds the + outputs from `range(-5)`, i.e. from none. + """ + tx = _two_in_one_out() + assert len(sig_hash.legacy(_SCRIPT_CODE, tx, 1, sig_hash.ALL)) == 32 + + for out_of_range in (-1, -5, 2, 99): + with pytest.raises(BTClibValueError, match="invalid input index: "): + sig_hash.legacy(_SCRIPT_CODE, tx, out_of_range, sig_hash.ALL) + for not_an_index in (1.0, "0", True): + with pytest.raises(BTClibTypeError, match="invalid input index type: "): + sig_hash.legacy(_SCRIPT_CODE, tx, not_an_index, sig_hash.ALL) # type: ignore[arg-type] diff --git a/tests/script/sig_hash_segwitv0_test.py b/tests/script/sig_hash_segwitv0_test.py index 5d9ab7da1..d90ed5380 100644 --- a/tests/script/sig_hash_segwitv0_test.py +++ b/tests/script/sig_hash_segwitv0_test.py @@ -409,3 +409,58 @@ def test_an_amount_no_field_can_hold_is_refused_rather_than_overflowing() -> Non prevouts = [TxOut(2**63, tx.vout[0].script_pub_key, check_validity=False)] with pytest.raises(BTClibValueError, match="invalid spent amount: "): sig_hash.from_tx(prevouts, tx, 0, sig_hash.ALL) + + +def test_every_width_the_bip143_preimage_writes_is_checked() -> None: + """This preimage never reaches `Tx.serialize`, so #724 left it alone. + + It is assembled from its own `int.to_bytes` calls, and each of them + answered a field too wide for it with an `OverflowError` -- an + `ArithmeticError`, outside the `except BTClibValueError` this library + invites. The version and the lock time are the two `Tx.serialize` + checks unconditionally for `legacy`; the sequence and the outpoint's + vout are written here and nowhere else. + """ + script_code = bytes.fromhex(_WITNESS_SCRIPT) + + for field, err_msg in ( + ("version", "invalid version: "), + ("lock_time", "invalid lock time: "), + ): + tx = Tx.parse(_BIP143_TX) + setattr(tx, field, 2**32) + with pytest.raises(BTClibValueError, match=err_msg): + sig_hash.segwit_v0(script_code, tx, 1, sig_hash.ALL, _AMOUNT) + + tx = Tx.parse(_BIP143_TX) + tx.vin[1].sequence = 2**32 + with pytest.raises(BTClibValueError, match="invalid sequence: "): + sig_hash.segwit_v0(script_code, tx, 1, sig_hash.ALL, _AMOUNT) + + tx = Tx.parse(_BIP143_TX) + tx.vin[1] = TxIn( + OutPoint(b"\x01" * 32, 2**32, check_validity=False), + b"", + 0xFFFFFFFF, + check_validity=False, + ) + with pytest.raises(BTClibValueError, match="invalid vout: "): + sig_hash.segwit_v0(script_code, tx, 1, sig_hash.ALL, _AMOUNT) + + # the output the SINGLE branch hashes on its own, which no + # precomputation serves and no whole-transaction walk reaches + tx = Tx.parse(_BIP143_TX) + tx.vout[0] = TxOut(2**63, tx.vout[0].script_pub_key, check_validity=False) + with pytest.raises(BTClibValueError, match="invalid output value: "): + sig_hash.segwit_v0(script_code, tx, 0, sig_hash.SINGLE, _AMOUNT) + + +def test_the_segwit_input_index_names_an_input_that_exists() -> None: + """`tx.vin[vin_i]` in the preimage was an IndexError, a LookupError.""" + tx = Tx.parse(_BIP143_TX) + script_code = bytes.fromhex(_WITNESS_SCRIPT) + assert len(sig_hash.segwit_v0(script_code, tx, 1, sig_hash.ALL, _AMOUNT)) == 32 + + for out_of_range in (-1, 2, 99): + with pytest.raises(BTClibValueError, match="invalid input index: "): + sig_hash.segwit_v0(script_code, tx, out_of_range, sig_hash.ALL, _AMOUNT) diff --git a/tests/script/sig_hash_taproot_test.py b/tests/script/sig_hash_taproot_test.py index 3ba7f61ba..338f1ff0a 100644 --- a/tests/script/sig_hash_taproot_test.py +++ b/tests/script/sig_hash_taproot_test.py @@ -21,7 +21,7 @@ import pytest -from btclib.alias import ScriptList, TaprootScriptTree +from btclib.alias import Octets, ScriptList, TaprootScriptTree from btclib.ecc import ssa from btclib.exceptions import ( BTClibRuntimeError, @@ -665,3 +665,130 @@ def sig_hash_of(input_index: int) -> bytes: for not_an_index in (1.0, "0", True): with pytest.raises(BTClibTypeError, match="invalid input index type: "): sig_hash_of(not_an_index) # type: ignore[arg-type] + + +def _two_input_p2tr() -> tuple[Tx, list[TxOut]]: + """Return a two-input p2tr spend and the prevouts it spends.""" + utxo = TxOut( + 100000000, + serialize( + ["OP_1", "cc71eb30d653c0c3163990c47b976f3fb3f37cccdcbedb169a1dfef58bbfbfaf"] + ), + ) + vin = [ + TxIn(OutPoint("01" * 32, 0), "", 1, Witness(["00" * 64])), + TxIn(OutPoint("02" * 32, 1), "", 1, Witness(["00" * 64])), + ] + tx = Tx(vin=vin, vout=[TxOut(100000000, ""), TxOut(1, "")]) + return tx, [utxo, utxo] + + +def test_every_width_the_bip341_preimage_writes_is_checked() -> None: + """SigMsg is assembled here too, and never through `Tx.serialize`. + + The sequence and the outpoint are written only under ANYONECANPAY, + which commits to the signed input alone; without it they reach the + preimage through `sha_sequences` and `sha_prevouts`, so both hash + types are asked. + """ + tx, prevouts = _two_input_p2tr() + + def sig_hash_of(transaction: Tx, hashtype: int) -> bytes: + return sig_hash.taproot(transaction, 0, prevouts, hashtype, 0, b"", b"") + + for field, err_msg in ( + ("version", "invalid version: "), + ("lock_time", "invalid lock time: "), + ): + bad, _ = _two_input_p2tr() + setattr(bad, field, 2**32) + with pytest.raises(BTClibValueError, match=err_msg): + sig_hash_of(bad, sig_hash.ALL) + + for hashtype in (sig_hash.ALL, sig_hash.ALL | sig_hash.ANYONECANPAY): + bad, _ = _two_input_p2tr() + bad.vin[0].sequence = 2**32 + with pytest.raises(BTClibValueError, match="invalid sequence: "): + sig_hash_of(bad, hashtype) + + bad, _ = _two_input_p2tr() + bad.vin[0] = TxIn( + OutPoint(b"\x01" * 32, 2**32, check_validity=False), + b"", + 1, + Witness(["00" * 64]), + check_validity=False, + ) + with pytest.raises(BTClibValueError, match="invalid vout: "): + sig_hash_of(bad, hashtype) + + bad, _ = _two_input_p2tr() + bad.vout[0] = TxOut(2**63, bad.vout[0].script_pub_key, check_validity=False) + with pytest.raises(BTClibValueError, match="invalid output value: "): + sig_hash_of(bad, sig_hash.SINGLE) + + assert len(sig_hash_of(tx, sig_hash.ALL)) == 32 + + +def test_the_spend_type_byte_holds_seven_bits_of_extension_flag() -> None: + """`(2 * ext_flag + annex_present).to_bytes(1)` was an OverflowError. + + BIP341's spend type is one byte, so the flag is 0 for a key path, 1 + for BIP342's tapscript, and nothing wider has anywhere to go. + """ + tx, prevouts = _two_input_p2tr() + + def sig_hash_with(ext_flag: int) -> bytes: + return sig_hash.taproot(tx, 0, prevouts, sig_hash.ALL, ext_flag, b"", b"") + + # the boundary itself, not only one past it + assert len(sig_hash_with(0x7F)) == 32 + for out_of_range in (-1, 0x80, 2**40): + with pytest.raises(BTClibValueError, match="invalid extension flag: "): + sig_hash_with(out_of_range) + for not_a_flag in (1.0, "0", True): + with pytest.raises(BTClibTypeError, match="invalid extension flag type: "): + sig_hash_with(not_a_flag) # type: ignore[arg-type] + + +def test_the_message_extension_is_octets_like_every_other_field() -> None: + """It is concatenated raw, where the annex goes through var_bytes. + + So a hex string -- which every other octets parameter of this library + takes -- met `b"".join` and answered `TypeError: sequence item 11`, + from underneath the library and about the join rather than about the + argument. + """ + tx, prevouts = _two_input_p2tr() + ext = b"\x01" * 37 + + def sig_hash_with(message_extension: Octets) -> bytes: + return sig_hash.taproot( + tx, 0, prevouts, sig_hash.ALL, 1, b"", message_extension + ) + + assert sig_hash_with(ext) == sig_hash_with(ext.hex()) + with pytest.raises(ValueError, match="fromhex"): + sig_hash_with("not hex at all") + + +def test_from_tx_names_an_input_both_lists_have() -> None: + """`prevouts[vin_i]` and `tx.vin[vin_i]` were two unchecked indexes. + + And nothing asked whether the two lists were of one length, which + `PrecomputedTxData` does refuse and `verify_transaction` before it: a + prevout list shorter than the vin hashes the amounts of one + transaction into the sig_hash of another. + """ + tx, prevouts = _two_input_p2tr() + assert len(sig_hash.from_tx(prevouts, tx, 1, sig_hash.ALL)) == 32 + + for out_of_range in (-1, 2, 99): + with pytest.raises(BTClibValueError, match="invalid input index: "): + sig_hash.from_tx(prevouts, tx, out_of_range, sig_hash.ALL) + with pytest.raises(BTClibValueError, match="invalid input index: "): + sig_hash.taproot_annex_and_ext(tx, out_of_range) + + err_msg = "1 prevouts for 2 transaction inputs" + with pytest.raises(BTClibValueError, match=err_msg): + sig_hash.from_tx(prevouts[:1], tx, 0, sig_hash.ALL) From 3494e1c33d3c5c8828710e7dd5c2effe86f02f31 Mon Sep 17 00:00:00 2001 From: Ferdinando Ametrano Date: Thu, 13 Aug 2026 21:39:09 +0200 Subject: [PATCH 2/3] Ask a value what it is before asking what it holds (#765) * Ask a value what it is before asking what it holds Ten places in `block/`, `hashes` and `utils` compared, added to or read an attribute off an argument nothing had checked. `"5" <= 16`, `"2015" + 1` and `"hard" <= 0` are bare TypeErrors about operands, raised from underneath the library and naming neither the parameter nor the function; `.tzinfo` on a str is an AttributeError, outside *both* halves of this library's exception contract, so nothing a caller is told to catch would have caught it; and two datetimes given as unix timestamps subtract to an int, whose missing `total_seconds` is an AttributeError again. The guard is `var_int.serialize`'s and the vocabulary `is_integer`'s: a bool is not a number, it being the height one, the block count one, the leaf index one. `bip34_commitment`, and `Block.assert_valid_coinbase_height` through it; `BlockHeader.assert_valid`'s timestamp and `assert_valid_time`'s `now`; `mining.mine`'s max_tries and its header, which `dataclasses.replace` used to complain about; `next_bits`'s two datetimes; `retarget_first_height`; all three numbers of `hash_rate`, where a difficulty and a timespan are float and an integer is one of those; `hashes.merkle_root_from_branch`'s leaf index, which is what `merkle_proof.assert_as_valid` and `merkle_proof.verify` reach it through; and `utils.encode_num`. `BlockHeader._assert_valid_types` is an extraction and not only a check: `assert_valid` was one branch under C901's ten. The bool half of the eight new integer guards goes in `tests/integer_policy_test.py`, where the other twenty-five already are. The fourth slice of #744. * Make `bytes_from_octets` the whole of what Octets means (#766) It is the coercion every `Octets` parameter of the library runs through, and it had two holes. A hex string that is not one left through `bytes.fromhex`'s bare ValueError -- the class the contract promises, with nothing saying it came from here. And anything that was not a `str` went through *untouched*, so `len` of a tuple of 33 ints was 33, which is how `taproot.assert_valid_control_block` accepted one as a control block size and how `bin_str_entropy_from_entropy(())` was reported as zero bits. Both are refused now. Every buffer is still taken, and returned as it came: a read must not rewrite the field it reads, which is what `bytes()` here would do to a bytearray a caller built. The message is `bytes.fromhex`'s own, which names a position and never the string: an Octets parameter is candidate key material as often as not, and `to_prv_key` puts this very message inside its own "not a private key" (issue #137). `to_prv_key`'s two "it must be octets" fallbacks catch a TypeError beside the ValueError, as `to_pub_key` already did. `mnemonic.entropy`'s five `int(x, 2)` parses and its `int(x, 16)` and `int(x)` ones are BTClibValueError too, and say a base rather than the digits. `borromean.sign` checks that its rings, signing indexes and nonces are of one length rather than leaving it to `zip(strict=True)`, whose message named "argument 3" and no parameter of the function; strict=True stays, as the assertion that the check and the loops cannot drift apart. Six test files stop asserting the bare class, `docs/source/guide.rst` shows the new spelling, and HISTORY.md carries the breaking-changes bullet for this and for the three slices before it. The last slice of #744. --- CHANGELOG.md | 63 +++++++++++++++++++++++++++ HISTORY.md | 23 ++++++++++ btclib/block/block.py | 15 ++++++- btclib/block/block_header.py | 45 ++++++++++++++----- btclib/block/mining.py | 11 ++++- btclib/block/proof_of_work.py | 32 +++++++++++++- btclib/ecc/borromean.py | 23 ++++++---- btclib/hashes.py | 7 ++- btclib/mnemonic/entropy.py | 44 +++++++++++++++---- btclib/to_prv_key.py | 7 ++- btclib/utils.py | 43 +++++++++++++++--- docs/source/guide.rst | 2 +- tests/block/block_context_test.py | 7 +++ tests/block/block_test.py | 10 +++++ tests/block/mining_test.py | 21 ++++++++- tests/block/proof_of_work_test.py | 27 +++++++++++- tests/ecc/borromean_test.py | 35 ++++++++++++++- tests/ecc/der_test.py | 4 +- tests/fetch/fetcher_test.py | 2 +- tests/integer_policy_test.py | 26 ++++++++++- tests/mnemonic/entropy_test.py | 32 ++++++++++++-- tests/script/sig_hash_taproot_test.py | 2 +- tests/script/taproot_test.py | 10 +++-- tests/utils_test.py | 49 +++++++++++++++++++-- 24 files changed, 479 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b716992c7..bf5ff337b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1740,6 +1740,69 @@ documented at release-notes length in the first place, and are still in ### The public API and the module layout +- **`bytes_from_octets` is the whole of what `Octets` means, and it is + total** (issue #744). It is the coercion every `Octets` parameter of + the library runs through, and it had two holes: a hex string that is + not one left through `bytes.fromhex`'s bare `ValueError`, and anything + that was not a `str` went through *untouched* -- so `len` of a tuple + of 33 ints was 33, which is how + `taproot.assert_valid_control_block` accepted one as a control block + size. Both are refused now, as `BTClibValueError` and + `BTClibTypeError`; every buffer is still taken, and returned as it + came, a read being no place to rewrite the field it reads. + + The message is `bytes.fromhex`'s own, which names a position and never + the string. That is deliberate: an `Octets` parameter is candidate key + material as often as not, and `to_prv_key` puts this very message + inside its own "not a private key" (issue #137). + + `int_from_integer` gets both through it, and `to_prv_key`'s two "it + must be octets" fallbacks now catch a `TypeError` beside the + `ValueError`, as `to_pub_key` already did: what is neither octets nor + a spelling of them means the same thing a wrong size does. + + `mnemonic.entropy`'s five `int(x, 2)` parses and its two `int(x, 16)` + and `int(x)` ones are `BTClibValueError` too, and say a base rather + than the digits -- raw entropy being seed material, as every other + message in that module already assumed. `borromean.sign` checks that + its rings, signing indexes and nonces are of one length rather than + leaving it to `zip(strict=True)`, whose message named "argument 3" and + no parameter of the function; `strict=True` stays, as the assertion + that the check and the loops cannot drift apart. + + **What moves for a caller**: the class is narrower and the control + flow identical, `BTClibValueError` being a `ValueError` and + `BTClibTypeError` a `TypeError`. A test or a caller matching on + `bytes.fromhex`'s message still matches -- it is carried through -- + and one matching the *class* now has a btclib one to match. + `docs/source/guide.rst` shows the new spelling. + +- **Ten places in `block/`, `hashes` and `utils` asked a value what it + is not** (issue #744): a comparison, an arithmetic operation or an + attribute lookup on an argument nothing had checked. `"5" <= 16`, + `"2015" + 1` and `"hard" <= 0` are bare `TypeError`s about operands, + raised from underneath the library and naming neither the parameter + nor the function; `.tzinfo` on a str is an `AttributeError`, which is + outside *both* halves of this library's exception contract, so nothing + a caller is told to catch would have caught it; and two datetimes + given as unix timestamps subtracted to an int, whose missing + `total_seconds` is an `AttributeError` again. + + The guard is `var_int.serialize`'s, and the vocabulary is + `utils.is_integer`'s: a bool is not a number, because it would be the + height one, the block count one, the leaf index one. + + `bip34_commitment` and, through it, `Block.assert_valid_coinbase_height`; + `BlockHeader.assert_valid`'s timestamp and `assert_valid_time`'s `now` + -- the default `check_validity=True` path being as exposed as the + explicit one; `mining.mine`'s `max_tries` *and* its header, which + `dataclasses.replace` used to complain about; `next_bits`'s two + datetimes; `retarget_first_height`; all three numbers of `hash_rate`, + where a difficulty and a timespan are `float` and an integer is one of + those; `hashes.merkle_root_from_branch`'s leaf index, which is what + `merkle_proof.assert_as_valid` and `merkle_proof.verify` reach it + through; and `utils.encode_num`. + - **Every field a sig_hash preimage writes is checked as a width, and every `vin_i` as an index** (issue #744). `int.to_bytes` answers a field too wide for it with an `OverflowError`, which is an diff --git a/HISTORY.md b/HISTORY.md index 5dc770d11..ee9edab31 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -33,6 +33,29 @@ full year, short month, short day (YYYY-M-D) spaces around it -- `"MAINNET"` to `b32.address_from_witness` -- is accepted now where it used to raise. +- **a malformed argument raises a btclib error, not a native one.** The + rest of issue #744's census, and the same move the network name above + made: an out-of-range derivation index was an `OverflowError`, an + out-of-range `vin_i` an `IndexError`, a header timestamp that is no + datetime an `AttributeError`, a hex string that is not one a bare + `ValueError`. Each is a `BTClibValueError` or a `BTClibTypeError` now. + Code catching `ValueError` or `TypeError` keeps working, the two + deriving from those; code catching `OverflowError`, `IndexError` or + `AttributeError` around one of these has to catch `BTClibException`, + or one of the two builtins, instead. + + Two of them are not a narrowing, and are what to act on. What is + neither `bytes` nor a hex `str` is a `BTClibTypeError` where + `bytes_from_octets` used to return it untouched -- a tuple of 33 ints + passed `taproot.assert_valid_control_block` as a control block size, + and `bin_str_entropy_from_entropy(())` was reported as zero bits. And + a dozen calls that answered a malformed argument with a *number* + refuse it: `sig_hash.taproot` with an `input_index` past the end of + the vin, `taproot.input_script_sig` with a negative leaf index, + `bech32.encode` with a negative digit, `mod_inv` with a float, + `int_from_json_number` with 1.5, `Psbt.weight_estimate` on an + incoherent psbt. CHANGELOG.md lists all twelve. + - **the individual point multiplications are private.** `from btclib.curves.curve_group import mult_jac` is an `ImportError` now, and so is every other variant of `curve_group` and `curve_group_2`: the diff --git a/btclib/block/block.py b/btclib/block/block.py index 1f09aed7e..8bcbfb13c 100644 --- a/btclib/block/block.py +++ b/btclib/block/block.py @@ -22,7 +22,7 @@ WITNESS_SCALE_FACTOR, ) from btclib.block.proof_of_work import MAINNET_POW_LIMIT_BITS -from btclib.exceptions import BTClibValueError +from btclib.exceptions import BTClibTypeError, BTClibValueError from btclib.hashes import ( hash256, merkle_root_and_mutated, @@ -31,7 +31,12 @@ from btclib.script.script import op_int from btclib.script.script import serialize as serialize_script from btclib.tx import Tx -from btclib.utils import assert_no_trailing, bytesio_from_binarydata, decode_num +from btclib.utils import ( + assert_no_trailing, + bytesio_from_binarydata, + decode_num, + is_integer, +) __all__ = [ "Block", @@ -86,6 +91,12 @@ def bip34_commitment(height: int) -> bytes: binds first and every node on such a chain compares against the op code. """ + # before the comparison below, which is what a non-number reaches: + # `-1 <= "5"` is a bare TypeError about the operands rather than + # about the height, and `True` is the height one + if not is_integer(height): + raise BTClibTypeError(f"invalid height type: {type(height).__name__}") + # Core's push_int64 has three branches -- OP_0 for zero, OP_1..OP_16 # for 1 to 16, OP_1NEGATE for -1 -- and op_int names all three command: Command = op_int(height) if -1 <= height <= 16 else height diff --git a/btclib/block/block_header.py b/btclib/block/block_header.py index 1fbf35343..a19d3afd6 100644 --- a/btclib/block/block_header.py +++ b/btclib/block/block_header.py @@ -283,6 +283,11 @@ def assert_valid_time(self, now: datetime) -> None: one, and it stays out of reach: it is the median time past of eleven ancestors, i.e. the chain. """ + # the type before the time zone: `.tzinfo` on anything else is an + # AttributeError, which is neither a ValueError nor a TypeError + # and so is caught by nothing this library tells a caller to catch + if not isinstance(now, datetime): + raise BTClibTypeError(f"invalid current time type: {type(now).__name__}") if now.tzinfo is None or now.tzinfo.utcoffset(now) is None: raise BTClibValueError(f"naive current time (no time zone): {now}") @@ -296,6 +301,35 @@ def assert_valid_time(self, now: datetime) -> None: err_msg += f" > {now} + {MAX_FUTURE_BLOCK_TIME} seconds" raise BTClibValueError(err_msg) + def _assert_valid_types(self) -> None: + """Refuse a field the eighty bytes could not be built from. + + The type check the bytes fields get from bytes() below, for the + three that get none. Not a coercion, which would repair the + mistake by rewriting the header being inspected (__init__ + coerces, this reports); not dropped either, which would let a + float reach to_bytes and leave the library through an + AttributeError, and a str reach `.tzinfo` and leave through + another one -- neither of them a half of this library's + exception contract. + + The timestamp is the one field __init__ coerces nothing into: a + moment has no single spelling to coerce from, + `datetime.fromtimestamp` needing a time zone the caller has not + given. + """ + for key in ("version", "nonce"): + value = getattr(self, key) + if not is_integer(value): + err_msg = f"invalid {key} type: {type(value).__name__}" + raise BTClibTypeError(err_msg) + + if not isinstance(self.time, datetime): + # unreachable to mypy, the field being annotated: the caller + # this is here for is the one mypy never sees + err_msg = f"invalid timestamp type: {type(self.time).__name__}" # type: ignore[unreachable] + raise BTClibTypeError(err_msg) + def assert_valid(self) -> None: """Refuse a header the eighty bytes could not hold. @@ -305,16 +339,7 @@ def assert_valid(self) -> None: are assert_valid_time and assert_valid_pow, whose docstrings say why they are separate. """ - # the type check the bytes fields get from bytes() below, for the - # two int ones. Not a coercion, which would repair the mistake by - # rewriting the header being inspected (__init__ coerces, this - # reports); not dropped either, which would let a float reach - # to_bytes and leave the library through an AttributeError - for key in ("version", "nonce"): - value = getattr(self, key) - if not is_integer(value): - err_msg = f"invalid {key} type: {type(value).__name__}" - raise BTClibTypeError(err_msg) + self._assert_valid_types() # must be a 4-bytes _signed_ integer if not 0 < self.version <= 0x7FFFFFFF: diff --git a/btclib/block/mining.py b/btclib/block/mining.py index 4c00436cf..c32f7cb33 100644 --- a/btclib/block/mining.py +++ b/btclib/block/mining.py @@ -28,8 +28,9 @@ from btclib.alias import Octets from btclib.block.block import merkle_root_and_mutated_from_transactions from btclib.block.block_header import BlockHeader -from btclib.exceptions import BTClibValueError +from btclib.exceptions import BTClibTypeError, BTClibValueError from btclib.tx import Tx +from btclib.utils import is_integer __all__ = [ "NONCE_SPACE", @@ -101,6 +102,14 @@ def mine(header: BlockHeader, max_tries: int = 1 << 20) -> BlockHeader | None: The caller's header is left alone: what comes back is a copy. """ + # `replace()` says "should be called on dataclass instances" for + # anything else, from the standard library and about a call the caller + # never made; `max_tries < 1` is a bare TypeError about the operands, + # and a float passes it to fail at `range` a few lines down + if not isinstance(header, BlockHeader): + raise BTClibTypeError(f"invalid header type: {type(header).__name__}") + if not is_integer(max_tries): + raise BTClibTypeError(f"invalid max_tries type: {type(max_tries).__name__}") if max_tries < 1: raise BTClibValueError(f"invalid max_tries: {max_tries}") diff --git a/btclib/block/proof_of_work.py b/btclib/block/proof_of_work.py index 1d5785228..450eaf535 100644 --- a/btclib/block/proof_of_work.py +++ b/btclib/block/proof_of_work.py @@ -31,8 +31,8 @@ from datetime import datetime from btclib.alias import Octets -from btclib.exceptions import BTClibValueError -from btclib.utils import bytes_from_octets +from btclib.exceptions import BTClibTypeError, BTClibValueError +from btclib.utils import bytes_from_octets, is_integer __all__ = [ "DIFFICULTY_ADJUSTMENT_INTERVAL", @@ -213,6 +213,11 @@ def retarget_first_height(last_height: int) -> int: error, so `GetNextWorkRequired` still reads `nHeight - (DifficultyAdjustmentInterval() - 1)`. """ + # a height, before the arithmetic: `"2015" + 1` is a bare TypeError + # about concatenating a str, and `True + 1` is the height two + if not is_integer(last_height): + raise BTClibTypeError(f"invalid height type: {type(last_height).__name__}") + # Core only retargets when the *next* height is a multiple of 2016, # so the last block of a period is the one 2015 blocks after its # first. Refused rather than answered for any other height: the @@ -251,6 +256,17 @@ def next_bits( Bitcoin Core spells this `CalculateNextWorkRequired`. """ + # both are datetimes, checked before the subtraction: two ints + # subtract to an int and answer `AttributeError: 'int' object has no + # attribute 'total_seconds'`, which is neither half of this library's + # exception contract, and a str answers a TypeError about the operands + for name, value in ( + ("first block time", first_block_time), + ("last block time", last_block_time), + ): + if not isinstance(value, datetime): + raise BTClibTypeError(f"invalid {name} type: {type(value).__name__}") + # the difference of two datetimes, not two timestamp() calls: aware # or naive, the subtraction is the same number of seconds, so the # answer does not depend on the machine's time zone for the naive @@ -344,6 +360,18 @@ def hash_rate(difficulty: float, timespan: float, block_count: int = 1) -> float with to within the 1/65536 by which the genesis target falls short of 2^224. """ + # the types before the three comparisons, each of which is a bare + # TypeError about the operands for anything that is not a number. A + # difficulty and a timespan are `float` here and an integer is one of + # those, `1.0` and `1` being the same rate; a block count is a count, + # so it is the narrower question -- and a bool is neither, `True` + # being one block, one second and difficulty one + for name, value in (("timespan", timespan), ("difficulty", difficulty)): + if isinstance(value, bool) or not isinstance(value, (int, float)): + raise BTClibTypeError(f"invalid {name} type: {type(value).__name__}") + if not is_integer(block_count): + raise BTClibTypeError(f"invalid block count type: {type(block_count).__name__}") + if timespan <= 0: raise BTClibValueError(f"invalid timespan: {timespan}") if block_count < 1: diff --git a/btclib/ecc/borromean.py b/btclib/ecc/borromean.py index ec581a114..9387c36fc 100644 --- a/btclib/ecc/borromean.py +++ b/btclib/ecc/borromean.py @@ -21,7 +21,7 @@ from btclib.alias import HashF, Octets, Point from btclib.curves import Curve, bytes_from_point, double_mult, mult, secp256k1 -from btclib.exceptions import BTClibRuntimeError +from btclib.exceptions import BTClibRuntimeError, BTClibValueError from btclib.utils import bytes_from_octets, int_from_bits __all__ = [ @@ -107,15 +107,20 @@ def sign( for pubk_ring in pubk_rings ] + # one entry per ring in each of the three, checked here rather than + # left to the `strict=True` of the two loops below: a short ks would + # truncate them silently and sign a subset of the rings -- a signature + # over fewer rings than the caller asked for, which is the one thing a + # ring signature must not do quietly. zip's own message is a + # BTClibValueError's class with none of its content, naming "argument + # 3" and no parameter of this function; strict=True stays, as the + # assertion that this check and those loops cannot drift apart + if not len(pubk_rings) == len(sign_key_idx) == len(ks): + err_msg = f"{len(pubk_rings)} rings, {len(sign_key_idx)} signing indexes" + err_msg += f" and {len(ks)} nonces" + raise BTClibValueError(err_msg) + # step 1 - # strict=True, and it is the one zip in this package that changes what - # an argument does rather than documenting an invariant already checked: - # nothing validates that ks, sign_key_idx and pubk_rings have one entry - # per ring, so a short ks would truncate the loop silently and sign a - # subset of the rings -- a signature over fewer rings than the caller - # asked for, which is the one thing a ring signature must not do - # quietly. ValueError, and BTClibValueError is a ValueError, so a caller - # already catching this package's errors catches it for i, (pubk_ring, j_star, k) in enumerate( zip(pubk_rings, sign_key_idx, ks, strict=True) ): diff --git a/btclib/hashes.py b/btclib/hashes.py index 270c28729..04271620d 100644 --- a/btclib/hashes.py +++ b/btclib/hashes.py @@ -18,7 +18,7 @@ from btclib._ripemd160 import ripemd160 as pure_python_ripemd160 from btclib.alias import HashDigestF, HashF, Octets from btclib.exceptions import BTClibTypeError, BTClibValueError -from btclib.utils import bytes_from_octets +from btclib.utils import bytes_from_octets, is_integer __all__ = [ "hash160", @@ -286,6 +286,11 @@ def merkle_root_from_branch( means knowing what a transaction looks like -- a layer this module sits below, and must not import. """ + # the type before the sign, `"0" < 0` being a bare TypeError about + # the operands: an index is a position and a bool is not one, `True` + # naming the second leaf of every tree it is passed to + if not is_integer(index): + raise BTClibTypeError(f"invalid leaf index type: {type(index).__name__}") if index < 0: raise BTClibValueError(f"negative leaf index: {index}") diff --git a/btclib/mnemonic/entropy.py b/btclib/mnemonic/entropy.py index 75aa0e28b..95f6a457e 100644 --- a/btclib/mnemonic/entropy.py +++ b/btclib/mnemonic/entropy.py @@ -68,6 +68,27 @@ def _bits_per_digit(base: int) -> int: return base.bit_length() - 1 +def _int_from_bin_str(entropy: BinStr) -> int: + """Return the number a binary 0/1 string spells, or refuse the string. + + `int(x, 2)` answers what is no binary string with the bare + ValueError "invalid literal for int() with base 2", which names + neither the parameter nor this library, and what is no string at all + with a bare TypeError. + + Neither message carries the value, and neither does this one: raw + entropy is seed material, and every error in this module says a + length or a count and never the digits (issue #137). + """ + try: + return int(entropy, 2) + except TypeError as e: + err_msg = f"invalid entropy type: {type(entropy).__name__}" + raise BTClibTypeError(err_msg) from e + except ValueError as e: + raise BTClibValueError("invalid entropy: not a binary 0/1 string") from e + + def wordlist_indexes_from_bin_str_entropy(entropy: BinStr, base: int) -> list[int]: """Return the digit indexes for the provided raw entropy. @@ -76,7 +97,7 @@ def wordlist_indexes_from_bin_str_entropy(entropy: BinStr, base: int) -> list[in entropy; leading zeros are not considered redundant padding. """ bits = len(entropy) - int_entropy = int(entropy, 2) + int_entropy = _int_from_bin_str(entropy) indexes = [] while int_entropy: int_entropy, index = divmod(int_entropy, base) @@ -194,7 +215,7 @@ def bytes_entropy_from_str(bin_str_entropy: BinStr) -> bytes: err_msg = f"invalid number of bits: {n_bits} instead of {_bits}" raise BTClibValueError(err_msg) nbytes = (n_bits + 7) // 8 - int_entropy = int(bin_str_entropy, 2) + int_entropy = _int_from_bin_str(bin_str_entropy) return int_entropy.to_bytes(nbytes, byteorder="big", signed=False) @@ -215,11 +236,18 @@ def bin_str_entropy_from_int( if isinstance(int_entropy, str): int_entropy = int_entropy.strip().lower() if int_entropy[:2] == "0b": - int_entropy = int(int_entropy, 2) - elif int_entropy[:2] == "0x": - int_entropy = int(int_entropy, 16) + int_entropy = _int_from_bin_str(int_entropy) else: - int_entropy = int(int_entropy) + # the two `int` readings left, and the same bare ValueError + # out of both: "invalid literal for int() with base 16", + # naming neither the parameter nor this library -- and + # carrying the digits, which the message here does not + base = 16 if int_entropy[:2] == "0x" else 10 + try: + int_entropy = int(int_entropy, base) + except ValueError as e: + err_msg = f"invalid entropy: not a base {base} number" + raise BTClibValueError(err_msg) from e if int_entropy < 0: raise BTClibValueError(f"negative entropy: {int_entropy}") @@ -253,7 +281,7 @@ def bin_str_entropy_from_str(str_entropy: str, bits: OneOrMoreInt = _bits) -> Bi Default bit-sizes are 128, 160, 192, 224, 256, or 512 bits. """ - int(str_entropy, 2) + _int_from_bin_str(str_entropy) # if a single int, make it a tuple if isinstance(bits, int): @@ -410,7 +438,7 @@ def bin_str_entropy_from_random( if len(entropy) > bits: # only the leftmost bits are retained entropy = entropy[:bits] - i = int(entropy, 2) + i = _int_from_bin_str(entropy) # XOR the current entropy with CSPRNG system entropy i ^= secrets.randbits(bits) diff --git a/btclib/to_prv_key.py b/btclib/to_prv_key.py index b3fa003f1..beffd53fe 100644 --- a/btclib/to_prv_key.py +++ b/btclib/to_prv_key.py @@ -75,7 +75,10 @@ def int_from_prv_key(prv_key: PrvKey, ec: Curve = secp256k1) -> int: try: prv_key = bytes_from_octets(prv_key, ec.n_size) q = int.from_bytes(prv_key, "big") - except ValueError as e: + # both, as `to_pub_key` catches both here: what is neither octets + # nor a spelling of them is a TypeError, and it means the same + # thing as a wrong size does -- this input is not a private key + except (TypeError, ValueError) as e: # never echo the input: it is candidate key material. What the # reasons carry is why each format rejected it -- a checksum, a # prefix, a size -- none of which is secret @@ -313,7 +316,7 @@ def prv_keyinfo_from_prv_key( try: prv_key = bytes_from_octets(prv_key, ec.n_size) q = int.from_bytes(prv_key, byteorder="big", signed=False) - except ValueError as e: + except (TypeError, ValueError) as e: # never echo the input: it is candidate key material. The # reasons say why each format rejected it, and none of them is # secret diff --git a/btclib/utils.py b/btclib/utils.py index b32f72929..27dff2ddd 100644 --- a/btclib/utils.py +++ b/btclib/utils.py @@ -86,7 +86,29 @@ def bytes_from_octets(octets: Octets, out_size: NoneOneOrMoreInt = None) -> byte and say it had checked a size. """ if isinstance(octets, str): # hex string - octets = bytes.fromhex(octets) + # `bytes.fromhex` raises a bare ValueError -- the same class the + # contract promises, so what was lost is only that it came from + # here. This is the one coercion every `Octets` parameter of the + # library runs through, so it is the one place worth saying it in. + # The message is fromhex's own, which names a position and never + # the string: an Octets parameter is candidate key material as + # often as not, and `to_prv_key` puts this very message inside its + # own "not a private key" (issue #137) + try: + octets = bytes.fromhex(octets) + except ValueError as e: + raise BTClibValueError(f"invalid hex string: {e}") from e + elif not isinstance(octets, (bytes, bytearray, memoryview)): + # what is neither went through untouched and reached whatever the + # caller went on to do with it: `len` of a tuple of 33 ints is 33, + # so `taproot.assert_valid_control_block` accepted one as a + # control block size. + # Every buffer and not `bytes` alone, and returned as it came: + # `assert_valid` is a read and must not rewrite the field it + # reads, which is what `bytes()` here would do to a bytearray a + # caller built (`tests/bip32/bip32_test.py` pins it) + err_msg = f"invalid octets type: {type(octets).__name__}" # type: ignore[unreachable] + raise BTClibTypeError(err_msg) if out_size is None: return octets @@ -290,11 +312,16 @@ def int_from_integer(i: Integer) -> int: if isinstance(i, str): i = i.strip().lower() if i.startswith(("0x", "-0x")): - return int(i, 16) - i = bytes.fromhex(i) + # the same bare ValueError bytes_from_octets names below, out + # of the one spelling that does not reach it + try: + return int(i, 16) + except ValueError as e: + raise BTClibValueError(f"invalid hex integer: {i!r}") from e - # must be bytes - return int.from_bytes(i, "big", signed=False) + # the hex string, and the refusal of what is neither that nor bytes, + # both being bytes_from_octets's to give + return int.from_bytes(bytes_from_octets(i), "big", signed=False) def hex_string(i: Integer) -> str: @@ -391,6 +418,12 @@ def encode_num(i: int) -> bytes: room for its magnitude in eight, which is what Core's `CScriptNum::serialize` writes for it as well. """ + # before the bound, which is a comparison: `"5" <= 2**63 - 1` is a + # bare TypeError about the operands, from underneath the library + # rather than through its exception contract, and True would be the + # script number one -- the reason `is_integer` names bool + if not is_integer(i): + raise BTClibTypeError(f"non-integer script number: {type(i).__name__}") if not _MIN_SCRIPT_NUM <= i <= _MAX_SCRIPT_NUM: err_msg = f"script number out of range: {i}" err_msg += f", not in [{_MIN_SCRIPT_NUM}, {_MAX_SCRIPT_NUM}]" diff --git a/docs/source/guide.rst b/docs/source/guide.rst index 973aeb8c1..a6de847f4 100644 --- a/docs/source/guide.rst +++ b/docs/source/guide.rst @@ -89,7 +89,7 @@ with ``bytes.fromhex``. Passing text where hex is expected fails: >>> from btclib.ecc import dsa >>> dsa.sign("hello world", 1) Traceback (most recent call last): -ValueError: non-hexadecimal number found in fromhex() arg at position 0 +btclib.exceptions.BTClibValueError: invalid hex string: non-hexadecimal number found in fromhex() arg at position 0 Pass ``bytes`` when you mean text, and let the hex spelling be for things that are bytes: diff --git a/tests/block/block_context_test.py b/tests/block/block_context_test.py index cd2f8a826..361cbd32b 100644 --- a/tests/block/block_context_test.py +++ b/tests/block/block_context_test.py @@ -227,6 +227,13 @@ def test_a_timestamp_may_be_two_hours_ahead_and_no_more() -> None: with pytest.raises(BTClibValueError, match=err_msg): header.assert_valid_time(header.time.replace(tzinfo=None)) + # and what is no datetime at all reaches `.tzinfo` no longer: an + # AttributeError is neither a ValueError nor a TypeError, so nothing + # this library tells a caller to catch would have caught it + for not_a_datetime in ("nope", 12345, None, header.time.date()): + with pytest.raises(BTClibTypeError, match="invalid current time type: "): + header.assert_valid_time(not_a_datetime) # type: ignore[arg-type] + def test_the_contextual_rules_are_not_asked_by_assert_valid() -> None: """Two questions, and a block answers the first one on its own. diff --git a/tests/block/block_test.py b/tests/block/block_test.py index 78d4b1922..d4ca05967 100644 --- a/tests/block/block_test.py +++ b/tests/block/block_test.py @@ -183,6 +183,16 @@ def test_exceptions() -> None: with pytest.raises(BTClibValueError, match=err_msg): header.assert_valid() + # and what is no datetime at all is refused before `.tzinfo` is asked + # of it: an AttributeError is outside both halves of this library's + # exception contract, and the default `check_validity=True` path was + # as exposed as this one + for not_a_datetime in ("not-a-datetime", 12345, None): + header = BlockHeader.parse(header_bytes) + header.time = not_a_datetime # type: ignore[assignment] + with pytest.raises(BTClibTypeError, match="invalid timestamp type: "): + header.assert_valid() + header = BlockHeader.parse(header_bytes) header.nonce = 0x100000000 with pytest.raises(BTClibValueError, match="invalid nonce: "): diff --git a/tests/block/mining_test.py b/tests/block/mining_test.py index 04a7f2508..c5886a662 100644 --- a/tests/block/mining_test.py +++ b/tests/block/mining_test.py @@ -30,7 +30,7 @@ from btclib.block import Block, BlockHeader from btclib.block.mining import VERSION, candidate_block_header, mine from btclib.block.proof_of_work import REGTEST_POW_LIMIT_BITS -from btclib.exceptions import BTClibValueError +from btclib.exceptions import BTClibTypeError, BTClibValueError from btclib.script import ScriptPubKey from btclib.tx import OutPoint, Tx, TxIn, TxOut from btclib.utils import encode_num @@ -204,6 +204,25 @@ def test_mine_exceptions() -> None: mine(candidate) +def test_mine_refuses_what_is_no_header_and_no_count() -> None: + """`replace()` complained about dataclasses, from the standard library. + + "should be called on dataclass instances", raised by `dataclasses` + about a call the caller never made -- and `max_tries < 1` was a bare + TypeError about the operands, with a float passing it to fail at + `range` a few lines further down. + """ + transactions = [_coinbase(1)] + candidate = candidate_block_header(_PREVIOUS, transactions, _TIME, _EASY_BITS) + + for not_a_header in ("not a header", None, 1): + with pytest.raises(BTClibTypeError, match="invalid header type: "): + mine(not_a_header) # type: ignore[arg-type] + for not_a_count in ("lots", None, 1.5): + with pytest.raises(BTClibTypeError, match="invalid max_tries type: "): + mine(candidate, not_a_count) # type: ignore[arg-type] + + def test_candidate_version() -> None: """BIP9 leaves the top three bits at 001, and version 1 is dead.""" assert VERSION == 0x20000000 diff --git a/tests/block/proof_of_work_test.py b/tests/block/proof_of_work_test.py index 20c8d5a7d..e777d8fe5 100644 --- a/tests/block/proof_of_work_test.py +++ b/tests/block/proof_of_work_test.py @@ -36,7 +36,7 @@ retarget_first_height, target_from_bits, ) -from btclib.exceptions import BTClibValueError +from btclib.exceptions import BTClibTypeError, BTClibValueError def _time(timestamp: int) -> datetime: @@ -314,6 +314,31 @@ def test_retarget_first_height() -> None: with pytest.raises(BTClibValueError, match="invalid retarget height: "): retarget_first_height(last) + # and what is no height at all is refused before the arithmetic: + # `"2015" + 1` complained about concatenating a str to an int + for not_a_height in ("2015", None, 1.5): + with pytest.raises(BTClibTypeError, match="invalid height type: "): + retarget_first_height(not_a_height) # type: ignore[arg-type] + + +def test_next_bits_takes_two_datetimes_and_says_so() -> None: + """Two ints subtract to an int, which has no `total_seconds`. + + So the timespan of a retarget window given as two unix timestamps -- + the spelling a caller reading a header's own field would reach for -- + left through an `AttributeError`, which is neither half of this + library's exception contract; a str left through a TypeError about + the operands of a subtraction. + """ + first, last = _time(1279008237), _time(1279297671) + assert next_bits("1c05a3f4", first, last) == bytes.fromhex("1c0168fd") + + for not_a_time in ("nope", 1279008237, None, first.date()): + with pytest.raises(BTClibTypeError, match="invalid first block time type: "): + next_bits("1c05a3f4", not_a_time, last) # type: ignore[arg-type] + with pytest.raises(BTClibTypeError, match="invalid last block time type: "): + next_bits("1c05a3f4", first, not_a_time) # type: ignore[arg-type] + def test_next_bits_mainnet_history() -> None: """The four retarget vectors of Core's `pow_tests.cpp`. diff --git a/tests/ecc/borromean_test.py b/tests/ecc/borromean_test.py index fb045116f..e7027dad3 100644 --- a/tests/ecc/borromean_test.py +++ b/tests/ecc/borromean_test.py @@ -14,7 +14,11 @@ from btclib.alias import Point from btclib.curves import mult, secp256k1 from btclib.ecc import borromean, dsa -from btclib.exceptions import BTClibRuntimeError, BTClibValueError +from btclib.exceptions import ( + BTClibRuntimeError, + BTClibTypeError, + BTClibValueError, +) from tests.curves.curve_test import low_card_curves @@ -46,7 +50,7 @@ def test_borromean() -> None: # a msg that is neither bytes nor a hex-str is a caller error, and # verify says so instead of answering False: catching Exception would # report an int msg as a failed ring signature - with pytest.raises(TypeError): + with pytest.raises(BTClibTypeError, match="invalid octets type: int"): borromean.verify(0, sig[0], sig[1], pubk_rings) # type: ignore[arg-type] # a forged signature must raise, not merely return a falsy value: @@ -212,3 +216,30 @@ def test_the_point_at_infinity_is_the_other_corner_case() -> None: assert not borromean.verify( b"\x00\x00\x00\x00", (21).to_bytes(32, "big"), [[1]], [[Q1]], ec=ec ) + + +def test_one_nonce_and_one_signing_index_per_ring() -> None: + """A short ks truncated the loops and signed a subset of the rings. + + `zip(..., strict=True)` is what caught it, with the message "zip() + argument 3 is shorter than argument 1" -- a `BTClibValueError`'s + class carrying none of its content, naming an argument position of + `zip` and no parameter of `sign`. The check is `sign`'s own now, and + `strict=True` stays as the assertion that the two cannot drift + apart. + """ + ring_sizes = [3, 4] + sign_key_idx = [2, 1] + key_rings = [[dsa.gen_keys() for _ in range(size)] for size in ring_sizes] + sign_keys = [key_rings[i][sign_key_idx[i]][0] for i in range(2)] + pubk_rings = [[key_rings[i][j][1] for j in range(ring_sizes[i])] for i in range(2)] + msg = b"Borromean ring signature" + + assert borromean.sign(msg, [1, 2], sign_key_idx, sign_keys, pubk_rings) + + err_msg = "2 rings, 2 signing indexes and 1 nonces" + with pytest.raises(BTClibValueError, match=err_msg): + borromean.sign(msg, [1], sign_key_idx, sign_keys, pubk_rings) + err_msg = "2 rings, 1 signing indexes and 2 nonces" + with pytest.raises(BTClibValueError, match=err_msg): + borromean.sign(msg, [1, 2], sign_key_idx[:1], sign_keys, pubk_rings) diff --git a/tests/ecc/der_test.py b/tests/ecc/der_test.py index 68a8760e7..289ca6144 100644 --- a/tests/ecc/der_test.py +++ b/tests/ecc/der_test.py @@ -37,8 +37,8 @@ def test_der_size() -> None: def test_der_deserialize() -> None: """Refuse each malformed DER field with its own message.""" - err_msg = "non-hexadecimal number found " - with pytest.raises(ValueError, match=err_msg): + err_msg = "invalid hex string: non-hexadecimal number found " + with pytest.raises(BTClibValueError, match=err_msg): Sig.parse("not a sig") sig = Sig(2**255 - 4, 2**247 - 1) diff --git a/tests/fetch/fetcher_test.py b/tests/fetch/fetcher_test.py index dc943eb99..fef899d58 100644 --- a/tests/fetch/fetcher_test.py +++ b/tests/fetch/fetcher_test.py @@ -75,7 +75,7 @@ def test_tx_id_hex_takes_whatever_octets_takes() -> None: @pytest.mark.parametrize("tx_id", ["", "00", TX_ID + "00", "not hex at all"]) def test_tx_id_hex_refuses_what_is_not_an_id(tx_id: str) -> None: """A mistyped id is the caller's error, not the remote host's 404.""" - with pytest.raises(ValueError): + with pytest.raises(BTClibValueError): tx_id_hex(tx_id) diff --git a/tests/integer_policy_test.py b/tests/integer_policy_test.py index 8ebcd7b15..06b22f8d5 100644 --- a/tests/integer_policy_test.py +++ b/tests/integer_policy_test.py @@ -32,14 +32,18 @@ str_from_index_int, ) from btclib.block import BlockHeader +from btclib.block.block import bip34_commitment from btclib.block.block_context import BlockContext +from btclib.block.mining import mine +from btclib.block.proof_of_work import hash_rate, retarget_first_height from btclib.exceptions import BTClibTypeError from btclib.fee import FeeRate, fee_from_vsize +from btclib.hashes import merkle_root_from_branch, sha256 from btclib.mnemonic.entropy import bin_str_entropy_from_wordlist_indexes from btclib.number_theory import mod_inv from btclib.script import input_script_sig, sig_hash from btclib.tx import OutPoint, Tx, TxIn, TxOut -from btclib.utils import bytes_from_octets, is_integer +from btclib.utils import bytes_from_octets, encode_num, is_integer _TX_ID = "01" * 32 _RATE = FeeRate(sats_per_kvbyte=1000) @@ -121,6 +125,17 @@ def _header(version: Any = 1, nonce: Any = 1) -> BlockHeader: "sig_hash input index", lambda v: sig_hash.taproot(_tx(), v, _PREVOUTS, 1, 0, b"", b""), ), + ("script number", encode_num), + ( + "merkle leaf index", + lambda v: merkle_root_from_branch(b"\x00" * 32, [], v, sha256), + ), + ("bip34 commitment height", bip34_commitment), + ("retarget height", retarget_first_height), + ("mining max tries", lambda v: mine(_header(), v)), + ("hash rate difficulty", lambda v: hash_rate(v, 600.0)), + ("hash rate timespan", lambda v: hash_rate(1.0, v)), + ("hash rate block count", lambda v: hash_rate(1.0, 600.0, v)), ] _IDS = [case[0] for case in _CASES] @@ -174,6 +189,15 @@ def test_the_integers_a_bool_refusal_must_not_take_with_it() -> None: assert mod_inv(3, 7) == 5 assert input_script_sig(None, _SCRIPT_TREE, 0)[0] == ["OP_1"] assert len(sig_hash.taproot(_tx(), 0, _PREVOUTS, 1, 0, b"", b"")) == 32 + assert encode_num(1) == b"\x01" + assert merkle_root_from_branch(b"\x00" * 32, [], 0, sha256) == b"\x00" * 32 + assert bip34_commitment(1) == b"Q" + assert retarget_first_height(2015) == 0 + assert mine(_header(), 1) is None + assert hash_rate(1.0, 600.0, 1) == 2**32 / 600.0 + # an integer difficulty and an integer timespan are numbers too: the + # bool refusal must not take the int with it where a float is annotated + assert hash_rate(1, 600) == hash_rate(1.0, 600.0) # the str and bytes spellings of a path are untouched by any of it assert indexes_from_der_path("m/44h/0h") == [2147483692, 2147483648] diff --git a/tests/mnemonic/entropy_test.py b/tests/mnemonic/entropy_test.py index 6bf642307..f42e8e32e 100644 --- a/tests/mnemonic/entropy_test.py +++ b/tests/mnemonic/entropy_test.py @@ -121,7 +121,7 @@ def test_conversions() -> None: # `== "0x"` weakened to `>=` is what a string of only letters tells # apart -- "ab" sorts above "0x" and parses as hex (171) where the # unweakened check falls through to `int("ab")` and raises instead - with pytest.raises(ValueError, match="invalid literal for int"): + with pytest.raises(BTClibValueError, match="not a base 10 number"): bin_str_entropy_from_int("ab", 8) max_bits = max(_bits) @@ -210,13 +210,16 @@ def test_exceptions() -> None: with pytest.raises(BTClibValueError, match=err_msg): bin_str_entropy_from_entropy(bytes_entropy216, 224) - with pytest.raises(BTClibValueError, match=err_msg): + # an empty tuple is entropy of no spelling at all, which is what it + # is refused as: it used to reach `len()` and be reported as zero + # bits, a number it never carried + with pytest.raises(BTClibTypeError, match="invalid octets type: tuple"): bin_str_entropy_from_entropy(()) # type: ignore[arg-type] - with pytest.raises(ValueError): + with pytest.raises(BTClibValueError, match="not a base 10 number"): bin_str_entropy_from_int("not an int") - with pytest.raises(TypeError): + with pytest.raises(BTClibTypeError, match="invalid entropy type: int"): bin_str_entropy_from_str(3) # type: ignore[arg-type] err_msg = "invalid number of bits: " @@ -481,3 +484,24 @@ def test_bin_str_entropy_from_random() -> None: assert len(bin_str_entropy_from_random(512)) == 512 with pytest.raises(BTClibValueError, match=err_msg): bin_str_entropy_from_random(513) + + +def test_what_is_no_binary_string_is_refused_without_being_echoed() -> None: + """`int(x, 2)` said "invalid literal for int() with base 2" and the digits. + + Raw entropy is seed material, so neither the class nor the message + was right: a bare ValueError naming neither the parameter nor this + library, carrying the very string it was handed. Every message in + this module says a length or a count instead. + """ + for not_binary in ("0b2", "0bxyz", "0b"): + with pytest.raises(BTClibValueError, match="not a binary") as excinfo: + bin_str_entropy_from_int(not_binary) + # the digits stay out of the message, being seed material + assert not_binary not in str(excinfo.value) + + for not_binary in ("2" * 128, "abc" * 43): + with pytest.raises(BTClibValueError, match="not a binary 0/1 string"): + bin_str_entropy_from_str(not_binary) + with pytest.raises(BTClibValueError, match="not a binary 0/1 string"): + wordlist_indexes_from_bin_str_entropy(not_binary, 2048) diff --git a/tests/script/sig_hash_taproot_test.py b/tests/script/sig_hash_taproot_test.py index 338f1ff0a..295476ae9 100644 --- a/tests/script/sig_hash_taproot_test.py +++ b/tests/script/sig_hash_taproot_test.py @@ -768,7 +768,7 @@ def sig_hash_with(message_extension: Octets) -> bytes: ) assert sig_hash_with(ext) == sig_hash_with(ext.hex()) - with pytest.raises(ValueError, match="fromhex"): + with pytest.raises(BTClibValueError, match="invalid hex string: "): sig_hash_with("not hex at all") diff --git a/tests/script/taproot_test.py b/tests/script/taproot_test.py index c24347aa3..a02f35e08 100644 --- a/tests/script/taproot_test.py +++ b/tests/script/taproot_test.py @@ -271,12 +271,14 @@ def test_a_control_block_size_is_octets_and_not_characters() -> None: with pytest.raises(BTClibValueError, match=err_msg): assert_valid_control_block(wrong_size) - # a str that is no hex string reaches the size check no longer; the - # class is `bytes_from_octets`'s to tighten, which issue 744's last - # slice is about + # a str that is no hex string reaches the size check no longer, and + # what is no octets at all is refused rather than measured: `len` of + # a tuple of 33 ints is 33 for not_octets in ("é" * 33, "a" * 33): - with pytest.raises(ValueError, match="fromhex"): + with pytest.raises(BTClibValueError, match="invalid hex string: "): assert_valid_control_block(not_octets) + with pytest.raises(BTClibTypeError, match="invalid octets type: tuple"): + assert_valid_control_block(tuple(range(33))) # type: ignore[arg-type] def test_a_leaf_is_named_from_the_start_of_the_tree() -> None: diff --git a/tests/utils_test.py b/tests/utils_test.py index cfef09a37..075346a9b 100644 --- a/tests/utils_test.py +++ b/tests/utils_test.py @@ -12,6 +12,7 @@ from btclib.exceptions import BTClibTypeError, BTClibValueError from btclib.utils import ( assert_no_trailing, + bytes_from_octets, decode_num, encode_num, hex_string, @@ -97,8 +98,9 @@ def test_int_from_integer_reads_a_str_as_hex() -> None: assert int_from_integer(1234) == 1234 # and an odd number of digits is not a one-digit decimal either - # (Python 3.14 rephrased the message bytes.fromhex raises) - with pytest.raises(ValueError, match="fromhex"): + # (the message is bytes.fromhex's own, which Python 3.14 rephrased, + # inside the class this library promises) + with pytest.raises(BTClibValueError, match="invalid hex string: "): int_from_integer("9") @@ -116,7 +118,7 @@ def test_hex_string() -> None: # invalid hex-string: odd number of hex digits # (Python 3.14 rephrased the message bytes.fromhex raises) a_str = "1deadbeef00000000" - with pytest.raises(ValueError, match="fromhex"): + with pytest.raises(BTClibValueError, match="invalid hex string: "): hex_string(a_str) int_ = -1 @@ -250,3 +252,44 @@ def test_a_json_number_is_a_whole_one_or_it_is_an_error() -> None: for not_a_number in (None, object(), [1]): with pytest.raises(BTClibTypeError, match="invalid version type: "): int_from_json_number(not_a_number, "version") + + +def test_octets_are_bytes_or_the_hex_string_of_bytes_and_nothing_else() -> None: + """A tuple went through untouched, to be measured as if it were octets. + + `bytes_from_octets` returned anything that was not a `str` + unchanged, so `len` of a tuple of 33 ints was 33 and + `taproot.assert_valid_control_block` accepted it as a control block + size. Every buffer is still taken, and returned as it came: a read + must not rewrite the field it reads, which is what `bytes()` here + would do to a bytearray a caller built. + """ + assert bytes_from_octets(b"\x00\x01") == b"\x00\x01" + assert bytes_from_octets("0001") == b"\x00\x01" + # every buffer, though `Octets` names only the two spellings a caller + # writes: what reaches this is whatever a field was built from + assert bytes_from_octets(bytearray(b"\x00\x01")) == b"\x00\x01" # type: ignore[arg-type] + assert bytes_from_octets(memoryview(b"\x00\x01")) == b"\x00\x01" # type: ignore[arg-type] + # unchanged, and not merely equal + buffer: object = bytes_from_octets(bytearray(b"\x00")) # type: ignore[arg-type] + assert isinstance(buffer, bytearray) + + for not_octets in (tuple(range(33)), [1, 2], None, 1.5): + with pytest.raises(BTClibTypeError, match="invalid octets type: "): + bytes_from_octets(not_octets) # type: ignore[arg-type] + with pytest.raises(BTClibTypeError, match="invalid octets type: "): + int_from_integer(not_octets) # type: ignore[arg-type] + # an int is an `Integer` and no `Octets`, so the two differ on it + assert int_from_integer(1) == 1 + with pytest.raises(BTClibTypeError, match="invalid octets type: int"): + bytes_from_octets(1) # type: ignore[arg-type] + + # the hex string that is not one, in both, with the message + # `bytes.fromhex` gives: a position, and never the string itself + for not_hex in ("9", "zz", "not hex at all"): + with pytest.raises(BTClibValueError, match="invalid hex string: "): + bytes_from_octets(not_hex) + with pytest.raises(BTClibValueError, match="invalid hex string: "): + int_from_integer(not_hex) + with pytest.raises(BTClibValueError, match="invalid hex integer: "): + int_from_integer("0xzz") From 96597c0632d496e99ad315c4e13cf27877cc79be Mon Sep 17 00:00:00 2001 From: Ferdinando Ametrano Date: Thu, 13 Aug 2026 21:52:37 +0200 Subject: [PATCH 3/3] Gate the input-validation rule, and let the gate enumerate (#774) `tests/input_validation_test.py` holds every public module-level function whose required parameters are all library input types to the rule of #744: a malformed argument leaves as a BTClibException. One predicate and not a tuple, which is what #743's base class was landed for. It calls with every argument malformed at once, and that is what makes it automatic: no valid values have to be tabulated -- a valid Octets is 20 bytes here, 32 there and any length elsewhere -- and whichever argument the function refuses first, the rule says it must refuse it as a btclib error. Three lists carry what the run finds, and each ratchets one way. `_MALFORMED` is the vocabulary, and a type renamed out of it fails rather than shrinking the walk in silence. `_EXCLUDED` is the nine `is_p2*` predicates, with the reason `script_pub_key._is_funct` already gives. `_OPEN` is what the census of #744 has left, each entry naming the class that escapes; an entry that has become compliant fails the run, as RUF100 fails an unused noqa, so a fix cannot land without deleting its line. What the walk cannot reach is stated rather than omitted: a parameter behind a default is never driven, `hf` and `network` among them, and a function taking a Tx, a Psbt or a callback needs an instance the vocabulary cannot build. It found what the reading missed: `ecc.dleq.verify_proof` answers False for a pub key that is None, where its own comment says a caller error must raise -- the shape #745 closed in five other verifications, in a function written after that census was taken. --- CHANGELOG.md | 30 +++ tests/input_validation_test.py | 351 +++++++++++++++++++++++++++++++++ 2 files changed, 381 insertions(+) create mode 100644 tests/input_validation_test.py diff --git a/CHANGELOG.md b/CHANGELOG.md index bf5ff337b..bc6b63707 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2736,6 +2736,36 @@ documented at release-notes length in the first place, and are still in ### Tests +- **The input-validation rule has a gate, and the gate enumerates by + running** (issues #743, #744). `tests/input_validation_test.py` holds + every public module-level function whose required parameters are all + library input types -- `Octets`, `Integer`, `String`, `Point`, and the + key and path aliases -- to the rule: a malformed argument leaves as a + `BTClibException`. The predicate is one class rather than a tuple, + which is what #743's base class was landed for. + + It calls with **every argument malformed at once**, which is what makes + it automatic: no valid values have to be tabulated, and whichever + argument the function refuses first, the rule says it must refuse it as + a btclib error. What is not reachable that way is stated rather than + omitted -- a parameter behind a default is never driven, `hf` and + `network` among them, and a function taking a `Tx` or a `Psbt` needs an + instance the vocabulary cannot build. + + Three lists carry what the run finds. `_MALFORMED` is the vocabulary, + and a type renamed out of it fails a test rather than shrinking the + walk in silence. `_EXCLUDED` is the nine `is_p2*` predicates, with the + reason `script_pub_key._is_funct` already gives: a bool function about + a script answers False for bytes that are not one. `_OPEN` is what the + census of #744 has left, each entry naming the class that escapes -- + and it can only shrink, an entry that has become compliant failing the + run exactly as RUF100 fails an unused `noqa`. + + The gate found what the reading missed: `ecc.dleq.verify_proof` answers + `False` for a pub key that is `None`, where its own comment says a + caller error must raise. That is the shape #745 closed in five other + verifications, in a function written after that census was taken. + - **The 88 high-s vectors are pinned, not assumed** (#695). Every vector of `tests/ecc/_data/signmessage.json` verifies, which is the answer Bitcoin Core's `verifymessage` gives for all 200 of them and the whole diff --git a/tests/input_validation_test.py b/tests/input_validation_test.py new file mode 100644 index 000000000..38093def6 --- /dev/null +++ b/tests/input_validation_test.py @@ -0,0 +1,351 @@ +# Copyright (c) The btclib developers +# Distributed under the MIT software license, see the accompanying +# LICENSE file or https://opensource.org/license/mit for the full text. + +"""The gate for the one rule about a public function's inputs. + +> Every public function guarantees the validation of all its inputs, +> directly or indirectly. A malformed argument leaves as +> `BTClibTypeError` or `BTClibValueError`. + +Both are `BTClibException`, which is what makes this one predicate +instead of a tuple that has to be kept in step with the hierarchy -- the +reason issue #743 landed that base class before this test rather than +after it. + +## How it calls what it calls + +The library's input types are few and well bounded, most of them named in +`btclib/alias.py` and the key and path ones beside their converters. +`_MALFORMED` gives each of them values that are not of it, and the walk +finds every public module-level function whose *required* parameters are +all of those types. Those it can call with no fixture and no knowledge of +what the function does, and what it asserts is the rule as written: +something is raised, and it is a `BTClibException`. + +Every argument is malformed at once, which is not weaker than one +malformed argument among valid ones: whichever the function refuses +first, the rule says it must refuse it as a btclib error. And it needs no +valid values, which is what makes the walk automatic -- a valid `Octets` +is 20 bytes for one function, 32 for another and any length for a third, +so the table of those is the hand-written thing this avoids. + +## What it does not reach, and why that is not a hole to plug here + +A **parameter with a default** is never driven: to reach `hf` or +`network` the arguments before them would have to be valid, which is the +table this design is built to do without. Those two are gated by hand +instead, where their own checks live. + +A **method**, and a function taking a `Tx`, a `Psbt` or a callback, needs +a valid instance the vocabulary cannot build. That is the part of issue +#744 that stays hand-read. `test_the_walk_reaches_what_it_claims` pins +what the walk does find, so a narrowing of it fails here rather than +quietly running over less. + +## The three lists, and which way each ratchets + +- `_MALFORMED` is the vocabulary, and every name in it is a type this + tree still declares: a rename would otherwise shrink the walk in + silence, which `test_the_vocabulary_is_the_libraries_input_types` is + against. +- `_EXCLUDED` is what must not be held to the rule this way, with the + reason. What is in it is a family whose own comment states the design. +- `_OPEN` is what issue #744's census left, each entry naming the class + that escapes. It can only shrink: `test_what_is_open_is_still_open` + fails on an entry that has become compliant, as RUF100 fails an unused + `noqa`, so a fix cannot land without deleting its line. +""" + +from __future__ import annotations + +import ast +import importlib +from pathlib import Path +from typing import Any + +import pytest + +from btclib.exceptions import BTClibException + +_LIBRARY = Path(__file__).parents[1] / "btclib" + +# a value of none of the library's input types is what each of these is, +# and the tuples are read round-robin so that a function taking three +# parameters of one type is called with three different wrong values. +# Constants and not a strategy: what this gate reports has to be the same +# on two runs, `_OPEN` below being read as a statement about the tree +_MALFORMED: dict[str, tuple[Any, ...]] = { + "BIP32Key": (None, 1.5, "not an xkey"), + "BinaryData": ("not hex at all", None, 1.5), + "DerPath": (-1, [2**32], "m/x", 1.5), + "Integer": ("not hex at all", None, 1.5), + "Key": (None, 1.5, "not a key"), + "Octets": ("not hex at all", "9", tuple(range(4)), None), + "Point": ((1,), "not a point", None), + "PrvKey": (None, 1.5, "not a key"), + "PubKey": (None, 1.5, "not a key"), + "ScriptList": (None, 1.5, "not a list"), + "String": (1, None, 1.5), +} + +# one reason for nine functions, and it is `script_pub_key._is_funct`'s +# own: "these bool functions answer 'are these bytes a p2sh script', so +# bytes that are not are False". A malformed hex string is bytes that are +# not, and what `bytes.fromhex` raises for it is a ValueError, which that +# `except ValueError` catches on purpose. A wrong *type* is not covered +# by it and does raise, which is the half of those nine the rule reaches +_A_PREDICATE_ANSWERS_FALSE = ( + "a bool function about a script answers False for bytes that are not" + " one, and a malformed hex string is bytes that are not: the reason is" + " in script_pub_key._is_funct, which catches ValueError alone" +) + +_EXCLUDED: dict[str, str] = { + "btclib.script.script_pub_key.is_nulldata": _A_PREDICATE_ANSWERS_FALSE, + "btclib.script.script_pub_key.is_p2ms": _A_PREDICATE_ANSWERS_FALSE, + "btclib.script.script_pub_key.is_p2pk": _A_PREDICATE_ANSWERS_FALSE, + "btclib.script.script_pub_key.is_p2pkh": _A_PREDICATE_ANSWERS_FALSE, + "btclib.script.script_pub_key.is_p2sh": _A_PREDICATE_ANSWERS_FALSE, + "btclib.script.script_pub_key.is_p2tr": _A_PREDICATE_ANSWERS_FALSE, + "btclib.script.script_pub_key.is_p2wpkh": _A_PREDICATE_ANSWERS_FALSE, + "btclib.script.script_pub_key.is_p2wsh": _A_PREDICATE_ANSWERS_FALSE, + "btclib.script.script_pub_key.is_segwit": _A_PREDICATE_ANSWERS_FALSE, +} + +# what issue #744's census left, by the class that escapes. Deleting a +# line is how a fix lands, the test below failing on an entry that has +# stopped leaking, so this cannot go stale in either direction +_OPEN: dict[str, str] = { + "btclib.b32.has_segwit_prefix": "AttributeError", + "btclib.b32.p2wpkh": "TypeError", + "btclib.b32.witness_from_address": "TypeError", + "btclib.b58.h160_from_address": "TypeError", + "btclib.b58.p2pkh": "TypeError", + "btclib.b58.p2wpkh_p2sh": "TypeError", + "btclib.b58.wif_from_prv_key": "TypeError", + "btclib.base58.decode": "TypeError", + "btclib.bech32.decode": "AttributeError", + "btclib.bip32.bip32.crack_prv_key": "TypeError", + "btclib.bip32.bip32.derive": "TypeError", + "btclib.bip32.bip32.xpub_from_xprv": "TypeError", + "btclib.bip32.der_path.bytes_from_der_path": "TypeError", + "btclib.bip32.der_path.hardenings_from_der_path": "TypeError", + "btclib.bip32.der_path.indexes_from_der_path": "TypeError", + "btclib.bip32.der_path.str_from_der_path": "TypeError", + "btclib.bip322.sign": "AttributeError", + "btclib.bip322.to_sign_psbt": "AttributeError", + "btclib.bip44.address_from_der_path": "TypeError", + "btclib.bip85.bytes_entropy_from_root_key": "TypeError", + "btclib.bip85.drng_from_der_path": "TypeError", + "btclib.bip85.entropy_from_der_path": "TypeError", + "btclib.bip85.mnemonic_from_root_key": "TypeError", + "btclib.bip85.wif_from_root_key": "TypeError", + "btclib.bip85.xprv_from_root_key": "TypeError", + "btclib.curves.curve.double_mult": "TypeError", + "btclib.curves.sec_point.bytes_from_point": "TypeError", + "btclib.descriptors.descriptors.account_descriptors": "TypeError", + "btclib.ecc.dleq.generate_proof": "TypeError", + "btclib.ecc.dleq.verify_proof": "no exception", + "btclib.ecc.ecies.derive_keys": "TypeError", + "btclib.ecc.ellswift.create": "TypeError", + "btclib.ecc.musig2.individual_pub_key": "TypeError", + "btclib.ecc.musig2.nonce_gen": "TypeError", + "btclib.psbt.psbt_utils.deserialize_map": "AttributeError", + "btclib.script.script.parse": "AttributeError", + "btclib.script.script_pub_key.address": "no exception", + "btclib.script.sig_hash.redeem_script": "AttributeError", + "btclib.script.taproot.output_prvkey": "TypeError", + "btclib.script.taproot.output_prvkey_from_merkle_root": "TypeError", + "btclib.script.taproot.parse": "AttributeError", + "btclib.script.taproot.serialize": "TypeError", + "btclib.silent_payments.keys_from_address": "TypeError", + # the name of a BIP352 function, and detect-secrets reads any + # "...secret": "..." as one + "btclib.silent_payments.shared_secret": "TypeError", # pragma: allowlist secret + "btclib.slip132.address_from_xkey": "TypeError", + "btclib.slip132.address_from_xpub": "TypeError", + "btclib.slip132.p2pkh_xkey": "TypeError", + "btclib.slip132.p2wpkh_p2sh_xkey": "TypeError", + "btclib.slip132.p2wpkh_xkey": "TypeError", + "btclib.to_prv_key.int_from_prv_key": "TypeError", + "btclib.to_prv_key.prv_keyinfo_from_prv_key": "TypeError", + "btclib.to_pub_key.fingerprint": "TypeError", + "btclib.to_pub_key.point_from_key": "TypeError", + "btclib.to_pub_key.pub_keyinfo_from_key": "TypeError", + "btclib.to_pub_key.pub_keyinfo_from_prv_key": "TypeError", + "btclib.tx_or_psbt.tx_or_psbt_from_any": "AttributeError", + "btclib.utils.bytesio_from_binarydata": "no exception", + "btclib.var_bytes.parse": "AttributeError", + "btclib.var_int.parse": "AttributeError", +} + + +def _alias_of(annotation: ast.expr) -> str | None: + """Return the input type an annotation names, `X | None` included.""" + name = ast.unparse(annotation).replace(" | None", "").strip() + return name if name in _MALFORMED else None + + +def _drivable() -> dict[str, list[str]]: + """Return every public function the vocabulary can call, by dotted name. + + Required parameters only: what carries a default is what a caller may + leave out, so a function is driven on the arguments it insists on. + All of them have to be in the vocabulary -- one `Tx` and the walk has + nothing to pass. + """ + found: dict[str, list[str]] = {} + for path in sorted(_LIBRARY.rglob("*.py")): + module = ".".join(path.relative_to(_LIBRARY.parent).with_suffix("").parts) + for node in ast.parse(path.read_text()).body: + if not isinstance(node, ast.FunctionDef) or node.name.startswith("_"): + continue + positional = [*node.args.posonlyargs, *node.args.args] + required = positional[: len(positional) - len(node.args.defaults)] + # `is not None` narrows for mypy and never filters: mypy runs + # strict over this package, so a parameter without an + # annotation is a state the library does not reach + annotations = [a.annotation for a in required if a.annotation is not None] + aliases = [_alias_of(a) for a in annotations] + if required and len(aliases) == len(required) and all(aliases): + found[f"{module}.{node.name}"] = [a for a in aliases if a] + return found + + +_DRIVABLE = _drivable() + + +def _leak(dotted: str) -> str | None: + """Return the class of what escapes the rule, or None if it holds.""" + module_name, _, name = dotted.rpartition(".") + function = getattr(importlib.import_module(module_name), name) + aliases = _DRIVABLE[dotted] + for round_ in range(max(len(_MALFORMED[a]) for a in aliases)): + args = [_MALFORMED[a][round_ % len(_MALFORMED[a])] for a in aliases] + try: + function(*args) + except BTClibException: + continue + # the class of what came out is the finding, so every one of them + # is caught and named rather than let out of the walk + except Exception as e: # noqa: BLE001 + return type(e).__name__ + return "no exception" + return None + + +_GATED = sorted(set(_DRIVABLE) - _EXCLUDED.keys() - _OPEN.keys()) + + +@pytest.mark.parametrize("dotted", _GATED) +def test_a_malformed_argument_leaves_as_a_btclib_exception(dotted: str) -> None: + """The rule, over every public function the vocabulary can drive.""" + leak = _leak(dotted) + assert leak is None, f"{dotted} answers a malformed argument with {leak}" + + +@pytest.mark.parametrize("dotted", sorted(_OPEN)) +def test_what_is_open_is_still_open(dotted: str) -> None: + """A fix cannot land without deleting its line from `_OPEN`. + + The ratchet, and the reason the list is in the tree rather than in a + report: an entry that has become compliant fails here, so what is + left cannot drift the way a census read by hand did. + """ + assert _leak(dotted) == _OPEN[dotted], ( + f"{dotted} no longer answers with {_OPEN[dotted]}: delete its line" + " from _OPEN, or correct it to what it answers with now" + ) + + +def test_the_vocabulary_is_the_libraries_input_types() -> None: + """A renamed type would narrow the walk without failing anything. + + Every name in `_MALFORMED` is still declared under `btclib/`, and + every type `alias.py` declares and a public parameter is annotated + with is either in the vocabulary or named below with the reason no + wrong value can be built for it. + """ + declared: set[str] = set() + in_alias_py: set[str] = set() + annotated: set[str] = set() + for path in sorted(_LIBRARY.rglob("*.py")): + tree = ast.parse(path.read_text()) + names = { + node.targets[0].id + for node in tree.body + if isinstance(node, ast.Assign) + and len(node.targets) == 1 + and isinstance(node.targets[0], ast.Name) + and node.targets[0].id[0].isupper() + } + declared |= names + if path.name == "alias.py": + in_alias_py = names + for node in ast.walk(tree): + if not isinstance(node, ast.FunctionDef) or node.name.startswith("_"): + continue + arguments = [*node.args.posonlyargs, *node.args.args, *node.args.kwonlyargs] + annotated |= { + ast.unparse(a.annotation).replace(" | None", "").strip() + for a in arguments + if a.annotation is not None + } + + assert set(_MALFORMED) <= declared + + without_a_wrong_value = { + # three Literals: a value outside them is what mypy refuses, and a + # test passing one would be testing the type checker + "BIP44ScriptType", + "NetworkField", + "ScriptType", + # the two hash-function types are always behind a default -- `hf` + # is the last parameter of everything that takes one -- so the + # walk cannot reach them for the reason the module docstring + # gives. `hashes._assert_valid_hf` is the check, and + # tests/hashes_test.py, dsa_test.py and ssa_test.py are where it + # is held to it + "HashDigestF", + "HashF", + # a callable, and the same again: its wrong values are the + # non-callables, and it is never a required parameter + "CipherF", + # the internal coordinates: no public parameter takes them from a + # caller, `curves` converting to them and back + "JacPoint", + # a nested structure whose wrong values are its leaves', and its + # leaves are Octets and int + "TaprootScriptTree", + } + assert in_alias_py & annotated <= set(_MALFORMED) | without_a_wrong_value + + +def test_the_walk_reaches_what_it_claims() -> None: + """The shapes the walk must find, and two it must not. + + A walk that found nothing would pass every test above. One function + per shape it has to reach -- a single parameter, two of different + types, one behind a default it must ignore -- and the two kinds it + must leave alone: a private name, and a function whose required + parameters are not all in the vocabulary. + """ + assert _DRIVABLE["btclib.hashes.sha256"] == ["Octets"] + assert _DRIVABLE["btclib.bip32.bip32.derive"] == ["BIP32Key", "DerPath"] + assert _DRIVABLE["btclib.to_pub_key.pub_keyinfo_from_key"] == ["Key"] + # `network` and `compressed` carry defaults and are not driven + assert _DRIVABLE["btclib.b58.p2pkh"] == ["Key"] + + assert "btclib.hashes._assert_valid_hf" not in _DRIVABLE + # a required parameter the vocabulary cannot build: a Tx, a Psbt + assert "btclib.script.sig_hash.legacy" not in _DRIVABLE + assert "btclib.psbt.psbt.finalize" not in _DRIVABLE + + +def test_every_driven_function_is_gated_excluded_or_open() -> None: + """No function leaves the run without a line saying why.""" + assert not _EXCLUDED.keys() & _OPEN.keys() + assert _EXCLUDED.keys() <= set(_DRIVABLE) + assert _OPEN.keys() <= set(_DRIVABLE) + assert set(_GATED) | _EXCLUDED.keys() | _OPEN.keys() == set(_DRIVABLE)