fix: use committed account size for buffer allocation to handle >10KB growth#943
Open
vikions wants to merge 7 commits intomagicblock-labs:masterfrom
Open
fix: use committed account size for buffer allocation to handle >10KB growth#943vikions wants to merge 7 commits intomagicblock-labs:masterfrom
vikions wants to merge 7 commits intomagicblock-labs:masterfrom
Conversation
…Add buffer_account_size field to PreparationTask- Use committed account size instead of diff size for realloc calculations- Ensures realloc instructions are generated when account growth > 10KB- Fixes magicblock-labs#878
Contributor
📝 WalkthroughWalkthroughThe PR adds a new public field Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
✨ 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 |
![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: 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.
Contributor
|
@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. |
Author
I’ll add tests for the >10KB growth case! |
magicblock-labs#924) Co-authored-by: Gabriele Picco <piccogabriele@gmail.com>
Contributor
There was a problem hiding this comment.
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 | 🟡 MinorSilent error on
delete_slotsafterset_lowest_cleanup_slothas already advanced.
set_lowest_cleanup_slot(to_slot)runs at line 283 beforedelete_slots. Ifdelete_slotsnow 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 theCompactionFilterindependently cleans data up tolowest_cleanup_slot(without tombstones), those slots silently accumulate.There is also a consistency gap with
truncate_fat_ledger(line 141), which still propagates thedelete_slotserror via?. Both paths call the same function with the same risk profile; the diverging error semantics is surprising.Consider either:
- Moving
set_lowest_cleanup_slotto after a successfuldelete_slots, or- Aligning
truncate_fat_ledgerto the same best-effort pattern and documenting the assumption that theCompactionFiltercovers 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.
Comment on lines
+2327
to
+2344
| tokio::time::timeout(TIMEOUT, async { | ||
| while accounts_bank.get_account(&eata_pubkey).is_none() { | ||
| tokio::time::sleep(POLL_INTERVAL).await; | ||
| } | ||
| }) | ||
| .await | ||
| .expect("timed out waiting for delegated eATA subscription update"); | ||
|
|
||
| let ata_after = accounts_bank | ||
| .get_account(&ata_pubkey) | ||
| .expect("ATA should still exist in bank"); | ||
| assert!(ata_after.delegated(), "ATA must remain delegated"); | ||
| assert_eq!( | ||
| ata_after.remote_slot(), | ||
| CURRENT_SLOT - 1, | ||
| "Delegated ATA should not be overwritten by chain update", | ||
| ); | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: poll condition waits for eATA only — ATA-projection decision may not be settled yet.
The test polls until eata_pubkey appears in the bank and then immediately reads the ATA. If the subscription handler processes the eATA clone and the ATA-projection-skip in separate async steps (with a yield between them), there is a brief window where the eATA is visible in the bank but the handler hasn't yet decided whether to overwrite the ATA. This means the test could produce a false-positive if a bug causes the ATA overwrite to happen after this check.
Existing test test_delegated_eata_subscription_update_clones_raw_eata_and_projects_ata (line 2224) polls for has_eata && has_ata precisely to bridge this gap. Consider adding a small stable-state guard, or simply adding a tokio::task::yield_now().await after the timeout block to let any pending projection task run before asserting the ATA state.
💡 Proposed guard (minimal change)
})
.await
.expect("timed out waiting for delegated eATA subscription update");
+// Yield to let any pending ATA-projection step complete before asserting.
+tokio::task::yield_now().await;
let ata_after = accounts_bank🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-chainlink/src/chainlink/fetch_cloner/tests.rs` around lines 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.
Comment on lines
+818
to
+903
| #[test] | ||
| fn test_mod_remote_slot_rejects_stale_update() { | ||
| init_logger!(); | ||
|
|
||
| let mod_key = Pubkey::new_unique(); | ||
| let mut account = AccountSharedData::new(100, 0, &mod_key); | ||
| account.set_remote_slot(100); | ||
| let mut account_data = { | ||
| let mut map = HashMap::new(); | ||
| map.insert(mod_key, account); | ||
| map | ||
| }; | ||
| ensure_started_validator(&mut account_data); | ||
|
|
||
| let ix = InstructionUtils::modify_accounts_instruction( | ||
| vec![AccountModification { | ||
| pubkey: mod_key, | ||
| lamports: Some(200), | ||
| remote_slot: Some(99), | ||
| ..Default::default() | ||
| }], | ||
| None, | ||
| ); | ||
| let transaction_accounts = ix | ||
| .accounts | ||
| .iter() | ||
| .flat_map(|acc| { | ||
| account_data | ||
| .remove(&acc.pubkey) | ||
| .map(|shared_data| (acc.pubkey, shared_data)) | ||
| }) | ||
| .collect(); | ||
|
|
||
| let _accounts = process_instruction( | ||
| ix.data.as_slice(), | ||
| transaction_accounts, | ||
| ix.accounts, | ||
| Err(MagicBlockProgramError::IncomingRemoteSlotIsOlderThanCurrentRemoteSlot.into()), | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_mod_remote_slot_allows_equal_update() { | ||
| init_logger!(); | ||
|
|
||
| let mod_key = Pubkey::new_unique(); | ||
| let mut account = AccountSharedData::new(100, 0, &mod_key); | ||
| account.set_remote_slot(100); | ||
| let mut account_data = { | ||
| let mut map = HashMap::new(); | ||
| map.insert(mod_key, account); | ||
| map | ||
| }; | ||
| ensure_started_validator(&mut account_data); | ||
|
|
||
| let ix = InstructionUtils::modify_accounts_instruction( | ||
| vec![AccountModification { | ||
| pubkey: mod_key, | ||
| lamports: Some(200), | ||
| remote_slot: Some(100), | ||
| ..Default::default() | ||
| }], | ||
| None, | ||
| ); | ||
| let transaction_accounts = ix | ||
| .accounts | ||
| .iter() | ||
| .flat_map(|acc| { | ||
| account_data | ||
| .remove(&acc.pubkey) | ||
| .map(|shared_data| (acc.pubkey, shared_data)) | ||
| }) | ||
| .collect(); | ||
|
|
||
| let mut accounts = process_instruction( | ||
| ix.data.as_slice(), | ||
| transaction_accounts, | ||
| ix.accounts, | ||
| Ok(()), | ||
| ); | ||
|
|
||
| let _account_authority = accounts.drain(0..1).next().unwrap(); | ||
| let modified_account = accounts.drain(0..1).next().unwrap(); | ||
| assert_eq!(modified_account.lamports(), 200); | ||
| assert_eq!(modified_account.remote_slot(), 100); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remote slot edge-case tests are thorough.
Stale rejection (slot 99 < 100) and equal-slot acceptance (slot 100 == 100) are both tested. Consider adding a test for the straightforward advancing case (e.g., incoming slot 101 > current slot 100) for completeness — though this is implicitly covered by test_mod_remote_slot which sets a remote_slot on a fresh account (slot 0).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs` around
lines 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.
Contributor
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 `@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).
Comment on lines
+457
to
+581
| #[test] | ||
| fn test_commit_diff_preparation_tracks_committed_account_size() { | ||
| setup(); | ||
| let base_len = 11_264usize; | ||
| let committed_len = 12_288usize; | ||
| let owner = Pubkey::new_unique(); | ||
|
|
||
| let base_data = vec![1u8; base_len]; | ||
| let mut committed_data = base_data.clone(); | ||
| committed_data.extend(vec![2u8; committed_len - base_len]); | ||
|
|
||
| let buffer_task = BufferTask::new_preparation_required( | ||
| BufferTaskType::CommitDiff(CommitDiffTask { | ||
| commit_id: 900, | ||
| allow_undelegation: false, | ||
| committed_account: CommittedAccount { | ||
| pubkey: Pubkey::new_unique(), | ||
| account: Account { | ||
| lamports: 4000, | ||
| data: committed_data, | ||
| owner, | ||
| executable: false, | ||
| rent_epoch: 0, | ||
| }, | ||
| remote_slot: Default::default(), | ||
| }, | ||
| base_account: Account { | ||
| lamports: 2000, | ||
| data: base_data, | ||
| owner, | ||
| executable: false, | ||
| rent_epoch: 0, | ||
| }, | ||
| }), | ||
| ); | ||
|
|
||
| let PreparationState::Required(preparation_task) = | ||
| buffer_task.preparation_state() | ||
| else { | ||
| panic!("invalid preparation state on creation!"); | ||
| }; | ||
|
|
||
| assert_eq!(preparation_task.buffer_account_size, committed_len as u64); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_commit_diff_preparation_large_growth_splits_reallocs() { | ||
| setup(); | ||
| let authority = Pubkey::new_unique(); | ||
| let base_len = 8_192usize; | ||
| let committed_len = 22_528usize; | ||
| let owner = Pubkey::new_unique(); | ||
|
|
||
| assert!( | ||
| committed_len - base_len | ||
| > magicblock_committor_program::consts::MAX_ACCOUNT_ALLOC_PER_INSTRUCTION_SIZE as usize | ||
| ); | ||
|
|
||
| let base_data = vec![1u8; base_len]; | ||
| let mut committed_data = base_data.clone(); | ||
| committed_data.extend(vec![2u8; committed_len - base_len]); | ||
|
|
||
| let buffer_task = BufferTask::new_preparation_required( | ||
| BufferTaskType::CommitDiff(CommitDiffTask { | ||
| commit_id: 902, | ||
| allow_undelegation: false, | ||
| committed_account: CommittedAccount { | ||
| pubkey: Pubkey::new_unique(), | ||
| account: Account { | ||
| lamports: 5000, | ||
| data: committed_data, | ||
| owner, | ||
| executable: false, | ||
| rent_epoch: 0, | ||
| }, | ||
| remote_slot: Default::default(), | ||
| }, | ||
| base_account: Account { | ||
| lamports: 2500, | ||
| data: base_data, | ||
| owner, | ||
| executable: false, | ||
| rent_epoch: 0, | ||
| }, | ||
| }), | ||
| ); | ||
|
|
||
| let PreparationState::Required(preparation_task) = | ||
| buffer_task.preparation_state() | ||
| else { | ||
| panic!("invalid preparation state on creation!"); | ||
| }; | ||
|
|
||
| assert_eq!(preparation_task.buffer_account_size, committed_len as u64); | ||
| assert!(preparation_task.realloc_instructions(&authority).len() > 1); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_realloc_instructions_use_buffer_account_size_not_diff_size() { | ||
| setup(); | ||
| let authority = Pubkey::new_unique(); | ||
| let committed_data = vec![9u8; 64]; | ||
| let make_preparation_task = | ||
| |buffer_account_size: u64| PreparationTask { | ||
| commit_id: 901, | ||
| pubkey: Pubkey::new_unique(), | ||
| chunks: Chunks::from_data_length( | ||
| committed_data.len(), | ||
| crate::consts::MAX_WRITE_CHUNK_SIZE, | ||
| ), | ||
| committed_data: committed_data.clone(), | ||
| buffer_account_size, | ||
| }; | ||
|
|
||
| let small_realloc_instructions = | ||
| make_preparation_task(12_288).realloc_instructions(&authority); | ||
| let large_realloc_instructions = | ||
| make_preparation_task(30_720).realloc_instructions(&authority); | ||
|
|
||
| assert_eq!(small_realloc_instructions.len(), 1); | ||
| assert_eq!(large_realloc_instructions.len(), 2); | ||
| assert!( | ||
| large_realloc_instructions.len() > small_realloc_instructions.len() | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tests adequately cover the fix; one redundant assertion is a nitpick.
The three tests cover orthogonal properties: field-assignment, multi-realloc splitting with large growth, and the buffer-size-vs-diff-size distinction. The math holds for all assertions. Past review concerns about insufficient coverage are fully addressed.
The assertion at lines 578–580 is redundant given the assert_eq! calls immediately above it (if len == 1 and len == 2, 2 > 1 is tautological).
🧹 Optional: remove redundant assertion
assert_eq!(small_realloc_instructions.len(), 1);
assert_eq!(large_realloc_instructions.len(), 2);
- assert!(
- large_realloc_instructions.len() > small_realloc_instructions.len()
- );📝 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
| #[test] | |
| fn test_commit_diff_preparation_tracks_committed_account_size() { | |
| setup(); | |
| let base_len = 11_264usize; | |
| let committed_len = 12_288usize; | |
| let owner = Pubkey::new_unique(); | |
| let base_data = vec![1u8; base_len]; | |
| let mut committed_data = base_data.clone(); | |
| committed_data.extend(vec![2u8; committed_len - base_len]); | |
| let buffer_task = BufferTask::new_preparation_required( | |
| BufferTaskType::CommitDiff(CommitDiffTask { | |
| commit_id: 900, | |
| allow_undelegation: false, | |
| committed_account: CommittedAccount { | |
| pubkey: Pubkey::new_unique(), | |
| account: Account { | |
| lamports: 4000, | |
| data: committed_data, | |
| owner, | |
| executable: false, | |
| rent_epoch: 0, | |
| }, | |
| remote_slot: Default::default(), | |
| }, | |
| base_account: Account { | |
| lamports: 2000, | |
| data: base_data, | |
| owner, | |
| executable: false, | |
| rent_epoch: 0, | |
| }, | |
| }), | |
| ); | |
| let PreparationState::Required(preparation_task) = | |
| buffer_task.preparation_state() | |
| else { | |
| panic!("invalid preparation state on creation!"); | |
| }; | |
| assert_eq!(preparation_task.buffer_account_size, committed_len as u64); | |
| } | |
| #[test] | |
| fn test_commit_diff_preparation_large_growth_splits_reallocs() { | |
| setup(); | |
| let authority = Pubkey::new_unique(); | |
| let base_len = 8_192usize; | |
| let committed_len = 22_528usize; | |
| let owner = Pubkey::new_unique(); | |
| assert!( | |
| committed_len - base_len | |
| > magicblock_committor_program::consts::MAX_ACCOUNT_ALLOC_PER_INSTRUCTION_SIZE as usize | |
| ); | |
| let base_data = vec![1u8; base_len]; | |
| let mut committed_data = base_data.clone(); | |
| committed_data.extend(vec![2u8; committed_len - base_len]); | |
| let buffer_task = BufferTask::new_preparation_required( | |
| BufferTaskType::CommitDiff(CommitDiffTask { | |
| commit_id: 902, | |
| allow_undelegation: false, | |
| committed_account: CommittedAccount { | |
| pubkey: Pubkey::new_unique(), | |
| account: Account { | |
| lamports: 5000, | |
| data: committed_data, | |
| owner, | |
| executable: false, | |
| rent_epoch: 0, | |
| }, | |
| remote_slot: Default::default(), | |
| }, | |
| base_account: Account { | |
| lamports: 2500, | |
| data: base_data, | |
| owner, | |
| executable: false, | |
| rent_epoch: 0, | |
| }, | |
| }), | |
| ); | |
| let PreparationState::Required(preparation_task) = | |
| buffer_task.preparation_state() | |
| else { | |
| panic!("invalid preparation state on creation!"); | |
| }; | |
| assert_eq!(preparation_task.buffer_account_size, committed_len as u64); | |
| assert!(preparation_task.realloc_instructions(&authority).len() > 1); | |
| } | |
| #[test] | |
| fn test_realloc_instructions_use_buffer_account_size_not_diff_size() { | |
| setup(); | |
| let authority = Pubkey::new_unique(); | |
| let committed_data = vec![9u8; 64]; | |
| let make_preparation_task = | |
| |buffer_account_size: u64| PreparationTask { | |
| commit_id: 901, | |
| pubkey: Pubkey::new_unique(), | |
| chunks: Chunks::from_data_length( | |
| committed_data.len(), | |
| crate::consts::MAX_WRITE_CHUNK_SIZE, | |
| ), | |
| committed_data: committed_data.clone(), | |
| buffer_account_size, | |
| }; | |
| let small_realloc_instructions = | |
| make_preparation_task(12_288).realloc_instructions(&authority); | |
| let large_realloc_instructions = | |
| make_preparation_task(30_720).realloc_instructions(&authority); | |
| assert_eq!(small_realloc_instructions.len(), 1); | |
| assert_eq!(large_realloc_instructions.len(), 2); | |
| assert!( | |
| large_realloc_instructions.len() > small_realloc_instructions.len() | |
| ); | |
| } | |
| #[test] | |
| fn test_commit_diff_preparation_tracks_committed_account_size() { | |
| setup(); | |
| let base_len = 11_264usize; | |
| let committed_len = 12_288usize; | |
| let owner = Pubkey::new_unique(); | |
| let base_data = vec![1u8; base_len]; | |
| let mut committed_data = base_data.clone(); | |
| committed_data.extend(vec![2u8; committed_len - base_len]); | |
| let buffer_task = BufferTask::new_preparation_required( | |
| BufferTaskType::CommitDiff(CommitDiffTask { | |
| commit_id: 900, | |
| allow_undelegation: false, | |
| committed_account: CommittedAccount { | |
| pubkey: Pubkey::new_unique(), | |
| account: Account { | |
| lamports: 4000, | |
| data: committed_data, | |
| owner, | |
| executable: false, | |
| rent_epoch: 0, | |
| }, | |
| remote_slot: Default::default(), | |
| }, | |
| base_account: Account { | |
| lamports: 2000, | |
| data: base_data, | |
| owner, | |
| executable: false, | |
| rent_epoch: 0, | |
| }, | |
| }), | |
| ); | |
| let PreparationState::Required(preparation_task) = | |
| buffer_task.preparation_state() | |
| else { | |
| panic!("invalid preparation state on creation!"); | |
| }; | |
| assert_eq!(preparation_task.buffer_account_size, committed_len as u64); | |
| } | |
| #[test] | |
| fn test_commit_diff_preparation_large_growth_splits_reallocs() { | |
| setup(); | |
| let authority = Pubkey::new_unique(); | |
| let base_len = 8_192usize; | |
| let committed_len = 22_528usize; | |
| let owner = Pubkey::new_unique(); | |
| assert!( | |
| committed_len - base_len | |
| > magicblock_committor_program::consts::MAX_ACCOUNT_ALLOC_PER_INSTRUCTION_SIZE as usize | |
| ); | |
| let base_data = vec![1u8; base_len]; | |
| let mut committed_data = base_data.clone(); | |
| committed_data.extend(vec![2u8; committed_len - base_len]); | |
| let buffer_task = BufferTask::new_preparation_required( | |
| BufferTaskType::CommitDiff(CommitDiffTask { | |
| commit_id: 902, | |
| allow_undelegation: false, | |
| committed_account: CommittedAccount { | |
| pubkey: Pubkey::new_unique(), | |
| account: Account { | |
| lamports: 5000, | |
| data: committed_data, | |
| owner, | |
| executable: false, | |
| rent_epoch: 0, | |
| }, | |
| remote_slot: Default::default(), | |
| }, | |
| base_account: Account { | |
| lamports: 2500, | |
| data: base_data, | |
| owner, | |
| executable: false, | |
| rent_epoch: 0, | |
| }, | |
| }), | |
| ); | |
| let PreparationState::Required(preparation_task) = | |
| buffer_task.preparation_state() | |
| else { | |
| panic!("invalid preparation state on creation!"); | |
| }; | |
| assert_eq!(preparation_task.buffer_account_size, committed_len as u64); | |
| assert!(preparation_task.realloc_instructions(&authority).len() > 1); | |
| } | |
| #[test] | |
| fn test_realloc_instructions_use_buffer_account_size_not_diff_size() { | |
| setup(); | |
| let authority = Pubkey::new_unique(); | |
| let committed_data = vec![9u8; 64]; | |
| let make_preparation_task = | |
| |buffer_account_size: u64| PreparationTask { | |
| commit_id: 901, | |
| pubkey: Pubkey::new_unique(), | |
| chunks: Chunks::from_data_length( | |
| committed_data.len(), | |
| crate::consts::MAX_WRITE_CHUNK_SIZE, | |
| ), | |
| committed_data: committed_data.clone(), | |
| buffer_account_size, | |
| }; | |
| let small_realloc_instructions = | |
| make_preparation_task(12_288).realloc_instructions(&authority); | |
| let large_realloc_instructions = | |
| make_preparation_task(30_720).realloc_instructions(&authority); | |
| assert_eq!(small_realloc_instructions.len(), 1); | |
| assert_eq!(large_realloc_instructions.len(), 2); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magicblock-committor-service/src/tasks/mod.rs` around lines 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).
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
4 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.
Description
Fixes #878
This PR addresses the issue where
CommitDifftasks would fail when account growth exceeded Solana's 10KB reallocation limit per instruction.Problem
Previously, the
CommitDiffflow 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
buffer_account_size: u64field toPreparationTaskto track the actual committed account size separately from the diff dataCommitDiffcase to usecommitted_account.account.data.len()for buffer allocation while keepingdiff.len()for write instructionsinit_instruction()andrealloc_instructions()to use the newbuffer_account_sizefieldThis ensures that when
(committed_account_datalen - base_account_datalen) > 10KB, the existing realloc splitting logic inrealloc_buffer.rscorrectly generates multiple realloc instructions before the commit instruction.Changes
magicblock-committor-service/src/tasks/mod.rs: Addedbuffer_account_sizefield toPreparationTaskmagicblock-committor-service/src/tasks/buffer_task.rs: UpdatedCommitDiffandCommitcases to properly set buffer sizeTesting
The existing realloc infrastructure (
MAX_ACCOUNT_ALLOC_PER_INSTRUCTION_SIZE = 10,240) will now correctly handle large account growth scenarios.Summary by CodeRabbit
Refactor
Bug Fixes
Tests