LiteSVM / litesvm

Apache License 2.0
132 stars 24 forks source link

feat: modify API to allow certain use cases; add ed25519/secp256k1 precompiles #78

Closed OliverNChalk closed 1 month ago

OliverNChalk commented 4 months ago

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

Aursen commented 4 months ago

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

kevinheavey commented 3 months ago

yeah can we split this up please?

OliverNChalk commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.

kevinheavey commented 2 months ago

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 commented 2 months ago

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

Which commit is this?

https://github.com/LiteSVM/litesvm/pull/78/commits/f7dc4ad4c680c012599504fee7865c66402db437

Usage:

            let tx = SanitizedTransaction::try_create(
                tx,
                MessageHash::Compute,
                Some(false),
                &svm.accounts,
            )
            .unwrap();
kevinheavey commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

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