feat: Execute CommitDiff as BufferTask #616
Conversation
📝 WalkthroughWalkthroughThis PR refactors commit handling across the task system: args_task no longer special-cases commit diffs and now switches commits to the buffer strategy; buffer_task uses computed diffs (when available) as the state source, updates chunking to use the diff length, and replaces manual construction of Possibly related PRs
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🧰 Additional context used🧠 Learnings (8)📚 Learning: 2025-12-03T09:33:48.707ZApplied to files:
📚 Learning: 2025-11-21T10:22:07.520ZApplied to files:
📚 Learning: 2025-11-24T14:21:00.996ZApplied to files:
📚 Learning: 2025-10-26T16:54:39.084ZApplied to files:
📚 Learning: 2025-11-20T08:57:07.217ZApplied to files:
📚 Learning: 2025-11-21T11:03:26.756ZApplied to files:
📚 Learning: 2025-11-19T09:34:37.917ZApplied to files:
📚 Learning: 2025-10-21T14:00:54.642ZApplied to files:
🧬 Code graph analysis (3)magicblock-committor-service/src/tasks/args_task.rs (3)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
🔇 Additional comments (11)
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 |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
4 times, most recently
from
dd95c9a to
4a09180
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
2 times, most recently
from
4ea5450 to
41493c5
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
2 times, most recently
from
17f396e to
3259f79
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
385e7a3 to
3999503
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
2 times, most recently
from
23a797e to
64d79cc
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
3999503 to
256ab3f
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
2 times, most recently
from
5838b75 to
e182eb1
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
59b7935 to
c07a177
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
2 times, most recently
from
18adf1e to
5b95cad
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
9da0edd to
cf6229d
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
2 times, most recently
from
2aada37 to
2eb5776
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
from
9be6a8a to
b94a549
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
9e8da2d to
34d41c2
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
from
b94a549 to
d445507
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
34d41c2 to
effba68
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/tasks/args_task.rs (1)
87-99: Production change in args_task.rs is correct; test cases violate the buffer strategy invariantThe change at
args_task.rs:92correctly appliesswitch_to_buffer_strategy()to ensure optimizedArgsTaskType::Committasks are executed via buffer-based strategy. This is the right pattern.However, test cases in
mod.rs:567andmod.rs:595constructBufferTaskwithCommitTaskinstances inTaskStrategy::Args(the default fromCommitTaskBuilder::create_commit_task()), without callingswitch_to_buffer_strategy(). WhenBufferTask::instruction()is called, it will emitArgs-based commit instructions (CommitState/CommitDiff) instead of buffer-based ones, violating the invariant that BufferTask should only produce buffer-strategy commits.Update both test cases to apply
switch_to_buffer_strategy()before passing toBufferTask::new_preparation_required(), matching the production pattern inargs_task.rs:92.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/schedulecommit/elfs/dlp.sois excluded by!**/*.so
📒 Files selected for processing (6)
Cargo.toml(1 hunks)magicblock-committor-service/src/tasks/args_task.rs(1 hunks)magicblock-committor-service/src/tasks/buffer_task.rs(2 hunks)magicblock-committor-service/src/tasks/mod.rs(5 hunks)test-integration/Cargo.toml(1 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/tasks/mod.rs
🧬 Code graph analysis (1)
magicblock-committor-service/src/tasks/buffer_task.rs (3)
magicblock-committor-program/src/state/chunks.rs (1)
from_data_length(46-49)magicblock-committor-service/src/tasks/mod.rs (1)
optimize(78-80)magicblock-committor-service/src/tasks/args_task.rs (1)
optimize(87-100)
🔇 Additional comments (6)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
256-267: Expanded order book size fuzzing looks good; just watch test runtimeThe wider
5..100ranges for bids/asks should better exercise large book handling and the new buffer-based CommitDiff path without changing test semantics. Just ensure CI runtime remains acceptable and that this level of randomness doesn’t make failures too rare or slow to reproduce beyond the logged seed.magicblock-committor-service/src/tasks/buffer_task.rs (1)
48-55: BufferTask diff/state preparation and delegation to CommitTask look coherentUsing
CommitTask::compute_diff()to decide between diff bytes and full state forcommitted_data, and derivingChunksfrom that length, aligns with the new buffer-based commit flow. Deferringinstruction()toCommitTask::create_commit_ixkeeps BufferTask thin and ensures it follows the chosenTaskStrategywithout extra size optimizations, which matches the intent described in the comment.Also applies to: 59-59, 65-69, 75-77
test-integration/Cargo.toml (1)
60-62: Verify updatedmagicblock-delegation-programrevision and cross-file consistencyThe new git
revformagicblock-delegation-programwith"no-entrypoint"looks fine and matches the rootCargo.toml. Please double-check that this commit exists on the remote and is the intended one for the pairedCommitDiffFromBufferchanges.Cargo.toml (1)
112-114: Rootmagicblock-delegation-programrev bump matches integration workspaceThe workspace-level dependency now points at the same delegation-program git revision as the integration workspace. Please confirm this rev is correct for the new buffer-based commit instructions and that downstream crates depending on dlp still build and pass tests with it.
magicblock-committor-service/src/tasks/mod.rs (2)
149-156: CommitTask strategy-based dispatch for Args vs Buffer and diff vs state looks correctThe new
CommitTaskshape andcreate_commit_ixdispatch cover all four cases cleanly:
TaskStrategy::Args+Some(base_account)→CommitDiffArgsviacommit_diff.TaskStrategy::Args+None→CommitStateArgsviacommit_state.TaskStrategy::Buffer+Some(base_account)→commit_diff_from_bufferusing the buffer PDA.TaskStrategy::Buffer+None→commit_state_from_bufferusing the buffer PDA.Defaulting
CommitTaskBuildertoTaskStrategy::Argsand then flipping to Buffer viaswitch_to_buffer_strategy()when optimizing keeps the creation API simple while making the execution path explicit. The separation between args-based and buffer-based instruction builders also makes it easier to reason about fallbacks whenbase_accountis unavailable (graceful degradation to CommitState).Also applies to: 160-166, 169-193
196-203:compute_diffhelper centralizes diff computation for buffer preparationExposing
compute_diffonCommitTask(wrappingdlp::compute_diffoverbase_account.data()and committed data) is a good consolidation for BufferTask preparation logic. UsingOption<AlignedVec>keeps the call sites straightforward and clearly signals that diff-based commits are only applicable when abase_accountis present.
| 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(), |
There was a problem hiding this comment.
🧹 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()), matchingPreparationTask::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
CommitTaskthat hasstrategy: TaskStrategy::Buffer(viaswitch_to_buffer_strategy()), or - Add dedicated tests that directly exercise
create_commit_state_from_buffer_ixandcreate_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-serviceLength 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_serializationto callswitch_to_buffer_strategy()on theCommitTaskbefore wrapping it inBufferTask, or - Add dedicated tests that directly construct a
CommitTaskwithTaskStrategy::Bufferand 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.
effba68 to
61c8c28
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
from
d445507 to
cbbf5ee
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
61c8c28 to
7d6f839
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
2 times, most recently
from
8f7f30d to
74155a6
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
7d6f839 to
faed161
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
from
74155a6 to
280eeb7
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
2 times, most recently
from
9c11945 to
a42745b
Compare
snawaz
force-pushed
the
snawaz/commit-diff-buffer
branch
from
280eeb7 to
e3ce6e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
magicblock-committor-service/src/tasks/mod.rs (1)
561-586: Tests still serialize Args-style commit instructions for BufferTask; add coverage for buffer strategy and*_from_bufferbuilders
test_buffer_task_instruction_serializationconstructsBufferTaskType::Commitwith aCommitTaskfromCommitTaskBuilder::create_commit_task(...), which still defaults toTaskStrategy::Args. BecauseBufferTask::instruction()now just delegates toCommitTask::create_commit_ix(), this test ends up exercising the Args path (plaincommit_state/commit_diff) rather than the new buffer-basedcommit_*_from_bufferinstructions.To actually cover the buffer paths and the new buffer instruction builders, update the test to either:
- call
switch_to_buffer_strategy()on theCommitTaskbefore wrapping it inBufferTaskType::Commit, or- construct a
CommitTaskwithTaskStrategy::Buffervia the builder or a helper, and assert thatcreate_commit_ix()produces the expected*_from_buffervariants.This aligns the test with the intended semantics of
BufferTaskand gives direct coverage of the new buffer commit flows.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/schedulecommit/elfs/dlp.sois excluded by!**/*.so
📒 Files selected for processing (6)
Cargo.toml(1 hunks)magicblock-committor-service/src/tasks/args_task.rs(1 hunks)magicblock-committor-service/src/tasks/buffer_task.rs(2 hunks)magicblock-committor-service/src/tasks/mod.rs(5 hunks)test-integration/Cargo.toml(1 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/tasks/mod.rs
🧬 Code graph analysis (3)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
test-integration/programs/schedulecommit/src/order_book.rs (1)
bids(182-184)
magicblock-committor-service/src/tasks/buffer_task.rs (3)
magicblock-committor-program/src/state/chunks.rs (1)
from_data_length(46-49)magicblock-committor-service/src/tasks/mod.rs (1)
optimize(78-80)magicblock-committor-service/src/tasks/args_task.rs (1)
optimize(87-100)
magicblock-committor-service/src/tasks/mod.rs (1)
magicblock-committor-service/src/persist/types/commit_strategy.rs (1)
args(17-23)
🔇 Additional comments (6)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
256-262: Change is appropriate for stress-testing the BufferTask implementation.The 10x increase in test data (max 200 entries vs. 20 previously) aligns with the PR's goal of validating CommitDiff as BufferTask for large accounts. Verification confirms:
- CI runs on
extra-largerunner with adequate resources- No explicit test timeout configured (relies on GitHub Actions default 360-minute limit)
- Test is intentionally "huge" and properly instrumented with seed output for reproducibility
No environment capacity concerns identified.
Cargo.toml (1)
112-114: Delegation-program git rev bump looks consistent; verify compatibility with new buffer commit instructionsThe new
magicblock-delegation-programrev is pinned consistently here and intest-integration/Cargo.toml, and the feature set is unchanged ("no-entrypoint"), so workspace wiring looks good. Please just confirm this rev contains the*_from_buffercommit instructions your newCommitTaskbuffer paths rely on and that integration tests pass against it.test-integration/Cargo.toml (1)
60-62: Test workspace dependency aligned with root; ensure integration tests cover new buffer commit pathsThe
magicblock-delegation-programrev here matches the root workspace and keeps"no-entrypoint", so dependency alignment looks correct. Please make sure the updated integration tests actually exercise the new buffer-based commit/diff paths against this program version.magicblock-committor-service/src/tasks/args_task.rs (1)
91-95: Commit optimization correctly switches to Buffer strategy before wrapping in BufferTaskRouting
ArgsTaskType::Committhroughvalue.switch_to_buffer_strategy()before constructingBufferTaskType::Commitensures that optimized commit tasks consistently take the buffer-based path (and thus use the new*_from_bufferinstructions). This aligns with the newTaskStrategydesign and fixes the prior need for ad‑hoc flags.magicblock-committor-service/src/tasks/mod.rs (2)
55-59: TaskStrategy + CommitTask refactor gives a clean two-path model (Args vs Buffer)Introducing
TaskStrategyand threading it throughCommitTask(withCommitTaskBuilderdefaulting toArgsandswitch_to_buffer_strategy()for buffer flows) makes the commit behavior explicit and removes the previous ad‑hoc flagging. Thecreate_commit_ix()match on(strategy, base_account)cleanly encodes:
- Args + no base account →
CommitState.- Args + base account →
CommitDiff.- Buffer + no base account →
CommitStateFromBuffer.- Buffer + base account →
CommitDiffFromBuffer.This layout is straightforward to reason about and aligns well with the PR’s goal of supporting CommitDiff via BufferTask without degrading to CommitState purely due to transaction-size constraints.
Also applies to: 149-167, 169-193
4-6: I'll search the public magicblock repositories to verify the API contract.I need to search for the actual source code of the delegation-program repository to verify the API contract. Let me look for the specific function signatures.
Based on my web search for the magicblock-delegation-program repository, I was unable to access the specific source code details needed to verify the API contract. The searches returned the repository index and documentation pages, but not the actual function signatures for
commit_state_from_buffer,commit_diff_from_buffer, or theCommitStateFromBufferArgsstruct definition.To properly verify the concerns raised in this review comment, you would need to:
- Review the delegation-program repository source code (specifically the instruction builders and args modules) to confirm that both
commit_state_from_bufferandcommit_diff_from_bufferaccept the sameCommitStateFromBufferArgsstruct- Examine the on-chain processor implementations to verify that buffer contents are interpreted consistently as either full state or diff bytes, matching how
BufferTask::preparation_requiredprepares them- Confirm the buffer PDA derivation uses the same parameters across all functions
The original concern about the API contract alignment is valid and warrants manual verification against the actual dlp crate implementation.
Verify that
commit_state_from_bufferandcommit_diff_from_bufferin the dlp crate both acceptCommitStateFromBufferArgs, and confirm the on-chain handlers interpret buffer contents consistently with howBufferTaskprepares them (lines 244-293 and related code).
| 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, |
There was a problem hiding this comment.
🛠️ 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(orpreparation_required) assert in debug builds that the innerCommitTaskhasTaskStrategy::Buffer, and/or - Moving the
switch_to_buffer_strategy()call into theBufferTaskconstruction 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.
| 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(), | ||
| ) | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 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.
e3ce6e6 to
044c9cf
Compare
snawaz
force-pushed
the
snawaz/commit-diff
branch
from
a42745b to
00b3b98
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
magicblock-committor-service/src/tasks/mod.rs (2)
221-243: Reusecompute_diff()to avoid duplicate diff computation logic.The inline
compute_diffcall at lines 229-232 duplicates the logic already encapsulated inself.compute_diff(). Consider delegating to the method for consistency.Based on past review comments, apply this refactor to reuse the existing method:
fn create_commit_diff_ix( &self, validator: &Pubkey, base_account: &Account, ) -> Instruction { + let diff = self + .compute_diff() + .expect("base_account must be Some when using CommitDiff"); + let args = CommitDiffArgs { nonce: self.commit_id, lamports: self.committed_account.account.lamports, - diff: compute_diff( - base_account.data(), - self.committed_account.account.data(), - ) - .to_vec(), + diff: diff.to_vec(), allow_undelegation: self.allow_undelegation, }; dlp::instruction_builder::commit_diff( *validator, self.committed_account.pubkey, self.committed_account.account.owner, args, ) }
563-587: Add test coverage for buffer instruction builders.The
test_buffer_task_instruction_serializationtest constructsCommitTaskviaCommitTaskBuilder::create_commit_task(), which defaults toTaskStrategy::Args. Since the test never callsswitch_to_buffer_strategy(), it never exercises the newcreate_commit_state_from_buffer_ixandcreate_commit_diff_from_buffer_ixpaths.Based on past review comments, update the test to cover buffer paths:
#[tokio::test] async fn test_buffer_task_instruction_serialization() { let validator = Pubkey::new_unique(); + // Test with buffer strategy to exercise buffer instruction builders let buffer_task = BufferTask::new_preparation_required(BufferTaskType::Commit( CommitTaskBuilder::create_commit_task( 456, false, CommittedAccount { pubkey: Pubkey::new_unique(), account: Account { lamports: 2000, data: vec![7, 8, 9], owner: Pubkey::new_unique(), executable: false, rent_epoch: 0, }, }, &Arc::new(NullTaskInfoFetcher), ) - .await, + .await + .switch_to_buffer_strategy(), )); assert_serializable(&buffer_task.instruction(&validator)); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
magicblock-committor-service/src/tasks/args_task.rs(1 hunks)magicblock-committor-service/src/tasks/buffer_task.rs(2 hunks)magicblock-committor-service/src/tasks/mod.rs(5 hunks)test-integration/Cargo.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
test-integration/Cargo.toml
🧬 Code graph analysis (2)
magicblock-committor-service/src/tasks/buffer_task.rs (3)
magicblock-committor-program/src/state/chunks.rs (1)
from_data_length(46-49)magicblock-committor-service/src/tasks/args_task.rs (2)
value(162-169)optimize(92-105)magicblock-committor-service/src/tasks/mod.rs (1)
optimize(79-81)
magicblock-committor-service/src/tasks/mod.rs (1)
magicblock-committor-service/src/persist/types/commit_strategy.rs (1)
args(17-23)
🔇 Additional comments (9)
magicblock-committor-service/src/tasks/args_task.rs (1)
96-99: LGTM! Clean refactoring to use buffer strategy switching.The simplified logic correctly switches to buffer strategy before wrapping in
BufferTaskType::Commit, eliminating the need for special-case handling. This aligns with the PR objectives to support CommitDiff as a BufferTask without degrading to CommitState.magicblock-committor-service/src/tasks/buffer_task.rs (2)
49-61: LGTM! Diff-aware state computation correctly supports CommitDiff as BufferTask.The logic now computes diff when available and uses it for both chunking and buffer preparation, which directly addresses the PR objective of supporting CommitDiff as a BufferTask without degrading to CommitState.
Note: The main flow in
args_task.rsline 98 correctly callsswitch_to_buffer_strategy()before constructingBufferTask, ensuring the strategy is set properly.
69-69: LGTM! Unified instruction creation simplifies the code.Replacing manual PDA-based instruction assembly with
value.create_commit_ix(validator)delegates to theCommitTask's strategy-aware instruction creation, making the code cleaner and more maintainable.magicblock-committor-service/src/tasks/mod.rs (6)
4-4: LGTM! TaskStrategy is now unconditionally public to support the two-strategy architecture.Making
TaskStrategypublic enables the Args/Buffer strategy pattern throughout the codebase, which is essential for the new buffer-based commit flow.Also applies to: 56-60
155-155: LGTM! Strategy field replaces force_commit_state with explicit Args/Buffer control.The new
strategyfield provides clearer control over instruction generation compared to the previousforce_commit_stateflag, aligning with the PR's goal of supporting CommitDiff as a BufferTask.Also applies to: 165-167
170-173: LGTM! Strategy switching method provides clean transition to buffer path.The
switch_to_buffer_strategy()method enables explicit control over strategy transitions, used byArgsTask::optimize()to move commits to the buffer path.
175-194: LGTM! Strategy-aware instruction dispatch correctly routes Args vs Buffer paths.The branching on
self.strategywith nested checks forbase_accountensures the correct instruction builder is called for each combination (Args with diff/state, Buffer with diff/state from buffer).
197-204: LGTM! Centralized diff computation reduces code duplication.The
compute_diff()method provides a single point for diff computation logic, used byBufferTaskduring preparation. This is good for maintainability.
245-268: LGTM! Buffer-based commit state instruction correctly uses buffer PDA.The implementation properly derives the buffer PDA and constructs the instruction with
CommitStateFromBufferArgs, aligning with howPreparationTaskinitializes and writes buffer data.
| 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, | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 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 {| magicblock-delegation-program = { git = "https://github.com/magicblock-labs/delegation-program.git", rev = "ea1f2f916268132248fe8d5de5f07d76765dd937", features = [ | ||
| "no-entrypoint", | ||
| ] } |
There was a problem hiding this comment.
🧹 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.
044c9cf to
a6c69e1
Compare
Problem
The previous PR #575 in this stack implements support for
CommitDiffthat executes asArgsTask. When the transaction size grows beyond the limit,CommitDifffirst degrades toCommitStateand then gets executed asBufferTaskto reduce the transaction size.Degration of
CommitDifftoCommitStateis not a desirable thing.Solution
So this PR implements support for
CommitDiffasBufferTask. Means degradation ofCommitDifftoCommitStateis not required anymore.Related
Another PR magicblock-labs/delegation-program#118 that implements
CommitDiffFromBufferinstruction, is created in the delegation-program.Screenshot
Summary by CodeRabbit
Refactor
Dependencies
Tests
✏️ Tip: You can customize this high-level summary in your review settings.