Skip to content

Check every width a sig_hash preimage writes, and every index - #764

Merged
fametrano merged 4 commits into
mainfrom
sig-hash-widths
Aug 13, 2026
Merged

Check every width a sig_hash preimage writes, and every index#764
fametrano merged 4 commits into
mainfrom
sig-hash-widths

Conversation

@fametrano

@fametrano fametrano commented Aug 13, 2026 •
edited by sourcery-ai Bot
Loading

Copy link
Copy Markdown
Member

The third slice of #744.
Stacked on #763 — its
_assert_valid_vin_i is what this reuses, and GitHub retargets this to
main when that one merges. Review the two commits, not the whole diff.

What was open

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. Two things it
did not close:

  • segwit_v0 and taproot assemble their preimage from their own
    to_bytes calls and never reach Tx.serialize, so all three widths
    leaked there.
  • TxIn.serialize and TxOut.serialize check nothing when told not to,
    so legacy kept leaking on a different field: the sequence, the
    output value, and the vout of the outpoint each input names.

And vin_i was unbounded in all four entry points.

The shape of the fix

The checks go in the serializations, not in the callers:

helper the write it stands in front of
_serialized_4_byte_field version, lock time, sequence, vout
_serialized_camount the spent amount and the output value
_serialized_out_point an outpoint, its vout checked
_serialized_output an output, its CAmount checked
_serialized_spend_type BIP341's 2 * ext_flag + annex_present

So PrecomputedTxData and both the sha_ and hash_ families get
them for free, and no future caller has to remember. legacy checks
what its branches leave behind rather than what it was handed: NONE
drops every output, 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.

_assert_valid_4_byte_field is imported from btclib.tx.tx rather than
written a second time — same field width, and a second spelling is a
second place to read before believing they agree.

Worth a reviewer's eye

  • from_tx now refuses a prevout list of a different length from the
    vin.     It indexes both at vin_i. PrecomputedTxData refuses the
    same mismatch with the same message, and verify_transaction before
    it; from_tx was the one public door without it.
  • _serialized_spend_type is an extraction, not only a check.
    taproot was one branch under C901's ten, and the two ext_flag
    checks put it over. Extracting is what pyproject's mccabe comment
    prefers to a noqa, and it matches the pattern above.
  • annex and message_extension are Octets now, which is what
    they always accepted through var_bytes.serialize — except
    message_extension, concatenated raw, where a hex string met
    b"".join and answered a TypeError about the join.
  • The vout of an outpoint is not in the census's sig_hash entries; it
    is the same leak one field along, in loops this walks anyway. The rest
    of tx/ and block/ is a slice of 88 public functions still let a malformed input through: the census #684 left open #744 still open.

Gates

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

Summary by Sourcery

Ensure all signature-hash preimages validate field widths and input indices consistently across legacy, segwit v0, and taproot sighash implementations.

Bug Fixes:

  • Prevent OverflowError and IndexError leaks from sighash preimage assembly by validating 4-byte fields, CAmount values, outpoint vout, and input indices.
  • Reject mismatched prevout lists in sighash.from_tx to avoid hashing amounts from one transaction into another’s sighash.
  • Treat taproot message_extension as generic octets and validate the spend-type extension flag range and type to avoid type and overflow errors.

Enhancements:

  • Introduce shared serialization helpers for 4-byte fields, CAmount values, outpoints, outputs, and taproot spend type so sighash-related code and precomputed data reuse consistent width checks.

Documentation:

  • Document the new sighash width and index validation behavior and related error conditions in the changelog.

Tests:

  • Add comprehensive tests covering width checks for all fields used in legacy, segwit v0, and taproot preimages, input index validation across sighash entry points, prevout list length mismatches, taproot spend-type flag bounds, and octets handling for message_extension.

sourcery-ai[bot]
sourcery-ai Bot reviewed Aug 13, 2026
View reviewed changes

@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 signature-hash preimage construction by centralizing width checks for all serialized integer fields and validating input indices and prevout list lengths across legacy, segwit_v0, and taproot sighash helpers, plus adds focused tests and changelog entries documenting the behavior.

Flow diagram for centralized width checks in sighash preimage serialization

flowchart LR
  subgraph Helpers
    H4[_serialized_4_byte_field]
    HC[_serialized_camount]
    HO[_serialized_out_point]
    HOut[_serialized_output]
    HSpend[_serialized_spend_type]
  end

  L[legacy]
  S[segwit_v0]
  T[taproot]
  P[PrecomputedTxData]

  L --> H4
  L --> HC

  S --> H4
  S --> HC
  S --> HO
  S --> HOut

  T --> H4
  T --> HC
  T --> HO
  T --> HOut
  T --> HSpend

  P --> HO
  P --> H4
  P --> HOut
  P --> HC

  H4 -->|uses| assert4[_assert_valid_4_byte_field]
  HC -->|uses| assertC[_assert_valid_camount]
Loading

File-Level Changes

Change Details Files
Introduce shared serializers that enforce width checks for 4‑byte fields, CAmount values, outpoints, and outputs and reuse the common 4‑byte field validator.
  • Add _serialized_4_byte_field wrapper that calls _assert_valid_4_byte_field before serializing int32 fields.
  • Add _serialized_camount to validate and serialize 8‑byte signed CAmount values consistently with TxOut.serialize.
  • Add _serialized_out_point to validate OutPoint.vout width then delegate to OutPoint.serialize with check_validity=False.
  • Add _serialized_output to validate TxOut.value width then delegate to TxOut.serialize with check_validity=False.
  • Reuse _assert_valid_4_byte_field from btclib.tx.tx instead of duplicating width validation logic.
 btclib/script/sig_hash.py
Apply the new serializers and index validation throughout legacy, segwit_v0, taproot, and from_tx sighash paths so every preimage write and vin index is checked.
  • Validate vin_i via _assert_valid_vin_i in legacy, segwit_v0, taproot_annex_and_ext, and from_tx before indexing tx.vin.
  • In legacy(), validate sequence, prev_out.vout, and output value on the copied transaction according to the active hash type before calling Tx.serialize.
  • Replace direct to_bytes calls for version, lock_time, sequences, amounts, outpoints, and outputs in segwit_v0 with the new helpers.
  • Ensure taproot preimage uses _serialized_4_byte_field for nVersion/nLockTime, _serialized_spend_type for the spend-type byte, and width-checked outpoint/amount/sequence when ANYONECANPAY.
  • Have from_tx enforce prevouts list length equality with tx.vin and raise BTClibValueError on mismatch.
 btclib/script/sig_hash.py
Refine taproot parameter handling by bounding ext_flag, treating annex and message_extension as Octets, and normalizing message_extension via bytes_from_octets.
  • Change taproot() signature so annex and message_extension are Octets instead of bytes.
  • Normalize message_extension with bytes_from_octets before concatenation to avoid TypeError from b"".join on hex strings.
  • Introduce _serialized_spend_type to validate ext_flag’s type and range (0..0x7F) and compute the spend-type byte safely.
  • Update tests to cover ext_flag range/type errors and Octets handling for message_extension.
 btclib/script/sig_hash.py
tests/script/sig_hash_taproot_test.py
Expand test coverage to assert all new width checks, index checks, and error classes for legacy, segwit_v0, taproot, taproot_annex_and_ext, and from_tx.
  • Add taproot tests ensuring version, lock_time, sequence, vout, and output value width violations raise BTClibValueError and that valid cases still produce 32‑byte sighashes.
  • Add taproot tests for spend-type ext_flag boundaries and type validation, and for Octets behavior of message_extension.
  • Extend legacy tests to cover sequence, vout, and output value width checks, NONE behavior with uncommitted outputs, and vin_i validation (including type errors).
  • Extend segwit_v0 tests to cover width checks on version, lock_time, sequence, vout, output value for SINGLE, and vin_i index validation.
  • Add from_tx tests asserting vin_i bounds and mismatched prevouts/vin length errors.
  • Import BTClibTypeError where needed in tests to assert type-error paths.
 tests/script/sig_hash_taproot_test.py
tests/script/sig_hash_legacy_test.py
tests/script/sig_hash_segwitv0_test.py
Document the new sighash width and index validation behavior in the changelog. 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

This was referenced Aug 13, 2026
Merged
Merged
@fametrano fametrano mentioned this pull request Aug 13, 2026
Closed
Base automatically changed from wrong-answers to main August 13, 2026 19:36
fametrano and others added 2 commits August 13, 2026 21:41
@fametrano
Check every width a sig_hash preimage writes, and every index
21c7fd6 
`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.
@fametrano
Ask a value what it is before asking what it holds (#765)
3494e1c 
* 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
fametrano force-pushed the sig-hash-widths branch 2 times, most recently from 3c2a92c to 3494e1c Compare August 13, 2026 19:43
@fametrano fametrano mentioned this pull request Aug 13, 2026
Merged
@fametrano
Gate the input-validation rule, and let the gate enumerate (#774)
96597c0 
`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.
@fametrano
Merge branch 'main' into sig-hash-widths
b67d053
@fametrano
fametrano merged commit 433dd59 into main Aug 13, 2026
7 of 8 checks passed
@fametrano
fametrano deleted the sig-hash-widths branch August 13, 2026 19:53
This was referenced Aug 13, 2026
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sourcery-ai sourcery-ai[bot] sourcery-ai[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@fametrano