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

Add built-in programs to InvokeContext #10383

Merged
merged 1 commit into from
Jun 3, 2020
Merged

Conversation

mvines
Copy link
Member

@mvines mvines commented Jun 3, 2020

Keep BPFLoader from reaching into runtime/src/builtin_programs to populate the message processor it uses to invoke other programs. This breaks the model of the bank being the decider of what built-in programs are available for a given operating mode/epoch.

@@ -146,6 +146,8 @@ macro_rules! declare_loader(
)
);

pub type ProcessInstruction = fn(&Pubkey, &[KeyedAccount], &[u8]) -> Result<(), InstructionError>;
Copy link
Member Author

Choose a reason for hiding this comment

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

This mimics the ProcessInstruction in sdk/src/entrypoint.rs so I felt pretty good about promoting this type from runtime/ to sdk/

Copy link
Contributor

Choose a reason for hiding this comment

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

This file should not be part of our developer sdk anyway since it's native program specific

@@ -767,9 +767,8 @@ fn call<'a>(
let program_account = (*accounts[callee_program_id_index]).clone();
let executable_accounts = vec![(callee_program_id, program_account)];
let mut message_processor = MessageProcessor::default();
let builtin_programs = get_builtin_programs();
Copy link
Member Author

Choose a reason for hiding this comment

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

The problem! As of #10375, get_builtin_programs() takes two arguments that are not available here (nor should they be IMO)

@@ -479,6 +490,7 @@ impl MessageProcessor {
instruction.program_id(&message.account_keys),
rent_collector.rent,
pre_accounts,
self.programs.clone(),
Copy link
Member Author

@mvines mvines Jun 3, 2020

Choose a reason for hiding this comment

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

I'm not proud of this clone, my main goal is to unblock 1.1 and 1.2 releases by landing #10375.

Removing this could be one of those "non-jit BPF optimizations" in the v1.3 time frame

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 should be able to pass a reference here, I'll file an issue to track when this lands

Copy link
Contributor

@jackcmay jackcmay left a comment

Choose a reason for hiding this comment

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

lgtm, nice cleanup!

@mvines mvines added the automerge Merge this Pull Request automatically once CI passes label Jun 3, 2020
@solana-grimes solana-grimes merged commit a4cd966 into solana-labs:master Jun 3, 2020
@codecov
Copy link

codecov bot commented Jun 3, 2020

Codecov Report

Merging #10383 into master will decrease coverage by 0.1%.
The diff coverage is 30.7%.

@@            Coverage Diff            @@
##           master   #10383     +/-   ##
=========================================
- Coverage    81.4%    81.3%   -0.2%     
=========================================
  Files         294      294             
  Lines       68250    68258      +8     
=========================================
- Hits        55577    55501     -76     
- Misses      12673    12757     +84     

mergify bot pushed a commit that referenced this pull request Jun 3, 2020
automerge

(cherry picked from commit a4cd966)
@mvines mvines deleted the ic branch June 3, 2020 19:53
solana-grimes pushed a commit that referenced this pull request Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants