gnolang / gno

Gno: An interpreted, stack-based Go virtual machine to build succinct and composable apps + Gno.land: a blockchain for timeless code and fair open-source
https://gno.land/
Other
841 stars 342 forks source link

feat: limit size of the machine string #2385

Closed deelawn closed 1 day ago

deelawn commented 1 week ago

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()
}
codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 0% with 101 lines in your changes missing coverage. Please review.

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

Files Patch % Lines
gnovm/pkg/gnolang/machine.go 0.00% 101 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2385 +/- ## ========================================== - Coverage 54.63% 54.59% -0.04% ========================================== Files 581 581 Lines 77967 78023 +56 ========================================== Hits 42596 42596 - Misses 32192 32248 +56 Partials 3179 3179 ``` | [Flag](https://app.codecov.io/gh/gnolang/gno/pull/2385/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang) | Coverage Δ | | |---|---|---| | [gnovm](https://app.codecov.io/gh/gnolang/gno/pull/2385/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang) | `59.92% <0.00%> (-0.15%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

thehowl commented 1 day ago

@deelawn can you add the txtar jeff provided?

https://github.com/gnolang/gno/pull/1736

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

deelawn commented 1 day ago

@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.