Conversation
snawaz
force-pushed
the
snawaz/commit-finalize-lamports
branch
3 times, most recently
from
124f94d to
17a016f
Compare
snawaz
force-pushed
the
snawaz/commit-finalize-lamports
branch
from
17a016f to
625998f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/error.rs`:
- Around line 152-155: The error message on the InsufficientRent enum variant
uses incorrect verb agreement; update the #[error(...)] string for the
InsufficientRent variant so it reads "The account lamports are too small to make
the account rent-exempt" -> change "is" to "are" (i.e., "The account lamports
are too small to make the account rent-exempt") by editing the #[error(...)]
attribute above the InsufficientRent variant.
In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 118-144: The delegation record is set from the account balance
after settling, which leaves excess lamports when the account initially had more
than delegation_record; update the assignment so delegation_record.lamports is
explicitly set to the target args.commit_lamports (instead of
args.delegated_account.lamports()), i.e., change the final reconciliation to
assign delegation_record.lamports = args.commit_lamports so the commit reaches
the intended value after the Transfer/increment/decrement logic in this function
handling args.commit_lamports, delegation_record, and args.delegated_account.
In `@tests/test_commit_finalize.rs`:
- Around line 319-378: Add a failing boundary test to ensure commit_finalize
rejects underfunded accounts: in tests/test_commit_finalize.rs (near
test_commit_finalize_lamports_decrease) add a case that computes the minimum
rent (Rent::minimum_balance) for the delegation record / delegated account data
length and sets commit_lamports to a value strictly less than that, build the
same commit_finalize instruction (using
dlp::instruction_builder::commit_finalize and CommitFinalizeArgs), send the
transaction with Transaction::new_signed_with_payer and call
banks.process_transaction(tx).await and assert it returns an error (use
unwrap_err() or is_err()), and optionally assert that neither the delegated PDA
nor the delegation_record PDA were created/changed (get_account returns None or
unchanged lamports); this ensures the rent guard prevents underfunded commits.
- Around line 302-317: The test currently asserts the destination balances
(delegated_account and DelegationRecord) but doesn't ensure the extra lamports
didn't come from the validator fees vault; capture the vault balance before the
operation and assert it is unchanged after the commit by fetching the vault
account (e.g., validator_fees_vault or the constant/ID used for the fees vault),
calling banks.get_account(VAULT_ID).await.unwrap().unwrap(), storing its
lamports, and then after the existing assertions
assert_eq!(vault_account.lamports, original_vault_lamports) to pin the increase
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: af3d8ac0-1a91-4da0-927f-318ae7cb22aa
📒 Files selected for processing (9)
src/error.rssrc/instruction_builder/commit_finalize.rssrc/instruction_builder/commit_finalize_from_buffer.rssrc/processor/fast/commit_finalize.rssrc/processor/fast/commit_finalize_from_buffer.rssrc/processor/fast/internal/commit_finalize_internal.rssrc/processor/fast/utils/mod.rssrc/processor/fast/utils/requires.rstests/test_commit_finalize.rs
| match args.commit_lamports.cmp(&delegation_record.lamports) { | ||
| std::cmp::Ordering::Greater => { | ||
| system::Transfer { | ||
| from: args.validator, | ||
| to: args.delegated_account, | ||
| lamports: args.commit_lamports - delegation_record.lamports, | ||
| } | ||
| .invoke()?; | ||
| } | ||
| std::cmp::Ordering::Less => { | ||
| let amount = delegation_record.lamports - args.commit_lamports; | ||
|
|
||
| args.delegated_account.lamports_decrement_by(amount)?; | ||
| args.validator_fees_vault.lamports_increment_by(amount)?; | ||
|
|
||
| require_ge!( | ||
| args.delegated_account.lamports(), | ||
| Rent::get()? | ||
| .try_minimum_balance(args.delegated_account.data_len())?, | ||
| DlpError::InsufficientRent | ||
| ); | ||
| } | ||
| std::cmp::Ordering::Equal => {} | ||
| } | ||
|
|
||
| // Update the delegation record lamports after settling. | ||
| delegation_record.lamports = args.delegated_account.lamports(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for tests that exercise the excess lamports scenario
rg -n "delegation_record.*lamports|commit_lamports" --type rust -g "*test*" -C 3
# Check if there's documentation about expected behavior
rg -n "excess.*lamport|lamport.*excess" --type rust -C 2Repository: magicblock-labs/delegation-program
Length of output: 4212
🏁 Script executed:
#!/bin/bash
# Find the setup function definition
rg -n "fn setup_program_test_env_with_record_lamports" --type rust -A 30
# Verify the exact implementation at line 144
sed -n '138,148p' src/processor/fast/internal/commit_finalize_internal.rsRepository: magicblock-labs/delegation-program
Length of output: 2387
🏁 Script executed:
#!/bin/bash
# Find the require_ge check that allows account to have more lamports than record
sed -n '105,120p' src/processor/fast/internal/commit_finalize_internal.rs
# Also check what happens before the match statement - what's the state of the account?
sed -n '80,145p' src/processor/fast/internal/commit_finalize_internal.rsRepository: magicblock-labs/delegation-program
Length of output: 2935
The lamport reconciliation logic has a bug when the account holds excess lamports.
The implementation sets delegation_record.lamports = args.delegated_account.lamports() at line 144, but this fails to achieve the target commit_lamports when delegated_account.lamports() > delegation_record.lamports initially (which is allowed by the require_ge! check at line 110).
Example (Less branch):
- Initial:
delegated_account.lamports() = 1000,delegation_record.lamports = 800,commit_lamports = 700 - After decrement:
delegated_account.lamports() = 900 - Result:
delegation_record.lamports = 900(expected: 700)
Example (Greater branch):
- Initial:
delegated_account.lamports() = 1000,delegation_record.lamports = 800,commit_lamports = 900 - After transfer:
delegated_account.lamports() = 1100 - Result:
delegation_record.lamports = 1100(expected: 900)
The amount calculations use delegation_record.lamports instead of delegated_account.lamports(), causing the reconciliation to leave excess lamports in the account. Change line 144 to delegation_record.lamports = args.commit_lamports to settle to the target value.
🤖 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 118 -
144, The delegation record is set from the account balance after settling, which
leaves excess lamports when the account initially had more than
delegation_record; update the assignment so delegation_record.lamports is
explicitly set to the target args.commit_lamports (instead of
args.delegated_account.lamports()), i.e., change the final reconciliation to
assign delegation_record.lamports = args.commit_lamports so the commit reaches
the intended value after the Transfer/increment/decrement logic in this function
handling args.commit_lamports, delegation_record, and args.delegated_account.
There was a problem hiding this comment.
No. Your reasoning is wrong. commit_lamports is not the target one, it is the current value on ER node but in the meanwhile, on the base chain, the delegated_account might have received lamports from others (but it cannot send it, because the account has been delegated to the delegation-program), so the only possibility is: delegated_account.lamports() can be equal or greater than the lamports which is delegated to the ER.
In the Less branch example: delegation_record.lamports = 900 is correct. (expected: 700) is wrong. Likewise, delegation_record.lamports = 1100 is correct.
and then delegation_record.lamports is set to the value on the base chain, not the ER.
Let me know if that makes sense now.
There was a problem hiding this comment.
@snawaz, yes, that makes complete sense now — thank you for the detailed explanation. My earlier reasoning was flawed.
To summarize the correct model:
commit_lamports= the lamport balance as observed on the ER node (not a target).delegation_record.lamports= the last recorded base-chain balance.delegated_account.lamports()= the actual current base-chain balance, which may exceeddelegation_record.lamportsdue to external transfers into the account (sends are blocked since the account is delegated to the program).
The match computes the delta relative to delegation_record.lamports, applies the transfer/decrement accordingly, and then delegation_record.lamports = args.delegated_account.lamports() correctly captures the true post-settlement base-chain balance — excess externally received lamports included. The logic is sound. I apologize for the noise.
✏️ Learnings added
Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 148
File: src/processor/fast/internal/commit_finalize_internal.rs:118-144
Timestamp: 2026-03-10T08:59:47.915Z
Learning: In `delegation-program` (`src/processor/fast/internal/commit_finalize_internal.rs`), `commit_lamports` represents the lamport balance observed on the Ephemeral Rollup (ER) node, NOT a target base-chain balance. The match block computes a delta relative to `delegation_record.lamports` (last recorded base-chain value) and applies a transfer or decrement. After settling, `delegation_record.lamports = args.delegated_account.lamports()` is intentionally set to the actual post-settlement base-chain balance, which can exceed `commit_lamports` because `delegated_account` can receive lamports from external parties on the base chain (but cannot send, since it is delegated to the delegation program). The invariant `delegated_account.lamports() >= delegation_record.lamports` is enforced by the `require_ge!` check.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: snawaz
Repo: magicblock-labs/delegation-program PR: 148
File: src/error.rs:152-155
Timestamp: 2026-03-10T08:21:51.883Z
Learning: In the `delegation-program` repository (`src/error.rs`), the `InsufficientRent` error message uses "The account lamports is too small to make the account rent-exempt". The team intentionally uses "is" (not "are") because `lamports` is treated as a single field name (a `u64` value), not as a plural noun. Do not flag this as a grammar issue.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_commit_finalize.rs (2)
268-317:⚠️ Potential issue | 🟠 MajorAdd the below-rent rejection case.
The new lamport tests still only cover successful grow/shrink flows. Please add a case where
CommitFinalizeArgs.lamportsisRent::default().minimum_balance(new_state.len()) - 1, assertprocess_transactionfails, and assert the delegated PDA plusDelegationRecord.lamportsstay unchanged so the rent guard cannot regress silently. Based on learnings, in Solana tests usingsolana_program_test, useRent::default()instead ofRent::get()because the rent sysvar is unavailable in the test context.Also applies to: 319-378
🤖 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 268 - 317, Add a new negative test case in the same test module (e.g., alongside test_commit_finalize_lamports_increase) that builds a CommitFinalizeArgs where lamports = Rent::default().minimum_balance(new_state_len) - 1 (use Rent::default() not Rent::get()), call the instruction via Transaction::new_signed_with_payer and assert banks.process_transaction(tx).await.unwrap_err() (i.e., the transaction fails), then reload the DELEGATED_PDA_ID account and pdas.delegation_record account and assert both their lamports values remain unchanged (use DelegationRecord::try_from_bytes_with_discriminator to read delegation_record.lamports) to ensure the rent guard prevented regression.
302-317:⚠️ Potential issue | 🟠 MajorAssert that the grow path does not drain
validator_fees_vault.This only proves the destination balances. It still passes if the extra lamports are sourced from
validator_fees_vaultinstead of the validator, so please snapshot the vault beforecommit_finalizeand assert it is unchanged afterwards.Suggested test update
let (ix, pdas) = dlp::instruction_builder::commit_finalize( authority.pubkey(), DELEGATED_PDA_ID, &mut CommitFinalizeArgs { commit_id: 1, allow_undelegation: false.into(), data_is_diff: false.into(), lamports: commit_lamports, bumps: Default::default(), reserved_padding: Default::default(), }, &vec![1; 8], ); + let initial_fees_vault_lamports = banks + .get_account(pdas.validator_fees_vault) + .await + .unwrap() + .unwrap() + .lamports; let tx = Transaction::new_signed_with_payer( &[ix], Some(&authority.pubkey()), &[&authority], @@ let delegation_record = DelegationRecord::try_from_bytes_with_discriminator( &delegation_record_account.data, ) .unwrap(); assert_eq!(delegation_record.lamports, commit_lamports); + + let fees_vault = banks + .get_account(pdas.validator_fees_vault) + .await + .unwrap() + .unwrap(); + assert_eq!(fees_vault.lamports, initial_fees_vault_lamports); }🤖 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 302 - 317, Before invoking commit_finalize in this test, snapshot the lamports of the validator_fees_vault account by calling banks.get_account(validator_fees_vault).await.unwrap().unwrap().lamports and store it (e.g., pre_vault_lamports), then after the existing assertions (delegated_account and delegation_record checks) fetch the vault account again and assert its lamports equal pre_vault_lamports; reference the validator_fees_vault identifier and the commit_finalize test flow so the snapshot is taken immediately before the operation and the equality check is done immediately after.
🤖 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/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 118-125: The top-up branch that creates and invokes
system::Transfer (using args.validator -> args.delegated_account with lamports =
commit_lamports - delegation_record.lamports) requires the funding account
(args.validator) to be [WRITE, SIGNER]; currently the validator is emitted
readonly causing CPIs to fail. Fix by ensuring the account metas passed into
this instruction (the account at index 0 / the validator account) are marked
writable and signer before the branch executes (or change the builder that
constructs the instruction metas so args.validator is emitted as writable), so
the system::Transfer::invoke() can succeed.
---
Duplicate comments:
In `@tests/test_commit_finalize.rs`:
- Around line 268-317: Add a new negative test case in the same test module
(e.g., alongside test_commit_finalize_lamports_increase) that builds a
CommitFinalizeArgs where lamports =
Rent::default().minimum_balance(new_state_len) - 1 (use Rent::default() not
Rent::get()), call the instruction via Transaction::new_signed_with_payer and
assert banks.process_transaction(tx).await.unwrap_err() (i.e., the transaction
fails), then reload the DELEGATED_PDA_ID account and pdas.delegation_record
account and assert both their lamports values remain unchanged (use
DelegationRecord::try_from_bytes_with_discriminator to read
delegation_record.lamports) to ensure the rent guard prevented regression.
- Around line 302-317: Before invoking commit_finalize in this test, snapshot
the lamports of the validator_fees_vault account by calling
banks.get_account(validator_fees_vault).await.unwrap().unwrap().lamports and
store it (e.g., pre_vault_lamports), then after the existing assertions
(delegated_account and delegation_record checks) fetch the vault account again
and assert its lamports equal pre_vault_lamports; reference the
validator_fees_vault identifier and the commit_finalize test flow so the
snapshot is taken immediately before the operation and the equality check is
done immediately after.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eccdb8b5-9a3a-47ce-9cf0-c0b911686d1e
📒 Files selected for processing (9)
src/error.rssrc/instruction_builder/commit_finalize.rssrc/instruction_builder/commit_finalize_from_buffer.rssrc/processor/fast/commit_finalize.rssrc/processor/fast/commit_finalize_from_buffer.rssrc/processor/fast/internal/commit_finalize_internal.rssrc/processor/fast/utils/mod.rssrc/processor/fast/utils/requires.rstests/test_commit_finalize.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_commit_finalize.rs`:
- Around line 102-103: Remove the leftover commented debug line
"//tokio::time::sleep(Duration::from_secs(10)).await;" from the
test_commit_finalize test in tests/test_commit_finalize.rs; simply delete that
commented sleep so the test file contains no commented-out debug code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9984f2a0-3247-4a1b-87b0-6e156d2f35b8
📒 Files selected for processing (10)
src/error.rssrc/instruction_builder/commit_finalize.rssrc/instruction_builder/commit_finalize_from_buffer.rssrc/processor/fast/commit_finalize.rssrc/processor/fast/commit_finalize_from_buffer.rssrc/processor/fast/internal/commit_finalize_internal.rssrc/processor/fast/utils/mod.rssrc/processor/fast/utils/requires.rstests/test_commit_finalize.rstests/test_commit_finalize_from_buffer.rs
snawaz
force-pushed
the
snawaz/commit-finalize-lamports
branch
from
b864cb4 to
1755f38
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/instruction_builder/commit_finalize_from_buffer.rs (1)
78-91:⚠️ Potential issue | 🟡 MinorSize budget includes account not in instruction.
The
commit_finalize_from_buffer_size_budgetfunction includesprogram_config_pda(line 89) in the account size calculation, but this account is not part of the instruction's account list (lines 50-58 show only 7 accounts, none of which isprogram_config).This will over-estimate the data size budget, which is wasteful but not incorrect. However, it may indicate stale code from a previous version.
🔧 Proposed fix
pub fn commit_finalize_from_buffer_size_budget( delegated_account: AccountSizeClass, ) -> u32 { total_size_budget(&[ DLP_PROGRAM_DATA_SIZE_CLASS, AccountSizeClass::Tiny, // validator delegated_account, // delegated_account AccountSizeClass::Tiny, // delegation_record_pda AccountSizeClass::Tiny, // delegation_metadata_pda delegated_account, // data_buffer AccountSizeClass::Tiny, // validator_fees_vault_pda - AccountSizeClass::Tiny, // program_config_pda AccountSizeClass::Tiny, // system_program ]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instruction_builder/commit_finalize_from_buffer.rs` around lines 78 - 91, The size budget function commit_finalize_from_buffer_size_budget is including an AccountSizeClass::Tiny entry annotated as program_config_pda that is not actually part of the instruction's account list; remove that program_config_pda entry from the array passed to total_size_budget so the returned budget matches the real accounts (keep the other entries: DLP_PROGRAM_DATA_SIZE_CLASS, validator, delegated_account, delegation_record_pda, delegation_metadata_pda, data_buffer, validator_fees_vault_pda, and system_program). Also scan for any stale comments/annotations around program_config_pda in commit_finalize_from_buffer_size_budget and delete them so the function reflects the current instruction accounts.tests/test_commit_finalize.rs (1)
189-214: 🧹 Nitpick | 🔵 TrivialAdd a regression case where the base-chain account already has extra lamports.
The new fixture still forces
delegated_account.lamports == delegation_record.lamports, so this suite won’t catch a future refactor that incorrectly persistsargs.commit_lamportsinstead of the actual post-settlement base-chain balance. Let the helper take separate account and record lamports, then add one increase/decrease case withdelegated_account.lamports() > delegation_record.lamports.Based on learnings:
commit_lamportsis the ER-observed balance, whiledelegated_account.lamports()may already be higher on the base chain anddelegation_record.lamportsis intentionally updated to that actual post-settlement balance.Also applies to: 269-403
🤖 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 189 - 214, The test helper setup_program_test_env_with_record_lamports currently forces the delegated account and delegation_record to have identical lamports; change it to accept separate "delegated_account_lamports" and "delegation_record_lamports" (keep the function name setup_program_test_env_with_record_lamports but update its parameters and usage) and initialize the ProgramTest added account (DELEGATED_PDA_ID) with delegated_account_lamports while populating the stored delegation record with delegation_record_lamports; then add a regression test case where delegated_account_lamports > delegation_record_lamports to ensure the code uses the actual base-chain balance (delegated_account.lamports()) rather than args.commit_lamports when persisting the final delegation_record.lamports (apply the same change pattern to all affected tests around the other ranges mentioned).src/processor/fast/internal/commit_finalize_internal.rs (1)
68-78:⚠️ Potential issue | 🟠 MajorDon’t require
validator_fees_vaultwritable unless the commit shrinks lamports.Only
std::cmp::Ordering::Lessmutates the vault, but this precheck now forces a write lock on every finalize. Because the vault PDA is shared per validator (src/pda.rs:163-169), that serializes otherwise unrelated commits for the same validator and rejects callers that still pass the legacy readonly meta even when the vault is never touched.🔧 Suggested change
require_initialized_pda_fast!( args.validator_fees_vault, &[ pda::VALIDATOR_FEES_VAULT_TAG, args.validator.address().as_ref(), &[args.bumps.validator_fees_vault], crate::fast::ID.as_ref(), PDA_MARKER ], - true + false ); ... std::cmp::Ordering::Less => { + require!(args.validator_fees_vault.is_writable(), ProgramError::Immutable); let amount = delegation_record.lamports - args.commit_lamports; args.delegated_account.lamports_decrement_by(amount)?; args.validator_fees_vault.lamports_increment_by(amount)?;🤖 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 68 - 78, The require_initialized_pda_fast! call currently forces the validator_fees_vault account to be writable unconditionally; change it to only require writable when the finalize actually decreases the vault lamports (i.e., when the operation is mutating). Detect that by comparing the before/after lamports (or using the existing delta/ordering value) and pass writable = true only when the comparison yields std::cmp::Ordering::Less (otherwise pass writable = false). Update the require_initialized_pda_fast! invocation that references args.validator_fees_vault and args.bumps.validator_fees_vault so the writable flag is conditional on the lamports-decrease 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/processor/fast/commit_finalize_from_buffer.rs`:
- Around line 20-26: The top doc comment listing accounts in
commit_finalize_from_buffer.rs incorrectly marks the validator only as
`[signer]`; update that first entry to `[signer, writable]` to match the
instruction builder and the same fix in commit_finalize.rs so the validator
account documentation is consistent with the actual account flags (update the
comment line that currently reads "0: `[signer]` the validator requesting the
commit" to include `[writable]`).
In `@src/processor/fast/commit_finalize.rs`:
- Around line 21-26: Update the doc comment in commit_finalize.rs to mark the
validator account as writable to match the instruction builder; change the first
entry from "`[signer]` the validator requesting the commit" to include
"`[writable]`" (e.g., "`[signer, writable]`" or similar) so it reflects
AccountMeta::new(validator, true) used when building the instruction and keeps
the comments consistent with the actual account metas.
In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 116-146: After the match that adjusts lamports (the block using
args.commit_lamports.cmp(&delegation_record.lamports)), add a single
rent-exemption check after the match to ensure the delegated account remains
rent-exempt regardless of branch; call Rent::get()? and use
try_minimum_balance(args.delegated_account.data_len()) to compute the minimum
and require_ge! that args.delegated_account.lamports() meets that minimum,
returning DlpError::InsufficientRent on failure — this uses the existing
args.delegated_account, Rent::get(), try_minimum_balance, and
DlpError::InsufficientRent symbols and should be placed immediately after the
match and before updating delegation_record.lamports.
---
Outside diff comments:
In `@src/instruction_builder/commit_finalize_from_buffer.rs`:
- Around line 78-91: The size budget function
commit_finalize_from_buffer_size_budget is including an AccountSizeClass::Tiny
entry annotated as program_config_pda that is not actually part of the
instruction's account list; remove that program_config_pda entry from the array
passed to total_size_budget so the returned budget matches the real accounts
(keep the other entries: DLP_PROGRAM_DATA_SIZE_CLASS, validator,
delegated_account, delegation_record_pda, delegation_metadata_pda, data_buffer,
validator_fees_vault_pda, and system_program). Also scan for any stale
comments/annotations around program_config_pda in
commit_finalize_from_buffer_size_budget and delete them so the function reflects
the current instruction accounts.
In `@src/processor/fast/internal/commit_finalize_internal.rs`:
- Around line 68-78: The require_initialized_pda_fast! call currently forces the
validator_fees_vault account to be writable unconditionally; change it to only
require writable when the finalize actually decreases the vault lamports (i.e.,
when the operation is mutating). Detect that by comparing the before/after
lamports (or using the existing delta/ordering value) and pass writable = true
only when the comparison yields std::cmp::Ordering::Less (otherwise pass
writable = false). Update the require_initialized_pda_fast! invocation that
references args.validator_fees_vault and args.bumps.validator_fees_vault so the
writable flag is conditional on the lamports-decrease check.
In `@tests/test_commit_finalize.rs`:
- Around line 189-214: The test helper
setup_program_test_env_with_record_lamports currently forces the delegated
account and delegation_record to have identical lamports; change it to accept
separate "delegated_account_lamports" and "delegation_record_lamports" (keep the
function name setup_program_test_env_with_record_lamports but update its
parameters and usage) and initialize the ProgramTest added account
(DELEGATED_PDA_ID) with delegated_account_lamports while populating the stored
delegation record with delegation_record_lamports; then add a regression test
case where delegated_account_lamports > delegation_record_lamports to ensure the
code uses the actual base-chain balance (delegated_account.lamports()) rather
than args.commit_lamports when persisting the final delegation_record.lamports
(apply the same change pattern to all affected tests around the other ranges
mentioned).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7db01719-0a91-4593-a605-e574cefc2ea3
📒 Files selected for processing (10)
src/error.rssrc/instruction_builder/commit_finalize.rssrc/instruction_builder/commit_finalize_from_buffer.rssrc/processor/fast/commit_finalize.rssrc/processor/fast/commit_finalize_from_buffer.rssrc/processor/fast/internal/commit_finalize_internal.rssrc/processor/fast/utils/mod.rssrc/processor/fast/utils/requires.rstests/test_commit_finalize.rstests/test_commit_finalize_from_buffer.rs
| /// 0: `[signer]` the validator requesting the commit | ||
| /// 1: `[]` the delegated account | ||
| /// 2: `[writable]` the PDA storing the new state | ||
| /// 3: `[writable]` the PDA storing the commit record | ||
| /// 4: `[]` the delegation record | ||
| /// 5: `[writable]` the delegation metadata | ||
| /// 6: `[]` the validator fees vault | ||
| /// 1: `[writable]` the delegated account | ||
| /// 2: `[writable]` the delegation record | ||
| /// 3: `[writable]` the delegation metadata | ||
| /// 4: `[]` the data buffer | ||
| /// 5: `[writable]` the validator fees vault | ||
| /// 6: `[]` system program |
There was a problem hiding this comment.
Doc comment missing [writable] marker for validator account.
Same as in commit_finalize.rs: line 20 shows the validator as [signer] only, but the instruction builder marks it as writable. The doc comment should be updated for consistency.
📝 Proposed fix
/// Accounts:
///
-/// 0: `[signer]` the validator requesting the commit
+/// 0: `[signer, writable]` the validator requesting the commit
/// 1: `[writable]` the delegated account
/// 2: `[writable]` the delegation record
/// 3: `[writable]` the delegation metadata🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/processor/fast/commit_finalize_from_buffer.rs` around lines 20 - 26, The
top doc comment listing accounts in commit_finalize_from_buffer.rs incorrectly
marks the validator only as `[signer]`; update that first entry to `[signer,
writable]` to match the instruction builder and the same fix in
commit_finalize.rs so the validator account documentation is consistent with the
actual account flags (update the comment line that currently reads "0:
`[signer]` the validator requesting the commit" to include `[writable]`).
| /// 0: `[signer]` the validator requesting the commit | ||
| /// 1: `[]` the delegated account | ||
| /// 2: `[]` the delegation record | ||
| /// 1: `[writable]` the delegated account | ||
| /// 2: `[writable]` the delegation record | ||
| /// 3: `[writable]` the delegation metadata | ||
| /// 4: `[]` the validator fees vault | ||
| /// 4: `[writable]` the validator fees vault | ||
| /// 5: `[]` system program |
There was a problem hiding this comment.
Doc comment missing [writable] marker for validator account.
Line 21 shows the validator as [signer] only, but the instruction builder marks it as writable (AccountMeta::new(validator, true)). The doc comment should be updated for consistency.
📝 Proposed fix
/// Accounts:
///
-/// 0: `[signer]` the validator requesting the commit
+/// 0: `[signer, writable]` the validator requesting the commit
/// 1: `[writable]` the delegated account
/// 2: `[writable]` the delegation record
/// 3: `[writable]` the delegation metadata🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/processor/fast/commit_finalize.rs` around lines 21 - 26, Update the doc
comment in commit_finalize.rs to mark the validator account as writable to match
the instruction builder; change the first entry from "`[signer]` the validator
requesting the commit" to include "`[writable]`" (e.g., "`[signer, writable]`"
or similar) so it reflects AccountMeta::new(validator, true) used when building
the instruction and keeps the comments consistent with the actual account metas.
| args.delegated_account.resize(args.new_state.data_len())?; | ||
|
|
||
| match args.commit_lamports.cmp(&delegation_record.lamports) { | ||
| std::cmp::Ordering::Greater => { | ||
| require!(args.validator.is_writable(), ProgramError::Immutable); | ||
| system::Transfer { | ||
| from: args.validator, | ||
| to: args.delegated_account, | ||
| lamports: args.commit_lamports - delegation_record.lamports, | ||
| } | ||
| .invoke()?; | ||
| } | ||
| std::cmp::Ordering::Less => { | ||
| let amount = delegation_record.lamports - args.commit_lamports; | ||
|
|
||
| args.delegated_account.lamports_decrement_by(amount)?; | ||
| args.validator_fees_vault.lamports_increment_by(amount)?; | ||
|
|
||
| // require the account is still rent-exempted even after decrementing lamports | ||
| require_ge!( | ||
| args.delegated_account.lamports(), | ||
| Rent::get()? | ||
| .try_minimum_balance(args.delegated_account.data_len())?, | ||
| DlpError::InsufficientRent | ||
| ); | ||
| } | ||
| std::cmp::Ordering::Equal => {} | ||
| } | ||
|
|
||
| // Update the delegation record lamports after settling. | ||
| delegation_record.lamports = args.delegated_account.lamports(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In pinocchio 0.10.1 / Solana realloc semantics, does AccountView::resize()enforce rent exemption when increasing account data length, or should programs explicitly verifylamports >= Rent::minimum_balance(data_len) after resizing?
💡 Result:
AccountView::resize() in the solana-account-view v1.x that Pinocchio 0.10.1 depends on does not enforce rent exemption. It only checks borrow state, updates data_len/resize_delta, enforces the max growth limit, and zero-fills any newly-added bytes—there is no lamports/rent check anywhere in the implementation. [1]
So if your program increases an account’s data length, you should explicitly ensure it remains rent-exempt, i.e. fund/check lamports >= Rent::minimum_balance(new_data_len) (typically fund before resizing so you can fail early). This is also the approach reflected by Pinocchio’s system helper, which computes the rent-exempt minimum, transfers missing lamports, and only then calls resize_unchecked. [2] Solana guidance likewise notes you should ensure an account remains rent-exempt when resizing. [3]
Answer: AccountView::resize() does not enforce rent exemption; programs should explicitly verify/fund to meet Rent::minimum_balance(data_len) when growing account data. [1][2][3]
References:
[1] solana-account-view v1.0.0 AccountView::resize / resize_unchecked source (no lamports/rent checks) (docs.rs)
[2] pinocchio-system create_account_with_minimum_balance_signed (compute minimum balance + transfer, then resize) (docs.rs)
[3] Solana course note: “Always ensure your account remains rent-exempt before resizing.” (solana.com)
Citations:
- 1: https://docs.rs/solana-account-view/1.0.0/src/solana_account_view/lib.rs.html
- 2: https://docs.rs/pinocchio-system/latest/src/pinocchio_system/lib.rs.html
- 3: https://solana.com/developers/courses/program-optimization/program-architecture?utm_source=openai
Add a post-match rent-exemption check for the delegated account.
AccountView::resize() does not enforce rent-exemption when growing account data. The explicit InsufficientRent guard only runs in the Less branch. If resizing increases the data length, the Greater and Equal branches could leave the account below the rent minimum despite transferring or moving lamports. Add a single check after the match block to verify rent-exemption holds regardless of which branch executed:
require_ge!(
args.delegated_account.lamports(),
Rent::get()?
.try_minimum_balance(args.delegated_account.data_len())?,
DlpError::InsufficientRent
);🤖 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 116 -
146, After the match that adjusts lamports (the block using
args.commit_lamports.cmp(&delegation_record.lamports)), add a single
rent-exemption check after the match to ensure the delegated account remains
rent-exempt regardless of branch; call Rent::get()? and use
try_minimum_balance(args.delegated_account.data_len()) to compute the minimum
and require_ge! that args.delegated_account.lamports() meets that minimum,
returning DlpError::InsufficientRent on failure — this uses the existing
args.delegated_account, Rent::get(), try_minimum_balance, and
DlpError::InsufficientRent symbols and should be placed immediately after the
match and before updating delegation_record.lamports.
1 participant
#131 did not settle lamports. This PR does it properly.
CodeRabbit comment on #131:
Summary by CodeRabbit
New Features
Bug Fixes
Tests