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

SendTransactionService sends in random order #35134

Open
apfitzge opened this issue Feb 7, 2024 · 3 comments
Open

SendTransactionService sends in random order #35134

apfitzge opened this issue Feb 7, 2024 · 3 comments

Comments

@apfitzge
Copy link
Contributor

apfitzge commented Feb 7, 2024

Problem

  • SendTransactionService is the primary way user transactions get sent from RPC to leader
  • SendTransactionService does not consider transaction priority at all in the sending order
    or in the dropping order when the "queue" is full. It is simply done randomly.
  • Random order incentivizes spamming multiple RPCs in hopes to randomly get to the top of their retry map

Proposed Solution

  • SendTransactionService should have some sort of priority-queue that we use to drop lowest priority transactions, and send/retry transactions with higher priority first.
  • Because we want to consider priority, we should be reasonably sure txs can pay for fees. We already load and check nonce-accounts, it seems reasonable we could load and check fee-payer balance.
@apfitzge
Copy link
Contributor Author

apfitzge commented Feb 7, 2024

@lijunwangs or @CriesofCarrots do you have thoughts on the proposed solution? It seems like the current behavior contributes to priority fees not working as well as they should, although there are certainly other factors at play.

@CriesofCarrots
Copy link
Contributor

SendTransactionService should have some sort of priority-queue that we use to drop lowest priority transactions, and send/retry transactions with higher priority first.

This feels a little dogmatic to me. It should really be up to the RPC operators how they want to filter or prioritize transactions. We could think about ways we could expose controls or customization to them, however...

Because we want to consider priority, we should be reasonably sure txs can pay for fees. We already load and check nonce-accounts, it seems reasonable we could load and check fee-payer balance.

The SendTransactionService only checks nonce accounts for (a) nonce transaction and (b) after the initial send to determine when to stop retrying. So much more limited than a fee-payer check before send. Adding this account load to every transaction before send will add some latency to the service, we can bench to see how much. In assessing whether that's worth imposing, we should see whether RPC operators generally support skipPreflight, because such a fee-payer check is already done in simulation.

@apfitzge
Copy link
Contributor Author

apfitzge commented Feb 8, 2024

This feels a little dogmatic to me. It should really be up to the RPC operators how they want to filter or prioritize transactions. We could think about ways we could expose controls or customization to them, however...

I'm all for letting things be modular and RPCs implementing their own logic, but we need to provide sane defaults, which the current one does not seem to be.

The SendTransactionService only checks nonce accounts for (a) nonce transaction and (b) after the initial send to determine when to stop retrying. So much more limited than a fee-payer check before send. Adding this account load to every transaction before send will add some latency to the service, we can bench to see how much. In assessing whether that's worth imposing, we should see whether RPC operators generally support skipPreflight, because such a fee-payer check is already done in simulation.

Happy to give RPCs ability to opt-out of fee-payer checks via some configuration similar to pre-flight.
However, pre-flight checks do not seem to be sufficient since they are only checked before we send the transaction the first timess; balance may have changed and the RPC is spamming transactions to leader that can never be processed due to lack of funds.

Adding this account load to every transaction before send will add some latency to the service, we can bench to see how much.

Yeah, agree this needs to be benched to see the affect. I think you are right in calling out what I said, in that nonce'd checks are far more limited.

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

No branches or pull requests

2 participants