TransactionBatch - hold RuntimeTransaction #3041

apfitzge · 2024-09-30T21:28:50Z

Problem

RuntimeTransaction helps us in multiple ways:
- caches metadata so we do not re-calculate later in pipeline
- easier transition to new transaction type (fields not present on new tx type are cached)

Summary of Changes

Use RuntimeTransaction wherever required by forcing it in TransactionBatch
RuntimeTransaction implements Deref for the wrapped transaction type, so in many places this does not require any changes in function implementation
This PR is a near minimum of changes required to get TransactionBatch to use RuntimeTransaction. This makes the type available wherever we may need it in our processing pipeline, and we can optimize some of the processing to use the meta in the pipeline in subsequent PRs.

Fixes #

mergify · 2024-09-30T21:29:28Z

If this PR represents a change to the public RPC API:

Make sure it includes a complementary update to rpc-client/ (example)
Open a follow-up PR to update the JavaScript client @solana/web3.js (example)

Thank you for keeping the RPC clients in sync with the server API @apfitzge.

unified-scheduler-logic/src/lib.rs

apfitzge · 2024-10-07T14:10:41Z

runtime-transaction/src/transaction_meta.rs

@@ -24,7 +24,7 @@ use {
 /// for example: message hash, simple-vote-tx flag, limits set by instructions
 pub trait StaticMeta {
    fn message_hash(&self) -> &Hash;
-    fn is_simple_vote_tx(&self) -> bool;
+    fn is_simple_vote_transaction(&self) -> bool;


@tao-stones changed the name here to be consistent with SanitizedTransaction, meaning no impl changes when we start to use the transaction traits.
Prefer the longer/explicit name anyway instead of abbreviation.

apfitzge · 2024-10-07T14:11:48Z

core/src/banking_stage/committer.rs

+            let txs = batch
+                .sanitized_transactions()
+                .iter()
+                .map(|tx| tx.deref().clone())


will be replaced by use of upcoming trait SVMTransactionAdapter in a future PR

banks-server/src/banks_server.rs

runtime-transaction/src/runtime_transaction.rs

core/src/banking_stage/consumer.rs

core/src/banking_stage/immutable_deserialized_packet.rs

jstarry · 2024-10-07T16:17:53Z

runtime/src/prioritization_fee_cache.rs

@@ -204,13 +209,11 @@ impl PrioritizationFeeCache {
                    continue;
                }

-                let compute_budget_limits = process_compute_budget_instructions(


jstarry · 2024-10-07T16:21:18Z

I'm in favor of introducing SVMTransactionAdapter in this PR to clean up the derefs

apfitzge · 2024-10-07T16:46:35Z

I'm in favor of introducing SVMTransactionAdapter in this PR to clean up the derefs

Introducing it now only gets rid of one deref. think for the comparison on fully_verify_transaction we need some other mechanism for comparing. Agree with you that comparing the .deref() of each isnt' good.

tao-stones · 2024-10-07T22:05:42Z

core/src/banking_stage/immutable_deserialized_packet.rs

+                address_loader,
+                reserved_account_keys,
+            )
+        })


So it has two steps here: creating the "static" version of RuntimeTransaction::<SanitizedVersionedTransaction>, then creating the "dynamic" version of RuntimeTransaction<SanitizedTransaction>; this illustrates the ideal use of RuntimeTransaction, which is: the "static" version can advance to "dynamic" version; and "dynamic" version is the superset of "static" version.

How do you think to create "static" version in ImmutableDeserializedPacket::new(), then consume "static" by advance it into "dynamic" when needed? I know you are expecting IDP to be retired soon, but I think it's worth to update IDP to properly use RuntimeTransaction to shake out its usages, but also save one clone of SanitizedVersionedTransaction at ln 128.

I'm thinking something like this:

change ImmutableDeserializePacket def to:

enum RuntimeTransactionVersion { Static (RuntimeTransaction::<SanitizedVersionedTransaction>), Dynamic (RuntimeTransaction::<SanitizedTransaction>), } pub struct ImmutableDeserializedPacket { original_packet: Packet, runtime_transaction: RuntimeTransactionVersion, // ^^ it caches the rest of stuff IDP used to have }

change new() to:

pub fn new(packet: Packet) -> Result<Self, DeserializedPacketError> { let versioned_transaction: VersionedTransaction = packet.deserialize_slice(..)?; let sanitized_transaction = SanitizedVersionedTransaction::try_from(versioned_transaction)?; Ok(Self { original_packet: packet, runtime_transaction: Static(RuntimeTransaction::<SanitizedVersionedTransaction>::try_from( sanitized_transaction, None, None)), }) }

So the build_sanitized_transaction() would be renamed to:

pub fn get_sanitized_transaction( &mut self, votes_only: bool, address_loader: impl AddressLoader, reserved_account_keys: &HashSet<Pubkey>, ) -> Result<()> { if votes_only && !self.is_simple_vote() { return None; } if self.runtime_transaction.is_static() { // consume static, advance to dynamic self.runtime_transaction = Dynamic( RuntimeTransaction::<SanitizedTransaction>::try_from( self.runtime_transaction, address_loader, reserved_account_keys) )?; } Ok(()) }

(sorry for pseudo code may not be practical)

could do something like that. enum should be relatively light overhead. i'll test it out tomorrow.

Thinking through it this morning. I'm not sure it's worth the added complexity.

If we stick to not implementing Clone for RuntimeTransaction (could practice imo), then to process the batch we need to take owership of the RuntimeTransaction; we'd then need to put the RuntimeTransaction back into the ImmutableDeserializedPacket on retry.
This adds a ton of complexity and possible bugs.

I think also transitionining from static->dynamic runtime also breaks the naming of Immutable.

one option is to only implement Clone for RuntimeTransaction<SanitizedTransaction>; but that feels a bit dirty, given how much effort I just spent to remove all the SanitizedTransaction::clone from replay stage.

Chatted offline, we believed IDP will be refactored out soon, so we'll leave it as is for now.

Sounds fine to leave as is but was also wondering about if we create and store a RuntimeTransaction<SanitizedVersionedTransaction> inside ImmutableDeserializedPacket::new and then add a new RuntimeTransaction<SanitizedTransaction>::try_from_ref function which takes a &RuntimeTransaction::<SanitizedVersionedTransaction> and does the clone internally?

This brings back some concern we had about adding Clone to the TransactionMeta which means we must be careful about the Meta size. Adding Clone now may it difficult for us to remove in the future.

See above comment, the IDP is short-lived and will be removed soon.

apfitzge · 2024-10-17T18:32:28Z

cost-model/src/transaction_cost.rs

@@ -139,7 +147,6 @@ pub struct UsageCostDetails<'a, Tx: SVMMessage> {
    pub programs_execution_cost: u64,
    pub loaded_accounts_data_size_cost: u64,
    pub allocated_accounts_data_size: u64,
-    pub signature_details: TransactionSignatureDetails,


this is stored by RuntimeTransaction so we can get rid of it here

tao-stones

looks great!

tao-stones · 2024-10-29T21:06:38Z

core/src/banking_stage/immutable_deserialized_packet.rs

+                address_loader,
+                reserved_account_keys,
+            )
+        })


Chatted offline, we believed IDP will be refactored out soon, so we'll leave it as is for now.

tao-stones · 2024-10-29T21:44:00Z

ledger/src/blockstore_processor.rs

+        let transactions: Vec<SanitizedTransaction> = batch
+            .sanitized_transactions()
+            .iter()
+            .map(|tx| tx.deref().clone())


Thought we touched on this before, just to be clear, the deref().clone() doesn't have meaningful impact on perf, as this function is on hot path.

apfitzge · 2024-10-30T16:26:33Z

@jstarry @tao-stones I rebased on master just to make sure CI still passes - make sure no SanitizedTransactions snuck in in new tests anywhere.

I also pushed the SVMTransactionAdapter trait so we can remove some of the derefs that were previously being done.

jstarry · 2024-11-01T16:22:52Z

cost-model/src/cost_model.rs

@@ -624,45 +616,6 @@ mod tests {
        assert_eq!(1, data_bytes_cost);
    }

-    #[test]
-    fn test_cost_model_with_failed_compute_budget_transaction() {


I think we can keep this test but change it so that there is a compute budget ix that fails in sanitize_and_convert_to_compute_budget_limits rather than in ComputeBudgetInstructionDetails::try_from since any errors in try_from would be caught earlier when RuntimeTransaction is created. Also, shouldn't get_transaction_cost be updated to use a runtime tx to avoid reprocessing compute budget ixs?

Added this test back as you described - ended up just updating cost-model code to use RuntimeTransaction as well to make it so I don't need to update the test again next PR

jstarry · 2024-11-01T16:32:07Z

core/src/banking_stage/consumer.rs

@@ -24,7 +24,10 @@ use {
        transaction_batch::TransactionBatch,
        verify_precompiles::verify_precompiles,
    },
-    solana_runtime_transaction::instructions_processor::process_compute_budget_instructions,
+    solana_runtime_transaction::{


Is there a reason that execute_and_commit_transactions_locked is recomputing the compute budget details still rather than using them from runtime tx?

jstarry · 2024-11-01T16:33:10Z

core/src/banking_stage/consumer.rs

-    solana_runtime_transaction::instructions_processor::process_compute_budget_instructions,
+    solana_runtime_transaction::{
+        instructions_processor::process_compute_budget_instructions,
+        runtime_transaction::RuntimeTransaction,


Also how come check_fee_payer_unlocked isn't using runtime tx now?

jstarry · 2024-11-01T16:45:50Z

core/src/banking_stage/immutable_deserialized_packet.rs

+                address_loader,
+                reserved_account_keys,
+            )
+        })


Sounds fine to leave as is but was also wondering about if we create and store a RuntimeTransaction<SanitizedVersionedTransaction> inside ImmutableDeserializedPacket::new and then add a new RuntimeTransaction<SanitizedTransaction>::try_from_ref function which takes a &RuntimeTransaction::<SanitizedVersionedTransaction> and does the clone internally?

jstarry · 2024-11-01T16:49:06Z

core/src/banking_stage/transaction_scheduler/scheduler_controller.rs

@@ -28,7 +28,10 @@ use {
    solana_cost_model::cost_model::CostModel,
    solana_measure::measure_us,
    solana_runtime::{bank::Bank, bank_forks::BankForks},
-    solana_runtime_transaction::instructions_processor::process_compute_budget_instructions,
+    solana_runtime_transaction::{


We could eliminate one compute budget calculation by using the compute budget details inside runtime tx inside buffer_packets

jstarry

I don't think it's very ergonomic to not implement clone for runtime tx, especially for tests. I would be in favor of adding a clone impl generally but can we at the very least add a dcou impl?

#[cfg(feature = "dev-context-only-utils")]
impl<T> Clone for RuntimeTransaction<T>
where
    T: Clone,
{
    fn clone(&self) -> Self {
        Self {
            transaction: self.transaction.clone(),
            meta: self.meta.clone(),
        }
    }
}

jstarry · 2024-11-01T17:18:25Z

runtime-transaction/src/svm_transaction_adapter.rs

+/// that are required by some outside interfaces.
+/// Eventually this trait should go away, but for now it is necessary.
+pub trait SVMTransactionAdapter: SVMTransaction {
+    fn as_sanitized_transaction(&self) -> impl Borrow<SanitizedTransaction>;


I think it makes more sense to have a to_sanitized_transaction method which does the clone internally since we only really need this adapter method for clones. The commit_transactions call-site for geyser can use deref directly

commit_transactions is where we actually need/want this as borrow.

Because we need to have a SanitizedTransaction and if we don't we need to convert to one - it can't just be deref once we are generic.

but doing the borrow there also let's us not clone the the SanitizedTransaction if that's what we already have

apfitzge force-pushed the sep30_big_runtime_transaction_transition branch 2 times, most recently from fb11a17 to 3fddfe9 Compare October 1, 2024 13:52

apfitzge self-assigned this Oct 1, 2024

apfitzge mentioned this pull request Oct 2, 2024

RuntimeTransaction::try_new_from_sanitized_transaction #2674

Closed

apfitzge force-pushed the sep30_big_runtime_transaction_transition branch 4 times, most recently from b766751 to 7c93e54 Compare October 7, 2024 13:16

apfitzge marked this pull request as ready for review October 7, 2024 14:07

apfitzge requested review from tao-stones and ryoqun October 7, 2024 14:07

apfitzge commented Oct 7, 2024

View reviewed changes

unified-scheduler-logic/src/lib.rs Outdated Show resolved Hide resolved

apfitzge commented Oct 7, 2024

View reviewed changes

apfitzge requested a review from jstarry October 7, 2024 14:35

jstarry reviewed Oct 7, 2024

View reviewed changes

apfitzge mentioned this pull request Oct 7, 2024

RuntimeTransaction::is_simple_vote_transaction #3099

Merged

tao-stones reviewed Oct 7, 2024

View reviewed changes

apfitzge force-pushed the sep30_big_runtime_transaction_transition branch from 7c93e54 to 33de7d8 Compare October 17, 2024 18:25

apfitzge commented Oct 17, 2024

View reviewed changes

apfitzge requested review from tao-stones and jstarry October 17, 2024 22:21

apfitzge added 4 commits October 29, 2024 12:42

RuntimeTransaction::try_create

c309b8e

use RuntimeTransaction

bfb09f6

remove broken test - runtime transaction verifies this doesn't happen

7df13fe

Use MessageHash

3303c0f

tao-stones previously approved these changes Oct 29, 2024

View reviewed changes

apfitzge added 2 commits October 30, 2024 11:14

SVMTransactionAdapter

abc2691

use SVMTransactionAdapter for as_sanitized_transaction

2c50748

apfitzge dismissed tao-stones’s stale review via 2c50748 October 30, 2024 16:25

apfitzge force-pushed the sep30_big_runtime_transaction_transition branch from f7d6794 to 2c50748 Compare October 30, 2024 16:25

fix newly added test

9f67c71

jstarry reviewed Nov 1, 2024

View reviewed changes

apfitzge added 2 commits November 4, 2024 09:34

Add dcou derive clone and revert some test changes

9787bbe

re-add test w/ runtime cost-model failure

b5c2255

jstarry approved these changes Nov 4, 2024

View reviewed changes

apfitzge mentioned this pull request Nov 4, 2024

SVMTransactionAdapter #3039

Closed

apfitzge changed the title ~~Use RuntimeTransaction~~ TransactionBatch - hold RuntimeTransaction Nov 4, 2024

apfitzge merged commit 9b56f30 into anza-xyz:master Nov 4, 2024
52 checks passed

apfitzge deleted the sep30_big_runtime_transaction_transition branch November 4, 2024 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TransactionBatch - hold RuntimeTransaction #3041

TransactionBatch - hold RuntimeTransaction #3041

apfitzge commented Sep 30, 2024 •

edited

Loading

mergify bot commented Sep 30, 2024

apfitzge Oct 7, 2024

apfitzge Oct 7, 2024

jstarry Oct 7, 2024

jstarry commented Oct 7, 2024

apfitzge commented Oct 7, 2024

tao-stones Oct 7, 2024

apfitzge Oct 7, 2024

apfitzge Oct 8, 2024

apfitzge Oct 8, 2024 •

edited

Loading

tao-stones Oct 29, 2024

jstarry Nov 1, 2024

apfitzge Nov 4, 2024

apfitzge Oct 17, 2024 •

edited

Loading

tao-stones left a comment

tao-stones Oct 29, 2024

tao-stones Oct 29, 2024

apfitzge commented Oct 30, 2024

jstarry Nov 1, 2024

apfitzge Nov 4, 2024

jstarry Nov 1, 2024

jstarry Nov 1, 2024

jstarry Nov 1, 2024

jstarry Nov 1, 2024

jstarry left a comment

jstarry Nov 1, 2024

apfitzge Nov 1, 2024

apfitzge Nov 1, 2024

TransactionBatch - hold RuntimeTransaction #3041

TransactionBatch - hold RuntimeTransaction #3041

Conversation

apfitzge commented Sep 30, 2024 • edited Loading

Problem

Summary of Changes

mergify bot commented Sep 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry commented Oct 7, 2024

apfitzge commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

tao-stones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge commented Sep 30, 2024 •

edited

Loading

apfitzge Oct 8, 2024 •

edited

Loading

apfitzge Oct 17, 2024 •

edited

Loading