fix: use committed account size for buffer allocation to handle >10KB growth

[vikions]

Description

Fixes #878

This PR addresses the issue where CommitDiff tasks would fail when account growth exceeded Solana's 10KB reallocation limit per instruction.

Problem

Previously, the CommitDiff flow used the diff size (diff.len()) to calculate buffer allocation size. When an account grew from 5KB to 16KB (11KB growth), the diff might only be 100 bytes, causing the realloc logic to skip generating multiple realloc instructions. This led to runtime failures when the commit instruction tried to reallocate more than 10KB at once.

Solution

• Added buffer_account_size: u64 field to PreparationTask to track the actual committed account size separately from the diff data
• Updated CommitDiff case to use committed_account.account.data.len() for buffer allocation while keeping diff.len() for write instructions
• Modified init_instruction() and realloc_instructions() to use the new buffer_account_size field

This ensures that when (committed_account_datalen - base_account_datalen) > 10KB, the existing realloc splitting logic in realloc_buffer.rs correctly generates multiple realloc instructions before the commit instruction.

Changes

• magicblock-committor-service/src/tasks/mod.rs: Added buffer_account_size field to PreparationTask
• magicblock-committor-service/src/tasks/buffer_task.rs: Updated CommitDiff and Commit cases to properly set buffer size

Testing

The existing realloc infrastructure (MAX_ACCOUNT_ALLOC_PER_INSTRUCTION_SIZE = 10,240) will now correctly handle large account growth scenarios.

Summary by CodeRabbit

• Refactor

  • Improved buffer-size tracking for account initialization and reallocation to prevent sizing errors during commits.

• Bug Fixes

  • Reject mutations on delegated accounts unless undelegating; reject updates with older remote_slot values (new runtime validations and error responses).
  • Preserve local delegation state and improve subscription/airdrop slot handling for delegated accounts.
  • Truncation now logs delete failures instead of aborting.

• Tests

  • Added coverage for delegation, remote_slot, realloc, subscription, and related behaviors.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR adds a new public field buffer_account_size: u64 to PreparationTask and populates it in both BufferTask::preparation_required branches (Commit and CommitDiff). Preparatory logic (init and realloc instruction construction) now uses buffer_account_size instead of deriving sizes from diff data. CommitDiff preparation computes and forwards the committed account size separately to enable correct realloc reasoning. Several tests were added to cover buffer sizing and realloc behavior.

Assessment against linked issues

Objective	Addressed	Explanation
Detect when data growth exceeds Solana's ~10KB realloc limit and issue dedicated realloc instruction(s) before commit [#878]	❓	The changes add tracking of existing buffer/account size and tests for splitting reallocs, but there is no explicit detection of the ~10KB threshold documented in the diffs or an explicit constant/logic referencing the 10KB Solana realloc limit.

Assessment against linked issues: Out-of-scope changes

Code Change (file_path)	Explanation
Early delegated-account unsubscribe/guarding and airdrop remote_slot handling (magicblock-chainlink/src/chainlink/fetch_cloner/mod.rs)	Delegation/subscription and airdrop remote_slot logic are unrelated to realloc splitting/reallocation objectives.
New fetch_cloner tests around delegation, undelegation, eATA, and airdrop behavior (magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs)	These tests exercise delegation/airdrop behavior, not realloc detection or pre-commit realloc generation.
Added new error variants for delegation and remote_slot ordering (programs/magicblock/src/errors.rs)	New program error variants pertain to delegation and remote_slot validation, unrelated to realloc requirements.
Runtime validation for delegated/undelegating and remote_slot staleness (programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs)	Mutation-time delegation and remote_slot checks change account mutation behavior and do not implement realloc threshold detection or realloc splitting.

Suggested reviewers

• thlorenz
• Dodecahedr0x
• bmuddha

✨ Finishing Touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

🤖 Fix all issues with AI agents

In `@magicblock-committor-service/src/tasks/buffer_task.rs`:
- Around line 64-70: The code unnecessarily clones `data` when building
PreparationState::Required; instead compute let buffer_account_size = data.len()
as u64 before constructing the PreparationTask and then move `data` into the
committed_data field (committed_data: data) without cloning; update the
PreparationTask construction in the PreparationState::Required branch so it uses
the precomputed buffer_account_size and the moved `data` value, keeping
commit_id and pubkey the same.

[snawaz]

@vikions... is this a complete work, or are there more changes coming? Please add some tests as well.

I converted this into draft mode as the PR does not seem complete to me.

[vikions]

@vikions... is this a complete work, or are there more changes coming? Please add some tests as well.

I converted this into draft mode as the PR does not seem complete to me.

I’ll add tests for the >10KB growth case!
or are there any other problems?

[coderabbitai]

Actionable comments posted: 4

Caution

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

⚠️ Outside diff range comments (1)

magicblock-ledger/src/ledger_truncator.rs (1)

283-298: ⚠️ Potential issue | 🟡 Minor

Silent error on delete_slots after set_lowest_cleanup_slot has already advanced.

set_lowest_cleanup_slot(to_slot) runs at line 283 before delete_slots. If delete_slots now fails silently (line 284–286), the cleanup pointer has permanently advanced past [from_slot, to_slot] without tombstones being inserted. Future truncation cycles will not revisit those slots, so unless the CompactionFilter independently cleans data up to lowest_cleanup_slot (without tombstones), those slots silently accumulate.

There is also a consistency gap with truncate_fat_ledger (line 141), which still propagates the delete_slots error via ?. Both paths call the same function with the same risk profile; the diverging error semantics is surprising.

Consider either:

• Moving set_lowest_cleanup_slot to after a successful delete_slots, or
• Aligning truncate_fat_ledger to the same best-effort pattern and documenting the assumption that the CompactionFilter covers cleanup without tombstones.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@magicblock-ledger/src/ledger_truncator.rs` around lines 283 - 298, The code
advances the cleanup pointer via ledger.set_lowest_cleanup_slot(to_slot) before
calling Self::delete_slots, so if delete_slots fails the pointer is moved and
slots may never get tombstones; fix by either moving the
ledger.set_lowest_cleanup_slot(to_slot) call to after a successful
Self::delete_slots(ledger, from_slot, to_slot) (so only advance when deletion
succeeded), or change this path to propagate the delete_slots error (make this
function return Err like truncate_fat_ledger does) so both paths have consistent
semantics; reference functions: set_lowest_cleanup_slot, delete_slots,
truncate_fat_ledger, and ensure any ledger.flush/compact_slot_range behavior
remains correct after reordering.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs`:
- Around line 2327-2344: The test currently polls until eata_pubkey appears and
then immediately inspects ata_pubkey, which can race with the projection
decision; after the tokio::time::timeout(...).await.expect(...) block add a
short stable-state guard (e.g., await tokio::task::yield_now() or an additional
small poll that confirms ATA projection work has run) before reading ata_pubkey
to ensure any pending async projection/handler tasks complete; update the test
function around the timeout block referencing eata_pubkey and ata_pubkey (the
timeout/poll loop) to include this yield/poll to avoid false positives.

In `@magicblock-committor-service/src/tasks/mod.rs`:
- Around line 458-500: Add a new test alongside
test_commit_diff_preparation_tracks_committed_account_size that constructs a
BufferTask::new_preparation_required with a CommitDiffTask where committed_len -
base_len > MAX_ACCOUNT_ALLOC_PER_INSTRUCTION_SIZE (10_240) so the code path that
splits reallocs is exercised; after extracting the
PreparationState::Required(preparation_task) assert that
preparation_task.buffer_account_size equals committed_len as u64 and also assert
preparation_task.realloc_instructions.len() > 1 to confirm realloc splitting
behavior (use the same CommitDiffTask/CommittedAccount/base Account construction
pattern from the existing test to locate the setup).
- Around line 503-523: The test
test_realloc_instructions_use_buffer_account_size_not_diff_size currently uses
buffer_account_size = 12_288 which only yields one realloc instruction and
doesn't exercise the multi-instruction split; update the test to use a larger
buffer_account_size (e.g., 20_480 or higher) so
PreparationTask.realloc_instructions(&authority) produces 2+ instructions and
assert on the expected count, or add an additional assertion comparing a smaller
buffer to the larger one (e.g., verify buffer_account_size = 12_288 yields 1
instruction while buffer_account_size = 20_480 yields 2) to validate the
multi-instruction behavior.

In `@programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs`:
- Around line 818-903: Add a new unit test that mirrors the existing remote-slot
tests but verifies an advancing update is accepted: create an account with
remote_slot 100 (like in test_mod_remote_slot_allows_equal_update), build an
Instruction via InstructionUtils::modify_accounts_instruction with
AccountModification.remote_slot set to 101, run process_instruction (expect
Ok(())), and assert the returned modified account has lamports updated and
remote_slot == 101; refer to the existing test names
(test_mod_remote_slot_rejects_stale_update,
test_mod_remote_slot_allows_equal_update), process_instruction, and
AccountModification to match setup/collection logic.

---

Outside diff comments:
In `@magicblock-ledger/src/ledger_truncator.rs`:
- Around line 283-298: The code advances the cleanup pointer via
ledger.set_lowest_cleanup_slot(to_slot) before calling Self::delete_slots, so if
delete_slots fails the pointer is moved and slots may never get tombstones; fix
by either moving the ledger.set_lowest_cleanup_slot(to_slot) call to after a
successful Self::delete_slots(ledger, from_slot, to_slot) (so only advance when
deletion succeeded), or change this path to propagate the delete_slots error
(make this function return Err like truncate_fat_ledger does) so both paths have
consistent semantics; reference functions: set_lowest_cleanup_slot,
delete_slots, truncate_fat_ledger, and ensure any
ledger.flush/compact_slot_range behavior remains correct after reordering.

[coderabbitai]

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 `@magicblock-committor-service/src/tasks/mod.rs`:
- Around line 457-581: Remove the redundant comparison assertion in
test_realloc_instructions_use_buffer_account_size_not_diff_size: the final
assert!(large_realloc_instructions.len() > small_realloc_instructions.len()) is
tautological given the preceding assert_eq! checks. Edit the test (function
test_realloc_instructions_use_buffer_account_size_not_diff_size) and delete that
last assert referencing large_realloc_instructions and
small_realloc_instructions (leave the two assert_eq! checks and the helper
make_preparation_task unchanged).

[vikions]

@vikions... is this a complete work, or are there more changes coming? Please add some tests as well.

I converted this into draft mode as the PR does not seem complete to me.

Tests are added covering the >10KB growth scenario. Could you please approve the CI workflows to run?