Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Severe issue of inconsistent instruction arg value. #2955

Closed
LXYY opened this issue May 9, 2024 · 10 comments
Closed

Severe issue of inconsistent instruction arg value. #2955

LXYY opened this issue May 9, 2024 · 10 comments
Labels

Comments

@LXYY
Copy link

LXYY commented May 9, 2024

Greetings.

I just observed a super weird instruction arg value inconsistency issue during development. It happened on my side in both Anchor 0.29.0 & 0.30.0

The program layout I have:

#[event_cpi]
#[derive(Accounts)]
#[instruction(
	amount: u64,
	from_deid: [u8; 16],
	to_deid: [u8; 16],
	request_id: [u8; 16],
)]
pub struct MakePaymentTransfer<'info> {
   ...
   #[account(
		seeds = [
			b"deid",
			from_deid.as_ref(),
		],
		bump = from_deid_account.bump,
		constraint = from_deid_account.deid == from_deid @DeIdProgramErrorCode::DeIdMismatch,
		constraint = from_deid != to_deid @DeIdProgramErrorCode::InvalidDeId,
	)]
  pub from_deid_account: Box<Account<'info, DeId>>,
  ...
}

pub fn handler(
  ctx: Context<MakePaymentTransfer>,
  amount: u64,
  from_deid: [u8; 16],
  to_deid: [u8; 16],
  request_id: [u8; 16],
) -> Result<()> {
  msg!("from_deid: {:?}", from_deid);
  msg!(
    "from_deid in account: {:?}",
    ctx.accounts.from_deid_account.deid
  );
  ...
}

The program logs I'm seeing:

Program log: from_deid: [48, 122, 0, 0, 3, 0, 0, 0, 120, 123, 0, 0, 3, 0, 0, 0]
Program log: from_deid in account: [141, 35, 38, 11, 227, 182, 69, 139, 137, 122, 57, 70, 20, 196, 114, 194]

As you can see, there is a constraint to check whether the from_deid_account.deid == from_deid that is satisfied with no issue. However within the instruction handler body, the from_deid arg value become inconsistent. This is a super disturbing observation as it makes me feel there could be some severe bugs within the Anchor framework.

Another disturbing fact is that: we didn't see the inconsistency issue in our previous revision, until when we added a new account to MakePaymentTransfer context. This suggests some uncertainty and flakiness for replacing the issue.

Please prioritize the investigation of this issue as it seems super severe.

@LXYY LXYY changed the title Severe issue of inconsistent arg value. Severe issue of inconsistent instruction arg value. May 9, 2024
@acheroncrypto
Copy link
Collaborator

Thanks for the report, but could you also include a full reproduction example, as I believe this issue is related to memory usage in your program, and cannot be isolated with only one account.

Another disturbing fact is that: we didn't see the inconsistency issue in our previous revision, until when we added a new account to MakePaymentTransfer context. This suggests some uncertainty and flakiness for replacing the issue.

I've first run into a similar issue during our Solana 1.18 upgrade in #2795, which was related to the memory and the Rust compiler. As far as I remember, when there were more accounts that the stack memory allowed, instead of failing, it continued to move forward in the instruction and failed in other spot due to the previous code not completing as expected, which was pretty lucky.

solana-labs/solana#35330 reduced the memory usage, and our examples continued to work again. However, the problem still seems to persist in some cases as your issue suggests.

If my reasoning is correct, this issue should also be reproducable in non-Anchor programs. It's just easier to hit this in Anchor programs due to comparatively higher memory usage.

@LXYY
Copy link
Author

LXYY commented May 9, 2024

Yeah that make sense. Any suggestions on how to walk around it?

@LXYY
Copy link
Author

LXYY commented May 9, 2024

FYI the full code:

use anchor_lang::prelude::*;
use anchor_spl::associated_token::AssociatedToken;
use anchor_spl::token::{Token, TokenAccount};

use crate::admin::state::GlobalStates;
use crate::common::{
  transfer_token, try_initialize_deid, validate_deid_account,
  DeIdProgramErrorCode,
};

#[event]
pub struct TokenPayment {
  pub request_id: [u8; 16],
  pub amount: u64,
  pub token_mint: Pubkey,
  pub from: [u8; 16],
  pub from_token_account: Pubkey,
  pub to: [u8; 16],
  pub to_token_account: Pubkey,
}

#[account]
#[derive(InitSpace, Default)]
pub struct DeId {
  pub bump: u8,
  pub initialized: bool,
  pub deid: [u8; 16],
}

#[account]
#[derive(InitSpace, Default)]
pub struct Identity {
  pub bump: u8,
  pub initialized: bool,
  #[max_len(150)]
  pub identity: String,
}

#[account]
#[derive(InitSpace, Default)]
pub struct TransferRequest {
  pub bump: u8,
  pub request_id: [u8; 16],
}

#[event_cpi]
#[derive(Accounts)]
#[instruction(
	amount: u64,
	from_deid: [u8; 16],
	to_deid: [u8; 16],
	request_id: [u8; 16],
)]
pub struct MakePaymentTransfer<'info> {
  #[account(
		seeds = [b"global_states"],
		bump = global_states.bump,
		constraint = amount > 0 @DeIdProgramErrorCode::InvalidTransferAmount,
		constraint = global_states.paused == false @DeIdProgramErrorCode::ProgramPaused,
	)]
  pub global_states: Box<Account<'info, GlobalStates>>,
  #[account(
		mut,
		constraint = relayer.key() == global_states.relayer_address @DeIdProgramErrorCode::InvalidRelayer,
	)]
  pub relayer: Signer<'info>,
  #[account(
		init,
		payer = relayer,
		space = 8 + TransferRequest::INIT_SPACE,
		seeds = [
			b"transfer_request",
			request_id.as_ref(),
		],
		bump,
	)]
  pub transfer_request: Box<Account<'info, TransferRequest>>,
  #[account(
		seeds = [
			b"deid",
			from_deid.as_ref(),
		],
		bump = from_deid_account.bump,
		constraint = from_deid_account.deid == from_deid @DeIdProgramErrorCode::DeIdMismatch,
		constraint = from_deid != to_deid @DeIdProgramErrorCode::InvalidDeId,
	)]
  pub from_deid_account: Box<Account<'info, DeId>>,
  #[account(
		mut,
		associated_token::mint = token_mint,
		associated_token::authority = from_deid_account,
		constraint = from_deid_token_account.amount >= amount @DeIdProgramErrorCode::InsufficientTokenBalance,
	)]
  pub from_deid_token_account: Box<Account<'info, TokenAccount>>,
  #[account(
		init_if_needed,
		payer = relayer,
		space = 8 + DeId::INIT_SPACE,
		seeds = [
			b"deid",
			to_deid.as_ref(),
		],
		bump,
	)]
  pub to_deid_account: Box<Account<'info, DeId>>,
  #[account(
		init_if_needed,
		payer = relayer,
		associated_token::mint = token_mint,
		associated_token::authority = to_deid_account,
	)]
  pub to_deid_token_account: Box<Account<'info, TokenAccount>>,
  /// CHECK: checked via constraint.
  pub token_mint: AccountInfo<'info>,
  pub token_program: Program<'info, Token>,
  pub associated_token_program: Program<'info, AssociatedToken>,
  pub system_program: Program<'info, System>,
}

pub fn handler(
  ctx: Context<MakePaymentTransfer>,
  amount: u64,
  from_deid: [u8; 16],
  to_deid: [u8; 16],
  request_id: [u8; 16],
) -> Result<()> {
  msg!("amount: {:?}", amount);
  msg!("from_deid: {:?}", from_deid);
  msg!("to_deid: {:?}", to_deid);
  msg!("request_id: {:?}", request_id);
  let request_account = &mut ctx.accounts.transfer_request;
  request_account.request_id = request_id.clone();
  request_account.bump = ctx.bumps.transfer_request;
  msg!("request_account: {:?}", request_account.key().to_string());
  let to_deid_account = &mut ctx.accounts.to_deid_account;
  try_initialize_deid(to_deid_account, &to_deid, ctx.bumps.to_deid_account);
  validate_deid_account(to_deid_account, &to_deid, ctx.bumps.to_deid_account)?;
  let bump_bytes = ctx.accounts.from_deid_account.bump.to_le_bytes();
  // let seeds = vec![b"deid".as_ref(), from_deid.as_ref(), bump_bytes.as_ref()];
  let seeds = vec![
    b"deid".as_ref(),
    ctx.accounts.from_deid_account.deid.as_ref(),
    bump_bytes.as_ref(),
  ];
  msg!(
    "from_deid_token_account_owner: {:?}",
    ctx.accounts.from_deid_token_account.owner.to_string(),
  );
  msg!(
    "from_deid_account: {:?}",
    ctx.accounts.from_deid_account.key().to_string()
  );
  msg!(
    "from_deid in account: {:?}",
    ctx.accounts.from_deid_account.deid
  );
  msg!("from_deid bump: {:?}", ctx.accounts.from_deid_account.bump);
  transfer_token(
    &ctx.accounts.token_program,
    &ctx.accounts.from_deid_token_account,
    &ctx.accounts.to_deid_token_account,
    ctx.accounts.from_deid_account.to_account_info(),
    amount,
    Some(seeds.as_ref()),
  )?;
  emit_cpi!(TokenPayment {
    request_id,
    amount,
    token_mint: ctx.accounts.token_mint.key(),
    from: from_deid,
    from_token_account: ctx.accounts.from_deid_token_account.key(),
    to: to_deid,
    to_token_account: ctx.accounts.to_deid_token_account.key(),
  });
  Ok(())
}

@LXYY
Copy link
Author

LXYY commented May 9, 2024

Update on this: I changed the instruction arg type from [u8; 16] to hex string with manual decode, and it works. Not sure whether it suggests an issue with array arg processing in Anchor.

@LXYY
Copy link
Author

LXYY commented May 9, 2024

Update on this: I changed the instruction arg type from [u8; 16] to hex string with manual decode, and it works. Not sure whether it suggests an issue with array arg processing in Anchor.

Based on this, probably not an issue with memory, as even the hex string doubled the memory usage of the args and still no issue.

@acheroncrypto
Copy link
Collaborator

Not sure whether it suggests an issue with array arg processing in Anchor.

Anchor doesn't have separate logic based on the parameter type you provide. They are all borsh deserialized. Also, removing an account wouldn't fix the problem if this was related to Anchor's argument processing.

@LXYY
Copy link
Author

LXYY commented May 12, 2024

Are you able to reproduce the issue?

@acheroncrypto
Copy link
Collaborator

Not from the code you shared since there is no client code, but I had similar examples before as I've mentioned earlier.

Need either a repo with both program and client code, or an example shared in Solana Playground to fully reproduce your problem.

For example using 1.18.0 program crate and CLI locally with this example code, you can reproduce a similar unexpected behavior.

@LucasSte
Copy link
Contributor

Seems to be related with anza-xyz/agave#1186.

@acheroncrypto
Copy link
Collaborator

Yeah, I suspected that to be the case in #2955 (comment), and now that we have more information on this topic, it's almost certain all these weird behavior issues are related to init constraints using more memory on Solana 1.18+.

Closing this as init constraints are no longer getting inlined after #2939.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants