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

Combine account locking and block space reserving in single loop #34909

Closed

Conversation

tao-stones
Copy link
Contributor

@tao-stones tao-stones commented Jan 23, 2024

Problem

Transaction takes up block space before acquiring accounts lock, if it fails getting lock, block space is not immediately released.

Summary of Changes

  • release reserved block space immediately after lock acquisition
  • replace transaction's cached cost with account lock error if block space released, this is to prevent possibility of double-removing its costs.

Fixes #34825

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8ff511e) 81.6% compared to head (7c3941c) 81.6%.
Report is 178 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34909     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         827      827             
  Lines      223884   223941     +57     
=========================================
+ Hits       182841   182876     +35     
- Misses      41043    41065     +22     

@tao-stones tao-stones marked this pull request as ready for review January 24, 2024 23:33
}
None
})
.collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to allocate a new vec here, we can just iterate over our qos and lock result pairs...something like

        // Remove reserved block space for transaction failed to acquire lock
        let mut cost_tracker = bank.write_cost_tracker().unwrap();
        transaction_qos_cost_results
            .iter_mut()
            .zip(batch.lock_results())
            .filter_map(|(qos_cost_result, lock_result)| {
                match (qos_cost_result.as_ref(), lock_result) {
                    (Ok(_), Err(lock_err)) => Some((qos_cost_result, lock_err)),
                    _ => None,
                }
            })
            .for_each(|(qos_cost_result, lock_err)| {
                cost_tracker.remove(qos_cost_result.as_ref().unwrap());
                *qos_cost_result = Err(lock_err.clone());
            });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL! I was having trouble to change qos_cost_result while it is borrowed by match (i think).

Before making the change tho, are we good with this approach, or would like to "lock-reserve-unlock" you mentioned elsewhere?

@@ -460,7 +460,7 @@ impl Consumer {
pre_results: impl Iterator<Item = Result<(), TransactionError>>,
) -> ProcessTransactionBatchOutput {
let (
(transaction_qos_cost_results, cost_model_throttled_transactions_count),
(mut transaction_qos_cost_results, cost_model_throttled_transactions_count),
Copy link
Contributor

Choose a reason for hiding this comment

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

as an alternative to mutability here. we could also use the lock-results to inform the cost-adjustment post-execution. Not sure if that would be cleaner though.

@github-actions github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Feb 20, 2024
@willhickey
Copy link
Contributor

This repository is no longer in use. Please re-open this pull request in the agave repo: https://github.com/anza-xyz/agave

@willhickey willhickey closed this Mar 3, 2024
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.

Transaction locking fails prematurely due to cost tracker constraints
3 participants