num%fix!: match reference implementation in overloading, parsing, rounding and block proof checks, partially delegate Arith256 to bitcoin-internals

[kwvg]

Additional Information

• Courtesy of rust-bitcoin#6715, bitcoin-internals 0.7 has made U256 reachable when historically it was not in public scope (source). This historic reason was why the dash-num crate came into existence instead of benefiting from upstream types.

  But now that is no longer a blocker, we can effectively prune the majority of our Arith256 implementation by simply delegating to rust-bitcoin's implementation, pruning code ownership and the test suite to only sections where we deviate from rust-bitcoin due to our no_std and panic-free mandates.

• As Arith256 aims to match Dash Core in behaviour where feasible, downstream consumer preparation identified deviations that have since been remedied.

  To pin the expected behaviour, a new corpus has been introduced, arith256.json5, with C++ test programs used to generate the vectors using Dash Core v24.0.0-rc.1 and vectors that failed before the fixes were applied were retained in the corpus to avoid bloating the corpus with trivially passing vectors (i.e. the corpus is a subset of the generated values).

  • Though lossy/opinionated parsing mostly gains its value when parsing external input (e.g. RPC), it should not be used when expecting correctly formed input (e.g. when reading from test corpora or from disk). Since the stricter of the two is already an existing API, lossy parsing is given its own definition.

• Division where the divisor is zero returns zero instead of panicking. To detect a zero-divisor instead of silently returning zero, use checked_div().

Breaking Changes

See changelog.

How Has This Been Tested?

./contrib/git_filter.py --fast-fail develop arith256 -- bash -c 'cargo clippy --all-targets --no-default-features -- -D warnings && cargo clippy --all-targets --features full -- -D warnings && cargo test --all-targets --features full && ./maint/lint_all.py'

Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: c447422b-da3b-416e-845d-e5f74d9c44d2

📥 Commits

Reviewing files that changed from the base of the PR and between e6402ce and 1ba2c44.

⛔ Files ignored due to path filters (2)

• Cargo.lock is excluded by !**/*.lock, !**/*.lock
• pkgs/num/corpus/arith256.json5 is excluded by !**/*.json5

📒 Files selected for processing (13)

• Cargo.toml
• pkgs/num/CHANGELOG.md
• pkgs/num/Cargo.toml
• pkgs/num/src/arith256.rs
• pkgs/num/src/compact.rs
• pkgs/num/src/hash.rs
• pkgs/num/src/lib.rs
• pkgs/num/src/prelude.rs
• pkgs/num/src/util.rs
• pkgs/num/tests/arith.rs
• pkgs/num/tests/compact.rs
• pkgs/p2p_core/Cargo.toml
• pkgs/primitives/Cargo.toml

💤 Files with no reviewable changes (4)

• pkgs/p2p_core/Cargo.toml
• pkgs/num/tests/compact.rs
• pkgs/num/tests/arith.rs
• pkgs/primitives/Cargo.toml

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The num crate now wraps bitcoin-internals’s U256 in Arith256. It adds lossy hexadecimal parsing and CompactTarget::block_proof, updates compact-target parsing, and adds tests for these changes.

Changes

Numeric primitives

Layer / File(s)	Summary
Dependency and crate setup Cargo.toml, pkgs/num/Cargo.toml, pkgs/p2p_core/Cargo.toml, pkgs/primitives/Cargo.toml, pkgs/num/src/lib.rs, pkgs/num/src/prelude.rs	Workspace dependency versions and declarations change. The num crate enables bitcoin-internals/std and adds a private prelude for alloc types. p2p_core and primitives remove their bitcoin-internals dependency declarations.
Arith256 representation and operations pkgs/num/src/arith256.rs, pkgs/num/Cargo.toml, pkgs/num/CHANGELOG.md, pkgs/num/tests/arith.rs	Arith256 wraps U256; its arithmetic, conversions, bitwise operations, formatting, and numeric helpers are updated. The changes add scalar u64 operators and revise to_f64. Arithmetic tests are added to the source module, and the former integration test target and file are removed.
Hex parsing and compact-target proofs pkgs/num/src/hash.rs, pkgs/num/src/util.rs, pkgs/num/src/compact.rs, pkgs/num/CHANGELOG.md, pkgs/num/tests/compact.rs	HashBlob and generated hash types gain lossy hexadecimal parsing. CompactTarget uses the shared hex reader and gains block_proof. Tests are added in the source module, and the former compact integration test target and file are removed.

Priority: ⬇️ Low

Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 1ba2c

This change moves Arith256 onto the shared U256 type and adds lossy hex parsing and block-proof APIs, with tests for each. No concrete defect has been identified. Before merging, confirm that the author's clippy and test command passes, because it covers the upstream formatting and hex-reader behavior.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 1ba2c

The examined validation checks remain intact, and permissive parsing is introduced as an explicit opt-in API rather than replacing strict parsing. No introduced security failure was demonstrated. External callers and the full path from network input to validation remain insufficiently established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The meaningful security exposure is numeric correctness in library consumers, including block proof-of-work validation. Manifest dependencies extend the potential propagation to primitives and P2P core, but do not establish an attacker-reachable network call chain or deployment-wide impact. No tenant, credential or infrastructure authority expansion is established by the examined changes.

Trust Boundaries and Controls

• observed — The examined validation boundary continues to reject invalid decoded targets before proof-of-work comparison. Permissive textual parsing remains separately named and opt-in; existing hash string deserialization retains its error-returning parser. These are concrete counterevidence to a claim that this PR replaces existing validation with lossy coercion.

Resilience and Maintainability Implications

• observed — The work calculation filters invalid compact encodings and handles ZERO and MAX explicitly. For other targets, target plus one is nonzero, preventing a zero denominator in this path. These guards preserve local failure containment; they do not establish network-specific difficulty policy or complete consensus validation.

Hardening Proposals

• proposed — Keep from_hex_lossy out of security-sensitive input validation unless coercion and truncation are explicitly intended by the caller's contract. Preserve error-returning parsing at those boundaries. This is a usage safeguard, not an observed bypass.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 90.99% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 111 functions across 6 files. (3 skipped: 3…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The description clearly explains the Arith256 alignment, delegation to bitcoin-internals, behavioral fixes, testing, and breaking-change documentation.
Title check	✅ Passed	The title clearly identifies the main changes: matching the reference implementation and partially delegating Arith256 to bitcoin-internals.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Note

This pull request has no conflicts! 🎊 🎉 🎊

[kwvg]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.