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

Patch the instruction discriminant to be 4 bytes #5

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

steveluscher
Copy link
Contributor

@steveluscher steveluscher commented Dec 3, 2024

Warning

I'm pretty sure that this is not landable, because if the legacy built-in and this program have different discriminator sizes, then the actual instruction data will not line up when we switch from one to the other. I couldn't find a way to configure Shank macros to produce 4-byte discriminants.

Fixes: #2.

...instruction,
discriminant: {
...instruction.discriminant,
type: "u32", // The legacy native program only accepts 4-byte instruction discriminants.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Paging @lorisleiva, Shank wizard. Is there a way to convince Shank to produce 4-byte instruction discriminators other than to hot-patch the idl?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can maybe update the comment here to say "this program" instead of "the legacy native program", since they are both the same, but idc.

Copy link
Member

Choose a reason for hiding this comment

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

Sadly no, Shank doesn't support custom enum discriminants. You could use a Codama visitor to update this but since we're currently saving the Anchor IDL and not the Codama one, your solution is currently the best thing to do.

@buffalojoec
Copy link
Contributor

buffalojoec commented Dec 4, 2024

This can land. The program's instructions aren't serialized or deserialized using Shank, but rather bincode.

Instruction::new_with_bincode(
crate::id(),
&LoaderV3Instruction::InitializeBuffer,
accounts,
)

This patch would just be for clients, and until Shank supports u32 (or we have Codama macros), this is fine IMO.

@steveluscher
Copy link
Contributor Author

But hang on, if I call, say DeployWithMaxDataLen which actually has instruction data, and the old program is like:

2 0 0 0 1 1 1 1 1 1 1 1
^^^^^^^ ^^^^^^^^^^^^^^^
disc    max_data_len

But the new program is like:

2 1 1 1 1 1 1 1 1
^ ^^^^^^^^^^^^^^^
d max_data_len

And then we have clients sending the old data to the new program, wouldn't it get interpreted like this?

2 0 0 0 1 1 1 1
^ ^^^^^^^^^^^^^
d max_data_len

@buffalojoec
Copy link
Contributor

buffalojoec commented Dec 4, 2024

But the new program is like:

This is what I'm saying, the new program is not using u8 discriminators. It's using u32, since I've derived serde's Serialize and Deserialize on them and I use bincode to pack/unpack within the helpers and processor.

fn limited_deserialize<T>(input: &[u8]) -> Result<T, ProgramError>

In other words, it's exactly the same instruction implementation as the builtin.

...instruction,
discriminant: {
...instruction.discriminant,
type: "u32", // The legacy native program only accepts 4-byte instruction discriminants.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can maybe update the comment here to say "this program" instead of "the legacy native program", since they are both the same, but idc.

...instruction,
discriminant: {
...instruction.discriminant,
type: "u32", // The legacy native program only accepts 4-byte instruction discriminants.
Copy link
Member

Choose a reason for hiding this comment

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

Sadly no, Shank doesn't support custom enum discriminants. You could use a Codama visitor to update this but since we're currently saving the Anchor IDL and not the Codama one, your solution is currently the best thing to do.

@buffalojoec buffalojoec merged commit a8d0bc0 into main Dec 4, 2024
9 checks passed
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.

Discriminator should be u32
3 participants