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

program: introduce core BPF implementation #7

Merged
merged 14 commits into from
Mar 13, 2024

Conversation

buffalojoec
Copy link
Contributor

@buffalojoec buffalojoec commented Mar 5, 2024

This PR introduces the Core BPF implementation of Address Lookup Table.

There are a few caveats with the implementation so far. You can find them in the
source code by searching for comments prefixed with:

[Core BPF]:

The following caveats will be solved in the next Solana release, and a
discussion can be had for backporting any of these changes:

  • InstructionError::Immutable has no ProgramError counterpart
    (#35113).
  • InstructionError::IncorrectAuthority has no ProgramError counterpart
    (#35113).
  • solana-program-test will not overwrite a builtin if the BPF program you've
    provided shares the same address as an existing builtin
    (#35233).

The following caveat seems to be unavoidable:

Finally, I've implemented a cooldown period based on Clock, but this will
likely not be sufficient. I think we should consider merging this initial
implementation and leaving #1 unfinished until the proper research is completed.

@buffalojoec buffalojoec requested a review from lorisleiva March 5, 2024 16:28
Copy link
Contributor Author

buffalojoec commented Mar 5, 2024

@buffalojoec buffalojoec linked an issue Mar 5, 2024 that may be closed by this pull request
@buffalojoec buffalojoec requested a review from joncinque March 5, 2024 16:29
@buffalojoec buffalojoec changed the base branch from 03-05-repo_bump_solana_crates_to_1.18.2_ to 03-05-program_wipe_directory_contents March 5, 2024 19:35
@buffalojoec buffalojoec force-pushed the 03-05-program_introduce_core_BPF_implementation branch from f4b68c0 to 227a8a0 Compare March 5, 2024 19:35
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks really good for the most part! Lots of comments, but mostly small things

@@ -0,0 +1,35 @@
[package]
name = "address-lookup-table"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: are we going with solana-program-address-lookup-table?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can do better. I'd love us to commit on a good naming convention. Our current one is:

Organization name:      solana-program
Program name:           address-lookup-table
Program crate name:     solana-program-address-lookup-table
Rust client crate name: solana-program-address-lookup-table-client
JS client package name: @solana-program/address-lookup-table

I like the JS client package name but maybe the crates are a bit too verbose. We could replace solana-program- with something like spl- for crate names but then we risk to have conflicts with existing ones.

Cc @steveluscher @mcintyre94

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about just sp-?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess for any programs we port over from SPL land, sp would be royally confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah... we haven't been able to come up with anything better. The crate is currently called solana-address-lookup-table-program so I'm not too fussed about solana-program-address-lookup-table, unless we want to create a new acronym to make it totally clear that it's different.

I haven't liked anything enough to push for it. I don't think we should use spl- though, that means something different. The best might be to use sbpf in it, so I could (weakly) vouch for sbpf-address-lookup-table-program

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with scbpf- for now, but let's see where we land in talks. Lmk if this should block merging. Ideally we aren't publishing anytime soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine, we can stick with that

Copy link
Member

Choose a reason for hiding this comment

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

For anyone reading this, the conversation continued here:
https://discord.com/channels/428295358100013066/1204657211402489876/1215666108141539358

program/build.rs Outdated Show resolved Hide resolved
program/Cargo.toml Show resolved Hide resolved
program/Cargo.toml Show resolved Hide resolved
program/Cargo.toml Outdated Show resolved Hide resolved
program/src/processor.rs Outdated Show resolved Hide resolved
program/src/processor.rs Outdated Show resolved Hide resolved
program/src/processor.rs Outdated Show resolved Hide resolved
program/src/processor.rs Outdated Show resolved Hide resolved
program/src/processor.rs Show resolved Hide resolved
@buffalojoec
Copy link
Contributor Author

@joncinque I believe I've addressed everything here in some follow-up commits. Just need to land on the crate name.

@buffalojoec buffalojoec force-pushed the 03-05-program_introduce_core_BPF_implementation branch from f78f0ec to 8fa1a4a Compare March 8, 2024 20:20
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for integrating all the feedback, you really... crushed it:

crush table

program/src/state.rs Outdated Show resolved Hide resolved
program/src/state.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

All right, good to go on my side!

@buffalojoec buffalojoec force-pushed the 03-05-program_wipe_directory_contents branch from 6a92d6a to ea4af0a Compare March 13, 2024 04:16
@buffalojoec buffalojoec force-pushed the 03-05-program_introduce_core_BPF_implementation branch from e697aa7 to 79db606 Compare March 13, 2024 04:16
Copy link
Contributor Author

buffalojoec commented Mar 13, 2024

Merge activity

  • Mar 13, 12:16 AM EDT: @buffalojoec started a stack merge that includes this pull request via Graphite.
  • Mar 13, 12:24 AM EDT: Graphite rebased this pull request as part of a merge.
  • Mar 13, 12:25 AM EDT: @buffalojoec merged this pull request with Graphite.

@buffalojoec buffalojoec force-pushed the 03-05-program_wipe_directory_contents branch from ea4af0a to 056f91b Compare March 13, 2024 04:19
@buffalojoec buffalojoec changed the base branch from 03-05-program_wipe_directory_contents to main March 13, 2024 04:22
@buffalojoec buffalojoec force-pushed the 03-05-program_introduce_core_BPF_implementation branch from 79db606 to f7164f3 Compare March 13, 2024 04:23
@buffalojoec buffalojoec merged commit ac8f05d into main Mar 13, 2024
1 check 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.

PR the initial Core BPF implementation for review
3 participants