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: modify API to allow certain use cases; add ed25519/secp256k1 precompiles #78

Closed
wants to merge 7 commits into from

Conversation

OliverNChalk
Copy link
Contributor

Hi, feel free to let me know if you want certain commits dropped/reworked. I made these changes to my fork to enable:

  • Passing AccountsDb to SanitizeTransaction::try_crate to get the account_keys iterable.
  • Support LoaderV2 and LoaderV4.
  • Return post account state when using simulate so test assertions can be run.
  • Added support for transactions that use ed25519/secp256k1 precompiles.

@Aursen Aursen requested review from kevinheavey and Aursen July 23, 2024 21:11
Copy link
Collaborator

@Aursen Aursen left a comment

Choose a reason for hiding this comment

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

Specifying the loader ID every time can be a bit tedious, perhaps adding a new function may be preferable for specific reasons

Copy link
Collaborator

@Aursen Aursen left a comment

Choose a reason for hiding this comment

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

Good to me

@Aursen
Copy link
Collaborator

Aursen commented Jul 29, 2024

A bit hard to review as all code is on the same PR

@kevinheavey
Copy link
Collaborator

yeah can we split this up please?

@OliverNChalk
Copy link
Contributor Author

Don't currently have the bandwidth to do this but if you wanna cherry pick the commits you want that's cool with me. Else I'll circle back to this when I'm looking to finalize my project that depends on it. For now im just building a fork

@kevinheavey
Copy link
Collaborator

ok i'll ask some questions here then. Firstly why does the precompiles commit need to add logic to check program IDs in process_transaction?

@OliverNChalk
Copy link
Contributor Author

OliverNChalk commented Aug 16, 2024

ok i'll ask some questions here then. Firstly why does the precompiles commit need to add logic to check program IDs in process_transaction?

When I was running TXs with precompiles I was getting a missing account error. To fix this I created dummy accounts, perhaps it would be more correct to avoid trying to load the account if it is a precompile?

I saw construct_instructions_account was doing a similar (though not dummy) operation.

@Aursen Aursen self-requested a review August 22, 2024 11:04
@kevinheavey
Copy link
Collaborator

Hey coming back to this:

When I was running TXs with precompiles I was getting a missing account error.

What are these missing accounts supposed to be?

Support LoaderV2 and LoaderV4.

I think for users who want more control over what loader gets used we should just direct them to build the loader instructions and send the required transactions

Return post account state when using simulate so test assertions can be run.

This is useful, I will include it in a separate PR

Passing AccountsDb to SanitizeTransaction::try_crate to get the account_keys iterable.

Which commit is this?

@OliverNChalk
Copy link
Contributor Author

Passing AccountsDb to SanitizeTransaction::try_crate to get the account_keys iterable.

Which commit is this?

f7dc4ad

Usage:

            let tx = SanitizedTransaction::try_create(
                tx,
                MessageHash::Compute,
                Some(false),
                &svm.accounts,
            )
            .unwrap();

@kevinheavey
Copy link
Collaborator

kevinheavey commented Oct 11, 2024

Passing AccountsDb to SanitizeTransaction::try_crate to get the account_keys iterable.

Which commit is this?

f7dc4ad

Usage:

            let tx = SanitizedTransaction::try_create(
                tx,
                MessageHash::Compute,
                Some(false),
                &svm.accounts,
            )
            .unwrap();

Coming back to this: what's the use case for passing all of svm.accounts in the reserved_account_keys arg?

@OliverNChalk
Copy link
Contributor Author

Passing AccountsDb to SanitizeTransaction::try_crate to get the account_keys iterable.

Which commit is this?

f7dc4ad
Usage:

            let tx = SanitizedTransaction::try_create(
                tx,
                MessageHash::Compute,
                Some(false),
                &svm.accounts,
            )
            .unwrap();

Coming back to this: what's the use case for passing all of svm.accounts in the reserved_account_keys arg?

Haha, this was pre 2.0 where that field did not exist. So it was my trying to communicate "pass the accounts db as an accounts loader". I have used this to make integration tests convenient to write (i just lazy load via RPC if network is enabled or from file if its not the first run/no network).

I think we can close this PR and I'll re-open individual PRs once 2.0 stabilizes. Then it will be easier to evaluate each proposed change's merit.

@kevinheavey
Copy link
Collaborator

kevinheavey commented Oct 13, 2024

Cool, the other thing I want to figure out is what's going on with precompiles. It sounds like there's some startup step we're missing?

@OliverNChalk
Copy link
Contributor Author

I'm no expert on this (I try to avoid reading the upstream solana source code if I can). My intuition is:

  • User creates a TX with precompile instruction.
  • This declares the precompile as the program ID.
  • Accounts DB loads the account, sees it is empty & thus returns an error due to CPI to non executable account.

I'll actually test this out when I open the new PRs, so for now the above is just my personal speculation

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.

3 participants