feat: add configurable access permission enforcement

[thlorenz]

Summary

Add configurable access permission enforcement to the transaction processor.

This change introduces an enforce_access_permissions flag to TransactionBatchProcessor that
allows disabling access permission checks during transaction execution. When disabled,
transactions can write to non-delegated accounts and undelegated accounts can be used as fee
payers.

Details

The transaction processor now exposes fine-grained control over access permission enforcement
through the enforce_access_permissions flag. This enables scenarios where stricter access
controls need to be relaxed, such as during conformance testing or special operational modes.

When enforce_access_permissions is true (default), the processor validates that accounts used
for writing operations are properly delegated and that fee payers meet access requirements.
Exactly as it did before.
When false, these checks are bypassed, allowing transactions to operate on any accounts.

Test coverage

Added three test cases to validate the feature:

• Unit test verifying different error types with flag enabled vs disabled
• Integration test confirming transactions succeed when writing to non-delegated accounts with flag disabled
• Integration test confirming transactions fail with proper error when flag is enabled

Summary by CodeRabbit

• New Features

  • Configurable enforcement flag for account-access and fee-payer validation (enabled by default); disabling it relaxes certain access checks and alters some execution outcomes (escrow, delegated/privileged payer behaviors).

• Tests

  • Expanded tests and integrations covering both enforcement states, fee-payer scenarios, program-loading paths, transfer outcomes, and related edge cases.

[coderabbitai]

Walkthrough

Added a public boolean field enforce_access_permissions to TransactionBatchProcessor, propagated it through constructors, cloning, program-cache initialization, and validation/execution paths. Validation helpers and error outcomes were updated to branch on this flag. Tests were updated and expanded to cover both flag states.

Changes

Cohort / File(s)	Summary
Core Implementation src/transaction_processor.rs	Added pub enforce_access_permissions: bool to TransactionBatchProcessor<FG>; updated constructors new_uninitialized(...) and new(...) to accept the flag; new_from and program-cache initialization/cloning now propagate it. Validation method signatures (validate_transaction_nonce_and_fee_payer, validate_transaction_fee_payer) accept the flag and account-access, fee-payer, execution, and error-construction paths conditionally follow strict or relaxed rules based on the flag.
Tests — conformance tests/conformance.rs	Updated TransactionBatchProcessor::new(...) call sites to include the new boolean parameter and an injected program-runtime environment argument.
Tests — integration & helpers tests/integration_test.rs	Expanded imports; added account helper constructors (create_delegated_account, create_non_delegated_account, create_privileged_account, borrowed-account helpers); replaced direct account constructions with helpers; added many integration tests exercising enforce_access_permissions true/false across fee-payer validation, delegated vs non-delegated writes, privileged/borrowed accounts, escrow fee flows, and related edge cases.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title accurately describes the main change: adding a configurable flag to control access permission enforcement in transaction processing.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch thlorenz/access-check-toggle

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (6)

tests/conformance.rs (1)

231-238: Constructor update matches new API; behavior preserved

The added true argument to TransactionBatchProcessor::new keeps conformance tests on the strict (enforced) path, matching previous behavior. If the long‑term intent is to run these ignored tests without delegation noise, consider threading a parameter or using false here instead, but it’s fine as‑is given the test is already #[ignore].

tests/integration_test.rs (1)

84-92: SvmTestEnvironment now explicitly opts into strict access enforcement

Passing true into TransactionBatchProcessor::new keeps all existing integration tests on the original strict behavior, which is what you want for backwards compatibility. If you foresee more tests needing relaxed enforcement, consider exposing this as a configurable parameter on SvmTestEntry/SvmTestEnvironment instead of hard‑coding true and mutating the field from specific tests.

src/transaction_processor.rs (4)

172-286: enforce_access_permissions field and constructor wiring are consistent

Adding pub enforce_access_permissions: bool with a Default of true, threading it through new_uninitialized, new, and new_from, correctly preserves existing behavior while making the flag configurable:

• Default and callers that don’t know about the flag remain strict.
• new_from inherits the parent processor’s policy, which is important for multi‑slot flows.

Two optional polish points:

• Consider making the field non‑pub (e.g., pub(crate) or a setter) if you want to confine toggling to controlled contexts; exposing a mutable flag on a shared processor is easy to misuse in production.
• Including enforce_access_permissions in the manual Debug impl would make it easier to see at a glance which mode a processor instance is in.

---

420-507: Account-access validation block works but currently sidesteps metrics and balance tracking

The new block:

• Guards loaded_transaction.validate_accounts_access(tx) behind self.enforce_access_permissions.
• On violation, synthesizes an ExecutedTransaction with status: Err(..) and a clear log message, pushes it into processing_results, and continues without calling execute_loaded_transaction.

Functionally that’s sound and matches the intent from access_permissions.rs (strict mode rejects writes to non‑delegated accounts with a helpful log). A couple of side effects to be aware of:

• This path does not update balances.pre/balances.post, so such “synthetic executions” will be absent from the AccountsBalances output.
• Because of the continue inside measure_us!, this branch doesn’t contribute to execution_us or any ExecuteTimingType::ExecuteUs accounting.
• No TransactionErrorMetrics counters are incremented for this specific TransactionError variant.

If you want these violations to show up uniformly in metrics and balance tracking, consider restructuring this branch to return a ProcessedTransaction from the match (letting the outer processing_results.push(processing_result) and timing logic run) instead of pushing and continue‑ing from inside the match.

---

593-750: Disabling access enforcement also disables escrow fallback in fee-payer validation

In validate_transaction_fee_payer, the loader logic is now:

let mut loaded_fee_payer = if !enforce_access_permissions || fee_lamports_per_signature == 0 {
    // use initial_loaded or default, no delegated/escrow checks
    initial_loaded.unwrap_or_else(|| LoadedTransactionAccount { ... })
} else {
    // prior behavior: require delegated/privileged fee-payer, else delegated escrow PDA
    initial_loaded
        .filter(is_delegated_or_privileged)
        .or_else(|| { /* escrow path */ })
        .ok_or_else(|| { error_counters.invalid_account_for_fee += 1; TransactionError::InvalidAccountForFee })?
};

This achieves the PR goal that an undelegated fee payer is accepted when enforce_access_permissions == false, but it also subtly changes behavior:

• When only a delegated escrow account exists (no on‑ledger fee-payer), strict mode (true) still finds and uses the escrow PDA; relaxed mode (false) now ignores escrow entirely and treats the (missing) fee-payer as a default zero‑lamport account, leading to a different error (e.g., InsufficientFundsForFee or rent) instead of the previous InvalidAccountForFee via the escrow path.
• Similarly, any future logic added to the escrow fallback (e.g., metrics, special handling) will be skipped when enforcement is disabled.

If the intent was strictly to skip the delegated/privileged requirement, not to change which account is considered as the fee source, a more targeted structure might be:

• Keep trying the escrow PDA when the direct fee-payer is absent, regardless of enforcement mode.
• Only gate the filter(is_delegated_or_privileged) checks on enforce_access_permissions.

That would preserve escrow behavior while still allowing undelegated accounts to act as fee payers when the flag is false.

---

2250-2901: Validation tests updated for the new flag; new fee-payer test covers strict vs relaxed modes

The existing validate_transaction_nonce_and_fee_payer tests have been correctly updated to pass an explicit true for enforce_access_permissions, so they continue to assert the strict behavior (delegated/privileged fee-payer, escrow fallback, correct error counters).

The new test_enforce_access_permissions_flag_for_fee_payer is a useful addition:

• With enforce_access_permissions = true, an undelegated fee-payer correctly results in Err(TransactionError::InvalidAccountForFee).
• With enforce_access_permissions = false, the same setup now gets past fee-payer validation and instead fails with TransactionError::InvalidProgramForExecution (because no valid program account is present), demonstrating that the flag only relaxes access checks and not general program loading.

Minor nits you might consider:

• The variable name processor_strict in the relaxed branch is misleading; renaming to something like processor_relaxed would improve clarity.
• This test relies on CheckedTransactionDetails::default() for fee config; if that default ever changes (e.g., nonzero lamports_per_signature), the test behavior might shift. Documenting that assumption in a comment would make the intent clearer.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 07a57a9 and f008663.

📒 Files selected for processing (3)

• src/transaction_processor.rs
• tests/conformance.rs
• tests/integration_test.rs

🧰 Additional context used

🧬 Code graph analysis (2)

tests/integration_test.rs (1)

src/transaction_processor.rs (3)

• new (250-269)
• default (155-164)
• default (207-219)

src/transaction_processor.rs (1)

src/access_permissions.rs (1)

• validate_accounts_access (25-44)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Run cargo test

🔇 Additional comments (1)

src/transaction_processor.rs (1)

1698-1724: Updated test constructors align with the new TransactionBatchProcessor::new signature

All internal tests that construct a TransactionBatchProcessor now pass true for enforce_access_permissions:

• test_replenish_program_cache_with_nonexistent_accounts
• test_replenish_program_cache
• test_add_builtin

This preserves their original behavior and keeps unit tests exercising the strict mode by default. No issues here.

Also applies to: 2209-2213

[bmuddha]

LGTM in general, but do you think it's worth it, to introduce extra logic into the SVM crate (which is primarily for tests and dev-tools), which could have been bypassed outside of the SVM. My main concern is that when we upgrade the SVM to 4.0, all of that extra code needs to be manually ported, and I'd like to keep the modified surface as small as possible.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/transaction_processor.rs (1)

195-203: 🧹 Nitpick | 🔵 Trivial

Consider including enforce_access_permissions in Debug output.

The enforce_access_permissions flag significantly affects transaction processing behavior. Including it in the Debug output would help with troubleshooting when the processor is in relaxed mode.

♻️ Proposed enhancement to Debug impl

     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         f.debug_struct("TransactionBatchProcessor")
             .field("slot", &self.slot)
             .field("epoch", &self.epoch)
             .field("sysvar_cache", &self.sysvar_cache)
             .field("program_cache", &self.program_cache)
+            .field("enforce_access_permissions", &self.enforce_access_permissions)
             .finish()
     }

🤖 Fix all issues with AI agents

In `@src/transaction_processor.rs`:
- Around line 2880-2888: The variable name processor_strict is misleading
because TransactionBatchProcessor::new is called with
enforce_access_permissions: false; rename processor_strict to a clearer name
like processor_permissive or processor_relaxed and update all uses accordingly
(e.g., where processor_strict is passed to methods or referenced later). Locate
the instantiation of TransactionBatchProcessor<TestForkGraph> and change the
identifier, and then update any subsequent references so the new name compiles
and clearly reflects the relaxed permission behavior.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/transaction_processor.rs`:
- Around line 664-668: The error counter error_counters.invalid_account_for_fee
is incremented unconditionally before using initial_loaded.ok_or(...),
corrupting metrics; change the logic in the loaded_fee_payer assignment so the
counter is only incremented when initial_loaded is None and you are about to
return Err(TransactionError::InvalidAccountForFee). For example, replace the
current inline increment + ok_or with either a match on initial_loaded that
increments the counter in the None arm and returns the Err, or use
initial_loaded.ok_or_else(|| { error_counters.invalid_account_for_fee += 1;
TransactionError::InvalidAccountForFee }) so the counter runs only on the error
path while keeping the enforce_access_permissions check and the loaded_fee_payer
binding intact.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/transaction_processor.rs (1)

195-203: 🧹 Nitpick | 🔵 Trivial

Include enforce_access_permissions in Debug output.

The new enforce_access_permissions field significantly affects transaction processing behavior but is not included in the Debug implementation. This could make debugging harder when trying to understand why certain transactions succeed or fail.

♻️ Proposed fix

 impl<FG: ForkGraph> Debug for TransactionBatchProcessor<FG> {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         f.debug_struct("TransactionBatchProcessor")
             .field("slot", &self.slot)
             .field("epoch", &self.epoch)
             .field("sysvar_cache", &self.sysvar_cache)
             .field("program_cache", &self.program_cache)
+            .field("enforce_access_permissions", &self.enforce_access_permissions)
             .finish()
     }
 }