-
Notifications
You must be signed in to change notification settings - Fork 476
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
API: MaxTealSourceBytes to 512 kb #6068
Conversation
MaxTealSourceBytes to 1M algorand#6031
The issue you link to says:
I don't see any place in go-algorand that such an error ("teal code too big") is returned. Can you point it out? Ideally, there should indeed be a test that shows that such an error is generated. |
@jannotti Just read the #6031 thread pls. You can reproduce the issue when you create teal code longer then 200kb .. Feel free to write test for it. You can reproduce the issue when you clone https://github.com/scholtz/BiatecCLAMM/tree/feat/update-tealscript and run |
Since I quoted from that thread in my message, and I was the first commenter on that thread, you may safely assume that I have read it and understand it. Nowhere does that thread explain where the error "teal code to big" might come from, since that text does not appear in go-algorand. I suspect that the error message in question is somewhere in algokit, which would also explain why you are so reluctant to provide a test case. It is impossible to provide a test case in go-algorand because you are describing a poor interaction between algokit and go-algorand, not a bug in go-algorand. It appears that algokit does not understand that when it receives a "request body too large" response, it should report the error to the user as "teal code to big". That is something that should be changed in algokit. I do not object to raising the size of programs that algod can assemble, but if the fundamental problem is that algokit does not understand the error in question, we have simply masked the problem until a larger a program is sent. If algokit has a built-in constant that limits the size of teal code to 1M, that chould also be changed in algokit. It would be better for algokit to simply submit what it's given, understand the error "request body too large" and report the size limitation when the error is received. At any rate, there's work to be done in algokit, and possible also a raise of this constant if we actually think it's reasonable to send such large programs to algod. As is, this change masks the real problem. |
hi @jannotti I believe the error is thrown here in algod:
I believe this teal code size limit is not something that should limit devs from building, but rather the opcode budget should be proper limit. Please merge it in pls. |
I've got a contract that compiles to 6320 bytes or so (and will grow) and the teal for it is 142K. 200K seems a little on the edge in what's allowed to even be compiled at all, so raising the limit seems warranted. Obviously there's no way 1M teal would be runnable with current limits but going from 142K to 200K seems very easy to hit and still possibly have a 'runnable' contract <=8K. |
I imagine a significant factor here is |
I guess it is safe to raise since it is developer API (normally restricted) |
Can I get this answer? I'm totally ok with using a constant that is reasonably proportional to how many bytes you need to end up with a program that's about as big as allowed:
|
If we assume this is a reasonable ratio of source TEAL bytes to bytecode (22.4) then a maximum sized (current) app of 8k would need Allow a little slack, and I'd be in favor of a half mb limit. I won't fight that hard against 1 mb if others prefer it. Please speak up. |
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.
With my suggested change, I'm happy. With the 1MB limit, I'm basically ok too. I'll approve and let another approver weigh in.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6068 +/- ##
==========================================
+ Coverage 55.77% 56.12% +0.34%
==========================================
Files 488 488
Lines 69434 69434
==========================================
+ Hits 38730 38969 +239
+ Misses 28017 27813 -204
+ Partials 2687 2652 -35 ☔ View full report in Codecov by Sentry. |
Co-authored-by: John Jannotti <jannotti@gmail.com>
i am ok with 512*1024 as well.. thanks for review |
fix lint error
@jannotti @gmalouf @algorandskiy Can you merge it please? We are talking about one line of code for 3 months now and it is still blocker for me. I dont want to trim comments for production release |
Please note that all nodes will not support this until the next consensus upgrade goes into effect. |
MaxTealSourceBytes to 1M
Summary
Fixes #6031
Test Plan
Test plan not needed. Just increases one constant to little bigger number so that the teal compilation proceed.