fix: correct varint boundary off-by-one in SatoshisPerKilobyte fee model - #190
Open
ALiberalVoluntarist wants to merge 4 commits into
Open
fix: correct varint boundary off-by-one in SatoshisPerKilobyte fee model#190ALiberalVoluntarist wants to merge 4 commits into
ALiberalVoluntarist wants to merge 4 commits into
Conversation
get_varint_size used `> 253` / `> 2**16` / `> 2**32` which under- estimated by 2–4 bytes when a script length landed exactly on a varint boundary (253, 65536, 4294967296). Aligned to `> 0xFC` / `> 0xFFFF` / `> 0xFFFFFFFF`, matching the canonical encoder in binary.py and the SV Node / Teranode / Go SDK implementations. Also fixed pre-existing test failures in fee_models_test_coverage.py (wrong constructor kwarg `rate=` → `value=`, and int passed instead of Transaction to compute_fee). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ruff 0.16.0 enables PLR0917 by default, which flags 41 existing functions. This mirrors PLR0913 (too-many-arguments) already ignored. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ALiberalVoluntarist
force-pushed
the
fix/varint-boundary-off-by-one
branch
from
July 27, 2026 07:11
8d0c439 to
b17e8f9
Compare
Test Coverage Report📊 Coverage: 80.7% View the full coverage report in the build artifacts. |
1 similar comment
Test Coverage Report📊 Coverage: 80.7% View the full coverage report in the build artifacts. |
- C extension tx_to_bytes/tx_preimages/tx_preimage_otda: replace silent uint32 truncation (PyArg_ParseTuple "I") with explicit parse_uint32() that raises OverflowError for out-of-range version/locktime values - C extension: reject non-bytes script types with TypeError instead of silently treating them as empty; validate source_txid is exactly 64 hex characters; check for missing dict keys with descriptive errors - Python side: introduce txid_to_bytes_le() in transaction_input.py with regex validation; replace all bytes.fromhex(*.source_txid)[::-1] call sites (serialize, preimage, BEEF) with the validated helper - Transaction.serialize(): add type(i) is TransactionInput check so subclassed inputs/outputs fall back to Python path, preserving serialize() overrides regardless of C extension availability - Add 190-test edge case suite covering varint boundaries, large NFT scripts (up to 20MB), all 12 sighash combinations, error rejection on both C and Python paths, and subclass fallback verification - Add Hypothesis differential tests verifying C/Python equivalence for serialize + BIP143/OTDA preimage with random transactions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
get_varint_sizeinSatoshisPerKilobyte.compute_feeused> 253/> 2**16/> 2**32, underestimating varint size by 2–4 bytes when a script length landed exactly on a Bitcoin varint boundary (253, 65536, 4294967296). Fixed to> 0xFC/> 0xFFFF/> 0xFFFFFFFF, aligning with the canonical encoder inbinary.pyand the SV Node / Teranode / Go SDK implementations.fee_models_test_coverage.py(wrong constructor kwargrate=→value=, andintpassed instead ofTransactiontocompute_fee).unsigned_to_varintat all boundaries.Cross-SDK status
serialize.h)< 253/<= uint16_max/<= uint32_maxutil/varint.go)< 0xfd/<= 0xffff/<= 0xffffffffutil/varint.go)< 253/< 65536/< 4294967296binary.py(encoder)<= 0xFC/<= 0xFFFF/<= 0xFFFFFFFFSatoshisPerKilobyte> 253/> 2**16/> 2**32SatoshisPerKilobyte.ts> 253/> 2**16/> 2**32Test plan
pytest tests/bsv/fee_model_test_coverage.py -v— all 15 tests pass including 4 new boundary regression testspytest tests/bsv/fee_models_test_coverage.py -v— all 7 tests pass (previously 4 failed)🤖 Generated with Claude Code