Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore_: bump go-waku fixes on peers and concurrent access #6187

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

@status-im-auto
Copy link
Member

status-im-auto commented Dec 10, 2024

Jenkins Builds

Click to see older builds (16)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 6b30855 #1 2024-12-10 13:00:36 ~4 min windows 📦zip
✔️ 6b30855 #1 2024-12-10 13:00:59 ~4 min linux 📦zip
✔️ 6b30855 #1 2024-12-10 13:01:57 ~5 min ios 📦zip
✔️ 6b30855 #1 2024-12-10 13:02:11 ~6 min android 📦aar
✔️ 6b30855 #1 2024-12-10 13:03:05 ~6 min tests-rpc 📄log
✔️ 6b30855 #1 2024-12-10 13:03:39 ~7 min macos 📦zip
✔️ 6b30855 #1 2024-12-10 13:05:45 ~9 min macos 📦zip
✔️ 6b30855 #1 2024-12-10 13:27:17 ~31 min tests 📄log
✔️ 6b30855 #3 2024-12-12 11:52:19 ~2 min windows 📦zip
✔️ 6b30855 #3 2024-12-12 11:53:41 ~4 min macos 📦zip
✔️ 6b30855 #3 2024-12-12 11:53:44 ~4 min linux 📦zip
✔️ 6b30855 #3 2024-12-12 11:53:50 ~4 min ios 📦zip
✔️ 6b30855 #3 2024-12-12 11:55:08 ~5 min android 📦aar
✖️ 6b30855 #3 2024-12-12 11:55:18 ~5 min tests-rpc 📄log
✔️ 6b30855 #3 2024-12-12 11:59:36 ~8 min macos 📦zip
✔️ 6b30855 #3 2024-12-12 12:18:23 ~29 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ ce5d409 #2 2024-12-12 11:42:34 ~1 min tests 📄log
✔️ ce5d409 #2 2024-12-12 11:43:55 ~3 min windows 📦zip
✔️ ce5d409 #2 2024-12-12 11:45:00 ~4 min macos 📦zip
✔️ ce5d409 #2 2024-12-12 11:45:05 ~4 min linux 📦zip
✔️ ce5d409 #2 2024-12-12 11:45:12 ~4 min ios 📦zip
✔️ ce5d409 #2 2024-12-12 11:46:09 ~5 min android 📦aar
✖️ ce5d409 #2 2024-12-12 11:46:30 ~5 min tests-rpc 📄log
✔️ ce5d409 #2 2024-12-12 11:50:39 ~10 min macos 📦zip
✔️ 49c48ae #4 2024-12-12 11:56:39 ~2 min windows 📦zip
✔️ 49c48ae #4 2024-12-12 11:58:16 ~4 min macos 📦zip
✔️ 49c48ae #4 2024-12-12 11:58:20 ~4 min linux 📦zip
✔️ 49c48ae #4 2024-12-12 11:58:39 ~4 min ios 📦zip
✔️ 49c48ae #4 2024-12-12 12:00:50 ~5 min android 📦aar
✖️ 49c48ae #4 2024-12-12 12:01:19 ~5 min tests-rpc 📄log
✔️ 49c48ae #4 2024-12-12 12:06:39 ~6 min macos 📦zip
✖️ 49c48ae #4 2024-12-12 12:20:00 ~1 min tests 📄log
✖️ 49c48ae #5 2024-12-12 14:59:01 ~1 min tests 📄log
✔️ 49c48ae #5 2024-12-12 14:59:59 ~2 min windows 📦zip
✖️ 49c48ae #6 2024-12-12 15:01:13 ~1 min tests 📄log
✔️ 49c48ae #5 2024-12-12 15:01:34 ~4 min macos 📦zip
✔️ 49c48ae #5 2024-12-12 15:01:39 ~4 min linux 📦zip
✔️ 49c48ae #5 2024-12-12 15:02:31 ~5 min android 📦aar
✔️ 49c48ae #6 2024-12-12 15:03:01 ~2 min windows 📦zip
✖️ 49c48ae #5 2024-12-12 15:03:04 ~5 min tests-rpc 📄log
✔️ 49c48ae #5 2024-12-12 15:03:13 ~6 min ios 📦zip
✔️ 49c48ae #5 2024-12-12 15:04:59 ~7 min macos 📦zip
✔️ 49c48ae #6 2024-12-12 15:05:59 ~4 min macos 📦zip
✔️ 49c48ae #6 2024-12-12 15:06:36 ~4 min linux 📦zip
✖️ 49c48ae #6 2024-12-12 15:07:20 ~4 min tests-rpc 📄log
✔️ 49c48ae #6 2024-12-12 15:07:48 ~5 min android 📦aar
✔️ 49c48ae #6 2024-12-12 15:08:00 ~4 min ios 📦zip
✔️ 49c48ae #6 2024-12-12 15:12:59 ~7 min macos 📦zip

@plopezlpz
Copy link
Contributor Author

[blocked]: cannot merge up until this is merged:
#5964

@plopezlpz plopezlpz marked this pull request as draft December 10, 2024 13:27
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 14.00%. Comparing base (e6c2f89) to head (49c48ae).
Report is 3 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6187      +/-   ##
===========================================
+ Coverage    13.89%   14.00%   +0.10%     
===========================================
  Files          817      817              
  Lines       108311   108311              
===========================================
+ Hits         15055    15164     +109     
+ Misses       91333    91218     -115     
- Partials      1923     1929       +6     
Flag Coverage Δ
functional 14.00% <ø> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 12 files with indirect coverage changes

Copy link
Member

@richard-ramos richard-ramos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Do you know if waku-org/go-waku#1244 been tested?
EDIT: just saw the comment above!

@richard-ramos
Copy link
Member

Assuming we can't get waku-org/go-waku#1244 tested & merged, we could try to create a branch in go-waku for 0.32 and only include the changes from waku-org/go-waku#1265
IMO we should include this change as a concurrent map write will likely end up with the app crashing and the fix is very straightfwd. Wdyt, @chaitanyaprem

cc: @churik @anastasiyaig

@anastasiyaig
Copy link
Contributor

@richard-ramos can the issues mentioned in this PR cause crashes?

@richard-ramos
Copy link
Member

richard-ramos commented Dec 11, 2024

@anastasiyaig: can the issues mentioned in this PR cause crashes?

This one can: waku-org/go-waku#1265 although it seems to happen rarely

@richard-ramos
Copy link
Member

This one can: waku-org/go-waku#1265 although it seems to happen rarely

@anastasiyaig @churik
Reason why I say that it's rare for this error to happen is because this issue that was introduced 2 months ago in status-go develop branch as it can be seen in this commit yet I have not seen this crash being reported before.

@anastasiyaig
Copy link
Contributor

thats why i ask, i did not face with this issue at all recently :)

@churik
Copy link
Member

churik commented Dec 12, 2024

So we have status-im/status-mobile#21799 (comment) based on this comment I suppose it might be the solution to this problem, right?

@richard-ramos can you please clarify the regression area for these fixes, what might be affected?

Based on description, I'd say app state on network conditions (online/offline/cellular) + fetching messages should work the same.

@richard-ramos
Copy link
Member

@churik: can you please clarify the regression area for these fixes, what might be affected?

@churik
Copy link
Member

churik commented Dec 12, 2024

@richard-ramos can we rebase it to release/7.1.x and update it in mobile PR?
explanation is in status-im/status-mobile#21809 (comment)

@richard-ramos
Copy link
Member

cc-ing: @plopezlpz as he is the owner of this PR 🚀

@plopezlpz plopezlpz marked this pull request as ready for review December 12, 2024 14:56
@plopezlpz plopezlpz changed the base branch from develop to release/7.1.x December 12, 2024 14:56
@plopezlpz plopezlpz changed the base branch from release/7.1.x to develop December 12, 2024 14:59
@ilmotta
Copy link
Contributor

ilmotta commented Dec 12, 2024

@plopezlpz could you force a commit hash change in this PR while also changing it to point to the release branch release/7.1.x? The script on mobile that grabs the status-go source zip is still pointing to the same hash even when the base branch changed in this PR. We just want to be sure the new mobile build actually uses the new code introduced from the change of base. Thank you!

Nevermind @plopezlpz I created a duplicate PR for mobile to experiment.

@plopezlpz
Copy link
Contributor Author

@ilmotta @churik @richard-ramos
I've created this one: #6209
It includes only the two fixes for concurrent map access panic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants