search

status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.88k stars 984 forks source link

Allow clients to blocklist signals from status-go #20791

Open ilmotta opened 1 month ago

ilmotta commented 1 month ago

Problem

There are signals the mobile client doesn't process or care about. Whenever a signal arrives, we dispatch it using re-frame and parse the signal with JSON.parse. Both operations add considerable overhead. JSON.parse is not cheap when everything else is busy in a single-threaded environment. Based on microbenchmarks from real signals we ignore, their cost is not negligible.

For an example, in the first few seconds of the app after login, it's common to dispatch and parse almost 40 signals that we never cared about. And many wallet events keep being emitted from status-go after login.

Another problem is that some signals sometimes are dispatched in bursts, like wakuv2.peerstats, and some arrive too close to each other, which potentially contributes to UI stuttering.

Implementation

The somewhat simple solution is to allow the client to tell status-go which signals and/or signals + events should be ignored.

This branch in status-go works well for the simple case where a signal can be detected solely by its type, like the history.request.started signal https://github.com/status-im/status-go/blob/b8a33a8d7b05b3bc9f714ff7dd4d51570b848ce8/signal/signals.go#L71-L72. But we need a different solution for the important case of wallet events, that all go in the bucket of the wallet signal. Because we can only detect the type of an event if we do a type switch in Go, but that requires importing, for example, the walletevent package inside signals.go, which creates a circular dependency and thus fail compilation.

A solution that would work is to change status-go to publish a signal as JSON, but add a globally unique identifier with a fixed length at the beginning, for example like a vector ["<ID>", <signal payload>]. That would allow the client to reliably detect the signal and event type without having to parse first as JSON.

As this is a breaking change, the solution has to be adapted in status-desktop too.

Acceptance Criteria

Signals that are not processed by the mobile client should be blocklisted. The full list as of 2024-07-17 is listed below:

ilmotta commented 1 month ago

@Samyoul @cammellos @J-Son89 @jrainville It's not our intention to solve this issue for 2.30.

qfrank commented 3 weeks ago

my two cents, can't we do the opposite like allowlist rather than blocklist? in this way, we can see clearly in client that what signals client are interested in, and if there is new signal added in status-go by desktop/mobile team, it won't be sent by default

ilmotta commented 3 weeks ago

my two cents, can't we do the opposite like allowlist rather than blocklist? in this way, we can see clearly in client that what signals client are interested in, and if there is new signal added in status-go by desktop/mobile team, it won't be sent by default

@qfrank, thanks for chiming in :) Allowlists can be better, but if we process 90% of the signals, but ignore only 10% (roughly our case), using an allowlist will have a higher cost of maintenance because we will manually need to keep track of signals we handle in the event handlers in :signals/signal-received and :wallet/signal-received event-js.

But what's more important, what will happen is that we will bump status-go version, but the newest status-go will publish more/different signals and we will silently ignore them, thus leading to weird bugs that will probably take a good amount of time for the dev to realize. All of this in the name of a small perf boost, so at least to me it's not worth the trouble. We could circumvent this problem by logging ignored signals, but still risky to introduce silent bugs in production.