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

Generic processing pipeline #3467

Merged
merged 17 commits into from
Nov 11, 2024

Conversation

apfitzge
Copy link

@apfitzge apfitzge commented Nov 4, 2024

Problem

  • To allow new transaction type, we need to switch types (or make generic) in our processing pipeline

Summary of Changes

  • Make (as much as reasonably possible at this time) the processing pipeline generic

Fixes #

@apfitzge apfitzge self-assigned this Nov 4, 2024
@apfitzge apfitzge force-pushed the generic_processing_pipeline branch from 9f6ae8e to 6df881d Compare November 6, 2024 16:56
@apfitzge
Copy link
Author

apfitzge commented Nov 6, 2024

From here, block-verification (replay) and block-production (banking) can be made generic independently.

@apfitzge apfitzge marked this pull request as ready for review November 6, 2024 20:18
@jstarry
Copy link

jstarry commented Nov 6, 2024

Is it necessary to have SVMTransactionAdapter? Can't we add those methods to SVMTransaction instead?

@apfitzge
Copy link
Author

apfitzge commented Nov 6, 2024

Is it necessary to have SVMTransactionAdapter? Can't we add those methods to SVMTransaction instead?

SVMTransaction is meant to be stuff necessary for svm. converting into our legacy formats for our internal/plugin interfaces elsewhere isn't necessary for that.

The goal is for SVMTransactionAdapter to go away eventually, as our interfaces become better. It just needs to exist until those change, but changing the interfaces involves breaking geyser plugins - so it will not happen immediately.

@tao-stones
Copy link

The goal is for SVMTransactionAdapter to go away eventually

What will the interfaces taks SVMTransaction when Adapter goes away? If so, can the adapter's methods be added to SVMTransaction as @jstarry suggested, then remove them when time comes?

@apfitzge
Copy link
Author

apfitzge commented Nov 7, 2024

What will the interfaces taks SVMTransaction when Adapter goes away? If so, can the adapter's methods be added to SVMTransaction as @jstarry suggested, then remove them when time comes?

Yeah they will take SVMTransaction when the time comes. BUT SVMTransaction is part of the public interface of svm. We need to be careful with what we're adding to that interface, and shouldn't add things in that aren't necessary. (cc @buffalojoec)

@buffalojoec
Copy link

Yeah they will take SVMTransaction when the time comes. BUT SVMTransaction is part of the public interface of svm. We need to be careful with what we're adding to that interface, and shouldn't add things in that aren't necessary. (cc @buffalojoec)

Yeah it gets a little hairy when you have methods on SVMTransaction that are never used within SVM. I really only skimmed this PR, but something like trait RuntimeTransaction: SVMTransaction could help.

@apfitzge
Copy link
Author

apfitzge commented Nov 8, 2024

Yeah it gets a little hairy when you have methods on SVMTransaction that are never used within SVM. I really only skimmed this PR, but something like trait RuntimeTransaction: SVMTransaction could help.

Yeah not opposed to an approach like this; think naming can't be RuntimeTransaction since that's a concrete type (at least rn 😉 )

Something like

pub trait RuntimeTransactionTrait: StaticMeta + SVMTransaction {
    // temporary conversion fns we need
}

Then for many of our processing functions we don't even need RuntimeTransaction<TRAIT> we can just do impl RuntimeTransactionTrait 🤔

@tao-stones / @jstarry any thoughts on a name here, do you like this approach more?

@tao-stones
Copy link

Then for many of our processing functions we don't even need RuntimeTransaction<TRAIT> we can just do impl RuntimeTransactionTrait

I like this approach - taking trait instead of concrete RuntimeTransaction or SVMTransactionAdapter.

As for naming, I feel RuntimeTransactionTrait isn't descriptive enough, is it? but I can't come up better one 🤔

@buffalojoec
Copy link

As for naming, I feel RuntimeTransactionTrait isn't descriptive enough, is it? but I can't come up better one

How about ExecutableRuntimeTransaction?

I just completely made up the executable bit.

@apfitzge apfitzge force-pushed the generic_processing_pipeline branch from 6ae908b to 29653f8 Compare November 8, 2024 23:15
@apfitzge apfitzge force-pushed the generic_processing_pipeline branch from 29653f8 to cd3ae51 Compare November 11, 2024 14:07
solana_svm_transaction::svm_transaction::SVMTransaction,
};

pub trait TransactionWithMeta: StaticMeta + SVMTransaction {
Copy link
Author

Choose a reason for hiding this comment

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

CI still running, but I'll check w/ you now. @tao-stones @buffalojoec is this kind of what you had in mind?

Copy link
Author

Choose a reason for hiding this comment

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

Eventually these trait fns go away and TransactionWithMeta will be a glorified alias

Choose a reason for hiding this comment

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

Yessir, that's basically what I was thinking. Looks good to me, and keeps things clean at the SVM level. As long as you don't mind carrying this alias around for a bit :)

@jstarry
Copy link

jstarry commented Nov 11, 2024

Sorry I still don't really understand why we need TransactionWithMeta. Why don't we just use RuntimeTransaction<SanitizedTransaction>?

@jstarry
Copy link

jstarry commented Nov 11, 2024

Sorry I still don't really understand why we need TransactionWithMeta. Why don't we just use RuntimeTransaction<SanitizedTransaction>?

Chatted with @apfitzge and I think I'm understanding things better now. Kinda spaced on the goal of being able to pass the upcoming RuntimeTransaction<ResolvedTransactionView> through the tx pipeline. So rather than using RuntimeTransaction<Tx> where Tx: SVMTransactionAdapter everywhere we are going with Tx: TransactionWithMeta which is nice because then when we need to convert to geyser / block entry tx types we can make use of the static metadata rather than needing to compute that metadata from the types that impl SVMTransactionAdapter.

Copy link

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

this approach looks good to me

Copy link

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Lgtm!

@apfitzge apfitzge merged commit 7037f88 into anza-xyz:master Nov 11, 2024
52 checks passed
@apfitzge apfitzge deleted the generic_processing_pipeline branch November 11, 2024 18:57
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.

4 participants