Box bumps struct

[febo]

As described in #2718, the new way bumps are allocated use more stack space in 0.29.0. While you can allocate other parts (e.g., accounts) on the heap, there are still situations where having many bumps on the stack result in stack frame errors during Anchor account validation.

This PR moves the allocation of the Bumps struct to the heap to alleviate this.

Fixes #2718

[vercel]

@febo is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

[acheroncrypto]

Thanks but have you confirmed whether this actually solves the issue? There needs to be a lot of accounts with seeds before it make sense for us to put them into heap.

[febo]

Thanks but have you confirmed whether this actually solves the issue? There needs to be a lot of accounts with seeds before it make sense for us to put them into heap.

It keeps the amount of used stack space by the Context struct constant. The "default" size of a Context object is 40 bytes:

pub struct Context<'a, 'b, 'c, 'info, T: Bumps> {
    // 8 bytes
    pub program_id: &'a Pubkey,  
    // 8 bytes
    pub accounts: &'b mut T,
    // 16 bytes
    pub remaining_accounts: &'c [AccountInfo<'info>],
    // variable (minimum 8 bytes since the Context struct has 8-bytes alignment)
    pub bumps: T::Bumps,
}

When you have 9 seeds, it grows to 48 bytes because of the memory alignment – it has to maintain 8-bytes alignment, so instead of 41 it goes to 48. Using a Box reference, it keeps the size of the Context constant at 40 bytes independently of the number of seeds.

It might not seem a lot, but I had the exact same issue described in #2718 when upgrading a program from 0.26.0 to 0.29.0 and at first did not understand what had changed to cause this. The instruction that I was having problems had 9 accounts with seeds.

[acheroncrypto]

Thanks for the explanation. I'm all for improving the memory usage but I believe we have a better shot of reducing stack memory usage in other places and this change should be done as the last resort because for most instructions(< 9 seeds), this change will result in extra heap allocation with no benefits.

It might not seem a lot, but I had the exact same issue described in #2718 when upgrading a program from 0.26.0 to 0.29.0 and at first did not understand what had changed to cause this. The instruction that I was having problems had 9 accounts with seeds.

Could you please share this instruction?

[febo]

Thanks for the explanation. I'm all for improving the memory usage but I believe we have a better shot of reducing stack memory usage in other places and this change should be done as the last resort because for most instructions(< 9 seeds), this change will result in extra heap allocation with no benefits.

Previous versions were allocating the seeds on the heap, since they were stored on a BTreeMap. So this PR is less about reducing stack memory usage and more about maintaining previous behaviour. I would argue that the change from heap -> stack resulting from using the Bumps struct introduces a problem that could have been avoided.

It might not seem a lot, but I had the exact same issue described in #2718 when upgrading a program from 0.26.0 to 0.29.0 and at first did not understand what had changed to cause this. The instruction that I was having problems had 9 accounts with seeds.

Could you please share this instruction?

I found a workaround at the moment, where instead of having the seeds on the Anchor macro I am validating the derivation on the instruction. But overall it is probably good to not increase the stack memory usage if there is a way to avoid it.

[acheroncrypto]

Previous versions were allocating the seeds on the heap, since they were stored on a BTreeMap. So this PR is less about reducing stack memory usage and more about maintaining previous behaviour.

Sorry but maintaining the previous behavior when it was worse for the vast majority of programs doesn't make sense to me. Heap allocating a fixed-size data that is only a few bytes is a waste and it would only makes sense if the size is at least many multiples of the pointer size.

Could you please share this instruction?

I found a workaround at the moment, where instead of having the seeds on the Anchor macro I am validating the derivation on the instruction. But overall it is probably good to not increase the stack memory usage if there is a way to avoid it.

I asked about the instruction because there are other improvements in 0.29.0 that saves much more than this bump change adds and I don't know if you're using them or not. For example, using UncheckedAccount instead of AccountInfo saves ~32 bytes per account. If you have 10 AccountInfo's, converting them to UncheckedAccount would reduce stack usage by ~320 bytes.

[febo]

Sorry but maintaining the previous behavior when it was worse for the vast majority of programs doesn't make sense to me. Heap allocating a fixed-size data that is only a few bytes is a waste and it would only makes sense if the size is at least many multiples of the pointer size.

Worse in this case is subjective. You are probably optimizing for programs that might not be at memory limits, which most likely are the "vast majority of programs" - unless this change was in response of a clear identified issue. Any optimization has trade-offs, but we need to be careful not to be optimizing things that don't need to be optimized - stack space is more "scarce" than heap, and programs that are not constrained for stack space are very likely not constrained for heap space.

Unfortunately I did not have any AccountInfos in my instruction – they were either Accounts or UncheckedAccounts – so the option was to remove some of the seeds macro validation.

I will close this PR, thanks for taking the take time looking into this.