Check every width a sig_hash preimage writes, and every index

CHANGELOG.md

@@ -1740,6 +1740,107 @@ 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
+  `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
@@ -2653,6 +2754,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

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

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

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:

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}")
 

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: