ws: allow filtering notification by parameters #3689

carpawell · 2024-11-18T08:49:04Z

Closes #3624.

carpawell · 2024-11-18T08:55:21Z

Some questions:

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.
Are type limitations correct?
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 · 2024-11-18T08:57:00Z

Codecov Report

Attention: Patch coverage is 60.37736% with 21 lines in your changes missing coverage. Please review.

Project coverage is 83.01%. Comparing base (ff15e39) to head (97bd73b).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/neorpc/filters.go	50.00%	13 Missing and 5 partials ⚠️
pkg/neorpc/rpcevent/filter.go	82.35%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3689      +/-   ##
==========================================
- Coverage   83.04%   83.01%   -0.04%     
==========================================
  Files         335      335              
  Lines       46719    46782      +63     
==========================================
+ Hits        38800    38834      +34     
- Misses       6327     6350      +23     
- Partials     1592     1598       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AnnaShaleva · 2024-11-18T09:31:44Z

Is there any full test for client-to-server communication to check that filtering works?

Use and extend the following tests:

neo-go/pkg/services/rpcsrv/subscription_test.go

Line 130 in 990634a

func TestSubscriptions(t *testing.T) {

neo-go/pkg/services/rpcsrv/client_test.go

Line 2131 in 990634a

func TestWSClient_SubscriptionsCompat(t *testing.T) {

That's the way how we test subscriptions. If it's not enough, then create your own test based on

neo-go/pkg/services/rpcsrv/client_test.go

Line 1895 in 990634a

func TestSubClientWait(t *testing.T) {

Are type limitations correct?

Will be answered in review.

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, #3490), then subscribe to RPC server with matching filters.

pkg/neorpc/filters.go

AnnaShaleva · 2024-11-18T09:45:13Z

pkg/neorpc/filters.go

+	if len(f.Parameters) != 0 { // todo: limit max size? what number?
+		for i, parameter := range f.Parameters {
+			if parameter.Type < smartcontract.AnyType || parameter.Type > smartcontract.SignatureType {
+				return fmt.Errorf("%w: NotificationFilter unsupported %d parameter type: %s", ErrInvalidSubscriptionFilter, i, parameter.Type)


NotificationFilter unsupported %d parameter type: %s

It's supposed to be a sentence, like other logs. Let's rephrase to NotificationFilter type parameter %d is unsupported: %s

pkg/neorpc/rpcevent/filter.go

AnnaShaleva · 2024-11-18T10:03:12Z

pkg/neorpc/rpcevent/filter.go

+					parametersOk = false
+					break
+				}
+				converted, err := p.ToStackItem()


It's too expensive to convert every filter's parameter every time you need to access it. Consider contract that emits 100500 notifications that match filter's requirements, every block. To improve it define an additional method (some (*neorpc.NotificationFilter) ParametersSI() []stackitem.Item), this method should convert filter parameters to stckitems and cache the resulting value inside filter's structure. Cache should be reused for subsequent invocations of this method. Cache should be cleaned on filter's Copy.

Also, prior to parameter's value comparison use parameter types comparison:

neo-go/pkg/smartcontract/param_type.go

Line 193 in 990634a

func (pt ParamType) Match(v stackitem.Item) bool {

Also please add a separate unit-test for various parameter types matching comparison.

not sure what is more scary: caching bugs and more complex logic or converting vars on stack, but don't mind. no mutexes since it should not be used concurrently, ping me if you don't agree

Also, prior to parameter's value comparison use parameter types comparison:

isn't stackitem.Item.Equal enough?

Also please add a separate unit-test for various parameter types matching comparison.

can, please, explain, why it is needed? there is a single smartcontract.Parameter -> stackitem.Item conversion rule and i use it (was not written by me). if it is not fixed, how does this work at all then? at least that is how i understand it

isn't stackitem.Item.Equal enough?

It's enough but type comparison allows to fail fast. Parameter to stackitem conversion is not cheap.

can, please, explain, why it is needed?

Fail fast in case of types mismatch.

Fail fast

got you

pkg/neorpc/rpcevent/filter.go

AnnaShaleva · 2024-11-18T10:20:48Z

pkg/neorpc/filters.go

@@ -134,6 +152,16 @@ func (f NotificationFilter) IsValid() error {
 	if f.Name != nil && len(*f.Name) > runtime.MaxEventNameLen {
 		return fmt.Errorf("%w: NotificationFilter name parameter must be less than %d", ErrInvalidSubscriptionFilter, runtime.MaxEventNameLen)
 	}
+	if len(f.Parameters) != 0 { // todo: limit max size? what number?
+		for i, parameter := range f.Parameters {
+			if parameter.Type < smartcontract.AnyType || parameter.Type > smartcontract.SignatureType {


The case when all parameter types are Any is also a no-op.

carpawell · 2024-11-20T17:33:23Z

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

AnnaShaleva

Tests are not yet checked, will review them after PR finalisation.

docs/notifications.md

pkg/neorpc/filters.go

AnnaShaleva · 2024-11-22T08:27:00Z

pkg/neorpc/filters.go

+	if len(f.Parameters) != 0 {
+		res.Parameters = slices.Clone(f.Parameters)
+	}
+	f.parametersCache = nil


But why? It's res.Parameters cache that should be cleaned (but it's not set anyway). So just remove f.parametersCache = nil and adjust method documentation a bit.

i tried to follow your suggestion, i believe:

Cache should be cleaned on filter's Copy.

i treat it like a new life for the struct (otherwise i do not know why somebody needs to copy something). if you copy smth, you can try to reuse the original struct and then it will be unexpected when you have copied the struct, changed f.Parameters, and then called ParametersSI with the old return values

Cache should be cleaned on filter's Copy.

This sentence was about the copy itself. Cache of the copy should be cleaned. It's the way how it works in NeoGo for cachable fields of transaction/block, we need to follow.

you can try to reuse the original struct

The original struct should be kept unchanged (including cache). The copy may be reused by the user and modified, hence it requires clean cache.

ok, i do not mind, if this approach has become established in neo-go. dropped cache resetting

pkg/neorpc/filters.go

pkg/services/rpcsrv/subscription_test.go

AnnaShaleva · 2024-11-25T09:21:14Z

@carpawell tests are failing.

AnnaShaleva

Some of previous conversations are unresolved.

AnnaShaleva · 2024-11-25T09:29:01Z

pkg/neorpc/filters.go

+	if f.parametersCache != nil {
+		return f.parametersCache, nil
+	}
+	f.parametersCache = make([]stackitem.Item, 0, len(f.Parameters))
+	for i, p := range f.Parameters {
+		si, err := p.ToStackItem()
+		if err != nil {
+			f.parametersCache = nil
+			return nil, fmt.Errorf("converting %d parameter to stack item: %w", i, err)
+		}
+		f.parametersCache = append(f.parametersCache, si)
+	}
+	return f.parametersCache, nil
+}


And BTW, usually in NeoGo it's vice-versa:

if f.parametersCache == nil { // fill parametersCache in } return f.parametersCache, nil

neo-go/pkg/core/transaction/transaction.go

Lines 331 to 334 in ff15e39

if t.size == 0 {

t.size = io.GetVarSize(t)

}

return t.size

it adds additional nesting, usually, i find it a more complex code, but ok

AnnaShaleva · 2024-11-25T09:36:09Z

docs/notifications.md

+   32 bytes and/or `parameters` field containing an ordered array of structs
+   with `type` and `value` fields. Parameter's `type` must be not-a-complex
+   type from the list: `Any`, `Boolean`, `Integer`, `ByteArray`, `String`,
+   `Hash160`, `Hash256`, `PublicKey` or `Signature`. Filter that allows any 
+   parameter must be omitted or must be `Any` typed with zero value. It is
+   prohibited to have `parameters` be filled with `Any` types only.


Please, mention 16 constraint here.

AnnaShaleva · 2024-11-25T09:37:31Z

pkg/neorpc/filters.go

+	// parameters. Notification parameter filters will be applied in the order
+	// corresponding to a produced notification's parameters. `Any`-typed
+	// parameter with zero value allows any notification parameter. Supported
+	// parameter types:


Also please mention MaxNotificationFilterParametersCount in the doc.

`Any` type with nil/null value is treated as a parameter filter that allows any notification value. Not more than 16 filter parameters are allowed. Closes #3624. Signed-off-by: Pavel Karpy <[email protected]>

Signed-off-by: Pavel Karpy <[email protected]>

carpawell · 2024-11-25T21:31:03Z

Some of previous conversations are unresolved.

Can you point me, please? As I see it: it is either fixed or I asked some questions that have not been answered or something is not relevant already.

carpawell self-assigned this Nov 18, 2024

carpawell force-pushed the feat/filter-events-by-parameters branch from d4643f2 to fd5ad7b Compare November 18, 2024 08:57

AnnaShaleva reviewed Nov 18, 2024

View reviewed changes

carpawell force-pushed the feat/filter-events-by-parameters branch 2 times, most recently from 9191c15 to b4fc362 Compare November 20, 2024 17:31

carpawell marked this pull request as ready for review November 20, 2024 17:37

carpawell requested a review from roman-khimov as a code owner November 20, 2024 17:37

carpawell requested a review from AliceInHunterland November 20, 2024 17:38

AnnaShaleva reviewed Nov 21, 2024

View reviewed changes

carpawell force-pushed the feat/filter-events-by-parameters branch from b4fc362 to a87d7cb Compare November 21, 2024 17:50

carpawell requested a review from AnnaShaleva November 21, 2024 23:12

AnnaShaleva reviewed Nov 22, 2024

View reviewed changes

carpawell force-pushed the feat/filter-events-by-parameters branch from a87d7cb to b7d0172 Compare November 22, 2024 15:45

carpawell requested a review from AnnaShaleva November 22, 2024 15:45

carpawell force-pushed the feat/filter-events-by-parameters branch from b7d0172 to 29d2a57 Compare November 22, 2024 16:00

AnnaShaleva reviewed Nov 25, 2024

View reviewed changes

carpawell force-pushed the feat/filter-events-by-parameters branch from 29d2a57 to 0919d65 Compare November 25, 2024 21:25

carpawell added 2 commits November 26, 2024 00:26

ws: allow filtering notification by parameters

0ba16fe

`Any` type with nil/null value is treated as a parameter filter that allows any notification value. Not more than 16 filter parameters are allowed. Closes #3624. Signed-off-by: Pavel Karpy <[email protected]>

rpcsrv/tests: simplify tests with parsed test contract hash

97bd73b

Signed-off-by: Pavel Karpy <[email protected]>

carpawell force-pushed the feat/filter-events-by-parameters branch from 0919d65 to 97bd73b Compare November 25, 2024 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ws: allow filtering notification by parameters #3689

ws: allow filtering notification by parameters #3689

carpawell commented Nov 18, 2024

carpawell commented Nov 18, 2024

codecov bot commented Nov 18, 2024 •

edited

Loading

AnnaShaleva commented Nov 18, 2024

AnnaShaleva Nov 18, 2024

AnnaShaleva Nov 18, 2024 •

edited

Loading

AnnaShaleva Nov 18, 2024

carpawell Nov 20, 2024 •

edited

Loading

AnnaShaleva Nov 21, 2024

carpawell Nov 21, 2024

AnnaShaleva Nov 18, 2024

carpawell commented Nov 20, 2024

AnnaShaleva left a comment

AnnaShaleva Nov 22, 2024

carpawell Nov 22, 2024

AnnaShaleva Nov 25, 2024

AnnaShaleva Nov 25, 2024

carpawell Nov 25, 2024

AnnaShaleva commented Nov 25, 2024

AnnaShaleva left a comment

AnnaShaleva Nov 25, 2024

carpawell Nov 25, 2024

AnnaShaleva Nov 25, 2024

AnnaShaleva Nov 25, 2024

carpawell commented Nov 25, 2024

ws: allow filtering notification by parameters #3689

Are you sure you want to change the base?

ws: allow filtering notification by parameters #3689

Conversation

carpawell commented Nov 18, 2024

carpawell commented Nov 18, 2024

codecov bot commented Nov 18, 2024 • edited Loading

Codecov Report

AnnaShaleva commented Nov 18, 2024

Choose a reason for hiding this comment

AnnaShaleva Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell commented Nov 20, 2024

AnnaShaleva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaShaleva commented Nov 25, 2024

AnnaShaleva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell commented Nov 25, 2024

codecov bot commented Nov 18, 2024 •

edited

Loading

AnnaShaleva Nov 18, 2024 •

edited

Loading

carpawell Nov 20, 2024 •

edited

Loading