Move address_lookup_table program id declaration to solana-program #30617

tao-stones · 2023-03-06T19:07:28Z

Problem

address_lookup_table_program ID is declared in its own crate, one has to pull in the program crate in order to use its id(). This would result circular dependencies if trying to use its id() from lower lever, eg. from program-runtime.

One use case is #30620 to make builtins to consume CUs at invoke_context::process_executable_chain(). I added test_define_default_cost_for_builtins to program-runtime::invoke_context to demonstrate possible use case.

Since program IDs are statically/globally declared, perhaps make sense not to encapsulate it into program crate.

Summary of Changes

Separate static ID decl from program crate, move declaration to solana-program alone with other native programs.
Program crate itself, and other call sites, both refer to solana-program for ID.
Add a test to demonstrate use case.

Fixes #

programs/address-lookup-table/Cargo.toml

mvines · 2023-03-07T04:49:32Z

move its id declaration to sdk.

It's being moved to solana-program, not solana-sdk, though

sdk/src/lib.rs

tao-stones · 2023-03-07T05:11:25Z

move its id declaration to sdk.

It's being moved to solana-program, not solana-sdk, though

thanks for pointing it out, reworded.

programs/address-lookup-table/src/instruction.rs

programs/address-lookup-table/src/lib.rs

joncinque · 2023-03-07T12:09:14Z

The idea of #23700 is to have less stuff in solana-sdk / solana-program. Having everything in those crates has caused unclear issues like #30515, where people got build errors for VoteState.

So in general, we should come up with a way to isolate programs in their crates rather putting more stuff in the sdk / program, same as spl program crates.

What's the motivation for moving this?

tao-stones · 2023-03-07T18:23:14Z

The idea of #23700 is to have less stuff in solana-sdk / solana-program. Having everything in those crates has caused unclear issues like #30515, where people got build errors for VoteState.

So in general, we should come up with a way to isolate programs in their crates rather putting more stuff in the sdk / program, same as spl program crates.

What's the motivation for moving this?

@joncinque and I chatted offline, I updated PR description and added a test that demonstrate the potential use case.

tao-stones · 2023-03-07T18:36:18Z

program-runtime/src/invoke_context.rs

+        // encapsulated within program-runtime, accessible to both invoke_context
+        // (to make builtins to consume CUs) and by runtime::cost_model.
+        //
+        lazy_static::lazy_static! {


This static HashMap currently exists in runtime, not directly accessible by program-runtime::invoke_context. IMO it makes sense to define native's default cost in program-runtime, where compute_budget declares costs for various actions. Other option is to pass the default costs HashMap from runtime via builtin programs call chain to invoke_context.

Native programs are defined / built on top of program-runtime, so it makes more sense to me to define their costs somewhere above program-runtime.

Currently, the way it goes:

the compute budget defines costs for syscalls that are implemented in the program-runtime

the runtime defines costs for its native programs

the runtime passes native programs down to the invoke context

So you could even have a cost field in invoke_context::BuiltinProgram, and deduct straight from there, and that way it'll be impossible to define a new builtin program and forget to associate a cost. What do you think?

so it makes more sense to me to define their costs somewhere above program-runtime.

While I think IDs can be declared elsewhere, but totally agree costs should be defined above program-runtime,

I like the idea a lot for builtin program to carry a mandatory cost field. That removes the extra HashMap<program_id, cost> which is hard to maintain separately. Also have builtin developer to identify its default cost is way better than what's been done so far. (can also promote dev to implement more complex builtins to properly consume units).

I made POC PR #30639 , is this somewhat close to what's in your mind?

If Address-lookup-table-program is the new way to go, what's our plan for existing builtins that somewhat resides in solana-program/SDk? Will they be refactored into self-contain crate just like ALT?

There's no plan in place just yet to move existing declarations back up, but we could:

deprecate native programs in solana-program and add them with crate features, which are enabled by default, ie. "stake" feature includes all of the stake program bindings

undeprecate their re-exports from their crates, ie. solana-stake-program becomes the place to import from

add the proper compile guards, just like the ALT program

whenever we do v2, completely move the structs / instructions / ids into their crates

It's kind of a dumpy situation (that I put us in 😓 ). it's always tougher to move something up than down, especially for people that depend on it.

Ok, good to know. Thanks for the explanation!

tao-stones · 2023-03-07T18:36:56Z

program-runtime/src/invoke_context.rs

+                (solana_program::bpf_loader_upgradeable::id(), 2370),
+                (solana_program::bpf_loader_deprecated::id(), 1140),
+                (solana_program::bpf_loader::id(), 570),
+                // inconsistent


still have two inconsistencies to address

…o solana_program

Co-authored-by: mvines <mvines@gmail.com>

tao-stones · 2023-03-08T16:09:49Z

close it in favor of #30639 (discussion #30617 (comment))

tao-stones force-pushed the move-alt-progrtam-id-to-sdk branch 3 times, most recently from 4d4894a to 9202ed8 Compare March 7, 2023 01:12

tao-stones commented Mar 7, 2023

View reviewed changes

programs/address-lookup-table/Cargo.toml Outdated Show resolved Hide resolved

mvines reviewed Mar 7, 2023

View reviewed changes

sdk/src/lib.rs Outdated Show resolved Hide resolved

mvines changed the title ~~Move address_lookup_table program id declaration to solana_sdk~~ Move address_lookup_table program id declaration to solana-program Mar 7, 2023

mvines reviewed Mar 7, 2023

View reviewed changes

programs/address-lookup-table/src/instruction.rs Outdated Show resolved Hide resolved

mvines reviewed Mar 7, 2023

View reviewed changes

programs/address-lookup-table/src/lib.rs Outdated Show resolved Hide resolved

tao-stones requested a review from joncinque March 7, 2023 18:25

tao-stones commented Mar 7, 2023

View reviewed changes

tao-stones and others added 3 commits March 7, 2023 18:44

Separate address_lookup_table_program id declaration from its crate t…

ce72aca

…o solana_program

Update programs/address-lookup-table/src/instruction.rs

8a46165

Co-authored-by: mvines <mvines@gmail.com>

Update programs/address-lookup-table/src/lib.rs

0f5db9e

Co-authored-by: mvines <mvines@gmail.com>

tao-stones force-pushed the move-alt-progrtam-id-to-sdk branch from 2c9662e to 0ccb9f9 Compare March 7, 2023 18:46

add test to demostrate use case

7e91c3d

tao-stones force-pushed the move-alt-progrtam-id-to-sdk branch from 0ccb9f9 to 7e91c3d Compare March 7, 2023 19:56

tao-stones mentioned this pull request Mar 8, 2023

add default_cost as mandatory field for Builtin #30639

Merged

tao-stones closed this Mar 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move address_lookup_table program id declaration to solana-program #30617

Move address_lookup_table program id declaration to solana-program #30617

tao-stones commented Mar 6, 2023 •

edited

Loading

mvines commented Mar 7, 2023

tao-stones commented Mar 7, 2023

joncinque commented Mar 7, 2023 •

edited

Loading

tao-stones commented Mar 7, 2023

tao-stones Mar 7, 2023

joncinque Mar 7, 2023 •

edited

Loading

tao-stones Mar 8, 2023

tao-stones Mar 8, 2023

joncinque Mar 8, 2023 •

edited

Loading

tao-stones Mar 8, 2023

tao-stones Mar 7, 2023

tao-stones commented Mar 8, 2023

Move address_lookup_table program id declaration to solana-program #30617

Move address_lookup_table program id declaration to solana-program #30617

Conversation

tao-stones commented Mar 6, 2023 • edited Loading

Problem

Summary of Changes

mvines commented Mar 7, 2023

tao-stones commented Mar 7, 2023

joncinque commented Mar 7, 2023 • edited Loading

tao-stones commented Mar 7, 2023

tao-stones Mar 7, 2023

Choose a reason for hiding this comment

joncinque Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

tao-stones Mar 8, 2023

Choose a reason for hiding this comment

tao-stones Mar 8, 2023

Choose a reason for hiding this comment

joncinque Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

tao-stones Mar 8, 2023

Choose a reason for hiding this comment

tao-stones Mar 7, 2023

Choose a reason for hiding this comment

tao-stones commented Mar 8, 2023

tao-stones commented Mar 6, 2023 •

edited

Loading

joncinque commented Mar 7, 2023 •

edited

Loading

joncinque Mar 7, 2023 •

edited

Loading

joncinque Mar 8, 2023 •

edited

Loading