Conversation
Contributor
Author
snawaz
force-pushed
the
snawaz/size-class
branch
from
02c7ed0 to
293a606
Compare
snawaz
force-pushed
the
snawaz/commit-finalize
branch
from
d579de1 to
f795dc8
Compare
snawaz
force-pushed
the
graphite-base/131
branch
from
293a606 to
1874b4f
Compare
snawaz
force-pushed
the
snawaz/commit-finalize
branch
from
f795dc8 to
fa1cc24
Compare
snawaz
force-pushed
the
snawaz/commit-finalize
branch
from
fa1cc24 to
b2d1983
Compare
snawaz
force-pushed
the
snawaz/commit-finalize
branch
4 times, most recently
from
1323715 to
a3c2954
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Fix all issues with AI agents
In @.github/workflows/run-tests.yml:
- Line 9: Update the Cargo dependency constraints for Solana to match CLI v3.1.5
by changing the solana-related crates in Cargo.toml: replace the current
solana-program constraint (currently ">=1.16, <3.0.0") with a v3 line (either
"=3.0.0" for a pinned on-chain stable build or ">=3.0.0, <4.0.0" if you want the
3.x range) and likewise update dev-dependencies solana-program-test and
solana-sdk to the v3.0.x line to keep them consistent with solana-program.
In `@Cargo.toml`:
- Line 50: The solana-sha256-hasher dependency is pinned to an exact version
which is inconsistent with other Solana deps; update the Cargo.toml entry for
solana-sha256-hasher (the dependency name "solana-sha256-hasher") to use a
version range like ">=3.1" to match the style used by other Solana dependencies
and avoid unnecessary version conflicts.
In `@src/args/commit_state.rs`:
- Around line 31-33: The doc comment for the struct field data_is_diff is a
copy‑paste and incorrect; update the comment above pub data_is_diff: Boolean to
accurately describe the field (e.g., "whether this commit contains a data diff /
only the changed data rather than a full snapshot") so the documentation matches
the field's purpose and remove the unrelated text about undelegation; ensure the
comment sits immediately above data_is_diff and uses the same comment style as
the other fields.
In `@src/args/types.rs`:
- Around line 39-53: The from_bytes implementation in ArgsWithBuffer::from_bytes
uses size_of::<H>() instead of the PodView-provided constant H::SPACE; change
the length check and the split_at size to use H::SPACE to remain consistent with
the PodView abstraction (replace size_of::<H>() occurrences with H::SPACE) so
header sizing and parsing align with H::try_view_from().
In `@src/error.rs`:
- Around line 157-164: The ToStr impl for pinocchio::program_error::ToStr on
DlpError is missing the feature guard; add the same cfg attribute used on the
preceding From<DlpError> for pinocchio::program_error::ProgramError
implementation (#[cfg(not(feature = "sdk"))]) to the impl block for ToStr so the
implementation is compiled only when the pinocchio/program feature is enabled
and avoids build failures when the feature is disabled; update the impl for
ToStr (fn to_str<E>(&self) -> &'static str) on DlpError to be annotated with
#[cfg(not(feature = "sdk"))].
In `@src/instruction_builder/commit_finalize.rs`:
- Around line 87-89: The doc comment for the function that "Returns
accounts-data-size budget for commit_state instruction." uses the wrong
instruction name — it should reference commit_finalize not commit_state; update
the docstring text to say "commit_finalize" (or the exact function name that
relates to commit_finalize) so the comment accurately describes the function in
commit_finalize.rs and any mentions of "commit_state" are replaced with
"commit_finalize".
In `@src/pod_view.rs`:
- Around line 139-155: The alignment offset calculation in try_from_unaligned is
using incorrect operator precedence when computing offset from base (the
expression with base, Self::ALIGN and bitwise &/!); change the expression to
explicitly parenthesize the mask operation so offset is computed as ((base +
Self::ALIGN - 1) & !(Self::ALIGN - 1)) - base, then use that offset to slice
oversized_memory and copy unaligned_buffer before calling Self::try_view_from
and mapping to *view; ensure you reference oversized_memory, base, offset,
Self::ALIGN, Self::SPACE and try_view_from when applying the fix.
In `@src/processor/fast/commit_finalize.rs`:
- Around line 15-41: The doc-comment for commit_finalize.rs is out-of-sync: it
lists 8 accounts and describes creating PDAs for commit-state and commit-record,
but the implementation accepts 7 accounts and writes state directly to the
delegated account and only creates a commit record PDA; update the top comment
and the "Requirements/Steps" to match the code by (1) changing the account list
to the actual seven accounts used by the implementation (include the signer,
delegated account, writable delegated-state account (if applicable in names
used), commit record PDA, delegation record, delegation metadata, validator fees
vault, program config as they appear in the code), (2) remove steps about
initializing a new PDA for the commit-state and copying into it, and instead
state that the new state is applied directly to the delegated account, and (3)
keep Instruction Data type (CommitFinalizeArgsWithBuffer) and ensure the
requirements reflect the actual checks performed (e.g., commit state
uninitialized vs. applied-on-delegated-account). Reference symbols:
commit_finalize.rs top comment and the commit_finalize function/handler and
CommitFinalizeArgsWithBuffer; adjust numbering and step descriptions to exactly
mirror the implementation.
In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 151-158: The commented block referencing
args.commit_record_lamports is dead code because CommitFinalizeInternalArgs no
longer contains commit_record_lamports; either delete this commented Transfer
block entirely from commit_finalize_internal.rs, or replace it with a
single-line TODO comment that clearly states that the transfer logic should be
reintroduced when CommitFinalizeInternalArgs is extended with
commit_record_lamports (include a short ticket/issue reference if available),
and ensure you update/remove any references to delegation_record.lamports at the
same time; locate the block by searching for the commented Transfer invocation
inside the commit_finalize_internal logic that mentions
args.commit_record_lamports and delegation_record.lamports.
- Around line 40-112: Remove the dead if/else wrapper and keep only the chosen
PDA validation calls (either the safe require_initialized_pda or the unsafe
require_initialized_pda_unsafe variant) — delete the entire `if false { ... }
else { ... }` block and leave a single sequence of the three validation calls
for `args.delegation_record_account`, `args.delegation_metadata_account`, and
`args.validator_fees_vault`; ensure the kept calls use the correct parameter
list (include or exclude `PDA_MARKER` as appropriate) and preserve the same bump
fields `args.bumps.delegation_record`, `args.bumps.delegation_metadata`,
`args.bumps.validator_fees_vault` and the same owner `&crate::fast::ID`.
- Around line 130-135: Remove the dead `if false` branch when loading the
delegation record: drop the unused path that calls
DelegationRecord::try_from_bytes_with_discriminator and keep the PodView-based
call DelegationRecord::try_view_from(&delegation_record_data.as_ref()[8..]) so
the code only uses the working view deserialization; ensure you leave the
variable name delegation_record and error handling as-is (propagate the existing
? error) and remove the conditional entirely.
- Around line 195-204: Add a defensive length check before calling
copy_from_slice in the NewState::FullBytes arm: after resizing via
args.delegated_account.resize(...) and obtaining delegated_account_data via
args.delegated_account.try_borrow_mut_data(), verify
delegated_account_data.len() equals bytes.len() and return a clear error (or map
to the existing error type) if they differ; keep the apply_diff_in_place(...)
path unchanged. Reference: args.delegated_account.resize,
delegated_account_data, NewState::FullBytes, copy_from_slice,
apply_diff_in_place.
In `@src/processor/fast/utils/requires.rs`:
- Around line 266-298: The unsafe PDA macros require_initialized_pda_unsafe! and
require_pda_unsafe! currently hash only the provided $seeds and ignore the
$program_id (making callers manually append program_id and PDA_MARKER); change
these macros to internally incorporate $program_id and the PDA_MARKER constant
into the seed derivation (so the hash/create_program_address behavior matches
the safe macros) and validate against that derived PDA, then update the call
sites in commit_finalize_internal.rs to stop manually appending &crate::fast::ID
and PDA_MARKER to seed arrays; ensure you still call require_owned_by!($info,
$program_id) and preserve the existing error returns
(ProgramError::InvalidSeeds/Immutable).
In `@src/state/delegation_metadata.rs`:
- Around line 33-78: The methods in DelegationMetadataFast (from_account,
last_update_nonce, set_last_update_nonce, replace_last_update_nonce,
set_is_undelegatable, replace_is_undelegatable) currently dereference self.data
at offsets based on AccountDiscriminator::SPACE without checking buffer length;
add explicit length guards before any unsafe ptr::read/ptr::write/ptr::replace
to ensure self.data.len() >= AccountDiscriminator::SPACE + N (use N = 8 for u64
reads/writes and N = 9 for the bool byte offset +1 as appropriate) and return a
suitable ProgramError (e.g., ProgramError::InvalidAccountData or a mapped error)
from from_account or have the accessor/mutator return Result to avoid undefined
behavior; update from_account to validate the minimum size upfront so callers
get a safe DelegationMetadataFast or adjust each accessor to check and error
before performing the unsafe operation.
In `@tests/test_commit_finalize.rs`:
- Around line 61-67: The log-count assertion is brittle; replace the
exact-length check on metadata.log_messages with a check that there are no
custom program logs emitted. Locate the assertion around metadata.log_messages
in tests/test_commit_finalize.rs (the block next to
metadata.compute_units_consumed) and change it to verify that none of
metadata.log_messages match program log patterns (e.g., filter/any on entries
containing "Program log:" or your runtime's program-log prefix) so the test
asserts "no program logs" rather than an exact total message count.
Comment on lines
+151
to
+158
| // if args.commit_record_lamports > delegation_record.lamports { | ||
| // system::Transfer { | ||
| // from: args.validator, | ||
| // to: args.commit_state_account, | ||
| // lamports: args.commit_record_lamports - delegation_record.lamports, | ||
| // } | ||
| // .invoke()?; | ||
| // } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove or track commented-out code.
This commented code references args.commit_record_lamports which doesn't exist in the current CommitFinalizeInternalArgs struct. Either remove this dead code or add a TODO comment explaining when this will be implemented.
🤖 Prompt for AI Agents
In `@src/processor/fast/internal/commit_finalize_internal.rs` around lines 151 -
158, The commented block referencing args.commit_record_lamports is dead code
because CommitFinalizeInternalArgs no longer contains commit_record_lamports;
either delete this commented Transfer block entirely from
commit_finalize_internal.rs, or replace it with a single-line TODO comment that
clearly states that the transfer logic should be reintroduced when
CommitFinalizeInternalArgs is extended with commit_record_lamports (include a
short ticket/issue reference if available), and ensure you update/remove any
references to delegation_record.lamports at the same time; locate the block by
searching for the commented Transfer invocation inside the
commit_finalize_internal logic that mentions args.commit_record_lamports and
delegation_record.lamports.
snawaz
commented
Jan 26, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/processor/fast/commit_state.rs (1)
187-209: Move the lamport transfer tocommit_state_accountafter PDA creation.The transfer at lines 203–207 occurs before
create_pdacreates and initializes the account. While Solana allows transfers to non-existent accounts, this creates the account prematurely with lamports but no data/owner, causingcreate_pdato follow the sub-optimal path (allocate after funding) instead of a clean initialization path. The developer's TODO comment directly questions this approach: "commit_state_account does not exist yet. So how do we transfer lamports to non-existent account?" Move the transfer aftercreate_pdacompletes.🐛 Proposed fix
@@ - if args.commit_record_lamports > delegation_record.lamports { - system::Transfer { - from: args.validator, - to: args.commit_state_account, - lamports: args.commit_record_lamports - delegation_record.lamports, - } - .invoke()?; - } + let extra_lamports = args + .commit_record_lamports + .saturating_sub(delegation_record.lamports); @@ create_pda( args.commit_state_account, &crate::fast::ID, args.commit_state_bytes.data_len(), &[Signer::from(&seeds!( pda::COMMIT_STATE_TAG, args.delegated_account.key(), &[commit_state_bump] ))], args.validator, )?; + if extra_lamports > 0 { + system::Transfer { + from: args.validator, + to: args.commit_state_account, + lamports: extra_lamports, + } + .invoke()?; + }Also applies to: 250–262
🤖 Fix all issues with AI agents
In `@src/discriminator.rs`:
- Around line 48-49: Update the doc comments for the fast-path discriminator
variants to reference the correct module path
crate::processor::fast::process_commit_finalize (and analogous fast functions)
instead of crate::processor::process_commit_finalize: change the comment on the
CommitFinalize variant to "See [crate::processor::fast::process_commit_finalize]
for docs." and likewise fix CommitState, CommitDiff, CommitDiffFromBuffer, and
CommitStateFromBuffer doc comments to point to their respective
crate::processor::fast::... symbols so all fast-path variants reference the fast
module functions.
In `@src/entrypoint.rs`:
- Around line 28-31: Replace the use of solana_program::msg! in the slow-path
error branch with pinocchio_log::log! to make entrypoint error logging
consistent with the fast path; locate the slow-path branch that logs the error
(currently calling solana_program::msg!) and change it to call
pinocchio_log::log! with the same message format (e.g.,
"fast_process_instruction: {}", error.to_str::<DlpError>()) so both paths use
pinocchio_log::log! consistently.
In `@src/error.rs`:
- Around line 1-8: The enum currently derives IntoStaticStr and your code calls
to_str, which returns the enum variant name (e.g., InvalidAuthority) instead of
the human‑readable message from #[error("...")]; update each variant to include
#[strum(serialize = "human text")] (matching the thiserror #[error("...")]
string) so IntoStaticStr::to_str yields the intended text, or alternatively
replace uses of to_str with a manual mapper function (e.g., impl ErrorEnum { fn
message(&self)->&'static str { match self { Self::Variant => "human text", ... }
}) that returns the #[error] message; ensure you update all references (calls to
to_str) to use the new serialized strings or the mapper.
In `@src/instruction_builder/commit_finalize.rs`:
- Around line 87-103: commit_finalize_size_budget currently budgets
AccountSizeClass::Tiny for program_config_pda which underestimates ProgramConfig
(BTreeSet<Pubkey>) when approved_validators grows; update the budgeting logic in
commit_finalize_size_budget to use an explicit larger AccountSizeClass (e.g.,
AccountSizeClass::Small) or compute the required AccountSizeClass from the
actual ProgramConfig serialized size (based on 8 + 4 + 32 *
approved_validators.len()) before calling total_size_budget, and add a short
comment documenting the chosen max approved_validators constraint if you opt for
a fixed class; reference commit_finalize_size_budget, ProgramConfig,
AccountSizeClass::Tiny and program_config_pda when making the change.
In `@src/pod_view.rs`:
- Around line 91-96: The doc comment for try_copy_to is incorrect: the function
actually returns ProgramError::InvalidArgument on size mismatch rather than
ErrorCode::SizeMismatch; update the doc on the try_copy_to method to mention
ProgramError::InvalidArgument (or map the implementation to return a specific
size-mismatch error if you prefer a distinct error type). Locate the try_copy_to
signature and its implementation and either change the returned error to a
dedicated size-mismatch error or change the doc text to state that a size
mismatch yields ProgramError::InvalidArgument, including a brief note about the
buffer needing to be exactly Self::SPACE bytes long.
- Around line 50-53: Update the safety documentation in pod_view.rs to remove
the incorrect blanket claim that instruction data is 8-byte aligned; explicitly
state that only Solana account data is guaranteed 8-byte aligned while
instruction data only has its buffer start aligned and its contents may be
unaligned unless you use a custom encoding. Reference the existing bytemuck
usage and mention/point to try_from_unaligned() (currently gated to
#[cfg(test)]) as the provided helper for handling unaligned instruction buffers,
and either note that helper’s test-only status or suggest making an
unaligned-safe conversion available outside tests if needed.
In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 151-184: The program-config whitelist check is currently disabled
by the surrounding `if false`; remove that `if false` wrapper so the validation
runs, and choose the fast variant explicitly by keeping
`require_program_config_fast!(args.program_config_account,
delegation_record.owner.as_array(), args.bumps.program_config, false)` (remove
the alternate `if false` branch and the safe macro). Preserve the existing logic
that loads `program_config` via
`ProgramConfig::try_from_bytes_with_discriminator(&program_config_data)` and
then checks
`program_config.approved_validators.contains(&(*args.validator.key()).into())`,
returning `DlpError::InvalidWhitelistProgramConfig` if not present. Ensure you
keep the `pubkey::log(args.validator.key())` and the `try_borrow_data()?` usage
so behavior remains identical aside from enabling the check.
- Around line 125-127: The code currently slices delegation_record_data with
&delegation_record_data.as_ref()[8..] which will panic if the account data is
shorter than 8 bytes; before calling DelegationRecord::try_view_from, check
delegation_record_data.len() >= 8 (after
args.delegation_record_account.try_borrow_data()) and return a
ProgramError::InvalidAccountData (or appropriate ProgramError) if it's too
short, then pass the safe slice to DelegationRecord::try_view_from (referencing
delegation_record_account, delegation_record_data, and
DelegationRecord::try_view_from to locate the change).
In `@src/processor/fast/utils/requires.rs`:
- Around line 388-404: The macro require_program_config_fast references
PDA_MARKER unqualified which forces callers to import it; update the macro to
use the crate-qualified symbol $crate::pda::PDA_MARKER inside the
$crate::require_pda_fast! invocation so the expanded code does not rely on the
call site scope (locate the macro require_program_config_fast and replace
PDA_MARKER with $crate::pda::PDA_MARKER); keep the rest of the macro (including
$crate::require_pda_fast!, $program_config, $program, $bump, $is_writable, and
the pubkey_eq check) unchanged.
- Around line 266-295: The fast PDA macro require_initialized_pda_fast currently
derives PDA bytes with solana_sha256_hasher::hashv (symbol: hashv) which skips
Solana's off-curve validation and allows caller-supplied bump values to be
accepted; replace the insecure hashv usage with a verification step using
Solana's create_program_address (and find_program_address for canonical bumps)
to reconstruct the PDA and confirm the provided bump/seed tuple is canonical, or
else call the safe helper macros (require_pda / require_initialized_pda) that
perform this validation; ensure the macro also rejects non-canonical bumps by
returning ProgramError::InvalidSeeds when create_program_address fails or the
resulting pubkey doesn't match $info.key(), and audit all call sites to ensure
bumps are derived only via find_program_address if you keep the fast path.
In `@tests/test_commit_finalize.rs`:
- Line 52: Update the comment that reads "// execute CommitFinalize and validate
CU performmace" to correct the typo—change "perfmace" to "performance" so it
reads "// execute CommitFinalize and validate CU performance"; locate and edit
the comment near the CommitFinalize test in tests/test_commit_finalize.rs (the
line referencing CommitFinalize) to apply the change.
♻️ Duplicate comments (5)
.github/workflows/run-tests.yml (1)
9-9: Verify CLI v3.1.5 aligns with Cargo Solana crate constraints.Line 9 bumps the Solana CLI to v3.1.5. Please confirm Cargo.toml’s solana-* versions are compatible to avoid toolchain/SDK mismatch in CI.
src/state/delegation_metadata.rs (1)
36-50: Length validation is present but could be more explicit about discriminator offset.The current check validates 45 bytes minimum (
8 + 1 + 32 + 4), which is sufficient for the unsafe reads atAccountDiscriminator::SPACEandAccountDiscriminator::SPACE + 8. However, the check doesn't explicitly referenceAccountDiscriminator::SPACE, which could lead to maintenance issues if that constant changes.♻️ Optional: Make the relationship explicit
pub fn from_account(account: &'a AccountInfo) -> Result<Self, ProgramError> { require_ge!( account.data_len(), - 8 // last_update_nonce + AccountDiscriminator::SPACE + + 8 // last_update_nonce + 1 // is_undelegatable + 32 // rent_payer + 4, // seeds (at least 4) ProgramError::InvalidAccountData );src/processor/fast/commit_finalize.rs (1)
17-18: Doc comment incorrectly marksdelegated_accountas non-writable.The doc lists
delegated_accountas[](non-writable), but the implementation resizes and writes new state to it (viaprocess_commit_finalize_internal). The instruction builder insrc/instruction_builder/commit_finalize.rscorrectly marks it as writable withAccountMeta::new(delegated_account, false).📝 Suggested fix
/// 0: `[signer]` the validator requesting the commit -/// 1: `[]` the delegated account +/// 1: `[writable]` the delegated account /// 2: `[]` the delegation recordsrc/processor/fast/internal/commit_finalize_internal.rs (2)
39-109: Remove the deadif false { … } else { … }PDA-validation branch before merge.Keeping both paths leaves unreachable code in the binary and obscures the chosen security/perf tradeoff.
142-149: Remove or track the commented-out transfer block.This is the same dead-code block called out earlier; it should be deleted or replaced with a TODO + issue link.
snawaz
force-pushed
the
snawaz/commit-finalize
branch
from
aebbf1c to
41f26b7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/processor/fast/commit_state.rs (1)
187-196: TODO comments indicate uncertainty about the lamport accounting logic.These TODO comments suggest there may be incomplete understanding of the invariants being enforced. The check at line 191 ensures the delegated account hasn't lost lamports unexpectedly (which would indicate a bug in prior commits). Consider documenting the rationale directly in the code or resolving these questions before merge.
🤖 Fix all issues with AI agents
In `@src/processor/fast/finalize.rs`:
- Around line 133-137: Before calling
delegated_account.resize(commit_state_data.len()), compute the rent-exempt
minimum using let min_balance =
Rent::get()?.minimum_balance(commit_state_data.len()) and validate the delegated
account has at least min_balance lamports; if not, transfer the difference from
the validator/fee payer (use the validator account's try_borrow_mut_lamports or
invoke a SystemProgram transfer) to top up delegated_account, then call
delegated_account.resize and proceed to borrow and copy
(delegated_account.try_borrow_mut_data and copy_from_slice). Ensure you return
errors on Rent::get failures or insufficient funds from the payer.
In `@src/state/delegation_metadata.rs`:
- Around line 37-45: The length check in from_account currently uses require_ge!
on account.data_len() but omits AccountDiscriminator::SPACE, so unsafe reads
starting at the discriminator offset can be OOB; update the require_ge! call in
from_account to add AccountDiscriminator::SPACE (the discriminator byte count)
to the summed minimum (alongside last_update_nonce, is_undelegatable,
rent_payer, seeds) so any short buffer fails with
ProgramError::InvalidAccountData before unsafe reads occur.
♻️ Duplicate comments (15)
.github/workflows/run-tests.yml (1)
9-9: Validate Solana CLI v3.1.5 compatibility with Cargo deps.Line 9 bumps the CLI; if any
solana-*crate constraints still cap<3.0.0or are otherwise mismatched, builds/tests may break. Please confirm the dependency ranges align with v3.x.#!/bin/bash # List solana-related dependency constraints across the repo. fd Cargo.toml -x rg -n 'solana-(program|sdk|program-test)\s*=' {}Cargo.toml (1)
50-50: Consider using a version range forsolana-sha256-hasherto match other Solana deps.Line 50 still pins
"3.1.0"while other Solana deps use ranges. If pinning isn’t intentional for reproducibility, align with the range style to reduce conflicts.🔧 Suggested tweak
-solana-sha256-hasher = "3.1.0" +solana-sha256-hasher = ">=3.1"src/discriminator.rs (1)
48-49: Fix doc link to the fast processor path.Line 48 should reference
crate::processor::fast::process_commit_finalize.✏️ Suggested fix
- /// See [crate::processor::process_commit_finalize] for docs. + /// See [crate::processor::fast::process_commit_finalize] for docs.src/processor/fast/utils/requires.rs (2)
266-295: Fast PDA checks still skip off-curve validation—confirm canonical bumps.Lines 269/343 use
hashvinstead ofcreate_program_address, so off-curve validation is bypassed. Please confirm all call sites only pass canonical bumps fromfind_program_address, or switch to the safe PDA helpers.Also applies to: 340-367
388-399: Macro hygiene: qualifyPDA_MARKERto avoid call-site imports.Line 398 references
PDA_MARKERunqualified, which requires call sites to import it.♻️ Suggested fix
- PDA_MARKER + $crate::pda::PDA_MARKERsrc/entrypoint.rs (1)
47-49: Usepinocchio_log::log!for slow‑path errors too.Line 48 still uses
solana_program::msg!; for consistency with the fast path and maintainer preference, switch topinocchio_log::log!.♻️ Proposed tweak
- solana_program::msg!("slow_process_instruction: {}", error); + pinocchio_log::log!("slow_process_instruction: {}", error);Based on learnings, prefer
log!in entrypoint error paths.tests/test_commit_finalize.rs (1)
52-52: Minor typo in comment.📝 Suggested fix
- // execute CommitFinalize and validate CU performmace + // execute CommitFinalize and validate CU performancesrc/processor/fast/internal/commit_finalize_internal.rs (4)
40-109: Remove dead code branches before merging.The
if false { } else { }pattern contains dead code that should be removed once the preferred PDA validation approach is finalized. Per the PR objectives, you've chosenrequire_initialized_pda_fast- remove the unusedrequire_initialized_pdabranch.
125-127: Guard against panic on short delegation-record data.The slice
[8..]will panic if the account data is shorter than 8 bytes. Add a bounds check to return a cleanProgramErrorinstead.🛡️ Suggested fix
let delegation_record_data = args.delegation_record_account.try_borrow_data()?; - let delegation_record = DelegationRecord::try_view_from(&delegation_record_data.as_ref()[8..])?; + let data = delegation_record_data.as_ref(); + let record_bytes = data.get(8..).ok_or(ProgramError::InvalidAccountData)?; + let delegation_record = DelegationRecord::try_view_from(record_bytes)?;
142-149: Remove or document commented-out lamport transfer code.This commented code references
args.commit_record_lamportswhich doesn't exist in the current struct. Either remove it or add a TODO with context explaining when this will be implemented.
151-184: Critical: Program-config whitelist validation is disabled.The entire
ProgramConfigwhitelist check is behindif false, meaning all validators can finalize commits regardless of whitelist configuration. If whitelisting is part of the security model, this allows unauthorized validators to finalize delegated account state.Enable this check before merge or explicitly document why it's intentionally disabled.
✅ Enable ProgramConfig validation
- // cost = 197 CU - if false { - // Load the program configuration and validate it, if any - let has_program_config = if false { - ... - } else { - require_program_config_fast!( - args.program_config_account, - delegation_record.owner.as_array(), - args.bumps.program_config, - false - ) - }; - if has_program_config { - ... - } - } + // Load the program configuration and validate it, if any + let has_program_config = require_program_config_fast!( + args.program_config_account, + delegation_record.owner.as_array(), + args.bumps.program_config, + false + ); + if has_program_config { + let program_config_data = args.program_config_account.try_borrow_data()?; + + let program_config = + ProgramConfig::try_from_bytes_with_discriminator(&program_config_data) + .map_err(to_pinocchio_program_error)?; + if !program_config + .approved_validators + .contains(&(*args.validator.key()).into()) + { + log!("validator is not whitelisted in the program config: "); + pubkey::log(args.validator.key()); + return Err(DlpError::InvalidWhitelistProgramConfig.into()); + } + }src/processor/fast/commit_finalize.rs (1)
17-18: Doc comment shows delegated_account as read-only, but it's written to.The
delegated_accountis marked as[](read-only) in the doc comment, butprocess_commit_finalize_internalwrites the new state directly to it viaresizeandcopy_from_slice. Update the doc to reflect the correct mutability.📝 Suggested fix
/// 0: `[signer]` the validator requesting the commit -/// 1: `[]` the delegated account +/// 1: `[writable]` the delegated account (new state applied here) /// 2: `[]` the delegation recordsrc/instruction_builder/commit_finalize.rs (1)
87-103: Confirm or set explicit max size for ProgramConfig in loaded-accounts budget.The docstring is now correctly referencing
commit_finalize. However, line 100 budgetsAccountSizeClass::Tiny(256 bytes) forprogram_config_pda. As noted in previous review,ProgramConfigstores aBTreeSet<Pubkey>where size =8 + 4 + 32 * approved_validators.len(). With 8+ validators, this exceeds 256 bytes.Either:
- Enforce a documented max validator count constraint, or
- Use a larger size class (e.g.,
AccountSizeClass::Small)#!/bin/bash # Check ProgramConfig struct definition and any size constraints rg -n "struct ProgramConfig" --type=rust -A 20src/pod_view.rs (2)
50-53: Correct the instruction data alignment claim in the safety documentation.Lines 50-53 assert that both account data and instruction data are 8-byte aligned. Per Solana documentation, only account data is guaranteed 8-byte aligned. For instruction data, only the buffer start is 8-byte aligned; the contents have no internal alignment guarantee unless you encode them that way.
This contradicts line 8's comment which correctly scopes the guarantee to account data only.
📝 Suggested doc fix
-/// Our codebase assumes that both `account data` and `instruction data` are aligned to -/// 8-byte boundary and therefore all structs/types have either 8-byte alignment requirement -/// or weaker one. The `bytemuck` crates enforces the alignment requirements, so we do not -/// have to do it ourselves. +/// Our codebase assumes that `account data` is aligned to 8-byte boundary (guaranteed by +/// Solana runtime). For `instruction data`, only the buffer start is 8-byte aligned; the +/// contents have no internal alignment guarantee unless explicitly encoded. All structs/types +/// have either 8-byte alignment requirement or weaker. The `bytemuck` crate enforces alignment +/// requirements at runtime, and `try_from_unaligned` is provided for test scenarios.
91-96: Doc comment mismatches the actual error returned.The doc states the function returns
ErrorCode::SizeMismatch, but the implementation at line 124 returnsProgramError::InvalidArgument.📝 Doc fix
/// Copy the raw bytes of Self into the given mutable buffer. /// /// This is a *copy*, not a cast. The buffer must be exactly Self::SPACE /// bytes long. On success, the buffer will contain a byte-for-byte - /// representation of this struct, else it will return ErrorCode::SizeMismatch. + /// representation of this struct, else it will return `ProgramError::InvalidArgument`. fn try_copy_to(&self, buffer: &mut [u8]) -> Result<(), ProgramError>;
snawaz
commented
Jan 27, 2026
Contributor
WalkthroughAdds a new commit_finalize instruction and fast-path implementation, supporting POD views, args-with-buffer types, PDA derivation/helpers, in-place commit finalize logic (full bytes or diffs), validation macros, tests, and updates CI Solana CLI and Cargo dependencies. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Line 57: The dependency "assertables" is pinned to an exact version
("assertables = \"9.8.2\""); change this in Cargo.toml to a caret constraint to
allow patch updates (e.g., assertables = "^9.8.2") so future bugfix releases
like 9.8.3/9.8.4 are permitted; locate the assertables entry in Cargo.toml and
update the version string accordingly.
In `@src/instruction_builder/commit_finalize.rs`:
- Around line 22-49: Document that commit_finalize mutates its args by setting
args.bumps to the derived PDA bumps: add a doc comment on the commit_finalize
function (above the signature) stating that the function derives PDAs
(delegation_record, delegation_metadata, validator_fees_vault) and writes their
bump seeds into args.bumps (CommitBumps), so callers should expect args to be
modified. Reference the symbols commit_finalize, args.bumps, and CommitBumps in
the comment so the side-effect is explicit.
This was referenced Feb 6, 2026
snawaz
force-pushed
the
snawaz/commit-finalize
branch
from
d9e5d4c to
ed82197
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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/pod_view.rs`:
- Around line 70-72: Fix the minor grammar issue in the doc comment above the
Pod-related declaration (the comment starting "raw bytes, cannot be Pod, and
must not appear in account or instruction layouts. Use u8 instead. Note that
`#[repr(u8)] enum` is disgusting type as well, so we cannot that either."); add
the missing word "use" so the sentence reads "so we cannot use that either" to
correct the wording near the Pod/u8/#[repr(u8)] explanation.
In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 74-84: The code currently calls metadata.replace_last_update_nonce
and metadata.replace_is_undelegatable (after
DelegationMetadataFast::from_account) before doing require_eq! and require!
checks, which mutates account state relying on Solana's revert-on-failure
semantics; add a brief inline comment just above those calls explaining that
these methods mutate account data but are safe here because failed
require!/require_eq! will cause a transaction revert and thus no partial state
persists, and also (optionally) prefer validating on temporary reads first by
using the non-mutating accessors (e.g., read current nonce and undelegatable
flag via getters on DelegationMetadataFast) and only call
replace_last_update_nonce/replace_is_undelegatable after validation if you want
to avoid relying on revert semantics.
In `@src/processor/fast/utils/requires.rs`:
- Around line 266-300: The truncated inline comment inside the
require_initialized_pda_fast macro should be fixed: either remove the dead
comment lines or replace them with a concise, complete explanation why using
solana_sha256_hasher::hashv over the provided $seeds is acceptable for
already-initialized PDAs (e.g., note that this macro validates the derived PDA
via pubkey_eq($info.key(), &pda) and therefore only needs a deterministic hash,
not a full SDK derive_with_seed call). Update the comment near
require_initialized_pda_fast so it clearly references
solana_sha256_hasher::hashv, pubkey_eq, and the validation intent, or drop the
comment entirely.
- Around line 191-211: The log string in the require_owned_by! macro incorrectly
references "require_owned_pda!" causing misleading diagnostics; update the log
invocation inside require_owned_by! to print the correct macro name
("require_owned_by!") so failures show the right context, leaving the rest of
the macro (pubkey_eq check, pubkey::log calls, and returning
ProgramError::InvalidAccountOwner) untouched.
- Around line 393-409: The exported macros require_program_config_fast! and
require_program_config! reference pda::PROGRAM_CONFIG_TAG unqualified, which
breaks macro hygiene; update both macros to use $crate::pda::PROGRAM_CONFIG_TAG
(and similarly qualify any other pda symbols) so callers do not need to have
crate::pda in scope, keeping the rest of the macro logic (including calls to
$crate::require_pda_fast! and pubkey_eq checks) unchanged.
- Around line 345-372: require_pda_fast! currently only checks the hash and
writability; add explicit ownership/off-curve validation to match the safe
variant—either invoke require_owned_by! on $info with the expected program owner
(as require_initialized_pda_fast! does) after the hash check, or replace the
hashv() approach with create_program_address() (and handle its Result) to
perform off-curve validation like require_pda! before returning Ok/Err; ensure
the change is applied inside the require_pda_fast! macro and update any call
sites such as require_program_config_fast! that rely on this macro.
In `@src/state/delegation_metadata.rs`:
- Around line 75-91: set_is_undelegatable and replace_is_undelegatable currently
write a Rust bool directly via raw pointer; add a short comment above these
functions stating the invariant that the on-chain storage byte must always be 0
or 1 (so ptr::write of a Rust bool is sound) and document that any future read
path (e.g., a getter/deserialize) must validate bytes (or read as u8 and map
0->false, 1->true and reject other values) or consider switching storage to an
explicit u8 to avoid UB; reference these functions by name in the comment so
reviewers can find the guarantee.
- Around line 38-46: The length check uses a hardcoded discriminator size (the
`8` inside the `require_ge!` call) — replace that literal with
`AccountDiscriminator::SPACE` so the guard matches other uses (accessor methods
and `serialized_size()`) and prevents future mismatches; update the
`require_ge!` expression that adds up the fields to use
`AccountDiscriminator::SPACE` instead of `8`, leaving the rest of the sum
unchanged.
- Around line 53-73: The unsafe reads/writes in last_update_nonce,
set_last_update_nonce, and replace_last_update_nonce rely on a specific field
layout of the DelegationMetadata struct (offsets based on
AccountDiscriminator::SPACE); add a compile-time assertion that the offset of
the struct field backing last_update_nonce equals AccountDiscriminator::SPACE
(and any subsequent field offsets used elsewhere) to prevent silent breakage if
fields are reordered — e.g., use memoffset::offset_of! or static_assertions to
assert offset_of!(DelegationMetadata, <field_name>) ==
AccountDiscriminator::SPACE (and assert sizes/alignments as needed), and add a
short comment in the impl noting the layout assumption.
In `@tests/test_commit_finalize.rs`:
- Around line 22-82: Add a new test (e.g., test_commit_finalize_diff) in
tests/test_commit_finalize.rs that exercises the commit_finalize path with
CommitFinalizeArgs.data_is_diff set to true: (1) construct and apply a valid
non-empty diff payload and assert the target account data changes as expected
and DelegationMetadata is updated (reference commit_finalize instruction_builder
and CommitFinalizeArgs), (2) test an empty-diff payload to trigger the
DiffSet::try_new empty-diff warning and assert the log message count/contents
and no data change, and (3) test a misaligned diff buffer that should produce
InvalidDiffAlignment from DiffSet::try_new/apply_diff_in_place and assert the
transaction fails with the expected error; reuse the existing test harness
setup_program_test_env, banks.process_transaction_with_metadata, and checks on
metadata.log_messages and account data to locate and validate behavior.
- Around line 52-55: The test currently discards the transaction outcome by
using `result: _`, causing misleading failures; change the destructuring from
`BanksTransactionResultWithMetadata { result: _, metadata }` to capture `result`
(e.g., `BanksTransactionResultWithMetadata { result, metadata }`) and then
assert the transaction succeeded before checking account state—either
`result.unwrap()` or `assert!(result.is_ok(), "transaction failed: {:?}",
result.err())` after calling
`process_transaction_with_metadata(tx).await.unwrap()` so that program errors
surface instead of a data mismatch on `assert_eq!(delegated_account.data,
new_state)`.
---
Duplicate comments:
In @.github/workflows/run-tests.yml:
- Line 9: The CI is using solana_version: v3.1.5 which ships v3.x crates but
Cargo.toml pins solana-program = ">=1.16, <3.0.0", causing a version mismatch;
fix by aligning them: either update the workflow key solana_version (change
solana_version to a v1/v2 tag that matches the Cargo.toml constraint) or relax
the Cargo.toml solana-program constraint to allow v3 (for example widen the
upper bound to <4.0.0 or add explicit support for v3.x) so both the
.github/workflows run-tests.yml solana_version entry and the Cargo.toml
solana-program dependency are compatible.
In `@Cargo.toml`:
- Line 57: The Cargo.toml dependency for assertables is strictly pinned to
"9.8.2"; change it to a caret range so patch releases are accepted (e.g., use
"^9.8.2" for the assertables entry) and then run cargo update or regenerate the
lockfile so the lockfile reflects the new range; edit the assertables line in
Cargo.toml (the dependency entry named assertables) accordingly.
In `@src/discriminator.rs`:
- Around line 51-52: Update the doc comment for the CommitFinalize enum variant
to point to the correct function path: replace the incorrect reference to
crate::processor::process_commit_finalize with
crate::processor::fast::process_commit_finalize so the doc link resolves to the
actual function (target the CommitFinalize enum variant's doc comment).
In `@src/entrypoint.rs`:
- Around line 47-49: Replace the call to solana_program::msg! in the slow-path
error handling with the pinocchio_log::log! macro for consistency: locate the
Err(error) branch (the slow_process_instruction error path) and swap
solana_program::msg!("slow_process_instruction: {}", error) to use
pinocchio_log::log! at the appropriate level (e.g., Error) and include the same
contextual message plus the error; ensure the pinocchio_log import/path is
available in this module and keep the existing error.into() return unchanged.
In `@src/error.rs`:
- Around line 160-168: The ToStr impl currently returns variant names via
self.into(), not the human-readable #[error(...)] messages; update the
pinocchio::program_error::ToStr for DlpError implementation to return the
human-friendly static messages by matching on DlpError variants (e.g., match
self { DlpError::InvalidAuthority => "Invalid Authority", ... }) and returning
those &'static str literals instead of calling self.into(); reference the
DlpError enum name and the impl block for pinocchio::program_error::ToStr when
making this change.
In `@src/instruction_builder/commit_finalize.rs`:
- Around line 20-27: The commit_finalize function mutates args.bumps by writing
derived PDA bump seeds (so callers' CommitFinalizeArgs will be modified); update
the function documentation above pub fn commit_finalize(...) to explicitly state
this side effect: mention that the function will populate/overwrite fields in
args.bumps with computed bump seeds (reference the args.bumps field) and that
callers should expect the args struct to be mutated or clone it beforehand if
they need an unmodified copy; keep the note brief and include which PDA-derived
bumps are set (the ones derived inside commit_finalize) so callers know what to
expect.
In `@src/pod_view.rs`:
- Around line 91-96: The doc for try_copy_to incorrectly mentions
ErrorCode::SizeMismatch while the implementation returns
ProgramError::InvalidArgument; update the docstring for fn try_copy_to
(referencing Self::SPACE) to state that the function returns
ProgramError::InvalidArgument on buffer-size mismatch (or align the
implementation to return ErrorCode::SizeMismatch if you prefer that variant),
ensuring the documentation and the implementation of try_copy_to are consistent.
- Around line 50-53: The comment is misleading about instruction data alignment:
verify that the ArgsWithBuffer payload is actually 8-byte aligned after the
discriminator by checking the discriminator size and ensuring
ArgsWithBuffer::from_bytes reads from a properly aligned offset; specifically
locate DISCRIMINATOR_LEN / DlpDiscriminator and the ArgsWithBuffer::from_bytes
implementation and either (a) enforce/assert that DISCRIMINATOR_LEN % 8 == 0 (or
that (DISCRIMINATOR_LEN % 8) plus any header equals 0 mod 8), or (b) add
explicit padding handling in ArgsWithBuffer::from_bytes to advance to the next
8-byte boundary before casting/using bytemuck conversions, and add a unit test
verifying parsing works when discriminator length is not 8-aligned.
In `@src/processor/fast/commit_finalize.rs`:
- Around line 17-22: The doc comment for the commit_finalize accounts list
incorrectly marks delegated_account (account 1) as non-writable (`[]`) even
though the code treats it as writable (AccountMeta::new(delegated_account,
false) and calls delegated_account.resize(...) and
delegated_account.try_borrow_mut_data()); update the annotation for account 1
from `[]` to `[writable]` in the accounts comment so it matches the actual usage
of delegated_account in the commit_finalize logic.
In `@src/processor/fast/finalize.rs`:
- Around line 134-137: Before calling
delegated_account.resize(commit_state_data.len()) ensure the account will remain
rent-exempt: compute required_lamports =
Rent::get()?.minimum_balance(commit_state_data.len()) and compare with
delegated_account.lamports(); if short, either return a clear error or request a
top-up (transfer lamports into delegated_account) before resize; update the
logic around delegated_account.resize and
delegated_account.try_borrow_mut_data() to perform this check and only proceed
when the balance meets required_lamports.
In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 87-88: The code slices delegation_record_data with
&delegation_record_data.as_ref()[8..] which will panic for data shorter than 8
bytes; change this to safely access the range with
delegation_record_data.as_ref().get(8..) and return a descriptive error if None
before calling DelegationRecord::try_view_from; update the code path around
args.delegation_record_account.try_borrow_data() and the
DelegationRecord::try_view_from call to use the safe slice and propagate a
suitable error instead of allowing a panic.
- Around line 104-111: The commented transfer block references a non-existent
field args.commit_record_lamports in CommitFinalizeInternalArgs and should not
remain as dead code; either delete the entire commented block or replace it with
a single-line TODO comment including a tracking issue/PR reference (eg. TODO:
revisit transfer logic - TRACKER-<id>) so future work can restore correct
behavior; locate the block in commit_finalize_internal.rs near
CommitFinalizeInternalArgs and delegation_record.lamports and apply the removal
or TODO insertion accordingly.
In `@src/processor/fast/utils/requires.rs`:
- Around line 42-51: The require_signer! macro references unqualified symbols
(log!, pubkey::log, ProgramError::MissingRequiredSignature); update the macro
body to use fully-qualified paths so it doesn't rely on call-site imports —
e.g., replace log! with $crate::log!, pubkey::log with $crate::pubkey::log, and
ProgramError::MissingRequiredSignature with the fully-qualified ProgramError
path (or $crate::ProgramError::MissingRequiredSignature if ProgramError is
re-exported from this crate); keep the rest of the macro logic unchanged.
In `@tests/test_commit_finalize.rs`:
- Line 50: Fix the typo in the inline comment that reads "execute CommitFinalize
and validate CU performmace" by changing "performmace" to "performance" so the
comment reads "execute CommitFinalize and validate CU performance"; locate and
update the comment string in the test (the comment containing "execute
CommitFinalize and validate CU") within the tests/test_commit_finalize.rs test
file.
Comment on lines
+70
to
+72
| /// raw bytes, cannot be Pod, and must not appear in account or instruction | ||
| /// layouts. Use u8 instead. Note that `#[repr(u8)] enum` is disgusting | ||
| /// type as well, so we cannot that either. |
Contributor
There was a problem hiding this comment.
Minor grammar: missing word.
Line 72: "so we cannot that either" → "so we cannot use that either".
📝 Fix
-/// type as well, so we cannot that either.
+/// type as well, so we cannot use that either.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pod_view.rs` around lines 70 - 72, Fix the minor grammar issue in the doc
comment above the Pod-related declaration (the comment starting "raw bytes,
cannot be Pod, and must not appear in account or instruction layouts. Use u8
instead. Note that `#[repr(u8)] enum` is disgusting type as well, so we cannot
that either."); add the missing word "use" so the sentence reads "so we cannot
use that either" to correct the wording near the Pod/u8/#[repr(u8)] explanation.
Comment on lines
+42
to
+60
| let commit_args = CommitFinalizeInternalArgs { | ||
| bumps: &args.bumps, | ||
| new_state: if args.data_is_diff.is_true() { | ||
| let diffset = DiffSet::try_new(args.buffer)?; | ||
| if diffset.segments_count() == 0 { | ||
| log!("WARN: noop; empty diff sent"); | ||
| } | ||
| NewState::Diff(diffset) | ||
| } else { | ||
| NewState::FullBytes(args.buffer) | ||
| }, | ||
| commit_id: args.commit_id, | ||
| allow_undelegation: args.allow_undelegation.is_true(), | ||
| validator, | ||
| delegated_account, | ||
| delegation_record_account, | ||
| delegation_metadata_account, | ||
| validator_fees_vault, | ||
| }; |
Contributor
There was a problem hiding this comment.
CommitFinalizeArgs.lamports is silently discarded — lamport settlement is incomplete.
args.lamports is accessible here via Deref but is never forwarded to CommitFinalizeInternalArgs. The commented-out transfer block in commit_finalize_internal.rs confirms this was planned but not implemented:
// if args.commit_record_lamports > delegation_record.lamports {
// system::Transfer { from: args.validator, to: ..., lamports: ... }.invoke()?;
// }Unlike the CommitState + Finalize path (where settle_lamports_balance adjusts the delegated account's lamports and delegation_record.lamports is updated to the new balance), CommitFinalize performs neither adjustment. Consequences:
- The
lamportsfield occupies wire space in every instruction but is never consumed on-chain. - The
delegation_record.lamportsis never updated after aCommitFinalize, leaving it permanently stale for all subsequent operations that compare against it (e.g., therequire_ge!guard in the internal function itself). - Any SOL top-up intent expressed by the validator is silently ignored.
Either implement the lamport settlement for this path or, if intentionally deferred, document this clearly and consider removing the lamports field from the wire format (or marking it _reserved_lamports) to prevent confusion.
Comment on lines
+74
to
+84
| { | ||
| let mut metadata = DelegationMetadataFast::from_account(args.delegation_metadata_account)?; | ||
|
|
||
| let prev_id = metadata.replace_last_update_nonce(args.commit_id); | ||
|
|
||
| require_eq!(args.commit_id, prev_id + 1, DlpError::NonceOutOfOrder); | ||
|
|
||
| require!( | ||
| !metadata.replace_is_undelegatable(args.allow_undelegation), | ||
| DlpError::AlreadyUndelegated | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mutation before validation: side effects are safe only because Solana reverts on error.
replace_last_update_nonce (line 77) and replace_is_undelegatable (line 82) mutate account data before the subsequent require_eq!/require! checks. This works because Solana reverts all account changes on transaction failure. Worth a brief inline comment so future readers don't reorder these into a context where partial mutation could persist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/processor/fast/internal/commit_finalize_internal.rs` around lines 74 -
84, The code currently calls metadata.replace_last_update_nonce and
metadata.replace_is_undelegatable (after DelegationMetadataFast::from_account)
before doing require_eq! and require! checks, which mutates account state
relying on Solana's revert-on-failure semantics; add a brief inline comment just
above those calls explaining that these methods mutate account data but are safe
here because failed require!/require_eq! will cause a transaction revert and
thus no partial state persists, and also (optionally) prefer validating on
temporary reads first by using the non-mutating accessors (e.g., read current
nonce and undelegatable flag via getters on DelegationMetadataFast) and only
call replace_last_update_nonce/replace_is_undelegatable after validation if you
want to avoid relying on revert semantics.
Comment on lines
+191
to
+211
| /// | ||
| /// require_owned_by( | ||
| /// info: &AccountInfo, | ||
| /// owner: &Pubkey | ||
| /// ) -> Result<(), ProgramError> | ||
| /// | ||
| #[macro_export] | ||
| macro_rules! require_owned_by { | ||
| ($info: expr, $owner: expr) => {{ | ||
| if !pubkey_eq($info.owner(), $owner) { | ||
| pinocchio_log::log!( | ||
| "require_owned_pda!({}, {})", | ||
| stringify!($info), | ||
| stringify!($owner) | ||
| ); | ||
| pubkey::log($info.key()); | ||
| pubkey::log($owner); | ||
| return Err(ProgramError::InvalidAccountOwner); | ||
| } | ||
| }}; | ||
| } |
Contributor
There was a problem hiding this comment.
Log message references wrong macro name.
Line 202 logs "require_owned_pda!" but the macro is require_owned_by!. This will produce misleading diagnostic output when the check fails.
📝 Proposed fix
($info: expr, $owner: expr) => {{
if !pubkey_eq($info.owner(), $owner) {
pinocchio_log::log!(
- "require_owned_pda!({}, {})",
+ "require_owned_by!({}, {})",
stringify!($info),
stringify!($owner)
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/processor/fast/utils/requires.rs` around lines 191 - 211, The log string
in the require_owned_by! macro incorrectly references "require_owned_pda!"
causing misleading diagnostics; update the log invocation inside
require_owned_by! to print the correct macro name ("require_owned_by!") so
failures show the right context, leaving the rest of the macro (pubkey_eq check,
pubkey::log calls, and returning ProgramError::InvalidAccountOwner) untouched.
Comment on lines
+266
to
+300
| #[macro_export] | ||
| macro_rules! require_initialized_pda_fast { | ||
| ($info:expr, $seeds: expr, $is_writable: expr) => {{ | ||
| // | ||
| // Ideally, we want to der | ||
| // | ||
| // | ||
| // | ||
| let pda = solana_sha256_hasher::hashv($seeds).to_bytes(); | ||
|
|
||
| if !pubkey_eq($info.key(), &pda) { | ||
| log!( | ||
| "require_initialized_pda!({}, {}, {}); pubkey_eq failed", | ||
| stringify!($info), | ||
| stringify!($seeds), | ||
| stringify!($is_writable) | ||
| ); | ||
| pubkey::log($info.key()); | ||
| return Err(ProgramError::InvalidSeeds); | ||
| } | ||
|
|
||
| require_owned_by!($info, &$crate::fast::ID); | ||
|
|
||
| if $is_writable && !$info.is_writable() { | ||
| log!( | ||
| "require_initialized_pda!({}, {}, {}); is_writable expectation failed", | ||
| stringify!($info), | ||
| stringify!($seeds), | ||
| stringify!($is_writable) | ||
| ); | ||
| pubkey::log($info.key()); | ||
| return Err(ProgramError::Immutable); | ||
| } | ||
| }}; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Truncated comment left in macro body.
Lines 269–273 contain an incomplete comment: "Ideally, we want to der" — this reads like a mid-sentence truncation. Either complete the explanation of the design rationale (why hashv is acceptable for already-initialized accounts) or remove the dead comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/processor/fast/utils/requires.rs` around lines 266 - 300, The truncated
inline comment inside the require_initialized_pda_fast macro should be fixed:
either remove the dead comment lines or replace them with a concise, complete
explanation why using solana_sha256_hasher::hashv over the provided $seeds is
acceptable for already-initialized PDAs (e.g., note that this macro validates
the derived PDA via pubkey_eq($info.key(), &pda) and therefore only needs a
deterministic hash, not a full SDK derive_with_seed call). Update the comment
near require_initialized_pda_fast so it clearly references
solana_sha256_hasher::hashv, pubkey_eq, and the validation intent, or drop the
comment entirely.
Comment on lines
+38
to
+46
| require_ge!( | ||
| account.data_len(), | ||
| 8 // discriminator | ||
| + 8 // last_update_nonce | ||
| + 1 // is_undelegatable | ||
| + 32 // rent_payer | ||
| + 4, // seeds (at least 4) | ||
| ProgramError::InvalidAccountData | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find AccountDiscriminator definition
rg -n "AccountDiscriminator" --type rust src/state/delegation_metadata.rs -B 2 -A 2Repository: magicblock-labs/delegation-program
Length of output: 1573
🏁 Script executed:
# Search for SPACE constant in discriminator-related code
rg -n "SPACE" --type rust | head -20Repository: magicblock-labs/delegation-program
Length of output: 1411
🏁 Script executed:
# Find the discriminator module/trait definition
fd "." --type f -name "*.rs" | xargs rg -l "AccountDiscriminator" | head -5Repository: magicblock-labs/delegation-program
Length of output: 477
🏁 Script executed:
# Get the full context of the from_account method and accessors
cat -n src/state/delegation_metadata.rs | sed -n '32,90p'Repository: magicblock-labs/delegation-program
Length of output: 2170
Replace the hardcoded 8 discriminator with AccountDiscriminator::SPACE.
The length check hardcodes 8 for the discriminator size, while accessor methods (lines 54, 60, 69, 78, 87) and serialized_size() (line 102) all use AccountDiscriminator::SPACE. Although both are currently 8, using the constant ensures the length guard remains correct if the constant changes, preventing potential out-of-bounds reads.
Proposed fix
require_ge!(
account.data_len(),
- 8 // discriminator
+ AccountDiscriminator::SPACE
+ 8 // last_update_nonce
+ 1 // is_undelegatable
+ 32 // rent_payer
+ 4, // seeds (at least 4)
ProgramError::InvalidAccountData
);📝 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
| require_ge!( | |
| account.data_len(), | |
| 8 // discriminator | |
| + 8 // last_update_nonce | |
| + 1 // is_undelegatable | |
| + 32 // rent_payer | |
| + 4, // seeds (at least 4) | |
| ProgramError::InvalidAccountData | |
| ); | |
| require_ge!( | |
| account.data_len(), | |
| AccountDiscriminator::SPACE | |
| 8 // last_update_nonce | |
| 1 // is_undelegatable | |
| 32 // rent_payer | |
| 4, // seeds (at least 4) | |
| ProgramError::InvalidAccountData | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state/delegation_metadata.rs` around lines 38 - 46, The length check uses
a hardcoded discriminator size (the `8` inside the `require_ge!` call) — replace
that literal with `AccountDiscriminator::SPACE` so the guard matches other uses
(accessor methods and `serialized_size()`) and prevents future mismatches;
update the `require_ge!` expression that adds up the fields to use
`AccountDiscriminator::SPACE` instead of `8`, leaving the rest of the sum
unchanged.
Comment on lines
+53
to
+73
| pub fn last_update_nonce(&self) -> u64 { | ||
| unsafe { ptr::read(self.data.as_ptr().add(AccountDiscriminator::SPACE) as *const u64) } | ||
| } | ||
|
|
||
| pub fn set_last_update_nonce(&mut self, val: u64) { | ||
| unsafe { | ||
| ptr::write( | ||
| self.data.as_mut_ptr().add(AccountDiscriminator::SPACE) as *mut u64, | ||
| val, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| pub fn replace_last_update_nonce(&mut self, val: u64) -> u64 { | ||
| unsafe { | ||
| ptr::replace( | ||
| self.data.as_mut_ptr().add(AccountDiscriminator::SPACE) as *mut u64, | ||
| val, | ||
| ) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find DelegationMetadata struct and AccountDiscriminator
echo "=== DelegationMetadata struct definition ==="
ast-grep --pattern $'struct DelegationMetadata {
$$$
}'
echo ""
echo "=== AccountDiscriminator and SPACE constant ==="
rg -n "AccountDiscriminator" --type rust -A 3 -B 1 | head -50
echo ""
echo "=== File structure in state directory ==="
fd . src/state --type fRepository: magicblock-labs/delegation-program
Length of output: 3254
🏁 Script executed:
#!/bin/bash
# Search for last_update_nonce and is_undelegatable fields
echo "=== Fields in DelegationMetadata (complete struct) ==="
rg -n "pub struct DelegationMetadata" --type rust -A 20
echo ""
echo "=== Check for existing compile-time assertions ==="
rg -n "const_assert\|#\[test\].*order" --type rust
echo ""
echo "=== Look for Borsh serialization patterns ==="
rg -n "Deserialize\|Serialize" --type rust src/state/ -B 2 -A 2Repository: magicblock-labs/delegation-program
Length of output: 2456
🏁 Script executed:
#!/bin/bash
# Find AccountDiscriminator definition
echo "=== AccountDiscriminator enum and SPACE constant ==="
cat src/state/utils/discriminator.rs
echo ""
echo "=== DelegationMetadata derives ==="
rg -n "DelegationMetadata" src/state/delegation_metadata.rs -B 3 | head -15
echo ""
echo "=== Check how data is provided to DelegationMetadataFast ==="
rg -n "DelegationMetadataFast::from_account" --type rust -B 2 -A 5Repository: magicblock-labs/delegation-program
Length of output: 1983
🏁 Script executed:
#!/bin/bash
# Check for tests or assertions related to field offsets
echo "=== Search for offset-related tests or size checks ==="
rg -n "offset\|size_of\|align_of" src/state/ --type rust
echo ""
echo "=== Check the complete is_undelegatable usage ==="
rg -n "is_undelegatable" src/state/delegation_metadata.rs -A 3 -B 1
echo ""
echo "=== Verify Borsh behavior with field ordering ==="
rg -n "pub fn from_account\|pub fn last_update_nonce\|pub fn is_undelegatable" src/state/delegation_metadata.rs -A 10 | head -60Repository: magicblock-labs/delegation-program
Length of output: 1530
🏁 Script executed:
#!/bin/bash
# Get the complete DelegationMetadataFast impl block
echo "=== Complete DelegationMetadataFast impl ==="
sed -n '36,110p' src/state/delegation_metadata.rs
echo ""
echo "=== Check for get_is_undelegatable ==="
rg -n "get_is_undelegatable" src/state/ --type rustRepository: magicblock-labs/delegation-program
Length of output: 2328
🏁 Script executed:
#!/bin/bash
# Check if is_undelegatable is read anywhere in the codebase
echo "=== All usages of is_undelegatable ==="
rg -n "is_undelegatable" --type rust -B 2 -A 2Repository: magicblock-labs/delegation-program
Length of output: 9177
Unsafe pointer writes assume Borsh-compatible memory layout — add safeguards against field reordering.
The last_update_nonce accessor reads at AccountDiscriminator::SPACE (offset 8) and both the setter and replace methods for is_undelegatable write at AccountDiscriminator::SPACE + 8 (offset 16), which correctly reflect the field order in the Borsh-serialized DelegationMetadata struct. However, no compile-time assertions anchor these offsets to the struct definition. Reordering last_update_nonce or is_undelegatable in DelegationMetadata would silently break these unsafe write operations—while the Borsh deserialization would also fail, explicit safeguards would catch the issue earlier and make the dependency explicit.
Consider adding a compile-time assertion (e.g., via static_assertions::assert_eq_align! or similar) or documenting the layout assumption directly in the impl block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state/delegation_metadata.rs` around lines 53 - 73, The unsafe
reads/writes in last_update_nonce, set_last_update_nonce, and
replace_last_update_nonce rely on a specific field layout of the
DelegationMetadata struct (offsets based on AccountDiscriminator::SPACE); add a
compile-time assertion that the offset of the struct field backing
last_update_nonce equals AccountDiscriminator::SPACE (and any subsequent field
offsets used elsewhere) to prevent silent breakage if fields are reordered —
e.g., use memoffset::offset_of! or static_assertions to assert
offset_of!(DelegationMetadata, <field_name>) == AccountDiscriminator::SPACE (and
assert sizes/alignments as needed), and add a short comment in the impl noting
the layout assumption.
Comment on lines
+75
to
+91
| pub fn set_is_undelegatable(&mut self, val: bool) { | ||
| unsafe { | ||
| ptr::write( | ||
| self.data.as_mut_ptr().add(AccountDiscriminator::SPACE + 8) as *mut bool, | ||
| val, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| pub fn replace_is_undelegatable(&mut self, val: bool) -> bool { | ||
| unsafe { | ||
| ptr::replace( | ||
| self.data.as_mut_ptr().add(AccountDiscriminator::SPACE + 8) as *mut bool, | ||
| val, | ||
| ) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
set_is_undelegatable / replace_is_undelegatable write a Rust bool via raw pointer — any non-0/1 byte would be UB.
Writing bool via ptr::write is safe only if val is a genuine Rust bool. Since the parameter type enforces this, the current code is sound. However, the read path (if added later) would need the same guarantee from the on-chain data. Worth a brief comment noting this invariant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state/delegation_metadata.rs` around lines 75 - 91, set_is_undelegatable
and replace_is_undelegatable currently write a Rust bool directly via raw
pointer; add a short comment above these functions stating the invariant that
the on-chain storage byte must always be 0 or 1 (so ptr::write of a Rust bool is
sound) and document that any future read path (e.g., a getter/deserialize) must
validate bytes (or read as u8 and map 0->false, 1->true and reject other values)
or consider switching storage to an explicit u8 to avoid UB; reference these
functions by name in the comment so reviewers can find the guarantee.
Comment on lines
+22
to
+82
| #[tokio::test] | ||
| async fn test_commit_finalize_perf() { | ||
| // Setup | ||
| let (banks, _, authority, blockhash) = setup_program_test_env(vec![0; 10240]).await; | ||
| let new_state: Vec<u8> = vec![1; 10240]; | ||
|
|
||
| let new_account_balance = 1_000_000; | ||
|
|
||
| let (ix, pdas) = dlp::instruction_builder::commit_finalize( | ||
| authority.pubkey(), | ||
| DELEGATED_PDA_ID, | ||
| &mut CommitFinalizeArgs { | ||
| commit_id: 1, | ||
| allow_undelegation: true.into(), | ||
| data_is_diff: false.into(), | ||
| lamports: new_account_balance, | ||
| bumps: Default::default(), | ||
| reserved_padding: Default::default(), | ||
| }, | ||
| &new_state, | ||
| ); | ||
| let tx = Transaction::new_signed_with_payer( | ||
| &[ix], | ||
| Some(&authority.pubkey()), | ||
| &[&authority], | ||
| blockhash, | ||
| ); | ||
|
|
||
| // execute CommitFinalize and validate CU performmace | ||
| { | ||
| let BanksTransactionResultWithMetadata { | ||
| result: _, | ||
| metadata, | ||
| } = banks.process_transaction_with_metadata(tx).await.unwrap(); | ||
|
|
||
| let metadata = metadata.unwrap(); | ||
|
|
||
| assertables::assert_lt!(metadata.compute_units_consumed, 1100); | ||
|
|
||
| assert_eq!( | ||
| metadata.log_messages.len(), | ||
| 3, | ||
| "CommitFinalize must not log anything in OK scenario" | ||
| ); | ||
| } | ||
|
|
||
| let delegated_account = banks.get_account(DELEGATED_PDA_ID).await.unwrap().unwrap(); | ||
| assert_eq!(delegated_account.data, new_state); | ||
|
|
||
| let delegation_metadata_account = banks | ||
| .get_account(pdas.delegation_metadata) | ||
| .await | ||
| .unwrap() | ||
| .unwrap(); | ||
|
|
||
| let delegation_metadata = | ||
| DelegationMetadata::try_from_bytes_with_discriminator(&delegation_metadata_account.data) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(delegation_metadata.is_undelegatable, true); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
No test coverage for the diff (data_is_diff: true) path.
Both tests use data_is_diff: false.into(). The diff branch (DiffSet::try_new → apply_diff_in_place) is completely untested, including the empty-diff warning path, non-empty diff application, and the InvalidDiffAlignment error for a misaligned buffer.
Do you want me to draft a test_commit_finalize_diff test covering a basic non-empty diff and the empty-diff (noop) warning? I can open a new issue to track this if you prefer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_commit_finalize.rs` around lines 22 - 82, Add a new test (e.g.,
test_commit_finalize_diff) in tests/test_commit_finalize.rs that exercises the
commit_finalize path with CommitFinalizeArgs.data_is_diff set to true: (1)
construct and apply a valid non-empty diff payload and assert the target account
data changes as expected and DelegationMetadata is updated (reference
commit_finalize instruction_builder and CommitFinalizeArgs), (2) test an
empty-diff payload to trigger the DiffSet::try_new empty-diff warning and assert
the log message count/contents and no data change, and (3) test a misaligned
diff buffer that should produce InvalidDiffAlignment from
DiffSet::try_new/apply_diff_in_place and assert the transaction fails with the
expected error; reuse the existing test harness setup_program_test_env,
banks.process_transaction_with_metadata, and checks on metadata.log_messages and
account data to locate and validate behavior.
Comment on lines
+52
to
+55
| let BanksTransactionResultWithMetadata { | ||
| result: _, | ||
| metadata, | ||
| } = banks.process_transaction_with_metadata(tx).await.unwrap(); |
Contributor
There was a problem hiding this comment.
Perf test ignores transaction success — assertion failures will be misleading.
result: _ discards whether CommitFinalize actually succeeded. If the transaction errors, the subsequent assert_eq!(delegated_account.data, new_state) fails with "data didn't match" rather than surfacing the real program error. Assert success explicitly:
🛠️ Proposed fix
let BanksTransactionResultWithMetadata {
- result: _,
+ result,
metadata,
} = banks.process_transaction_with_metadata(tx).await.unwrap();
+ assert!(result.is_ok(), "CommitFinalize transaction failed: {:?}", result);
let metadata = metadata.unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_commit_finalize.rs` around lines 52 - 55, The test currently
discards the transaction outcome by using `result: _`, causing misleading
failures; change the destructuring from `BanksTransactionResultWithMetadata {
result: _, metadata }` to capture `result` (e.g.,
`BanksTransactionResultWithMetadata { result, metadata }`) and then assert the
transaction succeeded before checking account state—either `result.unwrap()` or
`assert!(result.is_ok(), "transaction failed: {:?}", result.err())` after
calling `process_transaction_with_metadata(tx).await.unwrap()` so that program
errors surface instead of a data mismatch on `assert_eq!(delegated_account.data,
new_state)`.
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
2 participants
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.
This diff adds
CommitFinalizeinstruction. The best performance we get is this:1055 CU... to commit10KBof data.Just for comparison,
CommitStateandFinalizeconsumes close to50,000 CU.perf benchmark with few variations.
During the validations of the bumps/PDAs, we have two options:
require_initialized_pda— that computes the address + ensures its owner is DLP + validates whether it is on the curve or not.require_initialized_pda_fast— that computes the address + ensures its owner is DLP. That is it.Currently, the PR has both options in
if false {} else {}branches. Once we finalize the branch we want to go with, we'll remove the other unused branch.Here are some numbers:
With
require_initialized_pda_fastbut withoutprogram_config:We'll go with this option, unless we have a very strong reason to not use it.
With
require_initialized_pda_fastandprogram_config:With
require_initialized_pdaandprogram_config:Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests
Chores