Conversation
|
Review: changes needed before merge. Please add a like-for-like comparison against the direct checked byte splitter already merged in #34. This PR's 16-byte profile spends 1,168 script bytes and 546 combined stack items, including a 512-item table. Show the batch size at which amortization wins, including routing, output restoration and table cleanup on both sides. The direct scalar size alone is not a complete batch baseline, so I am not claiming this table is universally dominated; the evidence needed to justify adding the second implementation is missing. Evidence: |
07ba448 to
4e2ccb2
Compare
|
Addressed the review points and rebased the implementation onto current Changes:
Validation passed: focused unpack tests, the exact unpack metric fixture, |
|
Review of The direct-splitter comparison and strict preserved-state boundary cover the previous concerns. Resolve the module/catalog/README overlaps while retaining the new popcount and byte-plane entries, then rerun unpack correctness, scalar comparison metrics, and KB validation. Keep the table's byte/stack tradeoff explicit. Current integration conflicts: Validation scope: source/diff and existing CI review; no new full-repository or field-arithmetic test run was requested. |
…/byte-plane overlaps Integrates main's 27-PR batch (solving-bitcoin#244, at 3d1001f) with feat/u4-unpack. Conflicts resolved: - knowledge/catalog.json: merged the shared "arithmetic/u4" record's stack_contract sentence so it documents both the byte unpacker (PR) and the per-nibble/total popcount projections (main, solving-bitcoin#224). - knowledge/techniques/representations.md: kept both the u4 byte-unpacker paragraph and the u32 byte-plane adapter paragraph, moving the latter next to the other u32 mask paragraphs; corrected an inaccurate "table wins at larger batches" claim that was not backed by any measurement. - src/arithmetic/u4/README.md: kept both the unpack bullet (PR) and the threshold/clamp/equality bullets plus the updated lsb caveat (main); corrected an arithmetic error in the byte/stack tradeoff prose (the table path was described as "smaller by 32 bytes" when it is actually 102 bytes larger at the only measured batch size). - src/arithmetic/u4/mod.rs: sorted union of every `pub mod` from both sides (48 modules total; verified head and main are each full subsets and the union matches exactly). - tests/primitive_metrics.rs: kept both `u4_unpack_metrics_are_current` (PR) and `tapbranch_metrics_are_current` (main), independent tests that were inserted at the same line. Also corrected the same unsupported "table path trades stack occupancy for a smaller repeated-batch script" claim in knowledge/primitives/u4.md, and re-grounded all four locations (README, knowledge page, representations.md) in the only measured comparison (16 bytes: table 1,168 bytes/546 items vs. scalar 1,066 bytes/34 items), explicitly declining to claim a crossover batch size that isn't measured in this repository. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed on |
Cover both invalid byte bounds at every witness position and add a strict valid-input state-preservation control. Record matched table/scalar measurements at 16, 18, 19, 32, and 243 inputs, preserving existing snapshots and deployment qualifiers.
2 participants
Summary
Adds a checked byte-to-nibble converter that shares a 512-item high/low lookup table across a batch, preserves input order, and rejects values outside
0..=255. The standalone limit is 243 byte inputs; unrelated live state reduces that limit.Matched policy-compiled table/scalar fixtures include validation and output staging/restoration. Script bytes exclude the identical terminal harness; strict combined stack peaks include it. Both paths use one witness data item per byte and zero hint items.
The measured script-byte crossover is between 18 and 19 inputs. The scalar path uses 512 fewer combined stack items at every measured width. Static non-push opcode counts and the exact measurement boundary are recorded in the README and catalog; these are fragment measurements, excluding transaction context.
Correctness and evidence
-1,256, and oversized ScriptNums at every position in a three-byte witness; verify valid canonical witness inputs and preserved main/alt-stack state.256in the third input position; the production guard is restored.locally-reproduced. Deployment:unclassified; strict local tapscript execution is not Bitcoin Core differential validation.Validation
cargo test --locked --lib arithmetic::u4::unpackcargo test --locked --test primitive_metrics u4_unpack_metrics_are_current -- --exactpython3 tools/kb.py validatecargo fmt --all -- --checkcargo test --locked -- --skip fields::Rust validation uses the repository CI environment,
CARGO_PROFILE_DEV_OPT_LEVEL=1 CARGO_PROFILE_TEST_OPT_LEVEL=1, retaining debug assertions and overflow checks. Existing metric values are unchanged; the additional table and scalar rows are intentional measurements.