Rename types and metrics from executed to processed #2435

jstarry · 2024-08-05T01:53:03Z

Problem

Once we implement solana-foundation/solana-improvement-documents#82 we will need to distinguish between valid transactions that were executed and those that were not able to be executed (but still able valid for committing to a block).

Summary of Changes

Replaced usages of TransactionExecutionResult with a type alias TransactionProcessingResult which for now only wraps an ExecutedTransaction but in the future will wrap an enum that additionally has variants for valid transactions that were not executed.
Renamed consume worker and leader slot metric names to use "processed" instead of "executed"
Renamed other structs and variable names to use "processed" instead of "executed"

Fixes #

LucasSte · 2024-08-06T14:01:36Z

core/src/banking_stage/consumer.rs

+    pub(crate) processed_count: u64,
+    // Total number of the processed transactions that returned success/not


Can a processed transaction have failed? I think this parameter counts the number of transaction that succeeded (were executed).

Can a processed transaction have failed?

Yes a processed transaction could have had "failure" result meaning that we definitely validated the fee payer but may or may not have executed it successfully.

I think this parameter counts the number of transaction that succeeded (were executed)

This parameter used to count the number of transactions that were executed (whether the execution result was success or failure, is irrelevant). Now it counts the number of transactions that were "processed" (meaning valid for including in a block) whether they were executed or not.

LucasSte · 2024-08-06T14:26:28Z

runtime/src/bank.rs

                Ok(()) => {
-                    execution_counts.executed_successfully_count += 1;
+                    processed_counts.processed_successfully += 1;


Related to my comment above, are you considering that a transaction whose execution has failed has been successfully processed?

No, only transactions that were executed with a successful result are considered to have been "processed successfully" here.

Does "processed successfully" mean valid for including in a block?

No it means that they were executed with a successful execution result

I renamed to processed_with_successful_result_count

apfitzge · 2024-08-07T17:52:09Z

core/src/banking_stage/consumer.rs

@@ -1665,9 +1666,9 @@ mod tests {
            assert_eq!(
                transaction_counts,
                ExecuteAndCommitTransactionsCounts {


should this struct not be renamed too?

Yeah sure, I renamed to LeaderProcessedTransactionCounts

runtime/src/bank.rs

LucasSte · 2024-08-08T14:55:19Z

core/src/banking_stage/committer.rs

@@ -145,7 +147,7 @@ impl Committer {
            let batch_transaction_indexes: Vec<_> = commit_results
                .iter()
                .map(|commit_result| {
-                    if commit_result.was_executed() {
+                    if commit_result.was_committed() {


Should this be was_processed?

Committed implies processed. I think committed is more appropriate here

Yeah, think that committed is probably more appropriate here. This is meant to gather and communicate state changes, which will only occur if the tx is committed.

LucasSte · 2024-08-08T14:56:56Z

core/src/banking_stage/consumer.rs

@@ -71,15 +72,15 @@ pub struct ExecuteAndCommitTransactionsOutput {
 }

 #[derive(Debug, Default, PartialEq)]
-pub struct ExecuteAndCommitTransactionsCounts {
+pub struct LeaderProcessedTransactionCounts {
    // Total number of transactions that were passed as candidates for execution


Suggested change

// Total number of transactions that were passed as candidates for execution

// Total number of transactions that were passed as candidates for processing

LucasSte · 2024-08-08T15:30:34Z

core/src/banking_stage/consumer.rs

-                    if execution_result.was_executed() {
+                .filter_map(|(processing_result, tx)| {
+                    if processing_result.was_processed() {


In this case not only is the function name changing, but also the semantics. The was_executed() can be false even if the fee validation has passed, but was_processed() would be true in that case, right?

Yes, that's correct

LucasSte · 2024-08-08T15:31:44Z

core/src/banking_stage/consumer.rs

-                ExecuteAndCommitTransactionsCounts {
-                    attempted_execution_count: 1,
+                LeaderProcessedTransactionCounts {
+                    attempted_processing_count: 1,
                    // Transactions was still executed, just wasn't committed, so should be counted here.


Suggested change

// Transactions was still executed, just wasn't committed, so should be counted here.

// Transaction was still processed, just wasn't committed, so should be counted here.

LucasSte · 2024-08-08T15:41:58Z

svm/src/transaction_processing_result.rs

+    fn was_processed(&self) -> bool {
+        self.is_ok()
+    }


You are considering not processed if self is err, but you also replaced NotExecuted by Err. Are you accounting for the case where a processed transaction may have not been executed?

Not yet in this change. That change to support processing transactions that may not have been executed will come next in #2425. See https://github.com/anza-xyz/agave/pull/2425/files#diff-5eb12bc3fc00b926c18e4204f316d650074ce4fe4bce7d664459c3cf340af5aeR21-R24

jstarry · 2024-08-09T00:49:28Z

force pushed to rebase on master in order to pick up new svm/examples which also had some things that needed to be renamed. See b94019a

jstarry requested review from apfitzge, pgarg66 and LucasSte August 6, 2024 10:26

LucasSte reviewed Aug 6, 2024

View reviewed changes

apfitzge reviewed Aug 7, 2024

View reviewed changes

runtime/src/bank.rs Show resolved Hide resolved

apfitzge previously approved these changes Aug 8, 2024

View reviewed changes

LucasSte reviewed Aug 8, 2024

View reviewed changes

jstarry dismissed apfitzge’s stale review via d17ae1a August 9, 2024 00:16

jstarry added 4 commits August 9, 2024 00:38

Rename types and metrics from executed to processed

2c6f847

feedback

edc5214

feedback

6ee6d40

more fixes

b94019a

jstarry force-pushed the refactor/rename-to-processed branch from d17ae1a to b94019a Compare August 9, 2024 00:48

LucasSte approved these changes Aug 9, 2024

View reviewed changes

jstarry merged commit 935b45a into anza-xyz:master Aug 9, 2024
41 checks passed

jstarry deleted the refactor/rename-to-processed branch August 9, 2024 18:03

ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024

Rename types and metrics from executed to processed (anza-xyz#2435)

cc2ccad

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename types and metrics from executed to processed #2435

Rename types and metrics from executed to processed #2435

jstarry commented Aug 5, 2024

LucasSte Aug 6, 2024

jstarry Aug 6, 2024

LucasSte Aug 6, 2024

jstarry Aug 6, 2024

LucasSte Aug 7, 2024

jstarry Aug 7, 2024

jstarry Aug 8, 2024

apfitzge Aug 7, 2024

jstarry Aug 8, 2024

LucasSte Aug 8, 2024

jstarry Aug 8, 2024

apfitzge Aug 8, 2024

LucasSte Aug 8, 2024

LucasSte Aug 8, 2024

jstarry Aug 9, 2024

LucasSte Aug 8, 2024

LucasSte Aug 8, 2024

jstarry Aug 9, 2024

jstarry commented Aug 9, 2024

		pub(crate) processed_count: u64,
		// Total number of the processed transactions that returned success/not

	// Total number of transactions that were passed as candidates for execution
	// Total number of transactions that were passed as candidates for processing

	// Transactions was still executed, just wasn't committed, so should be counted here.
	// Transaction was still processed, just wasn't committed, so should be counted here.

Rename types and metrics from executed to processed #2435

Rename types and metrics from executed to processed #2435

Conversation

jstarry commented Aug 5, 2024

Problem

Summary of Changes

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

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

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 commented Aug 9, 2024