diff --git a/Cargo.lock b/Cargo.lock index 2a660a505..6d7abdad0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3110,7 +3110,7 @@ dependencies = [ [[package]] name = "magicblock-delegation-program" version = "1.1.3" -source = "git+https://github.com/magicblock-labs/delegation-program.git?rev=3edb41022#3edb41022afb0e813607ffd4e79d1548131eab2b" +source = "git+https://github.com/magicblock-labs/delegation-program.git?rev=1874b4f5f5f55cb9ab54b64de2cc0d41107d1435#1874b4f5f5f55cb9ab54b64de2cc0d41107d1435" dependencies = [ "bincode", "borsh 1.6.0", @@ -3126,7 +3126,7 @@ dependencies = [ "solana-security-txt", "static_assertions", "strum", - "thiserror 2.0.17", + "thiserror 1.0.69", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 0a046d92f..a867b863d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -98,7 +98,7 @@ magicblock-committor-program = { path = "./magicblock-committor-program", featur magicblock-committor-service = { path = "./magicblock-committor-service" } magicblock-config = { path = "./magicblock-config" } magicblock-core = { path = "./magicblock-core" } -magicblock-delegation-program = { git = "https://github.com/magicblock-labs/delegation-program.git", rev = "3edb41022", features = [ +magicblock-delegation-program = { git = "https://github.com/magicblock-labs/delegation-program.git", rev = "1874b4f5f5f55cb9ab54b64de2cc0d41107d1435", features = [ "no-entrypoint", ] } magicblock-ledger = { path = "./magicblock-ledger" } diff --git a/magicblock-committor-service/src/intent_executor/error.rs b/magicblock-committor-service/src/intent_executor/error.rs index 9484be711..234420a54 100644 --- a/magicblock-committor-service/src/intent_executor/error.rs +++ b/magicblock-committor-service/src/intent_executor/error.rs @@ -176,8 +176,10 @@ impl TransactionStrategyExecutionError { signature: Option, tasks: &[Box], ) -> Result { - // There's always 2 budget instructions in front - const OFFSET: u8 = 2; + // There's always 3 budget instructions in front + // TODO (snawaz): this is offset-sensitive, if we add or remove any instruction from the + // front, that leads to incorrect error reporting. so if possible, make it offset-insensitive. + const OFFSET: u8 = 3; const NONCE_OUT_OF_ORDER: u32 = dlp::error::DlpError::NonceOutOfOrder as u32; diff --git a/magicblock-committor-service/src/tasks/args_task.rs b/magicblock-committor-service/src/tasks/args_task.rs index 1bd5f1e5d..db5e482d6 100644 --- a/magicblock-committor-service/src/tasks/args_task.rs +++ b/magicblock-committor-service/src/tasks/args_task.rs @@ -1,6 +1,11 @@ use dlp::{ args::{CallHandlerArgs, CommitDiffArgs, CommitStateArgs}, compute_diff, + instruction_builder::{ + call_handler_size_budget, commit_diff_size_budget, commit_size_budget, + finalize_size_budget, undelegate_size_budget, + }, + AccountSizeClass, }; use magicblock_metrics::metrics::LabelValue; use solana_account::ReadableAccount; @@ -177,6 +182,38 @@ impl BaseTask for ArgsTask { } } + fn accounts_size_budget(&self) -> u32 { + match &self.task_type { + ArgsTaskType::Commit(task) => { + commit_size_budget(AccountSizeClass::Dynamic( + task.committed_account.account.data.len() as u32, + )) + } + ArgsTaskType::CommitDiff(task) => { + commit_diff_size_budget(AccountSizeClass::Dynamic( + task.committed_account.account.data.len() as u32, + )) + } + ArgsTaskType::BaseAction(task) => { + // assume all other accounts are Small accounts. + let other_accounts_budget = + task.action.account_metas_per_program.len() as u32 + * AccountSizeClass::Small.size_budget(); + + call_handler_size_budget( + AccountSizeClass::Medium, + other_accounts_budget, + ) + } + ArgsTaskType::Undelegate(_) => { + undelegate_size_budget(AccountSizeClass::Huge) + } + ArgsTaskType::Finalize(_) => { + finalize_size_budget(AccountSizeClass::Huge) + } + } + } + #[cfg(test)] fn strategy(&self) -> TaskStrategy { TaskStrategy::Args diff --git a/magicblock-committor-service/src/tasks/buffer_task.rs b/magicblock-committor-service/src/tasks/buffer_task.rs index 7a1fb1890..73e23adb8 100644 --- a/magicblock-committor-service/src/tasks/buffer_task.rs +++ b/magicblock-committor-service/src/tasks/buffer_task.rs @@ -1,4 +1,7 @@ -use dlp::args::CommitStateFromBufferArgs; +use dlp::{ + args::CommitStateFromBufferArgs, instruction_builder::commit_size_budget, + AccountSizeClass, +}; use magicblock_committor_program::Chunks; use magicblock_metrics::metrics::LabelValue; use solana_instruction::Instruction; @@ -128,6 +131,14 @@ impl BaseTask for BufferTask { } } + fn accounts_size_budget(&self) -> u32 { + match self.task_type { + BufferTaskType::Commit(_) => { + commit_size_budget(AccountSizeClass::Huge) + } + } + } + #[cfg(test)] fn strategy(&self) -> TaskStrategy { TaskStrategy::Buffer diff --git a/magicblock-committor-service/src/tasks/mod.rs b/magicblock-committor-service/src/tasks/mod.rs index ea8b5c17a..2bc88b45d 100644 --- a/magicblock-committor-service/src/tasks/mod.rs +++ b/magicblock-committor-service/src/tasks/mod.rs @@ -85,6 +85,9 @@ pub trait BaseTask: Send + Sync + DynClone + LabelValue { /// Returns [`Task`] budget fn compute_units(&self) -> u32; + /// Returns the max accounts-data-size that can be used with SetLoadedAccountsDataSizeLimit + fn accounts_size_budget(&self) -> u32; + /// Returns current [`TaskStrategy`] #[cfg(test)] fn strategy(&self) -> TaskStrategy; diff --git a/magicblock-committor-service/src/tasks/task_strategist.rs b/magicblock-committor-service/src/tasks/task_strategist.rs index edbbd74d3..285cc1748 100644 --- a/magicblock-committor-service/src/tasks/task_strategist.rs +++ b/magicblock-committor-service/src/tasks/task_strategist.rs @@ -224,8 +224,12 @@ impl TaskStrategist { let placeholder = Keypair::new(); // Gather all involved keys in tx let budgets = TransactionUtils::tasks_compute_units(tasks); - let budget_instructions = - TransactionUtils::budget_instructions(budgets, u64::default()); + let size_budgets = TransactionUtils::tasks_accounts_size_budget(tasks); + let budget_instructions = TransactionUtils::budget_instructions( + budgets, + u64::default(), + size_budgets, + ); let unique_involved_pubkeys = TransactionUtils::unique_involved_pubkeys( tasks, &placeholder.pubkey(), @@ -258,8 +262,12 @@ impl TaskStrategist { tasks: &[Box], ) -> Vec { let budgets = TransactionUtils::tasks_compute_units(tasks); - let budget_instructions = - TransactionUtils::budget_instructions(budgets, u64::default()); + let size_budgets = TransactionUtils::tasks_accounts_size_budget(tasks); + let budget_instructions = TransactionUtils::budget_instructions( + budgets, + u64::default(), + size_budgets, + ); TransactionUtils::unique_involved_pubkeys( tasks, diff --git a/magicblock-committor-service/src/tasks/utils.rs b/magicblock-committor-service/src/tasks/utils.rs index 0846a08fc..a986ad98b 100644 --- a/magicblock-committor-service/src/tasks/utils.rs +++ b/magicblock-committor-service/src/tasks/utils.rs @@ -1,5 +1,6 @@ use std::collections::HashSet; +use dlp::DLP_PROGRAM_DATA_SIZE_CLASS; use solana_compute_budget_interface::ComputeBudgetInstruction; use solana_hash::Hash; use solana_instruction::Instruction; @@ -66,6 +67,7 @@ impl TransactionUtils { let budget_instructions = Self::budget_instructions( Self::tasks_compute_units(tasks), compute_unit_price, + Self::tasks_accounts_size_budget(tasks), ); let ixs = Self::tasks_instructions(&authority.pubkey(), tasks); Self::assemble_tx_raw( @@ -127,16 +129,38 @@ impl TransactionUtils { tasks.iter().map(|task| task.as_ref().compute_units()).sum() } + pub fn tasks_accounts_size_budget( + tasks: &[impl AsRef], + ) -> u32 { + if tasks.is_empty() { + return 0; + } + + let total_budget: u32 = tasks + .iter() + .map(|task| task.as_ref().accounts_size_budget()) + .sum(); + + // DLP_PROGRAM_DATA_SIZE_CLASS has been added N times, once for each task. + // We need to add it once only, so minus (N-1) times. + total_budget + - (tasks.len() as u32 - 1) + * DLP_PROGRAM_DATA_SIZE_CLASS.size_budget() + } + pub fn budget_instructions( compute_units: u32, compute_unit_price: u64, - ) -> [Instruction; 2] { - let compute_budget_ix = - ComputeBudgetInstruction::set_compute_unit_limit(compute_units); - let compute_unit_price_ix = + accounts_size_budget: u32, + ) -> [Instruction; 3] { + [ + ComputeBudgetInstruction::set_compute_unit_limit(compute_units), ComputeBudgetInstruction::set_compute_unit_price( compute_unit_price, - ); - [compute_budget_ix, compute_unit_price_ix] + ), + ComputeBudgetInstruction::set_loaded_accounts_data_size_limit( + accounts_size_budget, + ), + ] } } diff --git a/test-integration/Cargo.lock b/test-integration/Cargo.lock index 58baee9b2..5688f294d 100644 --- a/test-integration/Cargo.lock +++ b/test-integration/Cargo.lock @@ -2875,7 +2875,7 @@ dependencies = [ "log", "magicblock-config", "magicblock-core", - "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=3edb41022)", + "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=1874b4f5f5f55cb9ab54b64de2cc0d41107d1435)", "random-port", "rayon", "serde", @@ -3442,7 +3442,7 @@ dependencies = [ "lru", "magicblock-config", "magicblock-core", - "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=3edb41022)", + "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=1874b4f5f5f55cb9ab54b64de2cc0d41107d1435)", "magicblock-magic-program-api 0.5.2", "magicblock-metrics", "solana-account", @@ -3504,7 +3504,7 @@ dependencies = [ "log", "lru", "magicblock-committor-program", - "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=3edb41022)", + "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=1874b4f5f5f55cb9ab54b64de2cc0d41107d1435)", "magicblock-metrics", "magicblock-program", "magicblock-rpc-client", @@ -3588,7 +3588,7 @@ dependencies = [ [[package]] name = "magicblock-delegation-program" version = "1.1.3" -source = "git+https://github.com/magicblock-labs/delegation-program.git?rev=3edb41022#3edb41022afb0e813607ffd4e79d1548131eab2b" +source = "git+https://github.com/magicblock-labs/delegation-program.git?rev=1874b4f5f5f55cb9ab54b64de2cc0d41107d1435#1874b4f5f5f55cb9ab54b64de2cc0d41107d1435" dependencies = [ "bincode", "borsh 1.6.0", @@ -3604,7 +3604,7 @@ dependencies = [ "solana-security-txt", "static_assertions", "strum 0.27.2", - "thiserror 2.0.17", + "thiserror 1.0.69", ] [[package]] @@ -3826,7 +3826,7 @@ name = "magicblock-validator-admin" version = "0.5.2" dependencies = [ "log", - "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=3edb41022)", + "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=1874b4f5f5f55cb9ab54b64de2cc0d41107d1435)", "magicblock-program", "magicblock-rpc-client", "solana-commitment-config", @@ -4772,7 +4772,7 @@ version = "0.0.0" dependencies = [ "borsh 1.6.0", "ephemeral-rollups-sdk", - "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=3edb41022)", + "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=1874b4f5f5f55cb9ab54b64de2cc0d41107d1435)", "magicblock-magic-program-api 0.5.2", "rkyv 0.7.45", "solana-program", @@ -5765,7 +5765,7 @@ dependencies = [ "integration-test-tools", "log", "magicblock-core", - "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=3edb41022)", + "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=1874b4f5f5f55cb9ab54b64de2cc0d41107d1435)", "program-schedulecommit", "solana-program", "solana-rpc-client", @@ -5783,7 +5783,7 @@ dependencies = [ "log", "magicblock-committor-program", "magicblock-committor-service", - "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=3edb41022)", + "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=1874b4f5f5f55cb9ab54b64de2cc0d41107d1435)", "magicblock-program", "magicblock-rpc-client", "magicblock-table-mania", @@ -9993,7 +9993,7 @@ dependencies = [ "log", "magicblock-chainlink", "magicblock-config", - "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=3edb41022)", + "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=1874b4f5f5f55cb9ab54b64de2cc0d41107d1435)", "program-flexi-counter", "program-mini", "solana-account", @@ -10076,7 +10076,7 @@ dependencies = [ "log", "magicblock-accounts-db", "magicblock-config", - "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=3edb41022)", + "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=1874b4f5f5f55cb9ab54b64de2cc0d41107d1435)", "program-flexi-counter", "solana-rpc-client", "solana-sdk", @@ -10097,7 +10097,7 @@ dependencies = [ "magic-domain-program", "magicblock-api", "magicblock-config", - "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=3edb41022)", + "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=1874b4f5f5f55cb9ab54b64de2cc0d41107d1435)", "magicblock-program", "magicblock-validator-admin", "solana-rpc-client", @@ -10137,7 +10137,7 @@ version = "0.0.0" dependencies = [ "integration-test-tools", "log", - "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=3edb41022)", + "magicblock-delegation-program 1.1.3 (git+https://github.com/magicblock-labs/delegation-program.git?rev=1874b4f5f5f55cb9ab54b64de2cc0d41107d1435)", "program-flexi-counter", "solana-rpc-client-api", "solana-sdk", diff --git a/test-integration/Cargo.toml b/test-integration/Cargo.toml index 4b08d2730..6a528b2a0 100644 --- a/test-integration/Cargo.toml +++ b/test-integration/Cargo.toml @@ -61,7 +61,7 @@ magic-domain-program = { git = "https://github.com/magicblock-labs/magic-domain- "modular-sdk", ] } 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 = "3edb41022", features = [ +magicblock-delegation-program = { git = "https://github.com/magicblock-labs/delegation-program.git", rev = "1874b4f5f5f55cb9ab54b64de2cc0d41107d1435", features = [ "no-entrypoint", ] } magicblock-program = { path = "../programs/magicblock" } diff --git a/test-integration/test-committor-service/tests/test_intent_executor.rs b/test-integration/test-committor-service/tests/test_intent_executor.rs index 912668019..ca9bfa101 100644 --- a/test-integration/test-committor-service/tests/test_intent_executor.rs +++ b/test-integration/test-committor-service/tests/test_intent_executor.rs @@ -112,7 +112,7 @@ impl TestEnv { #[tokio::test] async fn test_commit_id_error_parsing() { const COUNTER_SIZE: u64 = 70; - const EXPECTED_ERR_MSG: &str = "Accounts committed with an invalid Commit id: Error processing Instruction 2: custom program error: 0xc"; + const EXPECTED_ERR_MSG: &str = "Accounts committed with an invalid Commit id: Error processing Instruction 3: custom program error: 0xc"; let TestEnv { fixture, @@ -153,17 +153,18 @@ async fn test_commit_id_error_parsing() { let execution_result = execution_result.unwrap(); assert!(execution_result.is_err()); let err = execution_result.unwrap_err(); - assert!(matches!( - err, - TransactionStrategyExecutionError::CommitIDError(_, _) - )); + assert!( + matches!(err, TransactionStrategyExecutionError::CommitIDError(_, _)), + "err: {:?}", + err + ); assert!(err.to_string().contains(EXPECTED_ERR_MSG)); } #[tokio::test] async fn test_undelegation_error_parsing() { const COUNTER_SIZE: u64 = 70; - const EXPECTED_ERR_MSG: &str = "Invalid undelegation: Error processing Instruction 4: custom program error: 0x7a."; + const EXPECTED_ERR_MSG: &str = "Invalid undelegation: Error processing Instruction 5: custom program error: 0x7a."; let TestEnv { fixture, @@ -212,7 +213,7 @@ async fn test_undelegation_error_parsing() { #[tokio::test] async fn test_action_error_parsing() { const COUNTER_SIZE: u64 = 70; - const EXPECTED_ERR_MSG: &str = "User supplied actions are ill-formed: Error processing Instruction 5: Program arithmetic overflowed"; + const EXPECTED_ERR_MSG: &str = "User supplied actions are ill-formed: Error processing Instruction 6: Program arithmetic overflowed"; let TestEnv { fixture, @@ -272,7 +273,7 @@ async fn test_action_error_parsing() { async fn test_cpi_limits_error_parsing() { const COUNTER_SIZE: u64 = 102; const COUNTER_NUM: u64 = 10; - const EXPECTED_ERR_MSG: &str = "Max instruction trace length exceeded: Error processing Instruction 26: Max instruction trace length exceeded"; + const EXPECTED_ERR_MSG: &str = "Max instruction trace length exceeded: Error processing Instruction 27: Max instruction trace length exceeded"; let TestEnv { fixture, @@ -361,7 +362,13 @@ async fn test_commit_id_error_recovery() { inner: res, patched_errors, } = res; - assert!(res.is_ok()); + + assert!( + res.is_ok(), + "res: {:?}, patched_errors: {:#?}", + res, + patched_errors + ); assert!(matches!(res.unwrap(), ExecutionOutput::SingleStage(_))); assert_eq!(patched_errors.len(), 1, "Only 1 patch expected"); diff --git a/test-integration/test-committor-service/tests/test_ix_commit_local.rs b/test-integration/test-committor-service/tests/test_ix_commit_local.rs index 6af953d47..d4ece7564 100644 --- a/test-integration/test-committor-service/tests/test_ix_commit_local.rs +++ b/test-integration/test-committor-service/tests/test_ix_commit_local.rs @@ -101,17 +101,17 @@ async fn test_ix_commit_order_book_change_100_bytes() { } #[tokio::test] -async fn test_ix_commit_order_book_change_679_bytes() { - commit_book_order_account(679, CommitStrategy::Args, false).await; +async fn test_ix_commit_order_book_change_671_bytes() { + commit_book_order_account(671, CommitStrategy::Args, false).await; } #[tokio::test] -async fn test_ix_commit_order_book_change_680_bytes() { - // We cannot use 680 as changed_len because that both 679 and 680 produce encoded tx - // of size 1644 (which is the max limit), but while the size of raw bytes for 679 is within - // 1232 limit, the size for 680 execeds by 1 (1233). That is why we used - // 681 as changed_len where CommitStrategy goes from Args to FromBuffer. - commit_book_order_account(681, CommitStrategy::FromBuffer, false).await; +async fn test_ix_commit_order_book_change_673_bytes() { + // We cannot use 672 as changed_len because that both 671 and 672 produce encoded tx + // of size 1644 (which is the max limit), but while the size of raw bytes for 671 is within + // 1232 limit, the size for 672 exceeds by 1 (1233). That is why we used + // 673 as changed_len where CommitStrategy goes from Args to FromBuffer. + commit_book_order_account(673, CommitStrategy::FromBuffer, false).await; } #[tokio::test] diff --git a/test-integration/test-committor-service/tests/utils/transactions.rs b/test-integration/test-committor-service/tests/utils/transactions.rs index 166850104..d22d21d17 100644 --- a/test-integration/test-committor-service/tests/utils/transactions.rs +++ b/test-integration/test-committor-service/tests/utils/transactions.rs @@ -69,11 +69,10 @@ macro_rules! get_account { } #[allow(dead_code)] -pub async fn tx_logs_contain( +pub async fn fetch_tx_logs( rpc_client: &RpcClient, signature: &Signature, - needle: &str, -) -> bool { +) -> Vec { // NOTE: we encountered the following error a few times which makes tests fail for the // wrong reason: // Error { @@ -113,25 +112,40 @@ pub async fn tx_logs_contain( } }; }; - let logs = tx - .transaction + tx.transaction .meta .as_ref() .unwrap() .log_messages .clone() - .unwrap_or_else(Vec::new); - logs.iter().any(|log| { - // Lots of existing tests pass "CommitState" as needle argument to this function, but since now CommitTask - // could invoke CommitState or CommitDiff depending on the size of the account, we also look for "CommitDiff" - // in the logs when needle == CommitState. It's easier to make this little adjustment here than computing - // the decision and passing either CommitState or CommitDiff from the tests themselves. - if needle == "CommitState" { - log.contains(needle) || log.contains("CommitDiff") - } else { - log.contains(needle) - } - }) + .unwrap_or_else(Vec::new) +} + +#[allow(dead_code)] +pub async fn print_tx_logs(rpc_client: &RpcClient, signature: &Signature) { + println!("logs: {:#?}", fetch_tx_logs(rpc_client, signature).await); +} + +#[allow(dead_code)] +pub async fn tx_logs_contain( + rpc_client: &RpcClient, + signature: &Signature, + needle: &str, +) -> bool { + fetch_tx_logs(rpc_client, signature) + .await + .iter() + .any(|log| { + // Lots of existing tests pass "CommitState" as needle argument to this function, but since now CommitTask + // could invoke CommitState or CommitDiff depending on the size of the account, we also look for "CommitDiff" + // in the logs when needle == CommitState. It's easier to make this little adjustment here than computing + // the decision and passing either CommitState or CommitDiff from the tests themselves. + if needle == "CommitState" { + log.contains(needle) || log.contains("CommitDiff") + } else { + log.contains(needle) + } + }) } /// This needs to be run for each test that required a new counter to be delegated