Skip to content

Make bytes_from_octets the whole of what Octets means - #766

Merged
fametrano merged 1 commit into
block-type-guardsfrom
octets-are-total
Aug 13, 2026
Merged

Make bytes_from_octets the whole of what Octets means#766
fametrano merged 1 commit into
block-type-guardsfrom
octets-are-total

Conversation

@fametrano

@fametrano fametrano commented Aug 13, 2026

Copy link
Copy Markdown
Member

The last slice of #744.
Stacked on #765, which is
on #764, which is on #763. Review the last commit.

The issue proposed this "possibly as a documented decision rather than a
fix". It is a fix: the decision would have been defensible for the class
alone — BTClibValueError is a ValueError — but not for the hole
underneath it.

Two holes, not one

bytes_from_octets is the coercion every Octets parameter of the
library runs through.

  1. 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 no way to pin the message.
  2. Anything that was not a str went through untouched. So len of
    a tuple of 33 ints is 33, which is exactly how
    taproot.assert_valid_control_block accepted one as a control block
    size ( Refuse the malformed input twelve places answered anyway #763 fixed the character-count half and could not fix this
    one), and how bin_str_entropy_from_entropy(()) was reported as
    zero bits — a number it never carried.

Decisions worth a reviewer's eye

  • The message is bytes.fromhex's own, carried through rather than
    replaced. It names a position and never the string, which is the
    point: 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 Private keys reach exception messages and the generated repr #137). A first draft interpolated {octets!r} and
    tests/to_prv_key_test.py::test_a_mistyped_wif_is_reported_as_one
    caught it — the suite has a test for exactly this.
  • Every buffer is taken, and returned as it came. bytes alone
    would have broken
    tests/bip32/bip32_test.py::test_assert_valid_does_not_rewrite_the_key_data,
    which pins that a read must not coerce a bytearray a caller built.
    bytearray and memoryview are accepted with a # type: ignore at
    the call in the test: Octets names the two spellings a caller
    writes, not everything a field can hold.
  • to_prv_key catches TypeError beside ValueError at its two
    "it must be octets" fallbacks, as to_pub_key already did: what is
    neither octets nor a spelling of them means the same thing a wrong
    size does — this input is not a private key.
  • borromean.sign checks the three lengths itself. strict=True
    said "zip() argument 3 is shorter than argument 1", naming no
    parameter of the function; the comment above it defended the class,
    which was right, and left the message. The check is the function's
    now, and strict=True stays as the assertion that the two cannot
    drift apart.
  • HISTORY.md carries one breaking-changes bullet for all four
    slices
    , not just this one. Refuse the malformed input twelve places answered anyway #763 and Check every width a sig_hash preimage writes, and every index #764 turn OverflowError and
    IndexError into btclib classes and Ask a value what it is before asking what it holds #765 an AttributeError; a
    caller catching those three has to act, and that is a release note
    rather than four. Say the word if you want it split per PR.

What moves for a caller

The class is narrower and the control flow identical — BTClibValueError
is a ValueError. A test matching bytes.fromhex's message still
matches. The one non-narrowing is hole 2: a non-Octets argument is now
a BTClibTypeError where it used to be no error, or an error about
something else.

Six test files stop asserting the bare class, and
docs/source/guide.rst shows the new spelling.

Gates

  • uv run pytest — 26508 passed, coverage 100.00%
  • uv run pre-commit run --all-files — exit 0
  • sphinx-build -W --keep-going — exit 0

Summary by Sourcery

Tighten octet handling across the library so malformed or non-octet inputs are consistently rejected with btclib-specific errors and clearer messages, without exposing potentially sensitive data.

Bug Fixes:

  • Ensure bytes_from_octets rejects non-hex strings and non-buffer types instead of silently passing them through, preventing incorrect control-block sizes and entropy calculations.
  • Validate borromean ring signature inputs so mismatched ring, signing-index, and nonce counts raise a descriptive btclib error instead of a generic zip() ValueError.
  • Normalize binary and numeric entropy parsing to raise BTClibValueError/BTClibTypeError with messages that describe the problem without echoing raw entropy values.

Enhancements:

  • Make bytes_from_octets the central, total coercion for all Octets parameters, accepting bytes-like buffers and hex strings while returning btclib error types for invalid inputs.
  • Refactor mnemonic entropy helpers to route binary-string parsing through a shared helper and to convert malformed base-10/16 inputs into btclib errors with explicit base information.
  • Adjust int_from_integer and related callers to go through bytes_from_octets and to surface btclib error types for malformed hex integers.
  • Add tests that pin the new error classes, messages, and buffer-handling semantics for octet and entropy utilities, borromean signatures, DER parsing, fetch utilities, and taproot control blocks.

Documentation:

  • Update HISTORY and CHANGELOG to describe the shift from native exceptions to BTClibValueError/BTClibTypeError for malformed arguments, and note the octet-handling behavior change.
  • Refresh the user guide to reflect the new octet spelling and error semantics.

Tests:

  • Extend and adjust tests to expect BTClibValueError/BTClibTypeError instead of bare builtin exceptions when octet, entropy, and hex inputs are malformed, and to verify that error messages avoid echoing sensitive input data.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @fametrano, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@sourcery-ai

sourcery-ai Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Standardize Octets coercion via bytes_from_octets, propagate btclib-specific error classes/messages for malformed hex/entropy across utilities and mnemonic/borromean modules, and tighten argument validation while preserving secrecy of key/entropy material.

Sequence diagram for standardized Octets coercion via bytes_from_octets

sequenceDiagram
    actor Caller
    participant to_prv_key
    participant bytes_from_octets
    participant BTClibValueError
    participant BTClibTypeError

    Caller->>to_prv_key: int_from_prv_key(prv_key)
    to_prv_key->>bytes_from_octets: bytes_from_octets(prv_key, ec.n_size)

    alt hex string input
        bytes_from_octets->>bytes_from_octets: bytes.fromhex(octets)
        opt malformed hex string
            bytes_from_octets-->>BTClibValueError: raise BTClibValueError("invalid hex string: ...")
        end
    else non-buffer, non-str input
        bytes_from_octets-->>BTClibTypeError: raise BTClibTypeError("invalid octets type: ...")
    else bytes/bytearray/memoryview
        bytes_from_octets-->>to_prv_key: octets (unchanged buffer)
        to_prv_key->>to_prv_key: int.from_bytes(prv_key, "big")
        to_prv_key-->>Caller: q (private key integer)
    end

    rect rgb(230,230,230)
    Note over to_prv_key: Error handling path
    end
    to_prv_key-->>Caller: raise BTClibValueError("not a private key: ...")
    note right of Caller: to_prv_key catches (TypeError, ValueError)
    note right of Caller: and re-raises BTClibValueError without echoing key material
Loading

Sequence diagram for mnemonic entropy parsing via _int_from_bin_str

sequenceDiagram
    actor Caller
    participant wordlist_indexes_from_bin_str_entropy
    participant _int_from_bin_str
    participant BTClibValueError
    participant BTClibTypeError

    Caller->>wordlist_indexes_from_bin_str_entropy: wordlist_indexes_from_bin_str_entropy(entropy, base)
    wordlist_indexes_from_bin_str_entropy->>wordlist_indexes_from_bin_str_entropy: bits = len(entropy)
    wordlist_indexes_from_bin_str_entropy->>_int_from_bin_str: _int_from_bin_str(entropy)

    alt entropy is binary 0/1 string
        _int_from_bin_str->>_int_from_bin_str: int(entropy, 2)
        _int_from_bin_str-->>wordlist_indexes_from_bin_str_entropy: int_entropy
        wordlist_indexes_from_bin_str_entropy->>wordlist_indexes_from_bin_str_entropy: divmod loop to compute indexes
        wordlist_indexes_from_bin_str_entropy-->>Caller: indexes
    else entropy has wrong type
        _int_from_bin_str-->>BTClibTypeError: raise BTClibTypeError("invalid entropy type: ...")
    else entropy not binary 0/1 string
        _int_from_bin_str-->>BTClibValueError: raise BTClibValueError("invalid entropy: not a binary 0/1 string")
    end
Loading

File-Level Changes

Change Details Files
Make bytes_from_octets the single, total definition of Octets and ensure non-octet inputs raise BTClibTypeError while invalid hex strings raise BTClibValueError with a wrapped bytes.fromhex message.
  • Wrap bytes.fromhex in bytes_from_octets to catch ValueError and rethrow BTClibValueError with a prefixed message while preserving the original fromhex text.
  • Reject non-str, non-bytes/bytearray/memoryview octets inputs in bytes_from_octets with BTClibTypeError, instead of passing them through unchanged.
  • Route int_from_integer string/bytes handling through bytes_from_octets, and provide a dedicated BTClibValueError path for malformed 0x-prefixed hex integers.
  • Update tests (utils, taproot, borromean, DER, fetcher, sig_hash_taproot) to assert BTClibValueError/BTClibTypeError and the new messages instead of bare builtin errors.
  • Document the behavioral change and its rationale in CHANGELOG.md and HISTORY.md, emphasizing btclib error classes and the non-echoing of key material.
btclib/utils.py
btclib/to_prv_key.py
tests/utils_test.py
tests/script/taproot_test.py
tests/ecc/borromean_test.py
tests/ecc/der_test.py
tests/fetch/fetcher_test.py
tests/script/sig_hash_taproot_test.py
CHANGELOG.md
HISTORY.md
Normalize entropy parsing errors in btclib.mnemonic.entropy to BTClibValueError/BTClibTypeError without echoing entropy digits, and centralize binary string parsing via a helper.
  • Introduce _int_from_bin_str helper to parse binary strings, converting TypeError to BTClibTypeError and ValueError to BTClibValueError with non-echoing messages.
  • Replace direct int(x,2) calls in entropy functions with _int_from_bin_str for wordlist index derivation, bytes conversion, string validation, and random entropy processing.
  • Refine bin_str_entropy_from_int to distinguish 0b, 0x, and plain decimal strings, wrapping invalid numeric formats in BTClibValueError with base-specific messages.
  • Update entropy tests to expect btclib-specific error classes and messages, including coverage for non-binary strings and invalid numeric entropy inputs.
  • Add HISTORY/CHANGELOG entries summarizing that malformed arguments now raise btclib error types rather than native ones.
btclib/mnemonic/entropy.py
tests/mnemonic/entropy_test.py
CHANGELOG.md
HISTORY.md
Strengthen borromean.sign API-level invariants by explicitly checking consistency across rings, signing indexes, and nonces, and by using BTClibValueError for mismatches.
  • Add a pre-loop length check in borromean.sign to ensure pubk_rings, sign_key_idx, and ks all have matching lengths, raising BTClibValueError with a descriptive count mismatch message.
  • Retain zip(strict=True) for the signing loop as a secondary invariant, now backed by the explicit length check rather than relied upon for user-facing errors.
  • Extend borromean tests to validate the new error message and ensure one nonce and one signing index per ring are enforced.
  • Ensure TypeError from non-octet messages in borromean.verify is surfaced as BTClibTypeError via bytes_from_octets, and adjust the test expectations accordingly.
btclib/ecc/borromean.py
tests/ecc/borromean_test.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

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.
@fametrano
fametrano merged commit ae36e52 into block-type-guards Aug 13, 2026
37 of 38 checks passed
@fametrano
fametrano deleted the octets-are-total branch August 13, 2026 19:38
fametrano added a commit that referenced this pull request Aug 13, 2026
* 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.
fametrano added a commit that referenced this pull request Aug 13, 2026
* 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.
fametrano added a commit that referenced this pull request Aug 13, 2026
* 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.
fametrano added a commit that referenced this pull request Aug 13, 2026
* 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.

* 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.

* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant