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

agave-transaction-ffi #2125

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

apfitzge
Copy link

Problem

  • We currently have no c-compatible way to interact with transactions from existing geyser plugins, nor future planned plugins.
  • The existing interfaces effectively force the use of rust, and even more so, the use of the same version of rustc as the validator was compiled with
  • This is non-ideal

Summary of Changes

  • Create a new c-compatible interface for transactions to be passed into plugins from agave
  • Write generic function to abstract the transaction type behind SVMTransaction trait

Fixes #

@ksolana
Copy link

ksolana commented Jul 22, 2024

LGTM. Should we plan to have this in solana docs as it could be useful to a wider audience?

ksolana
ksolana previously approved these changes Jul 22, 2024

extern "C" fn iter_accounts<Tx: SVMTransaction>(
transaction_ptr: TransactionPtr,
callback: AccountCallback,
Copy link

Choose a reason for hiding this comment

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

this looks similar to any_of/none_of algorithm? https://cplusplus.com/reference/algorithm/none_of/ ?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah a bit similar to those in that we will stop iteration if the callback returns false for any element; but the iter_accounts itself doesn't return true or false here.

The return of the callback is just used to early exit on some condition, like a break statement.

@apfitzge
Copy link
Author

LGTM. Should we plan to have this in solana docs as it could be useful to a wider audience?

Yeah. I need to create a c-header for the structs in the added file as well. The plan is for this to be used in any plugin that needs to inspect transactions; including geyser.

Will need to document it somewhere. Not sure the doc-comments that will be on crates.io be enough.

@apfitzge apfitzge requested a review from lijunwangs July 23, 2024 14:03
@apfitzge
Copy link
Author

@lijunwangs added you to this review. This transaction interface should be used in geyser as well instead of SanitizedTransaction so that the plugins could be written in non-rust languages.

@apfitzge
Copy link
Author

@lijunwangs - this will soon be a blocking item for moving to the new transaction type.

Do you have any comments here? or thoughts on how we should transition geyser to using this interface?

@lijunwangs
Copy link

@lijunwangs - this will soon be a blocking item for moving to the new transaction type.

Do you have any comments here? or thoughts on how we should transition geyser to using this interface?

Transaction is just one of the interface -- what is plan for other information such as account, slot, block etc.? We also need to make sure the interface change in geyser introduce forced minimal changes for existing geyser plugins.

@apfitzge
Copy link
Author

Transaction is just one of the interface -- what is plan for other information such as account, slot, block etc.?

Yes, transaction is just one of the interfaces. Transaction item is somewhat separate because there are plans to use it for non-geyser plugins for scheduler, hence I have separated it here.

Other changes needed to items passed to geyser:

  • Slices need to be converted into either just ptrs (if we have fixed-length fields like Pubkey) or ptrs + lengths.
  • References to other structs, such as TransactionStatusMeta also need similar interfaces. There are a few of these.

We also need to make sure the interface change in geyser introduce forced minimal changes for existing geyser plugins.

Realistically, there is no way we can make the interface c-compatible without breaking existing code - unless we have a time machine.
The interface is extremely unstable and forces users to use rust. Not only that, but even the same version of rust that the validator was compiled with!

We could make this an easier transition by adding member functions on the new interfaces.
This would allow client-code to change from geyser_interface_thing.field to geyser_interface_thing.field(). But this will still cause code to break.

@apfitzge
Copy link
Author

To be clear as well. Only the transaction for account updates is blocking work on the new transaction type.
Without an abstract interface like this, the only other feasible option is to just re-create a SanitizedTransaction which will add significant overhead for anyone using geyser.

@apfitzge
Copy link
Author

@lijunwangs c09fc1a added a bit simpler rust interface so rust users can transition more easily.

@apfitzge
Copy link
Author

apfitzge commented Sep 4, 2024

@lijunwangs not having an abstract interface here (and then used in geyser) is making us have ugly code in #2820

/// Given a reference to any type that implements `SVMTransaction`, create a
/// `TransactionInterface` that can be used to interact with the transaction in
/// C-compatible code. This interface is only valid for the lifetime of the
/// transaction reference, which cannot be guaranteed by this function interface.
Copy link
Author

Choose a reason for hiding this comment

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

not sure this is strictly true, possibly could add an unsized marker to the TransactionInterface to hold the lifetime 🤔

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