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

Issue: invoke_transfer_checked failed with out of memory for 2 transfers with hook #7004

Open
makarychev opened this issue Jul 10, 2024 · 7 comments
Assignees

Comments

@makarychev
Copy link

makarychev commented Jul 10, 2024

Problem:
It is possible to brake external programs which use token extension with transfer hook if it requires more than 6 additional extra account meta.

sdk function invoke_transfer_checked use some part of data allocation on heap with Vector by pushing new elements into it dynamically while resolving extra account meta data.
So I suppose that during the preparation of accounts for extra accounts and pushing into Vectors it consumes a huge portion of heap memory.
Vectors creation:

Pushing into vectors:

The simple transfer hook program which fails with the out of memory due to intensive allocation inside invoke_transfer_checked https://github.com/makarychev/solana-transfer-extensions/
Failing integration test: https://github.com/makarychev/solana-transfer-extensions/actions/runs/9844698072/job/27178656558#step:9:74
Program log: Error: memory allocation failed, out of memory
Instruction code: https://github.com/makarychev/solana-transfer-extensions/blob/main/programs/transfer-extensions/src/instructions/multi_transfers.rs#L40
Transfer Hook code: https://github.com/makarychev/solana-transfer-extensions/blob/main/programs/transfer-hook/src/instructions/handler.rs#L41

Questions:
How can we send 2 transfers from 1 external program instruction if transfer hook has 8 extra account? Does transfer with transfer hook execution have constraint on extra accounts count?
Could invoke_transfer_checked be optimized or in such scenario or it is required to fill extra accounts manually (not by invoke_transfer_checked)?
Probably it is possible to execute transfer with transfer hook more efficient?

@buffalojoec
Copy link
Contributor

@makarychev Thanks for flagging. You're correct that the vector allocation when creating the CPI syscall is going to eat up memory, but it should depend on the size of the extra accounts, not the quantity.

If your hook program requires only small accounts - like system accounts - then it can include many more than 6. However, if you require a handful of program data accounts (for example), then this will definitely eat up a lot of heap, since the vector will contain AccountInfo objects with large data fields.

Out of curiosity, what are the 6 accounts you're using for the hook? Are they huge?

Regardless, we should probably benchmark this and document the constraint somewhere. I doubt there is much we can do about the upper bound of vector size, though.

@makarychev
Copy link
Author

makarychev commented Jul 10, 2024

thanks @buffalojoec for prompt response
In my example I used 8 extra accounts:

    pub additional_account_1: Program<'info, TransferExtensions>,
    pub wallet_counter_in_from: Account<'info, WalletCounterIn>,
    pub wallet_counter_in_to: Account<'info, WalletCounterIn>,
    pub wallet_counter_out_from: Account<'info, WalletCounterOut>,
    pub wallet_counter_out_to: Account<'info, WalletCounterOut>,
    pub mint_counter_in: Account<'info, MintCounterIn>,
    pub mint_counter_out: Account<'info, MintCounterOut>,
    pub global_program_data: Account<'info, GlobalProgramData>,

wallet_* and mint_* accounts u64 + Pubkey each one
global_program_data: u64

  • additional_account_1 which is a Program account
    So all accounts pretty small...

@makarychev
Copy link
Author

makarychev commented Jul 16, 2024

The accounts data len is next:

'Program log: account index [0], data len: 175',
    'Program log: account index [1], data len: 234',
    'Program log: account index [2], data len: 175',
    'Program log: account index [3], data len: 0',
    'Program log: account index [4], data len: 296',
    'Program log: account index [5], data len: 36',
    'Program log: account index [6], data len: 48',
    'Program log: account index [7], data len: 48',
    'Program log: account index [8], data len: 48',
    'Program log: account index [9], data len: 48',
    'Program log: account index [10], data len: 48',
    'Program log: account index [11], data len: 48',
    'Program log: account index [12], data len: 16',

where first 5 accounts are default for transfer hook execute instruction

pub struct ExecuteTransferHook<'info> {
    #[account(
      token::mint = mint,
      token::token_program = anchor_spl::token_interface::spl_token_2022::id(),
    )]
    pub source_account: Box<InterfaceAccount<'info, TokenAccount>>,

    #[account(
      token::token_program = anchor_spl::token_interface::spl_token_2022::id(),
    )]
    pub mint: Box<InterfaceAccount<'info, Mint>>,

    #[account(
      token::mint = mint,
      token::token_program = anchor_spl::token_interface::spl_token_2022::id(),
    )]
    pub destination_account: Box<InterfaceAccount<'info, TokenAccount>>,

    /// CHECK: can be any account
    pub owner_delegate: UncheckedAccount<'info>,

but all additional accounts are pretty small 16, 36 and 48 bytes which in total consume 340 bytes.
I have checked and before 2nd transfer we have 13.8 kb available free memory on heap but it is not enough to execute 1 transfer with transfer hook (account size mentioned above).

Looking through sdk code I have found that sdk allocate each iteration Vector account_key_data_refs.

@buffalojoec
Copy link
Contributor

@makarychev I ran a little test of my own to see if I could replicate. I was able to get to fifteen accounts, 14 of which are the size of an SPL Mint (85 bytes).

Check out the test over here.

In the meantime, I'm going to self-assign this and probably roll some more testing/benchmarking in the near future for extra meta limits.

@buffalojoec buffalojoec self-assigned this Jul 22, 2024
@joncinque
Copy link
Contributor

As a side note, we could probably remove some heap allocations by using an array of 32 AccountInfos on the stack and switching it to a vec only if it goes over that size. Looks like 32 AccountInfos takes up 1536 bytes:

    println!("{}", std::mem::size_of::<[AccountInfo; 32]>());

@joncinque
Copy link
Contributor

Or we could adopt an allocator that recovers heap space used after every call to invoke

@makarychev
Copy link
Author

makarychev commented Jul 23, 2024

I would like to highlight that we strugle with memory limitation when invoke is executed in transferChecked instruction from another instruction inside Solana Sealevel. To do that we use invoke_transfer_checked.

I have experimented a bit with invoke_transfer_checked and if next execution
ExtraAccountMetaList::add_to_cpi_instruction::<spl_transfer_hook_interface::instruction::ExecuteInstruction> is replaced with hardcoded additions

for i in 0..8 {
    execute_instruction.accounts.push(AccountMeta::new_readonly(additional_accounts[i].key(), false));
    execute_account_infos.push(additional_accounts[i].clone());
}

It saves 10KB on heap in example

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

No branches or pull requests

3 participants