Skip to content

implement get_builtin_program_ids() trait for TransactionProcessingCallback#316

Closed
Nagaprasadvr wants to merge 2 commits intoanza-xyz:masterfrom
Nagaprasadvr:improv-svm-entry-point-requires-builtins-registered-in-two-different-places
Closed

implement get_builtin_program_ids() trait for TransactionProcessingCallback#316
Nagaprasadvr wants to merge 2 commits intoanza-xyz:masterfrom
Nagaprasadvr:improv-svm-entry-point-requires-builtins-registered-in-two-different-places

Conversation

@Nagaprasadvr
Copy link
Copy Markdown

@Nagaprasadvr Nagaprasadvr commented Mar 19, 2024

Fixes Issue #303

Improvement

  1. In order to use the SVM, we must register the builtins as it is now done in the add_builtin function in bank.rs (link), and then pass them to load_and_execute_sanitized_transactions as a vector of public keys.
    instead of this we can implement get_builtin_programs() trait to TransactionProcessingCallback and get builtin_programs inside load_and_execute_sanitized_transactions fn

Summary of Changes

  1. Implement trait get_builtin_programs() for TransactionProcessingCallback
  2. remove vec of builtin_program passed as an array of pubkeys to load_and_execute_sanitized_transactions

@Nagaprasadvr Nagaprasadvr force-pushed the improv-svm-entry-point-requires-builtins-registered-in-two-different-places branch from f5f8bc0 to c2240b6 Compare March 19, 2024 16:10
@mergify mergify Bot requested a review from a team March 19, 2024 16:11
Copy link
Copy Markdown

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

How soon do you need this method exposed? It's included in this PR here.

#79

This commit: 735bcfd

@Nagaprasadvr
Copy link
Copy Markdown
Author

Nagaprasadvr commented Mar 19, 2024

How soon do you need this method exposed? It's included in this PR here.

#79

This commit: 735bcfd

U can close this , im fine with your pr

@buffalojoec
Copy link
Copy Markdown

How soon do you need this method exposed? It's included in this PR here.
#79
This commit: 735bcfd

U can close this , im fine with your pr

Cool I'll link back here when mine goes in. Thanks for contributing! You can still probably rebase yours and add the callback bit if you wanted!

@buffalojoec
Copy link
Copy Markdown

@Nagaprasadvr My commit went in if you wanted to rebase your changes!

@Nagaprasadvr
Copy link
Copy Markdown
Author

@Nagaprasadvr My commit went in if you wanted to rebase your changes!

hey thanks will do that

@Nagaprasadvr Nagaprasadvr force-pushed the improv-svm-entry-point-requires-builtins-registered-in-two-different-places branch from c2240b6 to c4a67dc Compare March 23, 2024 02:51
@Nagaprasadvr Nagaprasadvr force-pushed the improv-svm-entry-point-requires-builtins-registered-in-two-different-places branch from c4a67dc to 048d29c Compare March 23, 2024 02:52
@Nagaprasadvr Nagaprasadvr changed the title implement get_builtin_programs() trait for TransactionProcessingCallback implement get_builtin_program_ids() trait for TransactionProcessingCallback Mar 23, 2024
@LucasSte
Copy link
Copy Markdown

LucasSte commented Mar 26, 2024

  1. In order to use the SVM, we must register the builtins as it is now done in the add_builtin function in bank.rs (link), and then pass them to load_and_execute_sanitized_transactions as a vector of public keys.
    instead of this we can implement get_builtin_programs() trait to TransactionProcessingCallback and get builtin_programs inside load_and_execute_sanitized_transactions fn

Note that the core idea behind the issue is not just to eliminate the vector of builtins to load_and_execute_sanitized_transactions. The SVM is now offered as a separate create than the rest of runtime, and ideally, we would register the builtins directly on the SVM instead of doing it in bank. The SVM would keep a list of builtins it needs to account for during execution.

Your changes only eliminate the builtin from the function, but do not account for the needed re design. This PR does not fix the issue. It is only a step towards the planned refactor.

Either way, the refactor must only come after #79 is merged.

@Nagaprasadvr
Copy link
Copy Markdown
Author

  1. In order to use the SVM, we must register the builtins as it is now done in the add_builtin function in bank.rs (link), and then pass them to load_and_execute_sanitized_transactions as a vector of public keys.
    instead of this we can implement get_builtin_programs() trait to TransactionProcessingCallback and get builtin_programs inside load_and_execute_sanitized_transactions fn

Note that the core idea behind the issue is not just to eliminate the vector of builtins to load_and_execute_sanitized_transactions. The SVM is now offered as a separate create than the rest of runtime, and ideally, we would register the builtins directly on the SVM instead of doing it in bank. The SVM would keep a list of builtins it needs to account for during execution.

Your changes only eliminate the builtin from the function, but do not account for the needed re design. This PR does not fix the issue. It is only a step towards the planned refactor.

Either way, the refactor must only come after #79 is merged.

I see that issue is updated will make changes accordingly

@LucasSte
Copy link
Copy Markdown

LucasSte commented Apr 9, 2024

Superseded by #547

@LucasSte LucasSte closed this Apr 9, 2024
OliverNChalk pushed a commit to OliverNChalk/agave that referenced this pull request Nov 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants