feat: SSD persistence for Distinguished Points — zero DP loss between sessions#105
feat: SSD persistence for Distinguished Points — zero DP loss between sessions#105Cryptosapien34 wants to merge 9 commits intooritwoen:mainfrom
Conversation
- All DP data stored on SSD (append-only binary file)
- Only lightweight index in RAM (~16 bytes/DP vs 68 bytes)
- Collision check against all DPs (current + old sessions)
- No DP is ever lost between sessions
- Dynamic file path per puzzle: dps_range{bits}.bin
- Cross-session persistence test included
- SSD persistence feature by Cryptosapien34
- Tested on Apple Silicon M4 Max at 27 MKeys/s
📝 WalkthroughWalkthroughDPTable moved from an in-memory HashMap of StoredDPs to an SSD-backed fixed-size-record store with a RAM-only index of disk offsets; on startup the file header (magic + compressed pubkey) is validated and the index rebuilt or the file rotated to a timestamped backup on mismatch; constructor now requires Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Solver as Solver/Kangaroo
participant DPTable as DPTable (RAM index)
participant Disk as dps_range{bits}.bin
Solver->>DPTable: insert_and_check(new_dp)
DPTable->>DPTable: compute hash_key
DPTable->>DPTable: lookup offsets in RAM index
alt offsets found
DPTable->>Disk: read 68-byte record(s) at offsets
Disk-->>DPTable: bytes
DPTable->>DPTable: deserialize & verify affine_x, ktype
DPTable->>DPTable: run collision checks (tame/wild/cross)
alt collision
DPTable-->>Solver: return collision key
else no collision
DPTable->>Disk: append new 68-byte record at next_offset
Disk-->>DPTable: ack
DPTable->>DPTable: update RAM index & counters
DPTable-->>Solver: inserted (None)
end
else no offsets
DPTable->>Disk: append new 68-byte record
Disk-->>DPTable: ack
DPTable->>DPTable: update RAM index & counters
DPTable-->>Solver: inserted (None)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
coderabbitai
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cpu/dp_table.rs`:
- Around line 161-164: In build_index, detect and log partial/truncated record
cases by checking if file_size % DP_RECORD_SIZE != 0 before iterating; if true,
emit a warning (via the existing logger or processLogger) that the file ends
with a partial record and may be corrupted, then continue with current behavior.
Ensure you reference DP_RECORD_SIZE and the variables used to parse records
(hash_key from buf[0..8] and ktype from buf[64..68]) in the log message so it's
clear which format/offsets were affected. This adds an explicit alert instead of
silently ignoring a truncated final record while keeping the existing Err(_) =>
break behavior.
- Around line 483-563: Tests insert_stores_on_disk, test_collision_case1, and
test_cross_wild create a persistent file (dps_range40.bin) by constructing
DPTable via DPTable::new(..., 40) and never remove it; update each test to
either use a unique range_bits value (e.g., 41, 42, 43) when calling
DPTable::new or explicitly delete the persistence file after the test finishes
(mirror the cleanup in the existing persistence test), targeting the DPTable
persistence filename generation for range_bits 40 so the orphan file is not left
on disk.
- Around line 216-230: The write_dp_to_disk function currently swallows
write/flush errors (using let _ = writer.write_all(...) and let _ =
writer.flush(...)), causing silent data loss; update write_dp_to_disk (and its
use of self.dp_writer) to check and propagate I/O errors instead of ignoring
them: return a Result<u64, std::io::Error> (or propagate via ?), handle
writer.write_all(&buf) and writer.flush() failures by returning Err with the
io::Error, and only advance self.next_offset/return offset after a successful
write+flush (or document when flush is skipped); ensure callers of
write_dp_to_disk are updated to handle the Result accordingly (symbols:
write_dp_to_disk, dp_writer, next_offset, total_dps, DP_RECORD_SIZE).
- Around line 93-129: The DP file lacks a pubkey header so build_index() loads
DPs from a different puzzle; modify new(), build_index(), and write_dp_to_disk()
to embed and validate a header containing the puzzle pubkey before any 68-byte
records: when opening dp_file_path in new()/build_index(), read the header (if
file non-empty) and compare to self.pubkey (serialize the pubkey
deterministically, e.g. affine x/y or compressed bytes); if absent (new file)
write the header immediately before writing any records; if header present but
mismatches, log an error and truncate/recreate the file (or refuse to load) and
clear tbl.index and counters so collisions aren’t contaminated. Also update
write_dp_to_disk() to check and return/report I/O errors (do not silently ignore
writes), ensure the header is written/flushed once before any dp_writer append
operations, and reference dp_file_path, dp_writer, new(), build_index(), and
write_dp_to_disk() when making the changes.
In `@src/lib.rs`:
- Line 1354: The DPTable is being constructed with the original range_bits,
causing it to load/save persistence files for the wrong search space; change the
cpu::DPTable::new call that creates dp_table (currently using range_bits) to use
solve_range_bits so the DP persistence (dps_range{N}.bin) matches the
transformed problem defined by solve_start, solve_pubkey and solve_base_point.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3597b93-1209-41ab-aacf-d93cb639d628
📒 Files selected for processing (3)
src/cpu/dp_table.rssrc/lib.rssrc/solver.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (3)
src/solver.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/solver.rs: Implement solver state machine logic in src/solver.rs using the step() method and calibration
Auto-calibrate steps to stay under 50ms TDR (Timeout Detection and Recovery) threshold
Files:
src/solver.rs
src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Add CLI flags through the Args struct using clap derive in src/lib.rs
Files:
src/lib.rs
src/cpu/dp_table.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/cpu/dp_table.rs: Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Files:
src/cpu/dp_table.rs
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Applied to files:
src/solver.rssrc/lib.rssrc/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Applied to files:
src/solver.rssrc/lib.rssrc/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Applied to files:
src/solver.rssrc/lib.rssrc/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Auto-calculate DP bits from range_bits and clamp to 8-40 range
Applied to files:
src/solver.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/**/*.rs : Never add y_parity or symClass to GpuDistinguishedPoint; collision resolution must be done CPU-side via compute_candidate_keys()
Applied to files:
src/lib.rssrc/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/init.rs : Never split jump table for negation map; use add/sub operations with the same 256-entry table
Applied to files:
src/lib.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu_crypto/context.rs : Handle GPU context setup including wgpu adapter and device creation in src/gpu_crypto/context.rs
Applied to files:
src/lib.rs
📚 Learning: 2026-04-02T14:50:36.777Z
Learnt from: oritwoen
Repo: oritwoen/kangaroo PR: 103
File: src/solver.rs:601-605
Timestamp: 2026-04-02T14:50:36.777Z
Learning: In src/solver.rs, the calibration candidate ladder `[16, 17, 18, 24, 64, 128, 256, 512]` is intentional. The 17/18/24 cluster sits below a sharp GPU performance cliff; 24 benchmarks within noise of 18 on both AMD and Intel, so the doubling-heuristic early-exit firing after 18 is acceptable. The heuristic is preserved specifically for the 64+ range where the doubling gap is significant. Validated across AMD and Intel hardware.
Applied to files:
src/lib.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu_crypto/shaders/field.wgsl : Implement secp256k1 field arithmetic operations in src/gpu_crypto/shaders/field.wgsl as 256-bit modular arithmetic using array<u32, 8> in little-endian limbs
Applied to files:
src/lib.rs
🔇 Additional comments (2)
src/cpu/dp_table.rs (1)
134-144: LGTM on index rebuild logic.Clean separation: RAM index stores just offset+ktype (12 bytes) while full 68-byte records stay on disk. The sequential scan approach will scale reasonably for the stated use case.
src/solver.rs (1)
344-348: LGTM - correctly wired.When called via
new_with_base()fromrun_single_gpu_solver, therange_bitsparameter isc.effective_range_bits(the transformed value), so the DP file is named appropriately for the actual search space.This is consistent with how
start,pubkey, andbase_pointare all transformed values in that path.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 3 files
Confidence score: 2/5
- There is a high-confidence correctness risk in
src/cpu/dp_table.rs: persistence is keyed only byrange_bits, so runs for different target pubkeys can reuse stale DP data and produce false collision behavior. src/cpu/dp_table.rsalso has index consistency problems (trailing partial record handling and ignored disk write/open failures) that can make in-memory offsets point to invalid or missing records, leading to missed collisions.- Given one severe (8/10) and multiple medium-severity (6/10) persistence/index issues in core DP storage logic, this is higher merge risk than a typical safe-to-merge change.
- Pay close attention to
src/cpu/dp_table.rsandsrc/lib.rs- persistence file selection and index/write error handling can mix puzzle state or corrupt lookup behavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/lib.rs">
<violation number="1" location="src/lib.rs:1354">
P2: Multi-GPU DPTable uses original range_bits for persistence file even when modular constraints change solve_range_bits, causing the DP store to load/save under the wrong range file.</violation>
</file>
<file name="src/cpu/dp_table.rs">
<violation number="1" location="src/cpu/dp_table.rs:99">
P1: The persistence file is keyed only on `range_bits`, not on the target pubkey. If you solve two different 135-bit puzzles (different pubkeys), the second run loads DPs from the first puzzle's search. A hash collision between puzzle A's tame DP and puzzle B's wild DP triggers `check_collision()` with completely wrong distance data, producing bogus key candidates or wasting time on false positives. Include at least a pubkey hash in the filename (or store it as a file header and validate on load).</violation>
<violation number="2" location="src/cpu/dp_table.rs:182">
P2: build_index does not handle a trailing partial record; next_offset is set to the last full record offset even when the file length includes extra bytes. With append mode, new writes go to EOF beyond next_offset, so DiskRef offsets become misaligned and reads can seek to the wrong data.</violation>
<violation number="3" location="src/cpu/dp_table.rs:223">
P2: Disk write/open failures are ignored but offsets still advance and index entries are created, so the in‑memory index can point to non‑existent records and cause missed collisions.</violation>
</file>
Architecture diagram
sequenceDiagram
participant S as Solver (src/solver.rs)
participant T as DPTable (src/cpu/dp_table.rs)
participant R as RAM Index (HashMap)
participant D as SSD Storage (Binary File)
Note over S,D: Initialization Phase (Persistence Recovery)
S->>T: NEW: new(range_bits)
T->>T: Construct path: ~/Desktop/kangaroo_dps/dps_range{bits}.bin
T->>D: Open existing file (if any)
loop Sequential Scan
D-->>T: Read 68B record
T->>R: NEW: Insert { hash_key -> DiskRef(offset, ktype) }
end
Note right of R: RAM usage reduced from<br/>68B to ~16B per DP
Note over S,D: Runtime Flow (New Distinguished Point Found)
S->>T: add_dp(gpu_dp)
T->>R: Lookup hash_key
alt hash_key NOT in Index (New Point)
T->>D: NEW: Append full record (affine_x, dist, ktype)
opt Every 1000 DPs
T->>D: BufWriter::flush()
end
T->>R: NEW: Store DiskRef { offset, ktype }
else hash_key MATCH in Index (Potential Collision)
R-->>T: Return List<DiskRef>
loop For each DiskRef
alt ktype differs (Tame vs Wild)
T->>D: NEW: Seek to offset & Read 68B
D-->>T: Return StoredDP (Full affine_x)
alt NEW: Full affine_x match
T->>T: Compute Candidate Key
T-->>S: Return Success (Key Found)
end
else ktype matches
Note right of T: Skip disk read (Same type collision)
end
end
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
- Add file header with pubkey validation (KDP1 magic + 32B X coord) - Reject/backup DP file if pubkey doesn't match (prevents cross-puzzle corruption) - Log errors on disk write failures instead of silent discard - Use persistent file handle for collision reads (avoid repeated open/close) - Warn if DP file is truncated (partial records detected) - Add test: pubkey mismatch creates fresh file - Add test: cross-session persistence finds collision from disk
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/cpu/dp_table.rs">
<violation number="1" location="src/cpu/dp_table.rs:221">
P2: Header validation drops the compressed pubkey prefix byte, so P and −P (same X, opposite Y parity) are treated as identical. This can reuse a DP file from a different puzzle despite the pubkey mismatch check.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/cpu/dp_table.rs (1)
349-369:⚠️ Potential issue | 🟠 MajorIndex and counters updated even when disk write fails.
When
write_dp_to_diskfails (line 358) ordp_writerisNone, it still returns an offset. The caller at lines 478-483 unconditionally adds aDiskRefto the index and incrementstotal_dps.Result: index contains dangling references, counters are wrong, collision checks waste I/O reading non-existent data. The PR objective "no DP is ever lost" is silently violated.
Return Option to propagate failure
- fn write_dp_to_disk(&mut self, affine_x: &[u8; 32], dist: &[u8; 32], ktype: u32) -> u64 { + fn write_dp_to_disk(&mut self, affine_x: &[u8; 32], dist: &[u8; 32], ktype: u32) -> Option<u64> { let offset = self.next_offset; - if let Some(ref mut writer) = self.dp_writer { + let writer = self.dp_writer.as_mut()?; let mut buf = [0u8; 68]; buf[0..32].copy_from_slice(affine_x); buf[32..64].copy_from_slice(dist); buf[64..68].copy_from_slice(&ktype.to_le_bytes()); if let Err(e) = writer.write_all(&buf) { tracing::error!("Failed to write DP to disk: {} — DP may be lost!", e); - return offset; + return None; } if self.total_dps % 1000 == 0 { if let Err(e) = writer.flush() { tracing::error!("Failed to flush DP file: {}", e); } } - } self.next_offset += DP_RECORD_SIZE; - offset + Some(offset) }And in
insert_and_check:- let offset = self.write_dp_to_disk(&affine_x, &dist_bytes, dp.ktype); - self.index - .entry(hash_key) - .or_insert_with(Vec::new) - .push(DiskRef { offset, ktype: dp.ktype }); - self.total_dps += 1; - self.increment_type_counter(dp.ktype); + if let Some(offset) = self.write_dp_to_disk(&affine_x, &dist_bytes, dp.ktype) { + self.index + .entry(hash_key) + .or_insert_with(Vec::new) + .push(DiskRef { offset, ktype: dp.ktype }); + self.total_dps += 1; + self.increment_type_counter(dp.ktype); + }Also applies to: 476-483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/dp_table.rs` around lines 349 - 369, write_dp_to_disk currently increments next_offset and returns an offset even when writing fails or dp_writer is None, causing dangling DiskRef entries and wrong counters; change write_dp_to_disk (and any callers such as insert_and_check) to propagate failure by returning a Result<u64, E> or Option<u64> and only update self.next_offset and self.total_dps after a successful write/flush; in insert_and_check use the returned success value to decide whether to push a DiskRef and increment total_dps (leave DP_RECORD_SIZE, dp_writer, next_offset, total_dps, and DiskRef references intact to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cpu/dp_table.rs`:
- Around line 216-221: The comments around the pubkey comparison are
contradictory and confusing; update the comment above the comparison involving
stored_pubkey, header, and expected_pubkey to accurately state that
stored_pubkey (header[4..36]) is a 32-byte X-coordinate and it is being compared
to the X-coordinate portion of a 33-byte compressed key
(expected_pubkey[1..33]); remove mentions of comparing 33 bytes or "prefix + X
coordinate" and clearly state that the prefix byte is skipped when comparing.
- Line 119: The call to fs::create_dir_all(&dp_dir) currently discards errors
which can hide root causes for later init_file failures; change it to handle the
Result instead—check the return value of create_dir_all(&dp_dir) and on Err
either log the error (including the error details and dp_dir) and return Err
from the enclosing function or propagate the error (e.g., using ? or map_err) so
callers see the real failure before calling init_file; ensure the log/error
references dp_dir and the original io::Error to aid debugging.
---
Duplicate comments:
In `@src/cpu/dp_table.rs`:
- Around line 349-369: write_dp_to_disk currently increments next_offset and
returns an offset even when writing fails or dp_writer is None, causing dangling
DiskRef entries and wrong counters; change write_dp_to_disk (and any callers
such as insert_and_check) to propagate failure by returning a Result<u64, E> or
Option<u64> and only update self.next_offset and self.total_dps after a
successful write/flush; in insert_and_check use the returned success value to
decide whether to push a DiskRef and increment total_dps (leave DP_RECORD_SIZE,
dp_writer, next_offset, total_dps, and DiskRef references intact to locate the
change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c678e5e-8e92-476a-a5ec-317bd786ee1e
📒 Files selected for processing (1)
src/cpu/dp_table.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
src/cpu/dp_table.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/cpu/dp_table.rs: Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Files:
src/cpu/dp_table.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/**/*.rs : Never add y_parity or symClass to GpuDistinguishedPoint; collision resolution must be done CPU-side via compute_candidate_keys()
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/buffers.rs : Clamp DP buffer to 90% capacity per dispatch to prevent overflow
Applied to files:
src/cpu/dp_table.rs
🔇 Additional comments (9)
src/cpu/dp_table.rs (9)
1-43: Documentation and constants look solid.Architecture is well-explained. The RAM index vs SSD data split is clearly documented with memory usage estimates. Constants match the implementation.
57-65: LGTM. Correctly converts to affine before encoding.
75-101: Good design. 12-byteDiskRefin RAM keeps memory footprint low. Persistentdp_readerhandle addresses the previous review's syscall overhead concern.
143-194: Header validation and backup logic looks correct. Pubkey mismatch creates timestamped backup, preventing cross-puzzle corruption. Good error/warning logging throughout.
262-269: Truncated file detection is in. This addresses the previous review's concern about silently skipping partial records.
371-432: Collision detection flow is correct. Ktype filtering before disk I/O is a good optimization. Fullaffine_xverification before resolving handles hash collisions properly.
506-558: Collision resolution math looks correct. The 8-way scalar combination for tame↔wild andscalar_halftrick for wild₁↔wild₂ handle the sign ambiguities properly.
654-660: Tests properly clean up and validate cross-session persistence. Thecleanup_test_filehelper addresses the orphan file concern from previous review.test_persistence_across_sessionsvalidates the core SSD persistence feature.Also applies to: 711-743
114-120: Unable to verify the cross-file bug claim due to repository access failure. The review comment alleges thatsrc/lib.rs:1354passesrange_bitsinstead ofsolve_range_bitstoDPTable::new(), causing file path mismatches when modular constraints modify the effective range. This claim requires access to the actual codebase to confirm: (1) whetherDPTable::new()is called with the wrong parameter, (2) whethersolve_range_bitsis computed and in scope at the call site, and (3) the actual control flow showing how constraints affect file path selection. Verification deferred pending repository access.
| "{}/Desktop/kangaroo_dps", | ||
| std::env::var("HOME").unwrap_or_else(|_| ".".to_string()) | ||
| ); | ||
| let _ = fs::create_dir_all(&dp_dir); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Directory creation errors are silently ignored.
If create_dir_all fails (permissions, disk full), subsequent init_file operations will fail with confusing errors about the file, not the directory. Worth a log at minimum.
Log on failure
- let _ = fs::create_dir_all(&dp_dir);
+ if let Err(e) = fs::create_dir_all(&dp_dir) {
+ tracing::warn!("Failed to create DP directory {}: {}", dp_dir, e);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let _ = fs::create_dir_all(&dp_dir); | |
| if let Err(e) = fs::create_dir_all(&dp_dir) { | |
| tracing::warn!("Failed to create DP directory {}: {}", dp_dir, e); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cpu/dp_table.rs` at line 119, The call to fs::create_dir_all(&dp_dir)
currently discards errors which can hide root causes for later init_file
failures; change it to handle the Result instead—check the return value of
create_dir_all(&dp_dir) and on Err either log the error (including the error
details and dp_dir) and return Err from the enclosing function or propagate the
error (e.g., using ? or map_err) so callers see the real failure before calling
init_file; ensure the log/error references dp_dir and the original io::Error to
aid debugging.
| // Check pubkey (first 32 bytes of compressed pubkey, skip prefix byte for flexibility) | ||
| // We compare all 33 bytes: prefix + X coordinate | ||
| let stored_pubkey = &header[4..36]; | ||
| // stored_pubkey is 32 bytes, we compare with first 32 bytes of expected (skip 02/03 prefix) | ||
| // Actually store 32 bytes of X coordinate only for cleaner comparison | ||
| if stored_pubkey != &expected_pubkey[1..33] { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Comments contradict each other and don't match the code.
Lines 216-220 say multiple conflicting things: "skip prefix byte", "compare all 33 bytes", "first 32 bytes", etc. The code compares 32-byte X-coordinates (correct), but the comments are confusing.
Simplify comments
- // Check pubkey (first 32 bytes of compressed pubkey, skip prefix byte for flexibility)
- // We compare all 33 bytes: prefix + X coordinate
let stored_pubkey = &header[4..36];
- // stored_pubkey is 32 bytes, we compare with first 32 bytes of expected (skip 02/03 prefix)
- // Actually store 32 bytes of X coordinate only for cleaner comparison
+ // Compare X-coordinate only (32 bytes); we store without the 02/03 prefix
if stored_pubkey != &expected_pubkey[1..33] {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cpu/dp_table.rs` around lines 216 - 221, The comments around the pubkey
comparison are contradictory and confusing; update the comment above the
comparison involving stored_pubkey, header, and expected_pubkey to accurately
state that stored_pubkey (header[4..36]) is a 32-byte X-coordinate and it is
being compared to the X-coordinate portion of a 33-byte compressed key
(expected_pubkey[1..33]); remove mentions of comparing 33 bytes or "prefix + X
coordinate" and clearly state that the prefix byte is skipped when comparing.
- Header stores all 33 bytes (prefix + X) to distinguish P from -P - Use solve_range_bits in lib.rs for modular constraint handling - Log directory creation errors - Clean up misleading comments
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cpu/dp_table.rs`:
- Line 9: Comments claim header is 36 bytes with a "32B compressed pubkey" but
HEADER_SIZE is 37 and a compressed secp256k1 pubkey is 33 bytes (0x02/0x03
prefix + 32B X); update the stale comments to reflect 37 bytes and "33B
compressed pubkey" wherever the header comment appears (the top-of-file header
comment and the later comment around the dp table header) so they match the
HEADER_SIZE constant.
- Around line 232-234: The comment "X coordinate only" is incorrect for the code
in dp_table.rs where header[4..37].copy_from_slice(&pubkey_bytes[0..33]) copies
the full compressed pubkey (33 bytes including the prefix). Update the comment
(or the slice if you actually want only the X coordinate) to accurately reflect
intent—either change the slice to pubkey_bytes[1..33] to copy only the X
coordinate, or replace the comment with "compressed pubkey (33 bytes, including
prefix)" so header, pubkey_bytes and the copy_from_slice call are consistent.
- Around line 218-221: Update the misleading comments around the stored_pubkey
and comparison: note that stored_pubkey = &header[4..37] is 33 bytes (includes
the 0x02/0x03 prefix), and the code correctly compares the full 33-byte value
against expected_pubkey[0..33]; replace the existing comments that claim "32
bytes" and "X coordinate only" with a clear comment stating that the slice
includes the compressed public key prefix and that the comparison is for the
full 33-byte compressed key (or, if you intended to compare only the 32-byte X
coordinate, change the slice/compare to 4..36 and expected_pubkey[1..33]
accordingly).
- Around line 349-369: write_dp_to_disk currently returns an offset even when no
write occurred (dp_writer is None or write failed), causing phantom index
entries; change write_dp_to_disk signature to return Option<u64> and only
increment self.next_offset and return Some(offset) after a successful write and
flush (handle writer.write_all/flush errors by logging and returning None), and
update insert_and_check to only add to the index when write_dp_to_disk returns
Some(offset); keep read_dp_from_disk unchanged but rely on the absence in the
index to avoid phantom reads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c5cf99d-fb79-4c39-bba5-70ae55d7cd83
📒 Files selected for processing (2)
src/cpu/dp_table.rssrc/lib.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Add CLI flags through the Args struct using clap derive in src/lib.rs
Files:
src/lib.rs
src/cpu/dp_table.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/cpu/dp_table.rs: Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Files:
src/cpu/dp_table.rs
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Auto-calculate DP bits from range_bits and clamp to 8-40 range
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Applied to files:
src/lib.rssrc/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Applied to files:
src/lib.rssrc/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Applied to files:
src/lib.rssrc/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/**/*.rs : Never add y_parity or symClass to GpuDistinguishedPoint; collision resolution must be done CPU-side via compute_candidate_keys()
Applied to files:
src/lib.rssrc/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Auto-calculate DP bits from range_bits and clamp to 8-40 range
Applied to files:
src/lib.rs
📚 Learning: 2026-04-02T14:50:36.777Z
Learnt from: oritwoen
Repo: oritwoen/kangaroo PR: 103
File: src/solver.rs:601-605
Timestamp: 2026-04-02T14:50:36.777Z
Learning: In src/solver.rs, the calibration candidate ladder `[16, 17, 18, 24, 64, 128, 256, 512]` is intentional. The 17/18/24 cluster sits below a sharp GPU performance cliff; 24 benchmarks within noise of 18 on both AMD and Intel, so the doubling-heuristic early-exit firing after 18 is acceptable. The heuristic is preserved specifically for the 64+ range where the doubling gap is significant. Validated across AMD and Intel hardware.
Applied to files:
src/lib.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/buffers.rs : Clamp DP buffer to 90% capacity per dispatch to prevent overflow
Applied to files:
src/cpu/dp_table.rs
🔇 Additional comments (7)
src/cpu/dp_table.rs (6)
57-65: LGTM - correctly extracts 33-byte compressed pubkey.Uses
to_encoded_point(true)for compressed format and copies the full 33 bytes. This handles both the 02/03 prefix and the X coordinate, which is needed to distinguish P from -P.
145-196: LGTM - init_file handles all the edge cases properly.Header validation with backup on mismatch, truncation detection, persistent reader setup. The backup filename with unix timestamp is a nice touch for debugging.
245-326: LGTM - build_index efficiently rebuilds the in-RAM index.Only reads hash_key (8 bytes from affine_x) and ktype per record, keeps full DP data on disk. The truncation warning at lines 264-269 addresses the past review concern.
328-347: LGTM - persistent reader avoids per-read syscall overhead.Past review concern about opening file per collision check is addressed. Uses
self.dp_readerwith seek/read.
371-432: LGTM - collision check logic is sound.Clones refs to avoid borrow conflict (line 378), only reads full DP from disk when ktype differs, handles tame↔wild and cross-wild (wild1↔wild2) cases correctly.
654-779: Tests look solid - cleanup functions prevent orphan files.Each test uses unique range_bits (40, 41, 42, 98, 99) with matching cleanup. The pubkey mismatch test also cleans up backup files. Addresses past review concern about test pollution.
src/lib.rs (1)
1354-1354: Correctly uses solve_range_bits for DP persistence.Past review flagged that using
range_bitswith modular constraints would createdps_range135.bincontaining DPs from the constrained search space, contaminating future runs without constraints. Now usessolve_range_bitswhich accounts foreffective_range_bitswhen constraints exist.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/cpu/dp_table.rs">
<violation number="1" location="src/cpu/dp_table.rs:234">
P3: The inline comment `// X coordinate only` is wrong. `pubkey_bytes[0..33]` is the full 33-byte compressed pubkey (prefix + X). This comment was probably left over from when the code stored `pubkey_bytes[1..33]`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
- Header is 37 bytes (4B magic + 33B compressed pubkey), not 36 - Remove contradictory comments about pubkey storage - Fix comment: full compressed pubkey, not X coordinate only
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/cpu/dp_table.rs (2)
41-43:⚠️ Potential issue | 🟡 MinorComment still claims 36 bytes but HEADER_SIZE is 37.
Line 9 says "37 bytes: 4B magic + 33B compressed pubkey" (correct). This comment says "32 bytes compressed pubkey = 36 bytes" (wrong). Compressed secp256k1 pubkey is 33 bytes.
-/// File header: 4 bytes magic + 32 bytes compressed pubkey = 36 bytes +/// File header: 4 bytes magic + 33 bytes compressed pubkey = 37 bytes const HEADER_MAGIC: &[u8; 4] = b"KDP1"; const HEADER_SIZE: u64 = 37;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/dp_table.rs` around lines 41 - 43, The file header comment incorrectly states "32 bytes compressed pubkey = 36 bytes" while HEADER_SIZE is 37 and the correct compressed secp256k1 pubkey length is 33 bytes; update the comment that precedes HEADER_MAGIC/HEADER_SIZE to say "4 bytes magic + 33 bytes compressed pubkey = 37 bytes" (or otherwise make the text consistent with the constants HEADER_MAGIC and HEADER_SIZE and the 33-byte compressed pubkey length).
348-368:⚠️ Potential issue | 🟠 MajorWrite failure returns offset but next_offset not incremented → index corruption on subsequent success.
Failure path:
- Write DP_A fails at offset X → logs error, returns X early (next_offset stays at X)
- Caller adds DP_A → offset X to index
- Write DP_B succeeds at offset X → writes, increments next_offset to X+68
- Caller adds DP_B → offset X to index
Now both DP_A and DP_B index entries point to offset X, which contains DP_B's data. Collision checks against DP_A read DP_B's record.
Similar issue when
dp_writeris None: increments next_offset (line 366), returns phantom offset, caller adds to index pointing to unwritten location.Return Option to signal success
- fn write_dp_to_disk(&mut self, affine_x: &[u8; 32], dist: &[u8; 32], ktype: u32) -> u64 { - let offset = self.next_offset; + fn write_dp_to_disk(&mut self, affine_x: &[u8; 32], dist: &[u8; 32], ktype: u32) -> Option<u64> { if let Some(ref mut writer) = self.dp_writer { + let offset = self.next_offset; let mut buf = [0u8; 68]; buf[0..32].copy_from_slice(affine_x); buf[32..64].copy_from_slice(dist); buf[64..68].copy_from_slice(&ktype.to_le_bytes()); if let Err(e) = writer.write_all(&buf) { tracing::error!("Failed to write DP to disk: {} — DP may be lost!", e); - return offset; + return None; } if self.total_dps % 1000 == 0 { if let Err(e) = writer.flush() { tracing::error!("Failed to flush DP file: {}", e); } } + self.next_offset += DP_RECORD_SIZE; + return Some(offset); } - self.next_offset += DP_RECORD_SIZE; - offset + None }Then in
insert_and_check:- let offset = self.write_dp_to_disk(&affine_x, &dist_bytes, dp.ktype); - self.index - .entry(hash_key) - .or_insert_with(Vec::new) - .push(DiskRef { offset, ktype: dp.ktype }); + if let Some(offset) = self.write_dp_to_disk(&affine_x, &dist_bytes, dp.ktype) { + self.index + .entry(hash_key) + .or_insert_with(Vec::new) + .push(DiskRef { offset, ktype: dp.ktype }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/dp_table.rs` around lines 348 - 368, The write path in write_dp_to_disk currently returns the offset even on write failure or when dp_writer is None, causing index corruption; change write_dp_to_disk to return an Option<u64> (or Result<u64, _>) and only advance self.next_offset and return Some(offset) when the write (and flush if applicable) actually succeeds, otherwise return None without incrementing next_offset; update callers such as insert_and_check to treat None as a failure and avoid inserting an index entry. Ensure you reference the DP_RECORD_SIZE constant, the dp_writer field, and the write_all/flush error branches so all failure paths do not advance next_offset or produce a phantom offset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/cpu/dp_table.rs`:
- Around line 41-43: The file header comment incorrectly states "32 bytes
compressed pubkey = 36 bytes" while HEADER_SIZE is 37 and the correct compressed
secp256k1 pubkey length is 33 bytes; update the comment that precedes
HEADER_MAGIC/HEADER_SIZE to say "4 bytes magic + 33 bytes compressed pubkey = 37
bytes" (or otherwise make the text consistent with the constants HEADER_MAGIC
and HEADER_SIZE and the 33-byte compressed pubkey length).
- Around line 348-368: The write path in write_dp_to_disk currently returns the
offset even on write failure or when dp_writer is None, causing index
corruption; change write_dp_to_disk to return an Option<u64> (or Result<u64, _>)
and only advance self.next_offset and return Some(offset) when the write (and
flush if applicable) actually succeeds, otherwise return None without
incrementing next_offset; update callers such as insert_and_check to treat None
as a failure and avoid inserting an index entry. Ensure you reference the
DP_RECORD_SIZE constant, the dp_writer field, and the write_all/flush error
branches so all failure paths do not advance next_offset or produce a phantom
offset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e7843bcd-3a88-4a49-92ee-aa0dcf9c79f2
📒 Files selected for processing (1)
src/cpu/dp_table.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
src/cpu/dp_table.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/cpu/dp_table.rs: Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Files:
src/cpu/dp_table.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Auto-calculate DP bits from range_bits and clamp to 8-40 range
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/**/*.rs : Never add y_parity or symClass to GpuDistinguishedPoint; collision resolution must be done CPU-side via compute_candidate_keys()
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/buffers.rs : Clamp DP buffer to 90% capacity per dispatch to prevent overflow
Applied to files:
src/cpu/dp_table.rs
🔇 Additional comments (3)
src/cpu/dp_table.rs (3)
408-428: Early return after first full-affine_x match fails verification — might miss other valid collisions?If multiple DPs share the same
affine_x(rare but possible):
- tame at offset A
- wild1 at offset B
- Incoming: wild2
Loop finds tame first (line 394 matches). Tame-wild verification fails at line 427 → returns None without checking wild1 for cross-wild collision.
I'm not 100% sure this is a real bug. If points truly match and one collision formula fails, others at the same point likely fail too. But
continueinstead ofreturn Nonewould be safer:for candidate in &candidates { if verify_key_with_base(candidate, &self.pubkey, &self.base_point) { tracing::info!("Collision found! Key: 0x{}", hex::encode(candidate)); return Some(candidate.clone()); } } - return None; + continue; // Try other stored DPs at same hash_key }Same for line 411 in cross-wild branch.
Can you confirm whether multiple stored DPs at the same affine_x is a valid scenario in the kangaroo algorithm? If not, the early return is fine.
1-27: SSD persistence architecture looks solid.The index-only-in-RAM approach (~16 bytes/DP vs ~68 full record) with disk-backed storage is a good design for the persistence goal. File-per-puzzle via
range_bits+ pubkey validation prevents cross-puzzle contamination.The startup flow (validate header → build index or backup/fresh) handles the key scenarios. Good work on addressing the previous review feedback for persistent reader, truncation warnings, and error logging.
Also applies to: 104-143
653-778: Tests properly clean up persistence files now.
cleanup_test_file()helper + cleanup at start and end of each test prevents orphaned files. Using unique range_bits per test (40, 41, 42, 98, 99) avoids test interference.The cross-session test at lines 710-742 is a good integration test for the persistence feature.
- write_dp_to_disk now returns Option<u64> instead of u64 - Index entry is only added on successful disk write - Prevents phantom offsets pointing to non-existent records
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/cpu/dp_table.rs">
<violation number="1" location="src/cpu/dp_table.rs:352">
P1: If DP writer initialization fails, subsequent inserts are silently dropped (no disk write and no index update) while returning `None` like a normal no-collision path.</violation>
<violation number="2" location="src/cpu/dp_table.rs:363">
P1: Buffered SSD writes are indexed before flush, so collision checks can reference offsets not yet readable from disk and miss valid collisions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cpu/dp_table.rs`:
- Around line 119-121: DPTable::new currently logs errors (e.g., failed
fs::create_dir_all, header creation, file open) and returns a usable DPTable
even when persistence setup failed; change DPTable::new to return
Result<DPTable, Error> (or Result with a clear enum) and propagate failures
instead of swallowing them, or detect failures and construct an explicit
in-memory-only DPTable fallback; update callers (tests/usage) and ensure methods
that rely on persistence like DPTable::insert_and_check check an is_persistent
flag or the Result so they don't attempt file I/O when persistence init failed;
apply the same change to the other persistence-init blocks referenced (the
header creation and file open code paths) so all errors either bubble up or
select the in-memory fallback consistently.
- Around line 348-369: The write_dp_to_disk function currently writes the DP and
only flushes every 1000th write, then returns Some(offset) and increments
next_offset even if flush fails, which lets check_collision (using dp_reader)
observe an offset whose data may still be buffered; change write_dp_to_disk so
it forces the data to be visible before publishing the offset: after
writer.write_all(&buf) call perform a durable flush (e.g., writer.flush() or
writer.get_ref().sync_data()/sync_all() as appropriate) on every write, treat
any flush/sync error as fatal for this write (log the error and return None),
and only increment next_offset and return Some(offset) after the flush/sync
succeeds; reference symbols: write_dp_to_disk, writer, dp_reader, next_offset,
total_dps, DP_RECORD_SIZE, check_collision.
- Around line 186-190: build_index() detects a truncated tail but currently
leaves the garbage in the file and init_file() reopens with append(true),
causing logical offsets (self.next_offset) to diverge from physical EOF; fix by
having build_index() open the file with read+write
(OpenOptions::new().read(true).write(true).open(...)) so it can call
set_len(remainder_end) to truncate the file to the valid length before
returning, then reopen/init_file() as appendable. Also ensure write_dp_to_disk()
does not publish the new offset to the index before data is durably written:
either flush the BufWriter (or call sync_all on the underlying File) immediately
after writing a record and before inserting the offset into the index, or move
the index-publish step to after the flush, so read_dp_from_disk() never seeks to
an offset whose bytes haven’t been flushed to disk.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3cfc0959-e328-4774-b639-06ed228efb5d
📒 Files selected for processing (1)
src/cpu/dp_table.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
src/cpu/dp_table.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/cpu/dp_table.rs: Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Files:
src/cpu/dp_table.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Auto-calculate DP bits from range_bits and clamp to 8-40 range
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/**/*.rs : Never add y_parity or symClass to GpuDistinguishedPoint; collision resolution must be done CPU-side via compute_candidate_keys()
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/buffers.rs : Clamp DP buffer to 90% capacity per dispatch to prevent overflow
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/crypto/mod.rs : Implement key verification using verify_key and full_verify functions in src/crypto/mod.rs
Applied to files:
src/cpu/dp_table.rs
- Flush BufWriter after every DP write so reader always sees latest data - Truncate file to valid length when partial records detected - Prevents offset/data misalignment and missed collisions
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/cpu/dp_table.rs (2)
41-43:⚠️ Potential issue | 🟡 MinorComment says 36 bytes but constant is 37.
Line 41 claims "4 bytes magic + 32 bytes compressed pubkey = 36 bytes" but
HEADER_SIZEis 37. Compressed secp256k1 pubkey is 33 bytes (02/03 prefix + 32-byte X). The code is correct; the comment is stale.-/// File header: 4 bytes magic + 32 bytes compressed pubkey = 36 bytes +/// File header: 4 bytes magic + 33 bytes compressed pubkey = 37 bytes const HEADER_MAGIC: &[u8; 4] = b"KDP1"; const HEADER_SIZE: u64 = 37;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/dp_table.rs` around lines 41 - 43, The header comment is stale: it states "4 bytes magic + 32 bytes compressed pubkey = 36 bytes" while the constants HEADER_MAGIC and HEADER_SIZE indicate 37 bytes (correct for a 33-byte compressed secp256k1 pubkey). Update the comment to reflect the correct composition (4 bytes magic + 33 bytes compressed pubkey = 37 bytes) next to HEADER_MAGIC / HEADER_SIZE so the comment matches the code.
369-374:⚠️ Potential issue | 🔴 CriticalFlush failure still publishes the offset — reader won't see the data.
If
writer.flush()fails (line 369), the function logs it but still returnsSome(offset). The data sits inBufWriter's user-space buffer, invisible todp_reader(separate file handle). Nextcheck_collision()call will fail to read this DP and miss a potential collision.if let Err(e) = writer.flush() { tracing::error!("Failed to flush DP file: {}", e); + return None; } self.next_offset += DP_RECORD_SIZE; Some(offset)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/dp_table.rs` around lines 369 - 374, The function currently logs writer.flush() errors but still advances self.next_offset by DP_RECORD_SIZE and returns Some(offset), which publishes an invisible record; change the flow in the method that calls writer.flush() so that on Err(e) you do NOT advance self.next_offset or return Some(offset) — instead return None (or propagate the error) so the offset isn't published; only increment self.next_offset and return Some(offset) after a successful writer.flush(). Ensure this touches the block using writer.flush(), self.next_offset, DP_RECORD_SIZE and keep check_collision()/dp_reader behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cpu/dp_table.rs`:
- Around line 180-195: DPTable::new currently ignores
File::open/OpenOptions::open failures and returns a usable DPTable with no
reader/writer; change the constructor to return Result<DPTable, std::io::Error>
(or a custom error) so callers must handle failures: update DPTable::new to
propagate errors from File::open and OpenOptions::open (return Err on any
failure to open the persistent file), remove the silent tracing::error path that
still returns a table, and update callers (solver code that calls DPTable::new)
to handle the Result; alternatively, if you must keep a non-fallible
constructor, add a private bool field (e.g., healthy) set to false on any open
failure and expose a public is_healthy(&self) -> bool that solvers can poll
before using insert_and_check (also ensure insert_and_check checks is_healthy
and returns an error rather than silently dropping DPs).
- Around line 263-278: The truncation of the DP file is executed
unconditionally; move the
OpenOptions::new().write(true).open(&self.dp_file_path) and subsequent
set_len(valid_size) block inside the if remainder != 0 branch so set_len is only
called when there are trailing bytes to remove; use the existing variables
(remainder, expected, HEADER_SIZE, DP_RECORD_SIZE, self.dp_file_path) and keep
the tracing::warn!/tracing::error! behavior and error handling as-is.
---
Duplicate comments:
In `@src/cpu/dp_table.rs`:
- Around line 41-43: The header comment is stale: it states "4 bytes magic + 32
bytes compressed pubkey = 36 bytes" while the constants HEADER_MAGIC and
HEADER_SIZE indicate 37 bytes (correct for a 33-byte compressed secp256k1
pubkey). Update the comment to reflect the correct composition (4 bytes magic +
33 bytes compressed pubkey = 37 bytes) next to HEADER_MAGIC / HEADER_SIZE so the
comment matches the code.
- Around line 369-374: The function currently logs writer.flush() errors but
still advances self.next_offset by DP_RECORD_SIZE and returns Some(offset),
which publishes an invisible record; change the flow in the method that calls
writer.flush() so that on Err(e) you do NOT advance self.next_offset or return
Some(offset) — instead return None (or propagate the error) so the offset isn't
published; only increment self.next_offset and return Some(offset) after a
successful writer.flush(). Ensure this touches the block using writer.flush(),
self.next_offset, DP_RECORD_SIZE and keep check_collision()/dp_reader behavior
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 900551e4-a7ec-4d1e-9b4b-b0778181423f
📒 Files selected for processing (1)
src/cpu/dp_table.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
src/cpu/dp_table.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/cpu/dp_table.rs: Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Files:
src/cpu/dp_table.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/**/*.rs : Never add y_parity or symClass to GpuDistinguishedPoint; collision resolution must be done CPU-side via compute_candidate_keys()
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/buffers.rs : Clamp DP buffer to 90% capacity per dispatch to prevent overflow
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/crypto/mod.rs : Implement key verification using verify_key and full_verify functions in src/crypto/mod.rs
Applied to files:
src/cpu/dp_table.rs
🔇 Additional comments (2)
src/cpu/dp_table.rs (2)
668-677: Tests look solid with proper cleanup and unique range_bits per test.Good changes: each test uses distinct
range_bits(40-42, 98-99) to avoid interference, andcleanup_test_file()runs at both start and end. The persistence test (lines 717-749) correctly verifies cross-session collision detection, which is the core value prop of this PR.
512-564: Collision resolution logic is correct.The scalar arithmetic for tame↔wild and wild₁↔wild₂ collisions follows the standard kangaroo algorithm.
scalar_half()property (2 × SCALAR_HALF ≡ 1 mod n) is correctly tested at line 698.
| // Open persistent reader for collision checks | ||
| if let Ok(f) = File::open(&self.dp_file_path) { | ||
| self.dp_reader = Some(f); | ||
| } | ||
|
|
||
| // Open writer for appending new DPs | ||
| if let Ok(f) = OpenOptions::new() | ||
| .create(true) | ||
| .append(true) | ||
| .open(&self.dp_file_path) | ||
| { | ||
| tracing::info!("DP persistence: {}", self.dp_file_path); | ||
| self.dp_writer = Some(BufWriter::new(f)); | ||
| } else { | ||
| tracing::error!("Cannot open DP file for writing: {}", self.dp_file_path); | ||
| } |
There was a problem hiding this comment.
If file handles fail to open, solver runs with collision detection silently disabled.
When File::open (line 181) or OpenOptions::new()...open() (lines 186-189) fails, DPTable::new() still returns a valid table. Every subsequent insert_and_check() call:
- Calls
check_collision()→ returnsNone(no reader) - Calls
write_dp_to_disk()→ returnsNone(no writer) - Silently drops the DP
The solver burns GPU cycles thinking it's accumulating DPs, but no collision can ever be found. For a feature promising "zero DP loss", this is rough.
Either return Result<DPTable, _> to let callers decide, or add an is_healthy() check that solvers can poll.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cpu/dp_table.rs` around lines 180 - 195, DPTable::new currently ignores
File::open/OpenOptions::open failures and returns a usable DPTable with no
reader/writer; change the constructor to return Result<DPTable, std::io::Error>
(or a custom error) so callers must handle failures: update DPTable::new to
propagate errors from File::open and OpenOptions::open (return Err on any
failure to open the persistent file), remove the silent tracing::error path that
still returns a table, and update callers (solver code that calls DPTable::new)
to handle the Result; alternatively, if you must keep a non-fallible
constructor, add a private bool field (e.g., healthy) set to false on any open
failure and expose a public is_healthy(&self) -> bool that solvers can poll
before using insert_and_check (also ensure insert_and_check checks is_healthy
and returns an error rather than silently dropping DPs).
There was a problem hiding this comment.
3 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/cpu/dp_table.rs">
<violation number="1" location="src/cpu/dp_table.rs:270">
P2: The truncation block sits outside the `if remainder != 0` guard - it runs on every file load, not just when there's a partial record. The extra indentation makes it look like it belongs inside the `if`, and it almost certainly should be. Move the closing `}` of the `if remainder != 0` block to after this truncation code, or the intent of "truncate only corrupted files" doesn't match what actually executes.</violation>
<violation number="2" location="src/cpu/dp_table.rs:272">
P1: Startup truncation uses a stale file-size snapshot and can truncate valid concurrently appended DP records.</violation>
<violation number="3" location="src/cpu/dp_table.rs:369">
P1: Unconditional per-DP `BufWriter::flush()` in the write path introduces likely I/O throughput regression on the hot insert path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| tracing::error!("Failed to write DP to disk: {} — DP may be lost!", e); | ||
| return None; | ||
| } | ||
| if let Err(e) = writer.flush() { |
There was a problem hiding this comment.
P1: Unconditional per-DP BufWriter::flush() in the write path introduces likely I/O throughput regression on the hot insert path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cpu/dp_table.rs, line 369:
<comment>Unconditional per-DP `BufWriter::flush()` in the write path introduces likely I/O throughput regression on the hot insert path.</comment>
<file context>
@@ -360,10 +366,8 @@ impl DPTable {
- if let Err(e) = writer.flush() {
- tracing::error!("Failed to flush DP file: {}", e);
- }
+ if let Err(e) = writer.flush() {
+ tracing::error!("Failed to flush DP file: {}", e);
}
</file context>
- Truncation only runs when partial records are detected - Flush after every write to ensure reader sees latest data - Remove blank lines from build_index
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/cpu/dp_table.rs (4)
180-195:⚠️ Potential issue | 🟠 MajorFile handle failures leave solver running with collision detection disabled.
If
File::open(line 181) orOpenOptions::new()...open()(lines 186-189) fails,DPTable::new()still returns a valid table. Every subsequentinsert_and_check():
check_collision()returnsNone(no reader)write_dp_to_disk()returnsNone(no writer)- DP is silently dropped
The solver burns cycles thinking it's accumulating DPs, but no collision can ever be found. For "zero DP loss," this is a silent correctness failure.
Either return
Result<DPTable, _>or exposeis_healthy()so solvers can bail early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/dp_table.rs` around lines 180 - 195, DPTable::new currently continues when opening dp_file_path fails, leaving dp_reader/dp_writer None so insert_and_check/check_collision/write_dp_to_disk silently no-op; change DPTable::new to propagate file open errors by returning Result<DPTable, std::io::Error> (or a custom error) instead of constructing a partially broken table, updating call sites to handle the Result, or alternatively set an explicit health flag (e.g., self.is_healthy) and expose an is_healthy() method so callers can bail; update DPTable::new, the dp_reader/dp_writer handling, and any callers of DPTable::new and insert_and_check to handle the new error or health check, ensuring failures to open dp_file_path are not ignored.
41-43:⚠️ Potential issue | 🟡 MinorComment still says 32 bytes but HEADER_SIZE is 37.
Line 41 says "4 bytes magic + 32 bytes compressed pubkey = 36 bytes" but HEADER_SIZE is 37. Compressed secp256k1 pubkey is 33 bytes (02/03 prefix + 32-byte X). The code is correct, comment is stale.
-/// File header: 4 bytes magic + 32 bytes compressed pubkey = 36 bytes +/// File header: 4 bytes magic + 33 bytes compressed pubkey = 37 bytes const HEADER_MAGIC: &[u8; 4] = b"KDP1"; const HEADER_SIZE: u64 = 37;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/dp_table.rs` around lines 41 - 43, Update the stale header comment to reflect the correct compressed secp256k1 pubkey size: change the description from "4 bytes magic + 32 bytes compressed pubkey = 36 bytes" to "4 bytes magic + 33 bytes compressed pubkey = 37 bytes" so it matches the constants HEADER_MAGIC and HEADER_SIZE in dp_table.rs.
368-372:⚠️ Potential issue | 🟠 MajorFlush error doesn't prevent offset from being published.
If
writer.flush()fails (line 368-370), you still returnSome(offset)and the caller adds it to the index. The reader might not see the data yet (or ever if the error persists), causing silent collision misses.if let Err(e) = writer.flush() { tracing::error!("Failed to flush DP file: {}", e); + return None; } self.next_offset += DP_RECORD_SIZE; Some(offset)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/dp_table.rs` around lines 368 - 372, The code advances self.next_offset by DP_RECORD_SIZE and returns Some(offset) even when writer.flush() fails; change the logic in the function containing writer.flush() so that if writer.flush() returns Err(e) you log the error and do not increment self.next_offset or return Some(offset) (instead return None or propagate the error), ensuring the offset is only published after a successful flush; adjust the order so writer.flush() is checked before self.next_offset += DP_RECORD_SIZE and before returning offset.
275-277: 🧹 Nitpick | 🔵 TrivialInconsistent indentation on line 275.
The
if expected == 0block has extra leading space, making it look like it's inside theif remainder != 0block when it's not. Minor formatting nit.- if expected == 0 { + if expected == 0 { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/dp_table.rs` around lines 275 - 277, The `if expected == 0` block is indented one level too deep relative to the surrounding code (it appears to be inside the `if remainder != 0` block); fix by aligning the `if expected == 0 { return; }` statement with the outer block (same indentation level as the `if remainder != 0`), ensuring it sits at the correct scope next to the `remainder` check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cpu/dp_table.rs`:
- Around line 119-121: The code currently swallows errors from
fs::create_dir_all(&dp_dir) by only logging them, which hides persistence
failures from callers and leads to confusing errors later in init_file; change
the constructor/initializer that calls fs::create_dir_all (where dp_dir is used
and init_file is invoked) to return Result<Self, io::Error> and propagate the
create_dir_all error (using ? or map_err) instead of just logging so callers can
detect and handle persistence initialization failures.
- Line 168: The call to fs::rename(&self.dp_file_path, &backup) currently
ignores failures which can lead to data loss; change this to handle the Result:
check the Err case from fs::rename inside the same method (the block using
self.dp_file_path and backup), log the error with context (including paths) and
then fall back to a safe alternative such as copying the file to backup and
removing the original (or return an Err) instead of proceeding to
overwrite—ensure you reference and handle fs::rename, self.dp_file_path, and
backup in that method.
- Around line 380-384: The current match uses r.clone() when reading from
self.index.get(&hash_key) which allocates a new Vec on every collision check;
replace that clone with a stack-optimized SmallVec to avoid heap allocation for
the common small-case: add smallvec crate, change the refs local to something
like SmallVec<[OffsetType; N]>, and construct it with SmallVec::from_slice(r)
(or extend_from_slice) instead of r.clone(); update the use sites that expect
refs to work with SmallVec (it implements slice/iter APIs) and import
smallvec::SmallVec; the key symbols to edit are the match on
self.index.get(&hash_key), the r.clone() call, and the refs variable.
---
Duplicate comments:
In `@src/cpu/dp_table.rs`:
- Around line 180-195: DPTable::new currently continues when opening
dp_file_path fails, leaving dp_reader/dp_writer None so
insert_and_check/check_collision/write_dp_to_disk silently no-op; change
DPTable::new to propagate file open errors by returning Result<DPTable,
std::io::Error> (or a custom error) instead of constructing a partially broken
table, updating call sites to handle the Result, or alternatively set an
explicit health flag (e.g., self.is_healthy) and expose an is_healthy() method
so callers can bail; update DPTable::new, the dp_reader/dp_writer handling, and
any callers of DPTable::new and insert_and_check to handle the new error or
health check, ensuring failures to open dp_file_path are not ignored.
- Around line 41-43: Update the stale header comment to reflect the correct
compressed secp256k1 pubkey size: change the description from "4 bytes magic +
32 bytes compressed pubkey = 36 bytes" to "4 bytes magic + 33 bytes compressed
pubkey = 37 bytes" so it matches the constants HEADER_MAGIC and HEADER_SIZE in
dp_table.rs.
- Around line 368-372: The code advances self.next_offset by DP_RECORD_SIZE and
returns Some(offset) even when writer.flush() fails; change the logic in the
function containing writer.flush() so that if writer.flush() returns Err(e) you
log the error and do not increment self.next_offset or return Some(offset)
(instead return None or propagate the error), ensuring the offset is only
published after a successful flush; adjust the order so writer.flush() is
checked before self.next_offset += DP_RECORD_SIZE and before returning offset.
- Around line 275-277: The `if expected == 0` block is indented one level too
deep relative to the surrounding code (it appears to be inside the `if remainder
!= 0` block); fix by aligning the `if expected == 0 { return; }` statement with
the outer block (same indentation level as the `if remainder != 0`), ensuring it
sits at the correct scope next to the `remainder` check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9dffb8d4-0fca-42fd-923d-6cf9228ca0f8
📒 Files selected for processing (1)
src/cpu/dp_table.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
src/cpu/dp_table.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/cpu/dp_table.rs: Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Files:
src/cpu/dp_table.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/**/*.rs : Never add y_parity or symClass to GpuDistinguishedPoint; collision resolution must be done CPU-side via compute_candidate_keys()
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/buffers.rs : Clamp DP buffer to 90% capacity per dispatch to prevent overflow
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/crypto/mod.rs : Implement key verification using verify_key and full_verify functions in src/crypto/mod.rs
Applied to files:
src/cpu/dp_table.rs
🔇 Additional comments (7)
src/cpu/dp_table.rs (7)
57-65: Good: converts to affine before encoding.
pubkey_to_bytescorrectly convertsProjectivePointto affine before SEC1 encoding. This aligns with the guideline to never store Jacobian coordinates in DP storage.
75-81: RAM index design is efficient.DiskRef at 12 bytes per DP (8-byte offset + 4-byte ktype) is a significant improvement over storing full 68-byte records in RAM. Good tradeoff.
198-225: Header validation looks solid.Magic check + full 33-byte compressed pubkey comparison prevents cross-puzzle contamination. Old format files (no header) are correctly detected and backed up.
511-563: Collision resolution math unchanged and correct.The
compute_candidate_scalars,compute_candidate_keys, andcompute_candidate_keys_cross_wildfunctions implement standard kangaroo collision resolution. The 8 candidate formulas and cross-wild(d2 - d1) * SCALAR_HALFare textbook.
716-748: Cross-session persistence test validates core feature.This test correctly verifies that a tame DP stored in "session 1" produces a collision when a wild DP at the same point is inserted in "session 2". Good coverage of the main use case.
667-676: Tests properly clean up persistence files.
cleanup_test_filehelper called before and after each test prevents orphan files from polluting the system. Much better than the old design that left files behind.
480-488: Guideline violation:self.total_dpsincremented without clamping to MAX_DISTINGUISHED_POINTS.Per coding guidelines, never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS. Line 485 increments
self.total_dpsdirectly after a successful disk write, with no bounds check. The old design capped this at 65,536; the new design removed that limit for SSD persistence. If unbounded growth is intentional, it needs explicit bounds or safeguards — otherwise this violates the stated guideline.
| if let Err(e) = fs::create_dir_all(&dp_dir) { | ||
| tracing::error!("Failed to create DP directory {}: {}", dp_dir, e); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Directory creation failure logged but not propagated.
If create_dir_all fails, subsequent init_file operations will fail with confusing errors about the file. Current approach logs the error which is acceptable for graceful degradation, but callers have no way to know persistence is broken until writes fail later.
Consider returning Result<Self, io::Error> if persistence is critical to correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cpu/dp_table.rs` around lines 119 - 121, The code currently swallows
errors from fs::create_dir_all(&dp_dir) by only logging them, which hides
persistence failures from callers and leads to confusing errors later in
init_file; change the constructor/initializer that calls fs::create_dir_all
(where dp_dir is used and init_file is invoked) to return Result<Self,
io::Error> and propagate the create_dir_all error (using ? or map_err) instead
of just logging so callers can detect and handle persistence initialization
failures.
| // Clone the refs to avoid borrow conflict with self | ||
| let refs = match self.index.get(&hash_key) { | ||
| Some(r) => r.clone(), | ||
| None => return None, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cloning refs avoids borrow conflict, but allocates on every collision check.
r.clone() at line 382 allocates a new Vec on every collision check, even if there's just one ref. For hot paths with many hash collisions, this adds allocation pressure.
Alternative: extract offsets into a stack-allocated SmallVec or use indices. Not critical given typical workloads, but worth noting if profiling shows allocator contention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cpu/dp_table.rs` around lines 380 - 384, The current match uses r.clone()
when reading from self.index.get(&hash_key) which allocates a new Vec on every
collision check; replace that clone with a stack-optimized SmallVec to avoid
heap allocation for the common small-case: add smallvec crate, change the refs
local to something like SmallVec<[OffsetType; N]>, and construct it with
SmallVec::from_slice(r) (or extend_from_slice) instead of r.clone(); update the
use sites that expect refs to work with SmallVec (it implements slice/iter APIs)
and import smallvec::SmallVec; the key symbols to edit are the match on
self.index.get(&hash_key), the r.clone() call, and the refs variable.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cpu/dp_table.rs`:
- Around line 413-415: The code currently logs recovered private keys when
compute_candidate_keys_cross_wild returns a key (calls to tracing::info! that
include hex::encode(&key)); remove or redact any logging that prints the full
recovered secret in both places where the key is logged (the blocks handling
compute_candidate_keys_cross_wild and the similar branch later), and instead log
a non-sensitive event (e.g., "cross-wild collision detected" or an ID/short hash
up to 8 hex chars) or increment a metric; ensure the variable names (key,
compute_candidate_keys_cross_wild) remain for logic but never output the full
key to tracing or stdout/artifacts.
- Around line 171-174: The Err branch that calls write_header(&pubkey_bytes) is
dangerous because File::create() truncates the file and will wipe accumulated DP
data on transient header I/O errors; change this to either perform the same
backup routine used in the Ok(false) branch before calling write_header (reuse
the backup logic) or instead return an Err to fail initialization so the file is
left untouched and the table can be marked unhealthy; specifically update the
code path handling validate_header() errors to invoke the backup procedure (or
propagate the error) rather than directly calling write_header.
- Around line 364-370: The current error handling in the writer block
(writer.write_all / writer.flush) leaves a potentially poisoned BufWriter in use
and an out-of-date next_offset; on any flush/write error stop reusing that
writer and resync next_offset from the actual file size before accepting more
inserts. Specifically, when writer.write_all(...) or writer.flush(...) returns
Err(e) in the insert path, drop/close the current BufWriter instance referenced
by the DP file writer, mark the writer as invalid, obtain the actual on-disk
length via the underlying File (e.g., file.metadata().len() or
file.seek(SeekFrom::End(0))) and set next_offset to that length, and ensure
future inserts recreate a fresh BufWriter/File and rewind state before allowing
writes. Ensure this logic is applied around the code handling writer, write_all,
flush and next_offset so a subsequent successful insert cannot publish a stale
offset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b0ab929-f6a5-4850-a983-89968863a4b9
📒 Files selected for processing (1)
src/cpu/dp_table.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
src/cpu/dp_table.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/cpu/dp_table.rs: Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Files:
src/cpu/dp_table.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/**/*.rs : Never add y_parity or symClass to GpuDistinguishedPoint; collision resolution must be done CPU-side via compute_candidate_keys()
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/buffers.rs : Clamp DP buffer to 90% capacity per dispatch to prevent overflow
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/init.rs : Never split jump table for negation map; use add/sub operations with the same 256-entry table
Applied to files:
src/cpu/dp_table.rs
- Don't log private keys (security risk) - Continue scanning after failed collision candidate - Backup file before overwriting on header read error
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/cpu/dp_table.rs (1)
189-204: 🧹 Nitpick | 🔵 TrivialReader failure is silent while writer failure is logged.
Line 190-192: If
File::openfails for the reader, no log is produced. The asymmetry with the writer (which logs at 203) is inconsistent. Worth a log here too for parity.// Open persistent reader for collision checks -if let Ok(f) = File::open(&self.dp_file_path) { - self.dp_reader = Some(f); +match File::open(&self.dp_file_path) { + Ok(f) => self.dp_reader = Some(f), + Err(e) => tracing::error!("Cannot open DP file for reading: {}", e), }The bigger issue (DPTable returning valid when handles fail) was already flagged in previous review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpu/dp_table.rs` around lines 189 - 204, The File::open call that sets self.dp_reader currently swallows errors; change it to log the failure (e.g., tracing::warn or tracing::error) including self.dp_file_path and the captured error when File::open(&self.dp_file_path) returns Err, mirroring the writer branch; update the dp_reader assignment block (where dp_reader is set) to use match or if let Err(e) to emit the log. Also ensure any higher-level DPTable construction logic that previously allowed returning a valid table when both reader and writer failed is preserved or adjusted to return an error if both handles are missing (check the constructor/initializer that sets dp_reader/dp_writer).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cpu/dp_table.rs`:
- Around line 277-283: The truncation can silently fail on Windows because the
original file handle (`file`) is still open; before calling
OpenOptions::new().write(true).open(&self.dp_file_path) and
f.set_len(valid_size) you must close/drop the existing `file` handle (the
reader/BufReader created earlier) so the new write handle can set_len; then
reopen the file for reading (BufReader) after successful truncation and
propagate/set the error from set_len instead of silently swallowing it. Ensure
you reference and drop the original `file` handle, compute valid_size using
HEADER_SIZE and DP_RECORD_SIZE as before, call set_len on the fresh write
handle, handle/report any error, and then reopen the file for buffered reading.
---
Duplicate comments:
In `@src/cpu/dp_table.rs`:
- Around line 189-204: The File::open call that sets self.dp_reader currently
swallows errors; change it to log the failure (e.g., tracing::warn or
tracing::error) including self.dp_file_path and the captured error when
File::open(&self.dp_file_path) returns Err, mirroring the writer branch; update
the dp_reader assignment block (where dp_reader is set) to use match or if let
Err(e) to emit the log. Also ensure any higher-level DPTable construction logic
that previously allowed returning a valid table when both reader and writer
failed is preserved or adjusted to return an error if both handles are missing
(check the constructor/initializer that sets dp_reader/dp_writer).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52b16fe8-5efb-4df2-a6af-034b4e73d3eb
📒 Files selected for processing (1)
src/cpu/dp_table.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
src/cpu/dp_table.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/cpu/dp_table.rs: Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Files:
src/cpu/dp_table.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Implement collision detection using the Distinguished Points (DP) pattern in src/cpu/dp_table.rs
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never use Jacobian coordinates in final DP storage; must convert to affine coordinates
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/dp_table.rs : Never trust DP buffer count without clamping to MAX_DISTINGUISHED_POINTS
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/buffers.rs : Clamp DP buffer to 90% capacity per dispatch to prevent overflow
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/gpu/**/*.rs : Never add y_parity or symClass to GpuDistinguishedPoint; collision resolution must be done CPU-side via compute_candidate_keys()
Applied to files:
src/cpu/dp_table.rs
📚 Learning: 2026-02-20T07:57:41.039Z
Learnt from: CR
Repo: oritwoen/kangaroo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T07:57:41.039Z
Learning: Applies to src/cpu/init.rs : Never split jump table for negation map; use add/sub operations with the same 256-entry table
Applied to files:
src/cpu/dp_table.rs
🔇 Additional comments (12)
src/cpu/dp_table.rs (12)
1-27: Architecture docs are clear and accurate.Good breakdown of RAM vs SSD trade-offs with the memory table. Header size (37 bytes) now matches
HEADER_SIZEconstant.
67-81: Index-vs-storage split is well designed.DiskRef at 12 bytes/entry vs StoredDP at 68 bytes - the memory savings are real. StoredDP correctly uses
affine_x(not Jacobian). Based on learnings: "Never use Jacobian coordinates in final DP storage; must convert to affine coordinates."
171-183: Backup-before-overwrite is now properly implemented for error cases.Good fix. The
Err(e)branch now backs up the file before callingwrite_header(), matching theOk(false)branch behavior. This prevents data loss on transient I/O errors.
207-251: Header validation and writing are correct.Magic check + full 33-byte compressed pubkey comparison. Comments now accurately describe what's happening.
362-383: write_dp_to_disk now handles errors properly and flushes every write.Returns
Option<u64>, logs errors, flushes immediately. This ensures the reader (separate handle) sees the data before the offset gets published to the index. Addresses previous concerns about phantom entries and data visibility.The per-write flush is necessary for correctness but adds syscall overhead. If perf becomes an issue, you could buffer writes and only check collision after flush - but that's a larger refactor.
422-426: Good: continues scanning after failed cross-wild candidate.Previous review flagged that returning early after a failed
affine_xmatch missed subsequent valid collisions in the same bucket. Now it properlycontinues to check other refs. Same fix at line 443.
422-424: Good: private keys no longer logged.Now just logs "Cross-wild collision resolved" and "Collision found" without the actual key bytes. Prevents secrets leaking to stdout/logs/CI.
Also applies to: 439-440
491-498: Only indexes after successful disk write - good.Previous reviews flagged phantom index entries when
write_dp_to_diskfailed but still returned an offset. Now it returnsOption<u64>and index update is gated onSome(offset).
521-573: Collision resolution math looks unchanged and correct.
scalar_half()verified by test at line 707. Cross-wild formula(d2 - d1) * halfhandles wild₁↔wild₂ correctly.
669-675: Test cleanup properly implemented.Each test now uses a unique
range_bitsand callscleanup_test_filebefore/after. No more orphandps_range*.binfiles polluting dev machines.Also applies to: 678-686
726-758: Cross-session persistence test is solid.Session 1 inserts tame DP, drops table. Session 2 opens same file, inserts wild DP at same point, expects collision. This validates the core feature: "zero DP loss between sessions."
760-794: Pubkey mismatch test validates backup behavior.Session 1: puzzle A writes. Session 2: puzzle B (different pubkey, same range_bits) should see
total_dps == 0because old data was backed up and file recreated. Also cleans up backup files.
| if let Ok(f) = OpenOptions::new().write(true).open(&self.dp_file_path) { | ||
| let valid_size = HEADER_SIZE + expected * DP_RECORD_SIZE; | ||
| if let Err(e) = f.set_len(valid_size) { | ||
| tracing::error!("Failed to truncate DP file: {}", e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Windows: truncation may silently fail due to sharing violation.
At this point file (opened at line 257) is still live. On Windows you can't set_len() on a separate handle while another handle holds the file. The if let Ok(f) swallows the failure.
Unix: works fine. Windows: truncation silently skipped, leaving corrupted tail intact.
Fix: close file before truncating, reopen for BufReader. Not urgent unless Windows support matters.
Sketch
fn build_index(&mut self) {
- let file = match File::open(&self.dp_file_path) {
+ let file_size = match std::fs::metadata(&self.dp_file_path) {
+ Ok(m) => m.len(),
+ Err(_) => return,
+ };
...
if remainder != 0 {
tracing::warn!(...);
+ // No competing read handle now
if let Ok(f) = OpenOptions::new().write(true).open(&self.dp_file_path) {
...
+ // Now open for reading
+ let file = match File::open(&self.dp_file_path) {
+ Ok(f) => f,
+ Err(_) => return,
+ };
let mut reader = BufReader::new(file);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cpu/dp_table.rs` around lines 277 - 283, The truncation can silently fail
on Windows because the original file handle (`file`) is still open; before
calling OpenOptions::new().write(true).open(&self.dp_file_path) and
f.set_len(valid_size) you must close/drop the existing `file` handle (the
reader/BufReader created earlier) so the new write handle can set_len; then
reopen the file for reading (BufReader) after successful truncation and
propagate/set the error from set_len instead of silently swallowing it. Ensure
you reference and drop the original `file` handle, compute valid_size using
HEADER_SIZE and DP_RECORD_SIZE as before, call set_len on the fresh write
handle, handle/report any error, and then reopen the file for buffered reading.
|
All critical and major issues have been addressed:
Remaining suggestions (poisoned BufWriter recovery, Result return type, All 73 tests pass. Tested on Apple Silicon M4 Max at 27 MKeys/s. |
1 participant
Problem
The current implementation stores DPs in RAM with a hard cap of 65,536.
Once the table is full, new DPs overwrite old ones, causing potential
collision misses between tame and wild kangaroos.
Solution
~/Desktop/kangaroo_dps/dps_range{bits}.binPerformance impact
Files changed
src/cpu/dp_table.rs— rewritten for SSD persistencesrc/solver.rs— pass range_bits to DPTable::newsrc/lib.rs— pass range_bits to DPTable::newTesting