engine: create a max procedure call stack depth #609
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jchappelow
commented
Mar 18, 2024
| values: make(map[string]any), | ||
| DBID: p.DBID, | ||
| Procedure: p.Procedure, | ||
| StackDepth: p.StackDepth, |
There was a problem hiding this comment.
Initially I had this with a +1 from the parent procedure's context, but (*baseDataset).Call also does NewScope so it can get incremented twice unintentionally.
jchappelow
commented
Mar 18, 2024
| // Call calls a procedure from the dataset. | ||
| // If the procedure is not public, it will return an error. | ||
| // It implements the Namespace interface. | ||
| // It satisfies precompiles.Instance. |
There was a problem hiding this comment.
I think these interfaces have been renamed a few times. This seems to be the one it should mention.
jchappelow
commented
Mar 18, 2024
| // abusive recursive calls, but a hard upper limit will likely be necessary | ||
| // unless the price of an action call is extremely expensive or rises | ||
| // exponentially at each level of the call stack. | ||
| const MaxStackDepth = 1000 |
There was a problem hiding this comment.
We can do up to 10,000, but that sort of txn requires about 0.1sec to exec even with no actual queries and that is too much baseline IMO.
We sorta do need to spend each time an action is entered though, to make gas reflect DB utilization, but we can't just hard crash either.
There was a problem hiding this comment.
I think 1000 is certainly more than enough
brennanjl
approved these changes
Mar 19, 2024
jchappelow
added a commit
that referenced
this pull request
Mar 22, 2024
jchappelow
added a commit
that referenced
this pull request
Mar 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This resolves a bug with the handling of deeply nested action calls. Ultimately the fix may be a gas cost for each action call, possibly exponentially increasing with depth, but I think a hard limit is still a good idea when user-defined recursion is a possibility. See code comments for more.
I don't think we want to scheme authors to write recursive algorithms, but if we change our mind I think we can life this restriction.