fix: check for diff before cow

[bmuddha]

This avoids having the is_dirty flag set when no actual changes were applied.
This is critical to check that a non-delegated account was not touched.

Summary by CodeRabbit

• Refactor
  • Improved internal account-handling to skip unnecessary copying and mutations when incoming values match existing data. This reduces compute work, memory churn, and unnecessary state changes, yielding more efficient and faster account inspections and updates.

[coderabbitai]

Walkthrough

Added early-exit guards to multiple AccountSharedData mutation methods in the borrowed variant to prevent unnecessary copy-on-write operations and data updates when incoming values match existing ones.

Changes

Cohort / File(s)	Summary
Early-exit guards for mutation methods src/lib.rs	Added early-return conditions in set_lamports, set_owner, copy_into_owner_from_slice, set_executable, set_data_from_slice, set_data, and the create borrowed path to skip copy-on-write and mutations when new values match existing ones.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Caller
    participant AccountSharedData

    Caller->>AccountSharedData: mutation_method(new_value)
    Note over AccountSharedData: Compare new_value with current_value
    alt values equal
        AccountSharedData-->>Caller: return (no-op)
    else values differ
        rect rgb(230, 248, 255)
            AccountSharedData->>AccountSharedData: perform copy-on-write (if Borrowed)
            AccountSharedData->>AccountSharedData: update internal state / mutate data
        end
        AccountSharedData-->>Caller: return (updated)
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Pay attention to correctness of equality checks for owner, lamports, exec flag index, and data slice comparisons.
• Verify borrowed vs owned code paths and that unnecessary cow-ing is fully avoided without changing semantics.
• Check edge cases: zero-length slices, differing lengths, and concurrent mutation scenarios.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: check for diff before cow' directly describes the main change: adding early-exit checks to compare values before performing copy-on-write operations.

✨ 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 bmuddha/fix/pre-cow-check

---

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

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e19b492 and 35947c7.

📒 Files selected for processing (1)

• src/lib.rs (6 hunks)

🔇 Additional comments (8)

src/lib.rs (8)

304-306: LGTM - Efficient early exit for unchanged lamports.

The equality check correctly prevents unnecessary COW operations when the lamports value is already set to the desired value.

---

335-337: LGTM - Efficient guard for unchanged owner bytes.

The comparison correctly checks if the owner's byte array already matches the source, avoiding unnecessary COW and copy operations.

---

348-355: LGTM - Proper early exit with maintained safety.

The executable flag check correctly prevents unnecessary COW operations, and the unsafe block is properly scoped around the cow() call.

---

683-696: LGTM - Consistent optimization across both account variants.

Both Owned and Borrowed variants now properly check if the delegated flag already has the desired value before performing mutations, avoiding unnecessary work.

---

710-724: LGTM - Consistent flag optimization pattern.

The compressed flag optimization follows the same correct pattern as other flag setters, preventing unnecessary mutations when the value is unchanged.

---

737-751: LGTM - Consistent flag setter optimization.

The undelegating flag setter correctly implements the early-exit pattern used across all flag setters in this PR.

---

869-871: LGTM - Appropriate optimization for data mutation.

The equality check prevents unnecessary COW operations when data is unchanged. While the slice comparison has O(n) cost, it's still more efficient than COW + copy when the data is actually the same, which aligns with this PR's goal of avoiding unnecessary modifications.

---

924-926: LGTM - Consistent data comparison optimization.

The early exit when data is unchanged follows the same pattern as set_data_from_slice and correctly prevents unnecessary COW operations.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Verify the impact of early return on owner_changed flag.

The early return at lines 321-323 prevents updating the owner_changed flag at line 325. If owner_changed was previously true and set_owner is called with the current owner value, the flag will remain true instead of being set to false. Confirm whether this behavior is intentional or if the flag should be explicitly cleared when the owner matches.

Run the following script to examine how owner_changed is used:

---

🏁 Script executed:

#!/bin/bash
# Search for usages of owner_changed to understand its semantics
rg -nP -C5 'owner_changed' --type rust

Length of output: 2699

---

🏁 Script executed:

#!/bin/bash
# Find all places where owner_changed is READ or CHECKED (not just assigned)
rg -nP 'owner_changed\s*[!=|&]|if.*owner_changed|match.*owner_changed|\borner_changed\s*[,\)]' --type rust -C3

Length of output: 336

---

🏁 Script executed:

#!/bin/bash
# Search for serialization or validation logic that might use owner_changed
rg -nP '(serialize|write|persist|validate|check)' --type rust -C2 | grep -A10 -B10 'owner_changed'

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Simple grep for all owner_changed references to ensure nothing missed
grep -rn 'owner_changed' src/ --include='*.rs' | head -30

Length of output: 299

---

🏁 Script executed:

#!/bin/bash
# Check if owner_changed is part of public API or exported fields
rg -nP 'pub.*owner_changed|owner_changed.*pub' --type rust -B2 -A2

Length of output: 305

---

🏁 Script executed:

#!/bin/bash
# Search for any conditional logic or pattern matching that might read the flag
rg -nP '\\.owner_changed' --type rust -C4

Length of output: 0

---

Fix early return logic to properly clear the owner_changed flag.

The early return at lines 321-323 prevents updating owner_changed at line 325. This leaves the flag in an inconsistent state: if set_owner is called with the current owner value when the flag was previously true, it remains true instead of being cleared to false. This violates the flag's semantic meaning ("has the owner been changed") and creates a state inconsistency.

Move the owner_changed assignment before the early return or update it within the early return branch:

if *acc.owner == owner {
    acc.owner_changed = false;  // Explicitly clear when owner is unchanged
    return;
}
acc.cow();
acc.owner_changed = true;  // Simplified since we only reach here when changed
*acc.owner = owner;

🤖 Prompt for AI Agents

In src/lib.rs around lines 321 to 326, the early return when the new owner
equals the current owner prevents updating the owner_changed flag, leaving it
stale; update the logic so owner_changed is cleared when owner is unchanged and
set when changed—either set acc.owner_changed = false inside the early-return
branch before returning, or move the owner_changed assignment before the return
and set it to the appropriate boolean, then proceed with acc.cow() and assigning
*acc.owner = owner only when changed.