search

nspcc-dev / neo-go

Go Node and SDK for the Neo blockchain
MIT License
123 stars 79 forks source link

ws: allow filtering notification by parameters #3689

Open carpawell opened 4 days ago

carpawell commented 4 days ago

Closes #3624.

carpawell commented 4 days ago

Some questions:

  1. Is there any full test for client-to-server communication to check that filtering works? Not just unmarshalling but also creating notifications and receiving only the important ones. Have not found it.
  2. Are type limitations correct?
  3. What limit should be used for parameter filters? Why should we have it (the issue says)? Just not to have some OOMs on a server side or it is possible to abuse a node somehow?
codecov[bot] commented 4 days ago

Codecov Report

Attention: Patch coverage is 60.86957% with 18 lines in your changes missing coverage. Please review.

Project coverage is 82.97%. Comparing base (176593b) to head (b4fc362). Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
pkg/neorpc/filters.go 48.27% 11 Missing and 4 partials :warning:
pkg/neorpc/rpcevent/filter.go 82.35% 2 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3689 +/- ## ========================================== - Coverage 83.05% 82.97% -0.08% ========================================== Files 334 335 +1 Lines 46604 46753 +149 ========================================== + Hits 38705 38793 +88 - Misses 6320 6368 +48 - Partials 1579 1592 +13 ```

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

🚨 Try these New Features:

AnnaShaleva commented 4 days ago
  1. Is there any full test for client-to-server communication to check that filtering works?

Use and extend the following tests: https://github.com/nspcc-dev/neo-go/blob/990634a43af5be0609270b2fce6b103f92686998/pkg/services/rpcsrv/subscription_test.go#L130 https://github.com/nspcc-dev/neo-go/blob/990634a43af5be0609270b2fce6b103f92686998/pkg/services/rpcsrv/client_test.go#L2131 That's the way how we test subscriptions. If it's not enough, then create your own test based on https://github.com/nspcc-dev/neo-go/blob/990634a43af5be0609270b2fce6b103f92686998/pkg/services/rpcsrv/client_test.go#L1895

  1. Are type limitations correct?

Will be answered in review.

  1. What limit should be used for parameter filters?

Let's limit the number of parameters to 16 for now, it's pretty enough for notifications used by NeoFS and at the same time it won't allow to DoS the node with useless filtering process for large notifications/filters. Also, parameter types should be limited by non-compound types (simple Integer, String, Hash160 and etc.; excluding Arrays, Structs and Maps), we don't need compounds for now and NeoFS contracts don't use them in notifications; in future the set of supported types may be extended.

Why should we have it (the issue says)?

Avoid filters misuse and unwanted load for RPC server. This extension will be available on public RPC nodes.

it is possible to abuse a node somehow?

Deploy contract that emits thousands of notifications (it's possible, hi, https://github.com/nspcc-dev/neo-go/issues/3490), then subscribe to RPC server with matching filters.

carpawell commented 1 day ago

@AnnaShaleva thanks for the review! It was kinda draft with the main questions but tried to answer and fix all the threads you left, check one more time, please.