Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions magicblock-magic-program-api/src/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,11 @@ pub enum MagicBlockInstruction {
/// - **0.** `[SIGNER]` Validator Authority
/// - **1.** `[WRITE]` Account to evict
EvictAccount { pubkey: Pubkey },

/// Verifies if account's pubkey corresponds to current validator identity
/// # Account references
/// - **0.** `[]` Validator Authority
VerifyValidatorIdentity,
Comment on lines +306 to +310
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Instruction documentation does not match the handler's expected account layout.

The handler in verify_validator_identity.rs expects:

  • Index 0: Magic Context account (MAGIC_CONTEXT_PUBKEY)
  • Index 1: Validator Authority

However, the documentation only lists the Validator Authority at index 0. This mismatch could cause integration issues for developers using this instruction.

📝 Suggested documentation fix
     /// Verifies if account's pubkey corresponds to current validator identity
     /// # Account references
-    /// - **0.** `[]`  Validator Authority
+    /// - **0.** `[]`  Magic Context account
+    /// - **1.** `[]`  Validator Authority
     VerifyValidatorIdentity,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// Verifies if account's pubkey corresponds to current validator identity
/// # Account references
/// - **0.** `[]` Validator Authority
VerifyValidatorIdentity,
/// Verifies if account's pubkey corresponds to current validator identity
/// # Account references
/// - **0.** `[]` Magic Context account
/// - **1.** `[]` Validator Authority
VerifyValidatorIdentity,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@magicblock-magic-program-api/src/instruction.rs` around lines 306 - 310,
Update the VerifyValidatorIdentity instruction docs to match the handler in
verify_validator_identity.rs: list index 0 as the Magic Context account
(MAGIC_CONTEXT_PUBKEY) and index 1 as the Validator Authority, and ensure the
account reference descriptions and ordering reflect that the handler expects the
magic context first and validator authority second (so the documented account
layout aligns with VerifyValidatorIdentity and MAGIC_CONTEXT_PUBKEY usage).

}

impl MagicBlockInstruction {
Expand Down
1 change: 1 addition & 0 deletions programs/magicblock/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ pub mod magicblock_processor;
pub mod test_utils;
mod utils;
pub mod validator;
pub mod verify_validator_identity;

pub use magic_sys::init_magic_sys;
pub use magicblock_magic_program_api::*;
Expand Down
4 changes: 4 additions & 0 deletions programs/magicblock/src/magicblock_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use crate::{
process_schedule_intent_bundle, ProcessScheduleCommitOptions,
},
toggle_executable_check::process_toggle_executable_check,
verify_validator_identity::process_verify_validator_identity,
};

pub const DEFAULT_COMPUTE_UNITS: u64 = 150;
Expand Down Expand Up @@ -214,6 +215,9 @@ declare_process_instruction!(
remote_slot,
authority,
),
VerifyValidatorIdentity => {
process_verify_validator_identity(signers, invoke_context)
}
}
}
);
187 changes: 187 additions & 0 deletions programs/magicblock/src/verify_validator_identity.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
use std::collections::HashSet;

use solana_instruction::error::InstructionError;
use solana_log_collector::ic_msg;
use solana_program_runtime::invoke_context::InvokeContext;
use solana_pubkey::Pubkey;

use crate::{
schedule_transactions, utils::accounts::get_instruction_pubkey_with_idx,
validator::validator_authority_id,
};

pub(crate) fn process_verify_validator_identity(
_signers: HashSet<Pubkey>,
invoke_context: &mut InvokeContext,
) -> Result<(), InstructionError> {
const MAGIC_CONTEXT_IDX: u16 = 0;
const VALIDATOR_ACCOUNT_IDX: u16 = MAGIC_CONTEXT_IDX + 1;

schedule_transactions::check_magic_context_id(
invoke_context,
MAGIC_CONTEXT_IDX,
)?;

let transaction_context = &invoke_context.transaction_context;
let ix_ctx = transaction_context.get_current_instruction_context()?;

// Assert MagicBlock program
ix_ctx
.find_index_of_program_account(transaction_context, &crate::id())
.ok_or_else(|| {
ic_msg!(
invoke_context,
"ScheduleCommit ERR: Magic program account not found"
);
Comment on lines +32 to +35
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Error message references wrong instruction.

The error message says "ScheduleCommit ERR" but this is the VerifyValidatorIdentity instruction handler. This appears to be a copy-paste artifact that could confuse debugging.

📝 Suggested fix
         .ok_or_else(|| {
             ic_msg!(
                 invoke_context,
-                "ScheduleCommit ERR: Magic program account not found"
+                "VerifyValidatorIdentity ERR: Magic program account not found"
             );
             InstructionError::UnsupportedProgramId
         })?;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ic_msg!(
invoke_context,
"ScheduleCommit ERR: Magic program account not found"
);
.ok_or_else(|| {
ic_msg!(
invoke_context,
"VerifyValidatorIdentity ERR: Magic program account not found"
);
InstructionError::UnsupportedProgramId
})?;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@programs/magicblock/src/verify_validator_identity.rs` around lines 32 - 35,
Update the error logging string in the VerifyValidatorIdentity handler so it
correctly references the instruction name instead of "ScheduleCommit"; locate
the ic_msg! call in the VerifyValidatorIdentity logic that currently logs
"ScheduleCommit ERR: Magic program account not found" and change it to a
descriptive message like "VerifyValidatorIdentity ERR: Magic program account not
found" (or similar) so logs correctly reflect the VerifyValidatorIdentity
instruction context.

InstructionError::UnsupportedProgramId
})?;

let validator_identity = get_instruction_pubkey_with_idx(
transaction_context,
VALIDATOR_ACCOUNT_IDX,
)?;
let expected_validator_identity = validator_authority_id();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Respect authority override when verifying validator identity

The expected key is taken from validator_authority_id() instead of the override-aware effective_validator_authority_id(), so verification can fail in replication/replay configurations that set validator_authority_override. In those modes this instruction will reject the active validator identity even though other validator checks in the codebase accept the override, making behavior inconsistent and breaking callback validation for those deployments.

Useful? React with 👍 / 👎.

if validator_identity == &expected_validator_identity {
Ok(())
Comment on lines +43 to +45
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Require validator signature for identity verification

This check only compares the provided account pubkey to the configured validator key and returns success without verifying signer privileges, so any caller can pass the validator pubkey as a readonly account and satisfy the instruction. That means callback programs using this as an authorization gate can be spoofed by non-validator transactions, which defeats the feature’s stated purpose of proving validator-originated execution.

Useful? React with 👍 / 👎.

} else {
Err(InstructionError::IncorrectAuthority)
}
}

#[cfg(test)]
mod tests {
use std::collections::HashMap;

use magicblock_magic_program_api::{
instruction::MagicBlockInstruction, MAGIC_CONTEXT_PUBKEY,
};
use serial_test::serial;
use solana_account::AccountSharedData;
use solana_instruction::{
error::InstructionError, AccountMeta, Instruction,
};
use solana_pubkey::Pubkey;
use solana_sdk_ids::system_program;

use crate::{
magic_context::MagicContext,
test_utils::{
ensure_started_validator, process_instruction, AUTHORITY_BALANCE,
},
validator::validator_authority_id,
};

fn verify_validator_identity_instruction(validator: Pubkey) -> Instruction {
let account_metas = vec![
AccountMeta::new_readonly(MAGIC_CONTEXT_PUBKEY, false),
AccountMeta::new_readonly(validator, false),
];
Instruction::new_with_bincode(
crate::id(),
&MagicBlockInstruction::VerifyValidatorIdentity,
account_metas,
)
}

fn setup_transaction_accounts() -> Vec<(Pubkey, AccountSharedData)> {
let mut account_data = HashMap::new();
account_data.insert(
MAGIC_CONTEXT_PUBKEY,
AccountSharedData::new(u64::MAX, MagicContext::SIZE, &crate::id()),
);
ensure_started_validator(&mut account_data, None);
let validator = validator_authority_id();
vec![
(
MAGIC_CONTEXT_PUBKEY,
account_data.remove(&MAGIC_CONTEXT_PUBKEY).unwrap(),
),
(validator, account_data.remove(&validator).unwrap()),
]
}
Comment on lines +86 to +101
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Test helper uses .unwrap() on HashMap::remove() which could silently fail.

While .unwrap() is acceptable in test code, if ensure_started_validator doesn't populate the expected keys, the test would panic with a generic message. Consider using .expect() with a descriptive message for better debugging.

💡 Optional improvement for test debuggability
         vec![
             (
                 MAGIC_CONTEXT_PUBKEY,
-                account_data.remove(&MAGIC_CONTEXT_PUBKEY).unwrap(),
+                account_data.remove(&MAGIC_CONTEXT_PUBKEY)
+                    .expect("MAGIC_CONTEXT_PUBKEY should be in account_data"),
             ),
-            (validator, account_data.remove(&validator).unwrap()),
+            (validator, account_data.remove(&validator)
+                .expect("validator should be in account_data")),
         ]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn setup_transaction_accounts() -> Vec<(Pubkey, AccountSharedData)> {
let mut account_data = HashMap::new();
account_data.insert(
MAGIC_CONTEXT_PUBKEY,
AccountSharedData::new(u64::MAX, MagicContext::SIZE, &crate::id()),
);
ensure_started_validator(&mut account_data, None);
let validator = validator_authority_id();
vec![
(
MAGIC_CONTEXT_PUBKEY,
account_data.remove(&MAGIC_CONTEXT_PUBKEY).unwrap(),
),
(validator, account_data.remove(&validator).unwrap()),
]
}
fn setup_transaction_accounts() -> Vec<(Pubkey, AccountSharedData)> {
let mut account_data = HashMap::new();
account_data.insert(
MAGIC_CONTEXT_PUBKEY,
AccountSharedData::new(u64::MAX, MagicContext::SIZE, &crate::id()),
);
ensure_started_validator(&mut account_data, None);
let validator = validator_authority_id();
vec![
(
MAGIC_CONTEXT_PUBKEY,
account_data.remove(&MAGIC_CONTEXT_PUBKEY)
.expect("MAGIC_CONTEXT_PUBKEY should be in account_data"),
),
(validator, account_data.remove(&validator)
.expect("validator should be in account_data")),
]
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@programs/magicblock/src/verify_validator_identity.rs` around lines 86 - 101,
In setup_transaction_accounts(), avoid using HashMap::remove().unwrap() which
yields an opaque panic; replace both unwrap() calls with expect(...) that
include descriptive messages referencing the keys and the helper that should
have populated them (e.g., mention MAGIC_CONTEXT_PUBKEY and the validator
returned by validator_authority_id()) so if ensure_started_validator(...) failed
to insert the entries the test failure will report which key was missing and
where (setup_transaction_accounts / ensure_started_validator).


#[test]
#[serial]
fn test_verify_validator_identity_success() {
let validator = validator_authority_id();
let ix = verify_validator_identity_instruction(validator);
let transaction_accounts = setup_transaction_accounts();
process_instruction(
&ix.data,
transaction_accounts,
ix.accounts,
Ok(()),
);
}

#[test]
#[serial]
fn test_verify_validator_identity_wrong_validator() {
let wrong_validator = Pubkey::new_unique();
let ix = verify_validator_identity_instruction(wrong_validator);

let mut account_data = HashMap::new();
account_data.insert(
MAGIC_CONTEXT_PUBKEY,
AccountSharedData::new(u64::MAX, MagicContext::SIZE, &crate::id()),
);
ensure_started_validator(&mut account_data, None);

let transaction_accounts = vec![
(
MAGIC_CONTEXT_PUBKEY,
account_data.remove(&MAGIC_CONTEXT_PUBKEY).unwrap(),
),
(
wrong_validator,
AccountSharedData::new(
AUTHORITY_BALANCE,
0,
&system_program::id(),
),
),
];

process_instruction(
&ix.data,
transaction_accounts,
ix.accounts,
Err(InstructionError::IncorrectAuthority),
);
}

#[test]
#[serial]
fn test_verify_validator_identity_wrong_magic_context() {
let validator = validator_authority_id();
let wrong_context = Pubkey::new_unique();

let account_metas = vec![
AccountMeta::new_readonly(wrong_context, false),
AccountMeta::new_readonly(validator, false),
];
let ix = Instruction::new_with_bincode(
crate::id(),
&MagicBlockInstruction::VerifyValidatorIdentity,
account_metas,
);

let mut account_data = HashMap::new();
ensure_started_validator(&mut account_data, None);

let transaction_accounts = vec![
(
wrong_context,
AccountSharedData::new(0, 0, &system_program::id()),
),
(validator, account_data.remove(&validator).unwrap()),
];

process_instruction(
&ix.data,
transaction_accounts,
ix.accounts,
Err(InstructionError::MissingAccount),
);
}
}
Loading