Generic Transaction Trait #1918

apfitzge · 2024-06-28T16:27:30Z

Problem

The use of SanitizedTransaction is extremely slow in block-production
- This is mainly due to the high level of nested allocations.
- Legacy messages use 3 + 2 * num_instructions allocations
- V0 messages use 4 + 2 * num_instructions + 2 * num_lookups
This has motivated the use of a new transaction type, however runtime & SVM do not support using anything other than SanitizedTransaction.
It would be convenient if we could slowly migrate to new transaction type, and have the runtime and SVM be generic over some transaction trait

Summary of Changes

Create 2 new traits: MessageTrait, TransactionTrait
- MessageTrait is effectively to abstract current SanitizedMessage
- TransactionTrait is an extension of MessageTrait that is effectively to abstract SanitizedTransaction
Implemented trait for the "reference" types SanitizedMessage and SanitizedTransaction
Implemented trait(s) for &T if T implements traits
- this allows us to pass owned values or references to any function that is generic over the trait...this is really convenient
- makes some existing code simpler: no more impl core::borrow::Borrow<SanitizedTransaction>
New crate and trait names are placeholders, will be using this PR as a platform to discuss naming. Possible trait names:
- MessageTrait, TransactionTrait
- Message, SignedMessage (@Lichtso had mentioned to me that he felt we use "transaction" incorrectly right now, and a "transaction" is a loaded message. i.e. users always send messages, transactions are what come out of those messages).
Some possible crate names (probably depends on above choice):
- solana-transaction-trait
- solana-signed-message

Fixes #

apfitzge · 2024-06-28T16:31:22Z

@LucasSte @t-nelson @Lichtso @buffalojoec

Could I get input on naming here? I have a few options in the PR description, or feel free to submit your own idea(s)

Another open question is whether or not the traits should live inside of SVM or as a separate crate like I've done above.

buffalojoec · 2024-06-28T17:52:29Z

I'm in favor of this! I also think it should be its own crate, as you have it.

My thoughts on naming: solana-transaction (crate), SolanaMessage and SolanaTransaction. Maybe also SolanaInstruction.

apfitzge · 2024-06-28T17:56:25Z

I'm in favor of this! I also think it should be its own crate, as you have it.

My thoughts on naming: solana-transaction (crate), SolanaMessage and SolanaTransaction. Maybe also SolanaInstruction.

I like these more than the current names I have

kevinheavey · 2024-06-29T04:28:29Z

If we call this solana-transaction what do we call the crate we make when solana_sdk::transaction finally gets its own crate?

Fully support making this a new crate whatever we call it. I suggest solana-transaction-trait

buffalojoec · 2024-06-29T13:08:54Z

If we call this solana-transaction what do we call the crate we make when solana_sdk::transaction finally gets its own crate?

Fully support making this a new crate whatever we call it. I suggest solana-transaction-trait

This is just my own bias, but I don't like putting "trait" or "struct" in the names, I'd lean toward solana-transaction-interface if we didn't want to nab solana-transaction. They could also be the same crate.

kevinheavey · 2024-06-29T13:25:59Z

They could also be the same crate.

If they were the same crate then there would be more chance of users getting confused by the Instruction and MessageAddressTableLookup structs that this crate has.

Plus there'd be more bloat and it would be harder to rip out what's needed for transactions from solana-sdk, e.g. this crate requires solana_sdk::packet, which is not required by solana_sdk::transaction

LucasSte · 2024-06-30T21:09:49Z

I'm in favor of this! I also think it should be its own crate, as you have it.

My thoughts on naming: solana-transaction (crate), SolanaMessage and SolanaTransaction. Maybe also SolanaInstruction.

These names are good. I couldn't think of anything better (I'm terrible at naming things).

apfitzge · 2024-07-01T15:20:27Z

This is just my own bias, but I don't like putting "trait" or "struct" in the names, I'd lean toward solana-transaction-interface if we didn't want to nab solana-transaction. They could also be the same crate.

I actually want solana-transaction-interface for something else in the near future 😉 (an ffi capable interface for transactions). Which is kind of dependent on this work actually.

I think whatever name we choose, this should be a separate crate from the sdk transaction.

placeholder-transaction-trait/src/message_trait.rs

apfitzge · 2024-07-01T15:37:54Z

placeholder-transaction-trait/src/transaction_trait.rs

+
+    /// Returns the message hash.
+    // TODO: consider moving this to Message
+    fn message_hash(&self) -> &Hash;


for SanitizedTransaction this just fetches a pre-computed message hash, which is why i'd put it here.
That's a current implementation detail and I feel doesn't make too much sense honestly.

Could also override the impl for SanitizedTransaction to get the cached value, while calculating it on SanitizedMessage and then move this to the MessageTrait

apfitzge · 2024-07-01T15:38:08Z

placeholder-transaction-trait/src/transaction_trait.rs

+
+    /// Returns true if the transaction is a simple vote transaction.
+    // TODO: consider moving this to Message
+    fn is_simple_vote_transaction(&self) -> bool;


same comment here as for message_hash

ended up removing these 2 functions + conversion to VersionedTransaction.

Trimmed the traits down to what is necessary for SVM, and will have some extension that can be used inside core/runtime.

placeholder-transaction-trait/src/transaction_trait.rs

placeholder-transaction-trait/src/message_trait.rs

buffalojoec · 2024-07-01T23:42:03Z

It would be convenient if we could slowly migrate to new transaction type, and have the runtime and SVM be generic over some transaction trait

I guess it wouldn't hurt to at least consider SvmTransaction and SvmMessage.

They don't look as nice as SolanaTransaction and SolanaMessage, but perhaps the "SVM" prefix describes the traits' intentions more. Thinking a bit about people using solana-svm outside of the validator.

If we call this solana-transaction what do we call the crate we make when solana_sdk::transaction finally gets its own crate?

Not sure if this makes the relationship any more clear, but you could have solana-svm-transaction for the interface and solana-transaction for the SDK structs that implement it. It might appear a bit backwards, but people working with Solana tooling on the chain itself would just use solana_transaction::Transaction, meanwhile SVM projects would spot solana_svm_transaction::SvmTransaction in the solana-svm library and see they can implement it with a custom transaction.

Or maybe it's even solana_svm_transaction::SvmTransaction and solana_transaction::SolanaTransaction. Seems a tad redundant, though.

apfitzge · 2024-07-02T13:47:47Z

I guess it wouldn't hurt to at least consider SvmTransaction and SvmMessage.

This was my original idea, but I'm a bit hesistant since there are a few things in the interface that are not strictly necessary for SVM, but are there for either the use inside of Runtime or Core.

kevinheavey · 2024-07-02T13:51:15Z

Put me on record as strongly opposed to calling the new crate solana-transaction for the reasons above

apfitzge · 2024-07-02T15:04:48Z

ended up just going with solana-svm-transaction, SVMMessage and SVMTransaction.

nit on myself: any preference for SVM vs Svm in trait naming? Seems "proper" to do SVM, but also a bit unwieldy to read MMessage

buffalojoec

Nice, looking great! I left a few comments.

buffalojoec · 2024-07-03T04:15:42Z

svm-transaction/Cargo.toml

+solana-program = { workspace = true }
+solana-sdk = { workspace = true }


You only need solana-sdk here, since it re-exports all of solana-program.

svm-transaction/src/lib.rs

svm-transaction/src/svm_transaction/reference.rs

svm-transaction/src/svm_transaction.rs

svm-transaction/src/svm_message/reference.rs

svm-transaction/src/svm_message/sanitized_message.rs

apfitzge · 2024-07-03T16:11:38Z

I'll raise a question here about a related change.

There are several convenience functions that I plan to implement which use this trait, but imo don't belong on the trait itself.
These are largely fns that are useful within our codebase rather than something that needs to be public for using SVM, and I think ideally we'd either version it separately from this trait crate.

Some examples of functions I anticipate adding to that new module/crate:

    fn writable_accounts_iter(message: &impl SVMMessage) -> impl Iterator<Item = &Pubkey> + Clone;
    fn readable_accounts_iter(message: &impl SVMMessage) -> impl Iterator<Item = &Pubkey> + Clone;
    fn verify_precompiles(message: &impl SVMMessage) -> Result<(), TransactionError>;
    fn get_durable_nonce(message: &impl SVMMessage) -> Option<&Pubkey>;
    fn get_signature_details(message: &impl SVMMessage) -> TransactionSignatureDetails;

My gut feeling is these belong in some dependent crate, but wonder if you all think these belong in the same crate?

buffalojoec · 2024-07-03T18:49:27Z

There are several convenience functions that I plan to implement which use this trait, but imo don't belong on the trait itself. These are largely fns that are useful within our codebase rather than something that needs to be public for using SVM, and I think ideally we'd either version it separately from this trait crate.

I'd agree and lean toward a separate crate - maybe something like solana-svm-transaction-extractors. Just be careful not to end up with a miscellaneous kitchen-sink of utils.

apfitzge · 2024-07-11T18:56:54Z

@buffalojoec @LucasSte I removed a few extra things from SVMTransaction; it only has the accessors for signatures.
I think the other stuff can be an extended trait that is only used inside of core/runtime. Since we never use that stuff inside of SVM I think that makes more sense.

buffalojoec

Nice, lgtm!

I think the other stuff can be an extended trait that is only used inside of core/runtime. Since we never use that stuff inside of SVM I think that makes more sense.

Yep, I agree. This makes more sense.

apfitzge commented Jul 1, 2024

View reviewed changes

placeholder-transaction-trait/src/message_trait.rs Outdated Show resolved Hide resolved

apfitzge commented Jul 1, 2024

View reviewed changes

placeholder-transaction-trait/src/message_trait.rs Outdated Show resolved Hide resolved

apfitzge commented Jul 1, 2024

View reviewed changes

placeholder-transaction-trait/src/transaction_trait.rs Outdated Show resolved Hide resolved

apfitzge commented Jul 1, 2024

View reviewed changes

placeholder-transaction-trait/src/message_trait.rs Outdated Show resolved Hide resolved

apfitzge mentioned this pull request Jul 2, 2024

Update crate ownership transfer name #1970

Merged

apfitzge force-pushed the svm_transaction branch from eaf6266 to 85bd9e4 Compare July 2, 2024 18:53

apfitzge marked this pull request as ready for review July 2, 2024 20:47

apfitzge requested review from buffalojoec, LucasSte and tao-stones July 2, 2024 20:47

buffalojoec reviewed Jul 3, 2024

View reviewed changes

LucasSte reviewed Jul 3, 2024

View reviewed changes

svm-transaction/src/svm_message/sanitized_message.rs Show resolved Hide resolved

apfitzge mentioned this pull request Jul 3, 2024

TransactionCost - remove allocation #1987

Closed

apfitzge force-pushed the svm_transaction branch from 38e35bc to 869cb57 Compare July 10, 2024 00:45

apfitzge added 21 commits July 11, 2024 13:45

impl MessageTrait for SanitizedTransaction

7f0b3da

impl MessageTrait for &T

b829317

TransactionTrait

a41d562

impl TransactionTrait for &SanitizedTransaction

6db5970

impl TransactionTrait for &T

1a2be1e

crate: solana-transaction

017339f

rename SolanaTransaction and SolanaMessage

80cc8a2

move files/folders

09ce195

reduce interface

267d22c

remove Clone requirement

e79d7bf

simplify further

54806f8

move/rename crate: solana-svm-transaction

39ae848

rename/move files

0f17b94

rename traits

eea3b44

revert pub-ing of TransactionSignatureDetails fields

4e81e11

consistent style in implementations

ac1f89f

remove blanket reference implementations

dd3072a

remove glob exports

fe47f5f

remove explicit solana-program dependency

bf8b20d

rename helper types w/ SVM prefix

6319c6a

remove extra features from SVMTransaction

d131fea

apfitzge force-pushed the svm_transaction branch from 8950611 to d131fea Compare July 11, 2024 18:46

apfitzge requested a review from buffalojoec July 11, 2024 20:18

buffalojoec approved these changes Jul 12, 2024

View reviewed changes

LucasSte approved these changes Jul 12, 2024

View reviewed changes

apfitzge merged commit a191a77 into anza-xyz:master Jul 12, 2024
51 checks passed

apfitzge deleted the svm_transaction branch July 12, 2024 14:44

This was referenced Jul 12, 2024

[Tracking] SVM: Generic over Transaction Type #2108

Closed

Rename/deprecate num_signatures to num_total_signatures #2200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic Transaction Trait #1918

Generic Transaction Trait #1918

apfitzge commented Jun 28, 2024 •

edited

Loading

apfitzge commented Jun 28, 2024 •

edited

Loading

buffalojoec commented Jun 28, 2024

apfitzge commented Jun 28, 2024

kevinheavey commented Jun 29, 2024 •

edited

Loading

buffalojoec commented Jun 29, 2024

kevinheavey commented Jun 29, 2024 •

edited

Loading

LucasSte commented Jun 30, 2024

apfitzge commented Jul 1, 2024 •

edited

Loading

apfitzge Jul 1, 2024

apfitzge Jul 1, 2024

apfitzge Jul 11, 2024

buffalojoec commented Jul 1, 2024

apfitzge commented Jul 2, 2024

kevinheavey commented Jul 2, 2024

apfitzge commented Jul 2, 2024

buffalojoec left a comment

buffalojoec Jul 3, 2024

apfitzge commented Jul 3, 2024

buffalojoec commented Jul 3, 2024

apfitzge commented Jul 11, 2024 •

edited

Loading

buffalojoec left a comment

		solana-program = { workspace = true }
		solana-sdk = { workspace = true }

Generic Transaction Trait #1918

Generic Transaction Trait #1918

Conversation

apfitzge commented Jun 28, 2024 • edited Loading

Problem

Summary of Changes

apfitzge commented Jun 28, 2024 • edited Loading

buffalojoec commented Jun 28, 2024

apfitzge commented Jun 28, 2024

kevinheavey commented Jun 29, 2024 • edited Loading

buffalojoec commented Jun 29, 2024

kevinheavey commented Jun 29, 2024 • edited Loading

LucasSte commented Jun 30, 2024

apfitzge commented Jul 1, 2024 • edited Loading

apfitzge Jul 1, 2024

Choose a reason for hiding this comment

apfitzge Jul 1, 2024

Choose a reason for hiding this comment

apfitzge Jul 11, 2024

Choose a reason for hiding this comment

buffalojoec commented Jul 1, 2024

apfitzge commented Jul 2, 2024

kevinheavey commented Jul 2, 2024

apfitzge commented Jul 2, 2024

buffalojoec left a comment

Choose a reason for hiding this comment

buffalojoec Jul 3, 2024

Choose a reason for hiding this comment

apfitzge commented Jul 3, 2024

buffalojoec commented Jul 3, 2024

apfitzge commented Jul 11, 2024 • edited Loading

buffalojoec left a comment

Choose a reason for hiding this comment

apfitzge commented Jun 28, 2024 •

edited

Loading

apfitzge commented Jun 28, 2024 •

edited

Loading

kevinheavey commented Jun 29, 2024 •

edited

Loading

kevinheavey commented Jun 29, 2024 •

edited

Loading

apfitzge commented Jul 1, 2024 •

edited

Loading

apfitzge commented Jul 11, 2024 •

edited

Loading