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

send-transactions: optimize retry pool #31

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

fanatid
Copy link

@fanatid fanatid commented Mar 3, 2024

Moved from solana-labs#35081

Problem

Transactions not included in the retry pool on full utilization

Summary of Changes

  • do not insert transactions with zero max_retries to the retry pool
  • remove transactions reached max_retries in the same iteration of the loop
  • dynamically select sleep time between iterations based on last_sent_time in TransactionInfo

@mergify mergify bot requested a review from a team March 3, 2024 17:47
@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Mar 13, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Mar 13, 2024
@CriesofCarrots
Copy link

Looks like this one needs a rebase to fixup some whitespace lint.

@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Mar 13, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Mar 13, 2024
@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Mar 15, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Mar 15, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.9%. Comparing base (151675b) to head (1e6a6fc).
Report is 18 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master      #31     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         836      836             
  Lines      226629   226634      +5     
=========================================
+ Hits       185688   185690      +2     
- Misses      40941    40944      +3     

Copy link

@godmodegalactus godmodegalactus left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -602,7 +628,8 @@ impl SendTransactionService {
) -> ProcessTransactionsResult {
let mut result = ProcessTransactionsResult::default();

let mut batched_transactions = HashSet::new();
let mut batched_transactions = Vec::new();

Choose a reason for hiding this comment

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

I think hashset was used to have unique transaction, could you clarify why it is not needed? Are they already filtered here by construction?

Copy link
Author

Choose a reason for hiding this comment

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

transactions already unique because filled from transactions object that is HashMap

Choose a reason for hiding this comment

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

good catch

@KirillLykov
Copy link

generally good changes, i don't understand some minor details (see questions in the comments)

@fanatid fanatid force-pushed the send-transactions-opt-retry-master-based branch from 1e6a6fc to c7c05af Compare December 2, 2024 20:47
@fanatid
Copy link
Author

fanatid commented Dec 2, 2024

updated with fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants