Conversation
This was referenced May 12, 2026
Collaborator
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
bmuddha
force-pushed
the
nucleus
branch
2 times, most recently
from
May 14, 2026 14:46
410f558 to
d323e3a
Compare
This was referenced May 28, 2026
bmuddha
force-pushed
the
nucleus
branch
4 times, most recently
from
May 28, 2026 14:42
fddd6f0 to
c01202f
Compare
📝 WalkthroughWalkthroughChangesEngine primitives and calculator interface
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OS
participant ShutdownManager
participant ShutdownHandle
participant Service
OS->>ShutdownManager: Send shutdown signal
ShutdownManager->>ShutdownHandle: Cancel service tier
ShutdownHandle->>Service: Notify cancellation
Service->>ShutdownManager: Report termination reason
ShutdownManager->>ShutdownManager: Drain tier with timeout
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 14 UNAVAILABLE: Connection dropped 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 |
bmuddha
force-pushed
the
nucleus
branch
2 times, most recently
from
May 29, 2026 15:49
02c3fc8 to
5400e0f
Compare
bmuddha
force-pushed
the
nucleus
branch
3 times, most recently
from
June 30, 2026 21:16
a6754d6 to
9732110
Compare
bmuddha
force-pushed
the
nucleus
branch
3 times, most recently
from
July 3, 2026 22:01
9d93c24 to
5ea2822
Compare
bmuddha
force-pushed
the
nucleus
branch
2 times, most recently
from
July 6, 2026 12:36
88dd3ba to
88afbef
Compare
bmuddha
marked this pull request as ready for review
July 6, 2026 15:05
This was referenced Jul 27, 2026
Collaborator
Author
|
@CodeRabbit review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nucleus/src/heed.rs`:
- Around line 27-45: Remove the lifetime-extending transmute calls from
DatabaseIndex::write_txn and DatabaseIndex::read_txn. Bind newly opened
transactions to the borrow of self.env(), or adopt an owner that keeps the Env
alive, while preserving supplied transactions and caller-owned storage without
requiring unsafe lifetime assumptions.
In `@nucleus/src/testkit.rs`:
- Around line 60-66: Update the block function’s hash construction so all bits
of Slot contribute to a deterministic hash, avoiding collisions between slots
that differ beyond the lowest byte. Preserve the existing block fields and
ensure distinct Slot values produce distinct hashes within the supported Slot
range.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ac2e5c95-96a6-453e-a119-976b7e354a00
📒 Files selected for processing (18)
Cargo.tomlnucleus/Cargo.tomlnucleus/README.mdnucleus/src/config.rsnucleus/src/heed.rsnucleus/src/ledger.rsnucleus/src/lib.rsnucleus/src/metrics.rsnucleus/src/notifier.rsnucleus/src/runtime.rsnucleus/src/shutdown.rsnucleus/src/testkit.rsnucleus/src/tls.rsprograms/v42-calculator-interface/Cargo.tomlprograms/v42-calculator-interface/README.mdprograms/v42-calculator-interface/src/builder.rsprograms/v42-calculator-interface/src/lib.rsprograms/v42-calculator-interface/src/opcodes.rs
Comment on lines
+27
to
+45
| fn write_txn<'t, 'e>(&self, txn: OptRwTxn<'t, 'e>) -> Result<&'t mut RwTxn<'e>> { | ||
| if let Some(txn) = txn { | ||
| return Ok(txn); | ||
| } | ||
| // SAFETY: guaranteed by the trait contract. The transaction is stored | ||
| // in the caller-owned option and must be dropped before `env`. | ||
| let write = unsafe { mem::transmute::<RwTxn<'_>, RwTxn<'e>>(self.env().write_txn()?) }; | ||
| Ok(txn.insert(write)) | ||
| } | ||
|
|
||
| /// Uses the supplied read transaction or opens one on demand. | ||
| fn read_txn<'t, 'e>(&self, txn: OptRoTxn<'t, 'e>) -> Result<&'t RoTxnTls<'e>> { | ||
| if let Some(txn) = txn { | ||
| return Ok(txn); | ||
| } | ||
| // SAFETY: guaranteed by the trait contract. The transaction is stored | ||
| // in the caller-owned option and must be dropped before `env`. | ||
| let read = unsafe { mem::transmute::<RoTxnTls<'_>, RoTxnTls<'e>>(self.env().read_txn()?) }; | ||
| Ok(txn.insert(read)) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Do not extend transaction lifetimes with transmute.
'e comes from the caller-owned Option, but it is not bound to &self. A safe caller can pass Option<RwTxn<'static>>::None, store the widened transaction, and drop the index before that transaction. This can access a dropped Env during transaction use or destruction.
Bind the transaction lifetime to the Env borrow, or use a transaction owner that keeps the Env alive. Do not require safe callers to uphold an unsafe lifetime condition. Heed defines Env::write_txn and Env::read_txn with lifetimes borrowed from Env; its explicit static read transaction instead owns the environment. (docs.rs)
#!/bin/bash
set -euo pipefail
# Find every implementation and use of the lifetime-extending API.
rg -n -C 5 'unsafe\s+impl.*DatabaseIndex|impl.*DatabaseIndex|\.write_txn\(|\.read_txn\(' \
--glob '*.rs'
# Inspect the trait and all transaction-storage declarations.
rg -n -C 8 'DatabaseIndex|OptRwTxn|OptRoTxn|RwTxn|RoTxnTls|transmute' \
nucleus/src/heed.rs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nucleus/src/heed.rs` around lines 27 - 45, Remove the lifetime-extending
transmute calls from DatabaseIndex::write_txn and DatabaseIndex::read_txn. Bind
newly opened transactions to the borrow of self.env(), or adopt an owner that
keeps the Env alive, while preserving supplied transactions and caller-owned
storage without requiring unsafe lifetime assumptions.
Comment on lines
+60
to
+66
| /// A block boundary with a distinct hash and time derived from `slot`. | ||
| pub fn block(slot: Slot) -> Block { | ||
| Block { | ||
| slot, | ||
| hash: Hash::new_from_array([slot as u8; 32]), | ||
| time: slot as i64, | ||
| parent: Hash::default(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Generate a unique deterministic hash for each Slot.
slot as u8 discards all higher bits. For example, slots 1 and 257 produce the same hash. This contradicts the block rustdoc and can alias different block identities in tests.
Proposed fix
pub fn block(slot: Slot) -> Block {
+ let mut hash = [0; 32];
+ hash[..8].copy_from_slice(&slot.to_le_bytes());
Block {
slot,
- hash: Hash::new_from_array([slot as u8; 32]),
+ hash: Hash::new_from_array(hash),
time: slot as i64,
parent: Hash::default(),
}
}📝 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.
Suggested change
| /// A block boundary with a distinct hash and time derived from `slot`. | |
| pub fn block(slot: Slot) -> Block { | |
| Block { | |
| slot, | |
| hash: Hash::new_from_array([slot as u8; 32]), | |
| time: slot as i64, | |
| parent: Hash::default(), | |
| /// A block boundary with a distinct hash and time derived from `slot`. | |
| pub fn block(slot: Slot) -> Block { | |
| let mut hash = [0; 32]; | |
| hash[..8].copy_from_slice(&slot.to_le_bytes()); | |
| Block { | |
| slot, | |
| hash: Hash::new_from_array(hash), | |
| time: slot as i64, | |
| parent: Hash::default(), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nucleus/src/testkit.rs` around lines 60 - 66, Update the block function’s
hash construction so all bits of Slot contribute to a deterministic hash,
avoiding collisions between slots that differ beyond the lowest byte. Preserve
the existing block fields and ensure distinct Slot values produce distinct
hashes within the supported Slot range.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Added the
nucleuscrate with feature-gated primitives shared across the engine stack.Why
Ledger, runtime, storage, and orchestration crates need common types and lifecycle
coordination without assigning storage or execution policy to the shared dependency.
Closes #28.
Impact
metrics helpers, thread-local runtime state, and test support.
narrow crate features.
ShutdownManager, service handles, ordered cancellation tiers, andbounded termination reporting.
Reviewer notes
Shutdown cancels the replication client, pacemaker, sequencer, and remaining
services in order. Each tier receives its own bounded window to report completion
before shutdown advances.
Follow-up
Ledger, keeper, processor, engine, and replication services consume these
feature-gated primitives upstack.