search

neo-project / neo

NEO Smart Economy
MIT License
3.46k stars 1.03k forks source link

Improve session mechanism #3429

Open roman-khimov opened 1 month ago

roman-khimov commented 1 month ago

Summary or problem description We've got a session mechanism to expand iterators for RPC invocations since https://github.com/neo-project/neo-modules/pull/715 (which was developed to fix https://github.com/neo-project/neo/issues/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 https://github.com/neo-project/neo-modules/pull/715#discussion_r897641254 and https://github.com/neo-project/neo-modules/pull/715#discussion_r886406397). 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: https://github.com/nspcc-dev/neo-go/issues/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:

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?

roman-khimov commented 1 month ago

CC @fyfyrchik.

fyfyrchik commented 1 month ago

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 commented 1 month ago

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