Feature/full implementation - #8
Open
ufomasters77-ship-it wants to merge 11 commits into
Open
Conversation
… wallets, generators, CMake, configs, README
…putation + early-exit) with test harness; update CMake to build tests
…CL guarded), stubbed check_batch; prepare for full GPU PBKDF2 implementation
…into gpu_engine, update CMake to compile CUDA kernel when ENABLE_CUDA=ON
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (57)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
…ic (compute capability/multiprocessors), expose selected_device/name, and forward check_batch to backend-specific implementations; tuned for older GPUs (e.g., GTX 750 Ti).
PR Summary by QodoAdd CPU+CUDA PBKDF2-HMAC-SHA512 path, GPU probing, and skeleton recovery engine
AI Description
Diagram
High-Level Assessment
Files changed (43)
|
…d forward prototypes; update CMake to find OpenCL and build OpenCL/CUDA pipeline
Code Review by Qodo
1. Unconditional CUDA sources
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+17
to
31
| file(GLOB_RECURSE SRC_FILES src/*.cpp src/*.c src/*.cu) | ||
|
|
||
| set(UTILS_SOURCES | ||
| src/utils/crypto_utils.cpp | ||
| src/utils/file_utils.cpp | ||
| src/utils/string_utils.cpp | ||
| src/utils/logger.cpp | ||
| ) | ||
| # Main binary | ||
| add_executable(recovery ${SRC_FILES} src/gpu/cuda_kernel.cu) | ||
|
|
||
| set(GPU_SOURCES) | ||
| if(CUDA_FOUND) | ||
| set(GPU_SOURCES ${GPU_SOURCES} | ||
| src/gpu/cuda_recovery.cu | ||
| src/gpu/cuda_utils.cu | ||
| src/gpu/cuda_integrated.cpp | ||
| ) | ||
| endif() | ||
| target_link_libraries(recovery PRIVATE OpenSSL::SSL OpenSSL::Crypto Threads::Threads) | ||
|
|
||
| if(OpenCL_FOUND) | ||
| set(GPU_SOURCES ${GPU_SOURCES} | ||
| src/gpu/opencl_recovery.cpp | ||
| src/gpu/opencl_utils.cpp | ||
| src/gpu/integrated_gpu.cpp | ||
| ) | ||
| if(NOT MSVC) | ||
| target_compile_options(recovery PRIVATE -Wall -Wextra -Wpedantic) | ||
| endif() | ||
|
|
||
| # Main executable | ||
| add_executable(btc-recovery | ||
| src/main.cpp | ||
| ${CORE_SOURCES} | ||
| ${WALLET_SOURCES} | ||
| ${UTILS_SOURCES} | ||
| ${GPU_SOURCES} | ||
| ) | ||
|
|
||
| # Link libraries | ||
| target_link_libraries(btc-recovery | ||
| ${CMAKE_THREAD_LIBS_INIT} | ||
| ${OPENSSL_LIBRARIES} | ||
| ${CURL_LIBRARIES} | ||
| ${JSONCPP_LIBRARIES} | ||
| ) | ||
|
|
||
| if(CUDA_FOUND) | ||
| target_link_libraries(btc-recovery ${CUDA_LIBRARIES}) | ||
| if(ENABLE_CUDA) | ||
| enable_language(CUDA) | ||
| target_compile_definitions(recovery PRIVATE ENABLE_CUDA=1) | ||
| endif() |
There was a problem hiding this comment.
1. Unconditional cuda sources 🐞 Bug ☼ Reliability
CMakeLists.txt always adds .cu sources (and src/gpu/cuda_kernel.cu) even when ENABLE_CUDA is OFF, so default CPU-only builds can fail because the CUDA language/toolchain isn’t enabled. enable_language(CUDA) is only executed conditionally after the target is already created.
Agent Prompt
### Issue description
`recovery` includes CUDA (`.cu`) sources unconditionally, but CUDA is only enabled when `ENABLE_CUDA` is ON. This breaks CPU-only builds.
### Issue Context
- `file(GLOB_RECURSE ...)` includes `src/*.cu` regardless of options.
- `add_executable(recovery ... src/gpu/cuda_kernel.cu)` adds a `.cu` file unconditionally.
- `enable_language(CUDA)` is executed only inside `if(ENABLE_CUDA)` and after target creation.
### Fix Focus Areas
- CMakeLists.txt[17-35]
### Suggested fix
- Move `enable_language(CUDA)` before any `.cu` sources are added.
- Only glob/add `.cu` sources when `ENABLE_CUDA` is ON (or split `SRC_FILES` into CPU vs CUDA lists).
- Only add `src/gpu/cuda_kernel.cu` when CUDA is enabled.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+83
to
+90
| bool gpu::GPUEngine::is_available() const { return p->available; } | ||
|
|
||
| #include "src/gpu/pbkdf2_cuda.h" | ||
|
|
||
| bool gpu::GPUEngine::check_batch(const std::vector<std::string> &candidates, std::string &out_password) { | ||
| std::lock_guard<std::mutex> lk(p->m); | ||
| if (!p->available) return false; | ||
|
|
There was a problem hiding this comment.
2. Cuda header in cpu build 🐞 Bug ☼ Reliability
src/gpu/gpu_engine.cpp includes pbkdf2_cuda.h unconditionally, but that header uses the CUDA-only __global__ qualifier which will not compile with a normal C++ compiler. This breaks non-CUDA builds even if no CUDA codepath is executed.
Agent Prompt
### Issue description
A non-CUDA translation unit includes a header containing CUDA-specific syntax (`__global__`). When `ENABLE_CUDA` is OFF, the compiler will still parse the header and fail.
### Issue Context
- `gpu_engine.cpp` includes `src/gpu/pbkdf2_cuda.h` outside `#ifdef ENABLE_CUDA`.
- `pbkdf2_cuda.h` declares a `__global__` kernel.
### Fix Focus Areas
- src/gpu/gpu_engine.cpp[83-90]
- src/gpu/pbkdf2_cuda.h[1-13]
### Suggested fix
- Wrap the include with `#ifdef ENABLE_CUDA` (or make `pbkdf2_cuda.h` itself guard CUDA qualifiers).
- Prefer a CUDA-agnostic host header that declares a normal C++ wrapper function, implemented in a `.cu` file, rather than exposing `__global__` in headers compiled by C++.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+28
to
+30
| btc::Engine::Engine(): p(new Impl()){} | ||
| btc::Engine::~Engine(){ delete p; } | ||
|
|
There was a problem hiding this comment.
3. Deleting unique_ptr member 🐞 Bug ≡ Correctness
btc::Engine stores its implementation in a std::unique_ptr, but the destructor does delete p;, which is invalid C++ and prevents compilation. Even if corrected to compile, manual deletion defeats RAII and risks ownership bugs.
Agent Prompt
### Issue description
`btc::Engine` uses `std::unique_ptr<Impl> p`, but the destructor manually deletes `p`.
### Issue Context
`p` is a `std::unique_ptr`, so it should be destroyed automatically; manual `delete` is both incorrect and unnecessary.
### Fix Focus Areas
- include/btc/engine.h[25-28]
- src/core/engine.cpp[28-30]
### Suggested fix
- Replace `btc::Engine::~Engine(){ delete p; }` with `btc::Engine::~Engine() = default;` (or an empty destructor body).
- Do not manually free `p` anywhere else; use `p.reset()` only if you need early release.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+16
| #pragma once | ||
| #include <string> | ||
|
|
||
| namespace gpu { | ||
|
|
||
| class GPUEngine { | ||
| public: | ||
| GPUEngine(); | ||
| ~GPUEngine(); | ||
|
|
||
| bool init(const std::string &backend = "auto"); | ||
| bool is_available() const; | ||
|
|
||
| // Submit a batch of candidates for checking. Returns true if password found and writes to out_password. | ||
| bool check_batch(const std::vector<std::string> &candidates, std::string &out_password); | ||
|
|
There was a problem hiding this comment.
4. Missing vector include 🐞 Bug ≡ Correctness
src/gpu/gpu_engine.h uses std::vector in its public API but does not include <vector>, making compilation depend on include order and likely failing in files that include it first. This header is not self-contained.
Agent Prompt
### Issue description
`gpu_engine.h` references `std::vector` but does not include the standard header that defines it.
### Issue Context
Public headers must be self-contained; relying on transitive includes breaks compilation depending on include order.
### Fix Focus Areas
- src/gpu/gpu_engine.h[1-16]
### Suggested fix
- Add `#include <vector>` at the top of `src/gpu/gpu_engine.h`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+15
| #pragma once | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| class PasswordGenerator { | ||
| public: | ||
| PasswordGenerator(); | ||
| ~PasswordGenerator(); | ||
|
|
||
| // stream dictionary words | ||
| std::vector<std::string> load_dict(const std::string &path); | ||
|
|
||
| // simple brute-force generator (only lowercase and digits for skeleton) | ||
| void brute_force(int min_len, int max_len, std::function<void(const std::string&)> callback); | ||
| }; |
There was a problem hiding this comment.
5. Missing functional include 🐞 Bug ≡ Correctness
src/core/password_generator.h uses std::function but does not include <functional>, so translation units including it can fail to compile. engine.cpp includes this header without including <functional> first.
Agent Prompt
### Issue description
`PasswordGenerator::brute_force` takes a `std::function` but the header does not include `<functional>`.
### Issue Context
Headers should include everything needed for their declarations.
### Fix Focus Areas
- src/core/password_generator.h[1-15]
### Suggested fix
- Add `#include <functional>` to `src/core/password_generator.h`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+100
to
+103
| // copy T to out | ||
| size_t offset = (i-1) * SHA512_DIGEST_SIZE; | ||
| size_t copy_len = std::min((size_t)SHA512_DIGEST_SIZE, dkLen - offset); | ||
| memcpy(out.data() + offset, T.data(), copy_len); |
There was a problem hiding this comment.
8. Pbkdf2 missing algorithm include 🐞 Bug ≡ Correctness
pbkdf2_hmac_sha512.cpp uses std::min without including <algorithm>, so compilation may fail depending on the standard library and include order. This is a portability/build correctness issue.
Agent Prompt
### Issue description
`std::min` is used but `<algorithm>` is not included.
### Issue Context
Relying on transitive includes from OpenSSL or other headers is non-portable.
### Fix Focus Areas
- src/crypto/pbkdf2_hmac_sha512.cpp[1-6]
- src/crypto/pbkdf2_hmac_sha512.cpp[100-104]
### Suggested fix
- Add `#include <algorithm>` to `src/crypto/pbkdf2_hmac_sha512.cpp`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+16
to
+18
| Impl(size_t n) { | ||
| if (n == 0) n = std::max<size_t>(1, std::thread::hardware_concurrency()); | ||
| for (size_t i = 0; i < n; ++i) { |
There was a problem hiding this comment.
9. Threadpool missing algorithm include 🐞 Bug ≡ Correctness
thread_pool.cpp uses std::max but does not include <algorithm>, making compilation depend on transitive includes. This can break builds when headers change or across toolchains.
Agent Prompt
### Issue description
`std::max` is used without including `<algorithm>`.
### Issue Context
This is a new translation unit; it should include all required standard headers explicitly.
### Fix Focus Areas
- src/utils/thread_pool.cpp[1-8]
- src/utils/thread_pool.cpp[16-19]
### Suggested fix
- Add `#include <algorithm>` near the top of `src/utils/thread_pool.cpp`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+105
to
+117
| // Early-exit optimization: if caller provided a target prefix to compare and that prefix | ||
| // lies within the first block produced (i==1), compare only that prefix_len bytes and | ||
| // return quickly if mismatch. This avoids computing further blocks when the prefix doesn't match. | ||
| if (target_prefix && target_prefix_len > 0 && i == 1) { | ||
| size_t cmp = std::min(copy_len, target_prefix_len); | ||
| if (memcmp(out.data(), target_prefix, cmp) != 0) { | ||
| return false; // early exit: prefix mismatch | ||
| } | ||
| // if target_prefix_len > copy_len we cannot decide yet; continue | ||
| if (target_prefix_len <= copy_len) { | ||
| // prefix matches; if the caller only wanted to check prefix they can inspect out. | ||
| } | ||
| } |
There was a problem hiding this comment.
10. Pbkdf2 prefix check incomplete 🐞 Bug ≡ Correctness
pbkdf2_hmac_sha512 only compares target_prefix against the first derived block (i == 1), and if the prefix spans additional blocks it never compares the remaining bytes. This can return true even when later prefix bytes don’t match.
Agent Prompt
### Issue description
The optional `target_prefix` early-exit logic compares only bytes available in block 1, and does not validate the remaining prefix bytes when `target_prefix_len` extends beyond the first block.
### Issue Context
The comment states “cannot decide yet; continue”, but no later block comparison exists.
### Fix Focus Areas
- src/crypto/pbkdf2_hmac_sha512.cpp[105-117]
### Suggested fix
- Track how many prefix bytes have been verified.
- After each block copy into `out`, compare the newly-produced segment against the corresponding segment of `target_prefix` (up to `target_prefix_len`) and return false on mismatch.
- Alternatively, if the intent is “only compare up to first block”, enforce/validate `target_prefix_len <= 64` and document it.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+142
to
+166
| // HMAC-SHA512 using ipad/opad precomputed blocks (128 bytes each) | ||
| __device__ void hmac_sha512_precomp(const unsigned char ipad[128], const unsigned char opad[128], | ||
| const unsigned char *data, size_t data_len, unsigned char out[64]) { | ||
| unsigned char tmp[64]; | ||
| // Inner | ||
| // Build inner message = ipad || data | ||
| // We will process with sha512 by allocating buffer of (128 + data_len) | ||
| // For small data lengths avoid dynamic alloc: use stack when possible | ||
| // For simplicity, we'll create a small buffer for data <= (1<<20) which is fine for our salt sizes | ||
| // Create concatenated buffer | ||
| unsigned char *inner_buf = (unsigned char*)malloc(128 + data_len); | ||
| if (!inner_buf) { // out with zeros | ||
| for (int i=0;i<64;++i) out[i]=0; return; | ||
| } | ||
| memcpy(inner_buf, ipad, 128); | ||
| if (data && data_len) memcpy(inner_buf + 128, data, data_len); | ||
| sha512(inner_buf, 128 + data_len, tmp); | ||
| free(inner_buf); | ||
|
|
||
| // Outer: opad || tmp | ||
| unsigned char outer_buf[128 + 64]; | ||
| memcpy(outer_buf, opad, 128); | ||
| memcpy(outer_buf + 128, tmp, 64); | ||
| sha512(outer_buf, 128 + 64, out); | ||
| } |
There was a problem hiding this comment.
11. Cuda kernel heap allocation 🐞 Bug ➹ Performance
The CUDA PBKDF2 implementation performs device-side malloc/free inside hmac_sha512_precomp, which runs in the hot PBKDF2 loop. This can drastically reduce throughput and may fail under device-heap pressure depending on GPU/device heap configuration.
Agent Prompt
### Issue description
`hmac_sha512_precomp` allocates a buffer (`malloc(128 + data_len)`) and frees it for every HMAC call. PBKDF2 calls HMAC once for U1 and once per iteration, so this adds repeated allocations per candidate.
### Issue Context
PBKDF2 uses fixed-size inputs here (ipad/opad are 128 bytes; salt/U are small), so the allocation is avoidable.
### Fix Focus Areas
- src/gpu/pbkdf2_cuda.cu[142-166]
### Suggested fix
- Replace heap allocation with a fixed-size local buffer sized for the maximum expected `data_len` (e.g., handle salt_len up to a bounded maximum).
- Or rework `sha512()` to accept two segments (ipad + data) without concatenation.
- If heap allocation remains, document and configure the device heap size explicitly and handle allocation failure in a way that cannot create false matches.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+206
to
+212
| // Prepare salt || INT(1) | ||
| unsigned char salt_block[256]; | ||
| int sb_len = 0; | ||
| if (salt_len > 0) { memcpy(salt_block, salt, salt_len); sb_len += salt_len; } | ||
| // INT(1) big-endian | ||
| salt_block[sb_len+0] = 0x00; salt_block[sb_len+1] = 0x00; salt_block[sb_len+2] = 0x00; salt_block[sb_len+3] = 0x01; | ||
| sb_len += 4; |
There was a problem hiding this comment.
12. Cuda salt buffer overflow 🐞 Bug ⛨ Security
pbkdf2_cuda_kernel copies salt_len bytes into a fixed salt_block[256] without bounds checking, so a large salt can overflow the buffer and corrupt memory. This is a correctness/safety bug in the GPU implementation.
Agent Prompt
### Issue description
`salt_block` is a fixed 256-byte array, but the kernel does `memcpy(salt_block, salt, salt_len)` without checking that `salt_len` fits.
### Issue Context
The kernel then appends 4 bytes for `INT(1)`, so the maximum safe `salt_len` is 252.
### Fix Focus Areas
- src/gpu/pbkdf2_cuda.cu[206-213]
### Suggested fix
- Add an explicit guard: if `salt_len < 0` or `salt_len > 252`, return early (no match) or clamp safely.
- Consider making `salt_block` sized based on the maximum supported salt length and document that limit in the host-side API.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…ture detection (AVX2/AVX-512) and integrate dispatcher into PBKDF2 HMAC path. AVX implementations are TODO but the dispatch and integration are in place.
…CL kernel with full SHA-512+HMAC PBKDF2 first-block implementation for correctness; add headers
…benchmark; update CMake additions for AVX2 target and benchmark
…E status and build instructions
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
1 participant
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.
No description provided.