Set the default log level to error instead of disabling it for the release build.

[qfrank]

see relate comment for details

status: ready.

[qfrank]

I think we don't need manual QA for this simple change? @ilmotta @churik

[status-im-auto]

Jenkins Builds

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	63226f7	#1	2024-10-31 07:18:03	~5 min	tests	📄log
✔️	63226f7	#1	2024-10-31 07:19:30	~6 min	android-e2e	🤖apk 📲
✔️	63226f7	#1	2024-10-31 07:21:34	~8 min	android	🤖apk 📲
✔️	63226f7	#1	2024-10-31 07:22:17	~9 min	ios	📱ipa 📲

[flexsurfer]

before doing this , we need to make sure we don't leak any user data

[qfrank]

before doing this , we need to make sure we don't leak any user data

That seems like a hard job. Probably need to go through our whole code base? Or do you have any suggestions? @flexsurfer

[flexsurfer]

That seems like a hard job

yes it is, i don't see any other options :(

[jakubgs]

Indeed, we have to be quite careful about what we put in log at each step, and if we are unsure of what is in the error logs currently then we need to audit that.

Or we need a layer on top of the logger that obscures potentially sensitive info based on regex/other rules.

[ilmotta]

While I of course do agree we should double-check or even audit we are not leaking private data in error logs, the logs are going to be stored locally on device (although not encrypted at rest) and we should not ask users for their logs (agreement with legal team).

If the user do want to share logs is because they assume the risk, but we don't ask directly and we don't endorse this idea. This is similar to almost all logs written in our own devices, we shouldn't share them with anybody.

Even if we audit the entire code base before every release (completely unrealistic) we will never be able to say for sure logs are safe to share. Another point is that users can change the log level, in which case it's guaranteed that we will leak private data (like an account address), hence asking users to share is always a dangerous business.

I found ~400 references to error.Log in status-go, so it's reasonable to give a good pass in the code. I didn't check the client code (there are 165 references to log/error, most only print whatever is returned by status-go).

These are suspicious or bad practices to log at the error level:

• https://github.com/status-im/status-go/blob/635f19e3c7bc76dfb2cbf780eb7975b9fed1a024/mobile/status.go#L1683
• https://github.com/status-im/status-go/blob/635f19e3c7bc76dfb2cbf780eb7975b9fed1a024/mobile/status.go#L1688
• Logs address (there are many instances of this problem, but it's easy to fix) https://github.com/status-im/status-go/blob/635f19e3c7bc76dfb2cbf780eb7975b9fed1a024/services/web3provider/api.go#L251

This one concerns me more:

• At the error level, we log the stack trace on panics https://github.com/status-im/status-go/blob/635f19e3c7bc76dfb2cbf780eb7975b9fed1a024/mobile/status_request_log.go#L68. To play safe, the stack could be logged only at the debug level.

---

It might be too much to check for 2.31 @qfrank, in which case feel free to park this issue.

In practice, what problem are we trying to solve? To me, the main reason to change the default log level is to help us diagnose bugs that happen during the onboarding (where the default log level is disabled) because after login, we (and users) are able to change the log level in Advanced Settings, so there's no need to change the default log level. That's why I suggested in another issue that the onboarding logs should be segregated from other logs, separate file, separate level, nothing was decrypted, wallet signals are not running, etc so many differences (more details here #21501 (comment)).

And changing the default log level would be mostly useful to us and QAs, when we are able to replicate bugs during onboarding in release builds. It's not really about asking users to share logs, although a user who's locked out could consider that (true, sad story).

[qfrank]

This one concerns me more

When I saw this, I just want to die ...

because after login, we (and users) are able to change the log level in Advanced Settings, so there's no need to change the default log level

But random issue is not friendly.

Let's park this issue ATM .. Thank you all for your input! ❤️

[ilmotta]

When I saw this, I just want to die ...

In which way @qfrank? 😅 I think capturing and logging panics are very useful and you did a very important work related to that. When I reviewed, I missed the fact that this is debug data, therefore shouldn't use the error level. Even the code uses string(debug.Stack()), a good indicator that we shouldn't log at the error level ;)

Let's park this issue ATM .. Thank you all for your input!

We will come back to this problem I'm sure. Thanks @flexsurfer @jakubgs for raising the red flag.