-
Notifications
You must be signed in to change notification settings - Fork 1k
Consider Soroban limits in overlay #4925
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
base: master
Are you sure you want to change the base?
Conversation
bboston7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together. I think the approach of adding soroban limits to our existing limits is entirely logical, but (as you mention) I wonder whether our existing limits might need tweaking given that they assume that every transaction is a single op. At the same time, it seems to be working well, so maybe it's not worth touching that? I'd like to rope @marta-lokhova and @SirTyson into that broader discussion about these heuristics.
The calculation is also repeated a few times to support iteration (in this PR) and potential differences between pull mode and flood mode—this will need to get cleaned up.
I think we'll want these to be the same? Otherwise I worry about one choking the other.
| TxAdverts::getTxLimit() | ||
| { | ||
| auto& lm = mApp.getLedgerManager(); | ||
| size_t classic = lm.getLastMaxTxSetSizeOps(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you mention in your PR description, this assumes that all classic transactions are a single op, but that's probably not true. This has admittedly been working fine, but I wonder if there's a better way (such as dividing this value by the average number of ops per transaction). At the very least it might be worth checking what the average number of ops per classic transaction is these days to better answer the question of whether this needs adjusting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick Hubble query results
SELECT AVG(operation_count)
FROM crypto-stellar.crypto_stellar.history_transactions
WHERE batch_run_date >= DATE(2025, 05, 01) AND resource_fee = 0f0_
2.7467817158433689
SELECT AVG(operation_count)
FROM crypto-stellar.crypto_stellar.history_transactions
WHERE batch_run_date >= DATE(2025, 08, 01) AND resource_fee = 0f0_
3.3659880974068379
|
I think we have a path forward with this one based on the discussions last week:
|
Resolves #4732. Currently, this does the naive update of adding the Soroban tx limit to the classic ops limit. The calculation is also repeated a few times to support iteration (in this PR) and potential differences between pull mode and flood mode—this will need to get cleaned up. Putting this up now so the spots to update are apparent and so there is a spot for discussion.
Pull mode doesn't know ahead of time whether the transactions are Soroban or classic, but the queue limit is per peer, so we could plausibly lower it. Also, the limit is on advertised hashes that we haven't yet received full transactions for. Similarly, in flood mode, this is just limiting the outstanding messages to a given peer. That is, in both modes, although we do need some limit to cap maximum memory usage, the particular limit is somewhat arbitrary (consider also #3514).