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

feat(solana): add support for priority fees #11703

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

gabrielKerekes
Copy link
Contributor

@gabrielKerekes gabrielKerekes commented Mar 22, 2024

Add support for Solana priority fees to make it possible to submit transactions even at times of congestion.

Fixes #11518.

There's two versions of priority fees implemented in this PR. They're both dynamic i.e. if the network is congested the fees should increase automatically. However with one option we set a fixed compute unit limit of 50_000 for all transaction and with the other we leverage transaction simulation to get the number of compute units it needs and we use that for priority fees (with a 20% buffer). (here's an article about the second approach)

I consider the second approach to be a bit risky since for SOL transfers we only add 18 lamports to the fee i.e. the fee is 5018 instead of 5000 (edit: it's actually 54 lamports not 18 i.e. the fee is 5054). Theoretically this should work as expected since we do pay 100K per compute unit. But I really can't vouch for the Solana scheduler to prioritise smaller or more exactly estimated transactions - although it seems this should be the case when this PR gets released and validators update.

The first approach is used by other wallets - NuFi, Phantom and supposedly also Solflare. But it does result in larger fees and potentially transactions reserving more CUs are more difficult to schedule 🤷

I tested both approaches and transactions indeed did go through on this branch while they didn't go through on production (mainly token transactions didn't want to go through on production).

I'd prefer to start with the second, "risky" approach as it is more efficient overall. If problems would arise we could revert the two commits and fallback to static fees.

The last two commits of this PR are the ones adding tx simulation so if those are dropped then a constant compute unit limit will be used for all transactions.

We should test this properly once more during peak times which seems to be in the evening/night - I'll do that tonight.

This FW PR - trezor/trezor-firmware#3634 should be released alongside this PR since firmware's "predefined transactions" didn't consider ComputeBudget instructions before so the UI on Trezor wouldn't be aligned with Suite.

@gabrielKerekes
Copy link
Contributor Author

gabrielKerekes commented Mar 25, 2024

I pushed an additional commit 5ebba00 to attempt to fix Solana tx confirmation.

Sometimes it happened to me during testing that transactions would be properly executed but Suite wouldn't be notified about it - it would actually show a tx send error. It wasn't reliably reproducible so I'm not 100% that this fixes that issue.

However we previously first sent the transaction, then fetched the blockhash and then called confirmTransaction - so it might be possible that if the tx confirmation was too fast that maybe we didn't catch it? Not sure.

Now we take advantage of the web3 sdk's sendAndConfirmRawTransaction function which should hopefully reliably confirm transactions. More testing is required during network congestion - I will test again tonight. It also seemed to only happen on solana1.trezor.io but it might be just a coincidence.

cc @AdamSchinzel

Copy link

socket-security bot commented Mar 25, 2024

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@gabrielKerekes
Copy link
Contributor Author

I added 078cbe8. It makes the compute unit limit the same for all transactions i.e. we won't simulate transactions. The reason for the change is that during congestion the transactions with a proper amount of compute units allocated simply didn't go through. This might change if anza-xyz/agave#187 is used in production, but for now, I'd stick to static CU limit. This is what other wallets do as well and the transaction went through consistently with this setup.

Regarding the issue with transaction confirmation - I was able to reproduce the issue (still only during higher congestion) and it is indeed an issue with one of the RPC providers used. I'll report this to them and we can discuss the next steps offline. #11515 is actually also broken in the same way.

@tomasklim tomasklim requested a review from vytick March 26, 2024 06:15
Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

looks okay, would be nice if @vytick checks it

@gabrielKerekes
Copy link
Contributor Author

Regarding the issue with transaction confirmation - the issue should be resolved by e602ac3. As the commit description mentions:

Combine signatureSubscribe and getSignatureStatus when confirming transactions. This resolves the issue with Quicknode not sending a signature update event. Transaction will confirm if either a signature update event is received or if the periodic getSignatureStatus confirms the transaction as finalized.

@AdamSchinzel please review the last commit if you can 🙏 @vytick review the tx confirmation approach as well please.

Copy link
Contributor

@AdamSchinzel AdamSchinzel left a comment

Choose a reason for hiding this comment

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

Nice job with the transactionConfirmation.ts.

Just two points:

  • Now the steps inside the transaction review modal don't align with firmware steps at all. Could you add new steps and split the last steps into two individual since they are not on one screen.
  • Noticed that transaction gets added to transaction list after toast message disappears, It would be better if it gets added immediately.

@gabrielKerekes
Copy link
Contributor Author

Now the steps inside the transaction review modal don't align with firmware steps at all. Could you add new steps and split the last steps into two individual since they are not on one screen.

trezor/trezor-firmware#3634 fixes this.

Noticed that transaction gets added to transaction list after toast message disappears, It would be better if it gets added immediately.

That's not a simple fix (at least for me). The transaction is added as a result of the account being updated.

Copy link
Contributor

@vytick vytick left a comment

Choose a reason for hiding this comment

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

Looks good 👍

I have only few nit suggestions for better readability and one Q about the fees

Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

@gabrielKerekes could you please squash it into less commits? We need to cherrypick it. Thank you

Combine `signatureSubscribe` and `getSignatureStatus` when confirming transactions. This resolves the issue with Quicknode not sending a signature update event. Transaction will confirm if either a signature update event is received or if the periodic `getSignatureStatus` confirms the transaction as `finalized`.
@gabrielKerekes gabrielKerekes force-pushed the feat/solana-priority-fees branch from 47c5763 to ad45b89 Compare March 26, 2024 13:25
@gabrielKerekes
Copy link
Contributor Author

gabrielKerekes commented Mar 26, 2024

@gabrielKerekes could you please squash it into less commits? We need to cherrypick it. Thank you

Done. Is it good enough? I wouldn't squash them more.

cc @tomasklim

@tomasklim tomasklim merged commit 38cd7cc into trezor:develop Mar 27, 2024
59 of 61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Solana: Include priority fee in TX
4 participants