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: limit size of the machine string #2385

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

deelawn
Copy link
Contributor

@deelawn deelawn commented Jun 18, 2024

Closes #1981

Currently, it is possible to block machine execution by submitting a transaction that results in an error which produces a massive string due to it stringing together all of the Machine's contents. This PR limits the amount of data that is stringed together. It isn't very useful anyway.

Another solution that was considered
I had experimented with some concurrent solutions to retain the full contents of the string, but while the building of the string was rendered more efficient, the printing of it was not, as expected. A possible solution to this is to write it to a file -- far more performant -- but then there is the issue of these types of logs taking up a lot of disk space if someone continues to submit transactions that produce them.

I am aware that #2145 is a WIP to improve this, but even that does not limit the amount of data printed (I will comment on that PR); it also needs some additional work before it is merged and the intention for this PR is to fix the issue ASAP to avoid this causing a problem during the Challenge Series at Gophercon US.

Here is an example of something that blocks the machine before the changes in this PR are applied. You can run it with and without the changes to compare. Run with gnokey maketx run:

package main

func main() {
	killit()
}

func killit() {
	killit()
}

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jun 18, 2024
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

Attention: Patch coverage is 16.80000% with 104 lines in your changes missing coverage. Please review.

Project coverage is 57.33%. Comparing base (5fdbce0) to head (610b30d).
Report is 117 commits behind head on master.

Files Patch % Lines
gnovm/pkg/gnolang/machine.go 16.80% 98 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2385      +/-   ##
==========================================
+ Coverage   54.63%   57.33%   +2.70%     
==========================================
  Files         581      597      +16     
  Lines       77967    84809    +6842     
==========================================
+ Hits        42596    48628    +6032     
- Misses      32192    32690     +498     
- Partials     3179     3491     +312     
Flag Coverage Δ
contribs/gnodev 25.65% <ø> (+1.84%) ⬆️
contribs/gnofaucet 14.46% <ø> (ø)
contribs/gnokeykc 0.00% <ø> (ø)
contribs/gnomd 0.00% <ø> (ø)
gno.land 64.15% <ø> (+2.28%) ⬆️
gnovm 63.96% <16.80%> (+3.89%) ⬆️
misc/autocounterd 0.00% <ø> (ø)
misc/genproto 0.00% <ø> (ø)
misc/genstd 80.54% <ø> (+6.64%) ⬆️
misc/goscan 0.00% <ø> (ø)
misc/logos 17.38% <ø> (ø)
misc/loop 0.00% <ø> (ø)
tm2 54.51% <ø> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thehowl
Copy link
Member

thehowl commented Jun 27, 2024

@deelawn can you add the txtar jeff provided?

#1736

maybe also a golden test to see an example of a machine dump.

@deelawn
Copy link
Contributor Author

deelawn commented Jun 27, 2024

@deelawn can you add the txtar jeff provided?

#1736

maybe also a golden test to see an example of a machine dump.

I made this PR primarily with the infinite recursive case in mind. The txtar in #1736 exposes an issue with printing out a massive value. As this PR does nothing to address this, I will close it. My initial idea was to get this merged quickly until #2145 is ready, but I think it will be ready soon and that one will avoid this kind of issue because it is only printing out a trace, not all the values stored in the machine.

@deelawn deelawn closed this Jun 27, 2024
@deelawn deelawn reopened this Jul 24, 2024
@deelawn deelawn marked this pull request as draft July 24, 2024 17:08
@deelawn deelawn mentioned this pull request Aug 1, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: In Progress
Status: No status
Development

Successfully merging this pull request may close these issues.

Machine.String() hangs the VM on error when printing large structure
2 participants