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

feat: move precompile verification to SVM #2441

Merged
merged 5 commits into from
Aug 27, 2024

Conversation

jstarry
Copy link

@jstarry jstarry commented Aug 5, 2024

Problem

Transactions with precompile signatures that fail verification are currently not allowed to be committed to a block. After solana-foundation/solana-improvement-documents#159 they will be allowed to be committed so that block producers always get paid fees when verifying precompile signatures.

Summary of Changes

  • Introduced a new feature gate to turn off precompile verification where we currently doing it in the banking stage and replay stage and move it to the SVM instead
  • Expanded the scope of instruction processing metrics to include precompiles

Fixes #

@jstarry jstarry added the feature-gate Pull Request adds or modifies a runtime feature gate label Aug 5, 2024
Copy link

mergify bot commented Aug 5, 2024

If this PR represents a change to the public RPC API:

  1. Make sure it includes a complementary update to rpc-client/ (example)
  2. Open a follow-up PR to update the JavaScript client @solana/web3.js (example)

Thank you for keeping the RPC clients in sync with the server API @jstarry.

@jstarry jstarry force-pushed the feat/move-precompile-verification branch from 4eeb6f6 to 2ffc4bf Compare August 5, 2024 13:06
@jstarry jstarry requested review from apfitzge and tao-stones August 6, 2024 09:18
apfitzge
apfitzge previously approved these changes Aug 7, 2024
Copy link

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

Change makes sense to me. Users should still be able to verify that precompiles are valid through the interfaces, though they may need to change how they are doing it. THat should be communicated before feature-gate activation.

sdk/src/precompiles.rs Show resolved Hide resolved
let instruction_datas: Vec<_> = message_instruction_datas_iter.collect();
precompile
.verify(precompile_instruction_data, &instruction_datas, feature_set)
.map_err(InstructionError::from)
Copy link
Author

Choose a reason for hiding this comment

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

The new primitive traits are needed to convert to/from InstructionError::Custom and the ToPrimitive trait is specifically used here.

See this implementation for details:

impl<T> From<T> for InstructionError
where
    T: ToPrimitive,

@jstarry jstarry requested a review from Lichtso August 8, 2024 01:01
@jstarry
Copy link
Author

jstarry commented Aug 8, 2024

@Lichtso could you please check the message processor and invoke context changes?

@Lichtso
Copy link

Lichtso commented Aug 13, 2024

Instead of using InstructionError::Custom can we either add a new InstructionError::PrecompileError or map PrecompileError to the existing InstructionErrors?

@jstarry
Copy link
Author

jstarry commented Aug 13, 2024

No I think it's more appropriate to use custom errors here because these errors aren't general, they are program specific. Can you explain why adding new instruction error variants would make more sense?

@Lichtso
Copy link

Lichtso commented Aug 13, 2024

I might have missed it, but the changes in errors are not mentioned in the features SIMD, or are they?
Thus, I assumed they should stay close to the way they are now.

@jstarry
Copy link
Author

jstarry commented Aug 14, 2024

I might have missed it, but the changes in errors are not mentioned in the features SIMD, or are they? Thus, I assumed they should stay close to the way they are now.

No they are not but they probably should, thanks for bringing that up. There really isn't a good precedent for precompile error handling since we generally disregard any errors for dropped transactions. Currently we map precompile errors to InstructionError::InvalidAccountIndex inside SanitizedTransaction::verify_precompiles which I didn't feel was appropriate now that we will be storing precompile errors in the ledger.

I will update the SIMD with details about the errors and have them mapped to InstructionError::Custom as I have it here in this PR and then we can move forward with this PR as is. Sound good?

@jstarry
Copy link
Author

jstarry commented Aug 15, 2024

Any other feedback here @Lichtso and @apfitzge?

@jstarry
Copy link
Author

jstarry commented Aug 26, 2024

@Lichtso @apfitzge still waiting on feedback here

Comment on lines 70 to +76
assert_matches!(
client.process_transaction(transaction).await,
Err(BanksClientError::TransactionError(
TransactionError::InvalidAccountIndex
TransactionError::InstructionError(0, InstructionError::Custom(3))
))
);
// this assert is for documenting the matched error code above

Choose a reason for hiding this comment

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

@jstarry sorry I missed your last comment.

We should still have a test when the feature is disabled.
Is that tested somewhere else that I am missing?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry no it's not, I added an extra test for that here: 1084cfa

Choose a reason for hiding this comment

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

perfect

@apfitzge
Copy link

think we ought to push on the SIMD as well, but may not have luck merging until after breakpoint

Lichtso
Lichtso previously approved these changes Aug 26, 2024
@jstarry jstarry merged commit 3bf82c9 into anza-xyz:master Aug 27, 2024
52 checks passed
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants