Repository navigation
pow%perf: provide alternate software AES backends for Groestl and SHAvite, precompute final block schedule, run NTT of 16-bit lanes for SIMD, unroll and inline BMW - #80
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 9 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
|
Note This pull request has no conflicts! 🎊 🎉 🎊 |
|
✅ Final review complete — no blockers (commit d03126f) · triage: critical |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification found no blocking defects and retained one nonblocking suggestion to exercise upper counter words in the rewritten SHAvite schedule. Exact-head CI passed MSRV feature tests, native/portable sphlib replay, stable platform builds, cross-builds, and code-lint steps; the overall Lint job failed only because its PR-title check received the nonconforming title “Refine hash.” No local builds or tests were run.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The diff makes extensive, intricate changes directly to cryptographic hash implementations, including AES backends in pkgs/pow/src/groestl/ and pkgs/pow/src/shavite/, NTT arithmetic in pkgs/pow/src/simd.rs, and the schedule in pkgs/pow/src/bmw.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `pkgs/pow/src/shavite/common.rs`:
- [SUGGESTION] pkgs/pow/src/shavite/common.rs:36-40: Cover upper counter words in the rewritten rolling schedule
Add a compression-level known-answer vector with distinct, nonzero values in all four HAIFA counter words, and exercise it under both AES backends. The NIST vectors cover messages through 255 bytes, the largest committed SHAvite corpus file is 55,341 bytes, and the differential sweep stops at 512 bytes, so ordinary CI cannot distinguish permutations of the upper three counter words. The ignored counter_word_carries test reaches only the first 32-bit carry even when explicitly enabled. A vector checked against Dash Core's compression function would validate the rewritten injection ordering and its propagation through all four rolling-schedule checkpoints without hashing enormous inputs. The current assignments match Core on inspection; this is regression coverage, not a demonstrated hash defect. Reference: https://github.com/dashpay/dash/blob/v24.0.0-rc.3/src/crypto/x11/shavite.cpp#L142-L173.
`git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
|
@kwvg on it — I've queued this as job |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The exact head implements performance-only X11 backend and schedule rewrites without an identified consensus, safety, or compatibility defect. The previously noted SHAvite counter-word coverage gap is fixed by a direct compression known-answer test using distinct nonzero values in all four counter words. All listed build, test, lint, and cross-platform checks succeeded at this head; CodeRabbit remains pending but has supplied no findings.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The diff intricately rewrites cryptographic proof-of-work implementations, including Groestl software AES in pkgs/pow/src/groestl/aes_cols.rs, SHAvite scheduling in pkgs/pow/src/shavite/common.rs, and SIMD NTT arithmetic in pkgs/pow/src/simd.rs, where correctness affects consensus hashes. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
…ations to `const` expressions (#91) * [63a9ee8] pow%refac(echo): load the message words with `load_u32s_le` (Kittywhiskers Van Gogh) * [01e798e] pow%refac(blake): load message words with `util::memops` (Kittywhiskers Van Gogh) * [f926cdf] pow%refac(luffa): share the scalar and vector step bodies via macros (Kittywhiskers Van Gogh) * [f22220c] pow%refac(util): drop unused definitions (Kittywhiskers Van Gogh) * [42a58c5] pow%refac(luffa): pack round constants with `const_array!` (Kittywhiskers Van Gogh) * [88f292c] pow%refac(jh): fold the bit-group swaps into one function (Kittywhiskers Van Gogh) * [87a1e6b] pow%refac(jh): generate tables with `const_array!` (Kittywhiskers Van Gogh) * [cbda3b0] pow%doc: reword comments (Kittywhiskers Van Gogh) * [f320850] pow%refac(cubehash): build round index tables as xor masks (Kittywhiskers Van Gogh) * [15c9fc1] pow%refac(jh): derive `IP_8` and `IV` from the spec permutation (Kittywhiskers Van Gogh) * [a8d99bd] pow%refac(echo): derive the big ShiftRows permutation as 5i mod 16 (Kittywhiskers Van Gogh) * [a061d37] pow%refac(blake): build sigma rounds 10 to 15 from rows r mod 10 (Kittywhiskers Van Gogh) * [a61617c] pow%refac(skein): derive the IV by UBI of the config block (Kittywhiskers Van Gogh) * [d49f11d] pow%refac(shavite): load the message window with `load_u32s_le` (Kittywhiskers Van Gogh) Description: ````` ## Additional Information * Depends on #80 ## Breaking Changes None expected ## How Has This Been Tested? ``` just comb ``` ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional tests - [ ] I have made corresponding changes to the documentation - [x] I did not use agentic assistance (i.e. LLMs, AI) except as [disclosed](#agent-disclosure) and can answer questions, comments and concerns without said assistance. I assume authorship of the code submitted. - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ### Agent disclosure Code generated with Claude Code using Opus 5.5, medium effort. Reviewed with Codex using GPT-6-Astra, extra high effort as adversarial review and Claude Code with Fable 5.1, xhigh effort. `````
Motivation
The system requirements imposed by
dash-poware aimed at full nodes and other server-side applications but are unacceptable for mobile devicesFor instance, targeting iOS sets
+apple-a7by default which supports AES acceleration (source) but targeting Android sets+v8awhich treats it as optional (the whole situation around ARM extensions is troublesome on the best of days, see BoringSSL guidance for a general summary, source).A minimum requirement for transitioning away from
sphlib-based FFIs is performance at-par or better than the existing implementation. This means we had to rework theportable(i.e. software-only) backend to achieve performance improvements to justify the switchover for downstream consumers.Additional Information
The Groestl software backend builds the fused round column-first, holding the state as sixteen
u64columns and running each round with one table of SubBytes and the MixBytes column, rotated by8rbits for rowr.This is the single-table variant of the Groestl specification (source, section 8.1.2), where the circulant MixBytes matrix makes
T1throughT7byte rotations ofT0, and the T-table method itself originates in the Rijndael proposal (source, section 5.2.1).The hardware backend keeps the row-first construction of the specification's AES-NI strategy (source, section 8.1.1) and of Byte Slicing Groestl (source).
The SHAvite-3 software AES rounds run as T-table lookups on plain
u32words, with each key after the first entering as the key operand of the round before it, which is the keyed T-table round of the Rijndael proposal (source, section 5.2.1).The SHAvite-3 message expansion is generated inside the round loop from an eight-bundle window instead of a precomputed 112-entry schedule. The recurrence defined in the SHAvite-3 submission only reaches eight bundles back and the rounds consume bundles in order (source, section 4.2.2), so the window holds the whole live schedule.
Skein unrolls Threefish, four rounds per subkey injection, with the word permutation turned into register renaming and every rotation an immediate. The Skein paper describes exactly this, noting that the period-4 permutation lets an implementation use different registers each round instead of shuffling words (source, sections 7.1 and 8.3).
SIMD precomputes the message schedule of the length-only final block of a 64-byte message, skipping that block's NTT. The block is fixed because it carries only the bit count (source, section 1.2.4). The schedule is generated at compile time with a
const fn.SIMD runs its 256-point NTT over
Z/257on 16-bit lanes as eight parallel FFT-8 passes with transposes, reducing only where a lane could leavei16. SWIFFT is the earliest source of the method, with eight 8-point FFTs across SSE2 registers of signed 16-bit lanes, twiddles as shifts by 2, 4 and 6, and(x & 255) - (x >> 8)reducing only a few intermediate values into[-127, 383](source, source). The SIMD submission adopts the same field for that reason (source, section 2.1).BMW writes its message expansion and folding with literal indices, so every index and add/sub sign of the expansion defined in Blue Midnight Wish is a compile-time constant (source, listed only).
How Has This Been Tested?
Checklist
Agent disclosure
Code generated with Claude Code using Opus 5.5, medium effort and Fable 5.1, xhigh effort. Reviewed with Codex using GPT-6-Astra, extra high effort as adversarial review.