-
Notifications
You must be signed in to change notification settings - Fork 337
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
Runtime: Core BPF: Add test for CPI post-migration #2531
Runtime: Core BPF: Add test for CPI post-migration #2531
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm. I'd get @2501babe 's sign-off as well, though
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
does this new test fail if you turn off your fix (ie if the builtin is not marked executable)? the check im aware of that would fail this is before control drops to invoke context, though there might be another check inside it not that you shouldnt merge this, i think a lot more testing of transaction loading needs to take place and im brainstorming myself how to do that |
Yep. Directly invoking it works, but via CPI it throws Looks to me like this callsite: agave/program-runtime/src/invoke_context.rs Lines 437 to 439 in ae18213
|
(cherry picked from commit 8f675eb)
(cherry picked from commit 8f675eb)
…2531) (#2665) Runtime: Core BPF: Add test for CPI post-migration (#2531) (cherry picked from commit 8f675eb) Co-authored-by: Joe C <[email protected]>
Problem
Our test suite for migrating builtin programs to BPF sends a transaction with an instruction for the newly migrated program, to test that it can be loaded and executed successfully. However, this test suite is missing a test for CPI'ing to this program.
Building on the back of #2483, we should have coverage for this case.
Summary of Changes
Add a mocked-out builtin processor designed to CPI to the provided program to the test suite. Then simply send a transaction to this builtin (like we're already doing with the newly migrated program) directing it to CPI to the newly migrated program.