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

API: MaxTealSourceBytes to 512 kb #6068

Merged
merged 3 commits into from
Oct 25, 2024
Merged

API: MaxTealSourceBytes to 512 kb #6068

merged 3 commits into from
Oct 25, 2024

Conversation

scholtz
Copy link
Contributor

@scholtz scholtz commented Jul 13, 2024

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.

@gmalouf gmalouf requested a review from jannotti July 15, 2024 17:59
@jannotti
Copy link
Contributor

The issue you link to says:

Please increase post body size to the limit where any future build of the contract would rather fail on "teal code too big" instead of the input body is too big.

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 jannotti changed the title MaxTealSourceBytes to 1M #6031 API: MaxTealSourceBytes to 1M #6031 Jul 17, 2024
@scholtz
Copy link
Contributor Author

scholtz commented Jul 18, 2024

@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 npm i and npm run test

@jannotti
Copy link
Contributor

@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 npm i and npm run test

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.

@scholtz
Copy link
Contributor Author

scholtz commented Jul 18, 2024

hi @jannotti I believe the error is thrown here in algod:

tealDisassembleTest(t, largeProgram, 400, "http: request body too large", true)
when building the contract with bigger size then allowed.. Feel free to rename the text "http: request body too large" to "We allow maximum 1MB of teal code to be compiled" .. but my issue is not to get proper error text, but not to get the error when compiling.. i believe 1MB should be sustainable and usable long term for any optional extensions.. it does not effect any of other compilation limitations at the moment like opcode budget checking...

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.

@pbennett
Copy link

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.

@joe-p
Copy link
Contributor

joe-p commented Jul 31, 2024

I imagine a significant factor here is bytes pseudo-op arguments. For example, TEALScript uses bytes 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF for uint256 encoding. During assembly all of these values get reduced due to it being put in the constant block, but that doesn't help if the bottleneck is sending the TEAL to the node. For this specific scenario a solution is to optimize byte constants prior to sending to algod.

@algorandskiy
Copy link
Contributor

I guess it is safe to raise since it is developer API (normally restricted)

@jannotti
Copy link
Contributor

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:

Could you tell me how many bytes your teal compiles to? Since we are considering raising the potential size of a logicsig to 16kb, I'd like to have enough headroom to allow for program that compiles to something that big, along with a reasonable amount of comments.

@jannotti
Copy link
Contributor

jannotti commented Sep 19, 2024

I've got a contract that compiles to 6320 bytes or so (and will grow) and the teal for it is 142K.

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 8096*22.4 = 182kb source program. Since we're contemplating 16000 byte logicsigs, we might want to compile programs about 16000*22.4 = 358kb.

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.

jannotti
jannotti previously approved these changes Sep 25, 2024
Copy link
Contributor

@jannotti jannotti left a 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.

@jannotti jannotti self-assigned this Sep 25, 2024
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.12%. Comparing base (1493410) to head (8c164f3).
Report is 70 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@scholtz
Copy link
Contributor Author

scholtz commented Oct 2, 2024

i am ok with 512*1024 as well.. thanks for review

fix lint error
@cce cce changed the title API: MaxTealSourceBytes to 1M #6031 API: MaxTealSourceBytes to 1M Oct 18, 2024
@scholtz scholtz changed the title API: MaxTealSourceBytes to 1M API: MaxTealSourceBytes to 512 kb Oct 21, 2024
@scholtz
Copy link
Contributor Author

scholtz commented Oct 25, 2024

@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

@gmalouf
Copy link
Contributor

gmalouf commented Oct 25, 2024

Please note that all nodes will not support this until the next consensus upgrade goes into effect.

@gmalouf gmalouf merged commit 078aafe into algorand:master Oct 25, 2024
22 checks passed
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.

request body too large
7 participants