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

Adjustments of loader-v4 (part 3) #2821

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

Lichtso
Copy link

@Lichtso Lichtso commented Sep 3, 2024

Problem

Continuation of #2750.

Summary of Changes

  • Removes load_and_finalize_program() and load_program() in loader utils.
  • Adds instructions_to_load_program_of_loader_v4() and load_program_of_loader_v4() to loader utils.
  • Fixes incorrect program runtime environment in deployment verification check.
  • Introduces minimum of 1 lamport to avoid program account from being deleted if rent is disabled.

Copy link

mergify bot commented Sep 3, 2024

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

@Lichtso Lichtso requested a review from pgarg66 September 3, 2024 17:56
(program_keypair, instruction)
}

pub fn load_program<T: Client>(
Copy link

Choose a reason for hiding this comment

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

Was this function unused?

Copy link
Author

Choose a reason for hiding this comment

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

Yes they were for the loaders-v1 and v2, which we deprecated.

Copy link

Choose a reason for hiding this comment

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

So these are pure deletion of the code. The new functions are not related to these.
(trying to avoid comparing two independent functions).

Copy link
Author

@Lichtso Lichtso Sep 4, 2024

Choose a reason for hiding this comment

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

Yes, implementation / code wise they are unrelated. Conceptually they are related because these are their replacements but for the newest loader. However, loader-v4 is closest to loader-v3 so comparing the new functions against load_upgradeable_program_and_advance_slot() makes more sense.

for (index, instruction) in instructions.into_iter().enumerate() {
let message = Message::new(&[instruction], Some(&payer_keypair.pubkey()));
bank_client
.send_and_confirm_message(signers[index.min(signers.len() - 1)], message)
Copy link

Choose a reason for hiding this comment

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

Some documentation will be helpful here, as to why we are doing signers[index.min(signers.len() - 1)]. I am able to understand it right now, but may not be obvious for the future maintainers.

Copy link
Author

Choose a reason for hiding this comment

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

I reimplemented it using iterators instead of indices, hopefully that makes it self explanatory.

@@ -279,7 +280,7 @@ pub fn process_instruction_deploy(
};
let executor = ProgramCacheEntry::new(
&loader_v4::id(),
environments.program_runtime_v2.clone(),
environments.program_runtime_v1.clone(),
Copy link

Choose a reason for hiding this comment

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

Should we split this in its own PR?

Copy link
Author

Choose a reason for hiding this comment

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

This should have been in #2750 already but I missed it. Only noticed as I was working on the tests in #2796.

@pgarg66
Copy link

pgarg66 commented Sep 4, 2024

The code looks good to me. Can we add some tests?

@Lichtso
Copy link
Author

Lichtso commented Sep 5, 2024

Can we add some tests?

The tests will come in #2796. However we can't merge that until #2182, which depends on SIMD-0162.

…loader_v4().

Removes load_and_finalize_program() and load_program().
@Lichtso Lichtso marked this pull request as ready for review September 5, 2024 18:17
@Lichtso Lichtso merged commit 7f2013d into anza-xyz:master Sep 5, 2024
40 checks passed
@Lichtso Lichtso deleted the refactor/loader-v4 branch September 5, 2024 18:18
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