feat: added a way to verify validator identity within ER#1133
feat: added a way to verify validator identity within ER#1133
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughThis pull request introduces a new Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-magic-program-api/src/instruction.rs`:
- Around line 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).
In `@programs/magicblock/src/verify_validator_identity.rs`:
- Around line 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.
- Around line 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).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce461575-ae50-45d1-acf6-5288481e2219
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
magicblock-magic-program-api/src/instruction.rsprograms/magicblock/src/lib.rsprograms/magicblock/src/magicblock_processor.rsprograms/magicblock/src/verify_validator_identity.rs
|
|
||
| /// Verifies if account's pubkey corresponds to current validator identity | ||
| /// # Account references | ||
| /// - **0.** `[]` Validator Authority | ||
| VerifyValidatorIdentity, |
There was a problem hiding this comment.
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.
| /// 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).
| ic_msg!( | ||
| invoke_context, | ||
| "ScheduleCommit ERR: Magic program account not found" | ||
| ); |
There was a problem hiding this comment.
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.
| 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.
| 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()), | ||
| ] | ||
| } |
There was a problem hiding this comment.
🧹 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.
| 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).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7120c00c0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let expected_validator_identity = validator_authority_id(); | ||
| if validator_identity == &expected_validator_identity { | ||
| Ok(()) |
There was a problem hiding this comment.
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 👍 / 👎.
| transaction_context, | ||
| VALIDATOR_ACCOUNT_IDX, | ||
| )?; | ||
| let expected_validator_identity = validator_authority_id(); |
There was a problem hiding this comment.
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 👍 / 👎.
GabrielePicco
left a comment
GabrielePicco
left a comment
There was a problem hiding this comment.
The current implementation exposes the active validator identity directly to user programs and requires each callback handler to remember to authenticate it explicitly. “opt-in security” here is unavoidable, but could rework to make it much harder to mis-use/forget.
Additionally, the current implementation couples app behavior to validator identity / authority overrides, and pushes a security-sensitive check into every integrator's code.
I suggest reworking this so that the the service submit a DispatchCallback instruction to the magic program. The magic program validates internal state, then CPI’s into the destination program with invoke_signed using a deterministic callback-authority PDA. The app checks that PDA signer, not the active validator identity.
This keeps the trust boundary inside the magic program, and gives materially better developer experience with a much lower misuse risk as the program just need to validate that the PDA is a signer (with Anchor, just marking it as a signer).
To evaluate, but the PDA could also be scoped (derived) from the callback domain as well: intent_id, action_index, source_program.
Dodecahedr0x
left a comment
Dodecahedr0x
left a comment
There was a problem hiding this comment.
Magic context seems like a mistake here
| _signers: HashSet<Pubkey>, | ||
| invoke_context: &mut InvokeContext, | ||
| ) -> Result<(), InstructionError> { | ||
| const MAGIC_CONTEXT_IDX: u16 = 0; |
There was a problem hiding this comment.
Makes no sense to include magic context
| VALIDATOR_ACCOUNT_IDX, | ||
| )?; | ||
| let expected_validator_identity = validator_authority_id(); | ||
| if validator_identity == &expected_validator_identity { |
There was a problem hiding this comment.
Ix could also be idempotent but use return_data (depending on the use case)
3 participants
Summary
With callbacks it is importmant for developers to verify if callback was triggered by validator and not someone else. For that purpose they can now call VerifyValidatorIdentity in magicblock-program with "validator" account, magic-program will compare pubkeys and error if they do not match.
Compatibility
Testing
Checklist
Summary by CodeRabbit