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
snawaz wants to merge 1 commit into
base: snawaz/commit-diff
Choose a base branch
from
+102 −66
Open

feat: Execute CommitDiff as BufferTask #616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
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 @@ -89,21 +89,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 magicblock_metrics::metrics::LabelValue;
use solana_instruction::Instruction;
Expand Down Expand Up @@ -47,16 +46,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 +49 to 61
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 @@ -65,31 +66,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 @@ -53,7 +53,6 @@ pub enum PreparationState {
Cleanup(CleanupTask),
}

#[cfg(test)]
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum TaskStrategy {
Args,
Expand Down Expand Up @@ -153,7 +152,7 @@ impl CommitTaskBuilder {
allow_undelegation,
committed_account,
base_account,
force_commit_state: false,
strategy: TaskStrategy::Args,
}
}
}
Expand All @@ -164,29 +163,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 +197 to +205
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 @@ -205,17 +221,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(),
Expand All @@ -229,6 +241,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,
},
)
}
Comment on lines +270 to +294
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 16, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Clarify or remove the unused _fetched_account parameter.

The _fetched_account parameter in create_commit_diff_from_buffer_ix is marked as unused. If it's kept for API consistency with create_commit_diff_ix, consider adding a comment explaining why. Otherwise, it can be removed since the diff is read from the buffer account.

If the parameter is truly unnecessary, apply this diff:

 fn create_commit_diff_from_buffer_ix(
     &self,
     validator: &Pubkey,
-    _fetched_account: &Account,
 ) -> Instruction {

Or add a comment if it's kept for consistency:

 fn create_commit_diff_from_buffer_ix(
     &self,
     validator: &Pubkey,
+    // Kept for API symmetry with create_commit_diff_ix, though unused
+    // since diff is read from buffer
     _fetched_account: &Account,
 ) -> Instruction {

}

#[derive(Clone)]
Expand Down
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",
] }
Comment on lines +60 to 62
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 16, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider using a branch reference instead of a commit hash for the magicblock-delegation-program dependency.

While the delegation program is a professionally audited dependency with no identified security vulnerabilities, using a commit hash locks the dependency to a specific version. For consistency with repository best practices and to enable automatic updates when the repository is updated, switch to a branch reference (e.g., branch = "main") instead of the commit hash.

magicblock-delegation-program = { git = "https://github.com/magicblock-labs/delegation-program.git", branch = "main", features = [
 "no-entrypoint",
] }
🤖 Prompt for AI Agents 
In test-integration/Cargo.toml around lines 60 to 62, the
magicblock-delegation-program dependency is pinned to a specific commit hash
which prevents automatic updates; change the git dependency to reference a
branch (for example branch = "main") instead of rev so the crate pulls the
branch tip, keeping the features list intact, and update any lockfile or CI
caching if needed to ensure builds pick up the branch-based dependency.

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 @@ -303,13 +303,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