Add agave-precompiles crate by jstarry · Pull Request #5509 · anza-xyz/agave

jstarry · 2025-03-26T18:44:34Z

Problem

Precompile verification is subject to runtime feature gates and should live in the agave monorepo rather than creating a circular dependency between agave and the sdk repos.

Summary of Changes

Pull in precompile definitions and the verification code from all 3 precompiles

Everything from sdk that was pulled into agave in this PR will get deprecated in the sdk crates

Fixes #

mergify · 2025-03-26T18:45:08Z

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.

joncinque

Looks great to me overall! Note that I mostly skimmed the verify functions and tests. Just some small questions.

Also, there are two other places in agave that still use precompiles from the sdk that should get updated to use agave-precompiles instead:

agave/runtime/src/verify_precompiles.rs

Line 5 in 108fcb4

precompiles::get_precompiles,
agave/runtime/src/bank.rs

Line 136 in 108fcb4

precompiles::get_precompiles,

I was worried that this would be much worse, but thankfully there aren't any users of Transaction::verify_precompiles, so we can also deprecate that safely.

Finally, what's the plan for feature set usage in reserved-account-keys? Is the idea to refactor the one usage of it into the runtime? Thankfully, I only see one use at

agave/runtime/src/bank.rs

Line 6466 in 108fcb4

reserved_keys.update_active_set(&self.feature_set);

joncinque · 2025-03-26T19:32:16Z

+#![cfg_attr(docsrs, feature(doc_auto_cfg))]
+use {
+    lazy_static::lazy_static,
+    solana_feature_set::{enable_secp256r1_precompile, FeatureSet},


Just to make sure, this will use agave-feature-set in a future PR, correct?

joncinque · 2025-03-26T19:41:08Z

+solana-precompile-error = { workspace = true }
+solana-pubkey = { workspace = true }
+solana-sdk-ids = { workspace = true }
+solana-secp256k1-program = { workspace = true, features = ["bincode"] }


Is the bincode feature required on this dep? I couldn't find any parts of the code that were using stuff gated by it. It'll probably need serde though to ser/de the offsets

Yup turns out it is:

error[E0277]: the trait bound `SecpSignatureOffsets: serde::de::Deserialize<'_>` is not satisfied | --> precompiles/src/secp256k1.rs:51:45 | \| | 51 \| let offsets: SecpSignatureOffsets = bincode::deserialize(&data[start..end]) | \| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `serde::de::Deserialize<'_>` is not implemented for `SecpSignatureOffsets` |

Sorry, what I meant is just changing it to features = ["serde"]. I tested that locally and it works, since there's nothing required from the "bincode" feature (and bincode enables serde)

You don't have to be sorry that I'm dumb lol

Fixed, thanks! 0392e6b

joncinque · 2025-03-26T19:43:49Z

+fn get_data_slice<'a>(
+    instruction_datas: &'a [&[u8]],
+    instruction_index: u8,
+    offset_start: u16,
+    size: usize,
+) -> Result<&'a [u8], PrecompileError> {
+    let signature_index = instruction_index as usize;
+    if signature_index >= instruction_datas.len() {
+        return Err(PrecompileError::InvalidDataOffsets);
+    }
+    let signature_instruction = &instruction_datas[signature_index];
+    let start = offset_start as usize;
+    let end = start.saturating_add(size);
+    if end > signature_instruction.len() {
+        return Err(PrecompileError::InvalidSignature);
+    }
+
+    Ok(&instruction_datas[signature_index][start..end])
+}


Nothing to do here, but it's a bit baffling that the behavior of this isn't exactly the same between the precompiles

Nothing to do here, but it's a bit baffling that the behavior of this isn't exactly the same between the precompiles

don't look too closely at each of their "offsets" structs

jstarry · 2025-03-26T20:42:17Z

Finally, what's the plan for feature set usage in reserved-account-keys? Is the idea to refactor the one usage of it into the runtime? Thankfully, I only see one use at

I gave it a similar treatment here: #5513

It seems useful to have it be a separate crate for now.

t-nelson

disaster averted! 🎉

joncinque

Looks good to me, up to you if you want to downgrade that feature now or in future work. It's a mostly pedantic point since that feature's likely to get enabled by another user of the crate.

joncinque

Thanks!

mergify · 2025-03-26T21:36:47Z

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.

mergify · 2025-03-26T21:38:20Z

automerge label removed due to a CI failure

pgarg66 · 2025-03-26T21:39:45Z

This will add a dependency in SVM code to monorepo.
@t-nelson, @jstarry Any thoughts on how to resolve that once we split SVM to its own repo?

Should we move this along with SVM? Since the reason to move it here is runtime features.
Add a callback in SVM code to lookup precompiles?

jstarry · 2025-03-26T21:46:31Z

@pgarg66 I think regardless of this dependency change the SVM is going to have to figure out a reasonable interface for dealing with precompiles before it gets migrated out, this PR doesn't really change that fact. Happy to discuss approaches in a new issue.

jstarry · 2025-03-26T22:47:25Z

Just pushed changes:

I rebased to fix conflicts with the merged agave-reserved-account-keys PR
Updated the precompiles tests to NOT use Transaction::verify_precompiles: 58f7b3a
Fixed a clippy warning that showed up locally: 43e8ae7

codecov-commenter · 2025-03-26T23:55:48Z

Codecov Report

❌ Patch coverage is 97.13542% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.4%. Comparing base (df2d3d8) to head (3ef35c5).
⚠️ Report is 3816 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #5509     +/-   ##
=========================================
  Coverage    83.3%    83.4%             
=========================================
  Files         823      827      +4     
  Lines      372396   373548   +1152     
=========================================
+ Hits       310512   311606   +1094     
- Misses      61884    61942     +58

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

(cherry picked from commit e1162f7) # Conflicts: # Cargo.lock # Cargo.toml # program-runtime/Cargo.toml # programs/bpf_loader/src/syscalls/mod.rs # programs/sbf/Cargo.lock # runtime/Cargo.toml # runtime/src/bank.rs # svm/examples/Cargo.lock

jstarry requested a review from a team as a code owner March 26, 2025 18:44

jstarry requested a review from t-nelson March 26, 2025 19:00

t-nelson requested a review from joncinque March 26, 2025 19:21

joncinque reviewed Mar 26, 2025

View reviewed changes

joncinque mentioned this pull request Mar 26, 2025

feature-set-interface: Use it in the sdk anza-xyz/solana-sdk#86

Closed

jstarry force-pushed the agave-precompiles branch from 1e1faec to 83c2c98 Compare March 26, 2025 20:38

t-nelson previously approved these changes Mar 26, 2025

View reviewed changes

joncinque previously approved these changes Mar 26, 2025

View reviewed changes

jstarry dismissed stale reviews from joncinque and t-nelson via 0392e6b March 26, 2025 21:34

jstarry requested a review from joncinque March 26, 2025 21:35

joncinque previously approved these changes Mar 26, 2025

View reviewed changes

jstarry added automerge automerge Merge this Pull Request automatically once CI passes v2.2 labels Mar 26, 2025

mergify Bot removed the automerge automerge Merge this Pull Request automatically once CI passes label Mar 26, 2025

pgarg66 previously approved these changes Mar 26, 2025

View reviewed changes

jstarry added 7 commits March 26, 2025 22:28

Add agave-precompiles crate

aab053d

add tests

e32e0a8

feedback

edc1387

reinstate bincode feature

234e7a9

no more readme field yay

b8d89f4

change cargo feature

591da65

update cargo.lock files

255b759

jstarry dismissed pgarg66’s stale review via 255b759 March 26, 2025 22:29

jstarry dismissed joncinque’s stale review via 255b759 March 26, 2025 22:29

jstarry force-pushed the agave-precompiles branch from 0392e6b to 255b759 Compare March 26, 2025 22:29

jstarry added 2 commits March 26, 2025 22:46

migrate tests from Transaction::verify_precompile

58f7b3a

fix clippy warning

43e8ae7

fix test dependency feature

3ef35c5

t-nelson approved these changes Mar 26, 2025

View reviewed changes

joncinque approved these changes Mar 26, 2025

View reviewed changes

pgarg66 approved these changes Mar 27, 2025

View reviewed changes

jstarry merged commit e1162f7 into anza-xyz:master Mar 27, 2025
58 checks passed

jstarry deleted the agave-precompiles branch March 27, 2025 00:33

mergify Bot mentioned this pull request Mar 27, 2025

v2.2: Add agave-precompiles crate (backport of #5509) #5523

Merged

jstarry added a commit that referenced this pull request Mar 27, 2025

Add agave-precompiles crate (#5509)

38debe7

t-nelson pushed a commit that referenced this pull request Mar 27, 2025

Add agave-precompiles crate (#5509)

ce414dd

jstarry pushed a commit that referenced this pull request Mar 27, 2025

v2.2: Add agave-precompiles crate (backport of #5509) (#5523)

318dd92

Conversation

jstarry commented Mar 26, 2025

Problem

Summary of Changes

Uh oh!

mergify Bot commented Mar 26, 2025

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jstarry commented Mar 26, 2025

Uh oh!

t-nelson left a comment

Choose a reason for hiding this comment

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Mar 26, 2025

Uh oh!

mergify Bot commented Mar 26, 2025

Uh oh!

pgarg66 commented Mar 26, 2025

Uh oh!

jstarry commented Mar 26, 2025

Uh oh!

jstarry commented Mar 26, 2025

Uh oh!

codecov-commenter commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Mar 26, 2025 •

edited

Loading