Skip to content

Commit c05f313

Browse files
snawazsnawaz
committed
Redesign the implementation of CommitDiff support
1 parent 9cf7e54 commit c05f313

File tree

11 files changed

+75
-148
lines changed

11 files changed

+75
-148
lines changed

‎magicblock-accounts/src/scheduled_commits_processor.rs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,6 @@ impl ScheduledCommitsProcessorImpl {
343343
included_pubkeys: intent_meta.included_pubkeys,
344344
excluded_pubkeys: intent_meta.excluded_pubkeys,
345345
requested_undelegation: intent_meta.requested_undelegation,
346-
commit_diff: intent_meta.commit_diff,
347346
}
348347
}
349348
}
@@ -413,7 +412,6 @@ struct ScheduledBaseIntentMeta {
413412
excluded_pubkeys: Vec<Pubkey>,
414413
intent_sent_transaction: Transaction,
415414
requested_undelegation: bool,
416-
commit_diff: bool,
417415
}
418416

419417
impl ScheduledBaseIntentMeta {
@@ -431,7 +429,6 @@ impl ScheduledBaseIntentMeta {
431429
excluded_pubkeys,
432430
intent_sent_transaction: intent.action_sent_transaction.clone(),
433431
requested_undelegation: intent.is_undelegate(),
434-
commit_diff: intent.is_commit_diff(),
435432
}
436433
}
437434
}

‎magicblock-committor-service/src/tasks/args_task.rs‎

Lines changed: 49 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ use crate::{
2727
#[derive(Clone)]
2828
pub enum ArgsTaskType {
2929
Commit(CommitTask),
30-
CommitDiff(CommitTask),
3130
Finalize(FinalizeTask),
3231
Undelegate(UndelegateTask), // Special action really
3332
BaseAction(BaseActionTask),
@@ -71,55 +70,55 @@ impl BaseTask for ArgsTask {
7170
args,
7271
)
7372
}
74-
ArgsTaskType::CommitDiff(value) => {
75-
let chain_config =
76-
ChainConfig::local(ComputeBudgetConfig::new(1_000_000));
77-
78-
let rpc_client = RpcClient::new_with_commitment(
79-
chain_config.rpc_uri.to_string(),
80-
CommitmentConfig {
81-
commitment: chain_config.commitment,
82-
},
83-
);
84-
85-
let account = match rpc_client
86-
.get_account(&value.committed_account.pubkey)
87-
{
88-
Ok(account) => account,
89-
Err(e) => {
90-
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);
91-
let args = CommitStateArgs {
92-
nonce: value.commit_id,
93-
lamports: value.committed_account.account.lamports,
94-
data: value.committed_account.account.data.clone(),
95-
allow_undelegation: value.allow_undelegation,
96-
};
97-
return dlp::instruction_builder::commit_state(
98-
*validator,
99-
value.committed_account.pubkey,
100-
value.committed_account.account.owner,
101-
args,
102-
);
103-
}
104-
};
105-
106-
let args = CommitDiffArgs {
107-
nonce: value.commit_id,
108-
lamports: value.committed_account.account.lamports,
109-
diff: compute_diff(
110-
account.data(),
111-
value.committed_account.account.data(),
112-
)
113-
.to_vec(),
114-
allow_undelegation: value.allow_undelegation,
115-
};
116-
dlp::instruction_builder::commit_diff(
117-
*validator,
118-
value.committed_account.pubkey,
119-
value.committed_account.account.owner,
120-
args,
121-
)
122-
}
73+
// ArgsTaskType::CommitDiff(value) => {
74+
// let chain_config =
75+
// ChainConfig::local(ComputeBudgetConfig::new(1_000_000));
76+
77+
// let rpc_client = RpcClient::new_with_commitment(
78+
// chain_config.rpc_uri.to_string(),
79+
// CommitmentConfig {
80+
// commitment: chain_config.commitment,
81+
// },
82+
// );
83+
84+
// let account = match rpc_client
85+
// .get_account(&value.committed_account.pubkey)
86+
// {
87+
// Ok(account) => account,
88+
// Err(e) => {
89+
// 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);
90+
// let args = CommitStateArgs {
91+
// nonce: value.commit_id,
92+
// lamports: value.committed_account.account.lamports,
93+
// data: value.committed_account.account.data.clone(),
94+
// allow_undelegation: value.allow_undelegation,
95+
// };
96+
// return dlp::instruction_builder::commit_state(
97+
// *validator,
98+
// value.committed_account.pubkey,
99+
// value.committed_account.account.owner,
100+
// args,
101+
// );
102+
// }
103+
// };
104+
105+
// let args = CommitDiffArgs {
106+
// nonce: value.commit_id,
107+
// lamports: value.committed_account.account.lamports,
108+
// diff: compute_diff(
109+
// account.data(),
110+
// value.committed_account.account.data(),
111+
// )
112+
// .to_vec(),
113+
// allow_undelegation: value.allow_undelegation,
114+
// };
115+
// dlp::instruction_builder::commit_diff(
116+
// *validator,
117+
// value.committed_account.pubkey,
118+
// value.committed_account.account.owner,
119+
// args,
120+
// )
121+
// }
123122
ArgsTaskType::Finalize(value) => {
124123
dlp::instruction_builder::finalize(
125124
*validator,
@@ -168,8 +167,6 @@ impl BaseTask for ArgsTask {
168167
BufferTaskType::Commit(value),
169168
)))
170169
}
171-
// TODO (snawaz): discuss this with reviewers
172-
ArgsTaskType::CommitDiff(_) => Err(self),
173170
ArgsTaskType::BaseAction(_)
174171
| ArgsTaskType::Finalize(_)
175172
| ArgsTaskType::Undelegate(_) => Err(self),
@@ -196,7 +193,6 @@ impl BaseTask for ArgsTask {
196193
fn compute_units(&self) -> u32 {
197194
match &self.task_type {
198195
ArgsTaskType::Commit(_) => 70_000,
199-
ArgsTaskType::CommitDiff(_) => 65_000,
200196
ArgsTaskType::BaseAction(task) => task.action.compute_units,
201197
ArgsTaskType::Undelegate(_) => 70_000,
202198
ArgsTaskType::Finalize(_) => 70_000,
@@ -211,9 +207,6 @@ impl BaseTask for ArgsTask {
211207
fn task_type(&self) -> TaskType {
212208
match &self.task_type {
213209
ArgsTaskType::Commit(_) => TaskType::Commit,
214-
// TODO (snawaz): What should we use here? Commit (in the sense of "category of task"), or add a
215-
// new variant "CommitDiff" to indicate a specific instruction?
216-
ArgsTaskType::CommitDiff(_) => TaskType::Commit,
217210
ArgsTaskType::BaseAction(_) => TaskType::Action,
218211
ArgsTaskType::Undelegate(_) => TaskType::Undelegate,
219212
ArgsTaskType::Finalize(_) => TaskType::Finalize,
@@ -226,7 +219,6 @@ impl BaseTask for ArgsTask {
226219
}
227220

228221
fn reset_commit_id(&mut self, commit_id: u64) {
229-
// TODO (snawaz): handle CommitDiff as well? what is it about?
230222
let ArgsTaskType::Commit(commit_task) = &mut self.task_type else {
231223
return;
232224
};

‎magicblock-committor-service/src/tasks/task_builder.rs‎

Lines changed: 20 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -47,29 +47,24 @@ impl TasksBuilder for TaskBuilderImpl {
4747
base_intent: &ScheduledBaseIntent,
4848
persister: &Option<P>,
4949
) -> TaskBuilderResult<Vec<Box<dyn BaseTask>>> {
50-
let (accounts, allow_undelegation, commit_diff) =
51-
match &base_intent.base_intent {
52-
MagicBaseIntent::BaseActions(actions) => {
53-
let tasks = actions
54-
.iter()
55-
.map(|el| {
56-
let task = BaseActionTask { action: el.clone() };
57-
let task =
58-
ArgsTask::new(ArgsTaskType::BaseAction(task));
59-
Box::new(task) as Box<dyn BaseTask>
60-
})
61-
.collect();
62-
return Ok(tasks);
63-
}
64-
MagicBaseIntent::Commit(t) => {
65-
(t.get_committed_accounts(), false, t.is_commit_diff())
66-
}
67-
MagicBaseIntent::CommitAndUndelegate(t) => (
68-
t.commit_action.get_committed_accounts(),
69-
true,
70-
t.commit_action.is_commit_diff(),
71-
),
72-
};
50+
let (accounts, allow_undelegation) = match &base_intent.base_intent {
51+
MagicBaseIntent::BaseActions(actions) => {
52+
let tasks = actions
53+
.iter()
54+
.map(|el| {
55+
let task = BaseActionTask { action: el.clone() };
56+
let task =
57+
ArgsTask::new(ArgsTaskType::BaseAction(task));
58+
Box::new(task) as Box<dyn BaseTask>
59+
})
60+
.collect();
61+
return Ok(tasks);
62+
}
63+
MagicBaseIntent::Commit(t) => (t.get_committed_accounts(), false),
64+
MagicBaseIntent::CommitAndUndelegate(t) => {
65+
(t.commit_action.get_committed_accounts(), true)
66+
}
67+
};
7368

7469
let committed_pubkeys = accounts
7570
.iter()
@@ -93,17 +88,11 @@ impl TasksBuilder for TaskBuilderImpl {
9388
.iter()
9489
.map(|account| {
9590
let commit_id = *commit_ids.get(&account.pubkey).expect("CommitIdFetcher provide commit ids for all listed pubkeys, or errors!");
96-
let task = CommitTask {
91+
let task = ArgsTaskType::Commit(CommitTask {
9792
commit_id,
9893
allow_undelegation,
9994
committed_account: account.clone(),
100-
};
101-
let task = if commit_diff {
102-
ArgsTaskType::CommitDiff(task)
103-
} else {
104-
ArgsTaskType::Commit(task)
105-
};
106-
95+
});
10796
Box::new(ArgsTask::new(task)) as Box<dyn BaseTask>
10897
})
10998
.collect();
@@ -143,9 +132,6 @@ impl TasksBuilder for TaskBuilderImpl {
143132
CommitType::Standalone(accounts) => {
144133
accounts.iter().map(finalize_task).collect()
145134
}
146-
CommitType::StandaloneDiff(accounts) => {
147-
accounts.iter().map(finalize_task).collect()
148-
}
149135
CommitType::WithBaseActions {
150136
committed_accounts,
151137
base_actions,

‎magicblock-committor-service/src/tasks/task_strategist.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,7 @@ impl TaskStrategist {
157157
) -> Result<usize, SignerError> {
158158
// Get initial transaction size
159159
let calculate_tx_length = |tasks: &[Box<dyn BaseTask>]| {
160+
// TODO (snawaz): we seem to discard lots of heavy computations here
160161
match TransactionUtils::assemble_tasks_tx(
161162
&Keypair::new(), // placeholder
162163
tasks,

‎magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ where
2828
PersistorContext::PersistStrategy { uses_lookup_tables } => {
2929
let commit_task = match &task.task_type {
3030
ArgsTaskType::Commit(commit_task) => commit_task,
31-
ArgsTaskType::CommitDiff(commit_task) => commit_task,
3231
_ => return,
3332
};
3433

‎magicblock-magic-program-api/src/instruction.rs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,6 @@ pub enum MagicBlockInstruction {
107107
/// # Account references
108108
/// - **0.** `[SIGNER]` Validator authority
109109
EnableExecutableCheck,
110-
111-
ScheduleCommitDiffAndUndelegate,
112110
}
113111

114112
impl MagicBlockInstruction {

‎programs/magicblock/src/magic_scheduled_base_intent.rs‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -101,10 +101,6 @@ impl ScheduledBaseIntent {
101101
self.base_intent.is_undelegate()
102102
}
103103

104-
pub fn is_commit_diff(&self) -> bool {
105-
self.base_intent.is_commit_diff()
106-
}
107-
108104
pub fn is_empty(&self) -> bool {
109105
self.base_intent.is_empty()
110106
}
@@ -152,16 +148,6 @@ impl MagicBaseIntent {
152148
}
153149
}
154150

155-
pub fn is_commit_diff(&self) -> bool {
156-
match &self {
157-
MagicBaseIntent::BaseActions(_) => false,
158-
MagicBaseIntent::Commit(c) => c.is_commit_diff(),
159-
MagicBaseIntent::CommitAndUndelegate(c) => {
160-
c.commit_action.is_commit_diff()
161-
}
162-
}
163-
}
164-
165151
pub fn get_committed_accounts(&self) -> Option<&Vec<CommittedAccount>> {
166152
match self {
167153
MagicBaseIntent::BaseActions(_) => None,
@@ -318,7 +304,6 @@ impl<'a> From<CommittedAccountRef<'a>> for CommittedAccount {
318304
pub enum CommitType {
319305
/// Regular commit without actions
320306
Standalone(Vec<CommittedAccount>), // accounts to commit
321-
StandaloneDiff(Vec<CommittedAccount>), // accounts to commit
322307
/// Commits accounts and runs actions
323308
WithBaseActions {
324309
committed_accounts: Vec<CommittedAccount>,
@@ -445,14 +430,9 @@ impl CommitType {
445430
}
446431
}
447432

448-
pub fn is_commit_diff(&self) -> bool {
449-
matches!(self, Self::StandaloneDiff(_))
450-
}
451-
452433
pub fn get_committed_accounts(&self) -> &Vec<CommittedAccount> {
453434
match self {
454435
Self::Standalone(committed_accounts) => committed_accounts,
455-
Self::StandaloneDiff(committed_accounts) => committed_accounts,
456436
Self::WithBaseActions {
457437
committed_accounts, ..
458438
} => committed_accounts,
@@ -462,7 +442,6 @@ impl CommitType {
462442
pub fn get_committed_accounts_mut(&mut self) -> &mut Vec<CommittedAccount> {
463443
match self {
464444
Self::Standalone(committed_accounts) => committed_accounts,
465-
Self::StandaloneDiff(committed_accounts) => committed_accounts,
466445
Self::WithBaseActions {
467446
committed_accounts, ..
468447
} => committed_accounts,
@@ -474,9 +453,6 @@ impl CommitType {
474453
Self::Standalone(committed_accounts) => {
475454
committed_accounts.is_empty()
476455
}
477-
Self::StandaloneDiff(committed_accounts) => {
478-
committed_accounts.is_empty()
479-
}
480456
Self::WithBaseActions {
481457
committed_accounts, ..
482458
} => committed_accounts.is_empty(),

‎programs/magicblock/src/magicblock_processor.rs‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ declare_process_instruction!(
3333
let instruction_context =
3434
transaction_context.get_current_instruction_context()?;
3535
let signers = instruction_context.get_signers(transaction_context)?;
36-
3736
match instruction {
3837
ModifyAccounts(mut account_mods) => process_mutate_accounts(
3938
signers,
@@ -46,15 +45,13 @@ declare_process_instruction!(
4645
invoke_context,
4746
ProcessScheduleCommitOptions {
4847
request_undelegation: false,
49-
request_diff: false,
5048
},
5149
),
5250
ScheduleCommitAndUndelegate => process_schedule_commit(
5351
signers,
5452
invoke_context,
5553
ProcessScheduleCommitOptions {
5654
request_undelegation: true,
57-
request_diff: false,
5855
},
5956
),
6057
AcceptScheduleCommits => {
@@ -82,14 +79,6 @@ declare_process_instruction!(
8279
EnableExecutableCheck => {
8380
process_toggle_executable_check(signers, invoke_context, true)
8481
}
85-
ScheduleCommitDiffAndUndelegate => process_schedule_commit(
86-
signers,
87-
invoke_context,
88-
ProcessScheduleCommitOptions {
89-
request_undelegation: true,
90-
request_diff: true,
91-
},
92-
),
9382
}
9483
}
9584
);

0 commit comments

Comments
 (0)