Redesign the implementation of CommitDiff support

Author: snawaz

Cargo.lock

@@ -109,7 +109,9 @@ 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 = { path="../delegation-program", features = ["no-entrypoint"] }
+magicblock-delegation-program = { git = "https://github.com/magicblock-labs/delegation-program.git", rev = "e8d03936", features = [
+  "no-entrypoint",
+] }
 magicblock-geyser-plugin = { path = "./magicblock-geyser-plugin" }
 magicblock-ledger = { path = "./magicblock-ledger" }
 magicblock-metrics = { path = "./magicblock-metrics" }

Cargo.toml

@@ -343,7 +343,6 @@ impl ScheduledCommitsProcessorImpl {
             included_pubkeys: intent_meta.included_pubkeys,
             excluded_pubkeys: intent_meta.excluded_pubkeys,
             requested_undelegation: intent_meta.requested_undelegation,
-            commit_diff: intent_meta.commit_diff,
         }
     }
 }
@@ -413,7 +412,6 @@ struct ScheduledBaseIntentMeta {
     excluded_pubkeys: Vec<Pubkey>,
     intent_sent_transaction: Transaction,
     requested_undelegation: bool,
-    commit_diff: bool,
 }
 
 impl ScheduledBaseIntentMeta {
@@ -431,7 +429,6 @@ impl ScheduledBaseIntentMeta {
             excluded_pubkeys,
             intent_sent_transaction: intent.action_sent_transaction.clone(),
             requested_undelegation: intent.is_undelegate(),
-            commit_diff: intent.is_commit_diff(),
         }
     }
 }

magicblock-accounts/src/scheduled_commits_processor.rs

@@ -29,6 +29,7 @@ magicblock-metrics = { workspace = true }
 magicblock-program = { workspace = true }
 magicblock-rpc-client = { workspace = true }
 magicblock-table-mania = { workspace = true }
+magicblock-config = { workspace = true }
 rusqlite = { workspace = true }
 solana-account = { workspace = true }
 solana-pubkey = { workspace = true }

magicblock-committor-service/Cargo.toml

@@ -0,0 +1,61 @@
+use solana_account::Account;
+use solana_pubkey::Pubkey;
+use solana_rpc_client::nonblocking::rpc_client::RpcClient;
+use solana_sdk::commitment_config::CommitmentConfig;
+
+//
+// AccountFetcher is used by CommitTask
+//
+pub struct AccountFetcher {
+    rpc_client: RpcClient,
+}
+
+impl Default for AccountFetcher {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
+impl AccountFetcher {
+    pub fn new() -> Self {
+        use crate::{config::ChainConfig, ComputeBudgetConfig};
+
+        #[cfg(feature = "dev-context-only-utils")]
+        let chain_config =
+            ChainConfig::local(ComputeBudgetConfig::new(1_000_000));
+
+        #[cfg(not(feature = "dev-context-only-utils"))]
+        let chain_config = ChainConfig {
+            rpc_uri: magicblock_config::MagicBlockConfig::parse_config()
+                .config
+                .accounts
+                .remote
+                .url
+                .as_ref()
+                .map(|url| url.to_string())
+                .unwrap_or_else(|| {
+                    log::error!(
+                        "Remote URL not configured, falling back to mainnet"
+                    );
+                    "https://api.mainnet-beta.solana.com".to_string()
+                }),
+            ..ChainConfig::mainnet(ComputeBudgetConfig::new(1_000_000))
+        };
+
+        Self {
+            rpc_client: RpcClient::new_with_commitment(
+                chain_config.rpc_uri.to_string(),
+                CommitmentConfig {
+                    commitment: chain_config.commitment,
+                },
+            ),
+        }
+    }
+
+    pub async fn fetch_account(
+        &self,
+        pubkey: &Pubkey,
+    ) -> Result<Account, solana_rpc_client_api::client_error::Error> {
+        self.rpc_client.get_account(pubkey).await
+    }
+}

magicblock-committor-service/src/tasks/account_fetcher.rs

@@ -1,33 +1,20 @@
-use dlp::{
-    args::{CallHandlerArgs, CommitDiffArgs, CommitStateArgs},
-    compute_diff,
-};
-use solana_account::ReadableAccount;
+use dlp::args::CallHandlerArgs;
 use solana_pubkey::Pubkey;
-use solana_rpc_client::rpc_client::RpcClient;
-use solana_sdk::{
-    commitment_config::CommitmentConfig,
-    instruction::{AccountMeta, Instruction},
-};
+use solana_sdk::instruction::{AccountMeta, Instruction};
 
 #[cfg(test)]
 use crate::tasks::TaskStrategy;
-use crate::{
-    config::ChainConfig,
-    tasks::{
-        buffer_task::{BufferTask, BufferTaskType},
-        visitor::Visitor,
-        BaseActionTask, BaseTask, BaseTaskError, BaseTaskResult, CommitTask,
-        FinalizeTask, PreparationState, TaskType, UndelegateTask,
-    },
-    ComputeBudgetConfig,
+use crate::tasks::{
+    buffer_task::{BufferTask, BufferTaskType},
+    visitor::Visitor,
+    BaseActionTask, BaseTask, BaseTaskError, BaseTaskResult, CommitTask,
+    FinalizeTask, PreparationState, TaskType, UndelegateTask,
 };
 
 /// Task that will be executed on Base layer via arguments
 #[derive(Clone)]
 pub enum ArgsTaskType {
     Commit(CommitTask),
-    CommitDiff(CommitTask),
     Finalize(FinalizeTask),
     Undelegate(UndelegateTask), // Special action really
     BaseAction(BaseActionTask),
@@ -57,69 +44,7 @@ impl ArgsTask {
 impl BaseTask for ArgsTask {
     fn instruction(&self, validator: &Pubkey) -> Instruction {
         match &self.task_type {
-            ArgsTaskType::Commit(value) => {
-                let args = CommitStateArgs {
-                    nonce: value.commit_id,
-                    lamports: value.committed_account.account.lamports,
-                    data: value.committed_account.account.data.clone(),
-                    allow_undelegation: value.allow_undelegation,
-                };
-                dlp::instruction_builder::commit_state(
-                    *validator,
-                    value.committed_account.pubkey,
-                    value.committed_account.account.owner,
-                    args,
-                )
-            }
-            ArgsTaskType::CommitDiff(value) => {
-                let chain_config =
-                    ChainConfig::local(ComputeBudgetConfig::new(1_000_000));
-
-                let rpc_client = RpcClient::new_with_commitment(
-                    chain_config.rpc_uri.to_string(),
-                    CommitmentConfig {
-                        commitment: chain_config.commitment,
-                    },
-                );
-
-                let account = match rpc_client
-                    .get_account(&value.committed_account.pubkey)
-                {
-                    Ok(account) => account,
-                    Err(e) => {
-                        log::warn!("Fallback to commit_state and send full-bytes, as rpc failed to fetch the delegated-account from base chain, commmit_id: {} , error: {}", value.commit_id, e);
-                        let args = CommitStateArgs {
-                            nonce: value.commit_id,
-                            lamports: value.committed_account.account.lamports,
-                            data: value.committed_account.account.data.clone(),
-                            allow_undelegation: value.allow_undelegation,
-                        };
-                        return dlp::instruction_builder::commit_state(
-                            *validator,
-                            value.committed_account.pubkey,
-                            value.committed_account.account.owner,
-                            args,
-                        );
-                    }
-                };
-
-                let args = CommitDiffArgs {
-                    nonce: value.commit_id,
-                    lamports: value.committed_account.account.lamports,
-                    diff: compute_diff(
-                        account.data(),
-                        value.committed_account.account.data(),
-                    )
-                    .to_vec(),
-                    allow_undelegation: value.allow_undelegation,
-                };
-                dlp::instruction_builder::commit_diff(
-                    *validator,
-                    value.committed_account.pubkey,
-                    value.committed_account.account.owner,
-                    args,
-                )
-            }
+            ArgsTaskType::Commit(value) => value.create_commit_ix(validator),
             ArgsTaskType::Finalize(value) => {
                 dlp::instruction_builder::finalize(
                     *validator,
@@ -163,13 +88,23 @@ 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),
                 )))
             }
-            // TODO (snawaz): discuss this with reviewers
-            ArgsTaskType::CommitDiff(_) => Err(self),
             ArgsTaskType::BaseAction(_)
             | ArgsTaskType::Finalize(_)
             | ArgsTaskType::Undelegate(_) => Err(self),
@@ -196,7 +131,6 @@ impl BaseTask for ArgsTask {
     fn compute_units(&self) -> u32 {
         match &self.task_type {
             ArgsTaskType::Commit(_) => 70_000,
-            ArgsTaskType::CommitDiff(_) => 65_000,
             ArgsTaskType::BaseAction(task) => task.action.compute_units,
             ArgsTaskType::Undelegate(_) => 70_000,
             ArgsTaskType::Finalize(_) => 70_000,
@@ -211,9 +145,6 @@ impl BaseTask for ArgsTask {
     fn task_type(&self) -> TaskType {
         match &self.task_type {
             ArgsTaskType::Commit(_) => TaskType::Commit,
-            // TODO (snawaz): What should we use here? Commit (in the sense of "category of task"), or add a
-            // new variant "CommitDiff" to indicate a specific instruction?
-            ArgsTaskType::CommitDiff(_) => TaskType::Commit,
             ArgsTaskType::BaseAction(_) => TaskType::Action,
             ArgsTaskType::Undelegate(_) => TaskType::Undelegate,
             ArgsTaskType::Finalize(_) => TaskType::Finalize,
@@ -226,7 +157,6 @@ impl BaseTask for ArgsTask {
     }
 
     fn reset_commit_id(&mut self, commit_id: u64) {
-        // TODO (snawaz): handle CommitDiff as well? what is it about?
         let ArgsTaskType::Commit(commit_task) = &mut self.task_type else {
             return;
         };