Skip to content

Move add_builtin to SVM#547

Merged
LucasSte merged 4 commits intoanza-xyz:masterfrom
LucasSte:svm-builtin
Apr 9, 2024
Merged

Move add_builtin to SVM#547
LucasSte merged 4 commits intoanza-xyz:masterfrom
LucasSte:svm-builtin

Conversation

@LucasSte
Copy link
Copy Markdown

@LucasSte LucasSte commented Apr 2, 2024

Problem

Presently, external SVM users must register builtins in two different places separately: the program cache and a vector passed to load_and_process_sanitized_transactions. These steps are burdensome and can lead to potential mistakes. In addition, the execution of programs should be self contained in the SVM, so the registration of builtins is semantically related to the SVM scope.

Summary of Changes

  1. Moved add_builtin from bank.rs to the SVM folder.
  2. add_builtin_account is now part of the trait TransactionProcessingCallback.
  3. Moved builtin_program_ids from struct Bank to struct TransactionBatchProcessor.

Fixes #303

@LucasSte LucasSte requested review from Lichtso, dmakarov and pgarg66 April 2, 2024 21:20
@LucasSte LucasSte marked this pull request as ready for review April 2, 2024 21:20
dmakarov
dmakarov previously approved these changes Apr 4, 2024
@LucasSte
Copy link
Copy Markdown
Author

LucasSte commented Apr 4, 2024

@pgarg66 mentioned he wanted to have a look at these changes, so I'll wait for his review before merging.

Comment thread svm/src/transaction_processor.rs Outdated
Comment thread svm/src/account_loader.rs Outdated
}

fn add_builtin_account(&self, _name: &str, _program_id: &Pubkey, _must_replace: bool) {
todo!()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is still a todo!() left over here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@LucasSte if this is an optional trait method, maybe have a default implementation of it in the trait definition?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It is an optional method, but coming up with a default implementation is not straightforward. In order to add a builtin account, we need access to a generic accounts database, which developers will only provide when they create their own struct to replace Agave's Bank.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The default implementation can be empty. So you don't need to implement for test with a todo in it. If the users of SVM do not need builtin they can ignore it.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (c0be86d) to head (2406159).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master     #547     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         851      851             
  Lines      230475   230485     +10     
=========================================
- Hits       188785   188774     -11     
- Misses      41690    41711     +21     

Comment thread runtime/src/bank.rs Outdated
@LucasSte LucasSte merged commit 4753a8a into anza-xyz:master Apr 9, 2024
@LucasSte LucasSte deleted the svm-builtin branch April 9, 2024 19:55
OliverNChalk pushed a commit to OliverNChalk/agave that referenced this pull request Nov 11, 2025
`slow-rebase.sh` was actually printing incorrect commands - copy/pasting
them failed.

This is an attempt to compute correct commands from the standpoint of
the user running the script.
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.

SVM entry point requires builtins registered in two different places

5 participants