Skip to content

Refuse the malformed input twelve places answered anyway - #763

Merged
fametrano merged 2 commits into
mainfrom
wrong-answers
Aug 13, 2026
Merged

Refuse the malformed input twelve places answered anyway#763
fametrano merged 2 commits into
mainfrom
wrong-answers

Conversation

@fametrano

@fametrano fametrano commented Aug 13, 2026

Copy link
Copy Markdown
Member

The first slice of #744, and
the one the issue puts first: the only category of that census that
can cost a user money.
Not an exception of the wrong class — no
exception at all, and a value the caller goes on to use.

The twelve

where what it answered
script/sig_hash.py taproot an input_index past the end of the vin. Nothing dereferences it outside the ANYONECANPAY branch, so 99 and 100 on a two-input transaction gave two different 32-byte hashes, both returned. The bound was in the SIGHASH_SINGLE branch alone, and against the vout
script/taproot.py input_script_sig script_num=-1 picked the last leaf, -2 the one before it, each with a control block that correctly proves the leaf nobody asked for
script/taproot.py assert_valid_control_block len of whatever it was handed: "é" * 33 is 33 characters and 66 octets of UTF-8, and passed as a control block size
bech32.py encode a negative digit indexed _ALPHABET from the end and wrote a different address, correctly checksummed. Above 31 it was IndexError, a LookupError
bip32/der_path.py indexes_from_der_path [-5] for [-5]. The text spelling checks 0 <= index < 0x80000000 per step; the int and the iterable checked nothing
bip32/bip32.py pub_key_derivation_tweaks [] for 33 bytes that are no public key: if indexes: guarded the only code that ever looked at it
descriptors/descriptors.py miniscript_solver a negative vin_i solved the input at the other end
psbt/psbt.py Psbt.weight_estimate an estimate for an incoherent psbt, alone among the public methods reading a psbt's own data
number_theory.py xgcd, mod_inv, legendre_symbol, mod_sqrt, tonelli mod_inv(3.0, 7) is 5.0, out of a signature that says int; a modulus of zero was ZeroDivisionError
utils.py int_from_json_number 1.5 truncated to 1, silently
mnemonic/entropy.py bin_str_entropy_from_wordlist_indexes an index no word answers to: base-base arithmetic has no out of range, 2048 in a 2048-word list being a carry
fetch/fetcher.py tx_for_network a network no table has, written into every output with check_validity=False

Decisions worth a reviewer's eye

  • number_theory checks in each of the five, rather than five private
    twins.
    A pair of isinstance calls measured 0.065 µs against the
    9.9 µs of a 256-bit mod_inv — 0.65%, and the alternative is five
    more names. The comment says so without the numbers, which go stale.
  • bech32.encode walks the digits a second time. The range check
    alone is ~10%, with is_integer ~40%, on a 3.5 µs function called
    once per address — under the key derivation that produced the digits
    by orders of magnitude. Correctness of an address won.
  • assert_valid_control_block widens to Octets, matching
    check_output_pubkey right above it, which coerces the same
    argument. A str that is no hex string now reaches a bare
    ValueError from bytes.fromhex rather than passing; tightening that
    class is issue 88 public functions still let a malformed input through: the census #684 left open #744's last slice, and the test asserts ValueError so
    it stays true across it.
  • tx_for_network resolves the name instead of comparing it, so
    " MainNet " is the short-circuit it always should have been.

Two tests asserted the defect

Both in bip32, and both now assert the fix:

  • derive(xprv, 2**32) was OverflowError: int too big to convert;
    it is a BTClibValueError naming the index.
  • BIP32KeyOrigin("deadbeef", [0xFFFFFFFF + 1]) reached
    assert_valid's own check; the path reader refuses it first. That
    check still earns its keep — der_path is annotated Sequence[int]
    and holds a list, so the frozen dataclass stops a rebinding and not an
    append — and a test reaches it that way.

The bool half of the six new integer guards goes in
tests/integer_policy_test.py, beside the twenty-five parameters
already there rather than in six files.

Gates

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

Part of #744, which stays
open: the sig_hash widths, the block/ guards and bytes_from_octets
are three more slices.

Summary by Sourcery

Tighten validation across multiple modules so malformed inputs are consistently rejected instead of producing seemingly valid hashes, addresses, entropy, weights, BIP32 paths, arithmetic results, taproot scripts, PSBT estimates, and network-labelled transactions.

Bug Fixes:

  • Reject out-of-range or non-integer indexes in taproot sig-hash, taproot leaf selection, miniscript solver, mnemonic wordlist entropy, BIP32 derivation paths, and PSBT weight estimation so invalid positions no longer yield usable outputs.
  • Ensure bech32 encoding refuses non-5-bit or non-integer digit values instead of silently producing different but valid-looking addresses.
  • Enforce integer-only, positive moduli for number-theory routines so floats, bools, and zero/negative moduli no longer produce incorrect residues or uncaught arithmetic errors.
  • Make JSON integer coercion reject fractional, non-numeric, or boolean values so versions and similar fields cannot silently change meaning.
  • Ensure tx_for_network resolves network names via the registry and rejects unknown names instead of embedding invalid network labels into transactions.
  • Fix pub_key_derivation_tweaks to always validate the supplied public key, even for empty paths, so non-points no longer return an empty tweak list that appears successful.

Enhancements:

  • Centralize and strengthen integer policy checks (including new cases for bech32, taproot, BIP32, number theory, mnemonic entropy, and sig-hash) and extend tests to cover type and range validation for these inputs.

Documentation:

  • Document the twelve previously-accepted malformed-input cases in the changelog as a single consolidated entry, explaining their shape and the new refusal behavior.

@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

This PR hardens twelve previously under-validated call sites across the library so that malformed numeric and index-like inputs are refused with BTClib-specific errors instead of silently producing plausible but wrong results or leaking Python-native exceptions. It does so by introducing small, localized validation helpers, widening some APIs to accept generic octet inputs, and adding focused tests (including central integer-policy tests) that verify both correct behavior and new failure modes.

Sequence diagram for tx_for_network network resolution and labeling

sequenceDiagram
    actor Caller
    participant tx_for_network
    participant network_from_name
    participant NETWORKS

    Caller->>tx_for_network: tx_for_network(tx, network)
    tx_for_network->>network_from_name: network_from_name(network)
    network_from_name-->>tx_for_network: Network
    tx_for_network->>NETWORKS: NETWORKS["mainnet"]
    alt resolved network is mainnet
        tx_for_network-->>Caller: tx (unchanged)
    else other network
        tx_for_network->>tx_for_network: construct new TxOut labels
        tx_for_network-->>Caller: relabeled tx
    end
Loading

Sequence diagram for Psbt.weight_estimate with psbt validation

sequenceDiagram
    actor Caller
    participant Psbt

    Caller->>Psbt: weight_estimate(sizer)
    Psbt->>Psbt: assert_valid()
    Psbt-->>Psbt: psbt data validated
    Psbt->>Psbt: build transaction from psbt fields
    Psbt-->>Caller: estimated weight (int)
Loading

File-Level Changes

Change Details Files
Taproot-related helpers now validate leaf indices, control block sizes, and sig-hash input indices so that out-of-range or mistyped values raise BTClib errors instead of generating valid-looking but incorrect spends or hashes.
  • input_script_sig now treats script_num as a bounded, integer leaf index with type and range checks before indexing the tree.
  • assert_valid_control_block now accepts Octets and measures the length of the coerced byte sequence, preventing character-count-based size acceptance for non-hex strings.
  • taproot sig_hash’s taproot function introduces a shared vin index validator that enforces integer type and in-range indices across all sig-hash paths.
btclib/script/taproot.py
btclib/script/sig_hash.py
tests/script/taproot_test.py
tests/script/sig_hash_taproot_test.py
tests/integer_policy_test.py
Bech32 encoding and mnemonic entropy derivation now explicitly validate per-digit/index ranges and types, ensuring only valid 5-bit values and wordlist indices are processed and surfacing BTClibTypeError/BTClibValueError instead of IndexError/TypeError or silent arithmetic carries.
  • bech32.encode walks data once up front to enforce integer type and 0–31 bounds, mapping violations to BTClibTypeError/BTClibValueError.
  • bin_str_entropy_from_wordlist_indexes checks each index with is_integer and a base range guard, rejecting out-of-range or non-integer indices with BTClib errors.
  • New tests cover boundary behavior, negative and overly large digits/indices, and ensure valid edge values remain accepted.
btclib/bech32.py
btclib/mnemonic/entropy.py
tests/bech32_test.py
tests/mnemonic/entropy_test.py
tests/integer_policy_test.py
Number-theory primitives now share internal operand and modulus validators to reject non-integer or non-positive moduli, replacing Python arithmetic exceptions and float propagation with explicit BTClibTypeError/BTClibValueError while preserving behavior for valid integer inputs.
  • Introduced _assert_valid_operand and _assert_valid_modulus and wired them into xgcd, mod_inv, legendre_symbol, mod_sqrt, and tonelli.
  • Excluded bool explicitly from integer operands via is_integer, aligning with var_int.serialize’s policy.
  • Added tests that exercise float, string, None, bool, zero and negative modulus cases, verifying error types and messages while checking xgcd’s behavior with zero operand remains correct.
btclib/number_theory.py
tests/number_theory_test.py
tests/integer_policy_test.py
BIP32 derivation path handling and public-key tweak derivation now enforce index validity consistently across string, int, iterable, and byte spellings, and ensure even empty paths still validate the supplied public key, replacing OverflowError and silent wrong answers with BTClib errors.
  • Added _assert_valid_index helper and reused it from str_from_index_int and _indexes_from_der_path to centralize type and [0, 0xFFFFFFFF] range enforcement.
  • Updated _indexes_from_der_path to validate each element and single-int paths, rejecting bool and out-of-range integers early.
  • Changed pub_key_derivation_tweaks to always parse the public key once (even for empty paths) and to map libsecp256k1’s ValueError into BTClibValueError, returning [] only for truly empty paths.
  • Extended tests for derive, der_path, pub_key_derivation_tweaks, and key_origin to assert new BTClibValueError failures on bad indices and to confirm boundaries and key_origin.assert_valid still catch post-construction mutations.
btclib/bip32/der_path.py
btclib/bip32/bip32.py
tests/bip32/der_path_test.py
tests/bip32/bip32_test.py
tests/bip32/key_origin_test.py
Utils, PSBT handling, and descriptor/miniscript logic now validate numeric/json inputs and PSBT structure earlier, and guard PSBT input indexing, preventing truncation of fractional json numbers, inadvertent negative indexing, and weight estimation of invalid PSBTs.
  • int_from_json_number now distinguishes type vs value errors, refuses bools and non-whole floats using is_integer, and rewraps TypeError/ValueError into BTClibTypeError/BTClibValueError with context.
  • Psbt.weight_estimate now calls assert_valid before computing vin/weight, ensuring incoherent PSBTs fail with BTClibValueError rather than producing fee-impacting estimates.
  • miniscript_solver adds an explicit vin_i range check mirroring update_psbt_input/output, avoiding negative-index behavior and IndexError from public entry points.
  • Tests cover json number coercion rules, invalid PSBT_GLOBAL_TX_MODIFIABLE on v0 PSBTs across weight_estimate/estimated_weight/estimated_vsize, and miniscript solver behavior on out-of-range indices.
btclib/utils.py
btclib/psbt/psbt.py
btclib/descriptors/descriptors.py
tests/utils_test.py
tests/psbt/psbt_size_test.py
tests/descriptors/miniscript_test.py
Network labeling and tx relabelling now resolve network names via network_from_name instead of raw string comparison, refusing unknown networks and handling variant mainnet spellings without relabelling, thereby avoiding transactions tagged with non-existent networks.
  • tx_for_network now uses network_from_name to normalize/validate the network parameter and compares the resolved Network object to mainnet.
  • Unknown names now raise BTClibValueError, and non-string inputs raise BTClibTypeError rather than building transactions with invalid network labels under check_validity=False.
  • Tests assert that mainnet aliases short-circuit correctly, invalid names/types are refused, and relabelled outputs remain otherwise untouched.
btclib/fetch/fetcher.py
tests/fetch/fetcher_test.py
Central integer-policy tests were expanded to cover the new index and numeric guards, ensuring bool-refusal policies do not accidentally reject valid integer inputs across addressed APIs while confirming BTClibTypeError is raised consistently for boolean values.
  • Extended the integer-policy test matrix with new cases for bech32 digits, mnemonic wordlist indices, number-theory operands/moduli, taproot leaf indices, and sig_hash vin indices.
  • Augmented the "integers a bool refusal must not take with it" section to assert that valid integer inputs still function for all newly-guarded APIs.
  • Ensured policy tests align with existing base58 and var_int behaviors to keep the global integer policy coherent.
tests/integer_policy_test.py
Changelog entry documents the twelve previously silent malformed-argument cases, summarizing their impact and the new behavior, tying them to issue #744 and clarifying error-class improvements for callers relying on BTClib exceptions.
  • Added a bullet under "The public API and the module layout" describing the unified shape of the twelve bugs and their corrected refusal behavior.
  • Included per-location explanations for script.sig_hash.taproot, script.taproot.input_script_sig/assert_valid_control_block, bech32.encode, bip32.der_path, bip32.pub_key_derivation_tweaks, descriptors.miniscript_solver, Psbt.weight_estimate, number_theory primitives, int_from_json_number, mnemonic entropy index handling, and fetcher.tx_for_network.
  • Clarified how prior behavior leaked Python exceptions (IndexError, OverflowError, ZeroDivisionError, TypeError) or produced silent wrong answers, and how new guards map them into BTClibValueError/BTClibTypeError.
CHANGELOG.md

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

Not an exception of the wrong class: no exception at all, and a value
the caller goes on to use.  `sig_hash.taproot` hashed an input index
past the end of the vin, two out-of-range indexes giving two different
hashes; `bech32.encode` indexed its alphabet from the end for a
negative digit and wrote another address; `input_script_sig` read -1 as
the last leaf and proved it; `indexes_from_der_path` handed `[-5]`
back; `pub_key_derivation_tweaks` never looked at the key for a path of
no steps; `miniscript_solver` solved the input at the other end;
`weight_estimate` estimated an incoherent psbt; `number_theory`'s five
answered a float with a float; `int_from_json_number` truncated 1.5;
`bin_str_entropy_from_wordlist_indexes` carried an index no word
answers to; `tx_for_network` baked a network no table has into every
output; `assert_valid_control_block` measured characters.

The first slice of issue #744, and the one it puts first: the only
category of that census that can cost a user money.

Two tests asserted the defect and now assert the fix, both in bip32:
`derive(xprv, 2**32)` was an `OverflowError` and is a
`BTClibValueError`, and `BIP32KeyOrigin`'s out-of-range index is
refused by the path reader before `assert_valid` sees it -- which still
asks, the field being a list the frozen dataclass cannot stop an append
to.

The bool half of the six new integer guards goes in
`tests/integer_policy_test.py`, beside the twenty-five parameters
already there rather than in six files.

Closes part of #744.
@fametrano
fametrano merged commit b7da259 into main Aug 13, 2026
7 of 8 checks passed
@fametrano
fametrano deleted the wrong-answers branch August 13, 2026 19:36
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