Skip to content

CommitTransactionDetails - include result #7246

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 1, 2025
Merged

CommitTransactionDetails - include result #7246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b02c0c
return TransactionError in CommitTransactionDetails::NotCommitted
apfitzge Jul 30, 2025
65947a9
Add result to CommitTransactionDetails::Committed
apfitzge Jul 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions core/src/banking_stage/committer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use {
transaction_commit_result::{TransactionCommitResult, TransactionCommitResultExtensions},
transaction_processing_result::TransactionProcessingResult,
},
solana_transaction_error::TransactionError,
std::{num::Saturating, sync::Arc},
};

Expand All @@ -28,8 +29,9 @@ pub enum CommitTransactionDetails {
Committed {
compute_units: u64,
loaded_accounts_data_size: u32,
result: Result<(), TransactionError>,
},
NotCommitted,
NotCommitted(TransactionError),
}

#[derive(Clone)]
Expand Down Expand Up @@ -85,8 +87,9 @@ impl Committer {
loaded_accounts_data_size: committed_tx
.loaded_account_stats
.loaded_accounts_data_size,
result: committed_tx.status.clone(),
},
Err(_) => CommitTransactionDetails::NotCommitted,
Err(err) => CommitTransactionDetails::NotCommitted(err.clone()),
Comment on lines +90 to +92
Copy link
Author

@apfitzge apfitzge Jul 30, 2025

Choose a reason for hiding this comment

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

clones in most cases should be fast - 32 bytes.

only one variant is potentialy slow since it includes a String 🫠

Copy link
Author

@apfitzge apfitzge Jul 30, 2025

Choose a reason for hiding this comment

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

That String has been removed though, which reduces TransactionError size to 12 bytes 🥳

Will have to wait for sdk version bump to make it to gain the benefit.

anza-xyz/solana-sdk#12

})
.collect();

Expand Down
21 changes: 17 additions & 4 deletions core/src/banking_stage/consumer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,13 @@ impl Consumer {
} else {
(
0,
vec![CommitTransactionDetails::NotCommitted; processing_results.len()],
processing_results
.into_iter()
.map(|processing_result| match processing_result {
Ok(_) => unreachable!("processed transaction count is 0"),
Err(err) => CommitTransactionDetails::NotCommitted(err),
})
.collect(),
)
};

Expand Down Expand Up @@ -997,7 +1003,12 @@ mod tests {
assert!(retryable_transaction_indexes.is_empty());
assert_eq!(
commit_transactions_result.ok(),
Some(vec![CommitTransactionDetails::NotCommitted; 1])
Some(vec![
CommitTransactionDetails::NotCommitted(
TransactionError::AccountLoadedTwice
);
1
])
);

poh_recorder
Expand Down Expand Up @@ -1135,7 +1146,7 @@ mod tests {
);
assert_matches!(
commit_transactions_result.get(1),
Some(CommitTransactionDetails::NotCommitted)
Some(CommitTransactionDetails::NotCommitted(_))
);
assert_eq!(retryable_transaction_indexes, vec![1]);

Expand All @@ -1145,14 +1156,15 @@ mod tests {
CommitTransactionDetails::Committed {
compute_units,
loaded_accounts_data_size,
result: _,
} => (
*compute_units,
CostModel::calculate_loaded_accounts_data_size_cost(
*loaded_accounts_data_size,
&bank.feature_set,
),
),
CommitTransactionDetails::NotCommitted => {
CommitTransactionDetails::NotCommitted(_err) => {
unreachable!()
}
};
Expand Down Expand Up @@ -1787,6 +1799,7 @@ mod tests {
let CommitTransactionDetails::Committed {
compute_units,
loaded_accounts_data_size,
result: _,
} = consumer_output
.execute_and_commit_transactions_output
.commit_transactions_result
Expand Down
9 changes: 7 additions & 2 deletions core/src/banking_stage/qos_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ impl QosService {
CommitTransactionDetails::Committed {
compute_units,
loaded_accounts_data_size,
result: _,
} => {
cost_tracker.update_execution_cost(
tx_cost,
Expand All @@ -207,7 +208,7 @@ impl QosService {
),
);
}
CommitTransactionDetails::NotCommitted => {
CommitTransactionDetails::NotCommitted(_err) => {
cost_tracker.remove(tx_cost);
}
}
Expand Down Expand Up @@ -750,6 +751,7 @@ mod tests {
+ execute_units_adjustment,
loaded_accounts_data_size: loaded_accounts_data_size
+ loaded_accounts_data_size_adjustment,
result: Ok(()),
})
.collect();
let final_txs_cost = total_txs_cost
Expand Down Expand Up @@ -871,13 +873,16 @@ mod tests {
.enumerate()
.map(|(n, tx_cost)| {
if n % 2 == 0 {
CommitTransactionDetails::NotCommitted
CommitTransactionDetails::NotCommitted(
TransactionError::InsufficientFundsForFee,
)
} else {
CommitTransactionDetails::Committed {
compute_units: tx_cost.as_ref().unwrap().programs_execution_cost()
+ execute_units_adjustment,
loaded_accounts_data_size: loaded_accounts_data_size
+ loaded_accounts_data_size_adjustment,
result: Ok(()),
}
}
})
Expand Down
Loading