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.

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

Jump to bottom

Bugfix: MultiIterator batch priority guard #33454

Merged
merged 4 commits into from
Oct 5, 2023
Merged

Bugfix: MultiIterator batch priority guard #33454

merged 4 commits into from
Oct 5, 2023

Conversation

apfitzge
Copy link
Contributor

@apfitzge apfitzge commented Sep 29, 2023 •
edited
Loading

Problem

Shortcoming with the current multi-iterator decision function.
Allows non-conflicting txs to take higher-priority locks.

  • Take these transactions (internal nodes are accounts, edges indicate conflicts) for example:
graph LR
subgraph Tx1 - 3
Tx1_1(1)
style Tx1_1 fill:#CF0000
Tx1_2(2)
style Tx1_2 fill:#00CF00
end
subgraph Tx2 - 2
Tx2_2(2)
style Tx2_2 fill:#00CF00
Tx2_3(3)
style Tx2_3 fill:#0000CF
end
subgraph Tx3 - 2
Tx3_3(3)
style Tx3_3 fill:#0000CF
Tx3_4(4)
style Tx3_4 fill:#333333
end
Tx1_2 --> Tx2_2
Tx2_3 --> Tx3_3
Loading
  • Tx1 and Tx2 conflict so they cannot be in a batch together
  • Tx2 and Tx3 conflict so they cannot be in a batch together
  • However, Tx1 and Tx3 do not conflict, so they could be in a batch together
  • We do not want to allow Tx3 to execute before Tx2 though, since Tx2 has higher-priority.

Summary of Changes

  • Add a batch priority guard that prevents batch from taking locks on accounts that were previously seen during the inner iteration but were marked as Later.

Fixes #

@apfitzge
Copy link
Contributor Author

I have a branch off of this which makes the ReadWriteAccountSet just accumulate locks, which simplifies this code.
I want to make sure that won't break what I've got in #33332 before I push that here though.

@codecov
Copy link

codecov bot commented Sep 29, 2023 •
edited
Loading

Codecov Report

Merging #33454 (47de14e) into master (e3cd13e) will decrease coverage by 0.1%.
Report is 6 commits behind head on master.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #33454     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         799      799             
  Lines      217344   217367     +23     
=========================================
+ Hits       178169   178181     +12     
- Misses      39175    39186     +11

@tao-stones
Copy link
Contributor

tao-stones commented Sep 29, 2023 •
edited
Loading

We do not want to allow Tx3 to execute before Tx2 though, since Tx3 has higher-priority.

🤔 You mean "Tx3 has higherlower-priority"?

tao-stones
tao-stones previously approved these changes Sep 29, 2023
View reviewed changes
Copy link
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

lgtm

core/src/banking_stage/consumer.rs
@@ -2048,6 +2055,109 @@ mod tests {
Blockstore::destroy(ledger_path.path()).unwrap();
}

#[test]
fn test_consume_buffered_packets_batch_priority_guard() {
Copy link
Contributor

Choose a reason for hiding this comment

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

the test case makes sense to me.

@apfitzge
Copy link
Contributor Author

🤔 You mean "Tx3 has higherlower-priority"?

Yeah, thanks for pointing that out. I meant Tx2 has higher priority 😄

@apfitzge apfitzge marked this pull request as ready for review September 29, 2023 17:46
apfitzge
apfitzge commented Sep 29, 2023
View reviewed changes
core/src/banking_stage/read_write_account_set.rs
!message
/// Returns true if all account locks were available and false otherwise.
#[allow(dead_code)]
pub fn check_locks(&self, message: &SanitizedMessage) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused here, but I still need to check w/o adding for the central scheduler.

apfitzge
apfitzge commented Sep 29, 2023
View reviewed changes
core/src/banking_stage/read_write_account_set.rs
@@ -16,66 +13,42 @@ pub struct ReadWriteAccountSet {
}

impl ReadWriteAccountSet {
/// Check static account locks for a transaction message.
pub fn check_static_account_locks(&self, message: &VersionedMessage) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

checking static locks is not necessary anymore w/ priority-guarding the batch

tao-stones
tao-stones approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

lgtm

@apfitzge apfitzge merged commit 2a17be0 into solana-labs:master Oct 5, 2023
16 checks passed
@apfitzge apfitzge deleted the bugfix/MI-batch-priority-guard branch October 5, 2023 16:20
@willhickey willhickey mentioned this pull request Mar 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tao-stones tao-stones tao-stones approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@apfitzge @tao-stones