Skip to content

feat: Execute CommitDiff as BufferTask #616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: snawaz/commit-diff
Choose a base branch
from
Open

feat: Execute CommitDiff as BufferTask #616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9ed3f64
feat: Execute CommitDiff as BufferTask
snawaz Nov 4, 2025
1bb9384
Update dlp.so that now has CommitDiffFromBuffer
snawaz Nov 18, 2025
e3ce6e6
Remove is_commit_diff
snawaz Nov 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ magicblock-config-helpers = { path = "./magicblock-config-helpers" }
magicblock-config-macro = { path = "./magicblock-config-macro" }
magicblock-core = { path = "./magicblock-core" }
magicblock-aperture = { path = "./magicblock-aperture" }
magicblock-delegation-program = { git = "https://github.com/magicblock-labs/delegation-program.git", rev = "e8d03936", features = [
"no-entrypoint",
magicblock-delegation-program = { git = "https://github.com/magicblock-labs/delegation-program.git", rev = "ea1f2f916268132248fe8d5de5f07d76765dd937", features = [
"no-entrypoint",
] }
magicblock-geyser-plugin = { path = "./magicblock-geyser-plugin" }
magicblock-ledger = { path = "./magicblock-ledger" }
Expand Down
14 changes: 1 addition & 13 deletions magicblock-committor-service/src/tasks/args_task.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,21 +88,9 @@ impl BaseTask for ArgsTask {
self: Box<Self>,
) -> Result<Box<dyn BaseTask>, Box<dyn BaseTask>> {
match self.task_type {
ArgsTaskType::Commit(mut value) if value.is_commit_diff() => {
// TODO (snawaz): Currently, we do not support executing CommitDiff
// as BufferTask, which is why we're forcing CommitTask to use CommitState
// before converting this task into BufferTask. Once CommitDiff is supported
// by BufferTask, we do not have to force_commit_state and we can remove
// force_commit_state stuff, as it's essentially a downgrade.

value.force_commit_state();
Ok(Box::new(BufferTask::new_preparation_required(
BufferTaskType::Commit(value),
)))
}
ArgsTaskType::Commit(value) => {
Ok(Box::new(BufferTask::new_preparation_required(
BufferTaskType::Commit(value),
BufferTaskType::Commit(value.switch_to_buffer_strategy()),
)))
}
ArgsTaskType::BaseAction(_)
Expand Down
37 changes: 11 additions & 26 deletions magicblock-committor-service/src/tasks/buffer_task.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use dlp::args::CommitStateFromBufferArgs;
use magicblock_committor_program::Chunks;
use solana_pubkey::Pubkey;
use solana_sdk::instruction::Instruction;
Expand Down Expand Up @@ -46,16 +45,18 @@ impl BufferTask {

fn preparation_required(task_type: &BufferTaskType) -> PreparationState {
let BufferTaskType::Commit(ref commit_task) = task_type;
let committed_data = commit_task.committed_account.account.data.clone();
let chunks = Chunks::from_data_length(
committed_data.len(),
MAX_WRITE_CHUNK_SIZE,
);
let state_or_diff = if let Some(diff) = commit_task.compute_diff() {
diff.to_vec()
} else {
commit_task.committed_account.account.data.clone()
};
let chunks =
Chunks::from_data_length(state_or_diff.len(), MAX_WRITE_CHUNK_SIZE);

PreparationState::Required(PreparationTask {
commit_id: commit_task.commit_id,
pubkey: commit_task.committed_account.pubkey,
committed_data,
committed_data: state_or_diff,
chunks,
Comment on lines +48 to 60
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

BufferTask now writes diff-or-state into buffer; ensure CommitTask is always in Buffer strategy when used here

Using commit_task.compute_diff() to decide whether to write a diff or full state into PreparationTask.committed_data, and deriving Chunks from that length, is the right behavior for committing via buffers: buffer contents and chunking will match what *_from_buffer instructions expect.

One subtle requirement, though, is that the embedded CommitTask must have strategy == TaskStrategy::Buffer; otherwise instruction() will still emit the args-based commit instruction and completely ignore the prepared buffer, wasting the prep work and potentially re‑hitting tx-size limits.

Since ArgsTask::optimize already calls value.switch_to_buffer_strategy() before wrapping in BufferTaskType::Commit, this is satisfied for the main flow, but any other construction of BufferTask::Commit (including the tests) currently relies on the caller remembering to switch strategies.

Consider tightening this by either:

  • Having BufferTask::new_preparation_required (or preparation_required) assert in debug builds that the inner CommitTask has TaskStrategy::Buffer, and/or
  • Moving the switch_to_buffer_strategy() call into the BufferTask construction path so callers cannot accidentally pass an Args‑strategy commit task.

That would make BufferTask semantics self-contained and prevent silent misuse.

Also applies to: 65-77

🤖 Prompt for AI Agents 
In magicblock-committor-service/src/tasks/buffer_task.rs around lines 48-60 (and
similarly 65-77), the code accepts a CommitTask but does not guarantee its
TaskStrategy is Buffer, which can cause prepared buffer data to be ignored if
the task remains in Args strategy; update BufferTask construction to enforce
Buffer strategy by either calling commit_task.switch_to_buffer_strategy() before
using it and/or adding a debug_assert!(commit_task.strategy ==
TaskStrategy::Buffer) (or equivalent) inside
new_preparation_required/preparation_required so callers cannot accidentally
pass an Args-strategy commit task and the buffer path will always be used.

})
}
Expand All @@ -64,31 +65,15 @@ impl BufferTask {
impl BaseTask for BufferTask {
fn instruction(&self, validator: &Pubkey) -> Instruction {
let BufferTaskType::Commit(ref value) = self.task_type;
let commit_id_slice = value.commit_id.to_le_bytes();
let (commit_buffer_pubkey, _) =
magicblock_committor_program::pdas::buffer_pda(
validator,
&value.committed_account.pubkey,
&commit_id_slice,
);

dlp::instruction_builder::commit_state_from_buffer(
*validator,
value.committed_account.pubkey,
value.committed_account.account.owner,
commit_buffer_pubkey,
CommitStateFromBufferArgs {
nonce: value.commit_id,
lamports: value.committed_account.account.lamports,
allow_undelegation: value.allow_undelegation,
},
)
value.create_commit_ix(validator)
}

/// No further optimizations
fn optimize(
self: Box<Self>,
) -> Result<Box<dyn BaseTask>, Box<dyn BaseTask>> {
// Since the buffer in BufferTask doesn't contribute to the size of
// transaction, there is nothing we can do here to optimize/reduce the size.
Err(self)
}

Expand Down
109 changes: 86 additions & 23 deletions magicblock-committor-service/src/tasks/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::sync::Arc;

use dlp::{
args::{CommitDiffArgs, CommitStateArgs},
args::{CommitDiffArgs, CommitStateArgs, CommitStateFromBufferArgs},
compute_diff,
};
use dyn_clone::DynClone;
Expand Down Expand Up @@ -52,7 +52,6 @@ pub enum PreparationState {
Cleanup(CleanupTask),
}

#[cfg(test)]
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum TaskStrategy {
Args,
Expand Down Expand Up @@ -152,7 +151,7 @@ impl CommitTaskBuilder {
allow_undelegation,
committed_account,
base_account,
force_commit_state: false,
strategy: TaskStrategy::Args,
}
}
}
Expand All @@ -163,29 +162,46 @@ pub struct CommitTask {
pub allow_undelegation: bool,
pub committed_account: CommittedAccount,
base_account: Option<Account>,
force_commit_state: bool,
strategy: TaskStrategy,
}

impl CommitTask {
pub fn is_commit_diff(&self) -> bool {
!self.force_commit_state
&& self.committed_account.account.data.len()
> CommitTaskBuilder::COMMIT_STATE_SIZE_THRESHOLD
&& self.base_account.is_some()
}

pub fn force_commit_state(&mut self) {
self.force_commit_state = true;
pub fn switch_to_buffer_strategy(mut self) -> Self {
self.strategy = TaskStrategy::Buffer;
self
}

pub fn create_commit_ix(&self, validator: &Pubkey) -> Instruction {
if let Some(fetched_account) = self.base_account.as_ref() {
self.create_commit_diff_ix(validator, fetched_account)
} else {
self.create_commit_state_ix(validator)
match self.strategy {
TaskStrategy::Args => {
if let Some(base_account) = self.base_account.as_ref() {
self.create_commit_diff_ix(validator, base_account)
} else {
self.create_commit_state_ix(validator)
}
}
TaskStrategy::Buffer => {
if let Some(base_account) = self.base_account.as_ref() {
self.create_commit_diff_from_buffer_ix(
validator,
base_account,
)
} else {
self.create_commit_state_from_buffer_ix(validator)
}
}
}
}

pub fn compute_diff(&self) -> Option<dlp::rkyv::AlignedVec> {
self.base_account.as_ref().map(|base_account| {
compute_diff(
base_account.data(),
self.committed_account.account.data(),
)
})
}

Comment on lines +196 to +204
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Reuse compute_diff() where possible to avoid duplicated diff logic (optional)

CommitTask::compute_diff() encapsulates the base-vs-committed diff computation and is already used by BufferTask for preparation. create_commit_diff_ix() still re‑implements the same compute_diff(base_account.data(), committed_account.data()) call inline.

Not a bug, but you could slightly simplify and DRY this by delegating to self.compute_diff() and unwrapping (since this method is only called when base_account is Some), e.g.,

let diff = self
    .compute_diff()
    .expect("base_account must be Some when using CommitDiff");

before constructing CommitDiffArgs. That keeps all diff computation logic in one place and ensures Args and Buffer strategies remain in lockstep if the diffing behavior ever changes.

Also applies to: 220-232

🤖 Prompt for AI Agents 
In magicblock-committor-service/src/tasks/mod.rs around lines 196-204 and
220-232, duplicate diff computation is performed inline; instead call the
existing CommitTask::compute_diff() and unwrap it (since these call-sites only
occur when base_account is Some), e.g. replace the inline
compute_diff(base_account.data(), committed_account.account.data()) usage with
let diff = self.compute_diff().expect("base_account must be Some when using
CommitDiff") and then use diff to construct CommitDiffArgs and other structures;
this delegates all diff logic to the single method to avoid duplication and keep
behaviors in sync.

fn create_commit_state_ix(&self, validator: &Pubkey) -> Instruction {
let args = CommitStateArgs {
nonce: self.commit_id,
Expand All @@ -204,17 +220,13 @@ impl CommitTask {
fn create_commit_diff_ix(
&self,
validator: &Pubkey,
fetched_account: &Account,
base_account: &Account,
) -> Instruction {
if self.force_commit_state {
return self.create_commit_state_ix(validator);
}

let args = CommitDiffArgs {
nonce: self.commit_id,
lamports: self.committed_account.account.lamports,
diff: compute_diff(
fetched_account.data(),
base_account.data(),
self.committed_account.account.data(),
)
.to_vec(),
Comment on lines 220 to 232
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

❓ Verification inconclusive

Buffer-based commit instruction builders are consistent with PreparationTask PDAs but currently untested

The new create_commit_state_from_buffer_ix and create_commit_diff_from_buffer_ix:

  • Derive the buffer PDA with (validator, committed_account.pubkey, commit_id.to_le_bytes()), matching PreparationTask::buffer_pda.
  • Use CommitStateFromBufferArgs { nonce, lamports, allow_undelegation }, with actual state or diff bytes coming solely from the buffer account.

This is structurally sound and lines up with how PreparationTask initializes and writes to the buffer.

One gap is tests: the test_buffer_task_instruction_serialization and related preparation tests currently construct BufferTask from a CommitTaskBuilder::create_commit_task(...) without switching to buffer strategy, so they still serialize the args-based commit instruction and never hit these new *_from_buffer builders. It would be valuable to:

  • Either update those tests to use a CommitTask that has strategy: TaskStrategy::Buffer (via switch_to_buffer_strategy()), or
  • Add dedicated tests that directly exercise create_commit_state_from_buffer_ix and create_commit_diff_from_buffer_ix, asserting serialization and basic structure.

That would give direct coverage of the new buffer commit paths.

Also applies to: 244-268, 269-293

🏁 Script executed:

#!/bin/bash
# Inspect existing tests that mention commit_*from_buffer and BufferTask usage
rg -n "commit_.*from_buffer" magicblock-committor-service
rg -n "BufferTask::new_preparation_required" magicblock-committor-service

Length of output: 1114

I need to locate and inspect the test files to verify the coverage claim. Let me search for tests related to buffer tasks and instruction serialization.


---

🏁 Script executed:

```shell
#!/bin/bash
# Find test files and test functions related to buffer tasks
fd -e rs -path "*test*" magicblock-committor-service | head -20
rg -n "test_buffer_task_instruction_serialization|test.*commit.*instruction" magicblock-committor-service --type=rs

Length of output: 195

Let me fix the command syntax:


---

🏁 Script executed:

```shell
#!/bin/bash
# Find test files with Rust extension in any test-related directories
find magicblock-committor-service -name "*test*.rs" -o -name "tests" -type d

# Search for test functions related to buffer task instruction serialization
rg -n "fn test_.*buffer.*task|fn test_.*commit.*instruction" magicblock-committor-service

Length of output: 109

Now let me inspect the actual test implementation to verify the review's claim:


---

🏁 Script executed:

```shell
#!/bin/bash
# Examine the test_buffer_task_instruction_serialization test
sed -n '563,650p' magicblock-committor-service/src/tasks/mod.rs

Length of output: 3040

Let me inspect the CommitTaskBuilder and BufferTask instruction method to understand the dispatch logic:


---

🏁 Script executed:

```shell
#!/bin/bash
# Find CommitTaskBuilder::create_commit_task implementation
rg -n "impl CommitTaskBuilder|fn create_commit_task" magicblock-committor-service/src/tasks/mod.rs -A 20 | head -50

# Find BufferTask::instruction method
rg -n "fn instruction" magicblock-committor-service/src/tasks/mod.rs -B 2 -A 15 | grep -A 15 "BufferTask"

Length of output: 1417

Let me get more of the create_commit_task implementation and locate the instruction method:


---

🏁 Script executed:

```shell
#!/bin/bash
# Get full create_commit_task implementation
sed -n '120,180p' magicblock-committor-service/src/tasks/mod.rs

# Search for impl BufferTask and instruction method
rg -n "impl BufferTask" magicblock-committor-service/src/tasks/mod.rs -A 100 | grep -E "fn instruction|CommitTask"

Length of output: 2056

Now let me see the complete create_commit_ix match statement and the BufferTask::instruction() method:


---

🏁 Script executed:

```shell
#!/bin/bash
# Get the full create_commit_ix match statement
sed -n '168,210p' magicblock-committor-service/src/tasks/mod.rs

# Find BufferTask::instruction method
rg -n "pub fn instruction" magicblock-committor-service/src/tasks/mod.rs -B 2 -A 25 | head -60

Length of output: 2419

Let me search more specifically for the BufferTask instruction method:


---

🏁 Script executed:

```shell
#!/bin/bash
# Find BufferTask struct and impl blocks
rg -n "struct BufferTask|impl BufferTask" magicblock-committor-service/src/tasks/mod.rs

# Search for instruction method more broadly
rg -n "\.instruction\(&validator\)" magicblock-committor-service/src/tasks/mod.rs -B 5 -A 5

Length of output: 1871

Let me search across all files for BufferTask definition:


---

🏁 Script executed:

```shell
#!/bin/bash
# Search for BufferTask struct definition and impl across all files
rg -n "pub struct BufferTask|impl BufferTask" magicblock-committor-service

# Check args_task.rs since it appeared in earlier search
rg -n "pub fn instruction|pub struct BufferTask" magicblock-committor-service/src/tasks/args_task.rs -A 10 | head -40

Length of output: 155

Now let me examine the BufferTask implementation:


---

🏁 Script executed:

```shell
#!/bin/bash
# Read the buffer_task.rs file to find instruction() method
cat magicblock-committor-service/src/tasks/buffer_task.rs

Length of output: 3557

Add tests for buffer instruction builders to ensure coverage of new create_commit_state_from_buffer_ix and create_commit_diff_from_buffer_ix paths

The test_buffer_task_instruction_serialization test constructs CommitTask via CommitTaskBuilder::create_commit_task(), which defaults to TaskStrategy::Args. Since the test never calls switch_to_buffer_strategy(), the instruction dispatch in CommitTask::create_commit_ix() always takes the Args branch and never exercises the new buffer instruction builders.

To cover these paths, either:

  • Modify test_buffer_task_instruction_serialization to call switch_to_buffer_strategy() on the CommitTask before wrapping it in BufferTask, or
  • Add dedicated tests that directly construct a CommitTask with TaskStrategy::Buffer and verify the buffer instruction builders are called.
🤖 Prompt for AI Agents 
In magicblock-committor-service/src/tasks/mod.rs around lines 220 to 232, the
unit tests never exercise the new buffer-based instruction builders because
CommitTask is constructed with the default TaskStrategy::Args; update tests to
cover the buffer paths by either calling switch_to_buffer_strategy() on the
CommitTask instance inside test_buffer_task_instruction_serialization before
wrapping it in BufferTask, or add a new test that directly constructs a
CommitTask with TaskStrategy::Buffer (or uses the builder to set the strategy)
and asserts that create_commit_state_from_buffer_ix and
create_commit_diff_from_buffer_ix are produced/used; ensure the test verifies
serialization and expected instruction variants so both buffer branches are
covered.

Expand All @@ -228,6 +240,57 @@ impl CommitTask {
args,
)
}

fn create_commit_state_from_buffer_ix(
&self,
validator: &Pubkey,
) -> Instruction {
let commit_id_slice = self.commit_id.to_le_bytes();
let (commit_buffer_pubkey, _) =
magicblock_committor_program::pdas::buffer_pda(
validator,
&self.committed_account.pubkey,
&commit_id_slice,
);

dlp::instruction_builder::commit_state_from_buffer(
*validator,
self.committed_account.pubkey,
self.committed_account.account.owner,
commit_buffer_pubkey,
CommitStateFromBufferArgs {
nonce: self.commit_id,
lamports: self.committed_account.account.lamports,
allow_undelegation: self.allow_undelegation,
},
)
}

fn create_commit_diff_from_buffer_ix(
&self,
validator: &Pubkey,
_fetched_account: &Account,
) -> Instruction {
let commit_id_slice = self.commit_id.to_le_bytes();
let (commit_buffer_pubkey, _) =
magicblock_committor_program::pdas::buffer_pda(
validator,
&self.committed_account.pubkey,
&commit_id_slice,
);

dlp::instruction_builder::commit_diff_from_buffer(
*validator,
self.committed_account.pubkey,
self.committed_account.account.owner,
commit_buffer_pubkey,
CommitStateFromBufferArgs {
nonce: self.commit_id,
lamports: self.committed_account.account.lamports,
allow_undelegation: self.allow_undelegation,
},
)
}
}

#[derive(Clone)]
Expand Down
2 changes: 1 addition & 1 deletion test-integration/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions test-integration/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ magicblock-config = { path = "../magicblock-config" }
magicblock-core = { path = "../magicblock-core" }
magic-domain-program = { git = "https://github.com/magicblock-labs/magic-domain-program.git", rev = "ea04d46", default-features = false }
magicblock_magic_program_api = { package = "magicblock-magic-program-api", path = "../magicblock-magic-program-api" }
magicblock-delegation-program = { git = "https://github.com/magicblock-labs/delegation-program.git", rev = "e8d03936", features = [
"no-entrypoint",
magicblock-delegation-program = { git = "https://github.com/magicblock-labs/delegation-program.git", rev = "ea1f2f916268132248fe8d5de5f07d76765dd937", features = [
"no-entrypoint",
] }
magicblock-program = { path = "../programs/magicblock" }
magicblock-rpc-client = { path = "../magicblock-rpc-client" }
Expand Down
Binary file modified test-integration/schedulecommit/elfs/dlp.so
View file
Open in desktop
Binary file not shown.
4 changes: 2 additions & 2 deletions test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,13 +253,13 @@ fn test_committing_and_undelegating_huge_order_book_account() {
println!("Important: use {rng_seed} as seed to regenerate the random inputs in case of test failure");
let mut random = StdRng::seed_from_u64(rng_seed);
let mut update = BookUpdate::default();
update.bids.extend((0..random.gen_range(5..10)).map(|_| {
update.bids.extend((0..random.gen_range(5..100)).map(|_| {
OrderLevel {
price: random.gen_range(75000..90000),
size: random.gen_range(1..10),
}
}));
update.asks.extend((0..random.gen_range(5..10)).map(|_| {
update.asks.extend((0..random.gen_range(5..100)).map(|_| {
OrderLevel {
price: random.gen_range(125000..150000),
size: random.gen_range(1..10),
Expand Down
Loading