lang: Add QuickCheck<T> that checks owner (and other invariants) quickly, avoiding complete deserialization and consuming memory #2670
Conversation
…full deserialization
|
@snawaz is attempting to deploy a commit to the coral-xyz Team on Vercel. A member of the Team first needs to authorize it. |
|
This PR solves the problem in better way than a similar PR (#1475 created a long back) tried to solve (which does not compile, as noted by @armaniferrante ). |
|
Why do you not use a new account type for such cases? It would improve the readability of the code. Is this intentional? |
In terms of speed and quickness, it has zero advantage. |
How would you write an equivalent of How about If I use new account (wrapper?) type approach more than a couple of times, I'd quickly start feeling the verbosity without much readability improvement. QuickCheck is already readable, as you can see the real account type, meant to be used if you needed full deserialized account. |
Maybe my question is not very clear. I just mean this syntax: Against |
ah, I see. I thought about this solution as well. I've created another PR which implements However, @acheroncrypto probably thinks otherwise (as per his feedbacks on the other PR). Let me know what your current thoughts are. I'd happily work on that, if that is more desirable! (and hoping then my We could try implementing a third-solution: #[account(quick_check)]
safe_and_fast: Account<'info, Mint>,But my guess is that all these alternative solutions require more code and more changes to the existing codebase. |
Overall fair point.
Well, that's the intent because it's unsafe! While it may be true that using To illustrate this, let's take a test case you've added: anchor/lang/src/quick_check.rs Lines 102 to 108 in 894f6bd - let mut good_mint_data = ([0; 1024], 10959, Mint::owner());
+ struct WrongAccount;
+ impl anchor_lang::Owner for WrongAccount {
+ fn owner() -> Pubkey {
+ spl_token::ID
+ }
+ }
+
+ let mut good_mint_data = ([0; 1024], 10959, WrongAccount::owner());Make these changes and the test would still pass even though we are using
#[account(owner = spl_token::ID)]
my_account: UncheckedAccount<'info>,is much more readable and understandable compared to my_account: Account<'info, QuickCheck<Mint>>,
Given the idiomatic way of doing account validation in Anchor is with
The type At the end of the day, this all comes down to personal preference. Anchor, being an opinionated framework, has idiomatic way of doing things and allowing multiple ways of doing the same thing usually results in people getting confused rather than the intended effect of flexibility. |
Yes, the test would pass because Now if you think deserialization ensures some safety, then that's not true at all. Deserialization merely interprets buffer as the given type and for most types you'll get some value (even if it does not make sense to you) without any failure — it might fail at later stage though when you try to use the deserialized type but that's a different topic altogether. The following test defines 4 types, one of which is #[account]
#[derive(InitSpace, Debug)]
pub struct Correct {
pub simple: u32,
#[max_len(4)]
pub complicated: Vec<u32>,
}
#[account]
#[derive(InitSpace, Debug)]
pub struct Wrong1 {
pub same_size_array: [u32; Correct::INIT_SPACE],
}
#[account]
#[derive(InitSpace, Debug)]
pub struct Wrong2 {
pub smaller_size_array: [u32; Correct::INIT_SPACE / 2],
}
#[account]
#[derive(InitSpace, Debug)]
pub struct Wrong3 {
pub signer_bump: u8,
pub usdc_balance: u64,
pub deposit_count: u32,
pub withdraw_count: u32,
}
#[test]
fn test_deserialization_does_not_ensure_safety() {
let mut data = ([0; 1024], 10959, crate::ID);
let info = new_account_info(&mut data);
let correct = Account::<Correct>::try_from(&info);
assert!(correct.is_ok(), "correct: {:?}", correct);
let wrong1 = Account::<Wrong1>::try_from(&info);
assert!(wrong1.is_ok(), "wrong1: {:?}", wrong1);
let wrong2 = Account::<Wrong2>::try_from(&info);
assert!(wrong2.is_ok(), "wrong2: {:?}", wrong2);
let wrong3 = Account::<Wrong3>::try_from(&info);
assert!(wrong3.is_ok(), "wrong3: {:?}", wrong3);
}You can think of this another way: if the deserialization to the type would have provided any safety-guarantee, then Anchor would not have the notion of "discriminator". It's the discriminator which ensures safety! Also, |
Yes, I thought of using #[account(quick_check)]
safe_and_fast: Account<'info, Mint>,But later I realized, it wont work, because all of the members of
In the previous comment, I explained the type |
Is the |
I took it as example. If you're referring to the real
Does struct WrongAccount;
// some impl deserialization
impl anchor_lang::Owner for WrongAccount {
fn owner() -> Pubkey {
spl_token::ID
}
} |
This is incorrect — not checking the non-existent discriminator does not make them equally safe or unsafe. The following will ONLY accept #[derive(Accounts)]
pub struct MintAccountIx<'info> {
pub mint: Account<'info, Mint>,
}in contrast, the following(with #[derive(Accounts)]
pub struct QuickCheckMintAccountIx<'info> {
pub quick_check_mint: Account<'info, QuickCheck<Mint>>,
}Not having a discriminator does not make an account unsafe as Native programs have their own way of making sure the accounts are uniquely deserialized. We also do not blindly interpret the account data for the wrapper accounts like you might assume. As shown by the example above, inputting a token account to the Which part of being able to input a different account type than the one specified in the program being unsafe is unclear to you? |
You missed the following two points that I mentioned about the verification:
Yes, because it's not complete. Here are my thoughts:
|
|
I changed the implementation and now it uses full-deserialization to verify the accounts, just like An alternative solution could to have struct CheckedAccount<'info, T> {
info: AccountInfo<'info>,
_marker: PhantomData<T>,
}It's much more work though. |
|
I wonder why an unrelated code is failing due to 
|
for best experience, review commit by commit.
Problem
If a program requires few accounts to be used in CPI calls (or any scenario that uses
.to_account_info()only on the accounts), then the developers have 3 choices:choice1is safe but it's overkill if program never needs to theMintobject itself. It could be slow, as it completely deserializes the account. The biggest problem is, it consumes stack (or heap) memory unnecessarily!choice2andchoice3are fast (no deserialization at all) but unsafe — and requiresCHECK:doc comment as well.All choices are suboptimal —
choice1, despite being safe, is the worst, as it's memory hungry!Solution
QuickCheck<T>solves this satisfactorily (at least to me and we're using this solution in production).safeandfast, as it checks the owner of the account (and if there is any discriminator, that will be automatically checked by the Anchor framework already).CHECK:doc comment either.More
In some cases, we do not have any
#[account]types (such asMinttype above), instead we haveAccountInfoonly but we still want to (rather must) check owner of such account_infos. To support this, this PR also implementstrait Ownerfor the program type that enables us to write this:In this code,
CandyMachineis not account though — it's a program type. But we can use it to ensure thatsome_accountis owned byCandyMachine.Tests
I've added tests to cover both scenarios discussed above.
Docs
If this PR looks good to reviewers (and eventually acceptable), I'll also add the docs. So please let know.