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

Improve session mechanism #3429

Open
roman-khimov opened this issue Jul 18, 2024 · 3 comments
Open

Improve session mechanism #3429

roman-khimov opened this issue Jul 18, 2024 · 3 comments
Labels
discussion Initial issue state - proposed but not yet accepted
Milestone

Comments

@roman-khimov
Copy link
Contributor

Summary or problem description
We've got a session mechanism to expand iterators for RPC invocations since neo-project/neo-modules#715 (which was developed to fix #2699, the main problem there was MaxIteratorResultItems limitation). It requires some state to be saved server-side which can be leveraged for DoS attacks, so an option was added to disable them for public servers (see neo-project/neo-modules#715 (comment) and neo-project/neo-modules#715 (comment)). Since then C# node always returned SID/IID combination that can be used for traversing with sessions enabled and IID only that can't be used in any way with sessions disabled.

Back then sessions were disabled on all public C# nodes, so there were no security issues, but at the same time these nodes were useless for test invocations that returned iterator results (IID couldn't be traversed without a session). Then at some point this setting was flipped on some nodes which solved the data availability problem, but returned us to the security issue again, it's dangerous to have sessions enabled on public nodes.

NeoGo has also implemented session support in version 0.99.1, but to avoid the problem of missing data with disabled sessions we've kept the old behavior of iterator expansion with MaxIteratorResultItems limit. When sessions are enabled in NeoGo it behaves identically to the C# node, when they're disabled iterators are expanded and results are returned directly (which is documented in https://github.com/nspcc-dev/neo-go/blob/master/docs/rpc.md#invokefunction-invokescript).

While it's rather clear that we can improve C# node by providing the same behavior for "sessions disabled" case as NeoGo, I would also like to point to another related issue: nspcc-dev/neo-go#3272. This one is about session mechanism (in)efficiency, even when sessions are enabled and data is fully available they can be less efficient than the pre-sessions behavior for iterators that don't have a lot of values (which is the vast majority of cases). This particular problem was workarounded by a special invocation script (just like we've provided CreateCallAndUnwrapIteratorScript() previously to overcome data availability problem for C# nodes with sessions disabled), but we can do better than that.

Do you have any solution you want to propose?
Combine both ways to represent iterator in invocation result. Always do iterator expansion for up to MaxIteratorResultItems (which should be equal to VM stack limit, btw, I can always make a script that will return this many values irrespective of MaxIteratorResultItems). Then:

  • if we have no more results --- we're done
  • if we have additional items left and sessions are disabled --- mark the result as truncated (pre-sessions behavior)
  • if we have additional items left and sessions are enabled --- provide IID/SID in the result to fetch the rest of them

This way if sessions are disabled it'd be possible to easily get the result for most use cases. If sessions are enabled it'd be more efficient (providing all or the first batch of results immediately). No fancy invocation scripts needed in any of the cases.

Where in the software does this update applies to?

  • RPC (HTTP)
@roman-khimov roman-khimov added the discussion Initial issue state - proposed but not yet accepted label Jul 18, 2024
@roman-khimov roman-khimov added this to the v3.8.0 milestone Jul 18, 2024
@roman-khimov
Copy link
Contributor Author

CC @fyfyrchik.

@fyfyrchik
Copy link

which should be equal to VM stack limit, btw

VM stack limit applies to the total number of items on stack, including nested ones. What about the case when we are iterating over structs with multiple fields? I think this should be possible with FindDeserialize flag for storage.Find.

Another restriction we have (at least in neo-go, I believe) is the maximum size of the JSON we can return. Difficult to take into account during iteration, but may be we can have some pessimistic estimations.

For both cases it may be worth to allow user to lower the number of items it can receive in one query? So the MaxResultIteratorItems = vm.MaxStackSize is the default, but I override it with vm.MaxStackSize / 10 if I know what values I am iterating over.

@roman-khimov
Copy link
Contributor Author

Well, we can have an old ("safe") MaxResultIteratorItems default which is 100. It's not critical, the general approach outlined above is more important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

No branches or pull requests

2 participants