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

LoadedPrograms::extract() unnecessarily requires WorkingSlot #34169

Closed
pgarg66 opened this issue Nov 19, 2023 · 0 comments · Fixed by #34342
Closed

LoadedPrograms::extract() unnecessarily requires WorkingSlot #34169

pgarg66 opened this issue Nov 19, 2023 · 0 comments · Fixed by #34342
Assignees

Comments

@pgarg66
Copy link
Contributor

pgarg66 commented Nov 19, 2023

Problem

LoadedPrograms::extract() takes WorkingSlot as an argument. However, the information carried by WorkingSlot can be computed using ForkGraph, that's part of LoadedPrograms struct. The WorkingSlot can be removed, as it makes cooperative program loading (#33715) harder/impossible to implement.

The following are the reasons for passing WorkingSlot as a parameter to LoadedPrograms::extract().

  • In a validator node, the first bank creates and initializes an instance of LoadedPrograms.
  • ForkGraph trait is implemented by BankForks. When the first bank is inserted in BankForks, the ForkGraph gets assigned to the LoadedPrograms object in the bank.
  • Any bank created from the first bank clone the Arc of LoadedPrograms. So every bank is using the same instance of LoadedPrograms, where the ForkGraph has already been initialized.
  • There are tests that create bank, but the tests do not use/need BankForks. So the bank instance is never inserted in the BankForks. That means, ForkGraph is never set in the LoadedProgram instance in bank.

Proposed Solution

  • Add a new (or update current) Bank::new_for_tests(), which internally creates a BankForks, inserts the new bank in it, and returns Arc<Bank> to the caller.
  • Update tests to use this new/updated method.

Or, find a better solution to this issue.

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 a pull request may close this issue.

2 participants