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

Update Solana SDK to 1.16 and Anchor to the latest change including it #142

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

vadorovsky
Copy link
Contributor

The newest solana-program crate was pulled unintentionally anyway which resulted in unexpected errors due to incompatibility of dependencies. More context: coral-xyz/anchor#2512

@vadorovsky vadorovsky force-pushed the vadorovsky/update-anchor branch 2 times, most recently from 7a180fb to e5a3e48 Compare June 6, 2023 10:09
@vadorovsky vadorovsky marked this pull request as ready for review June 6, 2023 10:11
@@ -16,7 +16,7 @@ pub struct InitializeNewMessageMerkleTree<'info> {
/// CHECK: it should be unpacked internally
#[account(
init,
seeds = [&program_id.to_bytes()[..], MESSSAGE_MERKLE_TREE_SEED],
seeds = [&__program_id.to_bytes()[..], MESSSAGE_MERKLE_TREE_SEED],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is program_id changed to __program_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because Anchor did so. We are not defining that variable ourselves, it's being generated inside #[program] attribute macro.

The change was made in this commit:

coral-xyz/anchor@2e89b79

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see thanks

light-verifier-sdk/src/state.rs Outdated Show resolved Hide resolved
@@ -23,5 +24,7 @@ pub struct MerkleTreeUpdateState {

pub leaves: [[[u8; 32]; 2]; 16],
pub number_of_leaves: u8,
_padding1: [u8; 7],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is padding necessary because of repr c?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's necessary in zero-copy accounts. It was honestly our little f***-up that we didn't do it before. Fortunately, now bytemuck is smart enough to check for padding inside macros (bytemuck::Pod, which is getting used by anchor_lang::zero_copy), so I added it, because of the compilation error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want more context - bytemuck has this check:

https://github.com/Lokathor/bytemuck/blob/df5262b19ca4fde16bc1a15eecb7438f0ecdbe42/derive/src/traits.rs#L540C1-L544

which checks whether your struct and [u8; LEN_OF_ALL_YOUR_DEFINED_FIELDS] are transmuttable (which requires them to be of the equal size). Failure of this check means that your struct is not aligned and that the compiler added the padding. To pass this check, you need to define the padding yourself, so the compiler doesn't do it for you.

Non-explicit (autogenerated by compiler) padding is problematic in bytemuck, because Rust doesn't initialize the memory used for padding. And bytemuck uses unsafe code to use your structs as arrays of bytes. Having uninitialized memory there might end up in undefined behavior.

When you have

pub struct A {
    pub foo: u32, // 4 bytes; it's the biggest field, so the whole struct has to be aligned to 4 bytes
    pub bar: u8, // 1 byte
    _padding: [u8; 3], // 3 bytes for alignment
   // 4 + 1 + 3 = 8
   // 8 | 4
}

pub struct B {
    pub foo: u32,
    pub bar: u8,
    // compiler will add 3 bytes of uninitialized memory here
    // 5 bytes are safe to access, reading last 3 bytes with `unsafe` is undefined behavior
}

Bytemuck (which is used by Anchor, when doing zero-copy) is going to treat both structures as [u8; 8], doing unsafe stuff on it, so accessing uninitialized bytes of B is undefined behavior.

Copy link
Contributor

@ananas-block ananas-block Jun 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok makes sense. I was aware that the padding was not ideal before, that's why I put the two u8 variables last this way it did not mess with the overall alignment of the other variables.

And the following wouldn't work because bytemuck treats both variables ad [u8;8] right?

        pub number_of_leaves: u8,
        pub insert_leaves_index: u8,
        _padding: [u8; 6],
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, that wouldn't work, because padding has to be added explicitly to each field (so they match the biggest field with size). So both u8 fields have to be filled up until 8 bytes.

The newest solana-program crate was pulled unintentionally anyway
which resulted in unexpected errors due to incompatibility of
dependencies. More context: coral-xyz/anchor#2512
@vadorovsky vadorovsky force-pushed the vadorovsky/update-anchor branch from e5a3e48 to c335297 Compare June 6, 2023 21:28
@ananas-block ananas-block merged commit d345e16 into main Jun 6, 2023
@ananas-block ananas-block deleted the vadorovsky/update-anchor branch June 6, 2023 22:11
vadorovsky added a commit to vadorovsky/light-protocol that referenced this pull request May 1, 2024
Lightprotocol#142)

The newest solana-program crate was pulled unintentionally anyway
which resulted in unexpected errors due to incompatibility of
dependencies. More context: coral-xyz/anchor#2512
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

Successfully merging this pull request may close these issues.

2 participants