Repository navigation
feat(hashes)!: switch proof of work backend to dash-pow, bump Base SDK crates to dashpay/base-sdk@addedfa - #1214
Conversation
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
dash-pow, bump Base SDK crates to dashpay/base-sdk@addedfadash-pow, bump Base SDK crates to dashpay/base-sdk@addedfa
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
|
Bots are done — your move: address xdustinface requested changes, then post |
|
/self-reviewed |
xdustinface
left a comment
There was a problem hiding this comment.
@kwvg the Ubuntu / spv, Ubuntu / ffi and Windows / spv failures come from this PR, not from flakiness. Unoptimized dash-pow takes about 1.1 ms per X11 hash on x86_64, against about 50 µs for rs-x11-hash. Header decompression then blocks the network reader for seconds at a time, and the dashd tests time out. The hashes are correct. Adding this to the root Cargo.toml fixes it:
[profile.dev.package.dash-pow]
opt-level = 3| Measurement | dev (rs-x11-hash) |
PR | PR + override |
|---|---|---|---|
| X11 per header, debug, x86_64 (Rosetta) | 44 to 70 µs | 1113 to 1122 µs | 17 to 20 µs |
X11 per header, debug + llvm-cov, aarch64 |
57 µs | 443 µs | 17 µs |
| X11 per header, release, aarch64 | 8.4 to 10.3 µs | 8.6 to 10.7 µs | same as PR |
dash-spv dashd suite (35 tests) |
all pass, 133 s | 3 fail, 209 s | all pass, 94 s |
dash-spv-ffi dashd suite (7 tests) |
all pass, 51 s | all pass, 86 s | all pass, 52 s |
The suites ran on aarch64 with portable forced to match CI's x86_64 path. All variants produce identical hashes. The override adds about 3 to 5 s to compile dash-pow once, and rust-cache keeps that build between runs.
Follow-ups, not blocking:
- Cargo ignores profile overrides from dependencies, so downstream crates need the same override in their own workspace. Worth a changelog note.
dash-powselects hardware AES at compile time only, so default x86_64 builds never use AES-NI. Runtime detection inbase-sdkwould fix that.
Co-authored-by: xdustinface <xdustinfacex@gmail.com>
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
There was a problem hiding this comment.
🔇 Additional comments: 3
Cargo.toml: 1
49-51: LGTM!CHANGELOG.md: 1
18-18: LGTM!Also applies to: 75-83
CHANGELOG.md-86-86: 1
86-86: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Fix the verb agreement in the X11 entry.
Use “
rs-x11-hashalways reads 80 bytes.”Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: dashpay/rust-dashcore
- Branch: dev
- File: .coderabbit.yaml
- Commit: 6baf1d3
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Advanced
- Run ID:
5a366007-cc26-417b-8db4-bcea5c959b4f
📒 Files selected for processing: 2
CHANGELOG.mdCargo.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.
|
Bots are done — your move: address xdustinface requested changes, then post |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1214 +/- ##
==========================================
- Coverage 78.96% 78.68% -0.28%
==========================================
Files 304 304
Lines 80988 80998 +10
==========================================
- Hits 63951 63733 -218
- Misses 17037 17265 +228
|
|
/self-reviewed |
|
Ready for review — files with no dedicated owner: QuantumExplorer or ZocoLini or xdustinface · re-review or resolve: xdustinface. |
Additional Information
As
base-sdkcrates are expected to share the same revision, they've been bumped to the latestdevelopas of this writing (addedfa1). This has the effect of requiring updates to existing callers due to base-sdk#61 and base-sdk#62.The majority of performance improvements associated with
dash-powcome from support for hardware accelerated AES and SIMD extensions for x86_64 and ArmV8. For more information, see base-sdk#80.portableis deliberately all-or-nothing, having droppedaes_hwas a separate feature in base-sdk#66. Every additional feature is another configuration to fuzz and validate on any change or optimisation, and the benefits thereof are too narrow.wideonly supports acceleration for x86_64, Aarch64 and WASM (source)On AArch64, AES requires NEON, the hardware path asks for nothing beyond AES. A separate AES switch cover the exact same chips.
x86_64 chips are a bit different, Intel artificially restricted AVX/AVX2 to the Core and Xeon lineups till 2020 (source) so it is plausible for a chip to have AES-NI but lack AVX/AVX2. Unfortunately, dropping down from
x86_64-v3(source) would hamper performance on all chips Haswell and older, so they getportablealongside pre-Haswell chips.Benchmarks
On an Apple M4 Pro, rustc 1.99.1 for hashing an 80-byte input using
criterion.rs-x11-hashrs-x11-hash0.1.9 (sphlib C,-O2)portableOn an Intel Xeon E3-1275 v6, rustc 1.99.1 for hashing an 80-byte input using
criterion.rs-x11-hashrs-x11-hash0.1.9 (sphlib C,-O2)portablePR Hygiene ·
da224eeCHANGELOG.md,Cargo.toml,crypto/src/bls.rsand 4 more) — QuantumExplorer or ZocoLini or xdustinfaceWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.