feat(mimd-0025/mimd-0027): durable intent execution - #1368
Conversation
# Conflicts: # magicblock-accounts/src/scheduled_commits_processor.rs # magicblock-api/src/magic_validator.rs
# Conflicts: # magicblock-accounts/src/scheduled_commits_processor.rs # magicblock-api/src/magic_validator.rs # magicblock-committor-service/src/committor_processor.rs # magicblock-committor-service/src/service.rs # test-integration/test-committor-service/tests/test_ix_commit_local.rs
# Conflicts: # magicblock-api/src/magic_validator.rs # magicblock-committor-service/src/committor_processor.rs
# Conflicts: # magicblock-api/src/magic_sys_adapter.rs # magicblock-committor-service/src/intent_engine/intent_scheduler.rs # magicblock-committor-service/src/intent_executor/mod.rs # magicblock-committor-service/src/intent_executor/two_stage_executor.rs # magicblock-committor-service/src/intent_executor/utils.rs # magicblock-committor-service/src/persist/commit_persister.rs # magicblock-committor-service/src/tasks/commit_finalize_task.rs # magicblock-committor-service/src/tasks/commit_task.rs # magicblock-committor-service/src/tasks/task_builder.rs # magicblock-committor-service/src/tasks/task_strategist.rs # magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs # magicblock-metrics/src/metrics/mod.rs # test-integration/Cargo.lock # test-integration/test-committor-service/tests/test_delivery_preparator.rs # test-integration/test-committor-service/tests/test_ix_commit_local.rs
It does include refactoring. A lot of code that would be otherwise duplicated in different IntentExecutors was extracted into utils. Patching logic was extracted into The main logic shall be easily reviewable because of centralization in single general functions: I agree that would be better to split it off, but it wasn't really planned and is something that happened in the middle of writing code so it would be hard to extract those changes into separate PRs. Will try to forsee this next time |
# Conflicts: # magicblock-committor-service/src/service.rs
thlorenz
left a comment
There was a problem hiding this comment.
I found several lifecycle issues that look blocking before merge and commented in place except for the below two:
Outside-diff 1
Location: magicblock-committor-service/src/intent_engine/intent_execution_engine.rs:281-290
I could not find a good in-diff anchor for this one. The behavior appears to predate the PR, but it is still worth calling out if we want the new durable outbox design to close this lifecycle gap:
The intent scheduler completes and releases committed pubkeys even when result.inner is an error.
In a two-stage flow, the commit stage may already have been sent/recorded while the finalize stage failed.
Releasing the keys at that point lets later intents touching the same accounts execute before the failed intent is retried, finalized, or marked terminal, which conflicts with per-account intent serialization and recovery safety.
The engine should keep those account keys blocked or move the intent into a durable terminal/retry state before unblocking dependent intents.
Outside-diff 2
Location: magicblock-committor-service/src/tasks/task_strategist.rs:140-156
I could not find a fair in-diff anchor for this one either. The task-count limit was not introduced by this PR, but it still seems critical for the commit pipeline:
The strategist limits single-stage packing by task count only, while each commit/finalize/undelegate task advertises 120,000 CU and assemble_tasks_tx_with_uniqueness_nonce sums those values into one compute-budget limit. A 22-task single-stage transaction can request 2,640,000 CU, and large two-stage commit batches can exceed Solana's 1,400,000 CU transaction cap as well.
The strategist should enforce the transaction-level CU cap when selecting single-stage and two-stage task groups, or split stages so every produced transaction stays below the cap.
# Conflicts: # magicblock-committor-service/src/service.rs # test-integration/Cargo.lock
# Conflicts: # magicblock-committor-service/src/committor_processor.rs # magicblock-committor-service/src/intent_engine/intent_execution_engine.rs # magicblock-committor-service/src/intent_executor/error.rs # magicblock-committor-service/src/intent_executor/intent_execution_client.rs # magicblock-committor-service/src/intent_executor/mod.rs # magicblock-committor-service/src/intent_executor/two_stage_executor.rs # magicblock-committor-service/src/persist/commit_persister.rs # magicblock-committor-service/src/persist/db.rs # magicblock-committor-service/src/service.rs # magicblock-rpc-client/src/lib.rs # magicblock-rpc-client/src/utils.rs # test-integration/Cargo.lock
feat: switched notify and commitsent places. Doesn't break integration
# Conflicts: # magicblock-committor-service/src/committor_processor.rs # magicblock-committor-service/src/persist/commit_persister.rs # magicblock-committor-service/src/persist/db.rs # magicblock-committor-service/src/service.rs # test-integration/Cargo.lock
…returns none if commitment wasn't reached
# Conflicts: # test-integration/Cargo.lock
# Conflicts: # Cargo.lock # magicblock-committor-service/src/committor_processor.rs # magicblock-committor-service/src/persist/commit_persister.rs # magicblock-committor-service/src/persist/db.rs # magicblock-committor-service/src/persist/mod.rs # magicblock-committor-service/src/service.rs # test-integration/Cargo.lock
redsuite: PR vs masterSingle-run diff on shared runners — indicative only; statistical verdicts come from Bencher thresholds. |
|
| Project | magicblock-labs |
| Branch | feat/mimd-0025/main |
| Testbed | blacksmith-8vcpu-ubuntu-2404 |
⚠️ WARNING: Truncated view!The full continuous benchmarking report exceeds the maximum length allowed on this platform.
🚨 3 Alerts
🐰 View full continuous benchmarking report in Bencher
snawaz
left a comment
There was a problem hiding this comment.
Thanks for the huge work.
Overall direction looks good to me but this PR mixes large refactors with new behavior (we should avoid doing that in the future), so the real correctness/readability diff is hard to review.
I tried to review it as thoroughly as possible within a reasonable amount of time, and posted comments ranging from simplifying code with fewer indirections/layers to correctness concerns.
| pub fn apply_stage_transition( | ||
| &mut self, | ||
| stage: ExecutionStage, | ||
| ) -> Result<(), &'static str> { |
There was a problem hiding this comment.
better return Enum error? str are hard to match on!
| fn apply_stage_transition( | ||
| &mut self, | ||
| stage: TwoStageProgress, | ||
| ) -> Result<(), &'static str> { |
There was a problem hiding this comment.
better return Enum error? str are hard to match on!
| invoke_context.native_invoke( | ||
| Instruction { | ||
| program_id: crate::id(), | ||
| accounts: vec![ | ||
| AccountMeta::new(sponsor, true), | ||
| AccountMeta::new(pda, true), | ||
| AccountMeta::new(EPHEMERAL_VAULT_PUBKEY, false), | ||
| ], | ||
| data: MagicBlockInstruction::CreateEphemeralAccount { data_len } | ||
| .try_to_vec() | ||
| .map_err(|_| InstructionError::InvalidInstructionData)?, | ||
| }, | ||
| &[pda], | ||
| ) |
There was a problem hiding this comment.
perf: Can we avoid the self-CPI here (in a follow-up PR)?
Since this is invoking the same program, it may be cheaper and simpler to refactor the account-creation logic into a shared helper (say, create_ephemeral_account_unchecked) and call it directly from both paths. A shared-helper would avoid lots of overhead associated with CPI (instruction construction, serialization, account meta allocation, account remapping, signer/writable privilege checks, stack push/pop, and dispatch back through the processor).
It's especially important because AcceptScheduleCommits batches up to 50 intents, this overhead scales directly with the CHUNK_SIZE (currently set to 50 in send_accept_tx()) and RESCHEDULE_CHUNK_SIZE is set to 1000 !!
There was a problem hiding this comment.
Unfortunately I don't think this would be possible with new engine. ER accounts can't be created in magic-program anymore. We HAVE to CPI into special builtin magic_root as only it has a permission to create such accounts.
| // Original blockhash is stale after restart; signal recovery so | ||
| // notify_commit_sent rebuilds the tx with a fresh ER blockhash. | ||
| intent_bundles_chunk.iter_mut().for_each(|b| { | ||
| b.inner.sent_transaction = Transaction::default() |
There was a problem hiding this comment.
type-safety: I think we should change the type of sent_transaction to IntentSentTransaction, so this recovery state is explicit instead of implicitly-encoded as an empty/default transaction:
| b.inner.sent_transaction = Transaction::default() | |
| b.inner.sent_transaction = IntentSentTransaction::Recovered; |
Or at least Option<Transaction> (less explicit, but still better than empty-transaction) if you do not want to move the definition of IntentSentTransaction to the crate where bundle is defined.
| let mut items: Vec<OutboxIntentBundle> = unsafe { | ||
| let mut v = std::mem::ManuallyDrop::new(heap.into_vec()); | ||
| Vec::from_raw_parts( | ||
| v.as_mut_ptr() as *mut OutboxIntentBundle, | ||
| v.len(), | ||
| v.capacity(), | ||
| ) | ||
| }; |
There was a problem hiding this comment.
I agree with Rabbit.
The unsafe block isn't worth having just to avoid one memory allocation. We're not saving much, given that we're already paying for DB scanning, deserialization of all scanned items (including paying for their nested allocations), heap maintenance, sorting.
So we should prefer the safe version here unless we have a benchmark showing this conversion matters.
| let mut items: Vec<OutboxIntentBundle> = unsafe { | |
| let mut v = std::mem::ManuallyDrop::new(heap.into_vec()); | |
| Vec::from_raw_parts( | |
| v.as_mut_ptr() as *mut OutboxIntentBundle, | |
| v.len(), | |
| v.capacity(), | |
| ) | |
| }; | |
| let mut items: Vec<OutboxIntentBundle> = | |
| heap.into_iter().map(|ordered| ordered.inner).collect(); |
There was a problem hiding this comment.
perf/simplify: When zooming out, I think the current bounded-heap approach has two downsides:
-
The caller asks for
nitems in a loop (fromreschedule_intents()), so we scan the DB, serialize all items, refill the buffer and discards the extra items (> capacity), and then in the next iteration, we rescan the previously discarded items, and so on... and then repeat this again and again. -
It's quite verbose. Ordering wrapper and unsafe conversion adds to that as well.
Alternative (simpler and performant):
We should replace refill() with populate() that populates the buffer with all recovered items:
fn populate(&mut self) {
// making `buffer: Vec` avoids another conversion!
self.buffer = outbox_iter.collect::<Vec<_>>();
// NOTE: sort in "descending" order, not "ascending"
// Also, rename buffer -> desc_sorted_items
self.buffer.sort_unstable_by_key(|b| Reverse(b.id));
}Rationale:
Practically, there will not be lots of "recovered/persisted" intents. If we assume the number is 2000 recovered intents and each intent has 3 accounts (on an average), then we need ~2 MB to store everything in the self.buffer. Even if it is ~10 MB (or even more), that seems acceptable for startup recovery, and we avoid repeated DB scans/deserialization.
We should also consider storing a smaller value in the DB, not the full intent with the commit accounts. (can be a follow-up PR).
There was a problem hiding this comment.
The number of loaded intents are unknown. We had a case when 200k were loaded on restart.
Additionally, size of accounts is also unknown. Having a system that blindly loads the whole thing is incorrect IMO.
With regards to performance under CAPACITY, which as you said will be the most likely case, the strict algorithmic complexity is the same O(n*logn) same as provided snippet. But in an unlikely scenario we load an unbounded amount of intents which is imo incorrect. Tho current system can be improved of course.
We should also consider storing a smaller value in the DB not the full intent with the commit accounts. (can be a follow-up PR).
By DB you mean AccountsDB? If so, this is rather complex to achieve as of now. We need to commit state at the moment of execution. Fetching state by pubkey during commit after(exection) would yield a potentially newer state. I could see that being possible only via replay from snapshot to a particular commit slot, which feels rather expensive as well and not sure it will be better, tradeoff here again :)
What I think would be better - charge user extra for large Intent. As intents are supposed to be closed this would be our compensation for inconvinience.
If you mean other DB.
One of the reasons for Outbox was moving away from DB. The reason is that supplied state from db has also to become part of the whole fraud prood story. Replicas have to have it as well and so on.
| let statuses = client | ||
| .get_signature_statuses_with_history( | ||
| std::slice::from_ref(&pending.signature), | ||
| CommitmentConfig::finalized(), |
There was a problem hiding this comment.
bug-risk: do we really need CommitmentConfig::finalized() here?
Passing finalized() forces get_signature_statuses_with_history() convert a real status at a lower commitment level into None, which is discussed in the other comment where we lose the ability to distinguish:
- tx not found at all. (this is real
None). - tx found, but not finalized yet. (it is converted into
None).
Those are very different cases for the exact-once guarantee. I think we should fetch the status as-is and interpret the commitment level here, in this function, together with the blockhash validity!
Having said that, I realize get_signature_statuses_with_history is incorrectly named because it does more than what the name says.
A facade API should match the semantics of the underlying API. If we want commitment filtering, that should either be explicit in the name... or done by the caller.
| status.and_then(|status| { | ||
| status | ||
| .satisfies_commitment(commitment_config) | ||
| .then(|| status.err.map_or(Ok(()), Err)) | ||
| }) |
There was a problem hiding this comment.
design: if we want commitment filtering, then it should either be explicit in the name, or choose an entirely different name for the function, because a function with name get_signature_statuses_with_history should match the corresponding Solana RPC method contract, else we will end up having a well-known name to mean two different things in two different places.
| pub struct OutboxIntentBundle { | ||
| pub inner: ScheduledIntentBundle, | ||
| status: OutboxIntentBundleStatus, | ||
| } |
There was a problem hiding this comment.
We should also store bump to enable quick validation in the subsequent calls.
| // Validate pda we about to apply transition to | ||
| let provided_pda = | ||
| get_instruction_pubkey_with_idx(transaction_context, INTENT_PDA_IDX)?; | ||
| let expected_pda = outbox_intent_pda(intent_id); |
There was a problem hiding this comment.
If we store bump, we could use that to quickly validate the PDA.
| invoke_context.native_invoke( | ||
| Instruction { | ||
| program_id: crate::id(), | ||
| accounts: vec![ | ||
| AccountMeta::new(sponsor, true), | ||
| AccountMeta::new(pda, false), | ||
| AccountMeta::new(EPHEMERAL_VAULT_PUBKEY, false), | ||
| ], | ||
| data: MagicBlockInstruction::CloseEphemeralAccount | ||
| .try_to_vec() | ||
| .map_err(|_| InstructionError::InvalidInstructionData)?, | ||
| }, | ||
| &[], | ||
| ) |
There was a problem hiding this comment.
We could avoid the CPI call?
Please see the other comment on using CreateEphemeralAccount in CPI.
Summary
This PR implements both MIMD-0025 and MIMD-0027. Briefly, it introduces Outbox for Intent which allows to restore it execution stage and continue execution from a correct point.
Single Intent Executor split into 3 and which correspond to different starting points of Intents. Those executors are derived from Intent execution stage.
For
PendingTransactionlogic and use see MIMD-0027.Apart from pure MIMDs implementation PR extract a common logic into utils and changes file structure for better readability. This PR also removes Persister as we will use Solana Accounts instead of it
Breaking Changes
Test Plan
Summary by CodeRabbit
New Features
Bug Fixes