Make zero copy safe by default, add account(zero_copy(unsafe)) feature.

[ckamm]

The default account(zero_copy) feature unsafe impls the bytemuck traits

The new one derives it instead, which runs desired sanity checks like "struct has no padding".

[vercel]

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

A member of the Team first needs to authorize it.

[Aursen]

Why not do it by default? It would be interesting to remove all the unsafe code

[ckamm]

Why not do it by default? It would be interesting to remove all the unsafe code

That would break backwards compatibility. It'd prefer making the safe option the default, but it is a choice I'd leave up to the anchor maintainers.

[Henry-E]

Having talked to armani and robert chen about this a while ago, it's probably best to make this the default and anyone that wants to can manually implement their own unsafe types can do that.

• So we should make this default, that way we can use the existing zero copy tests (assuming they still pass)
• We probably need some extra documentation to explain what limitations this places on zero copy structs now, I know that we have to use repr(C) and there can't be any padding (unless you use some special option with repr(c) to align stuff).
• as with all these PRs, please add an entry to the changelog under the breaking change heading
• If you felt like it, we could be really accommodating to existing programs and make unsafe_bytemuck the optional value instead. While also marking the attribute as deprecated / unsupported

[ckamm]

@Henry-E I've covered some of your suggestions. The existing zero_copy docs already referenced bytemuck::Pod safety restrictions, so I didn't know what to add there.

• I didn't know where to add the note about deprecated unsafe_bytemuck_derives
• Fixing the test looked tricky, I made it pass by using unsafe_bytemuck for now

Could you take care of the rest?

[Henry-E]

Sure, thanks very much for all that. I'll look into finishing the rest soon.

[Henry-E]

158 |     pub events: [Event; 25000],
    |                 ^^^^^^^^^^^^^^ the trait `Pod` is not implemented for `[Event; 25000]`

I kind of forgot about this kind of error, where rust wants you to implement traits on arrays of specific lengths. Is this something you've had to deal with @ckamm or is it just the case that you only ever use Vec<T> with zero copy?

I can't quite remember but I thought there was some reason that people didn't use Vec<T> with zero copy.

[ckamm]

@Henry-E Use bytemuck with the features = ["min_const_generics"] feature -- newer rust versions allow generically implementing Pod for static arrays of all sizes.

You can't use Vec<> with zero-copy. Vec is just a struct of a few pointers and all the actual data ends up on the heap.

[Henry-E]

@ckamm that solved it, thanks very much!

Another thing that I find kind of annoying but don't really know what to do about is how even though we use a reference to anchor's privately exported bytemuck crate derive(anchor_lang::__private::bytemuck::Pod), once these macros are expanded they switch to using the local crate's bytemuck, e.g. ::bytemuck::Pod. I'm not sure if it's possible to do anything about this.

[ckamm]

Glad it helped! I don't know of a way to use the privately exported instance, unfortunately.

[Henry-E]

No worries, I think I'll do one more review of this tomorrow morning and then I think it's good to merge.

[Henry-E]

One solution is to use a crate alias but i don't know if we really want to overwrite the local crate's bytemuck. Now I'm wondering if we should just make it entirely using the local crate's bytemuck and not worry about whatever version of bytemuck anchor's using.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
anchor-docs	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Jan 26, 2023 at 2:48PM (UTC)